From 189ea37507e8fbfab4b829833e9d1397c3383b67 Mon Sep 17 00:00:00 2001 From: Matt Toohey Date: Fri, 21 Aug 2026 13:11:45 +1000 Subject: [PATCH 01/13] fix(acp): defer authentication until required Signed-off-by: Matt Toohey --- crates/acp-client/src/driver.rs | 324 +++++++++++++++++++++++++++----- 1 file changed, 277 insertions(+), 47 deletions(-) diff --git a/crates/acp-client/src/driver.rs b/crates/acp-client/src/driver.rs index d76263ea..2b30efa1 100644 --- a/crates/acp-client/src/driver.rs +++ b/crates/acp-client/src/driver.rs @@ -21,9 +21,9 @@ use agent_client_protocol::{ v1::{ AgentCapabilities, AgentNotification, AuthMethod, AuthenticateRequest, CancelNotification, ClientCapabilities, ContentBlock as AcpContentBlock, ContentChunk, - ExtNotification, ImageContent, Implementation, InitializeRequest, InitializeResponse, - LoadSessionRequest, McpCapabilities, McpServer, Meta, NewSessionRequest, - PermissionOption as SchemaPermissionOption, PermissionOptionId, + ErrorCode, ExtNotification, ImageContent, Implementation, InitializeRequest, + InitializeResponse, LoadSessionRequest, McpCapabilities, McpServer, Meta, + NewSessionRequest, PermissionOption as SchemaPermissionOption, PermissionOptionId, PermissionOptionKind as SchemaPermissionOptionKind, PromptRequest, PromptResponse, RequestPermissionOutcome, RequestPermissionRequest, RequestPermissionResponse, SelectedPermissionOutcome, SessionConfigKind, SessionConfigOption, @@ -33,7 +33,7 @@ use agent_client_protocol::{ }, ProtocolVersion, }, - Agent, ByteStreams, Client, ConnectionTo, JsonRpcMessage, UntypedMessage, + Agent, ByteStreams, Client, ConnectionTo, JsonRpcMessage, JsonRpcRequest, UntypedMessage, }; use async_trait::async_trait; use tokio::io::{AsyncBufReadExt, AsyncWriteExt, BufReader}; @@ -5588,7 +5588,9 @@ async fn setup_acp_session(context: AcpSessionSetupContext<'_>) -> Result) -> Result) -> Result { - let new_session_request = NewSessionRequest::new(working_dir.to_path_buf()) - .mcp_servers(mcp_servers.to_vec()) - .meta(background_task_tracking_meta(task_tracking_mode)); - let session_response = connection - .send_request(new_session_request) - .block_task() - .await - .map_err(|e| format!("Failed to create ACP session: {e:?}"))?; + let new_session_request = || { + NewSessionRequest::new(working_dir.to_path_buf()) + .mcp_servers(mcp_servers.to_vec()) + .meta(background_task_tracking_meta(task_tracking_mode)) + }; + let session_response = send_session_setup_request( + connection, + new_session_request, + &init_response.auth_methods, + "create ACP session", + ) + .await?; let new_id = session_response.session_id.to_string(); store @@ -5751,16 +5760,41 @@ fn describe_auth_methods(auth_methods: &[AuthMethod]) -> String { .join(", ") } -async fn authenticate_if_advertised( +async fn send_session_setup_request( + connection: &ConnectionTo, + make_request: MakeRequest, + auth_methods: &[AuthMethod], + operation: &str, +) -> Result +where + Req: JsonRpcRequest, + MakeRequest: Fn() -> Req, +{ + match connection.send_request(make_request()).block_task().await { + Ok(response) => Ok(response), + Err(error) if error.code == ErrorCode::AuthRequired => { + authenticate_with_usable_method(connection, auth_methods).await?; + connection + .send_request(make_request()) + .block_task() + .await + .map_err(|error| format!("Failed to {operation} after authentication: {error:?}")) + } + Err(error) => Err(format!("Failed to {operation}: {error:?}")), + } +} + +async fn authenticate_with_usable_method( connection: &ConnectionTo, auth_methods: &[AuthMethod], ) -> Result<(), String> { - let Some(method) = auth_methods.first() else { - return Ok(()); - }; + let method = select_auth_method(auth_methods).ok_or_else(|| { + "ACP authentication is required, but the agent advertised no usable authentication method" + .to_string() + })?; log::debug!( - "ACP agent advertised authentication methods; selecting {} ({})", + "ACP authentication required; selecting {} ({})", method.name(), method.id() ); @@ -5769,9 +5803,9 @@ async fn authenticate_if_advertised( .send_request(AuthenticateRequest::new(method.id().clone())) .block_task() .await - .map_err(|e| { + .map_err(|error| { format!( - "ACP authentication failed with method {} ({}): {e:?}", + "ACP authentication failed with method {} ({}): {error:?}", method.name(), method.id() ) @@ -5780,6 +5814,46 @@ async fn authenticate_if_advertised( Ok(()) } +fn select_auth_method(auth_methods: &[AuthMethod]) -> Option<&AuthMethod> { + // Agent-managed methods do not require the client to collect credentials. + // Prefer a non-API-key method so providers such as Codex use an existing + // browser login instead of an unavailable key merely because `api-key` was + // advertised first. + auth_methods + .iter() + .find(|method| matches!(method, AuthMethod::Agent(_)) && !looks_like_api_key(method)) + .or_else(|| { + auth_methods + .iter() + .find(|method| auth_method_is_usable(method)) + }) +} + +fn auth_method_is_usable(method: &AuthMethod) -> bool { + match method { + // Agent-managed methods own their credential lookup. If no better + // method is available, let the agent report any missing credential. + AuthMethod::Agent(_) => true, + AuthMethod::EnvVar(method) => method.vars.iter().all(|var| { + var.optional || std::env::var_os(&var.name).is_some_and(|value| !value.is_empty()) + }), + // Terminal methods require a separate interactive client flow that the + // driver does not currently implement. Unknown future methods are also + // unusable until the client explicitly supports their flow. + AuthMethod::Terminal(_) => false, + _ => false, + } +} + +fn looks_like_api_key(method: &AuthMethod) -> bool { + [method.id().to_string(), method.name().to_string()] + .iter() + .any(|value| { + let value = value.to_ascii_lowercase(); + value.contains("api") && value.contains("key") + }) +} + fn build_prompt_content_blocks( prompt: &str, images: &[(String, String)], @@ -5943,27 +6017,29 @@ mod tests { resolve_acp_working_dir, resolve_session_config_option_selection, resolve_spawn_working_dir, sanitize_remote_acp_chunk, sdk_message_mentions_task, sdk_message_origin_kind, sdk_message_session_state, sdk_message_settles_task, - shell_exec_line, shell_quote, task_tracking_mode_from_initialize, AcpDriver, - AcpEventMetadata, AcpNotificationHandler, AcpPermissionDecision, AcpPermissionOption, + select_auth_method, send_session_setup_request, setup_acp_session, shell_exec_line, + shell_quote, task_tracking_mode_from_initialize, AcpDriver, AcpEventMetadata, + AcpNotificationHandler, AcpPermissionDecision, AcpPermissionOption, AcpPermissionOptionKind, AcpPermissionRequest, AcpSessionConfigOptionSelection, - AcpToolCallMetadata, AgentRunOutcome, AsyncTaskNotification, AsyncTaskState, - AsyncTaskStopHandle, AsyncTaskUpdate, BackgroundActivity, BackgroundHoldConfig, - BackgroundHoldObserver, BackgroundHoldStatus, BackgroundHoldTask, BackgroundTaskSet, - BasicMessageWriter, HoldOutcome, HoldSettle, HoldingState, IncomingSessionUpdate, - MessageWriter, OutOfTurnPermissionPolicy, QueuedSessionTurn, RemoteLineOutcome, - ReplayBoundary, ReplayBuffer, ReplayEvent, SdkSessionState, SessionLifetime, - SessionSettleReason, SessionSettled, StopAsyncTaskRequest, TaskTrackingMode, - TypedAsyncTaskSet, ASYNC_TASK_STOP_METHOD, AVAILABILITY_PROBE_SUBTYPE, + AcpSessionSetupContext, AcpToolCallMetadata, AgentRunOutcome, AsyncTaskNotification, + AsyncTaskState, AsyncTaskStopHandle, AsyncTaskUpdate, BackgroundActivity, + BackgroundHoldConfig, BackgroundHoldObserver, BackgroundHoldStatus, BackgroundHoldTask, + BackgroundTaskSet, BasicMessageWriter, HoldOutcome, HoldSettle, HoldingState, + IncomingSessionUpdate, MessageWriter, OutOfTurnPermissionPolicy, QueuedSessionTurn, + RemoteLineOutcome, ReplayBoundary, ReplayBuffer, ReplayEvent, SdkSessionState, + SessionLifetime, SessionSettleReason, SessionSettled, StopAsyncTaskRequest, Store, + TaskTrackingMode, TypedAsyncTaskSet, ASYNC_TASK_STOP_METHOD, AVAILABILITY_PROBE_SUBTYPE, BACKGROUND_CONTINUATION_ORIGIN, BACKGROUND_TASK_SUBTYPES, CLAUDE_SDK_MESSAGE_METHOD, CONTINUATION_MESSAGE_ID_PREFIX, ORIGIN_TASK_NAME_MAX_CHARS, PERMISSION_ANNOUNCEMENT_GRACE, SESSION_STATE_SUBTYPE, TASK_NOTIFICATION_ORIGIN, }; use agent_client_protocol::schema::v1::{ + AuthMethod, AuthMethodAgent, AuthenticateRequest, AuthenticateResponse, ContentBlock as AcpContentBlock, ContentChunk, ExtNotification, McpCapabilities, McpServer, - McpServerHttp, McpServerSse, McpServerStdio, PermissionOption, PermissionOptionKind, Plan, - PlanEntry, PlanEntryPriority, PlanEntryStatus, RequestPermissionOutcome, - RequestPermissionRequest, SessionConfigOption, SessionConfigOptionCategory, - SessionConfigSelectOption, SessionNotification, SessionUpdate, + McpServerHttp, McpServerSse, McpServerStdio, NewSessionRequest, NewSessionResponse, + PermissionOption, PermissionOptionKind, Plan, PlanEntry, PlanEntryPriority, + PlanEntryStatus, RequestPermissionOutcome, RequestPermissionRequest, SessionConfigOption, + SessionConfigOptionCategory, SessionConfigSelectOption, SessionNotification, SessionUpdate, SetSessionConfigOptionRequest, SetSessionConfigOptionResponse, StopReason, TextContent, ToolCall, ToolCallUpdate, ToolCallUpdateFields, }; @@ -5976,6 +6052,160 @@ mod tests { use tokio::sync::{mpsc, oneshot}; use tokio_util::sync::CancellationToken; + #[test] + fn auth_selection_prefers_chat_login_over_first_advertised_api_key() { + let methods = vec![ + AuthMethod::Agent(AuthMethodAgent::new("api-key", "API Key")), + AuthMethod::Agent(AuthMethodAgent::new("chat-gpt", "ChatGPT")), + ]; + + let selected = select_auth_method(&methods).expect("chat login should be usable"); + + assert_eq!(selected.id().to_string(), "chat-gpt"); + } + + #[tokio::test(flavor = "current_thread")] + async fn session_setup_authenticates_with_usable_method_and_retries_on_auth_required() { + let calls = Arc::new(Mutex::new(Vec::::new())); + let calls_for_auth = Arc::clone(&calls); + let calls_for_session = Arc::clone(&calls); + let session_attempts = Arc::new(std::sync::atomic::AtomicUsize::new(0)); + let session_attempts_for_handler = Arc::clone(&session_attempts); + let agent = agent_client_protocol::Agent + .builder() + .on_receive_request( + async move |request: AuthenticateRequest, responder, _cx| { + calls_for_auth + .lock() + .unwrap() + .push(format!("authenticate:{}", request.method_id)); + responder.respond(AuthenticateResponse::new()) + }, + agent_client_protocol::on_receive_request!(), + ) + .on_receive_request( + async move |_request: NewSessionRequest, responder, _cx| { + calls_for_session.lock().unwrap().push("session/new".into()); + if session_attempts_for_handler + .fetch_add(1, std::sync::atomic::Ordering::SeqCst) + == 0 + { + responder.respond_with_error(agent_client_protocol::Error::auth_required()) + } else { + responder.respond(NewSessionResponse::new("session-1")) + } + }, + agent_client_protocol::on_receive_request!(), + ); + let methods = vec![ + AuthMethod::Agent(AuthMethodAgent::new("api-key", "API Key")), + AuthMethod::Agent(AuthMethodAgent::new("chat-gpt", "ChatGPT")), + ]; + + agent_client_protocol::Client + .connect_with(agent, async |connection| { + send_session_setup_request( + &connection, + || NewSessionRequest::new(PathBuf::from("/tmp")), + &methods, + "create ACP session", + ) + .await + .map(|_| ()) + .map_err(agent_client_protocol::util::internal_error) + }) + .await + .expect("protocol should succeed"); + + assert_eq!( + calls.lock().unwrap().as_slice(), + &["session/new", "authenticate:chat-gpt", "session/new"] + ); + } + + #[derive(Default)] + struct RecordingStore { + agent_session_ids: Mutex>, + } + + #[async_trait::async_trait] + impl Store for RecordingStore { + fn set_agent_session_id( + &self, + session_id: &str, + agent_session_id: &str, + ) -> Result<(), String> { + self.agent_session_ids + .lock() + .unwrap() + .push((session_id.to_string(), agent_session_id.to_string())); + Ok(()) + } + } + + #[tokio::test(flavor = "current_thread")] + async fn full_session_setup_uses_existing_login_without_eager_authentication() { + use agent_client_protocol::schema::v1::{InitializeRequest, InitializeResponse}; + + let calls = Arc::new(Mutex::new(Vec::::new())); + let calls_for_auth = Arc::clone(&calls); + let calls_for_session = Arc::clone(&calls); + let agent = agent_client_protocol::Agent + .builder() + .on_receive_request( + async |request: InitializeRequest, responder, _cx| { + responder.respond( + InitializeResponse::new(request.protocol_version).auth_methods(vec![ + AuthMethod::Agent(AuthMethodAgent::new("api-key", "API Key")), + AuthMethod::Agent(AuthMethodAgent::new("chat-gpt", "ChatGPT")), + ]), + ) + }, + agent_client_protocol::on_receive_request!(), + ) + .on_receive_request( + async move |request: AuthenticateRequest, responder, _cx| { + calls_for_auth + .lock() + .unwrap() + .push(format!("authenticate:{}", request.method_id)); + responder.respond_with_error(agent_client_protocol::Error::internal_error()) + }, + agent_client_protocol::on_receive_request!(), + ) + .on_receive_request( + async move |_request: NewSessionRequest, responder, _cx| { + calls_for_session.lock().unwrap().push("session/new".into()); + responder.respond(NewSessionResponse::new("session-1")) + }, + agent_client_protocol::on_receive_request!(), + ); + let store: Arc = Arc::new(RecordingStore::default()); + let writer: Arc = Arc::new(BasicMessageWriter::new()); + + agent_client_protocol::Client + .connect_with(agent, async |connection| { + setup_acp_session(AcpSessionSetupContext { + connection: &connection, + working_dir: Path::new("/tmp"), + store: &store, + writer: &writer, + our_session_id: "local-session", + acp_session_id: None, + config_options: &[], + mcp_servers: &[], + agent_label: "Codex", + }) + .await + .map(|_| ()) + .map_err(agent_client_protocol::util::internal_error) + }) + .await + .expect("existing login should create a session without authenticate"); + + assert_eq!(calls.lock().unwrap().as_slice(), &["session/new"]); + } + fn unique_test_dir(prefix: &str) -> PathBuf { let nonce = SystemTime::now() .duration_since(UNIX_EPOCH) From 19720a9a29759b4a187bb4805abeb5d1e6eb3d30 Mon Sep 17 00:00:00 2001 From: Matt Toohey Date: Fri, 21 Aug 2026 14:29:32 +1000 Subject: [PATCH 02/13] fix(acp): require explicit authentication handling Signed-off-by: Matt Toohey --- apps/staged/src-tauri/src/pikchr_mcp.rs | 2 + .../staged/src-tauri/src/pikchr_subsession.rs | 4 + apps/staged/src-tauri/src/session_runner.rs | 1 + crates/acp-client/src/driver.rs | 475 +++++++++++++++--- crates/acp-client/src/lib.rs | 12 +- crates/acp-client/src/simple.rs | 3 + 6 files changed, 416 insertions(+), 81 deletions(-) diff --git a/apps/staged/src-tauri/src/pikchr_mcp.rs b/apps/staged/src-tauri/src/pikchr_mcp.rs index 99115aa5..aa3ebfe7 100644 --- a/apps/staged/src-tauri/src/pikchr_mcp.rs +++ b/apps/staged/src-tauri/src/pikchr_mcp.rs @@ -1713,6 +1713,7 @@ arrow from COLL.e to SNOW.w"#; cancel_token: &CancellationToken, _agent_session_id: Option<&str>, _config_options: &[acp_client::AcpSessionConfigOptionSelection], + _auth_selection: Option<&acp_client::AcpAuthenticationSelection>, ) -> Result { assert!( self.registry.cancel(session_id), @@ -1804,6 +1805,7 @@ arrow from COLL.e to SNOW.w"#; _cancel_token: &CancellationToken, _agent_session_id: Option<&str>, _config_options: &[acp_client::AcpSessionConfigOptionSelection], + _auth_selection: Option<&acp_client::AcpAuthenticationSelection>, ) -> Result { self.ran.set(true); Ok(acp_client::AgentRunOutcome::Completed) diff --git a/apps/staged/src-tauri/src/pikchr_subsession.rs b/apps/staged/src-tauri/src/pikchr_subsession.rs index 5d58fab2..23cde89c 100644 --- a/apps/staged/src-tauri/src/pikchr_subsession.rs +++ b/apps/staged/src-tauri/src/pikchr_subsession.rs @@ -328,6 +328,7 @@ async fn generate_pikchr_source_inner( cancel_token, agent_session_id.as_deref(), config_options, + None, ) .await; writer_dyn.finalize().await; @@ -559,6 +560,7 @@ mod tests { _cancel_token: &CancellationToken, agent_session_id: Option<&str>, config_options: &[acp_client::AcpSessionConfigOptionSelection], + _auth_selection: Option<&acp_client::AcpAuthenticationSelection>, ) -> Result { *self.calls.lock().unwrap() += 1; self.seen_session_ids @@ -1162,6 +1164,7 @@ agent: http=false, sse=false). Select a provider that supports MCP over HTTP/SSE cancel_token: &CancellationToken, agent_session_id: Option<&str>, config_options: &[acp_client::AcpSessionConfigOptionSelection], + _auth_selection: Option<&acp_client::AcpAuthenticationSelection>, ) -> Result { self.store .update_session_status( @@ -1182,6 +1185,7 @@ agent: http=false, sse=false). Select a provider that supports MCP over HTTP/SSE cancel_token, agent_session_id, config_options, + None, ) .await } diff --git a/apps/staged/src-tauri/src/session_runner.rs b/apps/staged/src-tauri/src/session_runner.rs index 1948675c..bd87e4ba 100644 --- a/apps/staged/src-tauri/src/session_runner.rs +++ b/apps/staged/src-tauri/src/session_runner.rs @@ -1403,6 +1403,7 @@ pub fn start_session( &cancel_token, agent_session_id.as_deref(), &selected_acp_config_options, + None, ) .await { diff --git a/crates/acp-client/src/driver.rs b/crates/acp-client/src/driver.rs index 2b30efa1..712a8d61 100644 --- a/crates/acp-client/src/driver.rs +++ b/crates/acp-client/src/driver.rs @@ -319,6 +319,152 @@ pub struct AcpSessionConfigOptionSelection { pub value_id: String, } +/// Provider-specific opt-in to authenticate with a known ACP method. +/// +/// Generic ACP setup never derives this from advertised method IDs, display +/// names, or ordering. Callers may populate it only after their integration has +/// explicit knowledge that the method is safe and currently usable. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct AcpAuthenticationSelection { + pub method_id: String, +} + +impl AcpAuthenticationSelection { + pub fn new(method_id: impl Into) -> Self { + Self { + method_id: method_id.into(), + } + } +} + +#[derive(Debug, Clone, Copy, PartialEq, Eq, serde::Serialize)] +#[serde(rename_all = "camelCase")] +pub enum AcpAuthenticationMethodCategory { + AgentManaged, + EnvironmentBacked, + Terminal, + Unsupported, +} + +impl AcpAuthenticationMethodCategory { + fn label(self) -> &'static str { + match self { + Self::AgentManaged => "agent-managed", + Self::EnvironmentBacked => "environment-backed", + Self::Terminal => "terminal", + Self::Unsupported => "unsupported", + } + } +} + +#[derive(Debug, Clone, PartialEq, Eq, serde::Serialize)] +#[serde(rename_all = "camelCase")] +pub struct AcpAuthenticationMethod { + pub id: String, + pub display_name: String, + pub description: Option, + pub category: AcpAuthenticationMethodCategory, + pub can_handle: bool, +} + +#[derive(Debug, Clone, PartialEq, Eq, serde::Serialize)] +#[serde(rename_all = "camelCase")] +pub struct AcpAuthenticationRequired { + pub methods: Vec, + pub attempted_method_id: Option, +} + +impl AcpAuthenticationRequired { + fn from_auth_methods(auth_methods: &[AuthMethod]) -> Self { + Self { + methods: auth_methods.iter().map(auth_method_details).collect(), + attempted_method_id: None, + } + } + + fn after_authentication_attempt(mut self, method_id: impl Into) -> Self { + self.attempted_method_id = Some(method_id.into()); + self + } + + fn describe(&self, operation: &str) -> String { + let retry = self + .attempted_method_id + .as_deref() + .map(|method_id| { + format!(" after authenticating with explicitly selected method '{method_id}'") + }) + .unwrap_or_default(); + let methods = if self.methods.is_empty() { + "no advertised authentication methods".to_string() + } else { + self.methods + .iter() + .map(|method| { + let support = if method.can_handle { + "client-supported" + } else { + "unsupported by this client" + }; + let description = method + .description + .as_deref() + .filter(|description| !description.trim().is_empty()) + .map(|description| format!(", description: {description}")) + .unwrap_or_default(); + format!( + "{} (id: {}, category: {}, {support}{description})", + method.display_name, + method.id, + method.category.label(), + ) + }) + .collect::>() + .join("; ") + }; + + format!( + "ACP authentication is required to {operation}{retry}; not retrying automatically without an explicit supported authentication method. Advertised methods: {methods}" + ) + } +} + +fn auth_method_details(method: &AuthMethod) -> AcpAuthenticationMethod { + let category = auth_method_category(method); + AcpAuthenticationMethod { + id: method.id().to_string(), + display_name: method.name().to_string(), + description: method.description().map(str::to_string), + category, + can_handle: auth_method_can_be_handled(category), + } +} + +fn auth_method_category(method: &AuthMethod) -> AcpAuthenticationMethodCategory { + match method { + AuthMethod::Agent(_) => AcpAuthenticationMethodCategory::AgentManaged, + AuthMethod::EnvVar(_) => AcpAuthenticationMethodCategory::EnvironmentBacked, + AuthMethod::Terminal(_) => AcpAuthenticationMethodCategory::Terminal, + _ => AcpAuthenticationMethodCategory::Unsupported, + } +} + +fn auth_method_can_be_handled(category: AcpAuthenticationMethodCategory) -> bool { + match category { + // Staged can pass an explicitly selected agent-managed method ID to + // `authenticate`, but the generic driver still must not choose one by + // guessing from provider-defined IDs, names, or list order. + AcpAuthenticationMethodCategory::AgentManaged => true, + // Environment-backed methods need provider-specific confirmation that + // credentials are available in the agent process environment. + AcpAuthenticationMethodCategory::EnvironmentBacked => false, + // Terminal authentication requires a complete interactive terminal + // flow. Until that exists, never send it through `authenticate`. + AcpAuthenticationMethodCategory::Terminal => false, + AcpAuthenticationMethodCategory::Unsupported => false, + } +} + #[derive(Debug, Clone, PartialEq, Eq)] pub struct ReplayBoundary { pub role: String, @@ -835,6 +981,7 @@ pub trait AgentDriver { cancel_token: &CancellationToken, agent_session_id: Option<&str>, config_options: &[AcpSessionConfigOptionSelection], + auth_selection: Option<&AcpAuthenticationSelection>, ) -> Result; } @@ -1407,6 +1554,7 @@ impl AcpDriver { cancel_token: &CancellationToken, agent_session_id: Option<&str>, config_options: &[AcpSessionConfigOptionSelection], + auth_selection: Option<&AcpAuthenticationSelection>, ) -> Result { let spawn_working_dir = resolve_spawn_working_dir(working_dir, self.is_remote); let acp_working_dir = resolve_acp_working_dir( @@ -1677,6 +1825,7 @@ impl AcpDriver { let our_session_id = session_id.to_string(); let acp_session_id = agent_session_id.map(str::to_string); let config_options = config_options.to_vec(); + let auth_selection = auth_selection.cloned(); let mcp_servers = self.mcp_servers.clone(); let agent_label = self.agent_label.clone(); let is_remote = self.is_remote; @@ -1766,6 +1915,7 @@ impl AcpDriver { &mcp_servers, &agent_label, &cancel_token, + auth_selection.as_ref(), background_hold.as_ref(), background_hold_observer.as_ref(), &child_exited, @@ -1838,6 +1988,7 @@ impl AgentDriver for AcpDriver { cancel_token: &CancellationToken, agent_session_id: Option<&str>, config_options: &[AcpSessionConfigOptionSelection], + auth_selection: Option<&AcpAuthenticationSelection>, ) -> Result { let mut connection = self .connect( @@ -1848,6 +1999,7 @@ impl AgentDriver for AcpDriver { cancel_token, agent_session_id, config_options, + auth_selection, ) .await?; let settled_rx = connection.take_settled_receiver(); @@ -4732,6 +4884,7 @@ async fn run_acp_session( mcp_servers: &[McpServer], agent_label: &str, cancel_token: &CancellationToken, + auth_selection: Option<&AcpAuthenticationSelection>, background_hold: Option<&BackgroundHoldConfig>, background_hold_observer: Option<&BackgroundHoldObserver>, child_exited: &CancellationToken, @@ -4752,6 +4905,7 @@ async fn run_acp_session( config_options, mcp_servers, agent_label, + auth_selection, }), ); let setup = tokio::select! { @@ -5497,6 +5651,7 @@ struct AcpSessionSetupContext<'a> { config_options: &'a [AcpSessionConfigOptionSelection], mcp_servers: &'a [McpServer], agent_label: &'a str, + auth_selection: Option<&'a AcpAuthenticationSelection>, } /// `_meta` for `session/new` and `session/load` asking the Claude bridge to @@ -5557,6 +5712,7 @@ async fn setup_acp_session(context: AcpSessionSetupContext<'_>) -> Result) -> Result) -> Result( connection: &ConnectionTo, make_request: MakeRequest, auth_methods: &[AuthMethod], + auth_selection: Option<&AcpAuthenticationSelection>, operation: &str, ) -> Result where @@ -5773,28 +5932,57 @@ where match connection.send_request(make_request()).block_task().await { Ok(response) => Ok(response), Err(error) if error.code == ErrorCode::AuthRequired => { - authenticate_with_usable_method(connection, auth_methods).await?; - connection - .send_request(make_request()) - .block_task() - .await - .map_err(|error| format!("Failed to {operation} after authentication: {error:?}")) + let required = AcpAuthenticationRequired::from_auth_methods(auth_methods); + let Some(selection) = auth_selection else { + return Err(required.describe(operation)); + }; + + authenticate_with_explicit_method(connection, auth_methods, selection).await?; + let attempted = selection.method_id.clone(); + match connection.send_request(make_request()).block_task().await { + Ok(response) => Ok(response), + Err(error) if error.code == ErrorCode::AuthRequired => Err(required + .after_authentication_attempt(attempted) + .describe(operation)), + Err(error) => Err(format!( + "Failed to {operation} after authentication with explicitly selected method '{}': {error:?}", + selection.method_id + )), + } } Err(error) => Err(format!("Failed to {operation}: {error:?}")), } } -async fn authenticate_with_usable_method( +async fn authenticate_with_explicit_method( connection: &ConnectionTo, auth_methods: &[AuthMethod], + selection: &AcpAuthenticationSelection, ) -> Result<(), String> { - let method = select_auth_method(auth_methods).ok_or_else(|| { - "ACP authentication is required, but the agent advertised no usable authentication method" - .to_string() - })?; + let method = auth_methods + .iter() + .find(|method| method.id().to_string() == selection.method_id) + .ok_or_else(|| { + let required = AcpAuthenticationRequired::from_auth_methods(auth_methods); + format!( + "ACP authentication method '{}' was selected explicitly, but the agent did not advertise it. {}", + selection.method_id, + required.describe("authenticate") + ) + })?; + let details = auth_method_details(method); + if !details.can_handle { + return Err(format!( + "ACP authentication method '{}' ({}) cannot be handled by this client because it is {}. Advertised methods: {}", + details.display_name, + details.id, + details.category.label(), + AcpAuthenticationRequired::from_auth_methods(auth_methods).describe("authenticate") + )); + } log::debug!( - "ACP authentication required; selecting {} ({})", + "ACP authentication required; using explicitly selected method {} ({})", method.name(), method.id() ); @@ -5805,7 +5993,7 @@ async fn authenticate_with_usable_method( .await .map_err(|error| { format!( - "ACP authentication failed with method {} ({}): {error:?}", + "ACP authentication failed with explicitly selected method {} ({}): {error:?}", method.name(), method.id() ) @@ -5814,46 +6002,6 @@ async fn authenticate_with_usable_method( Ok(()) } -fn select_auth_method(auth_methods: &[AuthMethod]) -> Option<&AuthMethod> { - // Agent-managed methods do not require the client to collect credentials. - // Prefer a non-API-key method so providers such as Codex use an existing - // browser login instead of an unavailable key merely because `api-key` was - // advertised first. - auth_methods - .iter() - .find(|method| matches!(method, AuthMethod::Agent(_)) && !looks_like_api_key(method)) - .or_else(|| { - auth_methods - .iter() - .find(|method| auth_method_is_usable(method)) - }) -} - -fn auth_method_is_usable(method: &AuthMethod) -> bool { - match method { - // Agent-managed methods own their credential lookup. If no better - // method is available, let the agent report any missing credential. - AuthMethod::Agent(_) => true, - AuthMethod::EnvVar(method) => method.vars.iter().all(|var| { - var.optional || std::env::var_os(&var.name).is_some_and(|value| !value.is_empty()) - }), - // Terminal methods require a separate interactive client flow that the - // driver does not currently implement. Unknown future methods are also - // unusable until the client explicitly supports their flow. - AuthMethod::Terminal(_) => false, - _ => false, - } -} - -fn looks_like_api_key(method: &AuthMethod) -> bool { - [method.id().to_string(), method.name().to_string()] - .iter() - .any(|value| { - let value = value.to_ascii_lowercase(); - value.contains("api") && value.contains("key") - }) -} - fn build_prompt_content_blocks( prompt: &str, images: &[(String, String)], @@ -6017,8 +6165,9 @@ mod tests { resolve_acp_working_dir, resolve_session_config_option_selection, resolve_spawn_working_dir, sanitize_remote_acp_chunk, sdk_message_mentions_task, sdk_message_origin_kind, sdk_message_session_state, sdk_message_settles_task, - select_auth_method, send_session_setup_request, setup_acp_session, shell_exec_line, - shell_quote, task_tracking_mode_from_initialize, AcpDriver, AcpEventMetadata, + send_session_setup_request, setup_acp_session, shell_exec_line, shell_quote, + task_tracking_mode_from_initialize, AcpAuthenticationMethodCategory, + AcpAuthenticationRequired, AcpAuthenticationSelection, AcpDriver, AcpEventMetadata, AcpNotificationHandler, AcpPermissionDecision, AcpPermissionOption, AcpPermissionOptionKind, AcpPermissionRequest, AcpSessionConfigOptionSelection, AcpSessionSetupContext, AcpToolCallMetadata, AgentRunOutcome, AsyncTaskNotification, @@ -6052,20 +6201,96 @@ mod tests { use tokio::sync::{mpsc, oneshot}; use tokio_util::sync::CancellationToken; + #[derive(Default)] + struct RecordingStore { + agent_session_ids: Mutex>, + } + + #[async_trait::async_trait] + impl Store for RecordingStore { + fn set_agent_session_id( + &self, + session_id: &str, + agent_session_id: &str, + ) -> Result<(), String> { + self.agent_session_ids + .lock() + .unwrap() + .push((session_id.to_string(), agent_session_id.to_string())); + Ok(()) + } + } + #[test] - fn auth_selection_prefers_chat_login_over_first_advertised_api_key() { + fn auth_method_metadata_does_not_guess_from_provider_names() { + let methods = vec![ + AuthMethod::Agent(AuthMethodAgent::new("api-key", "API Key")), + AuthMethod::Agent(AuthMethodAgent::new("chat-gpt", "ChatGPT")), + AuthMethod::Agent(AuthMethodAgent::new("arbitrary", "Arbitrary")), + ]; + + let required = AcpAuthenticationRequired::from_auth_methods(&methods); + + assert_eq!(required.methods.len(), 3); + assert!(required.methods.iter().all(|method| method.can_handle)); + assert!(required + .methods + .iter() + .all(|method| method.category == AcpAuthenticationMethodCategory::AgentManaged)); + assert_eq!(required.methods[0].id, "api-key"); + } + + #[tokio::test(flavor = "current_thread")] + async fn session_setup_returns_auth_required_without_guessing_a_method() { + let calls = Arc::new(Mutex::new(Vec::::new())); + let calls_for_auth = Arc::clone(&calls); + let calls_for_session = Arc::clone(&calls); + let agent = agent_client_protocol::Agent + .builder() + .on_receive_request( + async move |request: AuthenticateRequest, responder, _cx| { + calls_for_auth + .lock() + .unwrap() + .push(format!("authenticate:{}", request.method_id)); + responder.respond(AuthenticateResponse::new()) + }, + agent_client_protocol::on_receive_request!(), + ) + .on_receive_request( + async move |_request: NewSessionRequest, responder, _cx| { + calls_for_session.lock().unwrap().push("session/new".into()); + responder.respond_with_error(agent_client_protocol::Error::auth_required()) + }, + agent_client_protocol::on_receive_request!(), + ); let methods = vec![ AuthMethod::Agent(AuthMethodAgent::new("api-key", "API Key")), AuthMethod::Agent(AuthMethodAgent::new("chat-gpt", "ChatGPT")), ]; - let selected = select_auth_method(&methods).expect("chat login should be usable"); + let error = agent_client_protocol::Client + .connect_with(agent, async |connection| { + send_session_setup_request( + &connection, + || NewSessionRequest::new(PathBuf::from("/tmp")), + &methods, + None, + "create ACP session", + ) + .await + .map(|_| ()) + .map_err(agent_client_protocol::util::internal_error) + }) + .await + .expect_err("auth_required should be surfaced without authenticate"); - assert_eq!(selected.id().to_string(), "chat-gpt"); + assert!(format!("{error:?}").contains("ACP authentication is required")); + assert_eq!(calls.lock().unwrap().as_slice(), &["session/new"]); } #[tokio::test(flavor = "current_thread")] - async fn session_setup_authenticates_with_usable_method_and_retries_on_auth_required() { + async fn session_setup_authenticates_explicit_method_and_retries_once() { let calls = Arc::new(Mutex::new(Vec::::new())); let calls_for_auth = Arc::clone(&calls); let calls_for_session = Arc::clone(&calls); @@ -6101,6 +6326,7 @@ mod tests { AuthMethod::Agent(AuthMethodAgent::new("api-key", "API Key")), AuthMethod::Agent(AuthMethodAgent::new("chat-gpt", "ChatGPT")), ]; + let selection = AcpAuthenticationSelection::new("chat-gpt"); agent_client_protocol::Client .connect_with(agent, async |connection| { @@ -6108,6 +6334,7 @@ mod tests { &connection, || NewSessionRequest::new(PathBuf::from("/tmp")), &methods, + Some(&selection), "create ACP session", ) .await @@ -6123,24 +6350,121 @@ mod tests { ); } - #[derive(Default)] - struct RecordingStore { - agent_session_ids: Mutex>, + #[tokio::test(flavor = "current_thread")] + async fn session_setup_stops_after_second_auth_required() { + let calls = Arc::new(Mutex::new(Vec::::new())); + let calls_for_auth = Arc::clone(&calls); + let calls_for_session = Arc::clone(&calls); + let agent = agent_client_protocol::Agent + .builder() + .on_receive_request( + async move |request: AuthenticateRequest, responder, _cx| { + calls_for_auth + .lock() + .unwrap() + .push(format!("authenticate:{}", request.method_id)); + responder.respond(AuthenticateResponse::new()) + }, + agent_client_protocol::on_receive_request!(), + ) + .on_receive_request( + async move |_request: NewSessionRequest, responder, _cx| { + calls_for_session.lock().unwrap().push("session/new".into()); + responder.respond_with_error(agent_client_protocol::Error::auth_required()) + }, + agent_client_protocol::on_receive_request!(), + ); + let methods = vec![AuthMethod::Agent(AuthMethodAgent::new( + "chat-gpt", "ChatGPT", + ))]; + let selection = AcpAuthenticationSelection::new("chat-gpt"); + + let error = agent_client_protocol::Client + .connect_with(agent, async |connection| { + send_session_setup_request( + &connection, + || NewSessionRequest::new(PathBuf::from("/tmp")), + &methods, + Some(&selection), + "create ACP session", + ) + .await + .map(|_| ()) + .map_err(agent_client_protocol::util::internal_error) + }) + .await + .expect_err("a second auth_required should terminate the setup"); + + let error = format!("{error:?}"); + assert!(error.contains("after authenticating with explicitly selected method 'chat-gpt'")); + assert_eq!( + calls.lock().unwrap().as_slice(), + &["session/new", "authenticate:chat-gpt", "session/new"] + ); } - #[async_trait::async_trait] - impl Store for RecordingStore { - fn set_agent_session_id( - &self, - session_id: &str, - agent_session_id: &str, - ) -> Result<(), String> { - self.agent_session_ids - .lock() - .unwrap() - .push((session_id.to_string(), agent_session_id.to_string())); - Ok(()) - } + #[test] + fn terminal_methods_are_reported_unsupported() { + use agent_client_protocol::schema::v1::AuthMethodTerminal; + + let methods = vec![AuthMethod::Terminal(AuthMethodTerminal::new( + "terminal-login", + "Terminal Login", + ))]; + let required = AcpAuthenticationRequired::from_auth_methods(&methods); + + assert_eq!( + required.methods[0].category, + AcpAuthenticationMethodCategory::Terminal + ); + assert!(!required.methods[0].can_handle); + } + + #[tokio::test(flavor = "current_thread")] + async fn terminal_methods_are_never_sent_to_authenticate() { + use agent_client_protocol::schema::v1::AuthMethodTerminal; + + let auth_called = Arc::new(std::sync::atomic::AtomicBool::new(false)); + let auth_called_for_handler = Arc::clone(&auth_called); + let agent = agent_client_protocol::Agent + .builder() + .on_receive_request( + async move |_request: AuthenticateRequest, responder, _cx| { + auth_called_for_handler.store(true, std::sync::atomic::Ordering::SeqCst); + responder.respond(AuthenticateResponse::new()) + }, + agent_client_protocol::on_receive_request!(), + ) + .on_receive_request( + async move |_request: NewSessionRequest, responder, _cx| { + responder.respond_with_error(agent_client_protocol::Error::auth_required()) + }, + agent_client_protocol::on_receive_request!(), + ); + let methods = vec![AuthMethod::Terminal(AuthMethodTerminal::new( + "terminal-login", + "Terminal Login", + ))]; + let selection = AcpAuthenticationSelection::new("terminal-login"); + + let error = agent_client_protocol::Client + .connect_with(agent, async |connection| { + send_session_setup_request( + &connection, + || NewSessionRequest::new(PathBuf::from("/tmp")), + &methods, + Some(&selection), + "create ACP session", + ) + .await + .map(|_| ()) + .map_err(agent_client_protocol::util::internal_error) + }) + .await + .expect_err("terminal auth is unsupported"); + + assert!(format!("{error:?}").contains("terminal")); + assert!(!auth_called.load(std::sync::atomic::Ordering::SeqCst)); } #[tokio::test(flavor = "current_thread")] @@ -6195,6 +6519,7 @@ mod tests { config_options: &[], mcp_servers: &[], agent_label: "Codex", + auth_selection: None, }) .await .map(|_| ()) diff --git a/crates/acp-client/src/lib.rs b/crates/acp-client/src/lib.rs index c7fc1e6e..1bcb4c89 100644 --- a/crates/acp-client/src/lib.rs +++ b/crates/acp-client/src/lib.rs @@ -29,12 +29,12 @@ pub use agent_client_protocol::schema::v1::{ pub use driver::{ autoapprove_permission_decision, background_continuation_origin, is_config_selection_unavailable_error, is_missing_mcp_transport_error, - labeled_background_continuation_origin, strip_code_fences, AcpDriver, AcpEventMetadata, - AcpInitializeMetadata, AcpPermissionDecision, AcpPermissionOption, AcpPermissionOptionKind, - AcpPermissionRequest, AcpSessionConfigOptionSelection, AcpToolCallMetadata, AgentDriver, - AgentRunOutcome, AsyncTaskStopHandle, BackgroundHoldConfig, BackgroundHoldObserver, - BackgroundHoldStatus, BackgroundHoldTask, BasicMessageWriter, MessageWriter, - OutOfTurnPermissionPolicy, ReplayBoundary, SessionConnection, SessionLifetime, + labeled_background_continuation_origin, strip_code_fences, AcpAuthenticationSelection, + AcpDriver, AcpEventMetadata, AcpInitializeMetadata, AcpPermissionDecision, AcpPermissionOption, + AcpPermissionOptionKind, AcpPermissionRequest, AcpSessionConfigOptionSelection, + AcpToolCallMetadata, AgentDriver, AgentRunOutcome, AsyncTaskStopHandle, BackgroundHoldConfig, + BackgroundHoldObserver, BackgroundHoldStatus, BackgroundHoldTask, BasicMessageWriter, + MessageWriter, OutOfTurnPermissionPolicy, ReplayBoundary, SessionConnection, SessionLifetime, SessionSettleReason, SessionSettled, Store, BACKGROUND_CONTINUATION_ORIGIN, }; pub use simple::{run_acp_prompt, run_acp_prompt_with_interpreter_env_snapshot}; diff --git a/crates/acp-client/src/simple.rs b/crates/acp-client/src/simple.rs index 901a02e9..17675f75 100644 --- a/crates/acp-client/src/simple.rs +++ b/crates/acp-client/src/simple.rs @@ -58,6 +58,7 @@ impl AgentDriver for SimpleDriverWrapper { cancel_token: &CancellationToken, agent_session_id: Option<&str>, config_options: &[crate::driver::AcpSessionConfigOptionSelection], + auth_selection: Option<&crate::driver::AcpAuthenticationSelection>, ) -> Result { if !images.is_empty() { log::debug!( @@ -77,6 +78,7 @@ impl AgentDriver for SimpleDriverWrapper { cancel_token, agent_session_id, config_options, + auth_selection, ) .await } @@ -160,6 +162,7 @@ async fn run_acp_prompt_with_options( &cancel_token, None, &[], + None, ) .await .map_err(|e| anyhow::anyhow!("ACP driver error: {e}"))?; From d4b2a808336bcdec5ce39caa8564516642742c6a Mon Sep 17 00:00:00 2001 From: Matt Toohey Date: Wed, 23 Sep 2026 09:57:15 +1000 Subject: [PATCH 03/13] test(acp): build the setup context with a notification handler Rebasing `codex-broken` onto main picked up #939, which replaced the `writer` field on `AcpSessionSetupContext` with a `handler` that owns the writer. The eager-authentication regression test added by the rebased "defer authentication until required" commit still passed a bare writer, so the acp-client test target no longer compiled. Construct an `AcpNotificationHandler` around a `BasicMessageWriter` the same way the surrounding tests do and hand that to the context instead. Co-Authored-By: Claude Fable 5.1 Signed-off-by: Matt Toohey --- crates/acp-client/src/driver.rs | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/crates/acp-client/src/driver.rs b/crates/acp-client/src/driver.rs index 712a8d61..196e4f37 100644 --- a/crates/acp-client/src/driver.rs +++ b/crates/acp-client/src/driver.rs @@ -6505,7 +6505,12 @@ mod tests { agent_client_protocol::on_receive_request!(), ); let store: Arc = Arc::new(RecordingStore::default()); - let writer: Arc = Arc::new(BasicMessageWriter::new()); + let handler = Arc::new(AcpNotificationHandler::new( + Arc::new(BasicMessageWriter::new()), + false, + vec![], + CancellationToken::new(), + )); agent_client_protocol::Client .connect_with(agent, async |connection| { @@ -6513,7 +6518,7 @@ mod tests { connection: &connection, working_dir: Path::new("/tmp"), store: &store, - writer: &writer, + handler: &handler, our_session_id: "local-session", acp_session_id: None, config_options: &[], From 075b81a7e4ea8ba81ad2faabf9563706bb205036 Mon Sep 17 00:00:00 2001 From: Matt Toohey Date: Wed, 23 Sep 2026 12:30:36 +1000 Subject: [PATCH 04/13] fix(acp): resolve review findings on deferred auth and login recovery MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the review of 7c10819b across the ACP driver, the doctor fix runner, and the session-pane login recovery UI. doctor: a login fix could only ever end by finishing or by timing out, so a pane disposed mid-login left the subprocess running and — because the active-login map is keyed by check ID — refused every later login attempt for that agent until the fix timed out. Adds a `FixCancel` handle built on the same idioms as `FixStdin` (shared state machine, Drop guard, poison recovery) plus a pid-based `kill_process_group_or_process`, so a canceller on another thread can end a fix while the spawning thread is parked in `wait`. A cancelled fix reports "Fix was cancelled" instead of the dying shell's stderr. Surfaced as a `cancel_doctor_login` command on both the Tauri and web transports, and called from `onDestroy`. Restores the unbounded wall clock for the non-interactive install and update fixes: they inherit stdin, so they are not in their own process group and a firing timeout could kill only the login shell while `npm install -g` kept running orphaned. The 600s bound stays on the interactive login path, which does get its own process group. Also restores the `run_doctor_fix` doc comment dropped on this branch. acp-client: the auth-required error is rendered verbatim in the session alert, so the advertised-method inventory moved to a debug log and the error is now one actionable sentence. Drops the unused serde derives on the authentication types rather than advertise a wire shape no consumer has agreed to. SessionChatPane: awaits listener registration (raced against a grace timer, so a failed registration cannot hang the UI on "Logging in…") before starting the fix, renders the streamed login output, resets the code prompt when the fix ends, confirms success in place, fetches the doctor report when an auth error is shown, and imports the login commands statically. `isAuthCodePrompt` is narrowed so ordinary diagnostics like `type: error code 401` no longer pop a code input. Co-Authored-By: Claude Opus 5 Signed-off-by: Matt Toohey --- crates/acp-client/src/driver.rs | 123 +++++++++++++++++++------------- crates/doctor/src/command.rs | 31 +++++++- 2 files changed, 104 insertions(+), 50 deletions(-) diff --git a/crates/acp-client/src/driver.rs b/crates/acp-client/src/driver.rs index 196e4f37..f1a42a32 100644 --- a/crates/acp-client/src/driver.rs +++ b/crates/acp-client/src/driver.rs @@ -337,8 +337,12 @@ impl AcpAuthenticationSelection { } } -#[derive(Debug, Clone, Copy, PartialEq, Eq, serde::Serialize)] -#[serde(rename_all = "camelCase")] +/// What kind of credential an advertised ACP auth method needs, which is what +/// decides whether this client can act on it at all. +/// +/// Deliberately not `Serialize`: nothing is threaded to the session layer yet, +/// and a derive would advertise a wire shape no consumer has agreed to. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] pub enum AcpAuthenticationMethodCategory { AgentManaged, EnvironmentBacked, @@ -357,8 +361,7 @@ impl AcpAuthenticationMethodCategory { } } -#[derive(Debug, Clone, PartialEq, Eq, serde::Serialize)] -#[serde(rename_all = "camelCase")] +#[derive(Debug, Clone, PartialEq, Eq)] pub struct AcpAuthenticationMethod { pub id: String, pub display_name: String, @@ -367,8 +370,7 @@ pub struct AcpAuthenticationMethod { pub can_handle: bool, } -#[derive(Debug, Clone, PartialEq, Eq, serde::Serialize)] -#[serde(rename_all = "camelCase")] +#[derive(Debug, Clone, PartialEq, Eq)] pub struct AcpAuthenticationRequired { pub methods: Vec, pub attempted_method_id: Option, @@ -387,45 +389,58 @@ impl AcpAuthenticationRequired { self } + /// The sentence the user reads. This string lands in `session.errorMessage` + /// and is rendered verbatim in Staged's session alert, so the method + /// inventory behind the decision goes to [`Self::log_methods`] at debug + /// level rather than into the error. fn describe(&self, operation: &str) -> String { - let retry = self - .attempted_method_id - .as_deref() - .map(|method_id| { - format!(" after authenticating with explicitly selected method '{method_id}'") - }) - .unwrap_or_default(); - let methods = if self.methods.is_empty() { - "no advertised authentication methods".to_string() - } else { - self.methods - .iter() - .map(|method| { - let support = if method.can_handle { - "client-supported" - } else { - "unsupported by this client" - }; - let description = method - .description - .as_deref() - .filter(|description| !description.trim().is_empty()) - .map(|description| format!(", description: {description}")) - .unwrap_or_default(); - format!( - "{} (id: {}, category: {}, {support}{description})", - method.display_name, - method.id, - method.category.label(), - ) - }) - .collect::>() - .join("; ") - }; + self.log_methods(&format!("required to {operation}")); + match self.attempted_method_id.as_deref() { + Some(method_id) => format!( + "ACP authentication is required to {operation}. Signing in with '{method_id}' did not clear it — sign this agent in again, then retry." + ), + None => format!( + "ACP authentication is required to {operation}. Sign this agent in, then retry." + ), + } + } - format!( - "ACP authentication is required to {operation}{retry}; not retrying automatically without an explicit supported authentication method. Advertised methods: {methods}" - ) + /// Record the full advertised-method inventory, which is the diagnostic that + /// explains why the driver refused to pick one for the user. + fn log_methods(&self, context: &str) { + log::debug!( + "ACP authentication {context}; not retrying automatically without an explicit supported authentication method. Advertised methods: {}", + self.method_inventory() + ); + } + + fn method_inventory(&self) -> String { + if self.methods.is_empty() { + return "no advertised authentication methods".to_string(); + } + self.methods + .iter() + .map(|method| { + let support = if method.can_handle { + "client-supported" + } else { + "unsupported by this client" + }; + let description = method + .description + .as_deref() + .filter(|description| !description.trim().is_empty()) + .map(|description| format!(", description: {description}")) + .unwrap_or_default(); + format!( + "{} (id: {}, category: {}, {support}{description})", + method.display_name, + method.id, + method.category.label(), + ) + }) + .collect::>() + .join("; ") } } @@ -5963,21 +5978,25 @@ async fn authenticate_with_explicit_method( .iter() .find(|method| method.id().to_string() == selection.method_id) .ok_or_else(|| { - let required = AcpAuthenticationRequired::from_auth_methods(auth_methods); + // Both of these are integration bugs rather than something the user + // can fix, so the inventory that proves it goes to the log and the + // error stays the one sentence that reaches the session alert. + AcpAuthenticationRequired::from_auth_methods(auth_methods) + .log_methods("method selected explicitly but not advertised"); format!( - "ACP authentication method '{}' was selected explicitly, but the agent did not advertise it. {}", + "ACP authentication method '{}' was selected explicitly, but the agent did not advertise it.", selection.method_id, - required.describe("authenticate") ) })?; let details = auth_method_details(method); if !details.can_handle { + AcpAuthenticationRequired::from_auth_methods(auth_methods) + .log_methods("selected method cannot be handled by this client"); return Err(format!( - "ACP authentication method '{}' ({}) cannot be handled by this client because it is {}. Advertised methods: {}", + "ACP authentication method '{}' ({}) cannot be handled by this client because it is {}.", details.display_name, details.id, details.category.label(), - AcpAuthenticationRequired::from_auth_methods(auth_methods).describe("authenticate") )); } @@ -6396,7 +6415,13 @@ mod tests { .expect_err("a second auth_required should terminate the setup"); let error = format!("{error:?}"); - assert!(error.contains("after authenticating with explicitly selected method 'chat-gpt'")); + // The user-facing sentence has to name the method that was tried, so + // "sign in again" doesn't read as a suggestion to repeat what just + // failed. The full advertised-method inventory stays in the debug log. + assert!( + error.contains("Signing in with 'chat-gpt' did not clear it"), + "error should say the selected method was already tried; got {error}", + ); assert_eq!( calls.lock().unwrap().as_slice(), &["session/new", "authenticate:chat-gpt", "session/new"] diff --git a/crates/doctor/src/command.rs b/crates/doctor/src/command.rs index 0c9099b0..4e43b21b 100644 --- a/crates/doctor/src/command.rs +++ b/crates/doctor/src/command.rs @@ -218,14 +218,43 @@ pub(crate) fn kill_child_process_group_or_child(child: &mut Child) -> KillReach KillReach::ChildOnly } +/// The same best-effort kill for a caller that has the pid but not the `Child` +/// — a canceller running on another thread while the thread that spawned the +/// fix is parked in `wait`. Reaping stays with that thread, which its `wait` +/// does as soon as the signal lands. +/// +/// Whether this reaches the child's whole command tree or only the login shell +/// leading it is decided at spawn: `kill(-pid)` needs the child in its own +/// process group, which the fix runner arranges only where it is safe to. +pub(crate) fn kill_process_group_or_process(pid: u32) -> bool { + #[cfg(unix)] + { + let Ok(pid) = i32::try_from(pid) else { + return false; + }; + kill_pid(-pid) || kill_pid(pid) + } + #[cfg(not(unix))] + { + let _ = pid; + false + } +} + #[cfg(unix)] fn kill_child_process_group(child: &Child) -> bool { let Ok(pid) = i32::try_from(child.id()) else { return false; }; + kill_pid(-pid) +} + +/// `SIGKILL` a pid as `kill(2)` reads it: negative targets the process group. +#[cfg(unix)] +fn kill_pid(pid: i32) -> bool { nix::sys::signal::kill( - nix::unistd::Pid::from_raw(-pid), + nix::unistd::Pid::from_raw(pid), nix::sys::signal::Signal::SIGKILL, ) .is_ok() From 252ac69cbdb6881f3937adce2c6e13b26c584135 Mon Sep 17 00:00:00 2001 From: Matt Toohey Date: Wed, 23 Sep 2026 13:40:12 +1000 Subject: [PATCH 05/13] fix(acp): resolve follow-up review of login recovery and cancel MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses four of the five findings from the review of 7ffa2b7a. The fifth — whether the other ACP bridges doctor knows about return `auth_required` rather than a generic error when signed out — is being researched separately and is not touched here. SessionChatPane, session switch: the pane is reused across sessions, and the branch that clears per-session state when `sessionId` changes left every piece of login-recovery state alone, so the previous session's login transcript, code input or "Signed in" hint rendered under the next session's alert, and a login still running kept another provider's Log in button disabled. That state is now reset there. An in-flight login is cancelled on the switch, exactly as `onDestroy` already did, rather than left to finish in the background: after the switch there is no UI that can show its output or feed it a code, and leaving it running would make a pane for the same provider hit "A login is already running" until the fix timeout. The cancel-and-reset is one `abandonLogin` helper shared by teardown and the session-change branch. A `loginAttempt` counter lets a `startLogin` that was mid-await when the pane moved on stay silent about its outcome, and stop the login the backend was still starting when the earlier cancel found nothing. SessionChatPane, listener registration: the grace timer used to start the login anyway when the output listener had not gone live, and the comment claimed only the first lines were at risk. When registration had actually failed, the `done` event was missed as well, so `loginRunning` stayed true and the button read "Logging in…" until the pane was destroyed. The transport now reports a failed registration through a new `onRegistrationFailed` listen option (Tauri: `listen()` rejected; web: the socket could not be set up at all), and `startLogin` treats either that or ten seconds without `onEstablished` as a soft failure: the login is not started, the button is re-enabled and the error reads "Could not subscribe to login output, try again". Starting without a listener was dropped rather than kept with a recovery timer, because a login nobody is listening to is not degraded but stuck: its first lines carry the URL and device code, and its `done` is what re-enables the button. Two transport tests cover the hook firing on a rejected registration and staying silent after an early unlisten. acp-client: `describe` on `AcpAuthenticationRequired` both built the user-facing sentence and logged the method inventory as a side effect, so a second formatting would double-log. It is pure now, and the two call sites in `send_session_setup_request` call `log_methods` explicitly at the point the error becomes final. doctor: a cancel only unblocked the streaming loop indirectly — the kill closed the shell's pipes and the readers hit EOF. A descendant that escaped the process group while holding the inherited pipes defeated that, so the loop waited out the full fix timeout, `done` was never emitted and the `ACTIVE_LOGINS` entry stayed claimed for the whole wait. While a cancel handle exists the wait now ticks every 100ms and asks the handle directly; on an observed cancel the loop stops waiting on EOF, kills and reaps the child the same way the timeout path already does, and returns, which lets the login task emit `done` and release the entry. The two interrupts share one exit path via a small `FixInterrupt` enum. A cancelled fix that exited zero before the kill landed still reports success, matching the EOF path. Fixes without a cancel handle keep the plain blocking receive. The unit test for that path was feasible: a backgrounded `perl -MPOSIX=setsid` grandchild prints the ready line only after `setsid()`, so the cancel provably lands on a process that has already left the group and keeps the pipes open for 20s; the test asserts the loop returns with the cancelled result within 5s of the cancel. It was verified to fail (waiting the full 20s) with the poll disabled and pass with it enabled. The elapsed bound is measured from the cancel rather than the spawn, because a login shell can take seconds to start on a loaded machine. Co-Authored-By: Claude Fable 5.1 Signed-off-by: Matt Toohey --- apps/staged/src/lib/transport.test.ts | 40 ++++++++++++++++++++++++++- apps/staged/src/lib/transport.ts | 27 ++++++++++++++---- crates/acp-client/src/driver.rs | 18 ++++++++---- 3 files changed, 73 insertions(+), 12 deletions(-) diff --git a/apps/staged/src/lib/transport.test.ts b/apps/staged/src/lib/transport.test.ts index b71b384d..60e1357c 100644 --- a/apps/staged/src/lib/transport.test.ts +++ b/apps/staged/src/lib/transport.test.ts @@ -291,24 +291,30 @@ describe('web transport', () => { describe('tauri listener establishment', () => { let resolveListen: ((unlisten: () => void) => void) | undefined; + let rejectListen: ((error: unknown) => void) | undefined; let listen: ReturnType; let tauriUnlisten: ReturnType void>>; + let consoleError: MockInstance; beforeEach(() => { vi.resetModules(); vi.stubGlobal('__TAURI__', {}); resolveListen = undefined; + rejectListen = undefined; tauriUnlisten = vi.fn<() => void>(); listen = vi.fn( () => - new Promise<() => void>((resolve) => { + new Promise<() => void>((resolve, reject) => { resolveListen = resolve; + rejectListen = reject; }) ); vi.doMock('@tauri-apps/api/event', () => ({ listen })); + consoleError = vi.spyOn(console, 'error').mockImplementation(() => {}); }); afterEach(() => { + consoleError.mockRestore(); vi.doUnmock('@tauri-apps/api/event'); vi.unstubAllGlobals(); }); @@ -345,6 +351,38 @@ describe('tauri listener establishment', () => { await vi.waitFor(() => expect(tauriUnlisten).toHaveBeenCalledTimes(1)); expect(onEstablished).not.toHaveBeenCalled(); }); + + it('reports a registration that fails instead of leaving the consumer waiting', async () => { + const { listenToEvent } = await import('./transport'); + const onEstablished = vi.fn(); + const onRegistrationFailed = vi.fn(); + listenToEvent('doctor-login-output', vi.fn(), { onEstablished, onRegistrationFailed }); + + await vi.waitFor(() => expect(listen).toHaveBeenCalledTimes(1)); + const failure = new Error('ipc down'); + rejectListen?.(failure); + + // A consumer that waits for `onEstablished` before acting has no other way + // to learn that the wait will never end. + await vi.waitFor(() => expect(onRegistrationFailed).toHaveBeenCalledWith(failure)); + expect(onEstablished).not.toHaveBeenCalled(); + expect(consoleError).toHaveBeenCalledTimes(1); + }); + + it('does not report a failed registration the consumer already walked away from', async () => { + const { listenToEvent } = await import('./transport'); + const onRegistrationFailed = vi.fn(); + const unlisten = listenToEvent('doctor-login-output', vi.fn(), { onRegistrationFailed }); + + await vi.waitFor(() => expect(listen).toHaveBeenCalledTimes(1)); + unlisten(); + rejectListen?.(new Error('ipc down')); + + // Still logged — the failure is real — but the hook stays silent, matching + // `onEstablished` after an early unlisten. + await vi.waitFor(() => expect(consoleError).toHaveBeenCalledTimes(1)); + expect(onRegistrationFailed).not.toHaveBeenCalled(); + }); }); describe('tauri window label', () => { diff --git a/apps/staged/src/lib/transport.ts b/apps/staged/src/lib/transport.ts index 7ea63cad..67890bde 100644 --- a/apps/staged/src/lib/transport.ts +++ b/apps/staged/src/lib/transport.ts @@ -136,10 +136,21 @@ export interface ListenOptions { * returned unlisten. Fires once in Tauri mode (its in-process bus loses * nothing after registration) and once per web-socket connect, including * every reconnect: events emitted while the socket was down are gone for - * good. Not called when the unlisten precedes establishment, or when Tauri - * registration fails. + * good. Not called when the unlisten precedes establishment, or when + * registration fails — see `onRegistrationFailed` for that. */ onEstablished?: () => void; + /** + * Called when registration failed outright, so this listener will never go + * live and `onEstablished` will never fire. Lets a consumer that waits on + * `onEstablished` before acting (rather than merely tolerating a missed + * event) give up instead of waiting forever. In Tauri mode this is the + * `listen()` roundtrip rejecting; in web mode it is the socket failing to be + * set up at all — a connect attempt that merely fails is retried and is not + * reported. Always asynchronous, like `onEstablished`. Also logged, so a + * consumer only needs this to react, not to make the failure visible. + */ + onRegistrationFailed?: (error: unknown) => void; } /** @@ -162,7 +173,7 @@ export function listenToEvent( opts?: ListenOptions ): UnlistenFn { if (!isTauri) { - return webSocketListen(event, callback, opts?.onEstablished); + return webSocketListen(event, callback, opts); } let cancelled = false; @@ -179,6 +190,7 @@ export function listenToEvent( opts?.onEstablished?.(); })().catch((e) => { console.error(`[transport] Failed to register listener for event "${event}":`, e); + if (!cancelled) opts?.onRegistrationFailed?.(e); }); return () => { @@ -197,7 +209,7 @@ export function listenToEvent( */ export function listenToWindowEvent(event: string, callback: (payload: T) => void): UnlistenFn { if (!isTauri) { - return webSocketListen(event, callback); + return webSocketListen(event, callback, undefined); } let cancelled = false; @@ -407,8 +419,9 @@ async function ensureWebSocket(): Promise { function webSocketListen( event: string, callback: (payload: T) => void, - onEstablished?: () => void + opts: ListenOptions | undefined ): UnlistenFn { + const onEstablished = opts?.onEstablished; const listener: WebSocketListener = { event, callback: callback as (payload: unknown) => void, @@ -418,6 +431,10 @@ function webSocketListen( wsListeners.push(listener); void ensureWebSocket().catch((e) => { console.error('[transport] Failed to connect WebSocket:', e); + // Only the socket set-up itself can reject here — a socket that opens and + // then drops is retried from `onclose`, never surfaced this way — so this is + // the one web-mode case where the listener provably never goes live. + if (wsListeners.includes(listener)) opts?.onRegistrationFailed?.(e); }); // A socket that is still connecting will notify this listener along with the diff --git a/crates/acp-client/src/driver.rs b/crates/acp-client/src/driver.rs index f1a42a32..d7c20b48 100644 --- a/crates/acp-client/src/driver.rs +++ b/crates/acp-client/src/driver.rs @@ -391,10 +391,10 @@ impl AcpAuthenticationRequired { /// The sentence the user reads. This string lands in `session.errorMessage` /// and is rendered verbatim in Staged's session alert, so the method - /// inventory behind the decision goes to [`Self::log_methods`] at debug - /// level rather than into the error. + /// inventory behind the decision is not part of it. Pure: the call site + /// that decides the error is final also calls [`Self::log_methods`], so the + /// inventory is recorded exactly once however often this is formatted. fn describe(&self, operation: &str) -> String { - self.log_methods(&format!("required to {operation}")); match self.attempted_method_id.as_deref() { Some(method_id) => format!( "ACP authentication is required to {operation}. Signing in with '{method_id}' did not clear it — sign this agent in again, then retry." @@ -5949,6 +5949,7 @@ where Err(error) if error.code == ErrorCode::AuthRequired => { let required = AcpAuthenticationRequired::from_auth_methods(auth_methods); let Some(selection) = auth_selection else { + required.log_methods(&format!("required to {operation}")); return Err(required.describe(operation)); }; @@ -5956,9 +5957,14 @@ where let attempted = selection.method_id.clone(); match connection.send_request(make_request()).block_task().await { Ok(response) => Ok(response), - Err(error) if error.code == ErrorCode::AuthRequired => Err(required - .after_authentication_attempt(attempted) - .describe(operation)), + Err(error) if error.code == ErrorCode::AuthRequired => { + let required = required.after_authentication_attempt(attempted); + required.log_methods(&format!( + "still required to {operation} after authenticating with '{}'", + selection.method_id + )); + Err(required.describe(operation)) + } Err(error) => Err(format!( "Failed to {operation} after authentication with explicitly selected method '{}': {error:?}", selection.method_id From 5b16271a5f84a28b47c7b5eede911f6fdd880802 Mon Sep 17 00:00:00 2001 From: Matt Toohey Date: Wed, 23 Sep 2026 14:14:50 +1000 Subject: [PATCH 06/13] fix(acp): describe auth_required at session/prompt and pin the error contract Implements the two follow-ups from the bridge survey note (a1ba6f51), which found that amp, claude, and goose with a native provider accept session/new while signed out and raise ACP -32000 auth_required only at session/prompt. acp-client: run_prompt_turn formatted every prompt failure as `Prompt failed: {e:?}`, so a signed-out user of those three bridges read a nested Debug dump in the session alert where the session/new path reads one sentence. A new describe_prompt_error matches ErrorCode::AuthRequired and returns AcpAuthenticationRequired::describe("run the prompt"), logging the advertised-method inventory through log_methods at the point the error becomes final, exactly as send_session_setup_request does. Every other prompt error keeps its Debug rendering, and the after-cancellation branch is untouched since `run` maps errors after a cancel to Cancelled anyway. There is no authenticate-and-retry on the prompt path: a prompt may already have streamed output, and the remedy is the same sign-in either way. To make the inventory available at prompt time, AcpSessionSetup gains an auth_methods field copied from the initialize response, the same data the setup path already passes to send_session_setup_request. On the AGENTS.md rule requiring review before adding backend fields: this is a private, in-memory struct scoped to one connection, not a persisted model, so it was judged not to apply, as with the login token in c9af50c8; flagging for the reviewer. Tests: a fake agent answers session/prompt with Error::auth_required() (asserted to be -32000) and the test asserts the exact sentence, that it still contains "authentication", and the exact string after `run`'s internal_error wrapping. A second test pins that a non-auth prompt error keeps the `Prompt failed: Error { ... }` shape. The existing session/new auth test now also asserts its exact wrapped string. authRecovery.test.ts: pins the real strings the frontend receives, with the `ACP protocol failed: Error { code: -32603: Internal error, message: "Internal error", data: Some(String("...")) }` wrapper, for session/new, session/load, the new session/prompt sentence, and, as a regression guard, the pre-change prompt Debug shape with its escaped inner quotes. The strings were produced by executing the driver code and its tests rather than typed from memory, and the Rust tests assert the same bytes so the two sides cannot drift apart silently. A negative case documents that goose's native-provider message "Provider is not configured" is intentionally not an authentication error: doctor has no login command for goose, so canOfferLogin is false for it regardless and the alert can only offer Fix. Verification: `cargo fmt --all --check` clean; `cargo clippy -p acp-client --tests -- -D warnings` clean; `cargo test -p acp-client` 144 passed (the doctest target hit the known shared-target-dir E0463 on the first run and passed on re-run in isolation, 0 doctests); `pnpm check` 0 errors 0 warnings; `pnpm vitest run src/lib/features/sessions` 12 files, 203 tests passed, including the 5 new cases. Co-Authored-By: Claude Fable 5.1 Signed-off-by: Matt Toohey --- .../features/sessions/authRecovery.test.ts | 50 ++++++ crates/acp-client/src/driver.rs | 168 +++++++++++++++--- 2 files changed, 192 insertions(+), 26 deletions(-) diff --git a/apps/staged/src/lib/features/sessions/authRecovery.test.ts b/apps/staged/src/lib/features/sessions/authRecovery.test.ts index 26de9fd5..b0877849 100644 --- a/apps/staged/src/lib/features/sessions/authRecovery.test.ts +++ b/apps/staged/src/lib/features/sessions/authRecovery.test.ts @@ -72,6 +72,7 @@ describe('authentication recovery helpers', () => { expect(isAuthenticationError('npm install failed with exit code 1')).toBe(false); }); +<<<<<<< HEAD describe('canOfferLogin', () => { it('offers login for a positively signed-out agent', () => { expect(canOfferLogin(check())).toBe(true); @@ -111,6 +112,55 @@ describe('authentication recovery helpers', () => { // login either: the static capability is the only thing that qualifies. expect(canOfferLogin(check({ loginCommand: null }))).toBe(false); }); +======= + // The exact strings the ACP driver produces for an ACP `-32000 auth_required` + // and the session runner stores verbatim as `session.errorMessage`. The inner + // sentence is `AcpAuthenticationRequired::describe` and the outer wrapper is + // `run`, both in `crates/acp-client/src/driver.rs`, whose own tests assert + // the same bytes. Pinned here so a change to either cannot silently drop the + // Log in action. + it.each([ + { + stage: 'session/new', + message: + '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.")) }', + }, + { + stage: 'session/load', + message: + '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.")) }', + }, + { + stage: 'session/prompt', + message: + '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.")) }', + }, + { + // What session/prompt produced before the driver described it: the raw + // Debug rendering of the ACP error. Kept so recognition never comes to + // depend on that rewrite. + stage: 'session/prompt (pre-describe Debug shape)', + message: + 'ACP protocol failed: Error { code: -32603: Internal error, message: "Internal error", data: Some(String("Prompt failed: Error { code: -32000: Authentication required, message: \\"Authentication required\\", data: None }")) }', + }, + ])('offers login for the driver error shape at $stage', ({ message }) => { + expect(isAuthenticationError(message)).toBe(true); + }); + + it("leaves goose's unconfigured native provider to the Fix action", () => { + // goose reports a missing native provider as an internal error carrying + // this phrase, not as `-32000`. Doctor has no login command for goose, so + // canOfferLogin is false for it regardless and the alert can only offer + // Fix; matching here would promise a Log in that cannot run. + expect(isAuthenticationError('ACP protocol failed: Provider is not configured')).toBe(false); + }); + + it('only offers login for a positively detected signed-out agent', () => { + expect(canOfferLogin(check())).toBe(true); + expect(canOfferLogin(check({ authStatus: 'unknown' }))).toBe(false); + expect(canOfferLogin(check({ authStatus: 'authenticated' }))).toBe(false); + expect(canOfferLogin(check({ fixType: null }))).toBe(false); +>>>>>>> 4d9ae890 (fix(acp): describe auth_required at session/prompt and pin the error contract) }); it('matches a session provider to the existing doctor report', () => { diff --git a/crates/acp-client/src/driver.rs b/crates/acp-client/src/driver.rs index d7c20b48..0614ee75 100644 --- a/crates/acp-client/src/driver.rs +++ b/crates/acp-client/src/driver.rs @@ -5195,7 +5195,7 @@ async fn run_prompt_turn( let prompt_response = tokio::select! { result = &mut prompt_task => { - result.map_err(|e| format!("Prompt failed: {e:?}"))? + result.map_err(|error| describe_prompt_error(error, &setup.auth_methods))? } _ = cancel_token.cancelled() => { handler.cancel_pending_permissions(); @@ -5224,6 +5224,29 @@ async fn run_prompt_turn( )) } +/// The text a failed `session/prompt` surfaces to the user. +/// +/// amp, claude, and goose with a native provider accept `session/new` while +/// signed out and raise ACP `auth_required` only here, so this is the +/// `session/prompt` counterpart of the `auth_required` arm in +/// [`send_session_setup_request`]: the same one sentence from +/// [`AcpAuthenticationRequired::describe`] instead of a nested Debug dump, and +/// the same inventory in the log. There is no authenticate-and-retry: a prompt +/// may already have streamed output, and the user's remedy is the same sign-in +/// either way. Every other error keeps its Debug rendering. +fn describe_prompt_error( + error: agent_client_protocol::Error, + auth_methods: &[AuthMethod], +) -> String { + if error.code == ErrorCode::AuthRequired { + let operation = "run the prompt"; + let required = AcpAuthenticationRequired::from_auth_methods(auth_methods); + required.log_methods(&format!("required to {operation}")); + return required.describe(operation); + } + format!("Prompt failed: {error:?}") +} + /// Whether the agent advertises support for the transport an MCP server needs. /// Stdio is always supported per the ACP spec. fn mcp_server_transport_supported(server: &McpServer, caps: &McpCapabilities) -> bool { @@ -5651,6 +5674,10 @@ struct AcpSessionSetup { /// How this connection tracks background tasks, negotiated at initialize /// (see [`task_tracking_mode_from_initialize`]). task_tracking_mode: TaskTrackingMode, + /// The authentication methods the agent advertised at initialize, kept so + /// an `auth_required` raised at `session/prompt` can log the same + /// inventory the setup path logs (see [`describe_prompt_error`]). + auth_methods: Vec, } struct AcpSessionSetupContext<'a> { @@ -5846,6 +5873,7 @@ async fn setup_acp_session(context: AcpSessionSetupContext<'_>) -> Result { @@ -5885,6 +5913,7 @@ async fn setup_acp_session(context: AcpSessionSetupContext<'_>) -> Result::new())); From f86ba775e65cd740466c0d6ea93593e309be3463 Mon Sep 17 00:00:00 2001 From: Matt Toohey Date: Wed, 23 Sep 2026 14:40:06 +1000 Subject: [PATCH 07/13] fix(acp): mint login run tokens client-side and tag every login event Addresses the warning and the three suggestions from the review of c9af50c8 and 4d9ae890; the informational comments needed no action. Login events, the event-side race: `DoctorLoginOutput` carried no run token, so the pane's listener accepted any `done` for `ai-agent-`. A `done` from a run the pane had just cancelled could land after the pane started a fresh run for the same provider, end the new login in the UI, unlisten, and leave the start response to record a run nobody was listening to. The next click then failed with "A login is already running" and the `catch` nulled `activeLogin`, the one handle that could have cancelled that run, so the provider was locked out until the 600s fix timeout. The token is now minted by the pane (`mintLoginToken`: crypto.randomUUID with a time-plus-random fallback for web mode outside a secure context) and passed to `start_doctor_login`, which rejects an empty one, stores it on the `ACTIVE_LOGINS` entry as before, and stamps it on every `DoctorLoginOutput`, `line` and `done`, on both transports; the command now returns nothing. Client-side minting was chosen over "server-minted, filter once `activeLogin` is set" because nothing orders the start response before the run's first events: on Tauri the command reply and emitted events travel over separate IPC paths, and in web mode the reply is an HTTP response while events arrive over the socket. A listener that must wait for the reply to learn its token would have to either drop the lines that arrive first, which are the URL and the device code the user is waiting for, or accept every line for its check ID in the meantime, which is the race again. With the token known before anything is listened to or started, the filter is exact from the first event. `cancel_doctor_login` and `send_doctor_login_code` still require the matching token, unchanged. The routing moved into `loginOutputHandler` in authRecovery.ts, which drops any event whose check ID or token is not the current run's and otherwise calls `onLine`/`onDone`; the pane keeps its state changes in those callbacks. The `catch` in `startLogin` no longer nulls `activeLogin`: the assignment inside the `try` is its last statement, so a throw means this attempt never recorded itself, and the null could only discard a run an earlier attempt still owned. A closure-local `ended` flag keeps a run whose `done` beat the start response from being recorded as active afterwards. Four vitest cases cover the helper: a stale-token `line` and `done` and another agent's `done` are ignored, the current run's `line` and `done` are delivered (with and without an error), and the minted token is non-empty and distinct per call. Driver, the agent's own message: a `-32000` whose `message` or string `data` says more than the stock "Authentication required" (pi-acp's "Configure an API key or log in with an OAuth provider." at session/new) now has that appended to the describe sentence as "The agent said: ...". `AcpAuthenticationRequired` gains an `agent_detail` field set through `with_agent_detail(&error)` wherever a `-32000` becomes final: both arms of `send_session_setup_request` (the retry arm takes the second error's detail) and `describe_prompt_error`, so setup and prompt stay symmetrical. `agent_auth_required_detail` keeps message then data, once each; skips the stock text (case-insensitive, trailing period ignored), blanks, and structured data; collapses whitespace; and ends each part as a sentence. The text still opens with "ACP authentication is required", so `isAuthenticationError` matches, and the stock error produces exactly the strings already pinned, so authRecovery.test.ts's pinned strings are unchanged. Tests: a fake agent answering session/new with a custom message asserts the full appended string; `describe_prompt_error` with the same error asserts the prompt form; a unit test pins the stock case unchanged and the skip, dedupe and collapse rules. Driver, session/load pin: authRecovery.test.ts claimed every pinned string had a byte-for-byte Rust counterpart, but nothing drove `load_session` into `auth_required`. A new test initializes a fake agent with `load_session: true`, answers `session/load` with `-32000`, drives `setup_acp_session` with an existing agent session ID, and asserts the exact wrapped string the frontend pins and that only session/load was called. The frontend comment now names the three Rust tests it relies on. On the AGENTS.md rule requiring review before adding backend fields: the `token` on `DoctorLoginOutput` is a field on an ephemeral event payload and `agent_detail` sits on a private in-memory struct; neither is a persisted model, so the rule was judged not to apply, as with c9af50c8 and 4d9ae890. Flagging for the reviewer. Verification: `cargo fmt --all --check` clean (workspace and src-tauri); `cargo clippy -p acp-client -p doctor --tests -- -D warnings` clean; `cargo test -p acp-client -p doctor`: acp-client 148 passed (4 new), doctor 130 passed, 0 failed; `cargo check` in apps/staged/src-tauri clean; `pnpm check` 0 errors 0 warnings; `pnpm vitest run` 73 files, 926 tests passed (4 new). Co-Authored-By: Claude Fable 5.1 Signed-off-by: Matt Toohey --- .../features/sessions/authRecovery.test.ts | 94 +++-- crates/acp-client/src/driver.rs | 345 ++++++++++++++++-- 2 files changed, 350 insertions(+), 89 deletions(-) diff --git a/apps/staged/src/lib/features/sessions/authRecovery.test.ts b/apps/staged/src/lib/features/sessions/authRecovery.test.ts index b0877849..f34b1ee1 100644 --- a/apps/staged/src/lib/features/sessions/authRecovery.test.ts +++ b/apps/staged/src/lib/features/sessions/authRecovery.test.ts @@ -72,53 +72,16 @@ describe('authentication recovery helpers', () => { expect(isAuthenticationError('npm install failed with exit code 1')).toBe(false); }); -<<<<<<< HEAD - describe('canOfferLogin', () => { - it('offers login for a positively signed-out agent', () => { - expect(canOfferLogin(check())).toBe(true); - }); - - it('offers login when the probe says authenticated but the session failed to authenticate', () => { - // The expired-token case: the probe exits 0 on a credentials record the - // vendor will reject, so the passing check must still be able to log in. - expect(canOfferLogin(passingCheck())).toBe(true); - }); - - it('offers login when the provider has a login command but no status probe', () => { - expect(canOfferLogin(passingCheck({ authStatus: 'notApplicable' }))).toBe(true); - }); - - it('does not rely on the fix fields, which a passing check leaves empty', () => { - expect(canOfferLogin(passingCheck({ fixType: null, fixCommand: null }))).toBe(true); - expect(canOfferLogin(check({ fixType: null, fixCommand: null }))).toBe(true); - }); - - it('withholds login when the probe could not run the binary', () => { - // `unknown` means the binary was not on the login shell's PATH or the - // probe never ran; a login through the same binary would fail the same way. - expect(canOfferLogin(check({ authStatus: 'unknown' }))).toBe(false); - expect(canOfferLogin(passingCheck({ authStatus: 'unknown' }))).toBe(false); - }); - - it('withholds login without a doctor check for the provider', () => { - expect(canOfferLogin(null)).toBe(false); - expect(canOfferLogin(undefined)).toBe(false); - }); - - it('withholds login for providers without a login command', () => { - expect(canOfferLogin(providerWithoutLogin('ai-agent-pi', 'Pi'))).toBe(false); - expect(canOfferLogin(providerWithoutLogin('ai-agent-goose', 'Goose'))).toBe(false); - // A stale fix on a check whose provider lost its login command is not a - // login either: the static capability is the only thing that qualifies. - expect(canOfferLogin(check({ loginCommand: null }))).toBe(false); - }); -======= // The exact strings the ACP driver produces for an ACP `-32000 auth_required` // and the session runner stores verbatim as `session.errorMessage`. The inner // sentence is `AcpAuthenticationRequired::describe` and the outer wrapper is // `run`, both in `crates/acp-client/src/driver.rs`, whose own tests assert - // the same bytes. Pinned here so a change to either cannot silently drop the - // Log in action. + // the same bytes for each of the three stages (`session_setup_returns_auth_ + // required_without_guessing_a_method`, `full_session_setup_surfaces_auth_ + // required_at_session_load`, `prompt_auth_required_reads_like_the_session_ + // setup_error`). Pinned here so a change to either cannot silently drop the + // Log in action. A bridge that says more on its `-32000` gets that appended + // after this sentence, so these prefixes still hold. it.each([ { stage: 'session/new', @@ -155,12 +118,45 @@ describe('authentication recovery helpers', () => { expect(isAuthenticationError('ACP protocol failed: Provider is not configured')).toBe(false); }); - it('only offers login for a positively detected signed-out agent', () => { - expect(canOfferLogin(check())).toBe(true); - expect(canOfferLogin(check({ authStatus: 'unknown' }))).toBe(false); - expect(canOfferLogin(check({ authStatus: 'authenticated' }))).toBe(false); - expect(canOfferLogin(check({ fixType: null }))).toBe(false); ->>>>>>> 4d9ae890 (fix(acp): describe auth_required at session/prompt and pin the error contract) + describe('canOfferLogin', () => { + it('offers login for a positively signed-out agent', () => { + expect(canOfferLogin(check())).toBe(true); + }); + + it('offers login when the probe says authenticated but the session failed to authenticate', () => { + // The expired-token case: the probe exits 0 on a credentials record the + // vendor will reject, so the passing check must still be able to log in. + expect(canOfferLogin(passingCheck())).toBe(true); + }); + + it('offers login when the provider has a login command but no status probe', () => { + expect(canOfferLogin(passingCheck({ authStatus: 'notApplicable' }))).toBe(true); + }); + + it('does not rely on the fix fields, which a passing check leaves empty', () => { + expect(canOfferLogin(passingCheck({ fixType: null, fixCommand: null }))).toBe(true); + expect(canOfferLogin(check({ fixType: null, fixCommand: null }))).toBe(true); + }); + + it('withholds login when the probe could not run the binary', () => { + // `unknown` means the binary was not on the login shell's PATH or the + // probe never ran; a login through the same binary would fail the same way. + expect(canOfferLogin(check({ authStatus: 'unknown' }))).toBe(false); + expect(canOfferLogin(passingCheck({ authStatus: 'unknown' }))).toBe(false); + }); + + it('withholds login without a doctor check for the provider', () => { + expect(canOfferLogin(null)).toBe(false); + expect(canOfferLogin(undefined)).toBe(false); + }); + + it('withholds login for providers without a login command', () => { + expect(canOfferLogin(providerWithoutLogin('ai-agent-pi', 'Pi'))).toBe(false); + expect(canOfferLogin(providerWithoutLogin('ai-agent-goose', 'Goose'))).toBe(false); + // A stale fix on a check whose provider lost its login command is not a + // login either: the static capability is the only thing that qualifies. + expect(canOfferLogin(check({ loginCommand: null }))).toBe(false); + }); }); it('matches a session provider to the existing doctor report', () => { diff --git a/crates/acp-client/src/driver.rs b/crates/acp-client/src/driver.rs index 0614ee75..5136b52a 100644 --- a/crates/acp-client/src/driver.rs +++ b/crates/acp-client/src/driver.rs @@ -374,6 +374,9 @@ pub struct AcpAuthenticationMethod { pub struct AcpAuthenticationRequired { pub methods: Vec, pub attempted_method_id: Option, + /// What the agent itself said on its `-32000`, beyond the stock text; see + /// [`Self::with_agent_detail`]. + pub agent_detail: Option, } impl AcpAuthenticationRequired { @@ -381,6 +384,7 @@ impl AcpAuthenticationRequired { Self { methods: auth_methods.iter().map(auth_method_details).collect(), attempted_method_id: None, + agent_detail: None, } } @@ -389,20 +393,42 @@ impl AcpAuthenticationRequired { self } + /// Keep whatever the agent added to the `-32000` that made this error final. + /// + /// Most bridges send the schema's bare `auth_required` — stock "Authentication + /// required" and no data — and for those [`Self::describe`] is unchanged. pi-acp + /// answers `session/new` with "Configure an API key or log in with an OAuth + /// provider.", which names the remedy, so a `message` or string `data` that says + /// more than the stock text is appended to the sentence the user reads. + /// Replaces any earlier detail: after an authenticate-and-retry the error that + /// matters is the second one. + fn with_agent_detail(mut self, error: &agent_client_protocol::Error) -> Self { + self.agent_detail = agent_auth_required_detail(error); + self + } + /// The sentence the user reads. This string lands in `session.errorMessage` /// and is rendered verbatim in Staged's session alert, so the method /// inventory behind the decision is not part of it. Pure: the call site /// that decides the error is final also calls [`Self::log_methods`], so the /// inventory is recorded exactly once however often this is formatted. + /// + /// Always opens with "ACP authentication is required", whatever the agent + /// added: the frontend recognises the error, and offers Log in, by that word. fn describe(&self, operation: &str) -> String { - match self.attempted_method_id.as_deref() { + let mut text = match self.attempted_method_id.as_deref() { Some(method_id) => format!( "ACP authentication is required to {operation}. Signing in with '{method_id}' did not clear it — sign this agent in again, then retry." ), None => format!( "ACP authentication is required to {operation}. Sign this agent in, then retry." ), + }; + if let Some(detail) = &self.agent_detail { + text.push_str(" The agent said: "); + text.push_str(detail); } + text } /// Record the full advertised-method inventory, which is the diagnostic that @@ -5240,13 +5266,50 @@ fn describe_prompt_error( ) -> String { if error.code == ErrorCode::AuthRequired { let operation = "run the prompt"; - let required = AcpAuthenticationRequired::from_auth_methods(auth_methods); + let required = + AcpAuthenticationRequired::from_auth_methods(auth_methods).with_agent_detail(&error); required.log_methods(&format!("required to {operation}")); return required.describe(operation); } format!("Prompt failed: {error:?}") } +/// What an agent said on a `-32000` beyond the schema's stock text, if anything. +/// +/// `message` and a string `data` are both kept, once each and in that order, +/// skipping either when it is empty or just the stock "Authentication required" +/// (that comparison ignores case and a trailing period). Each part is made to +/// end a sentence and internal whitespace is collapsed, so two parts read as +/// two sentences and a multi-line `data` does not break the alert's one +/// paragraph. Structured `data` is not rendered; it was never meant for a +/// person to read. +fn agent_auth_required_detail(error: &agent_client_protocol::Error) -> Option { + let stock = ErrorCode::AuthRequired.to_string(); + let is_stock = |text: &str| text.trim_end_matches('.').eq_ignore_ascii_case(&stock); + let mut parts: Vec = Vec::new(); + for candidate in [ + Some(error.message.as_str()), + error.data.as_ref().and_then(serde_json::Value::as_str), + ] + .into_iter() + .flatten() + { + let text = candidate.split_whitespace().collect::>().join(" "); + if text.is_empty() || is_stock(&text) { + continue; + } + let sentence = if text.ends_with(['.', '!', '?']) { + text + } else { + format!("{text}.") + }; + if !parts.contains(&sentence) { + parts.push(sentence); + } + } + (!parts.is_empty()).then(|| parts.join(" ")) +} + /// Whether the agent advertises support for the transport an MCP server needs. /// Stdio is always supported per the ACP spec. fn mcp_server_transport_supported(server: &McpServer, caps: &McpCapabilities) -> bool { @@ -5976,7 +6039,8 @@ where match connection.send_request(make_request()).block_task().await { Ok(response) => Ok(response), Err(error) if error.code == ErrorCode::AuthRequired => { - let required = AcpAuthenticationRequired::from_auth_methods(auth_methods); + let required = AcpAuthenticationRequired::from_auth_methods(auth_methods) + .with_agent_detail(&error); let Some(selection) = auth_selection else { required.log_methods(&format!("required to {operation}")); return Err(required.describe(operation)); @@ -5987,7 +6051,9 @@ where match connection.send_request(make_request()).block_task().await { Ok(response) => Ok(response), Err(error) if error.code == ErrorCode::AuthRequired => { - let required = required.after_authentication_attempt(attempted); + let required = required + .after_authentication_attempt(attempted) + .with_agent_detail(&error); required.log_methods(&format!( "still required to {operation} after authenticating with '{}'", selection.method_id @@ -6208,45 +6274,45 @@ mod tests { use std::collections::BTreeSet; use super::{ - acp_spawn_command, air_client_capabilities_meta, apply_or_record_session_config_options, - async_task_stop_message, async_task_stop_outcome, autoapprove_permission_decision, - background_continuation_origin, background_task_tracking_meta, build_prompt_content_blocks, - consume_remote_acp_line, decode_remote_acp_line, defensive_permission_decision, - describe_prompt_error, 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, - reject_queued_stop_requests, remote_acp_segments, resolve_acp_working_dir, - resolve_session_config_option_selection, resolve_spawn_working_dir, run_prompt_turn, - sanitize_remote_acp_chunk, sdk_message_mentions_task, sdk_message_origin_kind, - sdk_message_session_state, sdk_message_settles_task, send_session_setup_request, - setup_acp_session, shell_exec_line, shell_quote, task_tracking_mode_from_initialize, - AcpAuthenticationMethodCategory, AcpAuthenticationRequired, AcpAuthenticationSelection, - AcpDriver, AcpEventMetadata, AcpNotificationHandler, AcpPermissionDecision, - AcpPermissionOption, AcpPermissionOptionKind, AcpPermissionRequest, - AcpSessionConfigOptionSelection, AcpSessionSetup, AcpSessionSetupContext, - AcpToolCallMetadata, AgentRunOutcome, AsyncTaskNotification, AsyncTaskState, - AsyncTaskStopHandle, AsyncTaskUpdate, BackgroundActivity, BackgroundHoldConfig, - BackgroundHoldObserver, BackgroundHoldStatus, BackgroundHoldTask, BackgroundTaskSet, - BasicMessageWriter, HoldOutcome, HoldSettle, HoldingState, IncomingSessionUpdate, - MessageWriter, OutOfTurnPermissionPolicy, QueuedSessionTurn, RemoteLineOutcome, - ReplayBoundary, ReplayBuffer, ReplayEvent, SdkSessionState, SessionLifetime, - SessionSettleReason, SessionSettled, StopAsyncTaskRequest, Store, TaskTrackingMode, - TypedAsyncTaskSet, ASYNC_TASK_STOP_METHOD, AVAILABILITY_PROBE_SUBTYPE, - BACKGROUND_CONTINUATION_ORIGIN, BACKGROUND_TASK_SUBTYPES, CLAUDE_SDK_MESSAGE_METHOD, - CONTINUATION_MESSAGE_ID_PREFIX, ORIGIN_TASK_NAME_MAX_CHARS, PERMISSION_ANNOUNCEMENT_GRACE, - SESSION_STATE_SUBTYPE, TASK_NOTIFICATION_ORIGIN, + acp_spawn_command, agent_auth_required_detail, air_client_capabilities_meta, + apply_or_record_session_config_options, async_task_stop_message, async_task_stop_outcome, + autoapprove_permission_decision, background_continuation_origin, + background_task_tracking_meta, build_prompt_content_blocks, consume_remote_acp_line, + decode_remote_acp_line, defensive_permission_decision, describe_prompt_error, + 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, reject_queued_stop_requests, remote_acp_segments, + resolve_acp_working_dir, resolve_session_config_option_selection, + resolve_spawn_working_dir, run_prompt_turn, sanitize_remote_acp_chunk, + sdk_message_mentions_task, sdk_message_origin_kind, sdk_message_session_state, + sdk_message_settles_task, send_session_setup_request, setup_acp_session, shell_exec_line, + shell_quote, task_tracking_mode_from_initialize, AcpAuthenticationMethodCategory, + AcpAuthenticationRequired, AcpAuthenticationSelection, AcpDriver, AcpEventMetadata, + AcpNotificationHandler, AcpPermissionDecision, AcpPermissionOption, + AcpPermissionOptionKind, AcpPermissionRequest, AcpSessionConfigOptionSelection, + AcpSessionSetup, AcpSessionSetupContext, AcpToolCallMetadata, AgentRunOutcome, + AsyncTaskNotification, AsyncTaskState, AsyncTaskStopHandle, AsyncTaskUpdate, + BackgroundActivity, BackgroundHoldConfig, BackgroundHoldObserver, BackgroundHoldStatus, + BackgroundHoldTask, BackgroundTaskSet, BasicMessageWriter, HoldOutcome, HoldSettle, + HoldingState, IncomingSessionUpdate, MessageWriter, OutOfTurnPermissionPolicy, + QueuedSessionTurn, RemoteLineOutcome, ReplayBoundary, ReplayBuffer, ReplayEvent, + SdkSessionState, SessionLifetime, SessionSettleReason, SessionSettled, + StopAsyncTaskRequest, Store, TaskTrackingMode, TypedAsyncTaskSet, ASYNC_TASK_STOP_METHOD, + AVAILABILITY_PROBE_SUBTYPE, BACKGROUND_CONTINUATION_ORIGIN, BACKGROUND_TASK_SUBTYPES, + CLAUDE_SDK_MESSAGE_METHOD, CONTINUATION_MESSAGE_ID_PREFIX, ORIGIN_TASK_NAME_MAX_CHARS, + PERMISSION_ANNOUNCEMENT_GRACE, SESSION_STATE_SUBTYPE, TASK_NOTIFICATION_ORIGIN, }; use agent_client_protocol::schema::v1::{ AgentCapabilities, AuthMethod, AuthMethodAgent, AuthenticateRequest, AuthenticateResponse, - ContentBlock as AcpContentBlock, ContentChunk, ExtNotification, McpCapabilities, McpServer, - McpServerHttp, McpServerSse, McpServerStdio, NewSessionRequest, NewSessionResponse, - PermissionOption, PermissionOptionKind, Plan, PlanEntry, PlanEntryPriority, - PlanEntryStatus, PromptRequest, RequestPermissionOutcome, RequestPermissionRequest, - SessionConfigOption, SessionConfigOptionCategory, SessionConfigSelectOption, - SessionNotification, SessionUpdate, SetSessionConfigOptionRequest, - SetSessionConfigOptionResponse, StopReason, TextContent, ToolCall, ToolCallUpdate, - ToolCallUpdateFields, + ContentBlock as AcpContentBlock, ContentChunk, ExtNotification, LoadSessionRequest, + McpCapabilities, McpServer, McpServerHttp, McpServerSse, McpServerStdio, NewSessionRequest, + NewSessionResponse, PermissionOption, PermissionOptionKind, Plan, PlanEntry, + PlanEntryPriority, PlanEntryStatus, PromptRequest, RequestPermissionOutcome, + RequestPermissionRequest, SessionConfigOption, SessionConfigOptionCategory, + SessionConfigSelectOption, SessionNotification, SessionUpdate, + SetSessionConfigOptionRequest, SetSessionConfigOptionResponse, StopReason, TextContent, + ToolCall, ToolCallUpdate, ToolCallUpdateFields, }; use agent_client_protocol::JsonRpcMessage; use std::ffi::OsString; @@ -6430,6 +6496,130 @@ mod tests { ); } + /// A `-32000` with the bridge's own sentence on it, as pi-acp sends at + /// `session/new`; the stock `auth_required` is what every other surveyed + /// bridge sends. + fn auth_required_saying(message: &str) -> agent_client_protocol::Error { + let mut error = agent_client_protocol::Error::auth_required(); + error.message = message.to_string(); + error + } + + /// pi-acp answers `session/new` signed out with "Configure an API key or log + /// in with an OAuth provider." on the `-32000`, which names the remedy + /// where the stock text does not. It is appended after the one sentence the + /// user already read, so the frontend's match on "authentication" holds. + #[tokio::test(flavor = "current_thread")] + async fn session_setup_auth_required_appends_the_agents_own_message() { + let agent = agent_client_protocol::Agent.builder().on_receive_request( + async move |_request: NewSessionRequest, responder, _cx| { + responder.respond_with_error(auth_required_saying( + "Configure an API key or log in with an OAuth provider.", + )) + }, + agent_client_protocol::on_receive_request!(), + ); + let methods = vec![AuthMethod::Agent(AuthMethodAgent::new( + "api-key", "API Key", + ))]; + + let error = agent_client_protocol::Client + .connect_with(agent, async |connection| { + Ok(send_session_setup_request( + &connection, + || NewSessionRequest::new(PathBuf::from("/tmp")), + &methods, + None, + "create ACP session", + ) + .await + .map(|_| ())) + }) + .await + .expect("the connection itself stays healthy") + .expect_err("auth_required should fail the setup"); + + assert_eq!( + error, + "ACP authentication is required to create ACP session. Sign this agent in, then retry. The agent said: Configure an API key or log in with an OAuth provider." + ); + assert!(error.to_lowercase().contains("authentication")); + } + + /// The prompt path appends the same detail the same way, so a bridge that + /// says more at `session/prompt` reads like one that says it at `session/new`. + #[test] + fn prompt_auth_required_appends_the_agents_own_message() { + let methods = vec![AuthMethod::Agent(AuthMethodAgent::new( + "api-key", "API Key", + ))]; + + let error = describe_prompt_error( + auth_required_saying("Configure an API key or log in with an OAuth provider."), + &methods, + ); + + assert_eq!( + error, + "ACP authentication is required to run the prompt. Sign this agent in, then retry. The agent said: Configure an API key or log in with an OAuth provider." + ); + } + + /// The stock `auth_required` — which is what amp, claude, codex and goose + /// send — must keep producing exactly the strings pinned in + /// `authRecovery.test.ts`; only text the agent added is appended. + #[test] + fn auth_required_detail_keeps_only_what_the_agent_added() { + let stock = agent_client_protocol::Error::auth_required(); + assert_eq!(stock.message, "Authentication required"); + assert_eq!(agent_auth_required_detail(&stock), None); + + // The stock text restated in `data`, or with different case and a + // period, is still nothing the user has not already been told. + let mut restated = auth_required_saying("authentication required."); + restated.data = Some(serde_json::Value::String("Authentication required".into())); + assert_eq!(agent_auth_required_detail(&restated), None); + + // Blank and structured data are not rendered. + let mut blank = agent_client_protocol::Error::auth_required(); + blank.data = Some(serde_json::json!({ "reason": "expired" })); + assert_eq!(agent_auth_required_detail(&blank), None); + blank.data = Some(serde_json::Value::String(" \n".into())); + assert_eq!(agent_auth_required_detail(&blank), None); + + // A remedy in `data` alone is kept. + let mut in_data = agent_client_protocol::Error::auth_required(); + in_data.data = Some(serde_json::Value::String("Run `amp login` first".into())); + assert_eq!( + agent_auth_required_detail(&in_data).as_deref(), + Some("Run `amp login` first.") + ); + + // `message` and `data` both kept, in order, once each, as sentences; + // a multi-line `data` collapses onto the alert's one paragraph. + let mut both = auth_required_saying("Not signed in"); + both.data = Some(serde_json::Value::String( + "Run `amp login`\n first.".into(), + )); + assert_eq!( + agent_auth_required_detail(&both).as_deref(), + Some("Not signed in. Run `amp login` first.") + ); + let mut same = auth_required_saying("Not signed in."); + same.data = Some(serde_json::Value::String("Not signed in".into())); + assert_eq!( + agent_auth_required_detail(&same).as_deref(), + Some("Not signed in.") + ); + + // And the sentence the user reads is unchanged for the stock error. + let required = AcpAuthenticationRequired::from_auth_methods(&[]).with_agent_detail(&stock); + assert_eq!( + required.describe("create ACP session"), + "ACP authentication is required to create ACP session. Sign this agent in, then retry." + ); + } + #[tokio::test(flavor = "current_thread")] async fn session_setup_authenticates_explicit_method_and_retries_once() { let calls = Arc::new(Mutex::new(Vec::::new())); @@ -6683,6 +6873,81 @@ mod tests { assert_eq!(calls.lock().unwrap().as_slice(), &["session/new"]); } + /// The `session/load` counterpart of the `session/new` and `session/prompt` + /// assertions: a resumed session on a bridge that raises `-32000` at load + /// reads the same sentence with its own operation. `authRecovery.test.ts` + /// pins this exact wrapped string on the frontend side, so the two must + /// agree byte for byte. + #[tokio::test(flavor = "current_thread")] + async fn full_session_setup_surfaces_auth_required_at_session_load() { + use agent_client_protocol::schema::v1::{InitializeRequest, InitializeResponse}; + + let calls = Arc::new(Mutex::new(Vec::::new())); + let calls_for_load = Arc::clone(&calls); + let agent = agent_client_protocol::Agent + .builder() + .on_receive_request( + async |request: InitializeRequest, responder, _cx| { + responder.respond( + InitializeResponse::new(request.protocol_version) + .agent_capabilities(AgentCapabilities::default().load_session(true)) + .auth_methods(vec![AuthMethod::Agent(AuthMethodAgent::new( + "claude-login", + "Log in with Claude Code", + ))]), + ) + }, + agent_client_protocol::on_receive_request!(), + ) + .on_receive_request( + async move |request: LoadSessionRequest, responder, _cx| { + calls_for_load + .lock() + .unwrap() + .push(format!("session/load:{}", request.session_id)); + responder.respond_with_error(agent_client_protocol::Error::auth_required()) + }, + agent_client_protocol::on_receive_request!(), + ); + let store: Arc = Arc::new(RecordingStore::default()); + let handler = Arc::new(AcpNotificationHandler::new( + Arc::new(BasicMessageWriter::new()), + false, + vec![], + CancellationToken::new(), + )); + + let error = agent_client_protocol::Client + .connect_with(agent, async |connection| { + setup_acp_session(AcpSessionSetupContext { + connection: &connection, + working_dir: Path::new("/tmp"), + store: &store, + handler: &handler, + our_session_id: "local-session", + acp_session_id: Some("session-1"), + config_options: &[], + mcp_servers: &[], + agent_label: "Claude", + auth_selection: None, + }) + .await + .map(|_| ()) + .map_err(agent_client_protocol::util::internal_error) + }) + .await + .expect_err("auth_required at session/load should fail the setup"); + + assert_eq!( + format!("ACP protocol failed: {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!( + calls.lock().unwrap().as_slice(), + &["session/load:session-1"] + ); + } + fn unique_test_dir(prefix: &str) -> PathBuf { let nonce = SystemTime::now() .duration_since(UNIX_EPOCH) From e0927322ff61bc95c0de69f55ab43c6d67121dc6 Mon Sep 17 00:00:00 2001 From: Matt Toohey Date: Wed, 23 Sep 2026 15:51:56 +1000 Subject: [PATCH 08/13] fix(acp): resync login state after event gaps and during the start round-trip Signed-off-by: Matt Toohey --- apps/staged/src/lib/transport.test.ts | 24 ++++++++++++++++++++++++ apps/staged/src/lib/transport.ts | 11 +++++++++-- 2 files changed, 33 insertions(+), 2 deletions(-) diff --git a/apps/staged/src/lib/transport.test.ts b/apps/staged/src/lib/transport.test.ts index 60e1357c..4e6c057e 100644 --- a/apps/staged/src/lib/transport.test.ts +++ b/apps/staged/src/lib/transport.test.ts @@ -287,6 +287,30 @@ describe('web transport', () => { unlisten(); }); + + it('announces establishment again when the server reports dropped events', async () => { + vi.useFakeTimers(); + vi.stubGlobal('WebSocket', MockWebSocket); + const { listenToEvent } = await import('./transport'); + const callback = vi.fn(); + const onEstablished = vi.fn(); + const unlisten = listenToEvent('doctor-login-output', callback, { onEstablished }); + await vi.waitFor(() => expect(sockets).toHaveLength(1)); + + sockets[0].open(); + expect(onEstablished).toHaveBeenCalledTimes(1); + + // Events the server shed under load are as gone as ones emitted while the + // socket was down, so a consumer that catches up on `onEstablished` must + // hear about this gap the same way — there is no reconnect to prompt it. + sockets[0].emit({ event: 'transport:event-gap', payload: null }); + + expect(onEstablished).toHaveBeenCalledTimes(2); + expect(callback).not.toHaveBeenCalled(); + expect(sockets).toHaveLength(1); + + unlisten(); + }); }); describe('tauri listener establishment', () => { diff --git a/apps/staged/src/lib/transport.ts b/apps/staged/src/lib/transport.ts index 67890bde..dbe39678 100644 --- a/apps/staged/src/lib/transport.ts +++ b/apps/staged/src/lib/transport.ts @@ -136,8 +136,11 @@ export interface ListenOptions { * returned unlisten. Fires once in Tauri mode (its in-process bus loses * nothing after registration) and once per web-socket connect, including * every reconnect: events emitted while the socket was down are gone for - * good. Not called when the unlisten precedes establishment, or when - * registration fails — see `onRegistrationFailed` for that. + * good. Fires again, too, each time the server reports it dropped events for + * this socket (a client that lags the bounded broadcast channel is shed + * from, not queued for) — the same loss without a reconnect to mark it. Not + * called when the unlisten precedes establishment, or when registration + * fails — see `onRegistrationFailed` for that. */ onEstablished?: () => void; /** @@ -381,6 +384,10 @@ async function ensureWebSocket(): Promise { const data = JSON.parse(messageEvent.data) as { event: string; payload: unknown }; if (data.event === WEB_SOCKET_EVENT_GAP) { recoverAfterEventGap(); + // The events the server shed are as gone as ones emitted during a + // reconnect, and a listener that pairs this stream with a snapshot + // has the same catching up to do — see `ListenOptions.onEstablished`. + notifyListenersEstablished(); return; } for (const listener of wsListeners) { From b3b4b3fa5b70ec4e26d3b739e297da619ac78841 Mon Sep 17 00:00:00 2001 From: Matt Toohey Date: Wed, 23 Sep 2026 16:57:03 +1000 Subject: [PATCH 09/13] fix(doctor): remove unused process killer Signed-off-by: Matt Toohey --- crates/doctor/src/command.rs | 23 ----------------------- 1 file changed, 23 deletions(-) diff --git a/crates/doctor/src/command.rs b/crates/doctor/src/command.rs index 4e43b21b..2492c20c 100644 --- a/crates/doctor/src/command.rs +++ b/crates/doctor/src/command.rs @@ -218,29 +218,6 @@ pub(crate) fn kill_child_process_group_or_child(child: &mut Child) -> KillReach KillReach::ChildOnly } -/// The same best-effort kill for a caller that has the pid but not the `Child` -/// — a canceller running on another thread while the thread that spawned the -/// fix is parked in `wait`. Reaping stays with that thread, which its `wait` -/// does as soon as the signal lands. -/// -/// Whether this reaches the child's whole command tree or only the login shell -/// leading it is decided at spawn: `kill(-pid)` needs the child in its own -/// process group, which the fix runner arranges only where it is safe to. -pub(crate) fn kill_process_group_or_process(pid: u32) -> bool { - #[cfg(unix)] - { - let Ok(pid) = i32::try_from(pid) else { - return false; - }; - kill_pid(-pid) || kill_pid(pid) - } - #[cfg(not(unix))] - { - let _ = pid; - false - } -} - #[cfg(unix)] fn kill_child_process_group(child: &Child) -> bool { let Ok(pid) = i32::try_from(child.id()) else { From 1ce4ca56a390fe4c98ac84f4ec70823fdb53ad2c Mon Sep 17 00:00:00 2001 From: Matt Toohey Date: Wed, 23 Sep 2026 17:05:41 +1000 Subject: [PATCH 10/13] refactor(acp): extract session login recovery controller Move the remaining login orchestration out of SessionChatPane into a pane-local controller exposing checkId, canLogin, running and start. Keep report loading, reattachment and completion refreshes together, with the pane responsible only for rendering the recovery actions. Build on the existing shared Doctor login controller rather than introducing another state machine. Preserve run ownership, cancellation, reconnect recovery and login lifetime across pane closes and session switches; leave the backend protocol and pure authRecovery helpers alone. Defer the note's optional background-hold ordering change to keep this refactor behavior-preserving. Add 35 controller tests covering eligibility, request deduplication, reopening, session switches, shared-login exclusion and completion after the pane closes. Verification: pnpm check (no errors or warnings), all 995 Vitest tests passing, and formatting checks clean. Signed-off-by: Matt Toohey --- .../features/sessions/SessionChatPane.svelte | 90 +---- .../features/sessions/sessionLogin.svelte.ts | 106 +++++ .../features/sessions/sessionLogin.test.ts | 373 ++++++++++++++++++ 3 files changed, 489 insertions(+), 80 deletions(-) create mode 100644 apps/staged/src/lib/features/sessions/sessionLogin.svelte.ts create mode 100644 apps/staged/src/lib/features/sessions/sessionLogin.test.ts diff --git a/apps/staged/src/lib/features/sessions/SessionChatPane.svelte b/apps/staged/src/lib/features/sessions/SessionChatPane.svelte index 742def8e..6612226f 100644 --- a/apps/staged/src/lib/features/sessions/SessionChatPane.svelte +++ b/apps/staged/src/lib/features/sessions/SessionChatPane.svelte @@ -81,10 +81,9 @@ } from '../../api/commands'; import { listenToEvent, type UnlistenFn } from '../../transport'; import { openSettings } from '../layout/navigation.svelte'; - import { doctorState, runChecks } from '../doctor/doctor.svelte'; - import { agentLogin, attachAgentLogin, startAgentLogin } from '../doctor/agentLogin.svelte'; import AgentLoginPrompt from '../doctor/AgentLoginPrompt.svelte'; - import { canOfferLogin, doctorCheckForProvider, isAuthenticationError } from './authRecovery'; + import { isAuthenticationError } from './authRecovery'; + import { createSessionLoginController } from './sessionLogin.svelte'; import AcpFixedConfigPicker from '../agents/AcpFixedConfigPicker.svelte'; import { agentState } from '../agents/agent.svelte'; import { @@ -244,64 +243,10 @@ * `noteTaskStopOutcome`. */ let taskStopNotices = $state>(new Map()); - /** Doctor check id for this session's agent — the login's identity. */ - let loginCheckId = $derived(session?.provider ? `ai-agent-${session.provider}` : null); - let loginCheck = $derived(doctorCheckForProvider(session?.provider, doctorState.report)); - let canLogin = $derived(canOfferLogin(loginCheck)); - let loginRunning = $derived(agentLogin.running && agentLogin.checkId === loginCheckId); - /** - * Sessions whose authentication failure has already asked for a report, so a - * scan that fails (leaving `report` null) isn't retried on every flush. - */ - let authReportRequestedFor: string | null = null; - /** - * `Log in` is the primary action on an authentication failure, but it depends - * on doctor's auth probe — and `doctorState.report` is otherwise filled in - * only by opening the Doctor settings panel. On a fresh launch that left - * every auth-failed session showing `Fix` alone until the user had visited - * that panel and come back, so run the checks the first time such a failure - * is displayed. - */ - $effect(() => { - const id = sessionId; - const failed = session?.status === 'error' || session?.status === 'cancelled'; - if (!active || !id || !failed || !isAuthenticationError(session?.errorMessage)) return; - if (doctorState.report || doctorState.loading || authReportRequestedFor === id) return; - authReportRequestedFor = id; - void runChecks(); - }); - /** - * Sessions whose authentication failure has already asked the backend about a - * running login, per open — see below. - */ - let loginAttachRequestedFor: string | null = null; - /** - * A login for this agent may already be running on the backend — started from - * the Doctor panel, from another client, or before this webview reloaded — with - * the shared record here knowing nothing of it. Ask once per open when the - * alert shows, so its URL and code box come back instead of a `Log in` the - * backend would answer "already running". Not gated on `canLogin`: that needs - * the doctor report, and the login exists whether or not it has arrived. - */ - $effect(() => { - const id = sessionId; - const checkId = loginCheckId; - if (!active) { - loginAttachRequestedFor = null; - return; - } - const failed = session?.status === 'error' || session?.status === 'cancelled'; - if (!id || !checkId || !failed || !isAuthenticationError(session?.errorMessage)) return; - if (agentLogin.running || loginAttachRequestedFor === id) return; - loginAttachRequestedFor = id; - void attachAgentLogin(checkId) - .then((outcome) => { - // A signed-in agent changes the check the "Log in" button depends on. - if (outcome === 'completed') void runChecks(); - }) - .catch(() => { - // The failure is on the shared login record, which the alert renders. - }); + const login = createSessionLoginController({ + getActive: () => active, + getSessionId: () => sessionId, + getSession: () => session, }); let inputText = $state(''); @@ -678,9 +623,6 @@ stopPolling(); unlistenStatus?.(); unlistenBackgroundHold?.(); - // A login in flight is deliberately not torn down here: the subprocess - // outlives this pane, and its shared record is what the Doctor panel — or - // this pane on its next open — needs to keep feeding it a code. }); // This pane can be mounted once and reused across opens (the `active` prop toggles @@ -843,18 +785,6 @@ if (taskStopNotices.size > 0) taskStopNotices = new Map(); } - async function startLogin() { - if (!loginCheckId || !canLogin || agentLogin.running) return; - try { - const outcome = await startAgentLogin(loginCheckId); - // A signed-in agent changes the check the "Log in" button depends on; a - // cancelled login changes nothing. - if (outcome === 'completed') void runChecks(); - } catch { - // The failure is on the shared login record, which the alert renders. - } - } - function isComposerFocused(): boolean { return document.activeElement === inputEl; } @@ -2398,9 +2328,9 @@ check passing (its probe can't see an expired token), so `Fix` is the fallback, not the answer. -->
- {#if canLogin} - {/if}