diff --git a/Cargo.lock b/Cargo.lock index 5704f4e63..71812ee25 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -45,9 +45,9 @@ dependencies = [ [[package]] name = "agent-client-protocol" -version = "0.9.0" +version = "0.9.2" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "c2ffe7d502c1e451aafc5aff655000f84d09c9af681354ac0012527009b1af13" +checksum = "d3e527d7dfe0f334313d42d1d9318f0a79665f6f21c440d0798f230a77a7ed6c" dependencies = [ "agent-client-protocol-schema", "anyhow", @@ -62,9 +62,9 @@ dependencies = [ [[package]] name = "agent-client-protocol-schema" -version = "0.10.3" +version = "0.10.5" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "5253c29698d09f21f27c6150b132977253aa9463db594688a79aa4e69d02ec67" +checksum = "6903a00e8ac822f9bacac59a1932754d7387c72ebb7c9c7439ad021505591da4" dependencies = [ "anyhow", "derive_more", @@ -5396,6 +5396,7 @@ dependencies = [ "stakpak-tui", "tar", "tempfile", + "test-case", "tokio", "tokio-util", "toml 0.8.23", @@ -5761,6 +5762,39 @@ dependencies = [ "libc", ] +[[package]] +name = "test-case" +version = "3.3.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "eb2550dd13afcd286853192af8601920d959b14c401fcece38071d53bf0768a8" +dependencies = [ + "test-case-macros", +] + +[[package]] +name = "test-case-core" +version = "3.3.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "adcb7fd841cd518e279be3d5a3eb0636409487998a4aff22f3de87b81e88384f" +dependencies = [ + "cfg-if", + "proc-macro2", + "quote", + "syn 2.0.110", +] + +[[package]] +name = "test-case-macros" +version = "3.3.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "5c89e72a01ed4c579669add59014b9a524d609c0c88c6a585ce37485879f6ffb" +dependencies = [ + "proc-macro2", + "quote", + "syn 2.0.110", + "test-case-core", +] + [[package]] name = "textwrap" version = "0.16.2" diff --git a/cli/Cargo.toml b/cli/Cargo.toml index 658955560..eb4278128 100644 --- a/cli/Cargo.toml +++ b/cli/Cargo.toml @@ -36,7 +36,7 @@ crossterm = { workspace = true } open = { workspace = true } glob = "0.3" # ACP dependencies -agent-client-protocol = "0.9.0" +agent-client-protocol = "0.9.2" jsonrpc-core = "18.0.0" jsonrpc-derive = "18.0.0" tokio-util = { version = "0.7", features = ["compat"] } @@ -47,6 +47,7 @@ base64 = "0.22" [dev-dependencies] tempfile = "3" +test-case = "3" [lints.clippy] unwrap_used = "deny" diff --git a/cli/src/commands/acp/fs_handler.rs b/cli/src/commands/acp/fs_handler.rs index 433e104e4..39b9de191 100644 --- a/cli/src/commands/acp/fs_handler.rs +++ b/cli/src/commands/acp/fs_handler.rs @@ -86,8 +86,9 @@ pub async fn execute_acp_fs_tool( let args: serde_json::Value = serde_json::from_str(&tool_call.function.arguments) .map_err(|e| format!("Failed to parse tool arguments: {}", e))?; + use super::tool_names; match tool_call.function.name.as_str() { - "view" => { + tool_names::VIEW => { let path = args .get("path") .and_then(|p| p.as_str()) @@ -142,7 +143,7 @@ pub async fn execute_acp_fs_tool( structured_content: None, })) } - "create" => { + tool_names::CREATE => { let path = args .get("path") .and_then(|p| p.as_str()) @@ -185,7 +186,7 @@ pub async fn execute_acp_fs_tool( structured_content: None, })) } - "str_replace" => { + tool_names::STR_REPLACE => { let path = args .get("path") .and_then(|p| p.as_str()) diff --git a/cli/src/commands/acp/mod.rs b/cli/src/commands/acp/mod.rs index 029e1bdfc..85634053e 100644 --- a/cli/src/commands/acp/mod.rs +++ b/cli/src/commands/acp/mod.rs @@ -2,3 +2,4 @@ pub mod fs_handler; pub mod server; pub mod utils; pub use server::StakpakAcpAgent; +pub use stakpak_mcp_server::tool_names; diff --git a/cli/src/commands/acp/server.rs b/cli/src/commands/acp/server.rs index 009e37ba6..0c32e6acd 100644 --- a/cli/src/commands/acp/server.rs +++ b/cli/src/commands/acp/server.rs @@ -58,6 +58,8 @@ pub struct StakpakAcpAgent { streaming_buffer: Arc>, // Channel for native ACP filesystem operations fs_operation_tx: Option>, + // Capabilities advertised by the client during initialization + client_capabilities: Arc>, } impl StakpakAcpAgent { @@ -153,9 +155,17 @@ impl StakpakAcpAgent { current_streaming_message: Arc::new(tokio::sync::Mutex::new(String::new())), streaming_buffer: Arc::new(tokio::sync::Mutex::new(String::new())), fs_operation_tx: None, + client_capabilities: Arc::new(tokio::sync::Mutex::new( + acp::ClientCapabilities::default(), + )), }) } + async fn client_fs_capabilities(&self) -> (bool, bool) { + let caps = self.client_capabilities.lock().await; + (caps.fs.read_text_file, caps.fs.write_text_file) + } + // Helper method to send proper ACP tool call notifications #[allow(clippy::too_many_arguments)] async fn send_tool_call_notification( @@ -306,8 +316,9 @@ impl StakpakAcpAgent { // Helper method to generate appropriate tool title based on tool type and arguments fn generate_tool_title(&self, tool_name: &str, raw_input: &serde_json::Value) -> String { + use super::tool_names; match tool_name { - "view" => { + tool_names::VIEW => { // Extract path from arguments for view tool if let Some(path) = raw_input.get("path").and_then(|p| p.as_str()) { format!("Read {}", path) @@ -315,7 +326,7 @@ impl StakpakAcpAgent { "Read".to_string() } } - "run_command" => { + tool_names::RUN_COMMAND => { // Extract command from arguments for run_command tool if let Some(command) = raw_input.get("command").and_then(|c| c.as_str()) { format!("Run command {}", command) @@ -323,7 +334,7 @@ impl StakpakAcpAgent { "Run command".to_string() } } - "create" | "create_file" => { + tool_names::CREATE | tool_names::CREATE_FILE => { // Extract path from arguments for create tool if let Some(path) = raw_input.get("path").and_then(|p| p.as_str()) { format!("Creating {}", path) @@ -331,7 +342,7 @@ impl StakpakAcpAgent { "Creating".to_string() } } - "str_replace" | "edit_file" => { + tool_names::STR_REPLACE | tool_names::EDIT_FILE => { // Extract path from arguments for edit tool if let Some(path) = raw_input.get("path").and_then(|p| p.as_str()) { format!("Editing {}", path) @@ -339,7 +350,7 @@ impl StakpakAcpAgent { "Editing".to_string() } } - "delete_file" => { + tool_names::DELETE_FILE => { // Extract path from arguments for delete tool if let Some(path) = raw_input.get("path").and_then(|p| p.as_str()) { format!("Deleting {}", path) @@ -347,7 +358,7 @@ impl StakpakAcpAgent { "Deleting".to_string() } } - "search_docs" => { + tool_names::SEARCH_DOCS => { // Extract query from arguments for search tool if let Some(query) = raw_input.get("query").and_then(|q| q.as_str()) { format!("Search docs: {}", query) @@ -355,7 +366,7 @@ impl StakpakAcpAgent { "Search docs".to_string() } } - "local_code_search" => { + tool_names::LOCAL_CODE_SEARCH => { // Extract query from arguments for search tool if let Some(query) = raw_input.get("query").and_then(|q| q.as_str()) { format!("Search local context: {}", query) @@ -363,7 +374,7 @@ impl StakpakAcpAgent { "Search local context".to_string() } } - "read_rulebook" => "Read rulebook".to_string(), + tool_names::READ_RULEBOOK => "Read rulebook".to_string(), _ => { // Default case: format tool name nicely and add path if available let formatted_name = self.format_tool_name(tool_name); @@ -395,35 +406,36 @@ impl StakpakAcpAgent { // Helper method to get appropriate ToolKind based on tool name fn get_tool_kind(&self, tool_name: &str) -> acp::ToolKind { - match tool_name { - "view" | "read_rulebook" => acp::ToolKind::Read, - "run_command" => acp::ToolKind::Execute, - "create" | "create_file" | "str_replace" | "edit_file" => acp::ToolKind::Edit, - "delete_file" => acp::ToolKind::Delete, - "search_docs" | "local_code_search" => acp::ToolKind::Search, - _ => acp::ToolKind::Other, + use super::tool_names; + if tool_names::is_fs_file_read(tool_name) || tool_name == tool_names::READ_RULEBOOK { + acp::ToolKind::Read + } else if tool_names::is_fs_file_write(tool_name) { + acp::ToolKind::Edit + } else if tool_name == tool_names::RUN_COMMAND { + acp::ToolKind::Execute + } else if tool_name == tool_names::DELETE_FILE { + acp::ToolKind::Delete + } else if tool_name == tool_names::SEARCH_DOCS || tool_name == tool_names::LOCAL_CODE_SEARCH + { + acp::ToolKind::Search + } else { + acp::ToolKind::Other } } // Helper method to determine if a tool should use Diff content type fn should_use_diff_content(&self, tool_name: &str) -> bool { - matches!( - tool_name, - "create" | "create_file" | "str_replace" | "edit_file" - ) + super::tool_names::is_fs_file_write(tool_name) } // Helper method to determine if a tool is a file creation tool fn is_file_creation_tool(&self, tool_name: &str) -> bool { - matches!(tool_name, "create" | "create_file") + tool_name == super::tool_names::CREATE || tool_name == super::tool_names::CREATE_FILE } // Helper method to determine if a tool should be auto-approved fn is_auto_approved_tool(&self, tool_name: &str) -> bool { - matches!( - tool_name, - "view" | "search_docs" | "read_rulebook" | "local_code_search" - ) + super::tool_names::is_auto_approved(tool_name) } // Helper method to create proper rawInput for tool calls @@ -863,18 +875,24 @@ impl StakpakAcpAgent { // Check if this is a filesystem tool that should use native ACP // Decide if this should be handled by native ACP FS. Avoid read_text_file for directories. - let is_view_directory = if tool_call.function.name == "view" { + let is_view_directory = if tool_call.function.name == super::tool_names::VIEW { Path::new(&abs_path).is_dir() } else { false }; - let is_fs_tool = matches!( - tool_call.function.name.as_str(), - "view" | "create" | "str_replace" - ) && !is_view_directory; + let tool_name = tool_call.function.name.as_str(); + let is_read_tool = super::tool_names::is_fs_file_read(tool_name) && !is_view_directory; + let is_write_tool = super::tool_names::is_fs_file_write(tool_name); + + // Delegate fs operations to the client so it can access unsaved editor + // state and track modifications. Per ACP spec, both read and write + // require the client to advertise the corresponding capability. + let (client_reads, client_writes) = self.client_fs_capabilities().await; + let should_delegate = self.fs_operation_tx.is_some() + && ((is_read_tool && client_reads) || (is_write_tool && client_writes)); - let result = if is_fs_tool && self.fs_operation_tx.is_some() { + let result = if should_delegate { log::info!( "🔧 DEBUG: Executing filesystem tool via native ACP: {}", tool_call.function.name @@ -1440,6 +1458,7 @@ impl StakpakAcpAgent { current_streaming_message: self.current_streaming_message.clone(), streaming_buffer: self.streaming_buffer.clone(), fs_operation_tx: Some(fs_operation_tx), + client_capabilities: self.client_capabilities.clone(), }; // Start up the StakpakAcpAgent connected to stdio. @@ -1559,6 +1578,7 @@ impl Clone for StakpakAcpAgent { current_streaming_message: self.current_streaming_message.clone(), streaming_buffer: self.streaming_buffer.clone(), fs_operation_tx: self.fs_operation_tx.clone(), + client_capabilities: self.client_capabilities.clone(), } } } @@ -1571,6 +1591,12 @@ impl acp::Agent for StakpakAcpAgent { ) -> Result { log::info!("Received initialize request {args:?}"); + // Store client capabilities for later use + { + let mut caps = self.client_capabilities.lock().await; + *caps = args.client_capabilities.clone(); + } + // If no API key, provide an auth method that links to GitHub let auth_methods = if self.config.api_key.is_none() { vec![acp::AuthMethod::new( @@ -2072,3 +2098,39 @@ impl acp::Agent for StakpakAcpAgent { Ok(()) } } + +#[cfg(test)] +mod tests { + use crate::commands::acp::tool_names; + use test_case::test_case; + + // Per ACP spec, agents MUST check client capabilities before delegating fs operations. + // Both readTextFile and writeTextFile default to false - delegation requires explicit opt-in. + // + // Columns: tool_name, client_reads, client_writes, expected_delegate + #[test_case("view", true, true, true; "read tool, client reads: delegate")] + #[test_case("view", true, false, true; "read tool, client reads (no writes): delegate")] + #[test_case("view", false, true, false; "read tool, client no reads: fallback")] + #[test_case("view", false, false, false; "read tool, client no fs: fallback")] + #[test_case("create", true, true, true; "write tool, client writes: delegate")] + #[test_case("create", false, true, true; "write tool, client writes (no reads): delegate")] + #[test_case("create", true, false, false; "write tool, client no writes: fallback")] + #[test_case("create", false, false, false; "write tool, client no fs: fallback")] + fn test_fs_delegation_respects_client_capabilities( + tool_name: &str, + client_reads: bool, + client_writes: bool, + expected: bool, + ) { + let is_read_tool = tool_names::is_fs_file_read(tool_name); + let is_write_tool = tool_names::is_fs_file_write(tool_name); + + let should_delegate = (is_read_tool && client_reads) || (is_write_tool && client_writes); + + assert_eq!( + should_delegate, expected, + "tool={} (read={}, write={}), caps(r={}, w={}) => delegate={}", + tool_name, is_read_tool, is_write_tool, client_reads, client_writes, should_delegate + ); + } +} diff --git a/libs/mcp/server/src/lib.rs b/libs/mcp/server/src/lib.rs index 0dcd78182..4074e45d3 100644 --- a/libs/mcp/server/src/lib.rs +++ b/libs/mcp/server/src/lib.rs @@ -19,6 +19,39 @@ pub mod remote_tools; pub mod subagent_tools; pub mod tool_container; +pub mod tool_names { + pub const VIEW: &str = "view"; + pub const CREATE: &str = "create"; + pub const STR_REPLACE: &str = "str_replace"; + pub const CREATE_FILE: &str = "create_file"; + pub const EDIT_FILE: &str = "edit_file"; + pub const RUN_COMMAND: &str = "run_command"; + pub const SEARCH_DOCS: &str = "search_docs"; + pub const READ_RULEBOOK: &str = "read_rulebook"; + pub const LOCAL_CODE_SEARCH: &str = "local_code_search"; + pub const DELETE_FILE: &str = "delete_file"; + + const FS_FILE_READ: &[&str] = &[VIEW]; + const FS_FILE_WRITE: &[&str] = &[CREATE, CREATE_FILE, STR_REPLACE, EDIT_FILE]; + pub const AUTO_APPROVED: &[&str] = &[VIEW, SEARCH_DOCS, READ_RULEBOOK, LOCAL_CODE_SEARCH]; + + pub fn is_fs_file_read(name: &str) -> bool { + FS_FILE_READ.contains(&name) + } + + pub fn is_fs_file_write(name: &str) -> bool { + FS_FILE_WRITE.contains(&name) + } + + pub fn is_fs_tool(name: &str) -> bool { + is_fs_file_read(name) || is_fs_file_write(name) + } + + pub fn is_auto_approved(name: &str) -> bool { + AUTO_APPROVED.contains(&name) + } +} + #[derive(Clone, Debug, Default)] pub struct EnabledToolsConfig { pub slack: bool,