diff --git a/CHANGELOG.md b/CHANGELOG.md index 5253229..87e4179 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,22 @@ All notable changes to this project will be documented in this file. ## [Unreleased] +### New Features + +- **LSP Readiness Tool**: Added `wait_for_lsp_ready` tool for ensuring LSP client + readiness before performing operations, improving reliability of LSP workflows + +### Improved + +- **Configurable LSP Timeout**: Added `NeovimClientConfig` with configurable LSP + timeout settings (default: 3000ms) for better control over LSP operation timing +- **Enhanced Notification Tracking**: Comprehensive notification tracking system + for robust LSP synchronization and event handling +- **Autocmd Setup**: Unified autocmd setup replacing diagnostics-specific + implementation with more comprehensive event handling +- **Test Performance**: Optimized integration tests with better timing and + one-time binary compilation for improved CI performance + ## [v0.5.0] - 2025-08-20 ### Fixed diff --git a/CLAUDE.md b/CLAUDE.md index a380bac..2900cd0 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -134,6 +134,9 @@ The codebase follows a modular architecture with clear separation of concerns: - Supports both TCP and Unix socket/named pipe connections - Provides high-level operations: buffer management, diagnostics, LSP integration - Handles Lua code execution and autocmd setup + - Includes configurable LSP timeout settings via `NeovimClientConfig` + - Features comprehensive notification tracking system for LSP synchronization + - Provides methods for waiting on LSP readiness and diagnostic availability - **`src/neovim/connection.rs`**: Connection management layer - Wraps `nvim-rs` client with lifecycle management @@ -210,7 +213,7 @@ This modular architecture provides several advantages: ### Available MCP Tools -The server provides these 23 tools (implemented with `#[tool]` attribute): +The server provides these 24 tools (implemented with `#[tool]` attribute): **Connection Management:** @@ -224,30 +227,31 @@ The server provides these 23 tools (implemented with `#[tool]` attribute): 1. **`list_buffers`**: List all open buffers for specific connection 2. **`exec_lua`**: Execute arbitrary Lua code in specific Neovim instance -3. **`buffer_diagnostics`**: Get diagnostics for specific buffer on specific connection -4. **`lsp_clients`**: Get workspace LSP clients for specific connection -5. **`lsp_workspace_symbols`**: Search workspace symbols by query on specific +3. **`wait_for_lsp_ready`**: Wait for LSP client to be ready and attached with timeout +4. **`buffer_diagnostics`**: Get diagnostics for specific buffer on specific connection +5. **`lsp_clients`**: Get workspace LSP clients for specific connection +6. **`lsp_workspace_symbols`**: Search workspace symbols by query on specific connection -6. **`lsp_code_actions`**: Get LSP code actions with universal document +7. **`lsp_code_actions`**: Get LSP code actions with universal document identification (supports buffer IDs, project-relative paths, and absolute paths) -7. **`lsp_hover`**: Get LSP hover information with universal document +8. **`lsp_hover`**: Get LSP hover information with universal document identification (supports buffer IDs, project-relative paths, and absolute paths) -8. **`lsp_document_symbols`**: Get document symbols with universal document +9. **`lsp_document_symbols`**: Get document symbols with universal document identification (supports buffer IDs, project-relative paths, and absolute paths) -9. **`lsp_references`**: Get LSP references with universal document +10. **`lsp_references`**: Get LSP references with universal document identification (supports buffer IDs, project-relative paths, and absolute paths) -10. **`lsp_resolve_code_action`**: Resolve code actions that may have +11. **`lsp_resolve_code_action`**: Resolve code actions that may have incomplete data -11. **`lsp_apply_edit`**: Apply workspace edits using Neovim's LSP utility +12. **`lsp_apply_edit`**: Apply workspace edits using Neovim's LSP utility functions -12. **`lsp_definition`**: Get LSP definition with universal document identification -13. **`lsp_type_definition`**: Get LSP type definition with universal document identification -14. **`lsp_implementations`**: Get LSP implementations with universal document identification -15. **`lsp_declaration`**: Get LSP declaration with universal document identification -16. **`lsp_rename`**: Rename symbol across workspace using LSP -17. **`lsp_formatting`**: Format document using LSP with optional auto-apply -18. **`lsp_range_formatting`**: Format a specific range in a document using LSP -19. **`lsp_organize_imports`**: Sort and organize imports using LSP with +13. **`lsp_definition`**: Get LSP definition with universal document identification +14. **`lsp_type_definition`**: Get LSP type definition with universal document identification +15. **`lsp_implementations`**: Get LSP implementations with universal document identification +16. **`lsp_declaration`**: Get LSP declaration with universal document identification +17. **`lsp_rename`**: Rename symbol across workspace using LSP +18. **`lsp_formatting`**: Format document using LSP with optional auto-apply +19. **`lsp_range_formatting`**: Format a specific range in a document using LSP +20. **`lsp_organize_imports`**: Sort and organize imports using LSP with auto-apply by default ### Universal Document Identifier System @@ -505,6 +509,9 @@ the new `lua_tools.rs` module: lsp_apply_edit - **Test data**: Includes Go source files and LSP configuration for realistic testing scenarios +- **Enhanced reliability**: Robust LSP synchronization with notification tracking +- **Optimized timing**: Better test performance with improved setup and teardown +- **Notification testing**: Unit tests for notification tracking system ## Error Handling diff --git a/README.md b/README.md index 4883a52..12f1b08 100644 --- a/README.md +++ b/README.md @@ -219,7 +219,7 @@ and resources - only the transport layer changes. ## Available Tools -The server provides 23 MCP tools for interacting with Neovim: +The server provides 24 MCP tools for interacting with Neovim: ### Connection Management @@ -366,6 +366,11 @@ providing enhanced flexibility for code analysis and navigation. - **`exec_lua`**: Execute Lua code in Neovim - Parameters: `connection_id` (string), `code` (string) - Lua code to execute +- **`wait_for_lsp_ready`**: Wait for LSP client to be ready and attached + - Parameters: `connection_id` (string), `client_name` (string, optional), + `timeout_ms` (number, optional, default: 5000ms) + - Returns: Success confirmation with LSP client readiness status + ### Complete LSP Code Action Workflow The server now supports the full LSP code action lifecycle: diff --git a/docs/instructions.md b/docs/instructions.md index a7e0860..72a5eb6 100644 --- a/docs/instructions.md +++ b/docs/instructions.md @@ -4,7 +4,7 @@ ### Tools -The server provides 23 MCP tools for interacting with Neovim instances: +The server provides 24 MCP tools for interacting with Neovim instances: #### Connection Management @@ -48,6 +48,16 @@ All tools below require a `connection_id` parameter from connection establishmen - **Returns**: Object with execution result - **Usage**: Run Neovim commands, get editor state, or modify configuration +- **`wait_for_lsp_ready`**: Wait for LSP client to be ready and attached + - **Parameters**: + - `connection_id` (string): Target Neovim instance ID + - `client_name` (string, optional): Specific LSP client name to wait for + (waits for any if not specified) + - `timeout_ms` (number, optional): Timeout in milliseconds (default: 5000ms) + - **Returns**: Success confirmation with client readiness status + - **Usage**: Ensure LSP client is ready before performing LSP operations + for reliable results + - **`buffer_diagnostics`**: Get diagnostics for specific buffer - **Parameters**: - `connection_id` (string): Target Neovim instance ID diff --git a/src/neovim/client.rs b/src/neovim/client.rs index 6e17d87..0cd0745 100644 --- a/src/neovim/client.rs +++ b/src/neovim/client.rs @@ -5,13 +5,19 @@ use std::fmt::{self, Display}; use std::marker::PhantomData; use std::path::{Path, PathBuf}; use std::str::FromStr; +use std::sync::Arc; use async_trait::async_trait; use nvim_rs::{Handler, Neovim, create::tokio as create}; use rmpv::Value; use serde::de::{self, MapAccess, Visitor}; use serde::{Deserialize, Deserializer}; -use tokio::{io::AsyncWrite, net::TcpStream}; +use tokio::{ + io::AsyncWrite, + net::TcpStream, + sync::Mutex, + time::{Duration, timeout}, +}; use tracing::{debug, info, instrument}; use super::{connection::NeovimConnection, error::NeovimError}; @@ -32,7 +38,28 @@ pub trait NeovimClientTrait: Sync { async fn execute_lua(&self, code: &str) -> Result; /// Set up diagnostics changed autocmd - async fn setup_diagnostics_changed_autocmd(&self) -> Result<(), NeovimError>; + async fn setup_autocmd(&self) -> Result<(), NeovimError>; + + /// Wait for a specific notification with timeout + async fn wait_for_notification( + &self, + notification_name: &str, + timeout_ms: u64, + ) -> Result; + + /// Wait for LSP client to be ready and attached + async fn wait_for_lsp_ready( + &self, + client_name: Option<&str>, + timeout_ms: u64, + ) -> Result<(), NeovimError>; + + /// Wait for diagnostics to be available for a specific buffer or workspace + async fn wait_for_diagnostics( + &self, + buffer_id: Option, + timeout_ms: u64, + ) -> Result, NeovimError>; /// Get diagnostics for a specific buffer async fn get_buffer_diagnostics(&self, buffer_id: u64) -> Result, NeovimError>; @@ -178,22 +205,181 @@ pub trait NeovimClientTrait: Sync { ) -> Result<(), NeovimError>; } +/// Notification tracking structure +#[derive(Debug, Clone)] +pub struct Notification { + pub name: String, + pub args: Vec, + pub timestamp: std::time::SystemTime, +} + +/// Shared state for notification tracking +#[derive(Clone)] +pub struct NotificationTracker { + notifications: Arc>>, + notify_wakers: Arc>>>>, +} + +/// Configuration for notification cleanup +const MAX_STORED_NOTIFICATIONS: usize = 100; +const NOTIFICATION_EXPIRY_SECONDS: u64 = 30; + +impl NotificationTracker { + pub fn new() -> Self { + Self { + notifications: Arc::new(Mutex::new(Vec::new())), + notify_wakers: Arc::new(Mutex::new(HashMap::new())), + } + } + + /// Clean up expired and excess notifications + async fn cleanup_notifications(&self) { + let mut notifications = self.notifications.lock().await; + + // Remove expired notifications + let now = std::time::SystemTime::now(); + notifications.retain(|n| { + now.duration_since(n.timestamp) + .map(|d| d.as_secs() < NOTIFICATION_EXPIRY_SECONDS) + .unwrap_or(false) + }); + + // If still too many notifications, keep only the most recent ones + if notifications.len() > MAX_STORED_NOTIFICATIONS { + let excess = notifications.len() - MAX_STORED_NOTIFICATIONS; + notifications.drain(0..excess); + } + } + + /// Record a notification + pub async fn record_notification(&self, name: String, args: Vec) { + let notification = Notification { + name: name.clone(), + args, + timestamp: std::time::SystemTime::now(), + }; + + // Notify any waiting tasks for this specific notification name first + let mut wakers = self.notify_wakers.lock().await; + if let Some(waiters) = wakers.get_mut(&name) { + while let Some(waker) = waiters.pop() { + let _ = waker.send(notification.clone()); + } + } + + // Clean up wakers with no waiters + wakers.retain(|_, waiters| !waiters.is_empty()); + drop(wakers); // Release lock early + + // Always store recent notifications for potential future requests + // but clean up old/excess ones to prevent memory leaks + { + let mut notifications = self.notifications.lock().await; + notifications.push(notification); + + // Trigger cleanup if we're approaching the limit + if notifications.len() > MAX_STORED_NOTIFICATIONS * 3 / 4 { + drop(notifications); // Release lock before calling cleanup + self.cleanup_notifications().await; + } + } + } + + /// Wait for a specific notification with timeout + pub async fn wait_for_notification( + &self, + notification_name: &str, + timeout_duration: Duration, + ) -> Result { + // First check if a recent (non-expired) notification already exists + { + let notifications = self.notifications.lock().await; + let now = std::time::SystemTime::now(); + + if let Some(notification) = notifications + .iter() + .rev() // Check most recent first + .find(|n| { + n.name == notification_name + && now + .duration_since(n.timestamp) + .map(|d| d.as_secs() < NOTIFICATION_EXPIRY_SECONDS) + .unwrap_or(false) + }) + { + return Ok(notification.clone()); + } + } + + // Create a oneshot channel to wait for the notification + let (tx, rx) = tokio::sync::oneshot::channel(); + + // Register our interest in this notification + let mut wakers = self.notify_wakers.lock().await; + wakers + .entry(notification_name.to_string()) + .or_insert_with(Vec::new) + .push(tx); + + // Wait for the notification with timeout + drop(wakers); // Release the lock before awaiting + + match timeout(timeout_duration, rx).await { + Ok(Ok(notification)) => Ok(notification), + Ok(Err(_)) => Err(NeovimError::Api( + "Notification channel closed unexpectedly".to_string(), + )), + Err(_) => Err(NeovimError::Api(format!( + "Timeout waiting for notification: {}", + notification_name + ))), + } + } + + /// Clear all recorded notifications + pub async fn clear_notifications(&self) { + let mut notifications = self.notifications.lock().await; + notifications.clear(); + } + + /// Manually trigger cleanup of expired notifications + #[allow(dead_code)] + pub(crate) async fn cleanup_expired_notifications(&self) { + self.cleanup_notifications().await; + } + + /// Get current notification statistics (for debugging/monitoring) + #[allow(dead_code)] + pub(crate) async fn get_stats(&self) -> (usize, usize) { + let notifications = self.notifications.lock().await; + let wakers = self.notify_wakers.lock().await; + (notifications.len(), wakers.len()) + } +} + pub struct NeovimHandler { _marker: std::marker::PhantomData, + notification_tracker: NotificationTracker, } impl NeovimHandler { pub fn new() -> Self { NeovimHandler { _marker: std::marker::PhantomData, + notification_tracker: NotificationTracker::new(), } } + + pub fn notification_tracker(&self) -> NotificationTracker { + self.notification_tracker.clone() + } } impl Clone for NeovimHandler { fn clone(&self) -> Self { NeovimHandler { _marker: std::marker::PhantomData, + notification_tracker: self.notification_tracker.clone(), } } } @@ -207,6 +393,9 @@ where async fn handle_notify(&self, name: String, args: Vec, _neovim: Neovim) { info!("handling notification: {name:?}, {args:?}"); + self.notification_tracker + .record_notification(name, args) + .await; } async fn handle_request( @@ -1035,11 +1224,28 @@ pub struct RenameRequestParams { pub new_name: String, } +/// Configuration for Neovim client operations +#[derive(Debug, Clone)] +pub struct NeovimClientConfig { + /// Timeout in milliseconds for LSP operations (default: 3000ms) + pub lsp_timeout_ms: u64, +} + +impl Default for NeovimClientConfig { + fn default() -> Self { + Self { + lsp_timeout_ms: 3000, + } + } +} + pub struct NeovimClient where T: AsyncWrite + Send + 'static, { connection: Option>, + notification_tracker: Option, + config: NeovimClientConfig, } #[cfg(unix)] @@ -1103,6 +1309,7 @@ impl NeovimClient { debug!("Attempting to connect to Neovim at {}", path); let handler = NeovimHandler::new(); + let notification_tracker = handler.notification_tracker(); match create::new_path(path, handler).await { Ok((nvim, io_handler)) => { let connection = NeovimConnection::new( @@ -1115,6 +1322,7 @@ impl NeovimClient { path.to_string(), ); self.connection = Some(connection); + self.notification_tracker = Some(notification_tracker); debug!("Successfully connected to Neovim at {}", path); Ok(()) } @@ -1138,6 +1346,7 @@ impl NeovimClient { debug!("Attempting to connect to Neovim at {}", address); let handler = NeovimHandler::new(); + let notification_tracker = handler.notification_tracker(); match create::new_tcp(address, handler).await { Ok((nvim, io_handler)) => { let connection = NeovimConnection::new( @@ -1150,6 +1359,7 @@ impl NeovimClient { address.to_string(), ); self.connection = Some(connection); + self.notification_tracker = Some(notification_tracker); debug!("Successfully connected to Neovim at {}", address); Ok(()) } @@ -1166,7 +1376,18 @@ where T: AsyncWrite + Send + 'static, { pub fn new() -> Self { - Self { connection: None } + Self { + connection: None, + notification_tracker: None, + config: NeovimClientConfig::default(), + } + } + + /// Configure the Neovim client with custom settings + #[allow(dead_code)] + pub fn with_config(mut self, config: NeovimClientConfig) -> Self { + self.config = config; + self } #[instrument(skip(self))] @@ -1309,6 +1530,12 @@ where if let Some(connection) = self.connection.take() { let target = connection.target().to_string(); connection.io_handler.abort(); + + // Clear notification tracker to free memory + if let Some(tracker) = self.notification_tracker.take() { + tracker.clear_notifications().await; + } + debug!("Successfully disconnected from Neovim at {}", target); Ok(target) } else { @@ -1371,8 +1598,8 @@ where } #[instrument(skip(self))] - async fn setup_diagnostics_changed_autocmd(&self) -> Result<(), NeovimError> { - debug!("Setting up diagnostics changed autocmd"); + async fn setup_autocmd(&self) -> Result<(), NeovimError> { + debug!("Setting up autocmd"); let conn = self.connection.as_ref().ok_or_else(|| { NeovimError::Connection("Not connected to any Neovim instance".to_string()) @@ -1380,18 +1607,16 @@ where match conn .nvim - .exec_lua(include_str!("lua/diagnostics_autocmd.lua"), vec![]) + .exec_lua(include_str!("lua/setup_autocmd.lua"), vec![]) .await { Ok(_) => { - debug!("Autocmd for diagnostics changed set up successfully"); + debug!("autocmd set up successfully"); Ok(()) } Err(e) => { - debug!("Failed to set up diagnostics changed autocmd: {}", e); - Err(NeovimError::Api(format!( - "Failed to set up diagnostics changed autocmd: {e}" - ))) + debug!("Failed to set up autocmd: {}", e); + Err(NeovimError::Api(format!("Failed to set up autocmd: {e}"))) } } } @@ -1490,7 +1715,7 @@ where }) .unwrap(), ), // params - Value::from(1000), // timeout_ms + Value::from(self.config.lsp_timeout_ms), // timeout_ms Value::from(buffer_id), // bufnr ], ) @@ -1545,7 +1770,7 @@ where }) .unwrap(), ), // params - Value::from(1000), // timeout_ms + Value::from(self.config.lsp_timeout_ms), // timeout_ms Value::from(buffer_id), // bufnr ], ) @@ -1598,7 +1823,7 @@ where Value::from( serde_json::to_string(&DocumentSymbolParams { text_document }).unwrap(), ), // params - Value::from(1000), // timeout_ms + Value::from(self.config.lsp_timeout_ms), // timeout_ms Value::from(buffer_id), // bufnr ], ) @@ -1648,7 +1873,7 @@ where }) .unwrap(), ), // params - Value::from(1000), // timeout_ms + Value::from(self.config.lsp_timeout_ms), // timeout_ms ], ) .await @@ -1711,7 +1936,7 @@ where }) .unwrap(), ), // params - Value::from(1000), // timeout_ms + Value::from(self.config.lsp_timeout_ms), // timeout_ms Value::from(buffer_id), // bufnr ], ) @@ -1768,7 +1993,7 @@ where }) .unwrap(), ), // params - Value::from(1000), // timeout_ms + Value::from(self.config.lsp_timeout_ms), // timeout_ms ], ) .await @@ -1821,7 +2046,7 @@ where }) .unwrap(), ), // params - Value::from(1000), // timeout_ms + Value::from(self.config.lsp_timeout_ms), // timeout_ms ], ) .await @@ -1874,7 +2099,7 @@ where }) .unwrap(), ), // params - Value::from(1000), // timeout_ms + Value::from(self.config.lsp_timeout_ms), // timeout_ms ], ) .await @@ -1927,7 +2152,7 @@ where }) .unwrap(), ), // params - Value::from(1000), // timeout_ms + Value::from(self.config.lsp_timeout_ms), // timeout_ms ], ) .await @@ -2075,7 +2300,7 @@ where }) .unwrap(), ), - Value::from(1000), + Value::from(self.config.lsp_timeout_ms), Value::from(buffer_id), ], ) @@ -2193,7 +2418,7 @@ where }) .unwrap(), ), - Value::from(1000), // timeout_ms + Value::from(self.config.lsp_timeout_ms), // timeout_ms ], ) .await @@ -2257,7 +2482,7 @@ where }) .unwrap(), ), - Value::from(1000), // timeout_ms + Value::from(self.config.lsp_timeout_ms), // timeout_ms ], ) .await @@ -2327,7 +2552,7 @@ where }) .unwrap(), ), // params - Value::from(1000), // timeout_ms + Value::from(self.config.lsp_timeout_ms), // timeout_ms Value::from(buffer_id), // bufnr ], ) @@ -2393,6 +2618,121 @@ where } } } + + #[instrument(skip(self))] + async fn wait_for_notification( + &self, + notification_name: &str, + timeout_ms: u64, + ) -> Result { + debug!( + "Waiting for notification: {} with timeout: {}ms", + notification_name, timeout_ms + ); + + let tracker = self.notification_tracker.as_ref().ok_or_else(|| { + NeovimError::Connection("Not connected to any Neovim instance".to_string()) + })?; + + tracker + .wait_for_notification(notification_name, Duration::from_millis(timeout_ms)) + .await + } + + #[instrument(skip(self))] + async fn wait_for_lsp_ready( + &self, + client_name: Option<&str>, + timeout_ms: u64, + ) -> Result<(), NeovimError> { + debug!( + "Waiting for LSP client readiness: {:?} with timeout: {}ms", + client_name, timeout_ms + ); + + // Wait for NVIM_MCP_LspAttach notification + let notification = self + .wait_for_notification("NVIM_MCP_LspAttach", timeout_ms) + .await?; + + // If specific client name is requested, verify it matches + if let Some(expected_client_name) = client_name { + // Parse the notification args to check client name + if let Some(attach_data) = notification.args.first() { + // Extract client_name from the nvim_rs::Value + if let Value::Map(map) = attach_data { + // Find the client_name key-value pair in the map + let client_name_key = Value::String("client_name".into()); + let client_name_value = map + .iter() + .find(|(k, _)| k == &client_name_key) + .map(|(_, v)| v); + + if let Some(Value::String(actual_client_name)) = client_name_value { + let actual_str = actual_client_name.as_str().unwrap_or(""); + if actual_str != expected_client_name { + return Err(NeovimError::Api(format!( + "LSP client '{}' attached but expected '{}'", + actual_str, expected_client_name + ))); + } + } else { + return Err(NeovimError::Api( + "LSP attach notification missing or invalid client_name".to_string(), + )); + } + } else { + return Err(NeovimError::Api( + "LSP attach notification data is not a map".to_string(), + )); + } + } else { + return Err(NeovimError::Api( + "LSP attach notification missing data".to_string(), + )); + } + } + + debug!("LSP client readiness confirmed"); + Ok(()) + } + + #[instrument(skip(self))] + async fn wait_for_diagnostics( + &self, + buffer_id: Option, + timeout_ms: u64, + ) -> Result, NeovimError> { + debug!( + "Waiting for diagnostics for buffer {:?} with timeout: {}ms", + buffer_id, timeout_ms + ); + + // First try to get diagnostics immediately + match self.get_diagnostics(buffer_id).await { + Ok(diagnostics) if !diagnostics.is_empty() => { + debug!("Found {} diagnostics immediately", diagnostics.len()); + return Ok(diagnostics); + } + Ok(_) => { + // No diagnostics found, wait for diagnostic notification + debug!("No diagnostics found, waiting for notification"); + } + Err(e) => { + debug!("Error getting diagnostics: {}, waiting for notification", e); + } + } + + // Wait for diagnostic notification + let notification = self + .wait_for_notification("NVIM_MCP_DiagnosticsChanged", timeout_ms) + .await?; + + debug!("Received diagnostics notification: {:?}", notification); + + // After notification, try to get diagnostics again + self.get_diagnostics(buffer_id).await + } } #[cfg(test)] @@ -2902,4 +3242,191 @@ mod tests { let deserialized = deserialized.workspace_edit; assert!(deserialized.changes.is_some()); } + + #[tokio::test] + async fn test_notification_tracker_basic() { + let tracker = NotificationTracker::new(); + + // Test recording a notification + tracker + .record_notification( + "test_notification".to_string(), + vec![Value::from("test_arg")], + ) + .await; + + // Test waiting for the notification + let result = tracker + .wait_for_notification("test_notification", Duration::from_millis(100)) + .await; + + assert!(result.is_ok()); + let notification = result.unwrap(); + assert_eq!(notification.name, "test_notification"); + assert_eq!(notification.args.len(), 1); + assert_eq!(notification.args[0].as_str().unwrap(), "test_arg"); + } + + #[tokio::test] + async fn test_notification_tracker_timeout() { + let tracker = NotificationTracker::new(); + + // Test waiting for a notification that never comes (should timeout) + let result = tracker + .wait_for_notification("nonexistent_notification", Duration::from_millis(50)) + .await; + + assert!(result.is_err()); + let error = result.unwrap_err(); + assert!(matches!(error, NeovimError::Api(_))); + assert!( + error + .to_string() + .contains("Timeout waiting for notification") + ); + } + + #[tokio::test] + async fn test_notification_tracker_wait_then_send() { + let tracker = NotificationTracker::new(); + + // Spawn a task that will wait for a notification + let wait_handle = tokio::spawn({ + let tracker = tracker.clone(); + async move { + tracker + .wait_for_notification("test_async_notification", Duration::from_millis(500)) + .await + } + }); + + // Give the waiting task a moment to start waiting + tokio::time::sleep(Duration::from_millis(10)).await; + + // Now send the notification + tracker + .record_notification( + "test_async_notification".to_string(), + vec![Value::from("async_test_arg")], + ) + .await; + + // The waiting task should now receive the notification + let result = wait_handle.await.unwrap(); + assert!(result.is_ok()); + let notification = result.unwrap(); + assert_eq!(notification.name, "test_async_notification"); + assert_eq!(notification.args.len(), 1); + assert_eq!(notification.args[0].as_str().unwrap(), "async_test_arg"); + } + + #[tokio::test] + async fn test_notification_cleanup_expired() { + let tracker = NotificationTracker::new(); + + // Record a notification with a modified timestamp (simulate old notification) + let old_notification = Notification { + name: "old_notification".to_string(), + args: vec![Value::from("old_data")], + timestamp: std::time::SystemTime::now() + - Duration::from_secs(NOTIFICATION_EXPIRY_SECONDS + 1), + }; + + // Manually insert old notification to simulate existing data + { + let mut notifications = tracker.notifications.lock().await; + notifications.push(old_notification); + } + + // Record a fresh notification + tracker + .record_notification( + "fresh_notification".to_string(), + vec![Value::from("fresh_data")], + ) + .await; + + // Check initial state + let (count_before, _) = tracker.get_stats().await; + assert_eq!(count_before, 2); + + // Trigger cleanup + tracker.cleanup_expired_notifications().await; + + // Old notification should be removed, fresh one should remain + let (count_after, _) = tracker.get_stats().await; + assert_eq!(count_after, 1); + + // Verify the remaining notification is the fresh one + let result = tracker + .wait_for_notification("fresh_notification", Duration::from_millis(10)) + .await; + assert!(result.is_ok()); + + // Verify old notification is gone + let result = tracker + .wait_for_notification("old_notification", Duration::from_millis(10)) + .await; + assert!(result.is_err()); + } + + #[tokio::test] + async fn test_notification_cleanup_excess() { + let tracker = NotificationTracker::new(); + + // Record more than MAX_STORED_NOTIFICATIONS + for i in 0..(MAX_STORED_NOTIFICATIONS + 10) { + tracker + .record_notification(format!("notification_{}", i), vec![Value::from(i as i64)]) + .await; + } + + // Get current count + let (count, _) = tracker.get_stats().await; + + // Should be limited to MAX_STORED_NOTIFICATIONS due to automatic cleanup + assert!(count <= MAX_STORED_NOTIFICATIONS); + + // The most recent notifications should still be available + let result = tracker + .wait_for_notification( + &format!("notification_{}", MAX_STORED_NOTIFICATIONS + 9), + Duration::from_millis(10), + ) + .await; + assert!(result.is_ok()); + } + + #[tokio::test] + async fn test_notification_expiry_in_wait() { + let tracker = NotificationTracker::new(); + + // Create an expired notification manually + let expired_notification = Notification { + name: "expired_test".to_string(), + args: vec![Value::from("expired_data")], + timestamp: std::time::SystemTime::now() + - Duration::from_secs(NOTIFICATION_EXPIRY_SECONDS + 1), + }; + + // Manually insert expired notification + { + let mut notifications = tracker.notifications.lock().await; + notifications.push(expired_notification); + } + + // wait_for_notification should not return expired notification + let result = tracker + .wait_for_notification("expired_test", Duration::from_millis(50)) + .await; + + // Should timeout because expired notification is ignored + assert!(result.is_err()); + assert!( + result + .unwrap_err() + .to_string() + .contains("Timeout waiting for notification") + ); + } } diff --git a/src/neovim/integration_tests.rs b/src/neovim/integration_tests.rs index 57cea07..b5539df 100644 --- a/src/neovim/integration_tests.rs +++ b/src/neovim/integration_tests.rs @@ -1,15 +1,43 @@ use std::fs; -use std::time::Duration; use tempfile::TempDir; -use tokio::time::sleep; use tracing::info; use tracing_test::traced_test; use crate::neovim::client::{DocumentIdentifier, Position, Range}; -use crate::neovim::{NeovimClient, NeovimClientTrait}; +use crate::neovim::{NeovimClient, NeovimClientTrait, NeovimError}; use crate::test_utils::*; +// Test helper functions to reduce boilerplate + +/// Helper function to wait for LSP analysis to complete (LSP ready + diagnostics) +async fn wait_for_lsp_analysis_complete( + client: &mut NeovimClient, + timeout_ms: u64, +) -> Result<(), NeovimError> { + client.wait_for_lsp_ready(None, timeout_ms).await?; + client.wait_for_diagnostics(None, timeout_ms).await?; + Ok(()) +} + +/// Helper for complete LSP setup including autocmd setup and waiting for analysis +async fn setup_lsp_with_analysis( + client: &mut NeovimClient, +) -> Result<(), NeovimError> { + client.setup_autocmd().await?; + wait_for_lsp_analysis_complete(client, 5000).await?; + Ok(()) +} + +/// Helper for LSP setup that only waits for LSP readiness (no diagnostics) +async fn setup_lsp_ready_only( + client: &mut NeovimClient, +) -> Result<(), NeovimError> { + client.setup_autocmd().await?; + client.wait_for_lsp_ready(None, 5000).await?; + Ok(()) +} + #[tokio::test] #[traced_test] async fn test_tcp_connection_lifecycle() { @@ -189,14 +217,10 @@ async fn test_get_vim_diagnostics() { let result = client.connect_path(&ipc_path).await; assert!(result.is_ok(), "Failed to connect to instance"); - // Set up diagnostics and get diagnostics for buffer 0 - let result = client.setup_diagnostics_changed_autocmd().await; - assert!( - result.is_ok(), - "Failed to setup diagnostics autocmd: {result:?}" - ); - - sleep(Duration::from_secs(20)).await; // Allow time for LSP to initialize + // Set up LSP and wait for analysis to complete + setup_lsp_with_analysis(&mut client) + .await + .expect("LSP setup and analysis should complete"); let result = client.get_buffer_diagnostics(0).await; assert!(result.is_ok(), "Failed to get diagnostics: {result:?}"); @@ -225,14 +249,10 @@ async fn test_code_action() { let result = client.connect_path(&ipc_path).await; assert!(result.is_ok(), "Failed to connect to instance"); - // Set up diagnostics and wait for LSP - let result = client.setup_diagnostics_changed_autocmd().await; - assert!( - result.is_ok(), - "Failed to setup diagnostics autocmd: {result:?}" - ); - - sleep(Duration::from_secs(20)).await; // Allow time for LSP to initialize + // Set up LSP and wait for analysis to complete + setup_lsp_with_analysis(&mut client) + .await + .expect("LSP setup and analysis should complete"); let result = client.get_buffer_diagnostics(0).await; assert!(result.is_ok(), "Failed to get diagnostics: {result:?}"); @@ -289,14 +309,10 @@ async fn test_lsp_resolve_code_action() { let result = client.connect_path(&ipc_path).await; assert!(result.is_ok(), "Failed to connect to instance"); - // Set up diagnostics and wait for LSP - let result = client.setup_diagnostics_changed_autocmd().await; - assert!( - result.is_ok(), - "Failed to setup diagnostics autocmd: {result:?}" - ); - - sleep(Duration::from_secs(20)).await; // Allow time for LSP to initialize + // Set up LSP and wait for readiness + setup_lsp_ready_only(&mut client) + .await + .expect("LSP setup should complete"); // Position cursor inside fmt.Println call (line 6, character 6) let result = client @@ -408,14 +424,10 @@ async fn test_lsp_apply_workspace_edit() { let result = client.connect_path(&ipc_path).await; assert!(result.is_ok(), "Failed to connect to instance"); - // Set up diagnostics and wait for LSP - let result = client.setup_diagnostics_changed_autocmd().await; - assert!( - result.is_ok(), - "Failed to setup diagnostics autocmd: {result:?}" - ); - - sleep(Duration::from_secs(20)).await; // Allow time for LSP to initialize + // Set up LSP and wait for analysis to complete + setup_lsp_with_analysis(&mut client) + .await + .expect("LSP setup and analysis should complete"); // Get buffer diagnostics to find modernization opportunities let result = client.get_buffer_diagnostics(0).await; @@ -472,8 +484,7 @@ async fn test_lsp_apply_workspace_edit() { let result = client.execute_lua("vim.cmd('write')").await; assert!(result.is_ok(), "Failed to save buffer: {result:?}"); - // Give some time for the edit and save to be applied - sleep(Duration::from_millis(1000)).await; + // File operations should be synchronous in Neovim // Read the modified content to verify the change let modified_content = @@ -544,14 +555,10 @@ func main() { let result = client.connect_path(&ipc_path).await; assert!(result.is_ok(), "Failed to connect to instance"); - // Set up diagnostics and wait for LSP - let result = client.setup_diagnostics_changed_autocmd().await; - assert!( - result.is_ok(), - "Failed to setup diagnostics autocmd: {result:?}" - ); - - sleep(Duration::from_secs(15)).await; // Allow time for LSP to initialize + // Set up LSP and wait for readiness + setup_lsp_ready_only(&mut client) + .await + .expect("LSP setup should complete"); // Get LSP clients let lsp_clients = client.lsp_get_clients().await.unwrap(); @@ -664,14 +671,10 @@ pub fn main() !void { let result = client.connect_path(&ipc_path).await; assert!(result.is_ok(), "Failed to connect to instance"); - // Set up diagnostics and wait for LSP - let result = client.setup_diagnostics_changed_autocmd().await; - assert!( - result.is_ok(), - "Failed to setup diagnostics autocmd: {result:?}" - ); - - sleep(Duration::from_secs(15)).await; // Allow time for LSP to initialize + // Set up LSP and wait for analysis to complete + setup_lsp_with_analysis(&mut client) + .await + .expect("LSP setup and analysis should complete"); // Get LSP clients let lsp_clients = client.lsp_get_clients().await.unwrap(); @@ -782,14 +785,10 @@ func main() { let result = client.connect_path(&ipc_path).await; assert!(result.is_ok(), "Failed to connect to instance"); - // Set up diagnostics and wait for LSP - let result = client.setup_diagnostics_changed_autocmd().await; - assert!( - result.is_ok(), - "Failed to setup diagnostics autocmd: {result:?}" - ); - - sleep(Duration::from_secs(15)).await; // Allow time for LSP to initialize + // Set up LSP and wait for readiness + setup_lsp_ready_only(&mut client) + .await + .expect("LSP setup should complete"); // Get LSP clients let lsp_clients = client.lsp_get_clients().await.unwrap(); @@ -906,14 +905,10 @@ func main() { let result = client.connect_path(&ipc_path).await; assert!(result.is_ok(), "Failed to connect to instance"); - // Set up diagnostics and wait for LSP - let result = client.setup_diagnostics_changed_autocmd().await; - assert!( - result.is_ok(), - "Failed to setup diagnostics autocmd: {result:?}" - ); - - sleep(Duration::from_secs(15)).await; // Allow time for LSP to initialize + // Set up LSP and wait for readiness + setup_lsp_ready_only(&mut client) + .await + .expect("LSP setup should complete"); // Get LSP clients let lsp_clients = client.lsp_get_clients().await.unwrap(); @@ -1008,14 +1003,10 @@ async fn test_lsp_rename_with_prepare() { let result = client.connect_path(&ipc_path).await; assert!(result.is_ok(), "Failed to connect to instance"); - // Set up diagnostics and wait for LSP - let result = client.setup_diagnostics_changed_autocmd().await; - assert!( - result.is_ok(), - "Failed to setup diagnostics autocmd: {result:?}" - ); - - sleep(Duration::from_secs(20)).await; // Allow time for LSP to initialize + // Set up LSP and wait for readiness + setup_lsp_ready_only(&mut client) + .await + .expect("LSP setup should complete"); // Get LSP clients let lsp_clients = client.lsp_get_clients().await.unwrap(); @@ -1061,8 +1052,7 @@ async fn test_lsp_rename_with_prepare() { let result = client.execute_lua("vim.cmd('write')").await; assert!(result.is_ok(), "Failed to save buffer: {result:?}"); - // Give some time for the save operation to complete - sleep(Duration::from_millis(1000)).await; + // File operations should be synchronous in Neovim // Read the file content to verify the rename was applied let updated_content = @@ -1111,14 +1101,10 @@ async fn test_lsp_rename_without_prepare() { let result = client.connect_path(&ipc_path).await; assert!(result.is_ok(), "Failed to connect to instance"); - // Set up diagnostics and wait for LSP - let result = client.setup_diagnostics_changed_autocmd().await; - assert!( - result.is_ok(), - "Failed to setup diagnostics autocmd: {result:?}" - ); - - sleep(Duration::from_secs(20)).await; // Allow time for LSP to initialize + // Set up LSP and wait for readiness + setup_lsp_ready_only(&mut client) + .await + .expect("LSP setup should complete"); // Get LSP clients let lsp_clients = client.lsp_get_clients().await.unwrap(); @@ -1164,8 +1150,7 @@ async fn test_lsp_rename_without_prepare() { let result = client.execute_lua("vim.cmd('write')").await; assert!(result.is_ok(), "Failed to save buffer: {result:?}"); - // Give some time for the save operation to complete - sleep(Duration::from_millis(1000)).await; + // File operations should be synchronous in Neovim // Read the file content to verify the rename was applied let updated_content = @@ -1244,14 +1229,10 @@ main(); let result = client.connect_path(&ipc_path).await; assert!(result.is_ok(), "Failed to connect to instance"); - // Set up diagnostics and wait for LSP - let result = client.setup_diagnostics_changed_autocmd().await; - assert!( - result.is_ok(), - "Failed to setup diagnostics autocmd: {result:?}" - ); - - sleep(Duration::from_secs(20)).await; // Allow time for LSP to initialize + // Set up LSP and wait for analysis to complete + setup_lsp_with_analysis(&mut client) + .await + .expect("LSP setup and analysis should complete"); (temp_dir, guard, client) } @@ -1341,14 +1322,10 @@ main(); let result = client.connect_path(&ipc_path).await; assert!(result.is_ok(), "Failed to connect to instance"); - // Set up diagnostics and wait for LSP - let result = client.setup_diagnostics_changed_autocmd().await; - assert!( - result.is_ok(), - "Failed to setup diagnostics autocmd: {result:?}" - ); - - sleep(Duration::from_secs(20)).await; // Allow time for LSP to initialize + // Set up LSP and wait for analysis to complete + setup_lsp_with_analysis(&mut client) + .await + .expect("LSP setup and analysis should complete"); use crate::neovim::FormattingOptions; let tab_options = FormattingOptions { diff --git a/src/neovim/lua/diagnostics_autocmd.lua b/src/neovim/lua/diagnostics_autocmd.lua deleted file mode 100644 index a6c2345..0000000 --- a/src/neovim/lua/diagnostics_autocmd.lua +++ /dev/null @@ -1,17 +0,0 @@ -local group = vim.api.nvim_create_augroup("NVIM_MCP_DiagnosticsChanged", { clear = true }) -vim.api.nvim_create_autocmd("DiagnosticChanged", { - group = group, - callback = function(args) - vim.rpcnotify(0, "NVIM_MCP_DiagnosticsChanged", { - buf = args.buf, - diagnostics = args.data.diagnostics, - }) - end, -}) -vim.api.nvim_create_autocmd("LspAttach", { - group = group, - callback = function(args) - vim.rpcnotify(0, "NVIM_MCP_LspAttach", args.data.diagnostics) - end, -}) -vim.rpcnotify(0, "NVIM_MCP", "setup diagnostics changed autocmd") diff --git a/src/neovim/lua/setup_autocmd.lua b/src/neovim/lua/setup_autocmd.lua new file mode 100644 index 0000000..39d20b1 --- /dev/null +++ b/src/neovim/lua/setup_autocmd.lua @@ -0,0 +1,49 @@ +local group = vim.api.nvim_create_augroup("NVIM_MCP_DiagnosticsChanged", { clear = true }) + +vim.api.nvim_create_autocmd("DiagnosticChanged", { + group = group, + callback = function(args) + vim.rpcnotify(0, "NVIM_MCP_DiagnosticsChanged", { + buf = args.buf, + diagnostics = args.data.diagnostics, + }) + end, +}) + +vim.api.nvim_create_autocmd("LspAttach", { + group = group, + callback = function(args) + local client_id = args.data.client_id + local client = vim.lsp.get_client_by_id(client_id) + + if client then + vim.rpcnotify(0, "NVIM_MCP_LspAttach", { + client_name = client.name, + client_id = client_id, + buffer_id = args.buf, + server_capabilities = client.server_capabilities, + initialized = client.initialized, + attach_time = vim.uv.now(), + }) + end + end, +}) + +vim.api.nvim_create_autocmd("LspDetach", { + group = group, + callback = function(args) + local client_id = args.data.client_id + local client = vim.lsp.get_client_by_id(client_id) + + if client then + vim.rpcnotify(0, "NVIM_MCP_LspDetach", { + client_name = client.name, + client_id = client_id, + buffer_id = args.buf, + detach_time = vim.uv.now(), + }) + end + end, +}) + +vim.rpcnotify(0, "NVIM_MCP", "setup diagnostics changed autocmd") diff --git a/src/server/core.rs b/src/server/core.rs index 6c64714..8f8526c 100644 --- a/src/server/core.rs +++ b/src/server/core.rs @@ -240,7 +240,7 @@ pub async fn auto_connect_single_target( // Import NeovimClient here to avoid circular imports let mut client = crate::neovim::NeovimClient::new(); client.connect_path(target).await?; - client.setup_diagnostics_changed_autocmd().await?; + client.setup_autocmd().await?; server .nvim_clients diff --git a/src/server/integration_tests.rs b/src/server/integration_tests.rs index 81e9036..92c4f12 100644 --- a/src/server/integration_tests.rs +++ b/src/server/integration_tests.rs @@ -1,17 +1,72 @@ -use std::time::Duration; +use std::path::PathBuf; +use std::sync::OnceLock; use rmcp::{ model::{CallToolRequestParam, ReadResourceRequestParam}, serde_json::{Map, Value}, service::ServiceExt, - transport::{ConfigureCommandExt, TokioChildProcess}, + transport::TokioChildProcess, }; -use tokio::{process::Command, time}; +use tokio::process::Command; use tracing::{error, info}; use tracing_test::traced_test; use crate::test_utils::*; +static BINARY_PATH: OnceLock = OnceLock::new(); + +/// Get the compiled binary path, compiling only once +fn get_compiled_binary() -> PathBuf { + BINARY_PATH + .get_or_init(|| { + info!("Compiling nvim-mcp binary (one-time compilation)..."); + + // Build the binary using cargo build + let output = std::process::Command::new("cargo") + .args(["build", "--bin", "nvim-mcp"]) + .output() + .expect("Failed to execute cargo build"); + + if !output.status.success() { + let stderr = String::from_utf8_lossy(&output.stderr); + panic!("Failed to compile nvim-mcp binary: {}", stderr); + } + + // Determine the binary path + let manifest_dir = + std::env::var("CARGO_MANIFEST_DIR").expect("CARGO_MANIFEST_DIR not set"); + let mut binary_path = PathBuf::from(manifest_dir); + binary_path.push("target"); + binary_path.push("debug"); + binary_path.push("nvim-mcp"); + + // On Windows, add .exe extension + #[cfg(windows)] + binary_path.set_extension("exe"); + + if !binary_path.exists() { + panic!("Binary not found at expected path: {:?}", binary_path); + } + + info!("Binary compiled successfully at: {:?}", binary_path); + binary_path + }) + .clone() +} + +/// Macro to create an MCP service using the pre-compiled binary +macro_rules! create_mcp_service { + () => {{ + let binary_path = get_compiled_binary(); + ().serve(TokioChildProcess::new(Command::new(binary_path))?) + .await + .map_err(|e| { + error!("Failed to connect to server: {}", e); + e + })? + }}; +} + // Helper function to extract connection_id from connect response fn extract_connection_id( result: &rmcp::model::CallToolResult, @@ -37,18 +92,8 @@ fn extract_connection_id( async fn test_mcp_server_connection() -> Result<(), Box> { info!("Starting MCP client to test nvim-mcp server"); - // Connect to the server running as a child process (exact copy from original) - let service = () - .serve(TokioChildProcess::new(Command::new("cargo").configure( - |cmd| { - cmd.args(["run", "--bin", "nvim-mcp"]); - }, - ))?) - .await - .map_err(|e| { - error!("Failed to connect to server: {}", e); - e - })?; + // Connect to the server using pre-compiled binary + let service = create_mcp_service!(); // Get server information let server_info = service.peer_info(); @@ -79,17 +124,7 @@ async fn test_mcp_server_connection() -> Result<(), Box> async fn test_list_tools() -> Result<(), Box> { info!("Starting MCP client to test nvim-mcp server"); - let service = () - .serve(TokioChildProcess::new(Command::new("cargo").configure( - |cmd| { - cmd.args(["run", "--bin", "nvim-mcp"]); - }, - ))?) - .await - .map_err(|e| { - error!("Failed to connect to server: {}", e); - e - })?; + let service = create_mcp_service!(); // List available tools let tools = service.list_tools(Default::default()).await?; @@ -121,17 +156,7 @@ async fn test_list_tools() -> Result<(), Box> { async fn test_connect_nvim_tcp_tool() -> Result<(), Box> { info!("Starting MCP client to test nvim-mcp server"); - let service = () - .serve(TokioChildProcess::new(Command::new("cargo").configure( - |cmd| { - cmd.args(["run", "--bin", "nvim-mcp"]); - }, - ))?) - .await - .map_err(|e| { - error!("Failed to connect to server: {}", e); - e - })?; + let service = create_mcp_service!(); // Start a test Neovim instance let ipc_path = generate_random_ipc_path(); @@ -193,17 +218,7 @@ async fn test_connect_nvim_tcp_tool() -> Result<(), Box> async fn test_disconnect_nvim_tcp_tool() -> Result<(), Box> { info!("Starting MCP client to test nvim-mcp server"); - let service = () - .serve(TokioChildProcess::new(Command::new("cargo").configure( - |cmd| { - cmd.args(["run", "--bin", "nvim-mcp"]); - }, - ))?) - .await - .map_err(|e| { - error!("Failed to connect to server: {}", e); - e - })?; + let service = create_mcp_service!(); // Test disconnect without valid connection (should fail) let mut invalid_disconnect_args = Map::new(); @@ -298,17 +313,7 @@ async fn test_disconnect_nvim_tcp_tool() -> Result<(), Box Result<(), Box> { info!("Starting MCP client to test nvim-mcp server"); - let service = () - .serve(TokioChildProcess::new(Command::new("cargo").configure( - |cmd| { - cmd.args(["run", "--bin", "nvim-mcp"]); - }, - ))?) - .await - .map_err(|e| { - error!("Failed to connect to server: {}", e); - e - })?; + let service = create_mcp_service!(); // Test list buffers without connection (should fail) let mut invalid_args = Map::new(); @@ -388,17 +393,7 @@ async fn test_list_buffers_tool() -> Result<(), Box> { async fn test_complete_workflow() -> Result<(), Box> { info!("Starting MCP client to test nvim-mcp server"); - let service = () - .serve(TokioChildProcess::new(Command::new("cargo").configure( - |cmd| { - cmd.args(["run", "--bin", "nvim-mcp"]); - }, - ))?) - .await - .map_err(|e| { - error!("Failed to connect to server: {}", e); - e - })?; + let service = create_mcp_service!(); // Start Neovim instance let ipc_path = generate_random_ipc_path(); @@ -507,17 +502,7 @@ async fn test_complete_workflow() -> Result<(), Box> { async fn test_error_handling() -> Result<(), Box> { info!("Starting MCP client to test nvim-mcp server"); - let service = () - .serve(TokioChildProcess::new(Command::new("cargo").configure( - |cmd| { - cmd.args(["run", "--bin", "nvim-mcp"]); - }, - ))?) - .await - .map_err(|e| { - error!("Failed to connect to server: {}", e); - e - })?; + let service = create_mcp_service!(); // Test connecting to invalid address let mut invalid_args = Map::new(); @@ -569,17 +554,7 @@ async fn test_error_handling() -> Result<(), Box> { async fn test_exec_lua_tool() -> Result<(), Box> { info!("Starting MCP client to test nvim-mcp server"); - let service = () - .serve(TokioChildProcess::new(Command::new("cargo").configure( - |cmd| { - cmd.args(["run", "--bin", "nvim-mcp"]); - }, - ))?) - .await - .map_err(|e| { - error!("Failed to connect to server: {}", e); - e - })?; + let service = create_mcp_service!(); // Test exec_lua without connection (should fail) let mut lua_args = Map::new(); @@ -729,17 +704,7 @@ async fn test_exec_lua_tool() -> Result<(), Box> { async fn test_lsp_clients_tool() -> Result<(), Box> { info!("Starting MCP client to test nvim-mcp server"); - let service = () - .serve(TokioChildProcess::new(Command::new("cargo").configure( - |cmd| { - cmd.args(["run", "--bin", "nvim-mcp"]); - }, - ))?) - .await - .map_err(|e| { - error!("Failed to connect to server: {}", e); - e - })?; + let service = create_mcp_service!(); // Test lsp_clients without connection (should fail) let mut invalid_args = Map::new(); @@ -811,17 +776,7 @@ async fn test_lsp_clients_tool() -> Result<(), Box> { async fn test_list_diagnostic_resources() -> Result<(), Box> { info!("Starting MCP client to test diagnostic resources"); - let service = () - .serve(TokioChildProcess::new(Command::new("cargo").configure( - |cmd| { - cmd.args(["run", "--bin", "nvim-mcp"]); - }, - ))?) - .await - .map_err(|e| { - error!("Failed to connect to server: {}", e); - e - })?; + let service = create_mcp_service!(); // Test list_resources let result = service.list_resources(None).await?; @@ -857,17 +812,7 @@ async fn test_list_diagnostic_resources() -> Result<(), Box Result<(), Box> { info!("Starting MCP client to test reading workspace diagnostics"); - let service = () - .serve(TokioChildProcess::new(Command::new("cargo").configure( - |cmd| { - cmd.args(["run", "--bin", "nvim-mcp"]); - }, - ))?) - .await - .map_err(|e| { - error!("Failed to connect to server: {}", e); - e - })?; + let service = create_mcp_service!(); // Start Neovim instance let ipc_path = generate_random_ipc_path(); @@ -945,26 +890,17 @@ async fn test_read_workspace_diagnostics() -> Result<(), Box Result<(), Box> { info!("Testing lsp_organize_imports with non-existent file"); - let service = () - .serve(TokioChildProcess::new(Command::new("cargo").configure( - |cmd| { - cmd.args(["run", "--bin", "nvim-mcp"]); - }, - ))?) - .await - .map_err(|e| { - error!("Failed to connect to server: {}", e); - e - })?; + let service = create_mcp_service!(); // Start a test Neovim instance let ipc_path = generate_random_ipc_path(); - let _guard = setup_neovim_instance_ipc_advance( + let child = setup_neovim_instance_ipc_advance( &ipc_path, get_testdata_path("cfg_lsp.lua").to_str().unwrap(), get_testdata_path("organize_imports.go").to_str().unwrap(), ) .await; + let _guard = NeovimIpcGuard::new(child, ipc_path.clone()); // Establish connection let connection_id = { @@ -1022,28 +958,17 @@ async fn test_lsp_organize_imports_non_existent_file() -> Result<(), Box Result<(), Box> { info!("Testing lsp_organize_imports with LSP setup"); - let service = () - .serve(TokioChildProcess::new(Command::new("cargo").configure( - |cmd| { - cmd.args(["run", "--bin", "nvim-mcp"]); - }, - ))?) - .await - .map_err(|e| { - error!("Failed to connect to server: {}", e); - e - })?; + let service = create_mcp_service!(); // Start a test Neovim instance with LSP let ipc_path = generate_random_ipc_path(); - let _guard = setup_neovim_instance_ipc_advance( + let child = setup_neovim_instance_ipc_advance( &ipc_path, get_testdata_path("cfg_lsp.lua").to_str().unwrap(), get_testdata_path("main.go").to_str().unwrap(), ) .await; - - time::sleep(Duration::from_secs(1)).await; // Ensure LSP is ready + let _guard = NeovimIpcGuard::new(child, ipc_path.clone()); // Establish connection let connection_id = { @@ -1061,6 +986,27 @@ async fn test_lsp_organize_imports_with_lsp() -> Result<(), Box Result<(), Box Result<(), Box> { info!("Testing lsp_organize_imports in inspect mode (apply_edits=false)"); - let service = () - .serve(TokioChildProcess::new(Command::new("cargo").configure( - |cmd| { - cmd.args(["run", "--bin", "nvim-mcp"]); - }, - ))?) - .await - .map_err(|e| { - error!("Failed to connect to server: {}", e); - e - })?; + let service = create_mcp_service!(); // Start a test Neovim instance with LSP let ipc_path = generate_random_ipc_path(); - let _guard = setup_neovim_instance_ipc_advance( + let child = setup_neovim_instance_ipc_advance( &ipc_path, get_testdata_path("cfg_lsp.lua").to_str().unwrap(), get_testdata_path("organize_imports.go").to_str().unwrap(), ) .await; - - time::sleep(Duration::from_secs(1)).await; // Ensure LSP is ready + let _guard = NeovimIpcGuard::new(child, ipc_path.clone()); // Establish connection let connection_id = { @@ -1148,6 +1083,27 @@ async fn test_lsp_organize_imports_inspect_mode() -> Result<(), Box Result<(), Box Result<(), Box> { info!("Testing end-to-end Lua tools workflow"); - // Start MCP server with child process - let service = () - .serve(TokioChildProcess::new(Command::new("cargo").configure( - |cmd| { - cmd.args(["run", "--bin", "nvim-mcp"]); - }, - ))?) - .await - .map_err(|e| { - error!("Failed to connect to server: {}", e); - e - })?; + // Connect to pre-compiled MCP server + let service = create_mcp_service!(); info!("Connected to server"); - let connection_id = { - info!("starting IPC Neovim for testing"); + info!("starting IPC Neovim for testing"); - let test_ipc_path = generate_random_socket_path(); - let cfg_path = "src/testdata/cfg_lsp.lua"; - let open_file = "src/testdata/main.go"; + let ipc_path = generate_random_socket_path(); + let cfg_path = "src/testdata/cfg_lsp.lua"; + let open_file = "src/testdata/main.go"; - let _neovim_child = crate::test_utils::setup_neovim_instance_ipc_advance( - &test_ipc_path, - cfg_path, - open_file, - ) - .await; + let child = + crate::test_utils::setup_neovim_instance_ipc_advance(&ipc_path, cfg_path, open_file).await; + let _guard = NeovimIpcGuard::new(child, ipc_path.clone()); - // Wait for Neovim to be ready - time::sleep(Duration::from_millis(2000)).await; + // Neovim should be ready for connection immediately after process start - let mut connect_args = Map::new(); - connect_args.insert("target".to_string(), Value::String(test_ipc_path)); + let mut connect_args = Map::new(); + connect_args.insert("target".to_string(), Value::String(ipc_path)); - let result = service - .call_tool(CallToolRequestParam { - name: "connect".into(), - arguments: Some(connect_args), - }) - .await?; + let result = service + .call_tool(CallToolRequestParam { + name: "connect".into(), + arguments: Some(connect_args), + }) + .await?; - // Setup Lua tools configuration in Neovim - extract_connection_id(&result)? - }; + // Setup Lua tools configuration in Neovim + let connection_id = extract_connection_id(&result)?; // Test tool discovery by listing tools (should include our custom tool) let tools_result = service.list_tools(Default::default()).await?; diff --git a/src/server/tools.rs b/src/server/tools.rs index a2b4e8a..6739161 100644 --- a/src/server/tools.rs +++ b/src/server/tools.rs @@ -47,6 +47,23 @@ pub struct ExecuteLuaRequest { pub code: String, } +/// Wait for LSP client readiness parameters +#[derive(Debug, serde::Deserialize, schemars::JsonSchema)] +pub struct WaitForLspReadyRequest { + /// Unique identifier for the target Neovim instance + pub connection_id: String, + /// Optional specific LSP client name to wait for (waits for any client if None) + #[serde(skip_serializing_if = "Option::is_none")] + pub client_name: Option, + /// Timeout in milliseconds (default 5000ms) + #[serde(default = "default_timeout")] + pub timeout_ms: u64, +} + +fn default_timeout() -> u64 { + 5000 +} + /// Workspace symbols parameters #[derive(Debug, serde::Deserialize, schemars::JsonSchema)] pub struct WorkspaceSymbolsParams { @@ -358,7 +375,7 @@ impl NeovimMcpServer { let mut client = NeovimClient::new(); client.connect_path(&path).await?; - client.setup_diagnostics_changed_autocmd().await?; + client.setup_autocmd().await?; // Discover and register Lua tools for this connection if let Err(e) = @@ -410,7 +427,7 @@ impl NeovimMcpServer { let mut client = NeovimClient::new(); client.connect_tcp(&address).await?; - client.setup_diagnostics_changed_autocmd().await?; + client.setup_autocmd().await?; // Discover and register Lua tools for this connection if let Err(e) = @@ -510,6 +527,29 @@ impl NeovimMcpServer { )?])) } + #[tool(description = "Wait for LSP client to be ready and attached")] + #[instrument(skip(self))] + pub async fn wait_for_lsp_ready( + &self, + Parameters(WaitForLspReadyRequest { + connection_id, + client_name, + timeout_ms, + }): Parameters, + ) -> Result { + let client = self.get_connection(&connection_id)?; + client + .wait_for_lsp_ready(client_name.as_deref(), timeout_ms) + .await?; + Ok(CallToolResult::success(vec![Content::json( + serde_json::json!({ + "message": "LSP client ready", + "client_name": client_name.unwrap_or_else(|| "any".to_string()), + "timeout_ms": timeout_ms + }), + )?])) + } + #[tool(description = "Get buffer's diagnostics")] #[instrument(skip(self))] pub async fn buffer_diagnostics( diff --git a/src/test_utils.rs b/src/test_utils.rs index 497a4b6..dfb7f35 100644 --- a/src/test_utils.rs +++ b/src/test_utils.rs @@ -183,7 +183,7 @@ pub async fn setup_neovim_instance_advance( // Wait for Neovim to start and create the TCP socket let start = Instant::now(); loop { - sleep(Duration::from_millis(100)).await; + sleep(Duration::from_millis(50)).await; // Try to connect to see if Neovim is ready if tokio::net::TcpStream::connect(&listen).await.is_ok() { @@ -224,7 +224,7 @@ pub async fn setup_neovim_instance_socket_advance( // Wait for Neovim to start and create the Unix socket let start = Instant::now(); loop { - sleep(Duration::from_millis(100)).await; + sleep(Duration::from_millis(50)).await; // Try to connect to see if Neovim is ready if UnixStream::connect(socket_path).await.is_ok() { @@ -266,7 +266,7 @@ pub async fn setup_neovim_instance_pipe_advance( // Wait for Neovim to start and create the named pipe let start = Instant::now(); loop { - sleep(Duration::from_millis(100)).await; + sleep(Duration::from_millis(50)).await; // Try to connect to see if Neovim is ready if NamedPipeClient::connect(pipe_path).await.is_ok() { @@ -334,7 +334,7 @@ pub async fn setup_test_neovim_instance( // Wait for Neovim to start and create the socket/pipe let start = Instant::now(); loop { - sleep(Duration::from_millis(100)).await; + sleep(Duration::from_millis(50)).await; // Try to connect to see if Neovim is ready #[cfg(unix)]