From c1729c3269c4e3c1aa58d98fba4c488c04e8aa2e Mon Sep 17 00:00:00 2001 From: Matt Toohey Date: Tue, 7 Jul 2026 12:05:37 +1000 Subject: [PATCH 01/34] feat: persist ACP config selection Signed-off-by: Matt Toohey --- .../src-tauri/src/store/migration_tests.rs | 7 +- .../up.sql | 2 + apps/staged/src-tauri/src/store/models.rs | 30 ++++++++ apps/staged/src-tauri/src/store/sessions.rs | 48 ++++++++++-- apps/staged/src-tauri/src/store/tests.rs | 74 +++++++++++++++++++ apps/staged/src/lib/types.ts | 13 ++++ 6 files changed, 166 insertions(+), 8 deletions(-) create mode 100644 apps/staged/src-tauri/src/store/migrations/0020-add-session-acp-config-selection/up.sql diff --git a/apps/staged/src-tauri/src/store/migration_tests.rs b/apps/staged/src-tauri/src/store/migration_tests.rs index 79c8508e8..540d55cf2 100644 --- a/apps/staged/src-tauri/src/store/migration_tests.rs +++ b/apps/staged/src-tauri/src/store/migration_tests.rs @@ -145,7 +145,7 @@ fn test_store_bootstraps_fresh_database_with_baseline_migration() { ) .unwrap(); - assert_eq!(version, 19); + assert_eq!(version, 20); assert_eq!(app_version, super::APP_VERSION); assert!(table_exists(&conn, "projects")); assert!(table_exists(&conn, "project_notes")); @@ -158,6 +158,7 @@ fn test_store_bootstraps_fresh_database_with_baseline_migration() { "session_messages", "acp_agent_capabilities" )); + assert!(column_exists(&conn, "sessions", "acp_config_selection")); let trigger_count: i64 = conn .query_row( @@ -221,8 +222,9 @@ fn test_store_repairs_github_comment_tracking_user_version() { let version: i64 = conn .query_row("PRAGMA user_version", [], |row| row.get(0)) .unwrap(); - assert_eq!(version, 19); + assert_eq!(version, 20); assert!(column_exists(&conn, "sessions", "pipeline")); + assert!(column_exists(&conn, "sessions", "acp_config_selection")); assert!(column_exists( &conn, "session_messages", @@ -288,6 +290,7 @@ fn test_store_repairs_pipeline_user_version() { "acp_agent_capabilities" )); assert!(table_exists(&conn, "queued_session_messages")); + assert!(column_exists(&conn, "sessions", "acp_config_selection")); cleanup_db(&path); } diff --git a/apps/staged/src-tauri/src/store/migrations/0020-add-session-acp-config-selection/up.sql b/apps/staged/src-tauri/src/store/migrations/0020-add-session-acp-config-selection/up.sql new file mode 100644 index 000000000..3409f8d89 --- /dev/null +++ b/apps/staged/src-tauri/src/store/migrations/0020-add-session-acp-config-selection/up.sql @@ -0,0 +1,2 @@ +-- Persist selected ACP config values for a session as category-keyed JSON. +ALTER TABLE sessions ADD COLUMN acp_config_selection TEXT DEFAULT NULL; diff --git a/apps/staged/src-tauri/src/store/models.rs b/apps/staged/src-tauri/src/store/models.rs index b18c8acf9..64cabdd50 100644 --- a/apps/staged/src-tauri/src/store/models.rs +++ b/apps/staged/src-tauri/src/store/models.rs @@ -524,6 +524,9 @@ pub struct Session { /// command pipeline (deterministic steps before/instead of AI). #[serde(skip_serializing_if = "Option::is_none")] pub pipeline: Option, + /// Selected ACP config values to apply before prompting the agent. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub acp_config_selection: Option, } /// Persistent follow-up message waiting to be sent to an existing session. @@ -612,6 +615,7 @@ impl Session { updated_at: now, owner_pid: Some(std::process::id()), pipeline: None, + acp_config_selection: None, } } @@ -633,6 +637,7 @@ impl Session { updated_at: now, owner_pid: None, pipeline: None, + acp_config_selection: None, } } @@ -645,6 +650,31 @@ impl Session { self.agent_id = Some(agent_id.to_string()); self } + + pub fn with_acp_config_selection(mut self, selection: AcpConfigSelection) -> Self { + self.acp_config_selection = Some(selection); + self + } +} + +/// Session-level ACP config selections keyed by product-facing category. +#[derive(Debug, Clone, Default, PartialEq, Eq, Serialize, Deserialize)] +#[serde(rename_all = "camelCase")] +pub struct AcpConfigSelection { + #[serde(default, skip_serializing_if = "Option::is_none")] + pub model: Option, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub effort: Option, +} + +/// Selected value for one ACP `session/set_config_option` config ID. +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +#[serde(rename_all = "camelCase")] +pub struct AcpConfigValueSelection { + pub config_id: String, + pub value_id: String, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub label: Option, } // ============================================================================= diff --git a/apps/staged/src-tauri/src/store/sessions.rs b/apps/staged/src-tauri/src/store/sessions.rs index e8924039c..835402f7c 100644 --- a/apps/staged/src-tauri/src/store/sessions.rs +++ b/apps/staged/src-tauri/src/store/sessions.rs @@ -2,7 +2,9 @@ use rusqlite::{params, OptionalExtension}; -use super::models::{CompletionReason, PipelineExecution, Session, SessionStatus}; +use super::models::{ + AcpConfigSelection, CompletionReason, PipelineExecution, Session, SessionStatus, +}; use super::{now_timestamp, Store, StoreError}; impl Store { @@ -14,9 +16,11 @@ impl Store { .map(serde_json::to_string) .transpose() .map_err(|e| StoreError(format!("Failed to serialize pipeline: {e}")))?; + let acp_config_selection_json = + serialize_acp_config_selection(session.acp_config_selection.as_ref())?; conn.execute( - "INSERT INTO sessions (id, prompt, status, working_dir, provider, agent_id, error_message, completion_reason, created_at, updated_at, owner_pid, pipeline) - VALUES (?1, ?2, ?3, ?4, ?5, ?6, ?7, ?8, ?9, ?10, ?11, ?12)", + "INSERT INTO sessions (id, prompt, status, working_dir, provider, agent_id, error_message, completion_reason, created_at, updated_at, owner_pid, pipeline, acp_config_selection) + VALUES (?1, ?2, ?3, ?4, ?5, ?6, ?7, ?8, ?9, ?10, ?11, ?12, ?13)", params![ session.id, session.prompt, @@ -30,6 +34,7 @@ impl Store { session.updated_at, session.owner_pid, pipeline_json, + acp_config_selection_json, ], )?; Ok(()) @@ -38,7 +43,7 @@ impl Store { pub fn get_session(&self, id: &str) -> Result, StoreError> { let conn = self.conn.lock().unwrap(); conn.query_row( - "SELECT id, prompt, status, working_dir, provider, agent_id, error_message, completion_reason, created_at, updated_at, owner_pid, pipeline + "SELECT id, prompt, status, working_dir, provider, agent_id, error_message, completion_reason, created_at, updated_at, owner_pid, pipeline, acp_config_selection FROM sessions WHERE id = ?1", params![id], Self::row_to_session, @@ -212,7 +217,7 @@ impl Store { pub fn get_running_sessions(&self) -> Result, StoreError> { let conn = self.conn.lock().unwrap(); let mut stmt = conn.prepare( - "SELECT id, prompt, status, working_dir, provider, agent_id, error_message, completion_reason, created_at, updated_at, owner_pid, pipeline + "SELECT id, prompt, status, working_dir, provider, agent_id, error_message, completion_reason, created_at, updated_at, owner_pid, pipeline, acp_config_selection FROM sessions WHERE status = 'running'", )?; let sessions = stmt @@ -285,7 +290,7 @@ impl Store { ) -> Result, StoreError> { let conn = self.conn.lock().unwrap(); let mut stmt = conn.prepare( - "SELECT s.id, s.prompt, s.status, s.working_dir, s.provider, s.agent_id, s.error_message, s.completion_reason, s.created_at, s.updated_at, s.owner_pid, s.pipeline + "SELECT s.id, s.prompt, s.status, s.working_dir, s.provider, s.agent_id, s.error_message, s.completion_reason, s.created_at, s.updated_at, s.owner_pid, s.pipeline, s.acp_config_selection FROM sessions s WHERE s.status = 'queued' AND ( @@ -391,6 +396,21 @@ impl Store { Ok(()) } + /// Update the selected ACP config values for a session. + pub fn set_session_acp_config_selection( + &self, + id: &str, + selection: Option<&AcpConfigSelection>, + ) -> Result<(), StoreError> { + let conn = self.conn.lock().unwrap(); + let json = serialize_acp_config_selection(selection)?; + conn.execute( + "UPDATE sessions SET acp_config_selection = ?1, updated_at = ?2 WHERE id = ?3", + params![json, now_timestamp(), id], + )?; + Ok(()) + } + /// Update the pipeline execution state for a session. pub fn update_session_pipeline( &self, @@ -411,11 +431,17 @@ impl Store { let status_str: String = row.get(2)?; let reason_str: Option = row.get(7)?; let pipeline_json: Option = row.get(11)?; + let acp_config_selection_json: Option = row.get(12)?; let pipeline = pipeline_json.as_deref().and_then(|s| { serde_json::from_str(s) .map_err(|e| log::warn!("Failed to deserialize pipeline JSON: {e}")) .ok() }); + let acp_config_selection = acp_config_selection_json.as_deref().and_then(|s| { + serde_json::from_str(s) + .map_err(|e| log::warn!("Failed to deserialize ACP config selection JSON: {e}")) + .ok() + }); Ok(Session { id: row.get(0)?, prompt: row.get(1)?, @@ -429,6 +455,16 @@ impl Store { updated_at: row.get(9)?, owner_pid: row.get(10)?, pipeline, + acp_config_selection, }) } } + +fn serialize_acp_config_selection( + selection: Option<&AcpConfigSelection>, +) -> Result, StoreError> { + selection + .map(serde_json::to_string) + .transpose() + .map_err(|e| StoreError(format!("Failed to serialize ACP config selection: {e}"))) +} diff --git a/apps/staged/src-tauri/src/store/tests.rs b/apps/staged/src-tauri/src/store/tests.rs index c57da689c..bbeef4194 100644 --- a/apps/staged/src-tauri/src/store/tests.rs +++ b/apps/staged/src-tauri/src/store/tests.rs @@ -555,6 +555,80 @@ fn test_queued_session_message_claims_pending_images() { assert_eq!(claimed.session_id.as_deref(), Some(session.id.as_str())); } +#[test] +fn test_session_acp_config_selection_round_trips() { + let store = Store::in_memory().unwrap(); + let selection = AcpConfigSelection { + model: Some(AcpConfigValueSelection { + config_id: "model".to_string(), + value_id: "gpt-5".to_string(), + label: Some("GPT-5".to_string()), + }), + effort: Some(AcpConfigValueSelection { + config_id: "reasoning_effort".to_string(), + value_id: "high".to_string(), + label: None, + }), + }; + + let session = Session::new_running("configured", Path::new("/tmp")) + .with_acp_config_selection(selection.clone()); + store.create_session(&session).unwrap(); + + let fetched = store.get_session(&session.id).unwrap().unwrap(); + assert_eq!(fetched.acp_config_selection, Some(selection.clone())); + + let replacement = AcpConfigSelection { + model: Some(AcpConfigValueSelection { + config_id: "model".to_string(), + value_id: "gpt-5-mini".to_string(), + label: Some("GPT-5 mini".to_string()), + }), + effort: None, + }; + store + .set_session_acp_config_selection(&session.id, Some(&replacement)) + .unwrap(); + let updated = store.get_session(&session.id).unwrap().unwrap(); + assert_eq!(updated.acp_config_selection, Some(replacement)); + + store + .set_session_acp_config_selection(&session.id, None) + .unwrap(); + let cleared = store.get_session(&session.id).unwrap().unwrap(); + assert_eq!(cleared.acp_config_selection, None); +} + +#[test] +fn test_queued_session_acp_config_selection_round_trips() { + let store = Store::in_memory().unwrap(); + let project = Project::new("test-owner/test-repo"); + store.create_project(&project).unwrap(); + let branch = Branch::new(&project.id, "feature", "main"); + store.create_branch(&branch).unwrap(); + + let selection = AcpConfigSelection { + model: Some(AcpConfigValueSelection { + config_id: "model".to_string(), + value_id: "gpt-5".to_string(), + label: Some("GPT-5".to_string()), + }), + effort: Some(AcpConfigValueSelection { + config_id: "thought_level".to_string(), + value_id: "medium".to_string(), + label: Some("Medium".to_string()), + }), + }; + let session = Session::new_queued("queued").with_acp_config_selection(selection.clone()); + store.create_session(&session).unwrap(); + let note = Note::new(&branch.id, "queued", "").with_session(&session.id); + store.create_note(¬e).unwrap(); + + let queued = store.get_queued_sessions_for_branch(&branch.id).unwrap(); + assert_eq!(queued.len(), 1); + assert_eq!(queued[0].acp_config_selection, Some(selection)); +} + #[test] fn test_delete_queued_session_message_releases_claimed_images() { let store = Store::in_memory().unwrap(); diff --git a/apps/staged/src/lib/types.ts b/apps/staged/src/lib/types.ts index 4b0b35463..03d61c74b 100644 --- a/apps/staged/src/lib/types.ts +++ b/apps/staged/src/lib/types.ts @@ -356,6 +356,17 @@ export function isResumableReason(reason: string | null | undefined): boolean { return !!reason && RESUMABLE_REASONS.has(reason as CompletionReason); } +export interface AcpConfigValueSelection { + configId: string; + valueId: string; + label?: string | null; +} + +export interface AcpConfigSelection { + model?: AcpConfigValueSelection | null; + effort?: AcpConfigValueSelection | null; +} + export interface Session { id: string; prompt: string; @@ -369,6 +380,8 @@ export interface Session { updatedAt: number; /** Pipeline execution state. Present when the session was started via a command pipeline. */ pipeline?: PipelineExecution | null; + /** Selected ACP config values to apply before prompting the agent. */ + acpConfigSelection?: AcpConfigSelection | null; } export type QueuedSessionMessageStatus = 'queued' | 'sending' | 'sent'; From 5ca652c0e4db06b8750d6b5b998cc57614823287 Mon Sep 17 00:00:00 2001 From: Matt Toohey Date: Tue, 7 Jul 2026 12:13:44 +1000 Subject: [PATCH 02/34] feat(acp): normalize config options Signed-off-by: Matt Toohey --- apps/staged/src-tauri/Cargo.lock | 1 + apps/staged/src-tauri/Cargo.toml | 1 + apps/staged/src-tauri/src/acp_config.rs | 282 ++++++++++++++++++++++++ apps/staged/src-tauri/src/lib.rs | 1 + 4 files changed, 285 insertions(+) create mode 100644 apps/staged/src-tauri/src/acp_config.rs diff --git a/apps/staged/src-tauri/Cargo.lock b/apps/staged/src-tauri/Cargo.lock index c78d6c21c..c1024ff2e 100644 --- a/apps/staged/src-tauri/Cargo.lock +++ b/apps/staged/src-tauri/Cargo.lock @@ -7,6 +7,7 @@ name = "Staged" version = "0.1.8" dependencies = [ "acp-client", + "agent-client-protocol", "anyhow", "async-trait", "axum", diff --git a/apps/staged/src-tauri/Cargo.toml b/apps/staged/src-tauri/Cargo.toml index 6c673f1d5..54c1125f1 100644 --- a/apps/staged/src-tauri/Cargo.toml +++ b/apps/staged/src-tauri/Cargo.toml @@ -53,6 +53,7 @@ doctor = { path = "../../../crates/doctor" } # Actions framework builderbot-actions = { path = "../../../crates/builderbot-actions" } acp-client = { path = "../../../crates/acp-client" } +agent-client-protocol = { version = "0.15.1", features = ["unstable"] } blox-cli = { path = "../../../crates/blox-cli" } regex = "1" diff --git a/apps/staged/src-tauri/src/acp_config.rs b/apps/staged/src-tauri/src/acp_config.rs new file mode 100644 index 000000000..a0581b88f --- /dev/null +++ b/apps/staged/src-tauri/src/acp_config.rs @@ -0,0 +1,282 @@ +//! Normalization helpers for ACP session configuration options. + +use agent_client_protocol::schema::v1::{ + SessionConfigKind, SessionConfigOption, SessionConfigOptionCategory, SessionConfigSelectOption, + SessionConfigSelectOptions, +}; +use serde::{Deserialize, Serialize}; + +/// Product-facing ACP configuration selectors. +#[derive(Debug, Clone, Default, PartialEq, Eq, Serialize, Deserialize)] +#[serde(rename_all = "camelCase")] +pub(crate) struct NormalizedAcpConfigOptions { + #[serde(default, skip_serializing_if = "Option::is_none")] + pub(crate) model: Option, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub(crate) effort: Option, +} + +/// A normalized select-style ACP configuration option. +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +#[serde(rename_all = "camelCase")] +pub(crate) struct NormalizedAcpConfigSelector { + pub(crate) config_id: String, + pub(crate) label: String, + pub(crate) current_value_id: String, + pub(crate) options: Vec, +} + +/// One flattened selectable value for an ACP configuration option. +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +#[serde(rename_all = "camelCase")] +pub(crate) struct NormalizedAcpConfigValueOption { + pub(crate) value_id: String, + pub(crate) label: String, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub(crate) group_label: Option, +} + +/// Extract the model and reasoning-effort selectors from ACP config options. +pub(crate) fn normalize_acp_config_options( + config_options: &[SessionConfigOption], +) -> NormalizedAcpConfigOptions { + NormalizedAcpConfigOptions { + model: normalize_selector_for_category(config_options, &SessionConfigOptionCategory::Model), + effort: normalize_selector_for_category( + config_options, + &SessionConfigOptionCategory::ThoughtLevel, + ), + } +} + +fn normalize_selector_for_category( + config_options: &[SessionConfigOption], + category: &SessionConfigOptionCategory, +) -> Option { + config_options + .iter() + .filter(|option| option.category.as_ref() == Some(category)) + .find_map(normalize_select_option) +} + +fn normalize_select_option( + config_option: &SessionConfigOption, +) -> Option { + let SessionConfigKind::Select(select) = &config_option.kind else { + return None; + }; + + Some(NormalizedAcpConfigSelector { + config_id: config_option.id.to_string(), + label: config_option.name.clone(), + current_value_id: select.current_value.to_string(), + options: flatten_select_options(&select.options), + }) +} + +fn flatten_select_options( + options: &SessionConfigSelectOptions, +) -> Vec { + match options { + SessionConfigSelectOptions::Ungrouped(options) => options + .iter() + .map(|option| normalize_value(option, None)) + .collect(), + SessionConfigSelectOptions::Grouped(groups) => groups + .iter() + .flat_map(|group| { + group + .options + .iter() + .map(|option| normalize_value(option, Some(&group.name))) + }) + .collect(), + _ => Vec::new(), + } +} + +fn normalize_value( + option: &SessionConfigSelectOption, + group_label: Option<&str>, +) -> NormalizedAcpConfigValueOption { + NormalizedAcpConfigValueOption { + value_id: option.value.to_string(), + label: option.name.clone(), + group_label: group_label.map(str::to_string), + } +} + +#[cfg(test)] +mod tests { + use super::*; + use agent_client_protocol::schema::v1::{ + SessionConfigBoolean, SessionConfigSelectGroup, SessionConfigSelectOption, + }; + + #[test] + fn extracts_model_and_effort_selectors() { + let options = vec![ + SessionConfigOption::select( + "model", + "Model", + "gpt-5", + vec![ + SessionConfigSelectOption::new("gpt-5", "GPT-5"), + SessionConfigSelectOption::new("gpt-5-mini", "GPT-5 mini"), + ], + ) + .category(SessionConfigOptionCategory::Model), + SessionConfigOption::select( + "reasoning", + "Reasoning", + "high", + vec![ + SessionConfigSelectOption::new("low", "Low"), + SessionConfigSelectOption::new("high", "High"), + ], + ) + .category(SessionConfigOptionCategory::ThoughtLevel), + ]; + + let normalized = normalize_acp_config_options(&options); + + let model = normalized.model.expect("model selector"); + assert_eq!(model.config_id, "model"); + assert_eq!(model.current_value_id, "gpt-5"); + assert_eq!( + model.options, + vec![ + NormalizedAcpConfigValueOption { + value_id: "gpt-5".to_string(), + label: "GPT-5".to_string(), + group_label: None, + }, + NormalizedAcpConfigValueOption { + value_id: "gpt-5-mini".to_string(), + label: "GPT-5 mini".to_string(), + group_label: None, + }, + ] + ); + + let effort = normalized.effort.expect("effort selector"); + assert_eq!(effort.config_id, "reasoning"); + assert_eq!(effort.current_value_id, "high"); + assert_eq!(effort.options[1].value_id, "high"); + } + + #[test] + fn filters_unsupported_options() { + let options = vec![ + SessionConfigOption::select( + "mode", + "Mode", + "default", + vec![SessionConfigSelectOption::new("default", "Default")], + ) + .category(SessionConfigOptionCategory::Mode), + SessionConfigOption::new( + "model_toggle", + "Model toggle", + SessionConfigKind::Boolean(SessionConfigBoolean::new(false)), + ) + .category(SessionConfigOptionCategory::Model), + SessionConfigOption::select( + "model", + "Model", + "opus", + vec![SessionConfigSelectOption::new("opus", "Opus")], + ) + .category(SessionConfigOptionCategory::Model), + SessionConfigOption::select( + "effort", + "Effort", + "medium", + vec![SessionConfigSelectOption::new("medium", "Medium")], + ) + .category(SessionConfigOptionCategory::ThoughtLevel), + ]; + + let normalized = normalize_acp_config_options(&options); + + assert_eq!( + normalized.model.expect("model selector").current_value_id, + "opus" + ); + assert!(normalized.effort.is_some()); + } + + #[test] + fn flattens_grouped_options() { + let options = vec![SessionConfigOption::select( + "model", + "Model", + "sonnet", + vec![ + SessionConfigSelectGroup::new( + "fast", + "Fast", + vec![SessionConfigSelectOption::new("haiku", "Haiku")], + ), + SessionConfigSelectGroup::new( + "smart", + "Smart", + vec![ + SessionConfigSelectOption::new("sonnet", "Sonnet"), + SessionConfigSelectOption::new("opus", "Opus"), + ], + ), + ], + ) + .category(SessionConfigOptionCategory::Model)]; + + let normalized = normalize_acp_config_options(&options); + let model = normalized.model.expect("model selector"); + + assert_eq!(model.current_value_id, "sonnet"); + assert_eq!( + model.options, + vec![ + NormalizedAcpConfigValueOption { + value_id: "haiku".to_string(), + label: "Haiku".to_string(), + group_label: Some("Fast".to_string()), + }, + NormalizedAcpConfigValueOption { + value_id: "sonnet".to_string(), + label: "Sonnet".to_string(), + group_label: Some("Smart".to_string()), + }, + NormalizedAcpConfigValueOption { + value_id: "opus".to_string(), + label: "Opus".to_string(), + group_label: Some("Smart".to_string()), + }, + ] + ); + } + + #[test] + fn returns_none_for_missing_categories() { + let options = vec![ + SessionConfigOption::select( + "uncategorized_model", + "Model", + "default", + vec![SessionConfigSelectOption::new("default", "Default")], + ), + SessionConfigOption::select( + "custom", + "Custom", + "custom", + vec![SessionConfigSelectOption::new("custom", "Custom")], + ) + .category(SessionConfigOptionCategory::Other("_custom".to_string())), + ]; + + let normalized = normalize_acp_config_options(&options); + + assert!(normalized.model.is_none()); + assert!(normalized.effort.is_none()); + } +} diff --git a/apps/staged/src-tauri/src/lib.rs b/apps/staged/src-tauri/src/lib.rs index 04720c456..6f6d42afc 100644 --- a/apps/staged/src-tauri/src/lib.rs +++ b/apps/staged/src-tauri/src/lib.rs @@ -3,6 +3,7 @@ //! Tauri commands for the new frontend, built incrementally. //! See `src-archive/lib.rs` for the previous implementation. +pub(crate) mod acp_config; pub mod actions; pub mod agent; pub mod background_sync; From 1e1d6b61d82b10c13b5fb3b674409f82959f7bd6 Mon Sep 17 00:00:00 2001 From: Matt Toohey Date: Tue, 7 Jul 2026 12:31:40 +1000 Subject: [PATCH 03/34] feat(acp): apply selected config before prompt Signed-off-by: Matt Toohey --- apps/staged/src-tauri/src/acp_config.rs | 66 +++++ .../staged/src-tauri/src/pikchr_subsession.rs | 2 + apps/staged/src-tauri/src/session_runner.rs | 14 ++ crates/acp-client/src/driver.rs | 231 ++++++++++++++++-- crates/acp-client/src/lib.rs | 4 +- crates/acp-client/src/simple.rs | 3 + 6 files changed, 304 insertions(+), 16 deletions(-) diff --git a/apps/staged/src-tauri/src/acp_config.rs b/apps/staged/src-tauri/src/acp_config.rs index a0581b88f..cb3efb8f3 100644 --- a/apps/staged/src-tauri/src/acp_config.rs +++ b/apps/staged/src-tauri/src/acp_config.rs @@ -1,11 +1,14 @@ //! Normalization helpers for ACP session configuration options. +use acp_client::AcpSessionConfigOptionSelection; use agent_client_protocol::schema::v1::{ SessionConfigKind, SessionConfigOption, SessionConfigOptionCategory, SessionConfigSelectOption, SessionConfigSelectOptions, }; use serde::{Deserialize, Serialize}; +use crate::store::{AcpConfigSelection, AcpConfigValueSelection}; + /// Product-facing ACP configuration selectors. #[derive(Debug, Clone, Default, PartialEq, Eq, Serialize, Deserialize)] #[serde(rename_all = "camelCase")] @@ -49,6 +52,40 @@ pub(crate) fn normalize_acp_config_options( } } +pub(crate) fn selected_acp_config_options( + selection: Option<&AcpConfigSelection>, +) -> Vec { + let Some(selection) = selection else { + return Vec::new(); + }; + + let mut options = Vec::new(); + if let Some(model) = &selection.model { + options.push(selected_config_option( + SessionConfigOptionCategory::Model, + model, + )); + } + if let Some(effort) = &selection.effort { + options.push(selected_config_option( + SessionConfigOptionCategory::ThoughtLevel, + effort, + )); + } + options +} + +fn selected_config_option( + category: SessionConfigOptionCategory, + selection: &AcpConfigValueSelection, +) -> AcpSessionConfigOptionSelection { + AcpSessionConfigOptionSelection { + category, + config_id: selection.config_id.clone(), + value_id: selection.value_id.clone(), + } +} + fn normalize_selector_for_category( config_options: &[SessionConfigOption], category: &SessionConfigOptionCategory, @@ -279,4 +316,33 @@ mod tests { assert!(normalized.model.is_none()); assert!(normalized.effort.is_none()); } + + #[test] + fn selected_config_options_preserve_model_then_effort_order() { + let selection = AcpConfigSelection { + model: Some(AcpConfigValueSelection { + config_id: "model".to_string(), + value_id: "sonnet".to_string(), + label: Some("Sonnet".to_string()), + }), + effort: Some(AcpConfigValueSelection { + config_id: "reasoning".to_string(), + value_id: "high".to_string(), + label: Some("High".to_string()), + }), + }; + + let selected = selected_acp_config_options(Some(&selection)); + + assert_eq!(selected.len(), 2); + assert_eq!(selected[0].category, SessionConfigOptionCategory::Model); + assert_eq!(selected[0].config_id, "model"); + assert_eq!(selected[0].value_id, "sonnet"); + assert_eq!( + selected[1].category, + SessionConfigOptionCategory::ThoughtLevel + ); + assert_eq!(selected[1].config_id, "reasoning"); + assert_eq!(selected[1].value_id, "high"); + } } diff --git a/apps/staged/src-tauri/src/pikchr_subsession.rs b/apps/staged/src-tauri/src/pikchr_subsession.rs index e48dbeca5..bae44e0f4 100644 --- a/apps/staged/src-tauri/src/pikchr_subsession.rs +++ b/apps/staged/src-tauri/src/pikchr_subsession.rs @@ -81,6 +81,7 @@ pub(crate) async fn generate_pikchr_source( &writer_dyn, cancel_token, agent_session_id.as_deref(), + &[], ) .await?; @@ -302,6 +303,7 @@ box "Sink = NO-OP (default / external clone)" "no socket, no Block deps → buil writer: &Arc, _cancel_token: &CancellationToken, agent_session_id: Option<&str>, + _config_options: &[acp_client::AcpSessionConfigOptionSelection], ) -> Result { let idx = { let mut calls = self.calls.lock().unwrap(); diff --git a/apps/staged/src-tauri/src/session_runner.rs b/apps/staged/src-tauri/src/session_runner.rs index ddd60ae78..3820da484 100644 --- a/apps/staged/src-tauri/src/session_runner.rs +++ b/apps/staged/src-tauri/src/session_runner.rs @@ -403,6 +403,19 @@ pub fn start_session( } }; + let acp_config_selection = store + .get_session(&config.session_id) + .map_err(|e| { + format!( + "Failed to load ACP config selection for session {}: {e}", + config.session_id + ) + })? + .ok_or_else(|| format!("Session not found: {}", config.session_id))? + .acp_config_selection; + let selected_acp_config_options = + crate::acp_config::selected_acp_config_options(acp_config_selection.as_ref()); + // Persist the user message right away so it's visible immediately. // Include image IDs so the frontend can display them alongside the text. // We also mark attached images as session-scoped immediately after so they @@ -656,6 +669,7 @@ pub fn start_session( &writer_trait, &cancel_token, agent_session_id.as_deref(), + &selected_acp_config_options, ) .await; diff --git a/crates/acp-client/src/driver.rs b/crates/acp-client/src/driver.rs index 269c70436..c4ac8bbf1 100644 --- a/crates/acp-client/src/driver.rs +++ b/crates/acp-client/src/driver.rs @@ -25,8 +25,10 @@ use agent_client_protocol::{ NewSessionRequest, PermissionOption as SchemaPermissionOption, PermissionOptionId, PermissionOptionKind as SchemaPermissionOptionKind, PromptRequest, PromptResponse, RequestPermissionOutcome, RequestPermissionRequest, RequestPermissionResponse, - SelectedPermissionOutcome, SessionConfigOption, SessionInfoUpdate, SessionModeState, - SessionNotification, SessionUpdate, StopReason, TextContent, ToolCallContent, + SelectedPermissionOutcome, SessionConfigKind, SessionConfigOption, + SessionConfigOptionCategory, SessionConfigSelectOptions, SessionInfoUpdate, + SessionModeState, SessionNotification, SessionUpdate, SetSessionConfigOptionRequest, + StopReason, TextContent, ToolCallContent, }, ProtocolVersion, }, @@ -259,6 +261,13 @@ pub enum AcpPermissionDecision { Cancelled, } +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct AcpSessionConfigOptionSelection { + pub category: SessionConfigOptionCategory, + pub config_id: String, + pub value_id: String, +} + #[derive(Debug, Clone, PartialEq, Eq)] pub struct ReplayBoundary { pub role: String, @@ -373,6 +382,7 @@ pub trait AgentDriver { writer: &Arc, cancel_token: &CancellationToken, agent_session_id: Option<&str>, + config_options: &[AcpSessionConfigOptionSelection], ) -> Result; } @@ -805,6 +815,7 @@ impl AgentDriver for AcpDriver { writer: &Arc, cancel_token: &CancellationToken, agent_session_id: Option<&str>, + config_options: &[AcpSessionConfigOptionSelection], ) -> Result { let spawn_working_dir = resolve_spawn_working_dir(working_dir, self.is_remote); let acp_working_dir = resolve_acp_working_dir( @@ -1062,6 +1073,7 @@ impl AgentDriver for AcpDriver { store, session_id, agent_session_id, + config_options, &handler, &self.mcp_servers, &self.agent_label, @@ -2059,6 +2071,7 @@ async fn run_acp_protocol( store: &Arc, our_session_id: &str, acp_session_id: Option<&str>, + config_options: &[AcpSessionConfigOptionSelection], handler: &Arc, mcp_servers: &[McpServer], agent_label: &str, @@ -2073,6 +2086,7 @@ async fn run_acp_protocol( writer: &handler.writer, our_session_id, acp_session_id, + config_options, mcp_servers, agent_label, }), @@ -2186,6 +2200,132 @@ fn mcp_server_transport_supported(server: &McpServer, caps: &McpCapabilities) -> } } +async fn apply_or_record_session_config_options( + connection: &ConnectionTo, + agent_session_id: &str, + initial_options: Option<&[SessionConfigOption]>, + selections: &[AcpSessionConfigOptionSelection], + writer: &Arc, +) -> Result<(), String> { + if selections.is_empty() { + if let Some(options) = initial_options { + writer.on_config_option_update(options).await; + } + return Ok(()); + } + + let mut latest_options = initial_options + .ok_or_else(|| { + let labels = selections + .iter() + .map(|selection| config_selection_label(&selection.category)) + .collect::>() + .join("/"); + format!( + "Agent did not return ACP config options needed to apply selected {labels} before prompting" + ) + })? + .to_vec(); + + for selection in selections { + let config_id = resolve_session_config_option_selection(&latest_options, selection)?; + let response = connection + .send_request(SetSessionConfigOptionRequest::new( + agent_session_id.to_string(), + config_id, + selection.value_id.as_str(), + )) + .block_task() + .await + .map_err(|e| { + format!( + "Failed to set selected ACP {} config option: {e:?}", + config_selection_label(&selection.category) + ) + })?; + latest_options = response.config_options; + } + + writer.on_config_option_update(&latest_options).await; + Ok(()) +} + +fn resolve_session_config_option_selection( + options: &[SessionConfigOption], + selection: &AcpSessionConfigOptionSelection, +) -> Result { + let label = config_selection_label(&selection.category); + + if let Some(option) = options + .iter() + .find(|option| option.id.to_string() == selection.config_id) + { + ensure_config_option_has_value(option, &selection.value_id, label)?; + return Ok(option.id.to_string()); + } + + let Some(option) = options.iter().find(|option| { + option.category.as_ref() == Some(&selection.category) && is_select_option(option) + }) else { + return Err(format!( + "Selected ACP {label} config option '{}' is no longer available for this provider", + selection.config_id + )); + }; + + ensure_config_option_has_value(option, &selection.value_id, label)?; + Ok(option.id.to_string()) +} + +fn ensure_config_option_has_value( + option: &SessionConfigOption, + value_id: &str, + label: &str, +) -> Result<(), String> { + match select_option_has_value(option, value_id) { + Some(true) => Ok(()), + Some(false) => Err(format!( + "Selected ACP {label} value '{value_id}' is no longer available for config option '{}'", + option.id + )), + None => Err(format!( + "Selected ACP {label} config option '{}' is not a select option", + option.id + )), + } +} + +fn is_select_option(option: &SessionConfigOption) -> bool { + matches!(&option.kind, SessionConfigKind::Select(_)) +} + +fn select_option_has_value(option: &SessionConfigOption, value_id: &str) -> Option { + let SessionConfigKind::Select(select) = &option.kind else { + return None; + }; + + Some(match &select.options { + SessionConfigSelectOptions::Ungrouped(options) => options + .iter() + .any(|option| option.value.to_string() == value_id), + SessionConfigSelectOptions::Grouped(groups) => groups.iter().any(|group| { + group + .options + .iter() + .any(|option| option.value.to_string() == value_id) + }), + _ => false, + }) +} + +fn config_selection_label(category: &SessionConfigOptionCategory) -> &'static str { + match category { + SessionConfigOptionCategory::Model => "model", + SessionConfigOptionCategory::ThoughtLevel => "effort", + _ => "configuration", + } +} + struct AcpSessionSetup { agent_session_id: String, agent_capabilities: AgentCapabilities, @@ -2198,6 +2338,7 @@ struct AcpSessionSetupContext<'a> { writer: &'a Arc, our_session_id: &'a str, acp_session_id: Option<&'a str>, + config_options: &'a [AcpSessionConfigOptionSelection], mcp_servers: &'a [McpServer], agent_label: &'a str, } @@ -2210,6 +2351,7 @@ async fn setup_acp_session(context: AcpSessionSetupContext<'_>) -> Result) -> Result) -> Result, cancel_token: &CancellationToken, agent_session_id: Option<&str>, + config_options: &[crate::driver::AcpSessionConfigOptionSelection], ) -> Result { if !images.is_empty() { log::debug!( @@ -75,6 +76,7 @@ impl AgentDriver for SimpleDriverWrapper { writer, cancel_token, agent_session_id, + config_options, ) .await } @@ -155,6 +157,7 @@ async fn run_acp_prompt_with_options( &writer, &cancel_token, None, + &[], ) .await .map_err(|e| anyhow::anyhow!("ACP driver error: {e}"))?; From 6a546759f213ef7b6da5dec62f6e53361b6d29dd Mon Sep 17 00:00:00 2001 From: Matt Toohey Date: Tue, 7 Jul 2026 12:45:05 +1000 Subject: [PATCH 04/34] feat(acp): thread config selection through sessions Signed-off-by: Matt Toohey --- apps/staged/src-tauri/src/project_mcp.rs | 15 +++- apps/staged/src-tauri/src/session_commands.rs | 81 +++++++++++++++++-- apps/staged/src-tauri/src/session_runner.rs | 24 +++--- apps/staged/src-tauri/src/web_server.rs | 31 +++++++ apps/staged/src/lib/commands.ts | 31 +++++-- 5 files changed, 153 insertions(+), 29 deletions(-) diff --git a/apps/staged/src-tauri/src/project_mcp.rs b/apps/staged/src-tauri/src/project_mcp.rs index 1079dae83..a1e785b99 100644 --- a/apps/staged/src-tauri/src/project_mcp.rs +++ b/apps/staged/src-tauri/src/project_mcp.rs @@ -18,8 +18,8 @@ use tauri::AppHandle; use crate::actions::{ActionExecutor, ActionRegistry}; use crate::session_runner::SessionRegistry; use crate::store::{ - Branch, CompletionReason, MessageRole, ProjectRepo, Session, SessionMessage, SessionStatus, - Store, + AcpConfigSelection, Branch, CompletionReason, MessageRole, ProjectRepo, Session, + SessionMessage, SessionStatus, Store, }; use tokio_util::sync::CancellationToken; @@ -430,6 +430,10 @@ struct ProjectToolsHandler { /// ACP provider ID inherited from the parent project session. /// All repo sessions spawned by this handler use this provider. provider: Option, + /// ACP config selection inherited from the parent project session. + /// Repo sessions persist it when queued so queue drain uses the selection + /// active when the parent requested the work. + acp_config_selection: Option, /// Cancellation token for the parent project session. /// Signalled when the user cancels the project session. cancel_token: CancellationToken, @@ -445,6 +449,7 @@ impl ProjectToolsHandler { action_executor: Option>, action_registry: Option>, provider: Option, + acp_config_selection: Option, cancel_token: CancellationToken, ) -> Self { Self { @@ -456,6 +461,7 @@ impl ProjectToolsHandler { action_executor, action_registry, provider, + acp_config_selection, cancel_token, } } @@ -487,6 +493,9 @@ impl ProjectToolsHandler { if let Some(ref provider) = self.provider { session = session.with_provider(provider); } + if let Some(selection) = self.acp_config_selection.clone() { + session = session.with_acp_config_selection(selection); + } if let Err(e) = self.store.create_session(&session) { return format!("Error creating queued session: {e}"); } @@ -954,6 +963,7 @@ pub async fn start_project_mcp_server( action_executor: Option>, action_registry: Option>, provider: Option, + acp_config_selection: Option, cancel_token: CancellationToken, ) -> Result<(u16, JoinHandle<()>), String> { let listener = tokio::net::TcpListener::bind("127.0.0.1:0") @@ -972,6 +982,7 @@ pub async fn start_project_mcp_server( action_executor, action_registry, provider, + acp_config_selection, cancel_token, ); log::debug!( diff --git a/apps/staged/src-tauri/src/session_commands.rs b/apps/staged/src-tauri/src/session_commands.rs index e343a3018..47877723d 100644 --- a/apps/staged/src-tauri/src/session_commands.rs +++ b/apps/staged/src-tauri/src/session_commands.rs @@ -402,10 +402,14 @@ pub async fn start_session( prompt: String, working_dir: String, provider: Option, + acp_config_selection: Option, ) -> Result { let store = get_store(&store)?; let working_dir = PathBuf::from(working_dir); - let mut session = store::Session::new_running(&prompt, &working_dir); + let mut session = with_optional_acp_config_selection( + store::Session::new_running(&prompt, &working_dir), + acp_config_selection.clone(), + ); if let Some(ref p) = provider { session = session.with_provider(p); } @@ -428,6 +432,7 @@ pub async fn start_session( image_ids: vec![], queued_message_id: None, pending_auto_review_branch_id: None, + acp_config_selection, branch_id: None, project_id: None, expose_pikchr_tools: false, @@ -461,6 +466,7 @@ pub async fn resume_session( prompt: String, image_ids: Option>, branch_id: Option, + acp_config_selection: Option, ) -> Result<(), String> { let store = get_store(&store)?; resume_session_for_store( @@ -473,6 +479,7 @@ pub async fn resume_session( prompt, image_ids, branch_id, + acp_config_selection, None, None, ) @@ -490,6 +497,7 @@ pub(crate) async fn resume_session_for_store( prompt: String, image_ids: Option>, branch_id: Option, + acp_config_selection: Option, queued_message_id: Option, pending_auto_review_branch_id: Option, ) -> Result<(), String> { @@ -503,6 +511,13 @@ pub(crate) async fn resume_session_for_store( let provider = session.provider.clone(); let agent_session_id = session.agent_id.clone(); let working_dir = PathBuf::from(&session.working_dir); + let effective_acp_config_selection = + acp_config_selection.or_else(|| session.acp_config_selection.clone()); + if effective_acp_config_selection != session.acp_config_selection { + store + .set_session_acp_config_selection(&session_id, effective_acp_config_selection.as_ref()) + .map_err(|e| e.to_string())?; + } // Check if this session is linked to a project note — if so, we need // to start the MCP server so the agent has access to project tools. @@ -674,6 +689,7 @@ pub(crate) async fn resume_session_for_store( image_ids: image_ids.unwrap_or_default(), queued_message_id, pending_auto_review_branch_id, + acp_config_selection: effective_acp_config_selection, branch_id: config_branch_id, project_id: config_project_id, expose_pikchr_tools, @@ -770,6 +786,7 @@ pub(crate) async fn send_queued_session_message_for_store( message.content.clone(), Some(message.image_ids.clone()), message.branch_id.clone(), + None, Some(message.id.clone()), None, ) @@ -809,6 +826,7 @@ pub(crate) async fn drain_queued_message_for_session( message.content.clone(), Some(message.image_ids.clone()), message.branch_id.clone(), + None, Some(message.id.clone()), pending_auto_review_branch_id, ) @@ -1009,6 +1027,16 @@ fn extra_env_for_branch_session(session_type: &BranchSessionType) -> Vec<(String } } +fn with_optional_acp_config_selection( + session: store::Session, + acp_config_selection: Option, +) -> store::Session { + match acp_config_selection { + Some(selection) => session.with_acp_config_selection(selection), + None => session, + } +} + #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] enum BranchSessionScheduleKind { Commit, @@ -1398,6 +1426,7 @@ pub async fn start_project_session( prompt: String, provider: Option, image_ids: Option>, + acp_config_selection: Option, ) -> Result { let store = get_store(&store)?; @@ -1429,7 +1458,10 @@ pub async fn start_project_session( .unwrap_or_else(|_| std::path::PathBuf::from("/tmp")); // Create the session - let mut session = store::Session::new_running(&full_prompt, &working_dir); + let mut session = with_optional_acp_config_selection( + store::Session::new_running(&full_prompt, &working_dir), + acp_config_selection.clone(), + ); if let Some(ref p) = provider { session = session.with_provider(p); } @@ -1460,6 +1492,7 @@ pub async fn start_project_session( image_ids: image_ids.unwrap_or_default(), queued_message_id: None, pending_auto_review_branch_id: None, + acp_config_selection, branch_id: None, project_id: Some(project_id), // Project sessions are always local and write project notes. @@ -1699,8 +1732,12 @@ fn insert_running_branch_session( store: &Arc, prepared: &PreparedBranchSessionStart, prompt: &str, + acp_config_selection: Option, ) -> Result { - let mut session = store::Session::new_running(&prepared.full_prompt, &prepared.working_dir); + let mut session = with_optional_acp_config_selection( + store::Session::new_running(&prepared.full_prompt, &prepared.working_dir), + acp_config_selection, + ); if let Some(ref p) = prepared.provider { session = session.with_provider(p); } @@ -1743,9 +1780,13 @@ fn insert_queued_branch_session( provider: Option, image_ids: &[String], launch_context: Option<&BranchSessionLaunchContext>, + acp_config_selection: Option, ) -> Result { let queued_prompt = embed_launch_context(prompt, launch_context)?; - let mut session = store::Session::new_queued(&queued_prompt); + let mut session = with_optional_acp_config_selection( + store::Session::new_queued(&queued_prompt), + acp_config_selection, + ); if let Some(ref p) = provider { session = session.with_provider(p); } @@ -1828,6 +1869,7 @@ fn launch_running_branch_session( image_ids, queued_message_id: None, pending_auto_review_branch_id: None, + acp_config_selection: created.session.acp_config_selection.clone(), branch_id: Some(branch_id), project_id: Some(project_id), expose_pikchr_tools, @@ -1855,6 +1897,7 @@ pub async fn start_or_queue_branch_session_for_store( provider: Option, image_ids: Option>, launch_context: Option, + acp_config_selection: Option, ) -> Result { let image_ids = image_ids.unwrap_or_default(); @@ -1879,6 +1922,7 @@ pub async fn start_or_queue_branch_session_for_store( provider, &image_ids, launch_context.as_ref(), + acp_config_selection.clone(), ); } } @@ -1905,9 +1949,10 @@ pub async fn start_or_queue_branch_session_for_store( provider, &image_ids, launch_context.as_ref(), + acp_config_selection.clone(), ); } - insert_running_branch_session(&store, &prepared, &prompt)? + insert_running_branch_session(&store, &prepared, &prompt, acp_config_selection)? }; launch_running_branch_session(store, registry, app_handle, prepared, created, image_ids) @@ -1923,6 +1968,7 @@ pub fn queue_branch_session_for_store( provider: Option, image_ids: Option>, launch_context: Option, + acp_config_selection: Option, ) -> Result { let image_ids = image_ids.unwrap_or_default(); @@ -1944,6 +1990,7 @@ pub fn queue_branch_session_for_store( provider, &image_ids, launch_context.as_ref(), + acp_config_selection, ) } @@ -1963,6 +2010,7 @@ pub async fn start_branch_session( provider: Option, image_ids: Option>, launch_context: Option, + acp_config_selection: Option, ) -> Result { let store = get_store(&store)?; start_or_queue_branch_session_for_store( @@ -1975,6 +2023,7 @@ pub async fn start_branch_session( provider, image_ids, launch_context, + acp_config_selection, ) .await } @@ -1991,6 +2040,7 @@ pub async fn start_or_queue_branch_session( provider: Option, image_ids: Option>, launch_context: Option, + acp_config_selection: Option, ) -> Result { let store = get_store(&store)?; start_or_queue_branch_session_for_store( @@ -2003,6 +2053,7 @@ pub async fn start_or_queue_branch_session( provider, image_ids, launch_context, + acp_config_selection, ) .await } @@ -2027,6 +2078,7 @@ pub fn queue_branch_session( provider: Option, image_ids: Option>, launch_context: Option, + acp_config_selection: Option, ) -> Result { let store = get_store(&store)?; queue_branch_session_for_store( @@ -2038,6 +2090,7 @@ pub fn queue_branch_session( provider, image_ids, launch_context, + acp_config_selection, ) } @@ -2376,6 +2429,7 @@ async fn start_queued_session_for_branch( image_ids, queued_message_id: None, pending_auto_review_branch_id: None, + acp_config_selection: session.acp_config_selection.clone(), branch_id: Some(branch_id), project_id: Some(branch.project_id.clone()), expose_pikchr_tools: local_note_pikchr_tools_available( @@ -2729,6 +2783,7 @@ pub async fn trigger_auto_review( image_ids: vec![], queued_message_id: None, pending_auto_review_branch_id: None, + acp_config_selection: None, branch_id: Some(branch_id.clone()), project_id: Some(branch.project_id.clone()), // Auto-review sessions don't write notes. @@ -4610,9 +4665,21 @@ mod tests { } #[test] - fn explicit_queue_response_reports_queued_status() { + fn explicit_queue_response_reports_queued_status_and_stores_acp_config_selection() { let (store, branch) = setup_branch_store(); let registry = Arc::new(session_runner::SessionRegistry::new()); + let selection = store::AcpConfigSelection { + model: Some(store::AcpConfigValueSelection { + config_id: "model".to_string(), + value_id: "gpt-5".to_string(), + label: Some("GPT-5".to_string()), + }), + effort: Some(store::AcpConfigValueSelection { + config_id: "reasoning".to_string(), + value_id: "medium".to_string(), + label: Some("Medium".to_string()), + }), + }; let response = queue_branch_session_for_store( Arc::clone(&store), @@ -4623,12 +4690,14 @@ mod tests { None, None, None, + Some(selection.clone()), ) .unwrap(); assert_eq!(response.session_status, BranchSessionLaunchStatus::Queued); let session = store.get_session(&response.session_id).unwrap().unwrap(); assert_eq!(session.status, store::SessionStatus::Queued); + assert_eq!(session.acp_config_selection, Some(selection)); } #[test] diff --git a/apps/staged/src-tauri/src/session_runner.rs b/apps/staged/src-tauri/src/session_runner.rs index 3820da484..486e6a4ff 100644 --- a/apps/staged/src-tauri/src/session_runner.rs +++ b/apps/staged/src-tauri/src/session_runner.rs @@ -52,9 +52,9 @@ use crate::agent::{AcpDriver, AgentDriver, MessageWriter}; use crate::git::Span; use crate::shell_env::ShellEnvCache; use crate::store::{ - Comment, CommentAuthor, CommentType, CompletionReason, FailureStrategy, MessageRole, - PipelineExecution, PipelineKind, PipelineStep, SessionMessage, SessionStatus, StepStatus, - StepType, Store, + AcpConfigSelection, Comment, CommentAuthor, CommentType, CompletionReason, FailureStrategy, + MessageRole, PipelineExecution, PipelineKind, PipelineStep, SessionMessage, SessionStatus, + StepStatus, StepType, Store, }; const PIPELINE_STEP_PROMPT_OUTPUT_MAX_CHARS: usize = 30_000; @@ -336,6 +336,10 @@ pub struct SessionConfig { pub queued_message_id: Option, /// Branch with a commit waiting for auto-review once queued follow-ups drain. pub pending_auto_review_branch_id: Option, + /// Selected ACP config values to apply after session setup and before the + /// prompt. This is stored on the session row by command handlers before the + /// runner starts so queued and resumed sessions use their own selection. + pub acp_config_selection: Option, /// Branch that owns this session (branch-level sessions only). /// Threaded through so terminal events carry the same context as start events. pub branch_id: Option, @@ -403,18 +407,8 @@ pub fn start_session( } }; - let acp_config_selection = store - .get_session(&config.session_id) - .map_err(|e| { - format!( - "Failed to load ACP config selection for session {}: {e}", - config.session_id - ) - })? - .ok_or_else(|| format!("Session not found: {}", config.session_id))? - .acp_config_selection; let selected_acp_config_options = - crate::acp_config::selected_acp_config_options(acp_config_selection.as_ref()); + crate::acp_config::selected_acp_config_options(config.acp_config_selection.as_ref()); // Persist the user message right away so it's visible immediately. // Include image IDs so the frontend can display them alongside the text. @@ -527,6 +521,7 @@ pub fn start_session( config.action_executor.clone(), config.action_registry.clone(), config.provider.clone(), + config.acp_config_selection.clone(), cancel_token.clone(), ) .await @@ -1192,6 +1187,7 @@ pub fn start_pipeline_session( image_ids: vec![], queued_message_id: None, pending_auto_review_branch_id: None, + acp_config_selection: None, branch_id: config.branch_id.clone(), project_id: config.project_id.clone(), // Deterministic pipelines hand off to a code-focused AI step, diff --git a/apps/staged/src-tauri/src/web_server.rs b/apps/staged/src-tauri/src/web_server.rs index 2d92c9f2a..118075a13 100644 --- a/apps/staged/src-tauri/src/web_server.rs +++ b/apps/staged/src-tauri/src/web_server.rs @@ -2811,9 +2811,14 @@ async fn dispatch(command: &str, args: Value, state: &WebAppState) -> Result = opt_arg(&args, "provider")?; + let acp_config_selection: Option = + opt_arg(&args, "acpConfigSelection")?; let working_dir = std::path::PathBuf::from(working_dir); let mut session = store::Session::new_running(&prompt, &working_dir); + if let Some(selection) = acp_config_selection.clone() { + session = session.with_acp_config_selection(selection); + } if let Some(ref p) = provider { session = session.with_provider(p); } @@ -2836,6 +2841,7 @@ async fn dispatch(command: &str, args: Value, state: &WebAppState) -> Result Result> = opt_arg(&args, "imageIds")?; let branch_id: Option = opt_arg(&args, "branchId")?; + let acp_config_selection: Option = + opt_arg(&args, "acpConfigSelection")?; let session = store .get_session(&session_id) @@ -2862,6 +2870,16 @@ async fn dispatch(command: &str, args: Value, state: &WebAppState) -> Result Result Result> = opt_arg(&args, "imageIds")?; let launch_context: Option = opt_arg(&args, "launchContext")?; + let acp_config_selection: Option = + opt_arg(&args, "acpConfigSelection")?; let result = session_commands::start_or_queue_branch_session_for_store( store, @@ -3113,6 +3134,7 @@ async fn dispatch(command: &str, args: Value, state: &WebAppState) -> Result Result = opt_arg(&args, "provider")?; let image_ids: Option> = opt_arg(&args, "imageIds")?; + let acp_config_selection: Option = + opt_arg(&args, "acpConfigSelection")?; let project = store .get_project(&project_id) @@ -3150,6 +3174,9 @@ async fn dispatch(command: &str, args: Value, state: &WebAppState) -> Result Result Result> = opt_arg(&args, "imageIds")?; let launch_context: Option = opt_arg(&args, "launchContext")?; + let acp_config_selection: Option = + opt_arg(&args, "acpConfigSelection")?; let result = session_commands::queue_branch_session_for_store( store, @@ -3215,6 +3245,7 @@ async fn dispatch(command: &str, args: Value, state: &WebAppState) -> Result { return invokeCommand('start_project_session', { projectId, prompt, provider: provider ?? null, imageIds: imageIds?.length ? imageIds : null, + acpConfigSelection: acpConfigSelection ?? null, }); } @@ -745,9 +748,15 @@ export function countAssistantMessagesAfter( export function startSession( prompt: string, workingDir: string, - provider?: string + provider?: string, + acpConfigSelection?: AcpConfigSelection ): Promise { - return invokeCommand('start_session', { prompt, workingDir, provider: provider ?? null }); + return invokeCommand('start_session', { + prompt, + workingDir, + provider: provider ?? null, + acpConfigSelection: acpConfigSelection ?? null, + }); } /** Send a follow-up message to an existing session. @@ -756,13 +765,15 @@ export function resumeSession( sessionId: string, prompt: string, imageIds?: string[], - branchId?: string | null + branchId?: string | null, + acpConfigSelection?: AcpConfigSelection ): Promise { return invokeCommand('resume_session', { sessionId, prompt, imageIds: imageIds ?? null, branchId: branchId ?? null, + acpConfigSelection: acpConfigSelection ?? null, }); } @@ -819,7 +830,8 @@ export function startBranchSession( sessionType: BranchSessionType, provider?: string, imageIds?: string[], - launchContext?: BranchSessionLaunchContext + launchContext?: BranchSessionLaunchContext, + acpConfigSelection?: AcpConfigSelection ): Promise { return invokeCommand('start_branch_session', { branchId, @@ -828,6 +840,7 @@ export function startBranchSession( provider: provider ?? null, imageIds: imageIds ?? null, launchContext: launchContext ?? null, + acpConfigSelection: acpConfigSelection ?? null, }); } @@ -838,7 +851,8 @@ export function startOrQueueBranchSession( sessionType: BranchSessionType, provider?: string, imageIds?: string[], - launchContext?: BranchSessionLaunchContext + launchContext?: BranchSessionLaunchContext, + acpConfigSelection?: AcpConfigSelection ): Promise { return invokeCommand('start_or_queue_branch_session', { branchId, @@ -847,6 +861,7 @@ export function startOrQueueBranchSession( provider: provider ?? null, imageIds: imageIds ?? null, launchContext: launchContext ?? null, + acpConfigSelection: acpConfigSelection ?? null, }); } @@ -857,7 +872,8 @@ export function queueBranchSession( sessionType: BranchSessionType, provider?: string, imageIds?: string[], - launchContext?: BranchSessionLaunchContext + launchContext?: BranchSessionLaunchContext, + acpConfigSelection?: AcpConfigSelection ): Promise { return invokeCommand('queue_branch_session', { branchId, @@ -866,6 +882,7 @@ export function queueBranchSession( provider: provider ?? null, imageIds: imageIds ?? null, launchContext: launchContext ?? null, + acpConfigSelection: acpConfigSelection ?? null, }); } From b1cf3830e6ee8d95bcdac286c77317afb0b28e0f Mon Sep 17 00:00:00 2001 From: Matt Toohey Date: Tue, 7 Jul 2026 13:02:49 +1000 Subject: [PATCH 05/34] feat(acp): discover picker config selectors Signed-off-by: Matt Toohey --- apps/staged/src-tauri/src/lib.rs | 1 + apps/staged/src-tauri/src/session_commands.rs | 454 +++++++++++++++++- apps/staged/src-tauri/src/web_server.rs | 7 + apps/staged/src/lib/commands.test.ts | 51 ++ apps/staged/src/lib/commands.ts | 38 ++ 5 files changed, 550 insertions(+), 1 deletion(-) diff --git a/apps/staged/src-tauri/src/lib.rs b/apps/staged/src-tauri/src/lib.rs index 6f6d42afc..d8b75c9c7 100644 --- a/apps/staged/src-tauri/src/lib.rs +++ b/apps/staged/src-tauri/src/lib.rs @@ -2312,6 +2312,7 @@ pub fn run() { util_commands::open_in_app, // Sessions session_commands::discover_acp_providers, + session_commands::discover_acp_config, session_commands::get_session, session_commands::get_session_messages, session_commands::get_session_messages_since, diff --git a/apps/staged/src-tauri/src/session_commands.rs b/apps/staged/src-tauri/src/session_commands.rs index 47877723d..b87adb0d7 100644 --- a/apps/staged/src-tauri/src/session_commands.rs +++ b/apps/staged/src-tauri/src/session_commands.rs @@ -17,12 +17,26 @@ //! `Store` directly. use std::collections::{HashMap, HashSet}; +use std::ffi::OsString; +use std::io::Read; use std::path::{Path, PathBuf}; +use std::process::Stdio; use std::sync::{Arc, Mutex, OnceLock}; - +use std::time::Duration; + +use agent_client_protocol::{ + schema::{ + v1::{AuthenticateRequest, Implementation, InitializeRequest, NewSessionRequest}, + ProtocolVersion, + }, + ByteStreams, Client, +}; use serde::{Deserialize, Serialize}; use tauri::path::BaseDirectory; use tauri::Manager; +use tokio::io::{AsyncBufReadExt, BufReader}; +use tokio::process::Command; +use tokio_util::compat::{TokioAsyncReadCompatExt, TokioAsyncWriteCompatExt}; use crate::actions::{ActionExecutor, ActionRegistry}; use crate::agent::{self, AcpProviderInfo}; @@ -319,6 +333,405 @@ pub async fn discover_acp_providers() -> Vec { .unwrap_or_default() } +/// Product-facing ACP config discovery for the provider/model/effort picker. +#[derive(Debug, Clone, PartialEq, Eq, Serialize)] +#[serde(rename_all = "camelCase")] +pub struct AcpConfigDiscovery { + provider_id: String, + #[serde(default, skip_serializing_if = "Option::is_none")] + model: Option, + #[serde(default, skip_serializing_if = "Option::is_none")] + effort: Option, +} + +const ACP_CONFIG_DISCOVERY_SETUP_TIMEOUT: Duration = Duration::from_secs(90); + +#[derive(Debug)] +struct AcpConfigDiscoverySpawnCommand { + program: PathBuf, + args: Vec, + uses_explicit_interpreter: bool, +} + +fn acp_config_discovery_from_options( + provider_id: String, + config_options: &[acp_client::SessionConfigOption], +) -> AcpConfigDiscovery { + let normalized = crate::acp_config::normalize_acp_config_options(config_options); + AcpConfigDiscovery { + provider_id, + model: normalized.model, + effort: normalized.effort, + } +} + +fn resolve_acp_config_discovery_working_dir(working_dir: Option) -> PathBuf { + working_dir + .as_deref() + .map(str::trim) + .filter(|path| !path.is_empty()) + .map(PathBuf::from) + .or_else(|| std::env::current_dir().ok()) + .unwrap_or_else(std::env::temp_dir) +} + +fn env_shebang_interpreter(binary_path: &Path) -> Option { + let mut file = std::fs::File::open(binary_path).ok()?; + let mut buf = [0_u8; 512]; + let len = file.read(&mut buf).ok()?; + let first_line = std::str::from_utf8(&buf[..len]).ok()?.lines().next()?; + let shebang = first_line.strip_prefix("#!")?.trim(); + + let mut parts = shebang.split_whitespace(); + let command = parts.next()?; + let command_name = Path::new(command).file_name()?.to_str()?; + if command_name != "env" { + return None; + } + + for part in parts { + if part == "-S" || part.starts_with('-') || part.contains('=') { + continue; + } + if part.contains('/') { + return None; + } + return Some(part.to_string()); + } + + None +} + +fn is_executable_file(path: &Path) -> bool { + let Ok(metadata) = std::fs::metadata(path) else { + return false; + }; + if !metadata.is_file() { + return false; + } + + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + metadata.permissions().mode() & 0o111 != 0 + } + + #[cfg(not(unix))] + { + true + } +} + +fn executable_from_path(executable: &str, path_value: &str) -> Option { + std::env::split_paths(path_value) + .map(|dir| dir.join(executable)) + .find(|path| is_executable_file(path)) +} + +fn path_contains_executable(path_value: &str, executable: &str) -> bool { + executable_from_path(executable, path_value).is_some() +} + +fn env_shebang_interpreter_from_snapshot( + binary_path: &Path, + snapshot: &[(String, String)], +) -> Option { + let interpreter = env_shebang_interpreter(binary_path)?; + let path_value = snapshot + .iter() + .find(|(key, _)| key == "PATH") + .map(|(_, value)| value.as_str())?; + executable_from_path(&interpreter, path_value) +} + +fn is_broad_toolchain_dir(path: &Path) -> bool { + matches!( + path.to_str(), + Some("/opt/homebrew/bin" | "/usr/local/bin" | "/usr/bin" | "/bin" | "/opt/local/bin") + ) +} + +fn path_with_inserted_agent_bin_dir(existing_path: &str, bin_dir: &Path) -> Option { + let mut entries: Vec = if existing_path.is_empty() { + Vec::new() + } else { + std::env::split_paths(existing_path).collect() + }; + + if entries.iter().any(|entry| entry == bin_dir) { + return None; + } + + if is_broad_toolchain_dir(bin_dir) { + entries.push(bin_dir.to_path_buf()); + } else { + entries.insert(0, bin_dir.to_path_buf()); + } + + std::env::join_paths(entries).ok()?.into_string().ok() +} + +fn guarded_path_for_agent_binary(binary_path: &Path, existing_path: &str) -> Option { + let interpreter = env_shebang_interpreter(binary_path)?; + if path_contains_executable(existing_path, &interpreter) { + return None; + } + + let bin_dir = binary_path.parent()?; + if !is_executable_file(&bin_dir.join(&interpreter)) { + return None; + } + + path_with_inserted_agent_bin_dir(existing_path, bin_dir) +} + +fn acp_config_discovery_spawn_command( + binary_path: &Path, + acp_args: &[String], + interpreter_env_snapshot: &[(String, String)], +) -> AcpConfigDiscoverySpawnCommand { + if let Some(interpreter) = + env_shebang_interpreter_from_snapshot(binary_path, interpreter_env_snapshot) + { + let mut args = vec![binary_path.as_os_str().to_os_string()]; + args.extend(acp_args.iter().map(OsString::from)); + return AcpConfigDiscoverySpawnCommand { + program: interpreter, + args, + uses_explicit_interpreter: true, + }; + } + + AcpConfigDiscoverySpawnCommand { + program: binary_path.to_path_buf(), + args: acp_args.iter().map(OsString::from).collect(), + uses_explicit_interpreter: false, + } +} + +fn apply_acp_config_discovery_env( + cmd: &mut Command, + env_vars: &[(String, String)], + binary_path: &Path, + uses_explicit_interpreter: bool, +) { + cmd.env_clear(); + let mut path_value: Option<&str> = None; + for (key, value) in env_vars { + if key == "PATH" { + path_value = Some(value.as_str()); + } + cmd.env(key, value); + } + + if uses_explicit_interpreter { + return; + } + + if let Some(path) = guarded_path_for_agent_binary(binary_path, path_value.unwrap_or_default()) { + cmd.env("PATH", path); + } +} + +async fn run_acp_config_discovery_protocol( + provider_id: &str, + working_dir: &Path, + stdin: tokio::process::ChildStdin, + stdout: tokio::process::ChildStdout, +) -> Result, String> { + let stdin_compat = stdin.compat_write(); + let stdout_compat = stdout.compat(); + let transport = ByteStreams::new(stdin_compat, stdout_compat); + let working_dir = working_dir.to_path_buf(); + let provider_id = provider_id.to_string(); + let protocol_provider_id = provider_id.clone(); + + Ok(Client + .builder() + .name("staged-acp-config-discovery") + .connect_with(transport, async move |connection| { + tokio::time::timeout(ACP_CONFIG_DISCOVERY_SETUP_TIMEOUT, async { + let client_info = + Implementation::new("staged-acp-config-discovery", env!("CARGO_PKG_VERSION")); + let init_request = + InitializeRequest::new(ProtocolVersion::V1).client_info(client_info); + let init_response = connection + .send_request(init_request) + .block_task() + .await + .map_err(|e| { + format!( + "ACP config discovery init failed for {protocol_provider_id}: {e:?}" + ) + })?; + + if init_response.protocol_version != ProtocolVersion::V1 { + return Err(format!( + "Agent negotiated unsupported ACP protocol version {} (expected {})", + init_response.protocol_version, + ProtocolVersion::V1 + )); + } + + if let Some(method) = init_response.auth_methods.first() { + connection + .send_request(AuthenticateRequest::new(method.id().clone())) + .block_task() + .await + .map_err(|e| { + format!( + "ACP config discovery authentication failed for {protocol_provider_id} with method {} ({}): {e:?}", + method.name(), + method.id() + ) + })?; + } + + let session_response = connection + .send_request(NewSessionRequest::new(working_dir)) + .block_task() + .await + .map_err(|e| { + format!( + "ACP config discovery failed to create session for {protocol_provider_id}: {e:?}" + ) + })?; + + Ok(session_response.config_options.unwrap_or_default()) + }) + .await + .map_err(|_| { + agent_client_protocol::util::internal_error(format!( + "Timed out waiting for ACP config discovery startup after {}s", + ACP_CONFIG_DISCOVERY_SETUP_TIMEOUT.as_secs() + )) + })? + .map_err(agent_client_protocol::util::internal_error) + }) + .await + .map_err(|e| format!("ACP config discovery protocol failed for {provider_id}: {e:?}"))?) +} + +async fn discover_acp_config_for_provider_async( + provider_id: String, + working_dir: PathBuf, +) -> Result { + let agent = acp_client::find_acp_agent_by_id(&provider_id) + .ok_or_else(|| format!("Unknown or unavailable agent provider: {provider_id}"))?; + + let cache = session_runner::shell_env_cache(); + let home_snapshot = crate::shell_env::home_env_vars_with_extended_path(cache.as_ref()).await; + let spawn_command = + acp_config_discovery_spawn_command(agent.path(), &agent.acp_args, &home_snapshot); + + let mut cmd = Command::new(&spawn_command.program); + cmd.args(&spawn_command.args) + .current_dir(&working_dir) + .stdin(Stdio::piped()) + .stdout(Stdio::piped()) + .stderr(Stdio::piped()) + .kill_on_drop(true); + + match cache.get(&working_dir).await { + Ok(snapshot) => apply_acp_config_discovery_env( + &mut cmd, + snapshot.vars(), + agent.path(), + spawn_command.uses_explicit_interpreter, + ), + Err(e) => { + log::warn!( + "ACP config discovery: failed to capture shell env snapshot for {} \ + (falling back to inherited environment): {e}", + working_dir.display() + ); + } + } + + let mut child = cmd.spawn().map_err(|e| { + format!( + "Failed to spawn {} for ACP config discovery (binary: {}, cwd: {}): {e}", + agent.name(), + agent.path().display(), + working_dir.display() + ) + })?; + + if let Some(stderr) = child.stderr.take() { + let agent_label = agent.name().to_string(); + tokio::task::spawn_local(async move { + let mut lines = BufReader::new(stderr).lines(); + while let Ok(Some(line)) = lines.next_line().await { + if line.trim().is_empty() { + continue; + } + log::warn!("[{agent_label} stderr][config discovery] {line}"); + } + }); + } + + let stdin = child + .stdin + .take() + .ok_or_else(|| "Failed to get ACP config discovery stdin".to_string())?; + let stdout = child + .stdout + .take() + .ok_or_else(|| "Failed to get ACP config discovery stdout".to_string())?; + + let config_options = + run_acp_config_discovery_protocol(&provider_id, &working_dir, stdin, stdout).await; + + let _ = child.kill().await; + let _ = child.wait().await; + + let config_options = config_options?; + Ok(acp_config_discovery_from_options( + provider_id, + &config_options, + )) +} + +fn discover_acp_config_for_provider( + provider_id: String, + working_dir: PathBuf, +) -> Result { + let provider_id = provider_id.trim().to_string(); + if provider_id.is_empty() { + return Err("ACP provider ID is required for config discovery".to_string()); + } + + let handle = std::thread::spawn(move || { + let rt = tokio::runtime::Builder::new_current_thread() + .enable_all() + .build() + .map_err(|e| format!("Failed to create runtime for ACP config discovery: {e}"))?; + + let local = tokio::task::LocalSet::new(); + local.block_on( + &rt, + discover_acp_config_for_provider_async(provider_id, working_dir), + ) + }); + + handle + .join() + .map_err(|_| "ACP config discovery thread panicked".to_string())? +} + +/// Discover model/effort selectors for a provider in the given working +/// directory context. +#[tauri::command(rename_all = "camelCase")] +pub async fn discover_acp_config( + provider_id: String, + working_dir: Option, +) -> Result { + let working_dir = resolve_acp_config_discovery_working_dir(working_dir); + tokio::task::spawn_blocking(move || discover_acp_config_for_provider(provider_id, working_dir)) + .await + .map_err(|e| format!("ACP config discovery task failed: {e}"))? +} + // ============================================================================= // Read-only queries (used by frontend polling) // ============================================================================= @@ -4858,6 +5271,45 @@ mod tests { assert_eq!(resolve_preferred_provider_id(None, &[], &[]), None); } + #[test] + fn acp_config_discovery_returns_provider_when_selectors_are_absent() { + let discovery = acp_config_discovery_from_options("goose".to_string(), &[]); + + assert_eq!(discovery.provider_id, "goose"); + assert!(discovery.model.is_none()); + assert!(discovery.effort.is_none()); + } + + #[test] + fn acp_config_discovery_ignores_non_picker_config_options() { + use agent_client_protocol::schema::v1::{ + SessionConfigBoolean, SessionConfigKind, SessionConfigOption, + SessionConfigOptionCategory, SessionConfigSelectOption, + }; + + let options = vec![ + SessionConfigOption::select( + "mode", + "Mode", + "default", + vec![SessionConfigSelectOption::new("default", "Default")], + ) + .category(SessionConfigOptionCategory::Mode), + SessionConfigOption::new( + "model-toggle", + "Model toggle", + SessionConfigKind::Boolean(SessionConfigBoolean::new(false)), + ) + .category(SessionConfigOptionCategory::Model), + ]; + + let discovery = acp_config_discovery_from_options("claude".to_string(), &options); + + assert_eq!(discovery.provider_id, "claude"); + assert!(discovery.model.is_none()); + assert!(discovery.effort.is_none()); + } + #[test] fn resolve_provider_from_ids_rejects_unavailable_provider() { let available = ids(&["goose", "claude"]); diff --git a/apps/staged/src-tauri/src/web_server.rs b/apps/staged/src-tauri/src/web_server.rs index 118075a13..0ddcdfdc4 100644 --- a/apps/staged/src-tauri/src/web_server.rs +++ b/apps/staged/src-tauri/src/web_server.rs @@ -2758,6 +2758,13 @@ async fn dispatch(command: &str, args: Value, state: &WebAppState) -> Result { + let provider_id: String = arg(&args, "providerId")?; + let working_dir: Option = opt_arg(&args, "workingDir")?; + let config = + crate::session_commands::discover_acp_config(provider_id, working_dir).await?; + Ok(serde_json::to_value(config).unwrap()) + } "get_session" => { let store = get_store(store_mutex)?; let session_id: String = arg(&args, "sessionId")?; diff --git a/apps/staged/src/lib/commands.test.ts b/apps/staged/src/lib/commands.test.ts index 36224676d..8f31af42a 100644 --- a/apps/staged/src/lib/commands.test.ts +++ b/apps/staged/src/lib/commands.test.ts @@ -263,4 +263,55 @@ describe('cached mutation command wrappers', () => { }); expect(cachedCommand).toHaveBeenCalledWith('discover_acp_providers', undefined, { ttl: 0 }); }); + + it('caches ACP config discovery by provider and working directory', async () => { + const config = { + providerId: 'goose', + model: null, + effort: null, + }; + cachedCommand.mockResolvedValue({ data: config, revalidating: null }); + + const { discoverAcpConfig } = await import('./commands'); + + await expect(discoverAcpConfig('goose', '/repo')).resolves.toEqual({ + data: config, + revalidating: null, + }); + expect(cachedCommand).toHaveBeenCalledWith( + 'discover_acp_config', + { providerId: 'goose', workingDir: '/repo' }, + { ttl: 30 * 60_000 } + ); + }); + + it('forces ACP config discovery revalidation without bypassing the cached value', async () => { + const config = { + providerId: 'goose', + model: null, + effort: null, + }; + const revalidating = Promise.resolve({ + providerId: 'goose', + model: { + configId: 'model', + label: 'Model', + currentValueId: 'sonnet', + options: [{ valueId: 'sonnet', label: 'Sonnet' }], + }, + }); + cachedCommand.mockResolvedValue({ data: config, revalidating }); + + const { discoverAcpConfig } = await import('./commands'); + + await expect(discoverAcpConfig('goose', null, { force: true })).resolves.toEqual({ + data: config, + revalidating, + }); + expect(cachedCommand).toHaveBeenCalledWith( + 'discover_acp_config', + { providerId: 'goose', workingDir: null }, + { ttl: 0 } + ); + }); }); diff --git a/apps/staged/src/lib/commands.ts b/apps/staged/src/lib/commands.ts index 0d5c3bcfb..3ed670e84 100644 --- a/apps/staged/src/lib/commands.ts +++ b/apps/staged/src/lib/commands.ts @@ -694,10 +694,33 @@ export interface AcpProviderInfo { label: string; } +export interface AcpConfigValueOption { + valueId: string; + label: string; + groupLabel?: string | null; +} + +export interface AcpConfigSelector { + configId: string; + label: string; + currentValueId: string; + options: AcpConfigValueOption[]; +} + +export interface AcpConfigDiscovery { + providerId: string; + model?: AcpConfigSelector | null; + effort?: AcpConfigSelector | null; +} + export interface DiscoverAcpProvidersOptions { force?: boolean; } +export interface DiscoverAcpConfigOptions { + force?: boolean; +} + const ACP_PROVIDER_CACHE_TTL = 30 * 60_000; /** Scan the system for installed ACP-compatible agents. */ @@ -709,6 +732,21 @@ export function discoverAcpProviders( }); } +/** Discover model/effort selectors exposed by a provider for a working directory. */ +export function discoverAcpConfig( + providerId: string, + workingDir?: string | null, + options: DiscoverAcpConfigOptions = {} +): Promise> { + return cachedCommand( + 'discover_acp_config', + { providerId, workingDir: workingDir ?? null }, + { + ttl: options.force ? 0 : ACP_PROVIDER_CACHE_TTL, + } + ); +} + // ============================================================================= // Sessions // ============================================================================= From d013f8059ef2dae5bd5a500bf2c1b164e949f950 Mon Sep 17 00:00:00 2001 From: Matt Toohey Date: Tue, 7 Jul 2026 13:24:28 +1000 Subject: [PATCH 06/34] feat(acp): add provider config picker Signed-off-by: Matt Toohey --- .../features/agents/AcpConfigPicker.svelte | 282 ++++++++++++++++++ .../agents/AcpConfigPickerSection.svelte | 79 +++++ .../features/sessions/NewSessionModal.svelte | 11 +- 3 files changed, 370 insertions(+), 2 deletions(-) create mode 100644 apps/staged/src/lib/features/agents/AcpConfigPicker.svelte create mode 100644 apps/staged/src/lib/features/agents/AcpConfigPickerSection.svelte diff --git a/apps/staged/src/lib/features/agents/AcpConfigPicker.svelte b/apps/staged/src/lib/features/agents/AcpConfigPicker.svelte new file mode 100644 index 000000000..ac29cdf17 --- /dev/null +++ b/apps/staged/src/lib/features/agents/AcpConfigPicker.svelte @@ -0,0 +1,282 @@ + + + +{#if shouldRender} + {#if canOpen} + + + + {triggerParts.join(' · ')} + {#if configLoading} + + {:else} + + {/if} + + + {#if agents.length > 1} + Provider + + {#each agents as provider (provider.id)} + + + + {provider.label} + + + {/each} + + {/if} + + {#if modelSelector} + {#if agents.length > 1} + + {/if} + + {/if} + + {#if effortSelector} + {#if agents.length > 1 || modelSelector} + + {/if} + + {/if} + + {#if configLoading && !modelSelector && !effortSelector} + {#if agents.length > 1} + + {/if} + + + + Loading options… + + + {:else if configError && !modelSelector && !effortSelector && agents.length <= 1} + + Using provider defaults + + {/if} + + + {:else} + + {/if} +{/if} + + diff --git a/apps/staged/src/lib/features/agents/AcpConfigPickerSection.svelte b/apps/staged/src/lib/features/agents/AcpConfigPickerSection.svelte new file mode 100644 index 000000000..3706075ba --- /dev/null +++ b/apps/staged/src/lib/features/agents/AcpConfigPickerSection.svelte @@ -0,0 +1,79 @@ + + +{title} +{#if selector.options.length > 0} + onValueChange(next)} + > + {#each groups as group, groupIndex (`${group.label ?? 'ungrouped'}-${groupIndex}`)} + {#if group.label} +
{group.label}
+ {/if} + {#each group.options as option (option.valueId)} + + {option.label} + + {/each} + {/each} + +{:else} + + Default + +{/if} + + diff --git a/apps/staged/src/lib/features/sessions/NewSessionModal.svelte b/apps/staged/src/lib/features/sessions/NewSessionModal.svelte index 606e9f1b8..4efd6d0f8 100644 --- a/apps/staged/src/lib/features/sessions/NewSessionModal.svelte +++ b/apps/staged/src/lib/features/sessions/NewSessionModal.svelte @@ -27,7 +27,7 @@ import Spinner from '../../shared/Spinner.svelte'; import RepoLabel from '../../shared/RepoLabel.svelte'; import type { Branch, BranchSessionType, HashtagItem, Project, ProjectRepo } from '../../types'; - import AgentSelector from '../agents/AgentSelector.svelte'; + import AcpConfigPicker from '../agents/AcpConfigPicker.svelte'; import ImageAttachment from './ImageAttachment.svelte'; import HashtagInput from './HashtagInput.svelte'; import { buildBranchHashtagItems } from './hashtagItems'; @@ -103,6 +103,7 @@ let isProjectNote = $derived(!branch && !!project); let activeProjectId = $derived(branch?.projectId ?? project?.id ?? ''); let activeBranchId = $derived(branch?.id ?? null); + let activeWorkingDir = $derived(branch?.worktreePath ?? null); let currentWillQueue = $derived(willQueueForMode?.(currentMode) ?? willQueue); let canSubmit = $derived( !!activeProjectId && !starting && !submitDisabledReason && (isReview || !!prompt.trim()) @@ -538,7 +539,13 @@
- + {#if imageIds.length === 0} Date: Tue, 7 Jul 2026 13:35:43 +1000 Subject: [PATCH 07/34] feat(acp): wire new-session config selection Signed-off-by: Matt Toohey --- apps/staged/src/lib/commands.test.ts | 44 +++++++++++++ .../features/agents/AcpConfigPicker.svelte | 18 +++++- .../agents/acpConfigSelection.test.ts | 61 +++++++++++++++++++ .../lib/features/agents/acpConfigSelection.ts | 52 ++++++++++++++++ .../BranchCardSessionManager.svelte.ts | 34 +++++++++-- .../branches/branchSessionLaunch.svelte.ts | 10 ++- .../src/lib/features/diff/DiffModal.svelte | 5 ++ .../features/projects/ProjectSection.svelte | 31 ++++++++-- .../features/sessions/NewSessionModal.svelte | 37 +++++++++-- .../features/sessions/SessionLauncher.svelte | 24 ++++++-- 10 files changed, 294 insertions(+), 22 deletions(-) create mode 100644 apps/staged/src/lib/features/agents/acpConfigSelection.test.ts create mode 100644 apps/staged/src/lib/features/agents/acpConfigSelection.ts diff --git a/apps/staged/src/lib/commands.test.ts b/apps/staged/src/lib/commands.test.ts index 8f31af42a..207b9ebb3 100644 --- a/apps/staged/src/lib/commands.test.ts +++ b/apps/staged/src/lib/commands.test.ts @@ -120,6 +120,50 @@ describe('browser-native command wrappers', () => { ['send_queued_session_message', { id: 'queue-1' }], ]); }); + + it('forwards provider and ACP config selection when starting or queueing branch sessions', async () => { + const invokeCommand = vi.fn().mockResolvedValue({ + sessionId: 'session-1', + artifactId: 'commit-1', + sessionStatus: 'running', + }); + vi.doMock('./transport', () => ({ + invokeCommand, + isTauri: true, + })); + + const { startOrQueueBranchSession } = await import('./commands'); + const launchContext = { + source: 'diff_viewer' as const, + scope: 'commit' as const, + commitSha: 'abc123', + reviewId: 'review-1', + }; + const acpConfigSelection = { + model: { configId: 'model', valueId: 'opus', label: 'Opus' }, + effort: { configId: 'reasoning_effort', valueId: 'high', label: 'High' }, + }; + + await startOrQueueBranchSession( + 'branch-1', + 'Fix the bug', + 'commit', + 'codex', + ['image-1'], + launchContext, + acpConfigSelection + ); + + expect(invokeCommand).toHaveBeenCalledWith('start_or_queue_branch_session', { + branchId: 'branch-1', + prompt: 'Fix the bug', + sessionType: 'commit', + provider: 'codex', + imageIds: ['image-1'], + launchContext, + acpConfigSelection, + }); + }); }); describe('cached mutation command wrappers', () => { diff --git a/apps/staged/src/lib/features/agents/AcpConfigPicker.svelte b/apps/staged/src/lib/features/agents/AcpConfigPicker.svelte index ac29cdf17..7a15653f9 100644 --- a/apps/staged/src/lib/features/agents/AcpConfigPicker.svelte +++ b/apps/staged/src/lib/features/agents/AcpConfigPicker.svelte @@ -2,8 +2,8 @@ AcpConfigPicker.svelte — Compact provider/model/effort picker. Provider changes are persisted to the existing recent-agent preference so - current launch paths keep using the selected provider. Model and effort are - rendered here only; launch payload wiring lands separately. + other launch paths keep using the selected provider. New-session launch paths + can subscribe to the provider plus selected model/effort payload. --> @@ -188,6 +198,12 @@ disabled={creating} class="flex-1" /> + - {/if} -{/if} diff --git a/apps/staged/src/lib/features/agents/acpConfigSelection.test.ts b/apps/staged/src/lib/features/agents/acpConfigSelection.test.ts index 15e8708cb..6d9b30b8d 100644 --- a/apps/staged/src/lib/features/agents/acpConfigSelection.test.ts +++ b/apps/staged/src/lib/features/agents/acpConfigSelection.test.ts @@ -50,6 +50,17 @@ describe('buildAcpConfigSelection', () => { }); }); + it('falls back to the current selector value when a stale selected value is unavailable', () => { + expect( + buildAcpConfigSelection({ + model: { selector: selector(), valueId: 'removed-model' }, + }) + ).toEqual({ + model: { configId: 'model', valueId: 'sonnet', label: 'Sonnet' }, + effort: null, + }); + }); + it('omits unavailable selectors and returns null when there is no selectable value', () => { expect( buildAcpConfigSelection({ From 32d15f8764f52f08d7ed414f29df9673268597ed Mon Sep 17 00:00:00 2001 From: Matt Toohey Date: Tue, 7 Jul 2026 16:50:58 +1000 Subject: [PATCH 10/34] feat(acp): cache config discovery by provider Signed-off-by: Matt Toohey --- apps/staged/src-tauri/src/session_commands.rs | 255 +++++++++++++++++- apps/staged/src-tauri/src/web_server.rs | 4 +- apps/staged/src/lib/commands.test.ts | 47 ++-- apps/staged/src/lib/commands.ts | 15 +- .../features/agents/AcpConfigPicker.svelte | 18 +- .../agents/acpConfigSelection.test.ts | 29 +- .../lib/features/agents/acpConfigSelection.ts | 8 +- .../features/sessions/SessionChatPane.svelte | 41 ++- 8 files changed, 354 insertions(+), 63 deletions(-) diff --git a/apps/staged/src-tauri/src/session_commands.rs b/apps/staged/src-tauri/src/session_commands.rs index 1ff053548..70051b4d9 100644 --- a/apps/staged/src-tauri/src/session_commands.rs +++ b/apps/staged/src-tauri/src/session_commands.rs @@ -22,7 +22,7 @@ use std::io::Read; use std::path::{Path, PathBuf}; use std::process::Stdio; use std::sync::{Arc, Mutex, OnceLock}; -use std::time::Duration; +use std::time::{Duration, Instant}; use agent_client_protocol::{ schema::{ @@ -345,6 +345,28 @@ pub struct AcpConfigDiscovery { } const ACP_CONFIG_DISCOVERY_SETUP_TIMEOUT: Duration = Duration::from_secs(90); +const ACP_CONFIG_DISCOVERY_CACHE_TTL: Duration = Duration::from_secs(30 * 60); + +#[derive(Debug, Clone)] +struct AcpConfigDiscoveryCacheEntry { + discovery: AcpConfigDiscovery, + fetched_at: Instant, +} + +static ACP_CONFIG_DISCOVERY_CACHE: OnceLock>> = + OnceLock::new(); + +fn acp_config_discovery_cache() -> &'static Mutex> { + ACP_CONFIG_DISCOVERY_CACHE.get_or_init(|| Mutex::new(HashMap::new())) +} + +fn normalized_acp_config_provider_id(provider_id: &str) -> Result { + let provider_id = provider_id.trim().to_string(); + if provider_id.is_empty() { + return Err("ACP provider ID is required for config discovery".to_string()); + } + Ok(provider_id) +} #[derive(Debug)] struct AcpConfigDiscoverySpawnCommand { @@ -696,10 +718,7 @@ fn discover_acp_config_for_provider( provider_id: String, working_dir: PathBuf, ) -> Result { - let provider_id = provider_id.trim().to_string(); - if provider_id.is_empty() { - return Err("ACP provider ID is required for config discovery".to_string()); - } + let provider_id = normalized_acp_config_provider_id(&provider_id)?; let handle = std::thread::spawn(move || { let rt = tokio::runtime::Builder::new_current_thread() @@ -719,17 +738,74 @@ fn discover_acp_config_for_provider( .map_err(|_| "ACP config discovery thread panicked".to_string())? } +fn discover_acp_config_for_provider_with_cache( + provider_id: String, + working_dir: PathBuf, + force: bool, + fetch: F, +) -> Result +where + F: FnOnce(String, PathBuf) -> Result, +{ + let provider_id = normalized_acp_config_provider_id(&provider_id)?; + let cached_entry = { + let cache = acp_config_discovery_cache().lock().unwrap(); + cache.get(&provider_id).cloned() + }; + + if !force { + if let Some(entry) = &cached_entry { + if entry.fetched_at.elapsed() < ACP_CONFIG_DISCOVERY_CACHE_TTL { + return Ok(entry.discovery.clone()); + } + } + } + + match fetch(provider_id.clone(), working_dir) { + Ok(discovery) => { + let mut cache = acp_config_discovery_cache().lock().unwrap(); + cache.insert( + provider_id, + AcpConfigDiscoveryCacheEntry { + discovery: discovery.clone(), + fetched_at: Instant::now(), + }, + ); + Ok(discovery) + } + Err(e) => { + if let Some(entry) = cached_entry { + log::warn!( + "ACP config discovery refresh failed for {provider_id}; using stale cached options: {e}" + ); + Ok(entry.discovery) + } else { + Err(e) + } + } + } +} + /// Discover model/effort selectors for a provider in the given working /// directory context. #[tauri::command(rename_all = "camelCase")] pub async fn discover_acp_config( provider_id: String, working_dir: Option, + force: Option, ) -> Result { let working_dir = resolve_acp_config_discovery_working_dir(working_dir); - tokio::task::spawn_blocking(move || discover_acp_config_for_provider(provider_id, working_dir)) - .await - .map_err(|e| format!("ACP config discovery task failed: {e}"))? + let force = force.unwrap_or(false); + tokio::task::spawn_blocking(move || { + discover_acp_config_for_provider_with_cache( + provider_id, + working_dir, + force, + discover_acp_config_for_provider, + ) + }) + .await + .map_err(|e| format!("ACP config discovery task failed: {e}"))? } // ============================================================================= @@ -4674,8 +4750,8 @@ pub(crate) fn extract_launch_context( #[cfg(test)] mod tests { use super::*; - use std::path::Path; - use std::sync::Arc; + use std::path::{Path, PathBuf}; + use std::sync::{Arc, Mutex}; fn setup_branch_store() -> (Arc, store::Branch) { let store = Arc::new(Store::in_memory().unwrap()); @@ -4711,6 +4787,43 @@ mod tests { values.iter().map(|value| value.to_string()).collect() } + fn test_acp_config_discovery(provider_id: &str, current_value_id: &str) -> AcpConfigDiscovery { + AcpConfigDiscovery { + provider_id: provider_id.to_string(), + model: Some(crate::acp_config::NormalizedAcpConfigSelector { + config_id: "model".to_string(), + label: "Model".to_string(), + current_value_id: current_value_id.to_string(), + options: vec![ + crate::acp_config::NormalizedAcpConfigValueOption { + value_id: "sonnet".to_string(), + label: "Sonnet".to_string(), + group_label: None, + }, + crate::acp_config::NormalizedAcpConfigValueOption { + value_id: "opus".to_string(), + label: "Opus".to_string(), + group_label: None, + }, + ], + }), + effort: None, + } + } + + fn stale_acp_config_cache_fetch_time() -> Instant { + Instant::now() + .checked_sub(ACP_CONFIG_DISCOVERY_CACHE_TTL + Duration::from_secs(1)) + .unwrap_or_else(Instant::now) + } + + fn remove_acp_config_cache_entry(provider_id: &str) { + acp_config_discovery_cache() + .lock() + .unwrap() + .remove(provider_id); + } + fn create_auto_review( store: &Arc, branch_id: &str, @@ -5341,6 +5454,128 @@ mod tests { assert!(discovery.effort.is_none()); } + #[test] + fn acp_config_discovery_cache_is_provider_scoped_across_working_dirs() { + let provider_id = "cache-provider-scoped-dirs"; + remove_acp_config_cache_entry(provider_id); + let calls = Arc::new(Mutex::new(Vec::::new())); + let calls_for_fetch = Arc::clone(&calls); + + let first = discover_acp_config_for_provider_with_cache( + provider_id.to_string(), + PathBuf::from("/repo-one"), + false, + move |provider_id, working_dir| { + calls_for_fetch.lock().unwrap().push(working_dir); + Ok(test_acp_config_discovery(&provider_id, "sonnet")) + }, + ) + .unwrap(); + let second = discover_acp_config_for_provider_with_cache( + provider_id.to_string(), + PathBuf::from("/repo-two"), + false, + |_provider_id, _working_dir| panic!("fresh cache hit should not refetch"), + ) + .unwrap(); + + assert_eq!(first, second); + assert_eq!( + calls.lock().unwrap().as_slice(), + &[PathBuf::from("/repo-one")] + ); + } + + #[test] + fn acp_config_discovery_force_bypasses_and_replaces_fresh_cache() { + let provider_id = "cache-provider-force"; + remove_acp_config_cache_entry(provider_id); + + let first = discover_acp_config_for_provider_with_cache( + provider_id.to_string(), + PathBuf::from("/repo"), + false, + |provider_id, _working_dir| Ok(test_acp_config_discovery(&provider_id, "sonnet")), + ) + .unwrap(); + let second = discover_acp_config_for_provider_with_cache( + provider_id.to_string(), + PathBuf::from("/repo"), + true, + |provider_id, _working_dir| Ok(test_acp_config_discovery(&provider_id, "opus")), + ) + .unwrap(); + let third = discover_acp_config_for_provider_with_cache( + provider_id.to_string(), + PathBuf::from("/repo"), + false, + |_provider_id, _working_dir| panic!("replaced cache hit should not refetch"), + ) + .unwrap(); + + assert_eq!( + first.model.as_ref().unwrap().current_value_id, + "sonnet".to_string() + ); + assert_eq!( + second.model.as_ref().unwrap().current_value_id, + "opus".to_string() + ); + assert_eq!(third, second); + } + + #[test] + fn acp_config_discovery_refresh_failure_returns_stale_cache() { + let provider_id = "cache-provider-stale"; + let stale = test_acp_config_discovery(provider_id, "sonnet"); + acp_config_discovery_cache().lock().unwrap().insert( + provider_id.to_string(), + AcpConfigDiscoveryCacheEntry { + discovery: stale.clone(), + fetched_at: stale_acp_config_cache_fetch_time(), + }, + ); + + let result = discover_acp_config_for_provider_with_cache( + provider_id.to_string(), + PathBuf::from("/repo"), + false, + |_provider_id, _working_dir| Err("provider unavailable".to_string()), + ) + .unwrap(); + + assert_eq!(result, stale); + } + + #[test] + fn acp_config_discovery_refresh_failure_without_stale_cache_returns_error() { + let provider_id = "cache-provider-miss-failure"; + remove_acp_config_cache_entry(provider_id); + + let err = discover_acp_config_for_provider_with_cache( + provider_id.to_string(), + PathBuf::from("/repo"), + false, + |_provider_id, _working_dir| Err("provider unavailable".to_string()), + ) + .unwrap_err(); + + assert_eq!(err, "provider unavailable"); + } + + #[test] + fn acp_config_discovery_cache_rejects_blank_provider_ids() { + let err = discover_acp_config_for_provider_with_cache( + " ".to_string(), + PathBuf::from("/repo"), + false, + |_provider_id, _working_dir| panic!("blank provider should not fetch"), + ) + .unwrap_err(); + + assert!(err.contains("ACP provider ID is required")); + } + #[test] fn resolve_provider_from_ids_rejects_unavailable_provider() { let available = ids(&["goose", "claude"]); diff --git a/apps/staged/src-tauri/src/web_server.rs b/apps/staged/src-tauri/src/web_server.rs index a9f56ac20..637025299 100644 --- a/apps/staged/src-tauri/src/web_server.rs +++ b/apps/staged/src-tauri/src/web_server.rs @@ -2761,8 +2761,10 @@ async fn dispatch(command: &str, args: Value, state: &WebAppState) -> Result { let provider_id: String = arg(&args, "providerId")?; let working_dir: Option = opt_arg(&args, "workingDir")?; + let force: Option = opt_arg(&args, "force")?; let config = - crate::session_commands::discover_acp_config(provider_id, working_dir).await?; + crate::session_commands::discover_acp_config(provider_id, working_dir, force) + .await?; Ok(serde_json::to_value(config).unwrap()) } "get_session" => { diff --git a/apps/staged/src/lib/commands.test.ts b/apps/staged/src/lib/commands.test.ts index 7fc7e367b..4dec45b8d 100644 --- a/apps/staged/src/lib/commands.test.ts +++ b/apps/staged/src/lib/commands.test.ts @@ -440,13 +440,13 @@ describe('cached mutation command wrappers', () => { expect(cachedCommand).toHaveBeenCalledWith('discover_acp_providers', undefined, { ttl: 0 }); }); - it('caches ACP config discovery by provider and working directory', async () => { + it('discovers ACP config through the backend without frontend caching', async () => { const config = { providerId: 'goose', model: null, effort: null, }; - cachedCommand.mockResolvedValue({ data: config, revalidating: null }); + invokeCommand.mockResolvedValue(config); const { discoverAcpConfig } = await import('./commands'); @@ -454,40 +454,43 @@ describe('cached mutation command wrappers', () => { data: config, revalidating: null, }); - expect(cachedCommand).toHaveBeenCalledWith( + await discoverAcpConfig('goose', '/other-repo'); + + expect(invokeCommand).toHaveBeenNthCalledWith(1, 'discover_acp_config', { + providerId: 'goose', + workingDir: '/repo', + force: false, + }); + expect(invokeCommand).toHaveBeenNthCalledWith(2, 'discover_acp_config', { + providerId: 'goose', + workingDir: '/other-repo', + force: false, + }); + expect(cachedCommand).not.toHaveBeenCalledWith( 'discover_acp_config', - { providerId: 'goose', workingDir: '/repo' }, - { ttl: 30 * 60_000 } + expect.anything(), + expect.anything() ); }); - it('forces ACP config discovery revalidation without bypassing the cached value', async () => { + it('passes ACP config discovery force to the backend command', async () => { const config = { providerId: 'goose', model: null, effort: null, }; - const revalidating = Promise.resolve({ - providerId: 'goose', - model: { - configId: 'model', - label: 'Model', - currentValueId: 'sonnet', - options: [{ valueId: 'sonnet', label: 'Sonnet' }], - }, - }); - cachedCommand.mockResolvedValue({ data: config, revalidating }); + invokeCommand.mockResolvedValue(config); const { discoverAcpConfig } = await import('./commands'); await expect(discoverAcpConfig('goose', null, { force: true })).resolves.toEqual({ data: config, - revalidating, + revalidating: null, + }); + expect(invokeCommand).toHaveBeenCalledWith('discover_acp_config', { + providerId: 'goose', + workingDir: null, + force: true, }); - expect(cachedCommand).toHaveBeenCalledWith( - 'discover_acp_config', - { providerId: 'goose', workingDir: null }, - { ttl: 0 } - ); }); }); diff --git a/apps/staged/src/lib/commands.ts b/apps/staged/src/lib/commands.ts index 3ed670e84..cdc4ffee4 100644 --- a/apps/staged/src/lib/commands.ts +++ b/apps/staged/src/lib/commands.ts @@ -733,18 +733,17 @@ export function discoverAcpProviders( } /** Discover model/effort selectors exposed by a provider for a working directory. */ -export function discoverAcpConfig( +export async function discoverAcpConfig( providerId: string, workingDir?: string | null, options: DiscoverAcpConfigOptions = {} ): Promise> { - return cachedCommand( - 'discover_acp_config', - { providerId, workingDir: workingDir ?? null }, - { - ttl: options.force ? 0 : ACP_PROVIDER_CACHE_TTL, - } - ); + const data = await invokeCommand('discover_acp_config', { + providerId, + workingDir: workingDir ?? null, + force: options.force ?? false, + }); + return { data, revalidating: null }; } // ============================================================================= diff --git a/apps/staged/src/lib/features/agents/AcpConfigPicker.svelte b/apps/staged/src/lib/features/agents/AcpConfigPicker.svelte index 7a15653f9..eed1b2984 100644 --- a/apps/staged/src/lib/features/agents/AcpConfigPicker.svelte +++ b/apps/staged/src/lib/features/agents/AcpConfigPicker.svelte @@ -49,6 +49,8 @@ let configError = $state(null); let selectedModelValue = $state(null); let selectedEffortValue = $state(null); + let modelSelectionExplicit = $state(false); + let effortSelectionExplicit = $state(false); let modelSelectorKey = $state(null); let effortSelectorKey = $state(null); let discoveryRun = 0; @@ -72,8 +74,16 @@ let pickerSelection = $derived({ providerId: selectedProviderId, acpConfigSelection: buildAcpConfigSelection({ - model: { selector: modelSelector, valueId: selectedModelValue }, - effort: { selector: effortSelector, valueId: selectedEffortValue }, + model: { + selector: modelSelector, + valueId: selectedModelValue, + explicit: modelSelectionExplicit, + }, + effort: { + selector: effortSelector, + valueId: selectedEffortValue, + explicit: effortSelectionExplicit, + }, }), } satisfies AcpConfigPickerSelection); @@ -126,6 +136,7 @@ const nextKey = selectorKey(modelSelector); if (nextKey !== modelSelectorKey) { selectedModelValue = defaultSelectorValue(modelSelector); + modelSelectionExplicit = false; modelSelectorKey = nextKey; } }); @@ -134,6 +145,7 @@ const nextKey = selectorKey(effortSelector); if (nextKey !== effortSelectorKey) { selectedEffortValue = defaultSelectorValue(effortSelector); + effortSelectionExplicit = false; effortSelectorKey = nextKey; } }); @@ -148,10 +160,12 @@ function handleModelChange(value: string) { selectedModelValue = value; + modelSelectionExplicit = true; } function handleEffortChange(value: string) { selectedEffortValue = value; + effortSelectionExplicit = true; } function selectorKey(selector: AcpConfigSelector | null): string | null { diff --git a/apps/staged/src/lib/features/agents/acpConfigSelection.test.ts b/apps/staged/src/lib/features/agents/acpConfigSelection.test.ts index 6d9b30b8d..557bb5eff 100644 --- a/apps/staged/src/lib/features/agents/acpConfigSelection.test.ts +++ b/apps/staged/src/lib/features/agents/acpConfigSelection.test.ts @@ -30,8 +30,8 @@ describe('buildAcpConfigSelection', () => { expect( buildAcpConfigSelection({ - model: { selector: model, valueId: 'opus' }, - effort: { selector: effort, valueId: 'high' }, + model: { selector: model, valueId: 'opus', explicit: true }, + effort: { selector: effort, valueId: 'high', explicit: true }, }) ).toEqual({ model: { configId: 'model', valueId: 'opus', label: 'Opus' }, @@ -39,21 +39,18 @@ describe('buildAcpConfigSelection', () => { }); }); - it('falls back to the current selector value when no explicit value is selected', () => { + it('omits untouched default selector values', () => { expect( buildAcpConfigSelection({ - model: { selector: selector(), valueId: null }, + model: { selector: selector(), valueId: 'sonnet' }, }) - ).toEqual({ - model: { configId: 'model', valueId: 'sonnet', label: 'Sonnet' }, - effort: null, - }); + ).toBeNull(); }); - it('falls back to the current selector value when a stale selected value is unavailable', () => { + it('can explicitly send the current selector value', () => { expect( buildAcpConfigSelection({ - model: { selector: selector(), valueId: 'removed-model' }, + model: { selector: selector(), valueId: 'sonnet', explicit: true }, }) ).toEqual({ model: { configId: 'model', valueId: 'sonnet', label: 'Sonnet' }, @@ -61,11 +58,19 @@ describe('buildAcpConfigSelection', () => { }); }); + it('does not remap stale selected values to the current selector value', () => { + expect( + buildAcpConfigSelection({ + model: { selector: selector(), valueId: 'removed-model', explicit: true }, + }) + ).toBeNull(); + }); + it('omits unavailable selectors and returns null when there is no selectable value', () => { expect( buildAcpConfigSelection({ - model: { selector: selector({ options: [] }), valueId: null }, - effort: { selector: null, valueId: null }, + model: { selector: selector({ options: [] }), valueId: null, explicit: true }, + effort: { selector: null, valueId: null, explicit: true }, }) ).toBeNull(); }); diff --git a/apps/staged/src/lib/features/agents/acpConfigSelection.ts b/apps/staged/src/lib/features/agents/acpConfigSelection.ts index 1719c6e76..cafbc155b 100644 --- a/apps/staged/src/lib/features/agents/acpConfigSelection.ts +++ b/apps/staged/src/lib/features/agents/acpConfigSelection.ts @@ -9,6 +9,7 @@ export interface AcpConfigPickerSelection { interface SelectorSelection { selector: AcpConfigSelector | null; valueId: string | null; + explicit?: boolean; } interface AcpSelectorSelections { @@ -20,15 +21,12 @@ function selectedValueId(selector: AcpConfigSelector, valueId: string | null): s if (valueId && selector.options.some((option) => option.valueId === valueId)) { return valueId; } - if (selector.options.some((option) => option.valueId === selector.currentValueId)) { - return selector.currentValueId; - } - return selector.options[0]?.valueId ?? null; + return null; } function valueSelection(selection: SelectorSelection | undefined): AcpConfigValueSelection | null { const selector = selection?.selector ?? null; - if (!selector) return null; + if (!selector || !selection?.explicit) return null; const valueId = selectedValueId(selector, selection?.valueId ?? null); if (!valueId) return null; diff --git a/apps/staged/src/lib/features/sessions/SessionChatPane.svelte b/apps/staged/src/lib/features/sessions/SessionChatPane.svelte index c9b86652a..3d81fe751 100644 --- a/apps/staged/src/lib/features/sessions/SessionChatPane.svelte +++ b/apps/staged/src/lib/features/sessions/SessionChatPane.svelte @@ -207,6 +207,8 @@ let followupConfigError = $state(null); let selectedFollowupModelValue = $state(null); let selectedFollowupEffortValue = $state(null); + let followupModelTouched = $state(false); + let followupEffortTouched = $state(false); let followupModelStateKey = $state(null); let followupEffortStateKey = $state(null); let followupDiscoveryRun = 0; @@ -249,8 +251,26 @@ ); let followupAcpConfigSelection = $derived( buildAcpConfigSelection({ - model: { selector: followupModelSelector, valueId: selectedFollowupModelValue }, - effort: { selector: followupEffortSelector, valueId: selectedFollowupEffortValue }, + model: { + selector: followupModelSelector, + valueId: selectedFollowupModelValue, + explicit: shouldSendFollowupSelectorSelection( + followupModelSelector, + selectedFollowupModelValue, + session?.acpConfigSelection?.model ?? null, + followupModelTouched + ), + }, + effort: { + selector: followupEffortSelector, + valueId: selectedFollowupEffortValue, + explicit: shouldSendFollowupSelectorSelection( + followupEffortSelector, + selectedFollowupEffortValue, + session?.acpConfigSelection?.effort ?? null, + followupEffortTouched + ), + }, }) ); @@ -1223,7 +1243,7 @@ followupConfigLoading = true; followupConfigError = null; - discoverAcpConfig(providerId, workingDir, { force: true }) + discoverAcpConfig(providerId, workingDir) .then(({ data, revalidating }) => { if (cancelled || run !== followupDiscoveryRun) return; discoveredFollowupConfig = data; @@ -1264,6 +1284,7 @@ followupModelSelector, session?.acpConfigSelection?.model ?? null ); + followupModelTouched = false; followupModelStateKey = key; } }); @@ -1279,6 +1300,7 @@ followupEffortSelector, session?.acpConfigSelection?.effort ?? null ); + followupEffortTouched = false; followupEffortStateKey = key; } }); @@ -1322,12 +1344,25 @@ return selector.options[0]?.valueId ?? null; } + function shouldSendFollowupSelectorSelection( + selector: AcpConfigSelector | null, + valueId: string | null, + storedSelection: AcpConfigValueSelection | null, + touched: boolean + ): boolean { + if (!selector || !valueId) return false; + if (touched) return true; + return storedSelection?.configId === selector.configId && storedSelection.valueId === valueId; + } + function handleFollowupModelChange(value: string) { selectedFollowupModelValue = value; + followupModelTouched = true; } function handleFollowupEffortChange(value: string) { selectedFollowupEffortValue = value; + followupEffortTouched = true; } let grouped = $derived.by(() => From 4d1a68076a28fa5c4f38b8ef9dd643ce2b626199 Mon Sep 17 00:00:00 2001 From: Matt Toohey Date: Tue, 7 Jul 2026 20:29:20 +1000 Subject: [PATCH 11/34] fix(acp): lay out config picker controls in columns Signed-off-by: Matt Toohey --- .../features/agents/AcpConfigPicker.svelte | 117 ++++++++++++------ .../agents/AcpFixedConfigPicker.svelte | 96 +++++++++----- 2 files changed, 147 insertions(+), 66 deletions(-) diff --git a/apps/staged/src/lib/features/agents/AcpConfigPicker.svelte b/apps/staged/src/lib/features/agents/AcpConfigPicker.svelte index eed1b2984..43e9aaefc 100644 --- a/apps/staged/src/lib/features/agents/AcpConfigPicker.svelte +++ b/apps/staged/src/lib/features/agents/AcpConfigPicker.svelte @@ -220,53 +220,60 @@ align="start" side={dropUp ? 'top' : 'bottom'} sideOffset={4} - class="max-h-[min(360px,calc(100vh-48px))] min-w-[220px] max-w-[360px]" + class="max-h-[min(360px,calc(100vh-48px))] w-[min(680px,calc(100vw-16px))] max-w-[calc(100vw-16px)]" > - {#if agents.length > 1} - Provider - - {#each agents as provider (provider.id)} - +
+
+ Agent + {#if agents.length > 1} + + {#each agents as provider (provider.id)} + + + + {provider.label} + + + {/each} + + {:else} + - - {provider.label} + + {selectedProvider?.label ?? 'Agent'} - - {/each} - - {/if} + + {/if} +
- {#if modelSelector} - {#if agents.length > 1} - + {#if modelSelector} +
+ +
{/if} - - {/if} - {#if effortSelector} - {#if agents.length > 1 || modelSelector} - + {#if effortSelector} +
+ +
{/if} - - {/if} +
{#if configLoading && !modelSelector && !effortSelector} - {#if agents.length > 1} - - {/if} + @@ -299,6 +306,27 @@ {/if} diff --git a/apps/staged/src/lib/features/agents/AcpFixedConfigPicker.svelte b/apps/staged/src/lib/features/agents/AcpFixedConfigPicker.svelte index 5895b6d4f..fa3765fb6 100644 --- a/apps/staged/src/lib/features/agents/AcpFixedConfigPicker.svelte +++ b/apps/staged/src/lib/features/agents/AcpFixedConfigPicker.svelte @@ -88,37 +88,43 @@ align="start" side={dropUp ? 'top' : 'bottom'} sideOffset={4} - class="max-h-[min(360px,calc(100vh-48px))] min-w-[220px] max-w-[360px]" + class="max-h-[min(360px,calc(100vh-48px))] w-[min(680px,calc(100vw-16px))] max-w-[calc(100vw-16px)]" > - Provider - - - - {providerLabel ?? providerId ?? 'Agent'} - - +
+
+ Agent + + + + {providerLabel ?? providerId ?? 'Agent'} + + +
- {#if modelSelector} - - onModelChange?.(value)} - /> - {/if} + {#if modelSelector} +
+ onModelChange?.(value)} + /> +
+ {/if} - {#if effortSelector} - - onEffortChange?.(value)} - /> - {/if} + {#if effortSelector} +
+ onEffortChange?.(value)} + /> +
+ {/if} +
{#if loading && !modelSelector && !effortSelector} @@ -139,6 +145,27 @@ {/if} From 27809e34c4fc87b50869339ac563f825433d3c06 Mon Sep 17 00:00:00 2001 From: Matt Toohey Date: Tue, 7 Jul 2026 20:38:26 +1000 Subject: [PATCH 12/34] fix(acp): size picker columns to content Signed-off-by: Matt Toohey --- .../lib/features/agents/AcpConfigPicker.svelte | 15 ++++++++++++--- .../features/agents/AcpFixedConfigPicker.svelte | 15 ++++++++++++--- 2 files changed, 24 insertions(+), 6 deletions(-) diff --git a/apps/staged/src/lib/features/agents/AcpConfigPicker.svelte b/apps/staged/src/lib/features/agents/AcpConfigPicker.svelte index 43e9aaefc..c02f307c3 100644 --- a/apps/staged/src/lib/features/agents/AcpConfigPicker.svelte +++ b/apps/staged/src/lib/features/agents/AcpConfigPicker.svelte @@ -220,7 +220,7 @@ align="start" side={dropUp ? 'top' : 'bottom'} sideOffset={4} - class="max-h-[min(360px,calc(100vh-48px))] w-[min(680px,calc(100vw-16px))] max-w-[calc(100vw-16px)]" + class="max-h-[min(360px,calc(100vh-48px))] max-w-[calc(100vw-16px)]" >
@@ -307,13 +307,22 @@ diff --git a/apps/staged/src/lib/features/agents/AcpConfigPickerSection.svelte b/apps/staged/src/lib/features/agents/AcpConfigPickerSection.svelte index 562c8bbe8..20c484644 100644 --- a/apps/staged/src/lib/features/agents/AcpConfigPickerSection.svelte +++ b/apps/staged/src/lib/features/agents/AcpConfigPickerSection.svelte @@ -61,11 +61,6 @@ {/if} diff --git a/apps/staged/src/lib/features/agents/AcpFixedConfigPicker.svelte b/apps/staged/src/lib/features/agents/AcpFixedConfigPicker.svelte index 550c75d73..a46094990 100644 --- a/apps/staged/src/lib/features/agents/AcpFixedConfigPicker.svelte +++ b/apps/staged/src/lib/features/agents/AcpFixedConfigPicker.svelte @@ -1,13 +1,10 @@ {#if shouldRender} - - - - {triggerLabel} - {#if loading} - - {:else} - - {/if} - - handleAcpPickerOpenAutoFocus(event, contentEl)} - onkeydowncapture={handlePickerKeydown} - > - {#if hasPickerColumns} -
- {#if modelSelector} -
- onModelChange?.(value)} - /> -
- {/if} - - {#if effortSelector} -
- onEffortChange?.(value)} - /> -
- {/if} - - {#if loading && !modelSelector && !effortSelector} -
- - - - Loading options… - - -
- {/if} -
- {/if} + + {#if modelSelector} +
+ onModelChange?.(value)} + /> +
+ {/if} + + {#if effortSelector} +
+ onEffortChange?.(value)} + /> +
+ {/if} + + {#if loading && !modelSelector && !effortSelector} +
+ + + + Loading options… + + +
+ {/if} + {#snippet footer()} {#if error && !modelSelector && !effortSelector} {#if hasPickerColumns} @@ -158,60 +126,6 @@ Using provider defaults {/if} - - + {/snippet} + {/if} - - From 0fc6b4b7f31bf136b87524ba622bf1e9626e1a15 Mon Sep 17 00:00:00 2001 From: Matt Toohey Date: Wed, 8 Jul 2026 12:28:13 +1000 Subject: [PATCH 25/34] fix(acp): refresh effort options by model Signed-off-by: Matt Toohey --- apps/staged/src-tauri/src/session_commands.rs | 238 ++++++++++++++++-- apps/staged/src-tauri/src/web_server.rs | 11 +- apps/staged/src/lib/commands.test.ts | 27 ++ apps/staged/src/lib/commands.ts | 2 + .../features/agents/AcpConfigPicker.svelte | 53 +++- .../agents/AcpFixedConfigPicker.svelte | 2 +- .../agents/acpConfigSelection.test.ts | 22 ++ .../features/sessions/SessionChatPane.svelte | 95 ++++++- crates/acp-client/src/driver.rs | 146 ++++++++++- 9 files changed, 551 insertions(+), 45 deletions(-) diff --git a/apps/staged/src-tauri/src/session_commands.rs b/apps/staged/src-tauri/src/session_commands.rs index fbee64f1a..f64fd5c96 100644 --- a/apps/staged/src-tauri/src/session_commands.rs +++ b/apps/staged/src-tauri/src/session_commands.rs @@ -25,7 +25,11 @@ use std::time::{Duration, Instant}; use agent_client_protocol::{ schema::{ - v1::{Implementation, InitializeRequest, NewSessionRequest}, + v1::{ + Implementation, InitializeRequest, NewSessionRequest, SessionConfigKind, + SessionConfigOption, SessionConfigOptionCategory, SessionConfigSelectOptions, + SetSessionConfigOptionRequest, + }, ProtocolVersion, }, ByteStreams, Client, ConnectTo, ErrorCode, @@ -396,6 +400,54 @@ fn resolve_acp_config_discovery_working_dir(working_dir: Option) -> Path .unwrap_or_else(std::env::temp_dir) } +fn normalize_selected_model_value(selected_model_value: Option) -> Option { + selected_model_value + .as_deref() + .map(str::trim) + .filter(|value| !value.is_empty()) + .map(str::to_string) +} + +fn resolve_model_config_id_for_discovery( + config_options: &[SessionConfigOption], + selected_model_value: &str, +) -> Result { + let Some(option) = config_options.iter().find(|option| { + option.category.as_ref() == Some(&SessionConfigOptionCategory::Model) + && matches!(&option.kind, SessionConfigKind::Select(_)) + }) else { + return Err("ACP config discovery did not return a model selector".to_string()); + }; + + if !select_config_option_has_value(option, selected_model_value) { + return Err(format!( + "ACP config discovery model value '{selected_model_value}' is no longer available for config option '{}'", + option.id + )); + } + + Ok(option.id.to_string()) +} + +fn select_config_option_has_value(option: &SessionConfigOption, value_id: &str) -> bool { + let SessionConfigKind::Select(select) = &option.kind else { + return false; + }; + + match &select.options { + SessionConfigSelectOptions::Ungrouped(options) => options + .iter() + .any(|option| option.value.to_string() == value_id), + SessionConfigSelectOptions::Grouped(groups) => groups.iter().any(|group| { + group + .options + .iter() + .any(|option| option.value.to_string() == value_id) + }), + _ => false, + } +} + fn acp_config_discovery_spawn_command( binary_path: &Path, acp_args: &[String], @@ -456,17 +508,20 @@ async fn run_acp_config_discovery_protocol( working_dir: &Path, stdin: tokio::process::ChildStdin, stdout: tokio::process::ChildStdout, + selected_model_value: Option<&str>, ) -> Result, String> { let stdin_compat = stdin.compat_write(); let stdout_compat = stdout.compat(); let transport = ByteStreams::new(stdin_compat, stdout_compat); - run_acp_config_discovery_transport(provider_id, working_dir, transport).await + run_acp_config_discovery_transport(provider_id, working_dir, transport, selected_model_value) + .await } async fn run_acp_config_discovery_transport( provider_id: &str, working_dir: &Path, transport: T, + selected_model_value: Option<&str>, ) -> Result, String> where T: ConnectTo + 'static, @@ -474,6 +529,7 @@ where let working_dir = working_dir.to_path_buf(); let provider_id = provider_id.to_string(); let protocol_provider_id = provider_id.clone(); + let selected_model_value = selected_model_value.map(str::to_string); Ok(Client .builder() @@ -521,7 +577,28 @@ where } }; - Ok(session_response.config_options.unwrap_or_default()) + let config_options = session_response.config_options.unwrap_or_default(); + let Some(selected_model_value) = selected_model_value.as_deref() else { + return Ok(config_options); + }; + + let model_config_id = + resolve_model_config_id_for_discovery(&config_options, selected_model_value)?; + let response = connection + .send_request(SetSessionConfigOptionRequest::new( + session_response.session_id.to_string(), + model_config_id, + selected_model_value, + )) + .block_task() + .await + .map_err(|e| { + format!( + "ACP config discovery failed to set selected model for {protocol_provider_id}: {e:?}" + ) + })?; + + Ok(response.config_options) }) .await .map_err(|_| { @@ -539,6 +616,7 @@ where async fn discover_acp_config_for_provider_async( provider_id: String, working_dir: PathBuf, + selected_model_value: Option, ) -> Result { let agent = acp_client::find_acp_agent_by_id(&provider_id) .ok_or_else(|| format!("Unknown or unavailable agent provider: {provider_id}"))?; @@ -603,8 +681,14 @@ async fn discover_acp_config_for_provider_async( .take() .ok_or_else(|| "Failed to get ACP config discovery stdout".to_string())?; - let config_options = - run_acp_config_discovery_protocol(&provider_id, &working_dir, stdin, stdout).await; + let config_options = run_acp_config_discovery_protocol( + &provider_id, + &working_dir, + stdin, + stdout, + selected_model_value.as_deref(), + ) + .await; let _ = child.kill().await; let _ = child.wait().await; @@ -619,8 +703,10 @@ async fn discover_acp_config_for_provider_async( fn discover_acp_config_for_provider( provider_id: String, working_dir: PathBuf, + selected_model_value: Option, ) -> Result { let provider_id = normalized_acp_config_provider_id(&provider_id)?; + let selected_model_value = normalize_selected_model_value(selected_model_value); let handle = std::thread::spawn(move || { let rt = tokio::runtime::Builder::new_current_thread() @@ -631,7 +717,7 @@ fn discover_acp_config_for_provider( let local = tokio::task::LocalSet::new(); local.block_on( &rt, - discover_acp_config_for_provider_async(provider_id, working_dir), + discover_acp_config_for_provider_async(provider_id, working_dir, selected_model_value), ) }); @@ -640,6 +726,13 @@ fn discover_acp_config_for_provider( .map_err(|_| "ACP config discovery thread panicked".to_string())? } +fn discover_acp_config_for_provider_default( + provider_id: String, + working_dir: PathBuf, +) -> Result { + discover_acp_config_for_provider(provider_id, working_dir, None) +} + fn discover_acp_config_for_provider_with_cache( provider_id: String, working_dir: PathBuf, @@ -695,16 +788,22 @@ pub async fn discover_acp_config( provider_id: String, working_dir: Option, force: Option, + selected_model_value: Option, ) -> Result { let working_dir = resolve_acp_config_discovery_working_dir(working_dir); let force = force.unwrap_or(false); + let selected_model_value = normalize_selected_model_value(selected_model_value); tokio::task::spawn_blocking(move || { - discover_acp_config_for_provider_with_cache( - provider_id, - working_dir, - force, - discover_acp_config_for_provider, - ) + if selected_model_value.is_some() { + discover_acp_config_for_provider(provider_id, working_dir, selected_model_value) + } else { + discover_acp_config_for_provider_with_cache( + provider_id, + working_dir, + force, + discover_acp_config_for_provider_default, + ) + } }) .await .map_err(|e| format!("ACP config discovery task failed: {e}"))? @@ -5622,9 +5721,10 @@ mod tests { agent_client_protocol::on_receive_request!(), ); - let options = run_acp_config_discovery_transport("test-provider", Path::new("/tmp"), agent) - .await - .unwrap(); + let options = + run_acp_config_discovery_transport("test-provider", Path::new("/tmp"), agent, None) + .await + .unwrap(); assert_eq!(options.len(), 1); assert!(!auth_called.load(Ordering::SeqCst)); @@ -5665,14 +5765,116 @@ mod tests { agent_client_protocol::on_receive_request!(), ); - let options = run_acp_config_discovery_transport("test-provider", Path::new("/tmp"), agent) - .await - .unwrap(); + let options = + run_acp_config_discovery_transport("test-provider", Path::new("/tmp"), agent, None) + .await + .unwrap(); assert!(options.is_empty()); assert!(!auth_called.load(Ordering::SeqCst)); } + #[tokio::test(flavor = "current_thread")] + async fn acp_config_discovery_with_selected_model_returns_post_set_config_options() { + use agent_client_protocol::schema::v1::{ + InitializeRequest, InitializeResponse, NewSessionRequest, NewSessionResponse, + SessionConfigOption, SessionConfigOptionCategory, SessionConfigSelectOption, + SetSessionConfigOptionRequest, SetSessionConfigOptionResponse, + }; + + let calls = Arc::new(Mutex::new(Vec::<(String, String)>::new())); + let calls_for_handler = Arc::clone(&calls); + let agent = agent_client_protocol::Agent + .builder() + .on_receive_request( + async |initialize: InitializeRequest, responder, _cx| { + responder.respond(InitializeResponse::new(initialize.protocol_version)) + }, + agent_client_protocol::on_receive_request!(), + ) + .on_receive_request( + async |_request: NewSessionRequest, responder, _cx| { + responder.respond(NewSessionResponse::new("discovery-session").config_options( + vec![ + SessionConfigOption::select( + "model", + "Model", + "sonnet", + vec![ + SessionConfigSelectOption::new("sonnet", "Sonnet"), + SessionConfigSelectOption::new("opus", "Opus"), + ], + ) + .category(SessionConfigOptionCategory::Model), + SessionConfigOption::select( + "reasoning", + "Reasoning", + "high", + vec![ + SessionConfigSelectOption::new("low", "Low"), + SessionConfigSelectOption::new("high", "High"), + ], + ) + .category(SessionConfigOptionCategory::ThoughtLevel), + ], + )) + }, + agent_client_protocol::on_receive_request!(), + ) + .on_receive_request( + async move |request: SetSessionConfigOptionRequest, responder, _cx| { + calls_for_handler.lock().unwrap().push(( + request.config_id.to_string(), + request + .value + .as_value_id() + .expect("selected config value should be a value ID") + .to_string(), + )); + responder.respond(SetSessionConfigOptionResponse::new(vec![ + SessionConfigOption::select( + "model", + "Model", + "opus", + vec![ + SessionConfigSelectOption::new("sonnet", "Sonnet"), + SessionConfigSelectOption::new("opus", "Opus"), + ], + ) + .category(SessionConfigOptionCategory::Model), + SessionConfigOption::select( + "reasoning", + "Reasoning", + "low", + vec![SessionConfigSelectOption::new("low", "Low")], + ) + .category(SessionConfigOptionCategory::ThoughtLevel), + ])) + }, + agent_client_protocol::on_receive_request!(), + ); + + let options = run_acp_config_discovery_transport( + "test-provider", + Path::new("/tmp"), + agent, + Some("opus"), + ) + .await + .unwrap(); + let discovery = acp_config_discovery_from_options("test-provider".to_string(), &options); + + assert_eq!( + calls.lock().unwrap().as_slice(), + &[(String::from("model"), String::from("opus"))] + ); + assert_eq!(discovery.model.as_ref().unwrap().current_value_id, "opus"); + let effort = discovery.effort.as_ref().unwrap(); + assert_eq!(effort.current_value_id, "low"); + assert_eq!(effort.options.len(), 1); + assert_eq!(effort.options[0].value_id, "low"); + } + #[test] fn acp_config_discovery_cache_is_provider_scoped_across_working_dirs() { let provider_id = "cache-provider-scoped-dirs"; diff --git a/apps/staged/src-tauri/src/web_server.rs b/apps/staged/src-tauri/src/web_server.rs index 3a6753388..66eaeaff6 100644 --- a/apps/staged/src-tauri/src/web_server.rs +++ b/apps/staged/src-tauri/src/web_server.rs @@ -2762,9 +2762,14 @@ async fn dispatch(command: &str, args: Value, state: &WebAppState) -> Result = opt_arg(&args, "workingDir")?; let force: Option = opt_arg(&args, "force")?; - let config = - crate::session_commands::discover_acp_config(provider_id, working_dir, force) - .await?; + let selected_model_value: Option = opt_arg(&args, "selectedModelValue")?; + let config = crate::session_commands::discover_acp_config( + provider_id, + working_dir, + force, + selected_model_value, + ) + .await?; Ok(serde_json::to_value(config).unwrap()) } "get_session" => { diff --git a/apps/staged/src/lib/commands.test.ts b/apps/staged/src/lib/commands.test.ts index 4dec45b8d..9db0ed4dd 100644 --- a/apps/staged/src/lib/commands.test.ts +++ b/apps/staged/src/lib/commands.test.ts @@ -460,11 +460,13 @@ describe('cached mutation command wrappers', () => { providerId: 'goose', workingDir: '/repo', force: false, + selectedModelValue: null, }); expect(invokeCommand).toHaveBeenNthCalledWith(2, 'discover_acp_config', { providerId: 'goose', workingDir: '/other-repo', force: false, + selectedModelValue: null, }); expect(cachedCommand).not.toHaveBeenCalledWith( 'discover_acp_config', @@ -491,6 +493,31 @@ describe('cached mutation command wrappers', () => { providerId: 'goose', workingDir: null, force: true, + selectedModelValue: null, + }); + }); + + it('passes selected ACP model discovery through to the backend command', async () => { + const config = { + providerId: 'goose', + model: null, + effort: null, + }; + invokeCommand.mockResolvedValue(config); + + const { discoverAcpConfig } = await import('./commands'); + + await expect( + discoverAcpConfig('goose', '/repo', { selectedModelValue: 'opus' }) + ).resolves.toEqual({ + data: config, + revalidating: null, + }); + expect(invokeCommand).toHaveBeenCalledWith('discover_acp_config', { + providerId: 'goose', + workingDir: '/repo', + force: false, + selectedModelValue: 'opus', }); }); }); diff --git a/apps/staged/src/lib/commands.ts b/apps/staged/src/lib/commands.ts index cdc4ffee4..27b35a8a4 100644 --- a/apps/staged/src/lib/commands.ts +++ b/apps/staged/src/lib/commands.ts @@ -719,6 +719,7 @@ export interface DiscoverAcpProvidersOptions { export interface DiscoverAcpConfigOptions { force?: boolean; + selectedModelValue?: string | null; } const ACP_PROVIDER_CACHE_TTL = 30 * 60_000; @@ -742,6 +743,7 @@ export async function discoverAcpConfig( providerId, workingDir: workingDir ?? null, force: options.force ?? false, + selectedModelValue: options.selectedModelValue ?? null, }); return { data, revalidating: null }; } diff --git a/apps/staged/src/lib/features/agents/AcpConfigPicker.svelte b/apps/staged/src/lib/features/agents/AcpConfigPicker.svelte index 529d51520..97a189b4a 100644 --- a/apps/staged/src/lib/features/agents/AcpConfigPicker.svelte +++ b/apps/staged/src/lib/features/agents/AcpConfigPicker.svelte @@ -102,6 +102,12 @@ const run = ++discoveryRun; let cancelled = false; + selectedModelValue = null; + selectedEffortValue = null; + modelSelectionExplicit = false; + effortSelectionExplicit = false; + modelSelectorKey = null; + effortSelectorKey = null; config = null; configLoading = true; configError = null; @@ -139,8 +145,10 @@ $effect(() => { const nextKey = selectorKey(modelSelector); if (nextKey !== modelSelectorKey) { - selectedModelValue = defaultSelectorValue(modelSelector); - modelSelectionExplicit = false; + if (!(modelSelectionExplicit && selectorHasValue(modelSelector, selectedModelValue))) { + selectedModelValue = defaultSelectorValue(modelSelector); + modelSelectionExplicit = false; + } modelSelectorKey = nextKey; } }); @@ -165,6 +173,41 @@ function handleModelChange(value: string) { selectedModelValue = value; modelSelectionExplicit = true; + selectedEffortValue = null; + effortSelectionExplicit = false; + config = config ? { ...config, effort: null } : config; + + const providerId = selectedProviderId; + const discoveryWorkingDir = workingDir ?? null; + if (remote || !providerId) return; + + const run = ++discoveryRun; + configLoading = true; + configError = null; + + discoverAcpConfig(providerId, discoveryWorkingDir, { selectedModelValue: value }) + .then(({ data, revalidating }) => { + if (run !== discoveryRun) return; + config = data; + configLoading = false; + revalidating + ?.then((fresh) => { + if (run === discoveryRun) { + config = fresh; + } + }) + .catch((error) => { + if (run === discoveryRun) { + console.error('Failed to revalidate ACP config for selected model:', error); + } + }); + }) + .catch((error) => { + if (run !== discoveryRun) return; + console.error('Failed to discover ACP config for selected model:', error); + configLoading = false; + configError = error instanceof Error ? error.message : String(error); + }); } function handleEffortChange(value: string) { @@ -186,6 +229,10 @@ return selector.options[0]?.valueId ?? null; } + function selectorHasValue(selector: AcpConfigSelector | null, valueId: string | null): boolean { + return !!selector && !!valueId && selector.options.some((option) => option.valueId === valueId); + } + function selectorValueLabel( selector: AcpConfigSelector | null, valueId: string | null @@ -260,7 +307,7 @@
{/if} - {#if configLoading && !modelSelector && !effortSelector} + {#if configLoading && (!modelSelector || !effortSelector)}
diff --git a/apps/staged/src/lib/features/agents/AcpFixedConfigPicker.svelte b/apps/staged/src/lib/features/agents/AcpFixedConfigPicker.svelte index a46094990..b3fa38fa7 100644 --- a/apps/staged/src/lib/features/agents/AcpFixedConfigPicker.svelte +++ b/apps/staged/src/lib/features/agents/AcpFixedConfigPicker.svelte @@ -106,7 +106,7 @@
{/if} - {#if loading && !modelSelector && !effortSelector} + {#if loading && (!modelSelector || !effortSelector)}
diff --git a/apps/staged/src/lib/features/agents/acpConfigSelection.test.ts b/apps/staged/src/lib/features/agents/acpConfigSelection.test.ts index 557bb5eff..c8dacb0fc 100644 --- a/apps/staged/src/lib/features/agents/acpConfigSelection.test.ts +++ b/apps/staged/src/lib/features/agents/acpConfigSelection.test.ts @@ -58,6 +58,28 @@ describe('buildAcpConfigSelection', () => { }); }); + it('omits effort after a model change until effort is explicitly reselected', () => { + const effort = selector({ + configId: 'reasoning_effort', + label: 'Effort', + currentValueId: 'medium', + options: [ + { valueId: 'medium', label: 'Medium' }, + { valueId: 'high', label: 'High' }, + ], + }); + + expect( + buildAcpConfigSelection({ + model: { selector: selector(), valueId: 'opus', explicit: true }, + effort: { selector: effort, valueId: 'medium', explicit: false }, + }) + ).toEqual({ + model: { configId: 'model', valueId: 'opus', label: 'Opus' }, + effort: null, + }); + }); + it('does not remap stale selected values to the current selector value', () => { expect( buildAcpConfigSelection({ diff --git a/apps/staged/src/lib/features/sessions/SessionChatPane.svelte b/apps/staged/src/lib/features/sessions/SessionChatPane.svelte index 64b75f4b6..98d317ef8 100644 --- a/apps/staged/src/lib/features/sessions/SessionChatPane.svelte +++ b/apps/staged/src/lib/features/sessions/SessionChatPane.svelte @@ -203,6 +203,9 @@ let displayRoots = $state([]); let currentDisplayRootKey = ''; let discoveredFollowupConfig = $state(null); + let modelSpecificFollowupConfig = $state(null); + let modelSpecificFollowupModelValue = $state(null); + let modelSpecificFollowupSessionId = $state(null); let followupConfigLoading = $state(false); let followupConfigError = $state(null); let selectedFollowupModelValue = $state(null); @@ -240,12 +243,29 @@ let rediscoveredFollowupConfig = $derived( discoveredFollowupConfig?.providerId === session?.provider ? discoveredFollowupConfig : null ); - let followupModelSelector = $derived( - metadataFollowupConfig?.model ?? rediscoveredFollowupConfig?.model ?? null + let usesModelSpecificFollowupConfig = $derived( + !!modelSpecificFollowupModelValue && + modelSpecificFollowupModelValue === selectedFollowupModelValue && + modelSpecificFollowupSessionId === session?.id && + followupModelTouched ); - let followupEffortSelector = $derived( - metadataFollowupConfig?.effort ?? rediscoveredFollowupConfig?.effort ?? null + let activeModelSpecificFollowupConfig = $derived( + usesModelSpecificFollowupConfig && modelSpecificFollowupConfig?.providerId === session?.provider + ? modelSpecificFollowupConfig + : null ); + let followupModelSelector = $derived( + activeModelSpecificFollowupConfig?.model ?? + metadataFollowupConfig?.model ?? + rediscoveredFollowupConfig?.model ?? + null + ); + let followupEffortSelector = $derived.by(() => { + if (usesModelSpecificFollowupConfig) { + return activeModelSpecificFollowupConfig?.effort ?? null; + } + return metadataFollowupConfig?.effort ?? rediscoveredFollowupConfig?.effort ?? null; + }); let followupProviderLabel = $derived( agentState.providers.find((provider) => provider.id === session?.provider)?.label ?? session?.provider ?? @@ -269,7 +289,9 @@ explicit: shouldSendFollowupSelectorSelection( followupEffortSelector, selectedFollowupEffortValue, - session?.acpConfigSelection?.effort ?? null, + usesModelSpecificFollowupConfig && !followupEffortTouched + ? null + : (session?.acpConfigSelection?.effort ?? null), followupEffortTouched ), }, @@ -778,6 +800,9 @@ status: 'running', acpConfigSelection: acpConfigSelection ?? session.acpConfigSelection ?? null, }; + modelSpecificFollowupConfig = null; + modelSpecificFollowupModelValue = null; + modelSpecificFollowupSessionId = null; startPolling(); scrollToBottom(); } catch (e) { @@ -1237,6 +1262,11 @@ discoveredFollowupConfig = null; followupConfigLoading = false; followupConfigError = null; + if (!active || !providerId) { + modelSpecificFollowupConfig = null; + modelSpecificFollowupModelValue = null; + modelSpecificFollowupSessionId = null; + } return; } @@ -1282,11 +1312,15 @@ session?.acpConfigSelection?.model ?? null ); if (key !== followupModelStateKey) { - selectedFollowupModelValue = initialSelectorValue( - followupModelSelector, - session?.acpConfigSelection?.model ?? null - ); - followupModelTouched = false; + if (!( + followupModelTouched && selectorHasValue(followupModelSelector, selectedFollowupModelValue) + )) { + selectedFollowupModelValue = initialSelectorValue( + followupModelSelector, + session?.acpConfigSelection?.model ?? null + ); + followupModelTouched = false; + } followupModelStateKey = key; } }); @@ -1346,6 +1380,10 @@ return selector.options[0]?.valueId ?? null; } + function selectorHasValue(selector: AcpConfigSelector | null, valueId: string | null): boolean { + return !!selector && !!valueId && selector.options.some((option) => option.valueId === valueId); + } + function shouldSendFollowupSelectorSelection( selector: AcpConfigSelector | null, valueId: string | null, @@ -1367,6 +1405,43 @@ function handleFollowupModelChange(value: string) { selectedFollowupModelValue = value; followupModelTouched = true; + selectedFollowupEffortValue = null; + followupEffortTouched = false; + modelSpecificFollowupConfig = null; + modelSpecificFollowupModelValue = value; + modelSpecificFollowupSessionId = session?.id ?? null; + + const providerId = session?.provider ?? null; + const workingDir = session?.workingDir ?? null; + if (!active || !providerId) return; + + const run = ++followupDiscoveryRun; + followupConfigLoading = true; + followupConfigError = null; + + discoverAcpConfig(providerId, workingDir, { selectedModelValue: value }) + .then(({ data, revalidating }) => { + if (run !== followupDiscoveryRun) return; + modelSpecificFollowupConfig = data; + followupConfigLoading = false; + revalidating + ?.then((fresh) => { + if (run === followupDiscoveryRun) { + modelSpecificFollowupConfig = fresh; + } + }) + .catch((error) => { + if (run === followupDiscoveryRun) { + console.error('Failed to revalidate ACP config for selected model:', error); + } + }); + }) + .catch((error) => { + if (run !== followupDiscoveryRun) return; + console.error('Failed to discover ACP config for selected model:', error); + followupConfigLoading = false; + followupConfigError = error instanceof Error ? error.message : String(error); + }); } function handleFollowupEffortChange(value: string) { diff --git a/crates/acp-client/src/driver.rs b/crates/acp-client/src/driver.rs index c5e1e609d..6aff7feec 100644 --- a/crates/acp-client/src/driver.rs +++ b/crates/acp-client/src/driver.rs @@ -2132,8 +2132,19 @@ async fn apply_or_record_session_config_options( })? .to_vec(); + let mut model_selection_applied = false; for selection in selections { - let config_id = resolve_session_config_option_selection(&latest_options, selection)?; + let config_id = match resolve_session_config_option_selection(&latest_options, selection) { + Ok(config_id) => config_id, + Err(e) + if model_selection_applied + && selection.category == SessionConfigOptionCategory::ThoughtLevel => + { + log::warn!("Skipping stale ACP effort selection after model change: {e}"); + continue; + } + Err(e) => return Err(e), + }; let response = connection .send_request(SetSessionConfigOptionRequest::new( agent_session_id.to_string(), @@ -2149,6 +2160,9 @@ async fn apply_or_record_session_config_options( ) })?; latest_options = response.config_options; + if selection.category == SessionConfigOptionCategory::Model { + model_selection_applied = true; + } } writer.on_config_option_update(&latest_options).await; @@ -2597,21 +2611,22 @@ impl MessageWriter for BasicMessageWriter { #[cfg(test)] mod tests { use super::{ - acp_spawn_command, autoapprove_permission_decision, build_prompt_content_blocks, - consume_remote_acp_line, decode_remote_acp_line, mcp_server_transport_supported, - permission_response_for_options, remote_acp_segments, resolve_acp_working_dir, - resolve_session_config_option_selection, resolve_spawn_working_dir, - sanitize_remote_acp_chunk, shell_exec_line, shell_quote, AcpDriver, AcpEventMetadata, - AcpNotificationHandler, AcpPermissionOption, AcpPermissionOptionKind, AcpPermissionRequest, - AcpSessionConfigOptionSelection, AgentRunOutcome, BasicMessageWriter, MessageWriter, - RemoteLineOutcome, ReplayBoundary, ReplayBuffer, ReplayEvent, + acp_spawn_command, apply_or_record_session_config_options, autoapprove_permission_decision, + build_prompt_content_blocks, consume_remote_acp_line, decode_remote_acp_line, + mcp_server_transport_supported, permission_response_for_options, remote_acp_segments, + resolve_acp_working_dir, resolve_session_config_option_selection, + resolve_spawn_working_dir, sanitize_remote_acp_chunk, shell_exec_line, shell_quote, + AcpDriver, AcpEventMetadata, AcpNotificationHandler, AcpPermissionOption, + AcpPermissionOptionKind, AcpPermissionRequest, AcpSessionConfigOptionSelection, + AgentRunOutcome, BasicMessageWriter, MessageWriter, RemoteLineOutcome, ReplayBoundary, + ReplayBuffer, ReplayEvent, }; use agent_client_protocol::schema::v1::{ ContentBlock as AcpContentBlock, McpCapabilities, McpServer, McpServerHttp, McpServerSse, McpServerStdio, PermissionOption, PermissionOptionKind, Plan, PlanEntry, PlanEntryPriority, PlanEntryStatus, RequestPermissionOutcome, SessionConfigOption, SessionConfigOptionCategory, SessionConfigSelectOption, SessionNotification, SessionUpdate, - StopReason, + SetSessionConfigOptionRequest, SetSessionConfigOptionResponse, StopReason, }; use std::ffi::OsString; use std::path::{Path, PathBuf}; @@ -2677,6 +2692,109 @@ mod tests { assert!(error.contains("Selected ACP effort value 'high' is no longer available")); } + #[tokio::test(flavor = "current_thread")] + async fn skips_stale_effort_after_applying_selected_model() { + let calls = Arc::new(Mutex::new(Vec::<(String, String)>::new())); + let calls_for_handler = Arc::clone(&calls); + let agent = agent_client_protocol::Agent.builder().on_receive_request( + async move |request: SetSessionConfigOptionRequest, responder, _cx| { + calls_for_handler.lock().unwrap().push(( + request.config_id.to_string(), + request + .value + .as_value_id() + .expect("selected config value should be a value ID") + .to_string(), + )); + responder.respond(SetSessionConfigOptionResponse::new(vec![ + SessionConfigOption::select( + "model", + "Model", + "opus", + vec![ + SessionConfigSelectOption::new("sonnet", "Sonnet"), + SessionConfigSelectOption::new("opus", "Opus"), + ], + ) + .category(SessionConfigOptionCategory::Model), + SessionConfigOption::select( + "reasoning", + "Reasoning", + "low", + vec![SessionConfigSelectOption::new("low", "Low")], + ) + .category(SessionConfigOptionCategory::ThoughtLevel), + ])) + }, + agent_client_protocol::on_receive_request!(), + ); + let writer = Arc::new(RecordingMessageWriter::default()); + let message_writer: Arc = writer.clone(); + let initial_options = vec![ + SessionConfigOption::select( + "model", + "Model", + "sonnet", + vec![ + SessionConfigSelectOption::new("sonnet", "Sonnet"), + SessionConfigSelectOption::new("opus", "Opus"), + ], + ) + .category(SessionConfigOptionCategory::Model), + SessionConfigOption::select( + "reasoning", + "Reasoning", + "high", + vec![ + SessionConfigSelectOption::new("low", "Low"), + SessionConfigSelectOption::new("high", "High"), + ], + ) + .category(SessionConfigOptionCategory::ThoughtLevel), + ]; + let selections = vec![ + AcpSessionConfigOptionSelection { + category: SessionConfigOptionCategory::Model, + config_id: "model".to_string(), + value_id: "opus".to_string(), + }, + AcpSessionConfigOptionSelection { + category: SessionConfigOptionCategory::ThoughtLevel, + config_id: "reasoning".to_string(), + value_id: "high".to_string(), + }, + ]; + + agent_client_protocol::Client + .builder() + .name("acp-config-test") + .connect_with(agent, async |connection| { + apply_or_record_session_config_options( + &connection, + "session-1", + Some(&initial_options), + &selections, + &message_writer, + ) + .await + .map_err(agent_client_protocol::util::internal_error) + }) + .await + .expect("protocol should succeed"); + + assert_eq!( + calls.lock().unwrap().as_slice(), + &[(String::from("model"), String::from("opus"))] + ); + let updates = writer.config_option_updates.lock().unwrap(); + assert_eq!(updates.len(), 1); + assert_eq!( + resolve_session_config_option_selection(&updates[0], &selections[1]) + .expect_err("stale effort should remain unavailable"), + "Selected ACP effort value 'high' is no longer available for config option 'reasoning'" + ); + } + fn write_executable(path: &Path, content: &str) { if let Some(parent) = path.parent() { std::fs::create_dir_all(parent).expect("create executable parent"); @@ -2731,6 +2849,7 @@ mod tests { #[derive(Default)] struct RecordingMessageWriter { events: Mutex>, + config_option_updates: Mutex>>, } #[async_trait::async_trait] @@ -2760,6 +2879,13 @@ mod tests { async fn record_acp_event_metadata(&self, metadata: AcpEventMetadata) { self.events.lock().unwrap().push(metadata); } + + async fn on_config_option_update(&self, options: &[SessionConfigOption]) { + self.config_option_updates + .lock() + .unwrap() + .push(options.to_vec()); + } } fn test_plan() -> Plan { From e6df1be5e12eff7f21377bbbc383f192cdf61021 Mon Sep 17 00:00:00 2001 From: Matt Toohey Date: Wed, 8 Jul 2026 14:21:03 +1000 Subject: [PATCH 26/34] fix(acp): cache effort options by model Signed-off-by: Matt Toohey --- apps/staged/src-tauri/src/session_commands.rs | 255 +++++++++++++++++- 1 file changed, 253 insertions(+), 2 deletions(-) diff --git a/apps/staged/src-tauri/src/session_commands.rs b/apps/staged/src-tauri/src/session_commands.rs index f64fd5c96..3bdad5661 100644 --- a/apps/staged/src-tauri/src/session_commands.rs +++ b/apps/staged/src-tauri/src/session_commands.rs @@ -358,11 +358,19 @@ struct AcpConfigDiscoveryCacheEntry { static ACP_CONFIG_DISCOVERY_CACHE: OnceLock>> = OnceLock::new(); +static ACP_CONFIG_DISCOVERY_MODEL_CACHE: OnceLock< + Mutex>, +> = OnceLock::new(); fn acp_config_discovery_cache() -> &'static Mutex> { ACP_CONFIG_DISCOVERY_CACHE.get_or_init(|| Mutex::new(HashMap::new())) } +fn acp_config_discovery_model_cache( +) -> &'static Mutex> { + ACP_CONFIG_DISCOVERY_MODEL_CACHE.get_or_init(|| Mutex::new(HashMap::new())) +} + fn normalized_acp_config_provider_id(provider_id: &str) -> Result { let provider_id = provider_id.trim().to_string(); if provider_id.is_empty() { @@ -371,6 +379,10 @@ fn normalized_acp_config_provider_id(provider_id: &str) -> Result String { + format!("{provider_id}\0{selected_model_value}") +} + #[derive(Debug)] struct AcpConfigDiscoverySpawnCommand { program: PathBuf, @@ -781,6 +793,64 @@ where } } +fn discover_acp_config_for_model_with_cache( + provider_id: String, + working_dir: PathBuf, + selected_model_value: String, + force: bool, + fetch: F, +) -> Result +where + F: FnOnce(String, PathBuf, Option) -> Result, +{ + let provider_id = normalized_acp_config_provider_id(&provider_id)?; + let selected_model_value = normalize_selected_model_value(Some(selected_model_value)) + .ok_or_else(|| { + "ACP model value is required for model-specific config discovery".to_string() + })?; + let cache_key = acp_config_discovery_model_cache_key(&provider_id, &selected_model_value); + let cached_entry = { + let cache = acp_config_discovery_model_cache().lock().unwrap(); + cache.get(&cache_key).cloned() + }; + + if !force { + if let Some(entry) = &cached_entry { + if entry.fetched_at.elapsed() < ACP_CONFIG_DISCOVERY_CACHE_TTL { + return Ok(entry.discovery.clone()); + } + } + } + + match fetch( + provider_id.clone(), + working_dir, + Some(selected_model_value.clone()), + ) { + Ok(discovery) => { + let mut cache = acp_config_discovery_model_cache().lock().unwrap(); + cache.insert( + cache_key, + AcpConfigDiscoveryCacheEntry { + discovery: discovery.clone(), + fetched_at: Instant::now(), + }, + ); + Ok(discovery) + } + Err(e) => { + if let Some(entry) = cached_entry { + log::warn!( + "ACP config discovery refresh failed for {provider_id}/{selected_model_value}; using stale cached options: {e}" + ); + Ok(entry.discovery) + } else { + Err(e) + } + } + } +} + /// Discover model/effort selectors for a provider in the given working /// directory context. #[tauri::command(rename_all = "camelCase")] @@ -794,8 +864,14 @@ pub async fn discover_acp_config( let force = force.unwrap_or(false); let selected_model_value = normalize_selected_model_value(selected_model_value); tokio::task::spawn_blocking(move || { - if selected_model_value.is_some() { - discover_acp_config_for_provider(provider_id, working_dir, selected_model_value) + if let Some(selected_model_value) = selected_model_value { + discover_acp_config_for_model_with_cache( + provider_id, + working_dir, + selected_model_value, + force, + discover_acp_config_for_provider, + ) } else { discover_acp_config_for_provider_with_cache( provider_id, @@ -4932,6 +5008,29 @@ mod tests { } } + fn test_acp_config_discovery_with_effort( + provider_id: &str, + current_model_value_id: &str, + current_effort_value_id: &str, + effort_values: &[&str], + ) -> AcpConfigDiscovery { + let mut discovery = test_acp_config_discovery(provider_id, current_model_value_id); + discovery.effort = Some(crate::acp_config::NormalizedAcpConfigSelector { + config_id: "reasoning".to_string(), + label: "Reasoning".to_string(), + current_value_id: current_effort_value_id.to_string(), + options: effort_values + .iter() + .map(|value| crate::acp_config::NormalizedAcpConfigValueOption { + value_id: (*value).to_string(), + label: value.to_ascii_uppercase(), + group_label: None, + }) + .collect(), + }); + discovery + } + fn test_acp_config_value_selection( config_id: &str, value_id: &str, @@ -4957,6 +5056,12 @@ mod tests { .remove(provider_id); } + fn remove_acp_config_model_cache_entry(provider_id: &str, selected_model_value: &str) { + acp_config_discovery_model_cache().lock().unwrap().remove( + &acp_config_discovery_model_cache_key(provider_id, selected_model_value), + ); + } + fn create_auto_review( store: &Arc, branch_id: &str, @@ -5945,6 +6050,152 @@ mod tests { assert_eq!(third, second); } + #[test] + fn acp_config_discovery_model_cache_is_provider_and_model_scoped() { + let provider_id = "cache-provider-model-scoped"; + remove_acp_config_model_cache_entry(provider_id, "sonnet"); + remove_acp_config_model_cache_entry(provider_id, "opus"); + let calls = Arc::new(Mutex::new(Vec::<(PathBuf, Option)>::new())); + + let calls_for_sonnet = Arc::clone(&calls); + let sonnet = discover_acp_config_for_model_with_cache( + provider_id.to_string(), + PathBuf::from("/repo-one"), + "sonnet".to_string(), + false, + move |provider_id, working_dir, selected_model_value| { + calls_for_sonnet + .lock() + .unwrap() + .push((working_dir, selected_model_value)); + Ok(test_acp_config_discovery_with_effort( + &provider_id, + "sonnet", + "medium", + &["low", "medium"], + )) + }, + ) + .unwrap(); + let sonnet_cached = discover_acp_config_for_model_with_cache( + provider_id.to_string(), + PathBuf::from("/repo-two"), + "sonnet".to_string(), + false, + |_provider_id, _working_dir, _selected_model_value| { + panic!("fresh model cache hit should not refetch") + }, + ) + .unwrap(); + let calls_for_opus = Arc::clone(&calls); + let opus = discover_acp_config_for_model_with_cache( + provider_id.to_string(), + PathBuf::from("/repo-three"), + "opus".to_string(), + false, + move |provider_id, working_dir, selected_model_value| { + calls_for_opus + .lock() + .unwrap() + .push((working_dir, selected_model_value)); + Ok(test_acp_config_discovery_with_effort( + &provider_id, + "opus", + "low", + &["low"], + )) + }, + ) + .unwrap(); + + assert_eq!(sonnet_cached, sonnet); + assert_eq!( + sonnet + .effort + .as_ref() + .unwrap() + .options + .iter() + .map(|option| option.value_id.as_str()) + .collect::>(), + vec!["low", "medium"] + ); + assert_eq!( + opus.effort + .as_ref() + .unwrap() + .options + .iter() + .map(|option| option.value_id.as_str()) + .collect::>(), + vec!["low"] + ); + assert_eq!( + calls.lock().unwrap().as_slice(), + &[ + (PathBuf::from("/repo-one"), Some("sonnet".to_string())), + (PathBuf::from("/repo-three"), Some("opus".to_string())), + ] + ); + + remove_acp_config_model_cache_entry(provider_id, "sonnet"); + remove_acp_config_model_cache_entry(provider_id, "opus"); + } + + #[test] + fn acp_config_discovery_model_cache_does_not_replace_provider_cache() { + let provider_id = "cache-provider-model-does-not-poison-default"; + remove_acp_config_cache_entry(provider_id); + remove_acp_config_model_cache_entry(provider_id, "opus"); + + let default = discover_acp_config_for_provider_with_cache( + provider_id.to_string(), + PathBuf::from("/repo"), + false, + |provider_id, _working_dir| Ok(test_acp_config_discovery(&provider_id, "sonnet")), + ) + .unwrap(); + let model_specific = discover_acp_config_for_model_with_cache( + provider_id.to_string(), + PathBuf::from("/repo"), + "opus".to_string(), + false, + |provider_id, _working_dir, _selected_model_value| { + Ok(test_acp_config_discovery_with_effort( + &provider_id, + "opus", + "low", + &["low"], + )) + }, + ) + .unwrap(); + let default_again = discover_acp_config_for_provider_with_cache( + provider_id.to_string(), + PathBuf::from("/repo"), + false, + |_provider_id, _working_dir| panic!("provider cache should remain fresh"), + ) + .unwrap(); + + assert_eq!(default_again, default); + assert!(default_again.effort.is_none()); + assert_eq!( + model_specific + .effort + .as_ref() + .unwrap() + .options + .iter() + .map(|option| option.value_id.as_str()) + .collect::>(), + vec!["low"] + ); + + remove_acp_config_cache_entry(provider_id); + remove_acp_config_model_cache_entry(provider_id, "opus"); + } + #[test] fn acp_config_discovery_refresh_failure_returns_stale_cache() { let provider_id = "cache-provider-stale"; From 04d06802de205d3915d6a4a09dcfa199561061f1 Mon Sep 17 00:00:00 2001 From: Matt Toohey Date: Wed, 8 Jul 2026 14:36:58 +1000 Subject: [PATCH 27/34] fix(acp): stabilize picker loading layout Signed-off-by: Matt Toohey --- .../features/agents/AcpConfigPicker.svelte | 61 ++++++++++++-- .../agents/AcpConfigPickerShell.svelte | 84 ++++++++++++++++++- .../agents/AcpFixedConfigPicker.svelte | 72 ++++++++++++++-- 3 files changed, 200 insertions(+), 17 deletions(-) diff --git a/apps/staged/src/lib/features/agents/AcpConfigPicker.svelte b/apps/staged/src/lib/features/agents/AcpConfigPicker.svelte index 97a189b4a..b4b6fcc4b 100644 --- a/apps/staged/src/lib/features/agents/AcpConfigPicker.svelte +++ b/apps/staged/src/lib/features/agents/AcpConfigPicker.svelte @@ -52,6 +52,9 @@ let effortSelectionExplicit = $state(false); let modelSelectorKey = $state(null); let effortSelectorKey = $state(null); + let effortColumnWidth = $state(0); + let retainedEffortColumnWidth = $state(null); + let retainedEffortTriggerLabel = $state(null); let discoveryRun = 0; let agents = $derived(remote ? REMOTE_AGENTS : agentState.providers); @@ -59,16 +62,34 @@ let selectedProvider = $derived(agents.find((provider) => provider.id === selectedProviderId)); let modelSelector = $derived(config?.model ?? null); let effortSelector = $derived(config?.effort ?? null); + let loadingEffortOptions = $derived(configLoading && !!modelSelector && !effortSelector); + let modelTriggerLabel = $derived(selectorValueLabel(modelSelector, selectedModelValue)); + let effortTriggerLabel = $derived(selectorValueLabel(effortSelector, selectedEffortValue)); + let displayedEffortTriggerLabel = $derived( + effortTriggerLabel ?? (loadingEffortOptions ? retainedEffortTriggerLabel : null) + ); // The provider is identified by the trigger icon; the label only carries the // model/effort values, falling back to the provider name before discovery. let triggerParts = $derived( [ - selectorValueLabel(modelSelector, selectedModelValue), - selectorValueLabel(effortSelector, selectedEffortValue), - ].filter((part): part is string => !!part) + modelTriggerLabel ? { id: 'model', label: modelTriggerLabel } : null, + displayedEffortTriggerLabel ? { id: 'effort', label: displayedEffortTriggerLabel } : null, + ].filter((part): part is { id: string; label: string } => !!part) ); let triggerLabel = $derived( - triggerParts.length > 0 ? triggerParts.join(' · ') : (selectedProvider?.label ?? 'Agent') + triggerParts.length > 0 + ? triggerParts.map((part) => part.label).join(' · ') + : (selectedProvider?.label ?? 'Agent') + ); + let renderedTriggerParts = $derived( + triggerParts.length > 0 + ? triggerParts + : [{ id: 'provider', label: selectedProvider?.label ?? 'Agent' }] + ); + let effortColumnStyle = $derived( + loadingEffortOptions && retainedEffortColumnWidth + ? `width: ${retainedEffortColumnWidth}px;` + : undefined ); let canOpen = $derived( agents.length > 1 || !!modelSelector || !!effortSelector || configLoading || !!configError @@ -108,6 +129,9 @@ effortSelectionExplicit = false; modelSelectorKey = null; effortSelectorKey = null; + retainedEffortTriggerLabel = null; + retainedEffortColumnWidth = null; + effortColumnWidth = 0; config = null; configLoading = true; configError = null; @@ -162,6 +186,18 @@ } }); + $effect(() => { + if (!configLoading && effortTriggerLabel) { + retainedEffortTriggerLabel = effortTriggerLabel; + } + }); + + $effect(() => { + if (!configLoading && effortSelector && effortColumnWidth > 0) { + retainedEffortColumnWidth = Math.ceil(effortColumnWidth); + } + }); + $effect(() => { onSelectionChange?.(pickerSelection); }); @@ -250,6 +286,7 @@ +
+ {:else if loadingEffortOptions} +
+ Effort + + + + Loading options… + + +
{/if} - {#if configLoading && (!modelSelector || !effortSelector)} + {#if configLoading && !loadingEffortOptions && (!modelSelector || !effortSelector)}
- Loading options… + Loading options…
diff --git a/apps/staged/src/lib/features/agents/AcpConfigPickerShell.svelte b/apps/staged/src/lib/features/agents/AcpConfigPickerShell.svelte index ecd62f038..96d4f52a9 100644 --- a/apps/staged/src/lib/features/agents/AcpConfigPickerShell.svelte +++ b/apps/staged/src/lib/features/agents/AcpConfigPickerShell.svelte @@ -2,15 +2,22 @@ +{#snippet labelContent()} + + {#each renderedTriggerParts as part, index (part.id)} + {#if index > 0} + + {/if} + + {#key part.label} + + {part.label} + + {/key} + + {/each} + + +{/snippet} + {#if canOpen} - - {#each renderedTriggerParts as part, index (part.id)} - {#if index > 0} - - {/if} - - {#key part.label} - - {part.label} - - {/key} - - {/each} - + {@render labelContent()} {#if loading} {:else} @@ -123,34 +157,30 @@ title={triggerTitle} > - - {#each renderedTriggerParts as part, index (part.id)} - {#if index > 0} - - {/if} - - {#key part.label} - - {part.label} - - {/key} - - {/each} - + {@render labelContent()} {/if}