diff --git a/Cargo.lock b/Cargo.lock index e80d469a6330531d098b96d01fdba26873368c95..f8724ee07361e095605297ebcac7bc4133102b8c 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -271,6 +271,7 @@ dependencies = [ "collections", "credentials_provider", "env_logger 0.11.8", + "feature_flags", "fs", "futures 0.3.31", "google_ai", diff --git a/crates/acp_thread/src/connection.rs b/crates/acp_thread/src/connection.rs index 33692c90d7915b52d33764ce99f949ffab84e04e..5e8aa6ec5ce1f6a4b05981a8d9dc34246f7685c1 100644 --- a/crates/acp_thread/src/connection.rs +++ b/crates/acp_thread/src/connection.rs @@ -2,12 +2,13 @@ use crate::AcpThread; use agent_client_protocol::{self as acp}; use anyhow::Result; use chrono::{DateTime, Utc}; -use collections::IndexMap; +use collections::{HashMap, IndexMap}; use gpui::{Entity, SharedString, Task}; use language_model::LanguageModelProviderId; use project::{AgentId, Project}; use serde::{Deserialize, Serialize}; use std::{any::Any, error::Error, fmt, path::PathBuf, rc::Rc, sync::Arc}; +use task::{HideStrategy, SpawnInTerminal, TaskId}; use ui::{App, IconName}; use util::path_list::PathList; use uuid::Uuid; @@ -21,6 +22,28 @@ impl UserMessageId { } } +pub fn build_terminal_auth_task( + id: String, + label: String, + command: String, + args: Vec, + env: HashMap, +) -> SpawnInTerminal { + SpawnInTerminal { + id: TaskId(id), + full_label: label.clone(), + label: label.clone(), + command: Some(command), + args, + command_label: label, + env, + use_new_terminal: true, + allow_concurrent_runs: true, + hide: HideStrategy::Always, + ..Default::default() + } +} + pub trait AgentConnection { fn agent_id(&self) -> AgentId; @@ -90,6 +113,14 @@ pub trait AgentConnection { fn auth_methods(&self) -> &[acp::AuthMethod]; + fn terminal_auth_task( + &self, + _method: &acp::AuthMethodId, + _cx: &App, + ) -> Option { + None + } + fn authenticate(&self, method: acp::AuthMethodId, cx: &mut App) -> Task>; fn prompt( diff --git a/crates/agent_servers/Cargo.toml b/crates/agent_servers/Cargo.toml index 4fb4109129ee5b8896f7a62afe49e0bcaef701ed..1542466be35bbce80983a73a3fc2e0998799160c 100644 --- a/crates/agent_servers/Cargo.toml +++ b/crates/agent_servers/Cargo.toml @@ -30,6 +30,7 @@ env_logger = { workspace = true, optional = true } fs.workspace = true futures.workspace = true gpui.workspace = true +feature_flags.workspace = true gpui_tokio = { workspace = true, optional = true } credentials_provider.workspace = true google_ai.workspace = true diff --git a/crates/agent_servers/src/acp.rs b/crates/agent_servers/src/acp.rs index 43f2ea2f7923aedf6541bfa9c21c566595fe300b..f9be24f21f7beebffaf8bcfcbcd3392445784012 100644 --- a/crates/agent_servers/src/acp.rs +++ b/crates/agent_servers/src/acp.rs @@ -7,13 +7,14 @@ use action_log::ActionLog; use agent_client_protocol::{self as acp, Agent as _, ErrorCode}; use anyhow::anyhow; use collections::HashMap; +use feature_flags::{AcpBetaFeatureFlag, FeatureFlagAppExt as _}; use futures::AsyncBufReadExt as _; use futures::io::BufReader; use project::agent_server_store::AgentServerCommand; use project::{AgentId, Project}; use serde::Deserialize; use settings::Settings as _; -use task::ShellBuilder; +use task::{ShellBuilder, SpawnInTerminal}; use util::ResultExt as _; use util::path_list::PathList; use util::process::Child; @@ -33,6 +34,8 @@ use terminal::terminal_settings::{AlternateScroll, CursorShape, TerminalSettings use crate::GEMINI_ID; +pub const GEMINI_TERMINAL_AUTH_METHOD_ID: &str = "spawn-gemini-cli"; + #[derive(Debug, Error)] #[error("Unsupported version")] pub struct UnsupportedVersion; @@ -44,6 +47,7 @@ pub struct AcpConnection { connection: Rc, sessions: Rc>>, auth_methods: Vec, + command: AgentServerCommand, agent_capabilities: acp::AgentCapabilities, default_mode: Option, default_model: Option, @@ -286,6 +290,7 @@ impl AcpConnection { .read_text_file(true) .write_text_file(true)) .terminal(true) + .auth(acp::AuthCapabilities::new().terminal(true)) // Experimental: Allow for rendering terminal output from the agents .meta(acp::Meta::from_iter([ ("terminal_output".into(), true.into()), @@ -335,7 +340,7 @@ impl AcpConnection { }); let meta = acp::Meta::from_iter([("terminal-auth".to_string(), value)]); vec![acp::AuthMethod::Agent( - acp::AuthMethodAgent::new("spawn-gemini-cli", "Login") + acp::AuthMethodAgent::new(GEMINI_TERMINAL_AUTH_METHOD_ID, "Login") .description("Login with your Google or Vertex AI account") .meta(meta), )] @@ -345,6 +350,7 @@ impl AcpConnection { Ok(Self { id: agent_id, auth_methods, + command, connection, display_name, telemetry_id, @@ -468,6 +474,64 @@ impl Drop for AcpConnection { } } +fn terminal_auth_task_id(agent_id: &AgentId, method_id: &acp::AuthMethodId) -> String { + format!("external-agent-{}-{}-login", agent_id.0, method_id.0) +} + +fn terminal_auth_task( + command: &AgentServerCommand, + agent_id: &AgentId, + method: &acp::AuthMethodTerminal, +) -> SpawnInTerminal { + let mut args = command.args.clone(); + args.extend(method.args.clone()); + + let mut env = command.env.clone().unwrap_or_default(); + env.extend(method.env.clone()); + + acp_thread::build_terminal_auth_task( + terminal_auth_task_id(agent_id, &method.id), + method.name.clone(), + command.path.to_string_lossy().into_owned(), + args, + env, + ) +} + +/// Used to support the _meta method prior to stabilization +fn meta_terminal_auth_task( + agent_id: &AgentId, + method_id: &acp::AuthMethodId, + method: &acp::AuthMethod, +) -> Option { + #[derive(Deserialize)] + struct MetaTerminalAuth { + label: String, + command: String, + #[serde(default)] + args: Vec, + #[serde(default)] + env: HashMap, + } + + let meta = match method { + acp::AuthMethod::EnvVar(env_var) => env_var.meta.as_ref(), + acp::AuthMethod::Terminal(terminal) => terminal.meta.as_ref(), + acp::AuthMethod::Agent(agent) => agent.meta.as_ref(), + _ => None, + }?; + let terminal_auth = + serde_json::from_value::(meta.get("terminal-auth")?.clone()).ok()?; + + Some(acp_thread::build_terminal_auth_task( + terminal_auth_task_id(agent_id, method_id), + terminal_auth.label.clone(), + terminal_auth.command, + terminal_auth.args, + terminal_auth.env, + )) +} + impl AgentConnection for AcpConnection { fn agent_id(&self) -> AgentId { self.id.clone() @@ -813,6 +877,24 @@ impl AgentConnection for AcpConnection { &self.auth_methods } + fn terminal_auth_task( + &self, + method_id: &acp::AuthMethodId, + cx: &App, + ) -> Option { + let method = self + .auth_methods + .iter() + .find(|method| method.id() == method_id)?; + + match method { + acp::AuthMethod::Terminal(terminal) if cx.has_flag::() => { + Some(terminal_auth_task(&self.command, &self.id, terminal)) + } + _ => meta_terminal_auth_task(&self.id, method_id, method), + } + } + fn authenticate(&self, method_id: acp::AuthMethodId, cx: &mut App) -> Task> { let conn = self.connection.clone(); cx.foreground_executor().spawn(async move { @@ -979,6 +1061,149 @@ fn map_acp_error(err: acp::Error) -> anyhow::Error { } } +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn terminal_auth_task_reuses_command_and_merges_args_and_env() { + let command = AgentServerCommand { + path: "/path/to/agent".into(), + args: vec!["--acp".into(), "--verbose".into()], + env: Some(HashMap::from_iter([ + ("BASE".into(), "1".into()), + ("SHARED".into(), "base".into()), + ])), + }; + let method = acp::AuthMethodTerminal::new("login", "Login") + .args(vec!["/auth".into()]) + .env(std::collections::HashMap::from_iter([ + ("EXTRA".into(), "2".into()), + ("SHARED".into(), "override".into()), + ])); + + let terminal_auth_task = terminal_auth_task(&command, &AgentId::new("test-agent"), &method); + + assert_eq!( + terminal_auth_task.command.as_deref(), + Some("/path/to/agent") + ); + assert_eq!(terminal_auth_task.args, vec!["--acp", "--verbose", "/auth"]); + assert_eq!( + terminal_auth_task.env, + HashMap::from_iter([ + ("BASE".into(), "1".into()), + ("SHARED".into(), "override".into()), + ("EXTRA".into(), "2".into()), + ]) + ); + assert_eq!(terminal_auth_task.label, "Login"); + assert_eq!(terminal_auth_task.command_label, "Login"); + } + + #[test] + fn legacy_terminal_auth_task_parses_meta_and_retries_session() { + let method_id = acp::AuthMethodId::new("legacy-login"); + let method = acp::AuthMethod::Agent( + acp::AuthMethodAgent::new(method_id.clone(), "Login").meta(acp::Meta::from_iter([( + "terminal-auth".to_string(), + serde_json::json!({ + "label": "legacy /auth", + "command": "legacy-agent", + "args": ["auth", "--interactive"], + "env": { + "AUTH_MODE": "interactive", + }, + }), + )])), + ); + + let terminal_auth_task = + meta_terminal_auth_task(&AgentId::new("test-agent"), &method_id, &method) + .expect("expected legacy terminal auth task"); + + assert_eq!( + terminal_auth_task.id.0, + "external-agent-test-agent-legacy-login-login" + ); + assert_eq!(terminal_auth_task.command.as_deref(), Some("legacy-agent")); + assert_eq!(terminal_auth_task.args, vec!["auth", "--interactive"]); + assert_eq!( + terminal_auth_task.env, + HashMap::from_iter([("AUTH_MODE".into(), "interactive".into())]) + ); + assert_eq!(terminal_auth_task.label, "legacy /auth"); + } + + #[test] + fn legacy_terminal_auth_task_returns_none_for_invalid_meta() { + let method_id = acp::AuthMethodId::new("legacy-login"); + let method = acp::AuthMethod::Agent( + acp::AuthMethodAgent::new(method_id.clone(), "Login").meta(acp::Meta::from_iter([( + "terminal-auth".to_string(), + serde_json::json!({ + "label": "legacy /auth", + }), + )])), + ); + + assert!( + meta_terminal_auth_task(&AgentId::new("test-agent"), &method_id, &method).is_none() + ); + } + + #[test] + fn first_class_terminal_auth_takes_precedence_over_legacy_meta() { + let method_id = acp::AuthMethodId::new("login"); + let method = acp::AuthMethod::Terminal( + acp::AuthMethodTerminal::new(method_id, "Login") + .args(vec!["/auth".into()]) + .env(std::collections::HashMap::from_iter([( + "AUTH_MODE".into(), + "first-class".into(), + )])) + .meta(acp::Meta::from_iter([( + "terminal-auth".to_string(), + serde_json::json!({ + "label": "legacy /auth", + "command": "legacy-agent", + "args": ["legacy-auth"], + "env": { + "AUTH_MODE": "legacy", + }, + }), + )])), + ); + + let command = AgentServerCommand { + path: "/path/to/agent".into(), + args: vec!["--acp".into()], + env: Some(HashMap::from_iter([("BASE".into(), "1".into())])), + }; + + let terminal_auth_task = match &method { + acp::AuthMethod::Terminal(terminal) => { + terminal_auth_task(&command, &AgentId::new("test-agent"), terminal) + } + _ => unreachable!(), + }; + + assert_eq!( + terminal_auth_task.command.as_deref(), + Some("/path/to/agent") + ); + assert_eq!(terminal_auth_task.args, vec!["--acp", "/auth"]); + assert_eq!( + terminal_auth_task.env, + HashMap::from_iter([ + ("BASE".into(), "1".into()), + ("AUTH_MODE".into(), "first-class".into()), + ]) + ); + assert_eq!(terminal_auth_task.label, "Login"); + } +} + fn mcp_servers_for_project(project: &Entity, cx: &App) -> Vec { let context_server_store = project.read(cx).context_server_store().read(cx); let is_local = project.read(cx).is_local(); diff --git a/crates/agent_servers/src/agent_servers.rs b/crates/agent_servers/src/agent_servers.rs index 020e36b999e3586430ae99b12af55a845de91cb8..983d6b5088ccec74c7076b7956dd4ff5f68f7da0 100644 --- a/crates/agent_servers/src/agent_servers.rs +++ b/crates/agent_servers/src/agent_servers.rs @@ -17,7 +17,7 @@ use gpui::{App, AppContext, Entity, Task}; use settings::SettingsStore; use std::{any::Any, rc::Rc, sync::Arc}; -pub use acp::AcpConnection; +pub use acp::{AcpConnection, GEMINI_TERMINAL_AUTH_METHOD_ID}; pub struct AgentServerDelegate { store: Entity, diff --git a/crates/agent_ui/src/conversation_view.rs b/crates/agent_ui/src/conversation_view.rs index 1f9a7cdd9316db3ce0dda882a0433e06df287a57..46453c2143841039aa441a6ed41d7d40acee5532 100644 --- a/crates/agent_ui/src/conversation_view.rs +++ b/crates/agent_ui/src/conversation_view.rs @@ -8,9 +8,9 @@ use acp_thread::{AgentConnection, Plan}; use action_log::{ActionLog, ActionLogTelemetry, DiffStats}; use agent::{NativeAgentServer, NativeAgentSessionList, SharedThread, ThreadStore}; use agent_client_protocol as acp; -use agent_servers::AgentServer; #[cfg(test)] use agent_servers::AgentServerDelegate; +use agent_servers::{AgentServer, GEMINI_TERMINAL_AUTH_METHOD_ID}; use agent_settings::{AgentProfileId, AgentSettings}; use anyhow::{Result, anyhow}; use arrayvec::ArrayVec; @@ -1475,6 +1475,9 @@ impl ConversationView { window: &mut Window, cx: &mut Context, ) { + let Some(workspace) = self.workspace.upgrade() else { + return; + }; let Some(connected) = self.as_connected_mut() else { return; }; @@ -1491,119 +1494,65 @@ impl ConversationView { let agent_telemetry_id = connection.telemetry_id(); - // Check for the experimental "terminal-auth" _meta field - let auth_method = connection.auth_methods().iter().find(|m| m.id() == &method); + if let Some(login) = connection.terminal_auth_task(&method, cx) { + configuration_view.take(); + pending_auth_method.replace(method.clone()); - if let Some(terminal_auth) = auth_method - .and_then(|a| match a { - acp::AuthMethod::EnvVar(env_var) => env_var.meta.as_ref(), - acp::AuthMethod::Terminal(terminal) => terminal.meta.as_ref(), - acp::AuthMethod::Agent(agent) => agent.meta.as_ref(), - _ => None, - }) - .and_then(|m| m.get("terminal-auth")) - { - // Extract terminal auth details from meta - if let (Some(command), Some(label)) = ( - terminal_auth.get("command").and_then(|v| v.as_str()), - terminal_auth.get("label").and_then(|v| v.as_str()), - ) { - let args = terminal_auth - .get("args") - .and_then(|v| v.as_array()) - .map(|arr| { - arr.iter() - .filter_map(|v| v.as_str().map(String::from)) - .collect() - }) - .unwrap_or_default(); - - let env = terminal_auth - .get("env") - .and_then(|v| v.as_object()) - .map(|obj| { - obj.iter() - .filter_map(|(k, v)| v.as_str().map(|val| (k.clone(), val.to_string()))) - .collect::>() - }) - .unwrap_or_default(); - - // Build SpawnInTerminal from _meta - let login = task::SpawnInTerminal { - id: task::TaskId(format!("external-agent-{}-login", label)), - full_label: label.to_string(), - label: label.to_string(), - command: Some(command.to_string()), - args, - command_label: label.to_string(), - env, - use_new_terminal: true, - allow_concurrent_runs: true, - hide: task::HideStrategy::Always, - ..Default::default() - }; - - configuration_view.take(); - pending_auth_method.replace(method.clone()); - - if let Some(workspace) = self.workspace.upgrade() { - let project = self.project.clone(); - let authenticate = Self::spawn_external_agent_login( - login, - workspace, - project, - method.clone(), - false, - window, - cx, - ); - cx.notify(); - self.auth_task = Some(cx.spawn_in(window, { - async move |this, cx| { - let result = authenticate.await; - - match &result { - Ok(_) => telemetry::event!( - "Authenticate Agent Succeeded", - agent = agent_telemetry_id - ), - Err(_) => { - telemetry::event!( - "Authenticate Agent Failed", - agent = agent_telemetry_id, - ) - } - } + let project = self.project.clone(); + let authenticate = Self::spawn_external_agent_login( + login, + workspace, + project, + method.clone(), + false, + window, + cx, + ); + cx.notify(); + self.auth_task = Some(cx.spawn_in(window, { + async move |this, cx| { + let result = authenticate.await; + + match &result { + Ok(_) => telemetry::event!( + "Authenticate Agent Succeeded", + agent = agent_telemetry_id + ), + Err(_) => { + telemetry::event!( + "Authenticate Agent Failed", + agent = agent_telemetry_id, + ) + } + } - this.update_in(cx, |this, window, cx| { - if let Err(err) = result { - if let Some(ConnectedServerState { - auth_state: - AuthState::Unauthenticated { - pending_auth_method, - .. - }, + this.update_in(cx, |this, window, cx| { + if let Err(err) = result { + if let Some(ConnectedServerState { + auth_state: + AuthState::Unauthenticated { + pending_auth_method, .. - }) = this.as_connected_mut() - { - pending_auth_method.take(); - } - if let Some(active) = this.active_thread() { - active.update(cx, |active, cx| { - active.handle_thread_error(err, cx); - }) - } - } else { - this.reset(window, cx); - } - this.auth_task.take() - }) - .ok(); + }, + .. + }) = this.as_connected_mut() + { + pending_auth_method.take(); + } + if let Some(active) = this.active_thread() { + active.update(cx, |active, cx| { + active.handle_thread_error(err, cx); + }) + } + } else { + this.reset(window, cx); } - })); + this.auth_task.take() + }) + .ok(); } - return; - } + })); + return; } configuration_view.take(); @@ -1726,7 +1675,7 @@ impl ConversationView { cx: &mut App, ) -> Task> { let Some(terminal_panel) = workspace.read(cx).panel::(cx) else { - return Task::ready(Ok(())); + return Task::ready(Err(anyhow!("Terminal panel is unavailable"))); }; window.spawn(cx, async move |cx| { @@ -1734,17 +1683,14 @@ impl ConversationView { if let Some(cmd) = &task.command { // Have "node" command use Zed's managed Node runtime by default if cmd == "node" { - let resolved_node_runtime = project - .update(cx, |project, cx| { - let agent_server_store = project.agent_server_store().clone(); - agent_server_store.update(cx, |store, cx| { - store.node_runtime().map(|node_runtime| { - cx.background_spawn(async move { - node_runtime.binary_path().await - }) - }) + let resolved_node_runtime = project.update(cx, |project, cx| { + let agent_server_store = project.agent_server_store().clone(); + agent_server_store.update(cx, |store, cx| { + store.node_runtime().map(|node_runtime| { + cx.background_spawn(async move { node_runtime.binary_path().await }) }) - }); + }) + }); if let Some(resolve_task) = resolved_node_runtime { if let Ok(node_path) = resolve_task.await { @@ -1756,14 +1702,8 @@ impl ConversationView { task.shell = task::Shell::WithArguments { program: task.command.take().expect("login command should be set"), args: std::mem::take(&mut task.args), - title_override: None + title_override: None, }; - task.full_label = task.label.clone(); - task.id = task::TaskId(format!("external-agent-{}-login", task.label)); - task.command_label = task.label.clone(); - task.use_new_terminal = true; - task.allow_concurrent_runs = true; - task.hide = task::HideStrategy::Always; let terminal = terminal_panel .update_in(cx, |terminal_panel, window, cx| { @@ -1772,7 +1712,7 @@ impl ConversationView { .await?; let success_patterns = match method.0.as_ref() { - "claude-login" | "spawn-gemini-cli" => vec![ + "claude-login" | GEMINI_TERMINAL_AUTH_METHOD_ID => vec![ "Login successful".to_string(), "Type your message".to_string(), ], @@ -1806,7 +1746,9 @@ impl ConversationView { cx.background_executor().timer(Duration::from_secs(1)).await; let content = terminal.update(cx, |terminal, _cx| terminal.get_content())?; - if success_patterns.iter().any(|pattern| content.contains(pattern)) + if success_patterns + .iter() + .any(|pattern| content.contains(pattern)) { return anyhow::Ok(()); } @@ -1823,8 +1765,23 @@ impl ConversationView { } } _ = exit_status => { - if !previous_attempt && project.read_with(cx, |project, _| project.is_via_remote_server()) && login.label.contains("gemini") { - return cx.update(|window, cx| Self::spawn_external_agent_login(login, workspace, project.clone(), method, true, window, cx))?.await + if !previous_attempt + && project.read_with(cx, |project, _| project.is_via_remote_server()) + && method.0.as_ref() == GEMINI_TERMINAL_AUTH_METHOD_ID + { + return cx + .update(|window, cx| { + Self::spawn_external_agent_login( + login, + workspace, + project.clone(), + method, + true, + window, + cx, + ) + })? + .await; } return Err(anyhow!("exited before logging in")); } diff --git a/crates/project/src/agent_server_store.rs b/crates/project/src/agent_server_store.rs index d8978b6512fa0c437a3d07feb929132a24461d0e..a9003e8567e6bf57ea18f8d70619e9417a626f43 100644 --- a/crates/project/src/agent_server_store.rs +++ b/crates/project/src/agent_server_store.rs @@ -1374,13 +1374,8 @@ impl ExternalAgentServer for LocalRegistryNpxAgent { .await .unwrap_or_default(); - let mut exec_args = Vec::new(); - exec_args.push("--yes".to_string()); - exec_args.push(package.to_string()); - if !args.is_empty() { - exec_args.push("--".to_string()); - exec_args.extend(args); - } + let mut exec_args = vec!["--yes".to_string(), "--".to_string(), package.to_string()]; + exec_args.extend(args); let npm_command = node_runtime .npm_command(