From 1aef5a6d7c1176003d9be0fee843aff4c823932d Mon Sep 17 00:00:00 2001 From: Matt Toohey Date: Fri, 16 Oct 2026 14:21:25 +1100 Subject: [PATCH 1/3] fix(acp-client): retry Claude's model confirmation once before prompting MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Claude's SDK confirms every `setModel` with a short API probe on a fixed five-second deadline. When the probe misses it, the bridge returns an internal error whose `details` read `Couldn't confirm model "" with the API…`, Staged's pre-prompt setup failed on the spot, and the same selection was accepted seconds later on the next run. Session records show this exact failure recurring for `claude-fable-5-1[1m]`. The driver now treats that one error as confirmation uncertainty rather than an unavailable model: on a Claude bridge, the identical `session/set_config_option` request is sent once more after a one-second delay on the same connection — two attempts total, never while an attempt is still unanswered, and only for the model option. The existing setup timeout and cancellation boundary still bound the whole exchange by dropping the setup future, so a Stop during the delay or the second attempt sends nothing further and no prompt is released. On success the existing effort-selection, stale-effort skip, and fallback-announcement logic continue unchanged and the queued prompt is released exactly once. Exhausted recovery fails with a plain sentence ("Claude couldn't confirm the selected model after two attempts. Your selection is unchanged; try again.") that deliberately does not satisfy `is_config_selection_unavailable_error`, so the saved model/effort selection is kept. The matcher is isolated and keyed to the requested (resolved) model and the adapter's `initialize` name; authentication, invalid-selection, transport, and generic errors — and every other adapter — keep their single attempt and Debug-shaped error. Each attempt logs a sanitized record (phase, adapter, requested/resolved value, attempt, duration, outcome) with no prompts, credentials, or raw responses. The runner's post-run selection clearing is extracted into `clear_unavailable_acp_config_selection` so the retention contract can be tested through the real path; its behaviour is unchanged. Tests drive `run_acp_session` end to end against a scripted in-process agent under paused time: fail-then-succeed, both fail, cancellation during the delay and during the second attempt, non-matching errors and another adapter, an already-current model, effort options changed by the retried model, a retried fallback value, and setup-deadline expiry — plus the matcher, the adapter key, and a runner persistence regression. Co-Authored-By: Claude Fable 5.1 Signed-off-by: Matt Toohey --- apps/staged/src-tauri/src/session_runner.rs | 129 +++- crates/acp-client/src/driver.rs | 168 ++++- .../src/driver/tests/model_confirmation.rs | 710 ++++++++++++++++++ crates/acp-client/src/lib.rs | 1 + 4 files changed, 979 insertions(+), 29 deletions(-) create mode 100644 crates/acp-client/src/driver/tests/model_confirmation.rs diff --git a/apps/staged/src-tauri/src/session_runner.rs b/apps/staged/src-tauri/src/session_runner.rs index a5758b2f3..e8b959765 100644 --- a/apps/staged/src-tauri/src/session_runner.rs +++ b/apps/staged/src-tauri/src/session_runner.rs @@ -1572,17 +1572,12 @@ pub fn start_session( .unwrap_or(CompletionReason::Interrupted); if let Err(ref e) = result { - if config.acp_config_selection.is_some() - && acp_client::is_config_selection_unavailable_error(e) - { - if let Err(clear_err) = - store_for_status.set_session_acp_config_selection(&session_id_for_status, None) - { - log::warn!( - "Failed to clear stale ACP config selection for session {session_id_for_status}: {clear_err}" - ); - } - } + clear_unavailable_acp_config_selection( + &store_for_status, + &session_id_for_status, + config.acp_config_selection.as_ref(), + e, + ); } // Transition the session to its terminal state, but only if it is @@ -1847,6 +1842,32 @@ fn stored_agent_session_id(store: &Store, session_id: &str) -> Result, + error: &str, +) -> bool { + if selection.is_none() || !acp_client::is_config_selection_unavailable_error(error) { + return false; + } + if let Err(clear_err) = store.set_session_acp_config_selection(session_id, None) { + log::warn!( + "Failed to clear stale ACP config selection for session {session_id}: {clear_err}" + ); + return false; + } + true +} + pub(crate) fn session_has_note_artifact(store: &Store, session_id: &str) -> bool { store .get_note_by_session(session_id) @@ -4964,6 +4985,92 @@ mod tests { assert_eq!(step.error, None); } + fn fable_selection() -> AcpConfigSelection { + AcpConfigSelection { + model: Some(crate::store::AcpConfigValueSelection { + config_id: "model".to_string(), + value_id: "claude-fable-5-1[1m]".to_string(), + label: Some("Fable".to_string()), + }), + effort: None, + } + } + + /// acp-client retries Claude's model confirmation once and then fails + /// with a plain sentence. The post-run clearing must read that as + /// "selection fine, try again" and leave the saved pin alone, while a + /// genuinely stale pin keeps being cleared exactly as before. + #[test] + fn a_failed_model_confirmation_keeps_the_saved_selection_but_a_stale_pin_is_cleared() { + let store = Store::in_memory().unwrap(); + let session = crate::store::Session::new_running("prompt", &PathBuf::from("/tmp")); + store.create_session(&session).unwrap(); + let selection = fable_selection(); + store + .set_session_acp_config_selection(&session.id, Some(&selection)) + .unwrap(); + + // Exactly as the driver's `run` wraps the setup failure. + let exhausted = format!( + "ACP protocol failed: {:?}", + agent_client_protocol::util::internal_error( + acp_client::MODEL_CONFIRMATION_EXHAUSTED_MESSAGE + ) + ); + assert!(!clear_unavailable_acp_config_selection( + &store, + &session.id, + Some(&selection), + &exhausted, + )); + assert_eq!( + store + .get_session(&session.id) + .unwrap() + .unwrap() + .acp_config_selection, + Some(selection.clone()), + "a confirmation failure leaves the pin for the retry it asks for" + ); + + let stale = format!( + "ACP protocol failed: {:?}", + agent_client_protocol::util::internal_error( + "Selected ACP model value 'claude-fable-5-1[1m]' is no longer available for config option 'model'" + ) + ); + assert!(clear_unavailable_acp_config_selection( + &store, + &session.id, + Some(&selection), + &stale, + )); + assert_eq!( + store + .get_session(&session.id) + .unwrap() + .unwrap() + .acp_config_selection, + None + ); + } + + /// A run that never had a selection has nothing to clear, whatever it + /// failed with. + #[test] + fn a_run_without_a_selection_clears_nothing() { + let store = Store::in_memory().unwrap(); + let session = crate::store::Session::new_running("prompt", &PathBuf::from("/tmp")); + store.create_session(&session).unwrap(); + + assert!(!clear_unavailable_acp_config_selection( + &store, + &session.id, + None, + "Selected ACP model value 'x' is no longer available for config option 'model'", + )); + } + /// The immediate branch start has answered its client by the time the /// context build or `start_session` can fail, so the row is the only place /// the failure can go: it reads `error`/`Crashed` with the message, and diff --git a/crates/acp-client/src/driver.rs b/crates/acp-client/src/driver.rs index d98eba3fc..c3bf4daec 100644 --- a/crates/acp-client/src/driver.rs +++ b/crates/acp-client/src/driver.rs @@ -29,7 +29,7 @@ use agent_client_protocol::{ SelectedPermissionOutcome, SessionConfigKind, SessionConfigOption, SessionConfigOptionCategory, SessionConfigSelectOptions, SessionInfoUpdate, SessionModeState, SessionNotification, SessionUpdate, SetSessionConfigOptionRequest, - StopReason, TextContent, ToolCallContent, + SetSessionConfigOptionResponse, StopReason, TextContent, ToolCallContent, }, ProtocolVersion, }, @@ -5483,6 +5483,129 @@ pub fn is_config_selection_unavailable_error(error: &str) -> bool { || error.contains(CONFIG_SELECTION_STALE_NOT_SELECT))) } +// Claude's SDK confirms every `setModel` with a short API probe on a fixed +// five-second deadline, and a probe that misses it surfaces as this sentence +// in the `details` of an internal error. That is confirmation uncertainty, +// not an unavailable model: the same selection is routinely accepted seconds +// later. [`send_config_selection`] bounds its recovery to exactly this error. +const MODEL_CONFIRMATION_FAILED_PREFIX: &str = "Couldn't confirm model \""; +const MODEL_CONFIRMATION_ATTEMPTS: u32 = 2; +const MODEL_CONFIRMATION_RETRY_DELAY: Duration = Duration::from_secs(1); + +/// What the user reads once every confirmation attempt has failed. Kept clear +/// of the [`is_config_selection_unavailable_error`] fragments on purpose: the +/// selection itself is sound, so consumers must keep it for the retry the +/// sentence asks for. +pub const MODEL_CONFIRMATION_EXHAUSTED_MESSAGE: &str = "Claude couldn't confirm the selected model after two attempts. Your selection is unchanged; try again."; + +/// Whether `adapter` — the agent's `initialize` name, or the driver label when +/// it sends none — is a Claude bridge. Only Claude's SDK produces the +/// confirmation failure, so only Claude gets the retry; another agent that +/// happened to word an error the same way keeps the single attempt. +fn supports_model_confirmation_recovery(adapter: &str) -> bool { + adapter.to_ascii_lowercase().contains("claude") +} + +/// Whether a `session/set_config_option` error is Claude failing to confirm +/// `requested_model`: an internal error whose `data` quotes that model after +/// [`MODEL_CONFIRMATION_FAILED_PREFIX`], either as the bridge's +/// `{"details": …}` object or as a bare string. Authentication errors, invalid +/// values, transport failures, and a different model are all not this error. +fn is_model_confirmation_error( + error: &agent_client_protocol::Error, + requested_model: &str, +) -> bool { + if error.code != ErrorCode::InternalError { + return false; + } + let details = match &error.data { + Some(serde_json::Value::Object(data)) => data.get("details").and_then(|d| d.as_str()), + Some(serde_json::Value::String(details)) => Some(details.as_str()), + _ => None, + }; + details.is_some_and(|details| { + details.contains(&format!( + "{MODEL_CONFIRMATION_FAILED_PREFIX}{requested_model}\"" + )) + }) +} + +/// One `session/set_config_option` round trip for `selection`. +/// +/// When a Claude bridge can't confirm the selected model, the identical +/// request goes out exactly once more after [`MODEL_CONFIRMATION_RETRY_DELAY`] +/// on the same connection — never while an attempt is still unanswered, and +/// never for any other option, error, or adapter. The caller's setup timeout +/// and cancellation bound the whole exchange by dropping this future, so a +/// Stop during the delay or the second attempt sends nothing further. +/// Exhausted recovery reads as [`MODEL_CONFIRMATION_EXHAUSTED_MESSAGE`] +/// rather than the nested Debug dump. +/// +/// Each attempt logs a sanitized record — phase, adapter, requested and +/// resolved value, attempt, duration, classified outcome; never prompts, +/// credentials, or the raw response — so a failed start can be correlated +/// with the confirmation that produced it. +async fn send_config_selection( + connection: &ConnectionTo, + agent_session_id: &str, + adapter: &str, + selection: &AcpSessionConfigOptionSelection, + resolved: &ResolvedConfigSelection, +) -> Result { + let label = config_selection_label(&selection.category); + let requested = selection.value_id.as_str(); + let value = resolved.value.value_id.as_str(); + let recoverable = selection.category == SessionConfigOptionCategory::Model + && supports_model_confirmation_recovery(adapter); + let attempts = if recoverable { + MODEL_CONFIRMATION_ATTEMPTS + } else { + 1 + }; + let mut attempt = 1; + loop { + let started = Instant::now(); + let result = connection + .send_request(SetSessionConfigOptionRequest::new( + agent_session_id.to_string(), + resolved.config_id.clone(), + value, + )) + .block_task() + .await; + let diagnostic = format!( + "ACP config phase=setup adapter={adapter} option={label} requested={requested} \ + resolved={value} attempt={attempt}/{attempts} duration_ms={}", + started.elapsed().as_millis() + ); + match result { + Ok(response) => { + log::info!("{diagnostic} outcome=accepted"); + return Ok(response); + } + Err(error) if recoverable && is_model_confirmation_error(&error, value) => { + if attempt < attempts { + log::warn!( + "{diagnostic} outcome=unconfirmed; retrying in {}ms", + MODEL_CONFIRMATION_RETRY_DELAY.as_millis() + ); + tokio::time::sleep(MODEL_CONFIRMATION_RETRY_DELAY).await; + attempt += 1; + continue; + } + log::warn!("{diagnostic} outcome=unconfirmed; recovery exhausted"); + return Err(MODEL_CONFIRMATION_EXHAUSTED_MESSAGE.to_string()); + } + Err(error) => { + log::warn!("{diagnostic} outcome=failed code={}", i32::from(error.code)); + return Err(format!( + "Failed to set selected ACP {label} config option: {error:?}" + )); + } + } + } +} + // Fragment shared by `setup_acp_session`'s required-transport check and // [`is_missing_mcp_transport_error`] — the same producer/matcher contract as // the config-selection constants above. @@ -5499,6 +5622,7 @@ pub fn is_missing_mcp_transport_error(error: &str) -> bool { async fn apply_or_record_session_config_options( connection: &ConnectionTo, agent_session_id: &str, + adapter: &str, initial_options: Option<&[SessionConfigOption]>, selections: &[AcpSessionConfigOptionSelection], writer: &Arc, @@ -5548,20 +5672,9 @@ async fn apply_or_record_session_config_options( } Err(e) => return Err(e), }; - let response = connection - .send_request(SetSessionConfigOptionRequest::new( - agent_session_id.to_string(), - resolved.config_id, - resolved.value.value_id.as_str(), - )) - .block_task() - .await - .map_err(|e| { - format!( - "Failed to set selected ACP {} config option: {e:?}", - config_selection_label(&selection.category) - ) - })?; + let response = + send_config_selection(connection, agent_session_id, adapter, selection, &resolved) + .await?; // Announced only once the agent has accepted the substitute, so the // transcript never claims a fallback that did not take effect. if let Some(notice) = resolved.value.fallback_notice { @@ -6019,6 +6132,8 @@ async fn setup_acp_session(context: AcpSessionSetupContext<'_>) -> Result) -> Result) -> Result AcpInitializeMetad } } -fn log_initialize_response(agent_label: &str, init_response: &InitializeResponse) { - let agent_name = init_response +/// The name the agent gave itself on `initialize` (e.g. +/// `@agentclientprotocol/claude-agent-acp`), or the driver label when it sent +/// none — so a remote bridge is still recognized by what it is, not where it +/// runs. +fn adapter_name<'a>(agent_label: &'a str, init_response: &'a InitializeResponse) -> &'a str { + init_response .agent_info .as_ref() .map(|info| info.name.as_str()) - .unwrap_or(agent_label); + .unwrap_or(agent_label) +} + +fn log_initialize_response(agent_label: &str, init_response: &InitializeResponse) { + let agent_name = adapter_name(agent_label, init_response); let agent_version = init_response .agent_info .as_ref() @@ -6480,6 +6605,7 @@ impl MessageWriter for BasicMessageWriter { #[cfg(test)] mod tests { + mod model_confirmation; mod response_completion; mod settlement; @@ -7585,6 +7711,7 @@ mod tests { apply_or_record_session_config_options( &connection, "session-1", + "Claude Code", Some(&initial_options), &selections, &message_writer, @@ -7665,6 +7792,7 @@ agent: http=false, sse=false). Select a provider that supports MCP over HTTP/SSE apply_or_record_session_config_options( &connection, "session-1", + "Claude Code", None, &selections, &message_writer, @@ -7676,6 +7804,7 @@ agent: http=false, sse=false). Select a provider that supports MCP over HTTP/SSE let error = apply_or_record_session_config_options( &connection, "session-1", + "Claude Code", None, &selections, &message_writer, @@ -7751,6 +7880,7 @@ agent: http=false, sse=false). Select a provider that supports MCP over HTTP/SSE apply_or_record_session_config_options( &connection, "session-1", + "Claude Code", Some(&loaded_options), &selections, &message_writer, @@ -7820,6 +7950,7 @@ agent: http=false, sse=false). Select a provider that supports MCP over HTTP/SSE apply_or_record_session_config_options( &connection, "session-1", + "Claude Code", Some(&renamed_options()), &selections, &message_writer, @@ -7892,6 +8023,7 @@ agent: http=false, sse=false). Select a provider that supports MCP over HTTP/SSE apply_or_record_session_config_options( &connection, "session-1", + "Claude Code", Some(&renamed_options()), &selections, &message_writer, diff --git a/crates/acp-client/src/driver/tests/model_confirmation.rs b/crates/acp-client/src/driver/tests/model_confirmation.rs new file mode 100644 index 000000000..96930cb64 --- /dev/null +++ b/crates/acp-client/src/driver/tests/model_confirmation.rs @@ -0,0 +1,710 @@ +//! Bounded recovery for Claude's `Couldn't confirm model` failure while the +//! saved model selection is applied before the first prompt. +//! +//! The scenarios drive `run_acp_session` end to end against a scripted +//! in-process agent, so the setup timeout, the cancellation boundary, and +//! the prompt queue under test are the production ones. Time is paused: the +//! one-second retry delay and the 90-second setup deadline elapse instantly +//! once nothing else can make progress. + +use super::*; +use crate::driver::{ + is_model_confirmation_error, run_acp_session, supports_model_confirmation_recovery, + ACP_SETUP_TIMEOUT, MODEL_CONFIRMATION_EXHAUSTED_MESSAGE, +}; +use agent_client_protocol::schema::v1::{ + Implementation, InitializeRequest, InitializeResponse, PromptResponse, +}; +use std::cell::RefCell; +use std::collections::VecDeque; +use std::sync::atomic::{AtomicBool, AtomicUsize, Ordering}; +use std::sync::Mutex; + +const CLAUDE_ADAPTER: &str = "@agentclientprotocol/claude-agent-acp"; +const FABLE: &str = "claude-fable-5-1[1m]"; +const SONNET: &str = "claude-sonnet-5-5"; + +/// The exact shape the installed Claude bridge returns: the SDK's sentence +/// under `details` of a plain internal error. +fn confirmation_error(model: &str) -> agent_client_protocol::Error { + agent_client_protocol::Error::internal_error().data(serde_json::json!({ + "details": format!( + "Couldn't confirm model \"{model}\" with the API. Try again, or run /model to see available models." + ), + })) +} + +fn model_option(current: &str) -> SessionConfigOption { + SessionConfigOption::select( + "model", + "Model", + current.to_string(), + vec![ + SessionConfigSelectOption::new(SONNET, "Sonnet"), + SessionConfigSelectOption::new(FABLE, "Fable"), + ], + ) + .category(SessionConfigOptionCategory::Model) +} + +fn effort_option(current: &str, values: &[&str]) -> SessionConfigOption { + SessionConfigOption::select( + "reasoning", + "Reasoning", + current.to_string(), + values + .iter() + .map(|value| SessionConfigSelectOption::new(value.to_string(), value.to_string())) + .collect::>(), + ) + .category(SessionConfigOptionCategory::ThoughtLevel) +} + +fn model_selection(value_id: &str) -> AcpSessionConfigOptionSelection { + AcpSessionConfigOptionSelection { + category: SessionConfigOptionCategory::Model, + config_id: "model".to_string(), + value_id: value_id.to_string(), + } +} + +fn effort_selection(value_id: &str) -> AcpSessionConfigOptionSelection { + AcpSessionConfigOptionSelection { + category: SessionConfigOptionCategory::ThoughtLevel, + config_id: "reasoning".to_string(), + value_id: value_id.to_string(), + } +} + +fn call(config_id: &str, value_id: &str) -> (String, String) { + (config_id.to_string(), value_id.to_string()) +} + +/// What the scripted agent does with the next `session/set_config_option`. +enum ConfigReply { + Accept(Vec), + Fail(agent_client_protocol::Error), + /// Never answer; the request stays in flight until the connection drops. + Hang, +} + +#[derive(Default)] +struct AgentLog { + config_calls: Mutex>, + prompts: AtomicUsize, +} + +impl AgentLog { + fn config_calls(&self) -> Vec<(String, String)> { + self.config_calls.lock().unwrap().clone() + } + + fn prompts(&self) -> usize { + self.prompts.load(Ordering::SeqCst) + } +} + +struct Script { + adapter: &'static str, + initial_options: Vec, + replies: Vec, + /// Cancel this token when the n-th (1-based) config request arrives, + /// before its scripted reply — a user Stop racing the recovery. + cancel_on_config_call: Option<(usize, CancellationToken)>, +} + +impl Script { + fn claude(initial_options: Vec, replies: Vec) -> Self { + Self { + adapter: CLAUDE_ADAPTER, + initial_options, + replies, + cancel_on_config_call: None, + } + } +} + +struct Run { + outcome: Result<(AgentRunOutcome, SessionSettleReason), String>, + agent: Arc, + writer: Arc, +} + +/// One queued prompt through the real `run_acp_session` against `script`. +async fn run_session( + script: Script, + selections: Vec, + cancel: CancellationToken, +) -> Run { + let log = Arc::new(AgentLog::default()); + let log_for_config = Arc::clone(&log); + let log_for_prompt = Arc::clone(&log); + let adapter = script.adapter; + let initial_options = script.initial_options; + let replies = Arc::new(Mutex::new(VecDeque::from(script.replies))); + let cancel_on_config_call = script.cancel_on_config_call; + let agent = agent_client_protocol::Agent + .builder() + .on_receive_request( + async move |request: InitializeRequest, responder, _cx| { + responder.respond( + InitializeResponse::new(request.protocol_version) + .agent_info(Implementation::new(adapter, "0.0.0-test")), + ) + }, + agent_client_protocol::on_receive_request!(), + ) + .on_receive_request( + async move |_request: NewSessionRequest, responder, _cx| { + responder.respond( + NewSessionResponse::new("session-1").config_options(initial_options.clone()), + ) + }, + agent_client_protocol::on_receive_request!(), + ) + .on_receive_request( + async move |request: SetSessionConfigOptionRequest, responder, _cx| { + let call_index = { + let mut calls = log_for_config.config_calls.lock().unwrap(); + calls.push(( + request.config_id.to_string(), + request + .value + .as_value_id() + .expect("selected config value should be a value ID") + .to_string(), + )); + calls.len() + }; + if let Some((at, token)) = &cancel_on_config_call { + if *at == call_index { + token.cancel(); + } + } + let reply = replies + .lock() + .unwrap() + .pop_front() + .expect("more config requests than scripted replies"); + match reply { + ConfigReply::Accept(options) => { + responder.respond(SetSessionConfigOptionResponse::new(options)) + } + ConfigReply::Fail(error) => responder.respond_with_error(error), + ConfigReply::Hang => { + std::future::pending::<()>().await; + unreachable!("a hung request never resolves") + } + } + }, + agent_client_protocol::on_receive_request!(), + ) + .on_receive_request( + async move |_request: PromptRequest, responder, _cx| { + log_for_prompt.prompts.fetch_add(1, Ordering::SeqCst); + responder.respond(PromptResponse::new(StopReason::EndTurn)) + }, + agent_client_protocol::on_receive_request!(), + ); + + let writer = Arc::new(RecordingMessageWriter::default()); + let message_writer: Arc = writer.clone(); + let handler = Arc::new(AcpNotificationHandler::new( + message_writer, + false, + vec![], + cancel.clone(), + )); + let store: Arc = Arc::new(RecordingStore::default()); + let (prompt_tx, mut prompt_rx) = mpsc::unbounded_channel(); + let (_hold_control_tx, mut hold_control_rx) = mpsc::unbounded_channel(); + let hold_active = AtomicBool::new(false); + let pending_reply = RefCell::new(None); + let child_exited = CancellationToken::new(); + let (reply_tx, _reply_rx) = oneshot::channel(); + prompt_tx + .send(QueuedSessionTurn { + prompt: "hello".to_string(), + images: vec![], + reply: reply_tx, + }) + .expect("queued turn should send"); + + let outcome = agent_client_protocol::Client + .connect_with(agent, async |connection| { + Ok(run_acp_session( + &connection, + Path::new("/tmp"), + &store, + "local-session", + None, + &selections, + &handler, + &[], + "Claude Code", + &cancel, + None, + None, + None, + &child_exited, + &mut prompt_rx, + &mut hold_control_rx, + &hold_active, + &pending_reply, + ) + .await) + }) + .await + .expect("the connection itself stays healthy"); + + Run { + outcome, + agent: log, + writer, + } +} + +#[test] +fn matcher_accepts_only_claudes_confirmation_failure_for_the_requested_model() { + assert!(is_model_confirmation_error( + &confirmation_error(FABLE), + FABLE + )); + // A bridge that flattens `details` to a bare string still matches. + assert!(is_model_confirmation_error( + &agent_client_protocol::Error::internal_error() + .data(format!("Couldn't confirm model \"{FABLE}\" with the API.")), + FABLE, + )); + + // A different model, another error code with the same words, an internal + // error with other details, and one with no data at all are not it. + assert!(!is_model_confirmation_error( + &confirmation_error("claude-opus-5-5"), + FABLE + )); + let mut auth_with_details = agent_client_protocol::Error::auth_required(); + auth_with_details.data = confirmation_error(FABLE).data; + assert!(!is_model_confirmation_error(&auth_with_details, FABLE)); + assert!(!is_model_confirmation_error( + &agent_client_protocol::Error::internal_error() + .data(serde_json::json!({ "details": format!("Invalid value for config option model: {FABLE}") })), + FABLE, + )); + assert!(!is_model_confirmation_error( + &agent_client_protocol::Error::internal_error(), + FABLE + )); +} + +#[test] +fn recovery_is_keyed_to_claude_bridges_only() { + assert!(supports_model_confirmation_recovery(CLAUDE_ADAPTER)); + assert!(supports_model_confirmation_recovery( + "@zed-industries/claude-code-acp" + )); + assert!(supports_model_confirmation_recovery("Claude Code")); + assert!(!supports_model_confirmation_recovery("@openai/codex-acp")); + assert!(!supports_model_confirmation_recovery("Goose")); + assert!(!supports_model_confirmation_recovery("Blox")); +} + +/// The terminal sentence must keep the saved selection: the runner clears a +/// selection only when `is_config_selection_unavailable_error` fires. +#[test] +fn exhausted_message_is_not_an_unavailable_selection() { + assert!(!is_config_selection_unavailable_error( + MODEL_CONFIRMATION_EXHAUSTED_MESSAGE + )); + // Exactly as `run` wraps it on the way to the session alert. + let wrapped = format!( + "ACP protocol failed: {:?}", + agent_client_protocol::util::internal_error(MODEL_CONFIRMATION_EXHAUSTED_MESSAGE) + ); + assert!(!is_config_selection_unavailable_error(&wrapped)); + assert_eq!( + wrapped, + r#"ACP protocol failed: Error { code: -32603: Internal error, message: "Internal error", data: Some(String("Claude couldn't confirm the selected model after two attempts. Your selection is unchanged; try again.")) }"# + ); +} + +#[tokio::test(flavor = "current_thread", start_paused = true)] +async fn a_failed_confirmation_is_retried_once_then_effort_applies_and_one_prompt_runs() { + let after_model = vec![ + model_option(FABLE), + effort_option("medium", &["low", "medium", "high"]), + ]; + let after_effort = vec![ + model_option(FABLE), + effort_option("high", &["low", "medium", "high"]), + ]; + let run = run_session( + Script::claude( + vec![ + model_option(SONNET), + effort_option("medium", &["low", "medium", "high"]), + ], + vec![ + ConfigReply::Fail(confirmation_error(FABLE)), + ConfigReply::Accept(after_model), + ConfigReply::Accept(after_effort.clone()), + ], + ), + vec![model_selection(FABLE), effort_selection("high")], + CancellationToken::new(), + ) + .await; + + assert_eq!( + run.outcome + .expect("recovered setup should complete the turn"), + (AgentRunOutcome::Completed, SessionSettleReason::Immediate) + ); + assert_eq!( + run.agent.config_calls(), + vec![ + call("model", FABLE), + call("model", FABLE), + call("reasoning", "high") + ], + "the identical model request goes out exactly twice, then effort" + ); + assert_eq!(run.agent.prompts(), 1, "the queued prompt is released once"); + let updates = run.writer.config_option_updates.lock().unwrap(); + assert_eq!(updates.as_slice(), &[after_effort]); + assert!(run + .writer + .config_option_fallbacks + .lock() + .unwrap() + .is_empty()); +} + +#[tokio::test(flavor = "current_thread", start_paused = true)] +async fn two_failed_confirmations_fail_setup_plainly_without_prompting() { + let run = run_session( + Script::claude( + vec![model_option(SONNET)], + vec![ + ConfigReply::Fail(confirmation_error(FABLE)), + ConfigReply::Fail(confirmation_error(FABLE)), + ], + ), + vec![model_selection(FABLE)], + CancellationToken::new(), + ) + .await; + + let error = run.outcome.expect_err("exhausted recovery fails the setup"); + assert_eq!(error, MODEL_CONFIRMATION_EXHAUSTED_MESSAGE); + assert!( + !is_config_selection_unavailable_error(&error), + "the saved selection must survive a confirmation failure" + ); + assert_eq!( + run.agent.config_calls(), + vec![call("model", FABLE), call("model", FABLE)] + ); + assert_eq!(run.agent.prompts(), 0); + assert!(run.writer.config_option_updates.lock().unwrap().is_empty()); +} + +#[tokio::test(flavor = "current_thread", start_paused = true)] +async fn cancelling_during_the_retry_delay_sends_nothing_further() { + let cancel = CancellationToken::new(); + let run = run_session( + Script { + cancel_on_config_call: Some((1, cancel.clone())), + ..Script::claude( + vec![model_option(SONNET)], + vec![ConfigReply::Fail(confirmation_error(FABLE))], + ) + }, + vec![model_selection(FABLE)], + cancel, + ) + .await; + + assert_eq!( + run.outcome.expect("a cancel is a clean outcome"), + (AgentRunOutcome::Cancelled, SessionSettleReason::Cancelled) + ); + assert_eq!(run.agent.config_calls(), vec![call("model", FABLE)]); + assert_eq!(run.agent.prompts(), 0); +} + +#[tokio::test(flavor = "current_thread", start_paused = true)] +async fn cancelling_during_the_second_attempt_sends_no_prompt() { + let cancel = CancellationToken::new(); + let run = run_session( + Script { + cancel_on_config_call: Some((2, cancel.clone())), + ..Script::claude( + vec![model_option(SONNET)], + vec![ + ConfigReply::Fail(confirmation_error(FABLE)), + ConfigReply::Hang, + ], + ) + }, + vec![model_selection(FABLE)], + cancel, + ) + .await; + + assert_eq!( + run.outcome.expect("a cancel is a clean outcome"), + (AgentRunOutcome::Cancelled, SessionSettleReason::Cancelled) + ); + assert_eq!( + run.agent.config_calls(), + vec![call("model", FABLE), call("model", FABLE)] + ); + assert_eq!(run.agent.prompts(), 0); +} + +#[tokio::test(flavor = "current_thread", start_paused = true)] +async fn the_setup_deadline_still_bounds_a_hung_second_attempt() { + let run = run_session( + Script::claude( + vec![model_option(SONNET)], + vec![ + ConfigReply::Fail(confirmation_error(FABLE)), + ConfigReply::Hang, + ], + ), + vec![model_selection(FABLE)], + CancellationToken::new(), + ) + .await; + + let error = run.outcome.expect_err("the setup deadline fails the run"); + assert_eq!( + error, + format!( + "Timed out waiting for ACP protocol startup after {}s", + ACP_SETUP_TIMEOUT.as_secs() + ) + ); + assert_eq!( + run.agent.config_calls(), + vec![call("model", FABLE), call("model", FABLE)] + ); + assert_eq!(run.agent.prompts(), 0, "no late prompt after the deadline"); +} + +/// The retry carries the *resolved* value — here the hint-stripped rename a +/// resumed pin falls back to — and the fallback is announced once, only +/// after the agent has accepted it. +#[tokio::test(flavor = "current_thread", start_paused = true)] +async fn a_retried_fallback_value_is_announced_once_after_acceptance() { + let renamed = || { + vec![SessionConfigOption::select( + "model", + "Model", + "claude-fable-5-1", + vec![ + SessionConfigSelectOption::new("claude-fable-5-1", "Fable"), + SessionConfigSelectOption::new("claude-opus-5[1m]", "Opus"), + ], + ) + .category(SessionConfigOptionCategory::Model)] + }; + let run = run_session( + Script::claude( + renamed(), + vec![ + ConfigReply::Fail(confirmation_error("claude-fable-5-1")), + ConfigReply::Accept(renamed()), + ], + ), + vec![model_selection("claude-fable-5[1m]")], + CancellationToken::new(), + ) + .await; + + assert!(run.outcome.is_ok()); + assert_eq!( + run.agent.config_calls(), + vec![ + call("model", "claude-fable-5-1"), + call("model", "claude-fable-5-1") + ] + ); + assert_eq!(run.agent.prompts(), 1); + assert_eq!( + run.writer + .config_option_fallbacks + .lock() + .unwrap() + .as_slice(), + &[String::from( + "Selected model claude-fable-5[1m] was not available, falling back to \ + claude-fable-5-1 (Fable)." + )] + ); +} + +/// A model accepted on the retry can still narrow the effort options; the +/// existing stale-effort skip applies to the options the retry returned. +#[tokio::test(flavor = "current_thread", start_paused = true)] +async fn effort_options_changed_by_the_retried_model_keep_the_stale_effort_skip() { + let after_model = vec![ + model_option(FABLE), + effort_option("medium", &["low", "medium"]), + ]; + let run = run_session( + Script::claude( + vec![ + model_option(SONNET), + effort_option("medium", &["low", "medium", "high"]), + ], + vec![ + ConfigReply::Fail(confirmation_error(FABLE)), + ConfigReply::Accept(after_model.clone()), + ], + ), + vec![model_selection(FABLE), effort_selection("high")], + CancellationToken::new(), + ) + .await; + + assert!(run.outcome.is_ok()); + assert_eq!( + run.agent.config_calls(), + vec![call("model", FABLE), call("model", FABLE)], + "the now-unavailable effort is skipped, not sent" + ); + assert_eq!(run.agent.prompts(), 1); + assert_eq!( + run.writer.config_option_updates.lock().unwrap().as_slice(), + &[after_model] + ); +} + +/// The bridge re-applies an already-current model on purpose (to repair SDK +/// drift), so an advertised match must not short-circuit `setModel`. +#[tokio::test(flavor = "current_thread", start_paused = true)] +async fn an_already_current_model_is_still_applied() { + let run = run_session( + Script::claude( + vec![model_option(FABLE)], + vec![ConfigReply::Accept(vec![model_option(FABLE)])], + ), + vec![model_selection(FABLE)], + CancellationToken::new(), + ) + .await; + + assert!(run.outcome.is_ok()); + assert_eq!(run.agent.config_calls(), vec![call("model", FABLE)]); + assert_eq!(run.agent.prompts(), 1); +} + +/// Everything that is not Claude failing to confirm the selected model keeps +/// the single attempt and the existing Debug-shaped error. +#[tokio::test(flavor = "current_thread", start_paused = true)] +async fn other_failures_and_other_adapters_keep_the_single_attempt() { + struct Case { + name: &'static str, + adapter: &'static str, + selection: AcpSessionConfigOptionSelection, + error: agent_client_protocol::Error, + expected_call: (String, String), + expected_prefix: &'static str, + } + let cases = vec![ + Case { + name: "authentication required", + adapter: CLAUDE_ADAPTER, + selection: model_selection(FABLE), + error: agent_client_protocol::Error::auth_required(), + expected_call: call("model", FABLE), + expected_prefix: "Failed to set selected ACP model config option: Error { code: -32000", + }, + Case { + name: "unrelated internal error", + adapter: CLAUDE_ADAPTER, + selection: model_selection(FABLE), + error: agent_client_protocol::Error::internal_error(), + expected_call: call("model", FABLE), + expected_prefix: "Failed to set selected ACP model config option: Error { code: -32603", + }, + Case { + name: "confirmation failure for a different model", + adapter: CLAUDE_ADAPTER, + selection: model_selection(FABLE), + error: confirmation_error("claude-opus-5-5"), + expected_call: call("model", FABLE), + expected_prefix: "Failed to set selected ACP model config option: Error { code: -32603", + }, + Case { + name: "the same words from another adapter", + adapter: "@openai/codex-acp", + selection: model_selection(FABLE), + error: confirmation_error(FABLE), + expected_call: call("model", FABLE), + expected_prefix: "Failed to set selected ACP model config option: Error { code: -32603", + }, + Case { + name: "the same words on the effort option", + adapter: CLAUDE_ADAPTER, + selection: effort_selection("high"), + error: confirmation_error("high"), + expected_call: call("reasoning", "high"), + expected_prefix: + "Failed to set selected ACP effort config option: Error { code: -32603", + }, + ]; + + for case in cases { + let run = run_session( + Script { + adapter: case.adapter, + ..Script::claude( + vec![ + model_option(SONNET), + effort_option("medium", &["low", "medium", "high"]), + ], + vec![ConfigReply::Fail(case.error)], + ) + }, + vec![case.selection], + CancellationToken::new(), + ) + .await; + + let error = run + .outcome + .expect_err(&format!("{}: should fail the setup", case.name)); + assert!( + error.starts_with(case.expected_prefix), + "{}: unexpected error {error:?}", + case.name + ); + assert_eq!( + run.agent.config_calls(), + vec![case.expected_call], + "{}: exactly one attempt", + case.name + ); + assert_eq!(run.agent.prompts(), 0, "{}: no prompt", case.name); + } +} + +/// A stale pin is still reported as unavailable before any request goes out, +/// so the runner keeps clearing it exactly as before. +#[tokio::test(flavor = "current_thread", start_paused = true)] +async fn an_unavailable_selection_is_still_reported_without_a_request() { + let run = run_session( + Script::claude(vec![model_option(SONNET)], vec![]), + vec![model_selection("claude-haiku-4")], + CancellationToken::new(), + ) + .await; + + let error = run.outcome.expect_err("a stale pin fails the setup"); + assert!(is_config_selection_unavailable_error(&error), "{error}"); + assert!(run.agent.config_calls().is_empty()); + assert_eq!(run.agent.prompts(), 0); +} diff --git a/crates/acp-client/src/lib.rs b/crates/acp-client/src/lib.rs index f2084d29a..a474d0d48 100644 --- a/crates/acp-client/src/lib.rs +++ b/crates/acp-client/src/lib.rs @@ -37,6 +37,7 @@ pub use driver::{ BackgroundHoldStatus, BackgroundHoldTask, BasicMessageWriter, MessageWriter, OutOfTurnPermissionPolicy, PromptImage, ReplayBoundary, SessionConnection, SessionLifetime, SessionSettleReason, SessionSettled, Store, BACKGROUND_CONTINUATION_ORIGIN, + MODEL_CONFIRMATION_EXHAUSTED_MESSAGE, }; pub use simple::{run_acp_prompt, run_acp_prompt_with_interpreter_env_snapshot}; pub use types::{ From c91b510b41aa15bdf797f602de797654772c6cb4 Mon Sep 17 00:00:00 2001 From: Matt Toohey Date: Sat, 17 Oct 2026 14:46:27 +1100 Subject: [PATCH 2/3] fix(acp-client): surface exhausted model confirmation as a plain sentence and pin its wording source MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses both findings from the review of 1aef5a6d. Bridge-wording provenance. `is_model_confirmation_error` matches the sentence `Couldn't confirm model ""`, and the earlier commit gave no hint of where that text comes from. It is not the bridge's own: `@agentclientprotocol/claude-agent-acp` 0.90.0's `setSessionConfigOption` (`dist/acp-agent.js`) rethrows whatever `query.setModel(resolvedValue)` rejects with, `@anthropic-ai/claude-agent-sdk` 0.3.296 relays that from the Claude Code CLI it vendors (`claude-agent-sdk-darwin-arm64/claude`, Claude Code 2.1.296), and the CLI's model-validation module builds `Couldn't confirm model "${model}" with the API. Try again…` with `retryable: true` when its deadline-wrapped API probe returns no answer. The comment on `MODEL_CONFIRMATION_FAILED_PREFIX` now records that chain, that `${model}` is the value id echoed verbatim including `[1m]` (hence the matcher keys on the resolved value), that an upstream rewording silently disables the retry and falls back to today's single attempt, and that it must be re-verified on adapter upgrades. Exhausted recovery reaching the user. `run_acp_session` returned the plain `MODEL_CONFIRMATION_EXHAUSTED_MESSAGE`, but the `connect_with` closure has to wrap every setup `String` in `util::internal_error` to satisfy the ACP error type, and `run` then Debug-formatted it, so the session alert showed `ACP protocol failed: Error { code: -32603 …, data: Some(String("…")) }` while the doc comment claimed otherwise. `run` now maps the connection result through `describe_protocol_failure`, which unwraps exactly that one marker — an internal error whose `data` is the exhausted sentence — back to the sentence, and keeps the Debug shape for everything else. The fix lives in the driver rather than the runner because that is where the wrapping happens; the round trip through the ACP `Error` is forced by `connect_with`, so one targeted unwrap on the way out is the smallest change that makes the user-facing string plain. Auth-required, invalid-selection, transport, and generic errors keep the exact wrapper the driver's auth tests and the frontend's `isAuthenticationError` fixtures pin, nothing outside tests matches on the `ACP protocol failed:` prefix, and the plain sentence still does not satisfy `is_config_selection_unavailable_error`, so the saved selection survives. Per-attempt diagnostics stay in the sanitized `send_config_selection` log records. The doc comments on `send_config_selection` and the exhausted constant now describe the real behaviour. Tests: `exhausted_message_is_not_an_unavailable_selection` now asserts the alert text through `describe_protocol_failure` is the plain sentence and that an auth-required and a bare transport error keep their Debug shape; the runner's selection-retention test feeds the sentence unwrapped, as the driver now delivers it. `cargo fmt` clean; `cargo clippy --workspace -- -D warnings` and src-tauri `cargo clippy -- -D warnings` clean; `cargo test -p acp-client` 196 passed; src-tauri `session_runner::` 119 passed. Co-Authored-By: Claude Fable 5.1 Signed-off-by: Matt Toohey --- apps/staged/src-tauri/src/session_runner.rs | 18 +++--- crates/acp-client/src/driver.rs | 58 ++++++++++++++++--- .../src/driver/tests/model_confirmation.rs | 37 ++++++++---- 3 files changed, 84 insertions(+), 29 deletions(-) diff --git a/apps/staged/src-tauri/src/session_runner.rs b/apps/staged/src-tauri/src/session_runner.rs index e8b959765..87959f3b2 100644 --- a/apps/staged/src-tauri/src/session_runner.rs +++ b/apps/staged/src-tauri/src/session_runner.rs @@ -4997,9 +4997,11 @@ mod tests { } /// acp-client retries Claude's model confirmation once and then fails - /// with a plain sentence. The post-run clearing must read that as - /// "selection fine, try again" and leave the saved pin alone, while a - /// genuinely stale pin keeps being cleared exactly as before. + /// with a plain sentence, which its `run` hands through unwrapped. The + /// post-run clearing must read that as "selection fine, try again" and + /// leave the saved pin alone, while a genuinely stale pin — still + /// Debug-wrapped like every other setup failure — keeps being cleared + /// exactly as before. #[test] fn a_failed_model_confirmation_keeps_the_saved_selection_but_a_stale_pin_is_cleared() { let store = Store::in_memory().unwrap(); @@ -5010,18 +5012,12 @@ mod tests { .set_session_acp_config_selection(&session.id, Some(&selection)) .unwrap(); - // Exactly as the driver's `run` wraps the setup failure. - let exhausted = format!( - "ACP protocol failed: {:?}", - agent_client_protocol::util::internal_error( - acp_client::MODEL_CONFIRMATION_EXHAUSTED_MESSAGE - ) - ); + // Exactly what the driver's `run` records: the sentence, unwrapped. assert!(!clear_unavailable_acp_config_selection( &store, &session.id, Some(&selection), - &exhausted, + acp_client::MODEL_CONFIRMATION_EXHAUSTED_MESSAGE, )); assert_eq!( store diff --git a/crates/acp-client/src/driver.rs b/crates/acp-client/src/driver.rs index c3bf4daec..204ca228e 100644 --- a/crates/acp-client/src/driver.rs +++ b/crates/acp-client/src/driver.rs @@ -1985,7 +1985,7 @@ impl AcpDriver { .map_err(agent_client_protocol::util::internal_error) }) .await - .map_err(|e| format!("ACP protocol failed: {e:?}")); + .map_err(describe_protocol_failure); writer.finalize().await; graceful_stop(&mut child, is_remote).await; @@ -5488,16 +5488,57 @@ pub fn is_config_selection_unavailable_error(error: &str) -> bool { // in the `details` of an internal error. That is confirmation uncertainty, // not an unavailable model: the same selection is routinely accepted seconds // later. [`send_config_selection`] bounds its recovery to exactly this error. +// +// Provenance of the wording — re-verify whenever the adapter is upgraded. +// `@agentclientprotocol/claude-agent-acp` 0.90.0 does not word this itself: +// its `setSessionConfigOption` (`dist/acp-agent.js`) rethrows whatever +// `query.setModel(resolvedValue)` rejects with, and `@anthropic-ai/ +// claude-agent-sdk` 0.3.296 relays that from the Claude Code CLI it vendors +// (`@anthropic-ai/claude-agent-sdk-darwin-arm64/claude`, Claude Code 2.1.296). +// The CLI's model-validation module builds the message when its +// deadline-wrapped API probe returns without an answer, flagged +// `retryable: true` (identifiers minified in the shipped binary): +// +// `Couldn't confirm model "${model}" with the API. Try again[…].` +// +// `${model}` is the value id the bridge passed to `setModel`, echoed verbatim +// including suffixes such as `[1m]` (session records show +// `claude-fable-5-1[1m]`), which is why the matcher is keyed to the *resolved* +// value. A rewording upstream silently disables the retry — nothing fails, the +// run just falls back to today's single attempt and the raw error — and only +// this repo's own fixtures in `driver/tests/model_confirmation.rs` pin the +// contract. const MODEL_CONFIRMATION_FAILED_PREFIX: &str = "Couldn't confirm model \""; const MODEL_CONFIRMATION_ATTEMPTS: u32 = 2; const MODEL_CONFIRMATION_RETRY_DELAY: Duration = Duration::from_secs(1); -/// What the user reads once every confirmation attempt has failed. Kept clear -/// of the [`is_config_selection_unavailable_error`] fragments on purpose: the -/// selection itself is sound, so consumers must keep it for the retry the -/// sentence asks for. +/// What the user reads once every confirmation attempt has failed; +/// [`describe_protocol_failure`] delivers it to the session alert as this +/// sentence. Kept clear of the [`is_config_selection_unavailable_error`] +/// fragments on purpose: the selection itself is sound, so consumers must keep +/// it for the retry the sentence asks for. pub const MODEL_CONFIRMATION_EXHAUSTED_MESSAGE: &str = "Claude couldn't confirm the selected model after two attempts. Your selection is unchanged; try again."; +/// The text `run` records for a connection that ended in an error. +/// +/// Setup failures arrive here as the `internal_error` the `connect_with` +/// closure had to wrap their `String` in to satisfy the ACP error type, and +/// the default is to keep that Debug shape: the auth-recovery matchers here +/// and in the frontend (`authRecovery.ts`) are keyed to it, and the detail it +/// carries is what diagnostics want. Exhausted model confirmation is the one +/// setup failure whose wording is written for the user, so it alone is +/// unwrapped back to [`MODEL_CONFIRMATION_EXHAUSTED_MESSAGE`]; the per-attempt +/// records logged by [`send_config_selection`] keep the technical detail. +fn describe_protocol_failure(error: agent_client_protocol::Error) -> String { + if error.code == ErrorCode::InternalError + && error.data.as_ref().and_then(|data| data.as_str()) + == Some(MODEL_CONFIRMATION_EXHAUSTED_MESSAGE) + { + return MODEL_CONFIRMATION_EXHAUSTED_MESSAGE.to_string(); + } + format!("ACP protocol failed: {error:?}") +} + /// Whether `adapter` — the agent's `initialize` name, or the driver label when /// it sends none — is a Claude bridge. Only Claude's SDK produces the /// confirmation failure, so only Claude gets the retry; another agent that @@ -5511,6 +5552,8 @@ fn supports_model_confirmation_recovery(adapter: &str) -> bool { /// [`MODEL_CONFIRMATION_FAILED_PREFIX`], either as the bridge's /// `{"details": …}` object or as a bare string. Authentication errors, invalid /// values, transport failures, and a different model are all not this error. +/// The wording's origin and upgrade caveats are on +/// [`MODEL_CONFIRMATION_FAILED_PREFIX`]. fn is_model_confirmation_error( error: &agent_client_protocol::Error, requested_model: &str, @@ -5538,8 +5581,9 @@ fn is_model_confirmation_error( /// never for any other option, error, or adapter. The caller's setup timeout /// and cancellation bound the whole exchange by dropping this future, so a /// Stop during the delay or the second attempt sends nothing further. -/// Exhausted recovery reads as [`MODEL_CONFIRMATION_EXHAUSTED_MESSAGE`] -/// rather than the nested Debug dump. +/// Exhausted recovery fails with [`MODEL_CONFIRMATION_EXHAUSTED_MESSAGE`], +/// which [`describe_protocol_failure`] hands to the session alert as that +/// plain sentence while every other setup failure keeps its Debug shape. /// /// Each attempt logs a sanitized record — phase, adapter, requested and /// resolved value, attempt, duration, classified outcome; never prompts, diff --git a/crates/acp-client/src/driver/tests/model_confirmation.rs b/crates/acp-client/src/driver/tests/model_confirmation.rs index 96930cb64..74dd66302 100644 --- a/crates/acp-client/src/driver/tests/model_confirmation.rs +++ b/crates/acp-client/src/driver/tests/model_confirmation.rs @@ -9,8 +9,8 @@ use super::*; use crate::driver::{ - is_model_confirmation_error, run_acp_session, supports_model_confirmation_recovery, - ACP_SETUP_TIMEOUT, MODEL_CONFIRMATION_EXHAUSTED_MESSAGE, + describe_protocol_failure, is_model_confirmation_error, run_acp_session, + supports_model_confirmation_recovery, ACP_SETUP_TIMEOUT, MODEL_CONFIRMATION_EXHAUSTED_MESSAGE, }; use agent_client_protocol::schema::v1::{ Implementation, InitializeRequest, InitializeResponse, PromptResponse, @@ -309,22 +309,37 @@ fn recovery_is_keyed_to_claude_bridges_only() { assert!(!supports_model_confirmation_recovery("Blox")); } -/// The terminal sentence must keep the saved selection: the runner clears a -/// selection only when `is_config_selection_unavailable_error` fires. +/// The terminal sentence must keep the saved selection — the runner clears a +/// selection only when `is_config_selection_unavailable_error` fires — and it +/// must reach the session alert as the sentence itself: `run` unwraps the +/// `internal_error` the connection closure wrapped it in, while every other +/// setup failure keeps the Debug shape the auth-recovery fixtures pin. #[test] fn exhausted_message_is_not_an_unavailable_selection() { assert!(!is_config_selection_unavailable_error( MODEL_CONFIRMATION_EXHAUSTED_MESSAGE )); - // Exactly as `run` wraps it on the way to the session alert. - let wrapped = format!( - "ACP protocol failed: {:?}", - agent_client_protocol::util::internal_error(MODEL_CONFIRMATION_EXHAUSTED_MESSAGE) + // Exactly as the `connect_with` closure wraps a setup failure before + // `run` describes it for the session alert. + let shown = describe_protocol_failure(agent_client_protocol::util::internal_error( + MODEL_CONFIRMATION_EXHAUSTED_MESSAGE, + )); + assert_eq!(shown, MODEL_CONFIRMATION_EXHAUSTED_MESSAGE); + assert!(!is_config_selection_unavailable_error(&shown)); + + // Any other setup failure, and a bare transport error, keep the shape + // `isAuthenticationError` and the driver's auth tests are keyed to. + let auth = agent_client_protocol::util::internal_error( + "ACP authentication is required to create ACP session. Sign this agent in, then retry.", + ); + assert_eq!( + describe_protocol_failure(auth.clone()), + format!("ACP protocol failed: {auth:?}") ); - assert!(!is_config_selection_unavailable_error(&wrapped)); + let transport = agent_client_protocol::Error::internal_error(); assert_eq!( - wrapped, - r#"ACP protocol failed: Error { code: -32603: Internal error, message: "Internal error", data: Some(String("Claude couldn't confirm the selected model after two attempts. Your selection is unchanged; try again.")) }"# + describe_protocol_failure(transport.clone()), + format!("ACP protocol failed: {transport:?}") ); } From 82a2d3d36aa4878d8dc9a6ba1ebb21e87a645a88 Mon Sep 17 00:00:00 2001 From: Matt Toohey Date: Sat, 17 Oct 2026 14:57:09 +1100 Subject: [PATCH 3/3] test(acp-client): pin auth-required Debug shape through describe_protocol_failure MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the remaining finding from the review of c91b510b. That commit introduced `describe_protocol_failure` as the one place `run` turns a connection error into the session alert text, unwrapping exactly the exhausted-model-confirmation marker and keeping the `ACP protocol failed: {error:?}` Debug shape for everything else. The three auth-required tests that pin that Debug shape — the same bytes the frontend's `isAuthenticationError` fixtures in `authRecovery.test.ts` expect — were still hand-formatting `format!("ACP protocol failed: {error:?}")`, so they proved what `run` used to do rather than what the new function does. They now call `describe_protocol_failure` directly (the `session/new` and `session/load` setup variants the review named, plus the identical run-prompt variant between them). The expected strings are byte-for-byte unchanged; a future widening of the unwrap condition fails these tests directly instead of only the single auth sample in `exhausted_message_is_not_an_unavailable_selection`. The provenance comment on `MODEL_CONFIRMATION_FAILED_PREFIX` is also tightened against the installed bundle (`@agentclientprotocol/ claude-agent-acp` 0.90.0 → `@anthropic-ai/claude-agent-sdk` 0.3.296 → vendored Claude Code 2.1.296): - It now records the single-quoted sibling the same binary carries, `Couldn't confirm model '' is available (the check timed out) · try again`, which is the inline `/model` command's `{type: "text"}` reply rather than an error and never reaches `setModel` — the reason the matcher keys on the double-quoted form. - The review asked that `retryable: true` be qualified as sourced from un-minified Claude Code rather than the shipped bundle, or dropped. Reading the binary shows the flag does survive minification: the validation path returns `` error:`Couldn't confirm model "${…}" with the API. …`,retryable:!0 `` verbatim, with only identifiers mangled. The comment says that precisely instead of hedging. No production behaviour changes. `cargo fmt` clean; `cargo clippy --workspace -- -D warnings` clean; `cargo test -p acp-client` 196 passed. Co-Authored-By: Claude Fable 5.1 Signed-off-by: Matt Toohey --- crates/acp-client/src/driver.rs | 40 +++++++++++++++++++++------------ 1 file changed, 26 insertions(+), 14 deletions(-) diff --git a/crates/acp-client/src/driver.rs b/crates/acp-client/src/driver.rs index 204ca228e..d4b156125 100644 --- a/crates/acp-client/src/driver.rs +++ b/crates/acp-client/src/driver.rs @@ -5496,11 +5496,21 @@ pub fn is_config_selection_unavailable_error(error: &str) -> bool { // claude-agent-sdk` 0.3.296 relays that from the Claude Code CLI it vendors // (`@anthropic-ai/claude-agent-sdk-darwin-arm64/claude`, Claude Code 2.1.296). // The CLI's model-validation module builds the message when its -// deadline-wrapped API probe returns without an answer, flagged -// `retryable: true` (identifiers minified in the shipped binary): +// deadline-wrapped API probe returns without an answer. Identifiers are +// minified in the shipped binary, but the template literal and its +// `retryable:!0` (`true`) flag survive verbatim and can be read straight out +// of it: // // `Couldn't confirm model "${model}" with the API. Try again[…].` // +// The same binary carries a *single-quoted* sibling that must not be matched: +// +// `Couldn't confirm model '${model}' is available (the check timed out) · try again` +// +// That one is the inline `/model` command's `{type: "text"}` reply, not an +// error, and never reaches `setModel`; the matcher keys on the double-quoted +// form for that reason. +// // `${model}` is the value id the bridge passed to `setModel`, echoed verbatim // including suffixes such as `[1m]` (session records show // `claude-fable-5-1[1m]`), which is why the matcher is keyed to the *resolved* @@ -6661,7 +6671,7 @@ mod tests { autoapprove_permission_decision, background_continuation_origin, background_task_tracking_meta, build_prompt_content_blocks, carry_config_value_hint, consume_remote_acp_line, decode_remote_acp_line, defensive_permission_decision, - describe_prompt_error, hold_for_background_quiescence, + describe_prompt_error, describe_protocol_failure, hold_for_background_quiescence, is_config_selection_unavailable_error, is_missing_mcp_transport_error, labeled_background_continuation_origin, mcp_server_transport_supported, origin_task_name_label, permission_response_for_decision, permission_response_for_options, @@ -6791,10 +6801,12 @@ mod tests { .expect_err("auth_required should be surfaced without authenticate"); assert!(format!("{error:?}").contains("ACP authentication is required")); - // The exact string the session alert shows once `run` has wrapped it; - // `authRecovery.test.ts` pins the same bytes on the frontend side. + // The exact string the session alert shows once `run` has passed it + // through `describe_protocol_failure`; `authRecovery.test.ts` pins the + // same bytes on the frontend side, so the unwrap there must leave + // auth-required errors in their Debug shape. assert_eq!( - format!("ACP protocol failed: {error:?}"), + describe_protocol_failure(error), r#"ACP protocol failed: Error { code: -32603: Internal error, message: "Internal error", data: Some(String("ACP authentication is required to create ACP session. Sign this agent in, then retry.")) }"# ); assert_eq!(calls.lock().unwrap().as_slice(), &["session/new"]); @@ -6852,13 +6864,11 @@ mod tests { ); assert!(error.contains("authentication")); - // `run` wraps every protocol failure the same way before it reaches - // the session alert; `authRecovery.test.ts` pins this exact string on - // the frontend side, so the two must agree byte for byte. - let wrapped = format!( - "ACP protocol failed: {:?}", - agent_client_protocol::util::internal_error(error) - ); + // `run` passes every protocol failure through + // `describe_protocol_failure` before it reaches the session alert; + // `authRecovery.test.ts` pins this exact string on the frontend side, + // so the two must agree byte for byte. + let wrapped = describe_protocol_failure(agent_client_protocol::util::internal_error(error)); assert_eq!( wrapped, r#"ACP protocol failed: Error { code: -32603: Internal error, message: "Internal error", data: Some(String("ACP authentication is required to run the prompt. Sign this agent in, then retry.")) }"# @@ -7321,8 +7331,10 @@ mod tests { .await .expect_err("auth_required at session/load should fail the setup"); + // Same contract as the `session/new` variant: `describe_protocol_failure` + // must keep auth-required errors in the Debug shape the frontend pins. assert_eq!( - format!("ACP protocol failed: {error:?}"), + describe_protocol_failure(error), r#"ACP protocol failed: Error { code: -32603: Internal error, message: "Internal error", data: Some(String("ACP authentication is required to load ACP session. Sign this agent in, then retry.")) }"# ); assert_eq!(