From 90d7e0df14d2f0d618f220359b783e0a72ffd47c Mon Sep 17 00:00:00 2001 From: Duncan Date: Mon, 10 Aug 2026 13:37:00 -0400 Subject: [PATCH] fix(pool): tri-state effort capability model survives live model switches MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace the populate-once PoolEffortCapabilities cache + capabilities_ever_discovered bool pair with a three-state EffortCapabilityState enum: Unknown — no session has returned capabilities yet (pre-first-session window, or capabilities reset after a model switch) Supported { config_id, valid_values } — current model advertises thought_level Unsupported — current model confirmed to not have thought_level The old model populated capabilities once (on the first session/new) and never refreshed them. After a live model switch — which resets agent.model_capabilities to None to discard the pre-switch snapshot — return_agent wrote Unknown/kept stale state, so handle_set_config_option_control kept validating against the old model's option set and session_config_captured emitted pre-switch configOptions to Desktop. Changes: pool.rs: - EffortCapabilityState enum replaces PoolEffortCapabilities + capabilities_ever_discovered - return_agent derives tri-state from the returning worker's model_capabilities on every return (not just the first), so the pool state always reflects the current model - AgentPool initialises with Unknown; struct fields updated accordingly pool.rs (create_session_and_apply_model): - Capabilities are populated from EVERY session/new response, not just when None - After a successful apply_model_switch, agent.model_capabilities is reset to None so return_agent writes Unknown rather than stale-Supported pool.rs (session_config_captured): - On the switch path, configOptions emitted as null so Desktop clears its picker state rather than caching pre-switch capability data lib.rs (handle_set_config_option_control): - Four-way A/B/C/D classifier replaced with a three-way tri-state dispatch - Supported(config_id matches) → validate + store (case A) - Supported(config_id no-match) → synthetic ok, non-effort (case B) - Unsupported → reject with unsupported_model status (eliminates case-C occupancy inference: capability loss is now surfaced immediately) - Unknown → pre-discovery trust path: category=thought_level stores + pending_session pool.rs field comment: - last_acked_effort_gen comment updated to name resolve_effort_report as the sole ack authority (Thufir Pass 3 non-blocking MINOR) Tests (lib.rs control_result_tests): - pool_with_capabilities_via_return helper drives the PRODUCTION return_agent path, not the test-only notify_capabilities_discovered shortcut - test_tristate_switch_effort_to_no_effort_rejects_pick: effort→no-effort switch writes Unsupported; subsequent pick is rejected with unsupported_model status - test_tristate_switch_no_effort_to_effort_accepts_pick: no-effort→effort switch writes Supported with new configId; pick validates and stores correctly - test_tristate_switch_new_options_validates_against_new_snapshot: both models Supported with different option sets; stale value rejected, new value accepted - test_tristate_unknown_after_switch_uses_pre_discovery_trust_path: capabilities=None on return writes Unknown; pre-discovery category trust path remains active Co-authored-by: Will Pfleger Signed-off-by: Will Pfleger --- crates/buzz-acp/src/lib.rs | 450 ++++++++++++++++++++++++++++++------ crates/buzz-acp/src/pool.rs | 212 ++++++++++------- 2 files changed, 505 insertions(+), 157 deletions(-) diff --git a/crates/buzz-acp/src/lib.rs b/crates/buzz-acp/src/lib.rs index fc371e43f4..75d4d0bfac 100644 --- a/crates/buzz-acp/src/lib.rs +++ b/crates/buzz-acp/src/lib.rs @@ -1256,27 +1256,24 @@ fn handle_set_config_option_control( // advertised in session/new (agentConfigCore.ts uses the one from the // session cache via deferredUntilNativeOptionsAvailable resolution). // - // V-2 fix: use pool-level capability cache instead of scanning idle agents. - // Checked-out workers leave None slots; the cache is written at return_agent + // V-2 fix: use pool-level capability state instead of scanning idle agents. + // Checked-out workers leave None slots; the state is written at return_agent // once the first worker completes a session and returns to the pool. // - // Four cases: - // A. effort_capabilities.config_id is Some and matches → full validation path - // B. effort_capabilities.config_id is None AND capabilities not yet discovered - // AND no category trust → unknown configId → synthetic ok (non-effort) - // C. effort_capabilities.config_id is None AND capabilities were discovered → - // all workers are currently busy; trust the incoming configId from the - // Desktop's session cache and store for apply at next checkout/session. - // D. effort_capabilities.config_id is None AND capabilities not yet discovered - // AND category == "thought_level" → pre-first-session window; Desktop - // sends its session-cache configId with category as the trust signal. - // Store the value and ack pending_session — the first session creation - // will apply it and emit the honest final ok/failure. - let thought_level_id = pool - .effort_capabilities - .config_id - .as_deref() - .map(str::to_string); + // Three-state classifier (replaces the former case A/B/C/D four-way): + // Supported(config_id, valid_values): + // incoming configId matches → full I-7 validation path (case A). + // incoming configId doesn't match → synthetic ok (non-effort, case B). + // Unsupported: + // current model confirmed to not have thought_level; reject the pick + // immediately so Desktop surfaces the capability loss rather than + // silently queuing a value that will never apply. + // Unknown: + // no session has returned capabilities yet (pre-first-session window, + // or capabilities reset after a model switch pending the next session/new). + // Trust the Desktop's category field as the trust signal (case D): + // if category == "thought_level", store and ack pending_session — + // the first session creation will apply it and emit the honest result. // Category from the incoming frame (Desktop sends "thought_level" for all // effort picks and clears, including during the pre-discovery window). @@ -1285,51 +1282,75 @@ fn handle_set_config_option_control( .and_then(|v| v.as_str()) .unwrap_or(""); - // Determine if this is a thought_level pick: - // Case A: cache has a matching configId. - // Case C: capabilities known from a prior session, all workers busy. - // Case D: pre-first-session, Desktop asserts category = "thought_level". - // - // For the clear path (empty value), the same cases apply. - let cache_matches = thought_level_id.as_deref() == Some(config_id); - // Case C: capabilities were discovered before (so configId is known) but - // workers are currently checked out and the cache is temporarily None. - let all_busy_with_known_caps = - pool.capabilities_ever_discovered && thought_level_id.is_none() && config_id != "unknown"; - // Case D: pre-first-session; Desktop sends category as the trust signal. - let pre_discovery_trusted = !pool.capabilities_ever_discovered - && frame_category == "thought_level" - && config_id != "unknown"; - let is_thought_level = cache_matches || all_busy_with_known_caps || pre_discovery_trusted; - - let (status, include_category) = if is_thought_level { - // I-7: validate the incoming value against adapter-advertised options - // from the pool-level capability cache. Works even when all workers - // are checked out — the cache was written at the first session creation. - // Skip validation when cache is empty (case C — workers busy, no cache - // to validate against; trust the Desktop's session-cache configId). - let valid_values = &pool.effort_capabilities.valid_values; - - if !valid_values.is_empty() - && !value.is_empty() - && !valid_values.contains(&value.to_string()) - { - // Value is not in the adapter's advertised option set. + // Tri-state dispatch: Supported → validate + store, Unsupported → reject, + // Unknown → pre-discovery trust path. + use crate::pool::EffortCapabilityState; + let (status, include_category) = match &pool.effort_capability_state { + EffortCapabilityState::Supported { + config_id: known_id, + valid_values, + } if known_id.as_str() == config_id => { + // Case A: configId matches the pool's known supported configId. + // I-7: validate value against adapter-advertised options. + if !valid_values.is_empty() + && !value.is_empty() + && !valid_values.contains(&value.to_string()) + { + tracing::warn!( + target: "pool::effort", + "effort value {value:?} not in advertised options {valid_values:?} — rejecting" + ); + let mut ack = serde_json::json!({ + "type": "set_config_option", + "configId": config_id, + "status": "invalid_value", + "value": value, + }); + ack["category"] = serde_json::json!("thought_level"); + if let Some(ref n) = nonce { + ack["nonce"] = serde_json::json!(n); + } + obs.emit( + "control_result", + None, + &observer::ObserverContext::default(), + ack, + ); + return; + } + // Value is valid (or no validation possible for clears). + if value.is_empty() { + pool.pending_effort_nonce = nonce.clone(); + pool.clear_pool_effort(); + ("pending_session", true) + } else { + pool.pending_effort_nonce = nonce.clone(); + let result = pool.set_pool_effort(config_id, value); + match result { + SetPoolEffortResult::Stored { .. } => ("pending_session", true), + } + } + } + EffortCapabilityState::Supported { .. } => { + // Case B: state is Supported but configId doesn't match the incoming + // frame — not an effort option; emit synthetic ok (no-op). + ("ok", false) + } + EffortCapabilityState::Unsupported => { + // Current model does not support effort. Reject so Desktop can surface + // the capability loss rather than silently queuing a value that will + // never apply. tracing::warn!( target: "pool::effort", - "effort value {value:?} not in advertised options {valid_values:?} — rejecting" + "effort pick configId={config_id} rejected — current model is known-unsupported" ); let mut ack = serde_json::json!({ "type": "set_config_option", "configId": config_id, - "status": "invalid_value", + "status": "unsupported_model", "value": value, }); ack["category"] = serde_json::json!("thought_level"); - // P2-1: echo the nonce on the early-return rejection path, matching - // the behaviour of the common ack builder below. Without this the - // Desktop's awaitEffortOutcome nonce guard rejects the ack, causing - // an 8 s timeout instead of an immediate "invalid_value" outcome. if let Some(ref n) = nonce { ack["nonce"] = serde_json::json!(n); } @@ -1341,22 +1362,27 @@ fn handle_set_config_option_control( ); return; } - - // Empty value = clear (Auto). Bypass set_pool_effort. - if value.is_empty() { - pool.pending_effort_nonce = nonce.clone(); - pool.clear_pool_effort(); - ("pending_session", true) - } else { - pool.pending_effort_nonce = nonce.clone(); - let result = pool.set_pool_effort(config_id, value); - match result { - SetPoolEffortResult::Stored { .. } => ("pending_session", true), + EffortCapabilityState::Unknown => { + // Pre-first-session window (or reset after a model switch pending + // the next session/new). Trust the Desktop's category field as the + // signal: if category == "thought_level", store for apply and ack + // pending_session; otherwise synthetic ok (non-effort). + if frame_category == "thought_level" && config_id != "unknown" { + if value.is_empty() { + pool.pending_effort_nonce = nonce.clone(); + pool.clear_pool_effort(); + ("pending_session", true) + } else { + pool.pending_effort_nonce = nonce.clone(); + let result = pool.set_pool_effort(config_id, value); + match result { + SetPoolEffortResult::Stored { .. } => ("pending_session", true), + } + } + } else { + ("ok", false) } } - } else { - // Not a thought_level option — synthetic ok (no-op behaviour unchanged). - ("ok", false) }; // B5: include "category": "thought_level" ONLY on the real-forward branch. @@ -8046,6 +8072,7 @@ mod observer_payload_trim_tests { #[cfg(test)] mod control_result_tests { use super::*; + use crate::pool::{AgentModelCapabilities, EffortCapabilityState}; // ── B5 harness-level tests for handle_set_config_option_control ────────── // @@ -8584,4 +8611,289 @@ mod control_result_tests { "synthetic ok ack must not carry category field" ); } + + // ── Tri-state switch-path regressions ───────────────────────────────────── + + /// Helper: build a pool with an idle agent, drive capabilities via the real + /// `return_agent` path (not the test-only `notify_capabilities_discovered` + /// helper), and return the pool ready for further assertions. + /// + /// `thought_level_id` — Some means the agent supports effort; None means + /// the model does not. + async fn pool_with_capabilities_via_return(thought_level_id: Option<&str>) -> AgentPool { + use crate::acp::AcpClient; + let acp = AcpClient::spawn( + "bash", + &["-c".to_string(), "sleep 10".to_string()], + &[], + false, + ) + .await + .expect("failed to spawn test agent for capability helper"); + let agent = OwnedAgent { + index: 0, + acp, + state: SessionState::default(), + model_capabilities: None, + desired_model: None, + model_overridden: false, + desired_effort: None, + startup_effort: None, + desired_effort_gen: None, + pending_effort_nonce: None, + last_effort_result: None, + agent_name: "test".into(), + goose_system_prompt_supported: None, + protocol_version: 2, + }; + let mut pool = AgentPool::from_slots(vec![Some(agent)]); + // Claim and immediately return with populated capabilities — drives the + // `return_agent` capability-refresh path, the PRODUCTION code path. + let mut agent = pool.try_claim(None).unwrap(); + agent.model_capabilities = Some(AgentModelCapabilities { + thought_level_config_id: thought_level_id.map(str::to_string), + config_options_raw: thought_level_id + .map(|id| { + vec![serde_json::json!({ + "id": id, + "options": [{"value": "low"}, {"value": "medium"}, {"value": "high"}] + })] + }) + .unwrap_or_default(), + available_models_raw: None, + }); + pool.return_agent(agent); + pool + } + + /// Switch effort-supporting model → non-effort model: + /// - `return_agent` must write `Unsupported` + /// - a subsequent effort pick must be REJECTED with `"unsupported_model"` status + /// - the pick must not update `pool.desired_effort` + #[tokio::test] + async fn test_tristate_switch_effort_to_no_effort_rejects_pick() { + // Start Supported (effort-capable model). + let mut pool = pool_with_capabilities_via_return(Some("effort")).await; + assert!( + matches!( + pool.effort_capability_state, + EffortCapabilityState::Supported { .. } + ), + "setup: pool must be Supported" + ); + + // Simulate a model switch: agent returns with capabilities but no + // thought_level — the new model does not support effort. + let mut agent = pool.try_claim(None).unwrap(); + agent.model_capabilities = Some(AgentModelCapabilities { + thought_level_config_id: None, + config_options_raw: vec![], + available_models_raw: None, + }); + pool.return_agent(agent); + + // Pool must now be Unsupported. + assert!( + matches!( + pool.effort_capability_state, + EffortCapabilityState::Unsupported + ), + "after switch to non-effort model: pool must be Unsupported" + ); + + // A subsequent effort pick must be REJECTED. + let obs = observer::ObserverHandle::in_process(); + let payload = serde_json::json!({ + "type": "set_config_option", + "configId": "effort", + "value": "high", + "category": "thought_level", + }); + handle_set_config_option_control(&payload, &mut pool, Some(&obs)); + + let events = obs.snapshot(); + assert_eq!(events.len(), 1, "exactly one ack emitted"); + assert_eq!( + events[0].payload["status"].as_str().unwrap(), + "unsupported_model", + "effort pick on Unsupported state must emit unsupported_model" + ); + // Pick must not have been stored — desired_effort unchanged. + assert!( + pool.desired_effort.is_none(), + "Unsupported pick must not update pool.desired_effort" + ); + } + + /// Switch no-effort model → effort-supporting model: + /// - `return_agent` must write `Supported` with the new option set + /// - a subsequent pick with the correct configId must be STORED and acked + /// `pending_session` + #[tokio::test] + async fn test_tristate_switch_no_effort_to_effort_accepts_pick() { + // Start Unsupported (non-effort model). + let mut pool = pool_with_capabilities_via_return(None).await; + assert!( + matches!( + pool.effort_capability_state, + EffortCapabilityState::Unsupported + ), + "setup: pool must be Unsupported" + ); + + // Simulate a model switch: agent returns with thought_level capability. + let new_config_id = "effort_v2"; + let mut agent = pool.try_claim(None).unwrap(); + agent.model_capabilities = Some(AgentModelCapabilities { + thought_level_config_id: Some(new_config_id.to_string()), + config_options_raw: vec![serde_json::json!({ + "id": new_config_id, + "options": [{"value": "low"}, {"value": "high"}] + })], + available_models_raw: None, + }); + pool.return_agent(agent); + + // Pool must now be Supported with the new configId. + assert!( + matches!( + &pool.effort_capability_state, + EffortCapabilityState::Supported { config_id, .. } if config_id == new_config_id + ), + "after switch to effort-capable model: pool must be Supported with new configId" + ); + + // A pick with the correct configId must be STORED. + let obs = observer::ObserverHandle::in_process(); + let payload = serde_json::json!({ + "type": "set_config_option", + "configId": new_config_id, + "value": "high", + }); + handle_set_config_option_control(&payload, &mut pool, Some(&obs)); + + let events = obs.snapshot(); + assert_eq!(events.len(), 1, "exactly one ack emitted"); + assert_eq!( + events[0].payload["status"].as_str().unwrap(), + "pending_session", + "pick on newly-Supported state must emit pending_session" + ); + assert_eq!( + pool.desired_effort + .as_ref() + .map(|(id, v)| (id.as_str(), v.as_str())), + Some((new_config_id, "high")), + "pick must be stored in pool.desired_effort" + ); + } + + /// Option-set CHANGE across a switch (both models support effort, different option sets): + /// - value valid for the OLD model but absent from the new model's options → `invalid_value` + /// - value in the NEW model's option set → `pending_session` + #[tokio::test] + async fn test_tristate_switch_new_options_validates_against_new_snapshot() { + // Start with configId="effort", options=[low, medium, high]. + let mut pool = pool_with_capabilities_via_return(Some("effort")).await; + + // Simulate a switch to a model with configId="effort", options=[standard, extended] only. + let mut agent = pool.try_claim(None).unwrap(); + agent.model_capabilities = Some(AgentModelCapabilities { + thought_level_config_id: Some("effort".to_string()), + config_options_raw: vec![serde_json::json!({ + "id": "effort", + "options": [{"value": "standard"}, {"value": "extended"}] + })], + available_models_raw: None, + }); + pool.return_agent(agent); + + // Verify snapshot updated. + let new_valid_values = match &pool.effort_capability_state { + EffortCapabilityState::Supported { valid_values, .. } => valid_values.clone(), + other => panic!("expected Supported, got {other:?}"), + }; + assert_eq!( + new_valid_values, + vec!["standard", "extended"], + "option set must reflect new model's options" + ); + + // Old value "high" (valid for old model, absent from new) → invalid_value. + let obs = observer::ObserverHandle::in_process(); + let stale_payload = serde_json::json!({ + "type": "set_config_option", + "configId": "effort", + "value": "high", + }); + handle_set_config_option_control(&stale_payload, &mut pool, Some(&obs)); + let events = obs.snapshot(); + assert_eq!(events.len(), 1, "exactly one ack for stale pick"); + assert_eq!( + events[0].payload["status"].as_str().unwrap(), + "invalid_value", + "stale value from old model option set must be rejected" + ); + + // New value "extended" (valid for new model) → pending_session. + let obs2 = observer::ObserverHandle::in_process(); + let valid_payload = serde_json::json!({ + "type": "set_config_option", + "configId": "effort", + "value": "extended", + }); + handle_set_config_option_control(&valid_payload, &mut pool, Some(&obs2)); + let events2 = obs2.snapshot(); + assert_eq!(events2.len(), 1, "exactly one ack for new-model pick"); + assert_eq!( + events2[0].payload["status"].as_str().unwrap(), + "pending_session", + "value from new model option set must be accepted" + ); + } + + /// After a model switch, `effort_capability_state` becomes `Unknown` while + /// the agent is checked out (capabilities reset). Once the agent returns + /// with `model_capabilities = None` (post-switch, pre-next-session/new), + /// a pick with category trust must use the Unknown path (pending_session), + /// NOT be rejected as Unsupported. + #[tokio::test] + async fn test_tristate_unknown_after_switch_uses_pre_discovery_trust_path() { + // Start Supported. + let mut pool = pool_with_capabilities_via_return(Some("effort")).await; + + // Simulate post-switch return with capabilities=None (reset by switch). + let mut agent = pool.try_claim(None).unwrap(); + agent.model_capabilities = None; // reset — capabilities unknown until next session/new + pool.return_agent(agent); + + // Pool must be Unknown. + assert!( + matches!(pool.effort_capability_state, EffortCapabilityState::Unknown), + "capabilities=None on return must produce Unknown state" + ); + + // A pick with category trust must use the pre-discovery path (pending_session). + let obs = observer::ObserverHandle::in_process(); + let payload = serde_json::json!({ + "type": "set_config_option", + "configId": "effort", + "value": "high", + "category": "thought_level", + }); + handle_set_config_option_control(&payload, &mut pool, Some(&obs)); + + let events = obs.snapshot(); + assert_eq!(events.len(), 1, "exactly one ack emitted"); + assert_eq!( + events[0].payload["status"].as_str().unwrap(), + "pending_session", + "Unknown state with category trust must emit pending_session (pre-discovery path)" + ); + assert_eq!( + pool.desired_effort.as_ref().map(|(_, v)| v.as_str()), + Some("high"), + "Unknown path must store pick for apply at next session" + ); + } } diff --git a/crates/buzz-acp/src/pool.rs b/crates/buzz-acp/src/pool.rs index 621109c50c..5bfc426135 100644 --- a/crates/buzz-acp/src/pool.rs +++ b/crates/buzz-acp/src/pool.rs @@ -280,21 +280,30 @@ impl OwnedAgent { } } -/// Pool-level capability snapshot for the `thought_level` config option. +/// Pool-level effort capability state derived from worker session data. /// -/// Written at `return_agent` when a worker with populated capabilities returns -/// to the pool. Because checked-out agents carry their own `model_capabilities`, -/// this cache ensures `handle_set_config_option_control` can identify and -/// validate effort picks even when all workers are checked out (pool slots -/// are `None`). +/// Written at `return_agent` from the returning worker's `model_capabilities`. +/// Drives the classifier in `handle_set_config_option_control`: +/// - `Unknown` → no session data yet; trust the Desktop's `category` field +/// (pre-first-session window, case D). +/// - `Supported` → current model advertises `thought_level`; validate picks +/// against the snapshot's option set (case A). +/// - `Unsupported` → current model confirmed to not advertise `thought_level`; +/// reject picks immediately without updating the pool. #[derive(Debug, Clone, Default)] -pub struct PoolEffortCapabilities { - /// The adapter's `thought_level` configId from `session/new`. - /// `None` until the first session has been created. - pub config_id: Option, - /// Adapter-advertised option values for the `thought_level` config option. - /// Empty until the first session returns options. - pub valid_values: Vec, +pub enum EffortCapabilityState { + /// No session has returned capabilities yet — pre-first-session window. + #[default] + Unknown, + /// Current model advertises `thought_level`. Contains the configId and + /// the adapter-advertised valid option values. + Supported { + config_id: String, + valid_values: Vec, + }, + /// Current model does not advertise `thought_level` (confirmed via a + /// session/new response that carries no `thought_level` configOption). + Unsupported, } /// Pre-prompt effort application result sent from a worker task to the main loop @@ -364,23 +373,21 @@ pub struct AgentPool { /// pool level — a live pick/clear is always authoritative over startup seeding, /// even if the pool value is `None` (i.e. the user explicitly cleared it). pub effort_ever_picked: bool, - /// Pool-level capability cache for the `thought_level` config option. + /// Pool-level effort capability state for the `thought_level` config option. /// - /// Written at `return_agent` (refreshed from the returned agent's - /// capabilities). Allows `handle_set_config_option_control` to identify - /// and validate effort picks regardless of idle occupancy. - pub effort_capabilities: PoolEffortCapabilities, - /// True once any worker has ever had capabilities populated and returned to - /// the pool. Used to distinguish "pre-first-return" (capabilities unknown - /// — case D trust path applies) from "all workers currently busy" - /// (capabilities known but cache temporarily empty — case C trust path) - /// in `handle_set_config_option_control`. - pub capabilities_ever_discovered: bool, + /// Written at `return_agent` from the returning worker's `model_capabilities`, + /// refreshing on every return so the state always reflects the current model. + /// Drives `handle_set_config_option_control`'s three-way classifier: + /// `Supported` → validate and store; `Unsupported` → reject; `Unknown` → + /// pre-discovery trust (store for apply at first session). + pub effort_capability_state: EffortCapabilityState, /// Generation of the last effort pick/clear for which a terminal ack /// (ok/failure/cleared) has already been emitted. Used by `return_agent` /// to emit exactly one terminal ack per generation when parallelism > 1: /// the first worker whose result is processed owns the ack; subsequent - /// workers with the same generation are silenced. + /// workers with the same generation are silenced. The sole ack authority + /// is `resolve_effort_report`; `return_agent` only reads this field for + /// the stale-generation force-invalidation (F1) guard. /// /// `None` means no terminal ack has been emitted yet for any generation. pub last_acked_effort_gen: Option, @@ -743,8 +750,7 @@ impl AgentPool { effort_generation: 0, pending_effort_nonce: None, effort_ever_picked: false, - effort_capabilities: PoolEffortCapabilities::default(), - capabilities_ever_discovered: false, + effort_capability_state: EffortCapabilityState::Unknown, last_acked_effort_gen: None, } } @@ -819,9 +825,9 @@ impl AgentPool { /// so the next session creation applies the current pool value. This closes /// the window where a busy worker's surviving session runs at a stale effort. /// - /// **Capability refresh:** the pool-level `effort_capabilities` cache is - /// updated from the returned agent's capabilities (if populated), ensuring - /// the cache is available even when all other workers are checked out. + /// **Capability refresh:** the pool-level `effort_capability_state` is + /// re-derived from the returned agent's `model_capabilities`, so the state + /// always reflects the current model even after live model switches. pub fn return_agent(&mut self, mut agent: OwnedAgent) { let idx = agent.index; @@ -911,11 +917,18 @@ impl AgentPool { agent.state.invalidate_all(); } - // Capability refresh: write back the agent's capability snapshot to the - // pool-level cache so validation remains available when all workers are - // checked out. Always overwrite — returned workers have fresh capabilities. - if let Some(ref caps) = agent.model_capabilities { - if caps.thought_level_config_id.is_some() { + // Capability refresh: derive the pool-level tri-state from the returned + // agent's model_capabilities. Refresh on every return so the pool state + // always reflects the current model — not just the first model seen. + // + // `Some(caps)` with a thought_level configId → Supported + // `Some(caps)` without a thought_level configId → Unsupported (model + // confirmed to not advertise thought_level; future picks are rejected) + // `None` → Unknown (capability not yet resolved; keep the pre-first- + // session trust path active for the next session) + match &agent.model_capabilities { + Some(caps) if caps.thought_level_config_id.is_some() => { + let config_id = caps.thought_level_config_id.clone().unwrap(); let valid_values = caps .config_options_raw .iter() @@ -923,7 +936,7 @@ impl AgentPool { opt.get("id") .or_else(|| opt.get("configId")) .and_then(|v| v.as_str()) - == caps.thought_level_config_id.as_deref() + == Some(&config_id) }) .and_then(|opt| opt.get("options")) .and_then(|o| o.as_array()) @@ -935,24 +948,20 @@ impl AgentPool { .collect() }) .unwrap_or_default(); - self.effort_capabilities = PoolEffortCapabilities { - config_id: caps.thought_level_config_id.clone(), + self.effort_capability_state = EffortCapabilityState::Supported { + config_id, valid_values, }; - self.capabilities_ever_discovered = true; - } else if self.capabilities_ever_discovered { - // P3: the returning agent has populated capabilities but no - // thought_level configId — the model was swapped to one that - // does not support effort. Clear the pool-level cache so - // subsequent picks are not validated against stale options. - // - // capabilities_ever_discovered stays true: we have seen at - // least one session. This activates case C in - // handle_set_config_option_control, letting picks through - // without value-validation while the cache is empty (correct - // — the user can still send a pick; the adapter will reject - // it if the value is unsupported). - self.effort_capabilities = PoolEffortCapabilities::default(); + } + Some(_) => { + // Populated capabilities, no thought_level — model is confirmed unsupported. + self.effort_capability_state = EffortCapabilityState::Unsupported; + } + None => { + // Capabilities not yet resolved (e.g. reset after a model switch). + // Leave the tri-state Unknown so the pre-discovery trust path stays + // active; the next session/new will resolve it. + self.effort_capability_state = EffortCapabilityState::Unknown; } } @@ -1318,13 +1327,13 @@ impl AgentPool { /// Notify the pool that capabilities have been discovered for a worker. /// - /// **Test-only helper.** In production the pool capability cache is written + /// **Test-only helper.** In production the pool capability state is written /// by `return_agent` when a worker returns after completing its first session. - /// This function lets tests seed the cache directly without going through the + /// This function lets tests seed the state directly without going through the /// full spawn-session-return cycle. #[cfg(test)] pub fn notify_capabilities_discovered(&mut self, caps: &AgentModelCapabilities) { - if caps.thought_level_config_id.is_some() && self.effort_capabilities.config_id.is_none() { + if let Some(ref config_id) = caps.thought_level_config_id { let valid_values = caps .config_options_raw .iter() @@ -1332,7 +1341,7 @@ impl AgentPool { opt.get("id") .or_else(|| opt.get("configId")) .and_then(|v| v.as_str()) - == caps.thought_level_config_id.as_deref() + == Some(config_id.as_str()) }) .and_then(|opt| opt.get("options")) .and_then(|o| o.as_array()) @@ -1342,11 +1351,12 @@ impl AgentPool { .collect() }) .unwrap_or_default(); - self.effort_capabilities = PoolEffortCapabilities { - config_id: caps.thought_level_config_id.clone(), + self.effort_capability_state = EffortCapabilityState::Supported { + config_id: config_id.clone(), valid_values, }; - self.capabilities_ever_discovered = true; + } else { + self.effort_capability_state = EffortCapabilityState::Unsupported; } } } @@ -1536,14 +1546,15 @@ async fn create_session_and_apply_model( } } - // Populate model capabilities on first session creation. - if agent.model_capabilities.is_none() { - agent.model_capabilities = Some(AgentModelCapabilities { - config_options_raw: extract_agent_config_options(&resp.raw), - available_models_raw: extract_model_state(&resp.raw), - thought_level_config_id: extract_thought_level_config_id(&resp.raw), - }); - } + // Populate model capabilities from every session/new response — not just the + // first. This ensures that after a model switch (which resets capabilities to + // None to discard the pre-switch snapshot), the next session/new supplies + // fresh capability data for the new model. + agent.model_capabilities = Some(AgentModelCapabilities { + config_options_raw: extract_agent_config_options(&resp.raw), + available_models_raw: extract_model_state(&resp.raw), + thought_level_config_id: extract_thought_level_config_id(&resp.raw), + }); // B5 startup-default: arm desired_effort from startup_effort + capabilities. // No-op when desired_effort already set (live pick takes precedence, via the @@ -1558,6 +1569,14 @@ async fn create_session_and_apply_model( match resolve_model_switch_method(&resp.raw, desired) { Some(method) => { apply_model_switch(&mut agent.acp, &resp.session_id, desired, &method).await?; + // The session/new response captured above reflects the pre-switch + // model's configOptions. Reset capabilities to None so: + // 1. `return_agent` writes EffortCapabilityState::Unknown, keeping + // the pre-discovery trust path active (not stale-Supported). + // 2. The next session/new (after this session is invalidated on + // the next model switch or rotation) re-derives capabilities + // from the then-current model's response. + agent.model_capabilities = None; true } None => { @@ -1680,10 +1699,23 @@ async fn create_session_and_apply_model( // post-switch state. modelOverridden reflects whether the switch actually // applied — false on the unsupported arm so the panel doesn't show a // stale override badge. + // + // When a model switch succeeded, the configOptions in resp.raw are pre-switch + // data and must not be cached — the next session/new will supply fresh options + // for the new model. Emit null for configOptions on the switch path so Desktop + // clears its picker state rather than rendering stale capability data. + let config_options_for_cache = if switch_succeeded { + serde_json::Value::Null + } else { + resp.raw + .get("configOptions") + .cloned() + .unwrap_or(serde_json::Value::Null) + }; agent.acp.observe( "session_config_captured", serde_json::json!({ - "configOptions": resp.raw.get("configOptions").cloned().unwrap_or(serde_json::Value::Null), + "configOptions": config_options_for_cache, "modes": resp.raw.get("modes").cloned().unwrap_or(serde_json::Value::Null), "models": resp.raw.get("models").cloned().unwrap_or(serde_json::Value::Null), "modelOverridden": agent.model_overridden && switch_succeeded, @@ -8235,9 +8267,14 @@ mod effort_tests { available_models_raw: None, }); - // All slots are None (workers checked out) but capabilities ARE known. - assert!(pool.effort_capabilities.config_id.is_some()); - assert!(pool.capabilities_ever_discovered); + // All slots are None (workers checked out) but capabilities ARE known (Supported). + assert!( + matches!( + pool.effort_capability_state, + EffortCapabilityState::Supported { .. } + ), + "V-2 setup: pool must be Supported after notify_capabilities_discovered" + ); assert!(pool.agents_mut().iter().flatten().next().is_none()); // set_pool_effort should store, not return NoCatalog. @@ -9222,12 +9259,12 @@ mod effort_tests { ); } - // ── P3: stale capability cache cleared on model swap ───────────────────── + // ── P3: capability state updated to Unsupported on model swap ───────────────── /// P3: when a returning agent has populated capabilities but no /// thought_level configId (model swapped to non-effort), the pool-level - /// effort_capabilities cache is cleared so stale options are not used for - /// validation on the next pick. + /// effort_capability_state transitions to `Unsupported` so subsequent picks + /// are rejected immediately rather than validated against stale options. #[tokio::test] async fn test_capability_cache_cleared_when_model_loses_thought_level() { let acp = AcpClient::spawn( @@ -9260,8 +9297,13 @@ mod effort_tests { config_options_raw: vec![serde_json::json!({"id": "effort", "options": [{"value": "low"}, {"value": "high"}]})], available_models_raw: None, }); - assert!(pool.effort_capabilities.config_id.is_some()); - assert!(pool.capabilities_ever_discovered); + assert!( + matches!( + pool.effort_capability_state, + EffortCapabilityState::Supported { .. } + ), + "P3 setup: must start Supported" + ); // Worker returns after a model swap — no thought_level in new model. let mut agent = pool.try_claim(None).unwrap(); @@ -9272,19 +9314,13 @@ mod effort_tests { }); pool.return_agent(agent); - // P3: cache must be cleared. + // P3: state must transition to Unsupported — not Unknown, not stale-Supported. assert!( - pool.effort_capabilities.config_id.is_none(), - "P3: effort_capabilities must be cleared when model loses thought_level" - ); - assert!( - pool.effort_capabilities.valid_values.is_empty(), - "P3: valid_values must be cleared" - ); - // capabilities_ever_discovered stays true (case C remains active). - assert!( - pool.capabilities_ever_discovered, - "capabilities_ever_discovered must stay true after cache clear" + matches!( + pool.effort_capability_state, + EffortCapabilityState::Unsupported + ), + "P3: effort_capability_state must be Unsupported when model loses thought_level" ); }