diff --git a/CLAUDE.md b/CLAUDE.md index 9cd99fb..faeab1d 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -105,7 +105,7 @@ docker logs 1. **Modular Trait-Based Design**: Each major component is defined by a trait (EmbeddingProvider, VectorDatabase) with concrete implementations, enabling easy swapping of backends. -2. **MCP Protocol Integration**: Uses `rmcp` macros (`#[tool]`, `#[prompt]`, `#[tool_router]`, `#[prompt_router]`) to define 9 MCP tools and 9 slash commands. The server communicates over stdio following MCP spec. +2. **MCP Protocol Integration**: Uses `rmcp` macros (`#[tool]`, `#[prompt]`, `#[tool_router]`, `#[prompt_router]`) to define 13 MCP tools and 12 slash commands. The server communicates over stdio following MCP spec. 3. **Async-First Architecture**: Built on Tokio runtime with async traits. File walking runs on blocking threads via `tokio::task::spawn_blocking` to avoid blocking the async runtime. @@ -117,7 +117,7 @@ docker logs ``` src/ -├── mcp_server.rs # Main MCP server with 9 tools + 9 prompts +├── mcp_server.rs # Main MCP server with 13 tools + 12 prompts │ ├── RagMcpServer # Server state (embedding provider, vector DB, chunker, hash cache) │ ├── Tool handlers # index_codebase (smart), query_codebase, find_definition, etc. │ └── Prompt handlers # Slash commands for each tool @@ -142,10 +142,12 @@ src/ │ ├── repomap/ # AST-based symbol extraction (fallback provider) │ │ ├── mod.rs # RepoMapProvider implementing RelationsProvider │ │ ├── symbol_extractor.rs # Extract definitions from AST nodes +│ │ ├── import_extractor.rs # Extract import/use/include bindings (SymbolKind::Import) │ │ └── reference_finder.rs # Find references via identifier matching │ ├── storage/ # Relations storage layer │ │ ├── mod.rs # RelationsStore trait -│ │ └── lance_store.rs # LanceDB storage for definitions/references +│ │ └── lance_store/ # LanceDB storage (definitions + references tables, +│ │ # populated with definitions during indexing) │ └── stack_graphs/ # Optional: High-precision name resolution (feature-gated) │ └── mod.rs # StackGraphsProvider for Python, TypeScript, Java, Ruby ├── bm25_search.rs # Tantivy BM25 keyword search with RRF fusion @@ -220,6 +222,8 @@ src/ - **Hybrid Architecture**: Uses stack-graphs (high precision, ~95%) for Python, TypeScript, Java, Ruby when feature enabled; RepoMap fallback (~70%) for all other languages - **RelationsProvider Trait**: Abstraction for extracting definitions and references from source files - **SymbolExtractor**: Uses tree-sitter AST to extract function, class, method, struct definitions +- **ImportExtractor**: Extracts import/use/include statements as `SymbolKind::Import` definitions, one per bound name (`use a::{B, C as D}` yields `B` and `D`); glob/side-effect imports are reported as skipped definitions +- **LanceRelationsStore**: Definitions are persisted to LanceDB tables (`relations_definitions`, `relations_references`) during indexing; idempotent per file, cleaned up on file removal and clear_index - **ReferenceFinder**: Text-based identifier matching with context analysis (call, read, write, import, etc.) - **PrecisionLevel**: High (stack-graphs), Medium (AST-based RepoMap), Low (text-based) - **Symbol Types**: Function, Method, Class, Struct, Interface, Trait, Enum, Module, Variable, Constant, etc. @@ -286,11 +290,12 @@ When adding new tools: All tools return JSON responses conforming to types defined in `types.rs`. Key response types: - `IndexResponse`: files_indexed, chunks_created, embeddings_generated, duration_ms, errors, mode (full or incremental) - `QueryResponse`: results (SearchResult[] with vector_score, keyword_score, combined_score), duration_ms -- `StatisticsResponse`: total_files, total_chunks, language_breakdown +- `StatisticsResponse`: total_files, total_chunks, language_breakdown, total_definitions, total_references, files_with_definitions - `ClearResponse`: success, message - `FindDefinitionResponse`: definitions (DefinitionResult[] with file_path, line, symbol info), precision_level - `FindReferencesResponse`: references (ReferenceResult[] with file_path, line, reference_kind), precision_level - `GetCallGraphResponse`: node (CallGraphNode with callers/callees), precision_level +- `FindUnusedResponse`: candidates (UnusedCandidate[] with confidence high/medium/low and reason), unverifiable_imports, probes_exhausted, truncated ### Prompt (Slash Command) Pattern Prompts in `#[prompt_router]` expand to user messages that instruct the AI to call the corresponding tool. Example: @@ -302,7 +307,7 @@ async fn index_prompt(&self, Parameters(args): Parameters) ### Server Capabilities Defined in `ServerHandler::get_info()`: -- Tools: Enabled (9 tools available): +- Tools: Enabled (13 tools available): - `index_codebase` - Index a codebase with smart full/incremental detection - `query_codebase` - Semantic search across indexed code - `get_statistics` - Get index statistics @@ -312,7 +317,11 @@ Defined in `ServerHandler::get_info()`: - `find_definition` - Find where a symbol is defined (LSP-like) - `find_references` - Find all references to a symbol (LSP-like) - `get_call_graph` - Get callers/callees for a function -- Prompts: Enabled (9 slash commands: /project:index, /project:query, /project:stats, /project:clear, /project:search, /project:git-search, /project:definition, /project:references, /project:callgraph) + - `list_symbols` - List every symbol defined in one file + - `read_file` - Read a slice (or all) of a file inside an indexed project root + - `edit_file` - Replace a line range (or the whole file), then auto-reindex the affected root + - `find_unused` - Find unused imports and dead-code candidates (report-only, confidence-rated) +- Prompts: Enabled (12 slash commands: /project:index, /project:query, /project:stats, /project:clear, /project:search, /project:git-search, /project:definition, /project:references, /project:callgraph, /project:read, /project:edit, /project:unused) - Resources: Not implemented - Sampling: Not implemented diff --git a/benches/indexing_benchmark.rs b/benches/indexing_benchmark.rs index c7d485d..4c4c37c 100644 --- a/benches/indexing_benchmark.rs +++ b/benches/indexing_benchmark.rs @@ -100,6 +100,7 @@ fn benchmark_indexing(c: &mut Criterion) { 1_048_576, None, None, + None, ) .await .unwrap() diff --git a/examples/basic_indexing.rs b/examples/basic_indexing.rs index cbd46a1..ecf6846 100644 --- a/examples/basic_indexing.rs +++ b/examples/basic_indexing.rs @@ -16,9 +16,7 @@ async fn main() -> anyhow::Result<()> { tracing_subscriber::fmt::init(); // Get codebase path from command line or use current directory - let codebase_path = env::args() - .nth(1) - .unwrap_or_else(|| ".".to_string()); + let codebase_path = env::args().nth(1).unwrap_or_else(|| ".".to_string()); println!("=== Project RAG - Basic Indexing Example ===\n"); diff --git a/examples/mcp_client.rs b/examples/mcp_client.rs index 6f106ef..28a81d6 100644 --- a/examples/mcp_client.rs +++ b/examples/mcp_client.rs @@ -11,7 +11,7 @@ //! Run with: cargo run --example mcp_client //! (The server will wait for MCP protocol messages on stdin) -use project_rag::{mcp_server::RagMcpServer, Config, RagClient}; +use project_rag::{Config, RagClient, mcp_server::RagMcpServer}; use std::sync::Arc; #[tokio::main] diff --git a/src/bm25_search/mod.rs b/src/bm25_search/mod.rs index d8d71ff..74e7964 100644 --- a/src/bm25_search/mod.rs +++ b/src/bm25_search/mod.rs @@ -10,7 +10,7 @@ use tantivy::{Index, IndexWriter, ReloadPolicy, TantivyDocument, doc}; /// BM25-based keyword search using Tantivy pub struct BM25Search { index: Index, - id_field: Field, + chunk_id_field: Field, content_field: Field, file_path_field: Field, /// Path to the index directory (needed for lock cleanup) @@ -20,9 +20,13 @@ pub struct BM25Search { } /// Search result from BM25 +/// +/// `chunk_id` is the chunk's stable `file_path:start_line` identifier -- the same value +/// stored in the vector table's `id` column. Both retrieval arms must key on it, or +/// Reciprocal Rank Fusion silently fuses nothing. See the note on `add_documents`. #[derive(Debug, Clone)] pub struct BM25Result { - pub id: u64, + pub chunk_id: String, pub score: f32, } @@ -31,26 +35,58 @@ impl BM25Search { pub fn new>(index_path: P) -> Result { let index_path = index_path.as_ref().to_path_buf(); - // Create schema with ID, content, and file_path fields - let mut schema_builder = Schema::builder(); - let id_field = schema_builder.add_u64_field("id", STORED | INDEXED); - let content_field = schema_builder.add_text_field("content", TEXT); - let file_path_field = schema_builder.add_text_field("file_path", STRING | STORED); - let schema = schema_builder.build(); + // Schema keyed by the chunk's stable `file_path:start_line` id. + let build_schema = || { + let mut b = Schema::builder(); + b.add_text_field("chunk_id", STRING | STORED); + b.add_text_field("content", TEXT); + b.add_text_field("file_path", STRING | STORED); + b.build() + }; // Create or open index std::fs::create_dir_all(&index_path).context("Failed to create BM25 index directory")?; - let index = if index_path.join("meta.json").exists() { + let mut index = if index_path.join("meta.json").exists() { Index::open_in_dir(&index_path).context("Failed to open existing BM25 index")? } else { - Index::create_in_dir(&index_path, schema.clone()) + Index::create_in_dir(&index_path, build_schema()) .context("Failed to create BM25 index")? }; + // Older builds keyed documents by a u64 `id` holding a table row number. Field + // handles are positional, so opening one of those directories with the new schema + // would read a u64 field as text and corrupt every lookup. Detect and rebuild -- + // the BM25 index is a derived artifact, so throwing it away costs only a re-index. + if index.schema().get_field("chunk_id").is_err() { + tracing::warn!( + "BM25 index at {:?} predates chunk_id keying; rebuilding it", + index_path + ); + drop(index); + std::fs::remove_dir_all(&index_path) + .context("Failed to remove stale BM25 index directory")?; + std::fs::create_dir_all(&index_path) + .context("Failed to recreate BM25 index directory")?; + index = Index::create_in_dir(&index_path, build_schema()) + .context("Failed to recreate BM25 index")?; + } + + // Take handles from the schema actually on disk, never from a locally built one. + let schema = index.schema(); + let chunk_id_field = schema + .get_field("chunk_id") + .context("BM25 schema is missing the chunk_id field")?; + let content_field = schema + .get_field("content") + .context("BM25 schema is missing the content field")?; + let file_path_field = schema + .get_field("file_path") + .context("BM25 schema is missing the file_path field")?; + Ok(Self { index, - id_field, + chunk_id_field, content_field, file_path_field, index_path, @@ -111,8 +147,14 @@ impl BM25Search { /// Add documents to the index /// /// Arguments: - /// * `documents` - Vec of (id, content, file_path) tuples - pub fn add_documents(&self, documents: Vec<(u64, String, String)>) -> Result<()> { + /// * `documents` - Vec of (chunk_id, content, file_path) tuples + /// + /// `chunk_id` MUST be the same `file_path:start_line` value stored in the vector + /// table's `id` column. It used to be a `count_rows()`-derived row number, which + /// matched the vector arm's batch-relative index only for the first insert into a + /// fresh table -- which is exactly the shape the unit tests create, so the mismatch + /// never showed up there while fusion was dead in every real index. + pub fn add_documents(&self, documents: Vec<(String, String, String)>) -> Result<()> { // Lock to ensure only one writer at a time (within this process) let _guard = self .writer_lock @@ -161,9 +203,9 @@ impl BM25Search { } }; - for (id, content, file_path) in documents { + for (chunk_id, content, file_path) in documents { let doc = doc!( - self.id_field => id, + self.chunk_id_field => chunk_id, self.content_field => content, self.file_path_field => file_path, ); @@ -206,18 +248,22 @@ impl BM25Search { .doc(doc_address) .context("Failed to retrieve document")?; - if let Some(id_value) = retrieved_doc.get_first(self.id_field) - && let Some(id) = id_value.as_u64() + if let Some(id_value) = retrieved_doc.get_first(self.chunk_id_field) + && let Some(chunk_id) = id_value.as_str() { - results.push(BM25Result { id, score }); + results.push(BM25Result { + chunk_id: chunk_id.to_string(), + score, + }); } } Ok(results) } - /// Delete all documents for a specific ID - pub fn delete_by_id(&self, id: u64) -> Result<()> { + /// Delete all documents for a specific chunk ID + #[allow(dead_code)] + pub fn delete_by_chunk_id(&self, chunk_id: &str) -> Result<()> { // Lock to ensure only one writer at a time let _guard = self .writer_lock @@ -229,7 +275,7 @@ impl BM25Search { .writer(50_000_000) .context("Failed to create index writer")?; - let term = Term::from_field_u64(self.id_field, id); + let term = Term::from_field_text(self.chunk_id_field, chunk_id); index_writer.delete_term(term); index_writer.commit().context("Failed to commit deletion")?; @@ -318,12 +364,15 @@ pub const RRF_K_CONSTANT: f32 = 60.0; /// This is a convenience wrapper around `reciprocal_rank_fusion_generic` for the common case /// of combining vector search results (u64 IDs) with BM25 results. pub fn reciprocal_rank_fusion( - vector_results: Vec<(u64, f32)>, + vector_results: Vec<(String, f32)>, bm25_results: Vec, k: usize, -) -> Vec<(u64, f32)> { +) -> Vec<(String, f32)> { // Convert BM25 results to the same format as vector results - let bm25_tuples: Vec<(u64, f32)> = bm25_results.into_iter().map(|r| (r.id, r.score)).collect(); + let bm25_tuples: Vec<(String, f32)> = bm25_results + .into_iter() + .map(|r| (r.chunk_id, r.score)) + .collect(); // Use the generic implementation reciprocal_rank_fusion_generic([vector_results, bm25_tuples], k) diff --git a/src/client/file_ops.rs b/src/client/file_ops.rs new file mode 100644 index 0000000..c05c073 --- /dev/null +++ b/src/client/file_ops.rs @@ -0,0 +1,786 @@ +//! Reading and editing individual files inside an already-indexed project root, +//! with automatic incremental reindexing after a successful write. +//! +//! Both operations are scoped to files that already live under an indexed root +//! (tracked in `HashCache.roots`) - this is a deliberate security boundary that +//! prevents reading or writing arbitrary filesystem paths outside a project the +//! user has explicitly asked us to index. + +use super::{IndexLockResult, RagClient}; +use crate::types::*; +use anyhow::{Context, Result}; +use sha2::{Digest, Sha256}; +use std::path::{Path, PathBuf}; +use std::time::Instant; + +/// Maximum number of lines returned by a single read_file call. Larger ranges +/// are capped (not silently dropped - `truncated` is set so the caller can page). +const READ_MAX_LINES: usize = 2000; + +fn sha256_hex(text: &str) -> String { + let mut hasher = Sha256::new(); + hasher.update(text.as_bytes()); + format!("{:x}", hasher.finalize()) +} + +/// Same heuristic as `FileWalker::is_text_file`: >=30% non-printable bytes means binary. +fn is_probably_binary(bytes: &[u8]) -> bool { + if bytes.is_empty() { + return false; + } + let non_printable = bytes + .iter() + .filter(|&&b| b < 0x20 && b != b'\n' && b != b'\r' && b != b'\t') + .count(); + (non_printable as f64 / bytes.len() as f64) >= 0.3 +} + +/// Split text into lines, each slice retaining its original trailing `\n`/`\r\n` +/// (except possibly the last, if the file has no trailing newline). An empty +/// string yields zero lines. +fn split_lines(text: &str) -> Vec<&str> { + if text.is_empty() { + Vec::new() + } else { + text.split_inclusive('\n').collect() + } +} + +fn count_lines(text: &str) -> usize { + split_lines(text).len() +} + +/// Splice `request.content` into `current_text` at the requested line range and +/// return the resulting full file content plus its new line count. +fn build_new_content( + current_text: Option<&str>, + request: &EditFileRequest, +) -> Result<(String, usize)> { + match (request.start_line, request.end_line) { + (None, None) => { + let new_full = request.content.clone(); + let total = count_lines(&new_full); + Ok((new_full, total)) + } + (Some(start_line), Some(end_line)) => { + let current_text = current_text.ok_or_else(|| { + anyhow::anyhow!( + "Cannot edit a line range on a file that does not exist yet: {}", + request.file_path + ) + })?; + let lines = split_lines(current_text); + // start_idx/end_idx are 0-indexed; the removed range is [start_idx, end_idx). + // When start_line == end_line + 1 this is an empty range - a pure insertion. + let start_idx = start_line - 1; + let end_idx = end_line; + if start_idx > lines.len() || end_idx > lines.len() { + anyhow::bail!( + "start_line/end_line out of range: file has {} lines, requested [{}, {}]", + lines.len(), + start_line, + end_line + ); + } + + let mut new_full = String::new(); + new_full.push_str(&lines[..start_idx].concat()); + // If the last kept line has no trailing newline (the original file didn't + // end with one), give it one before splicing in new content so the two + // don't run together on the same line. + if !request.content.is_empty() && start_idx > 0 && !lines[start_idx - 1].ends_with('\n') + { + new_full.push('\n'); + } + new_full.push_str(&request.content); + // Keep whatever follows on its own line, unless we're deleting to EOF or + // the caller's content already ends with a newline. + if !request.content.is_empty() + && !request.content.ends_with('\n') + && end_idx < lines.len() + { + new_full.push('\n'); + } + new_full.push_str(&lines[end_idx..].concat()); + + let total = count_lines(&new_full); + Ok((new_full, total)) + } + _ => unreachable!( + "EditFileRequest::validate ensures start_line/end_line are both set or both omitted" + ), + } +} + +impl RagClient { + /// Resolve a user-supplied path to its canonical form and the indexed root + /// that contains it. Errors if the path (or, for a not-yet-created file, its + /// parent directory) doesn't exist, or if it falls outside every indexed root. + async fn resolve_in_indexed_root(&self, file_path: &str) -> Result<(PathBuf, String, bool)> { + let path = Path::new(file_path); + let exists = path.exists(); + + let canonical = if exists { + std::fs::canonicalize(path) + .with_context(|| format!("Failed to canonicalize path: {}", file_path))? + } else { + let parent = match path.parent() { + Some(p) if !p.as_os_str().is_empty() => p, + _ => Path::new("."), + }; + let canonical_parent = std::fs::canonicalize(parent) + .with_context(|| format!("Parent directory does not exist: {:?}", parent))?; + let file_name = path + .file_name() + .ok_or_else(|| anyhow::anyhow!("Invalid file path: {}", file_path))?; + canonical_parent.join(file_name) + }; + + let root = { + let cache = self.hash_cache.read().await; + let mut best_root: Option = None; + for root in cache.roots.keys() { + if canonical.starts_with(Path::new(root)) + && best_root.as_ref().is_none_or(|b| root.len() > b.len()) + { + best_root = Some(root.clone()); + } + } + best_root + }; + + let root = root.ok_or_else(|| { + anyhow::anyhow!( + "'{}' is outside any indexed project root; run index_codebase on its project first", + file_path + ) + })?; + + Ok((canonical, root, exists)) + } + + /// Read a slice (or all) of a file's current on-disk content. + /// + /// The file must live under an already-indexed project root. Returns a + /// SHA256 hash of the full file that can be passed as `expected_hash` to + /// `edit_file` to detect concurrent modification. + pub async fn read_file(&self, request: ReadFileRequest) -> Result { + let (canonical, _root, exists) = self.resolve_in_indexed_root(&request.file_path).await?; + if !exists { + anyhow::bail!("File not found: {}", request.file_path); + } + + let bytes = std::fs::read(&canonical) + .with_context(|| format!("Failed to read file: {}", request.file_path))?; + if is_probably_binary(&bytes) { + anyhow::bail!("Cannot read binary file: {}", request.file_path); + } + let text = String::from_utf8(bytes) + .map_err(|_| anyhow::anyhow!("File is not valid UTF-8: {}", request.file_path))?; + + let lines = split_lines(&text); + let total_lines = lines.len(); + + let (start_line, end_line, truncated) = if total_lines == 0 { + (0, 0, false) + } else { + let requested_start = request.start_line.unwrap_or(1); + let requested_end = request.end_line.unwrap_or(total_lines); + + let clamped_start = requested_start.clamp(1, total_lines); + let clamped_end = requested_end.clamp(clamped_start, total_lines); + + let capped_end = if clamped_end - clamped_start + 1 > READ_MAX_LINES { + clamped_start + READ_MAX_LINES - 1 + } else { + clamped_end + }; + + let truncated = clamped_start != requested_start + || clamped_end != requested_end + || capped_end != clamped_end; + + (clamped_start, capped_end, truncated) + }; + + let content = if total_lines == 0 { + String::new() + } else { + lines[start_line - 1..end_line].concat() + }; + + let extension = canonical + .extension() + .and_then(|e| e.to_str()) + .map(|s| s.to_string()); + let language = extension + .as_ref() + .and_then(|ext| crate::indexer::detect_language(ext)); + + Ok(ReadFileResponse { + content, + start_line, + end_line, + total_lines, + truncated, + file_hash: sha256_hex(&text), + language, + }) + } + + /// Replace a line range (or the whole file) with new content, then + /// incrementally reindex the affected project root. + /// + /// The file must live under an already-indexed project root, or (for a + /// brand new file) its parent directory must. If `expected_hash` is set + /// and doesn't match the file's current hash, the edit is rejected and + /// `status: "hash_conflict"` is returned instead of applied. If the write + /// succeeds but reindexing fails, the write is kept (it's the source of + /// truth), the affected root is left marked dirty via the same mechanism + /// `index_codebase` uses, and `reindexed: false` is returned so the caller + /// knows search results for this file may be stale until the next + /// `index_codebase` call repairs it. + pub async fn edit_file(&self, request: EditFileRequest) -> Result { + let start = Instant::now(); + request.validate().map_err(|e| anyhow::anyhow!(e))?; + + let (canonical, root, exists) = self.resolve_in_indexed_root(&request.file_path).await?; + + if !exists && request.start_line.is_some() { + anyhow::bail!( + "Cannot edit a line range: file does not exist: {}", + request.file_path + ); + } + + let current_text: Option = + if exists { + let bytes = std::fs::read(&canonical) + .with_context(|| format!("Failed to read file: {}", request.file_path))?; + if is_probably_binary(&bytes) { + anyhow::bail!("Cannot edit binary file: {}", request.file_path); + } + Some(String::from_utf8(bytes).map_err(|_| { + anyhow::anyhow!("File is not valid UTF-8: {}", request.file_path) + })?) + } else { + None + }; + + let actual_hash = current_text.as_ref().map(|t| sha256_hex(t)); + if let Some(expected) = &request.expected_hash + && actual_hash.as_deref() != Some(expected.as_str()) + { + return Ok(EditFileResponse { + status: "hash_conflict".to_string(), + file_hash: None, + total_lines: None, + expected_hash: Some(expected.clone()), + actual_hash, + reindexed: false, + warning: None, + duration_ms: start.elapsed().as_millis() as u64, + }); + } + + let (new_content, total_lines) = build_new_content(current_text.as_deref(), &request)?; + + let max_file_size = self.config.indexing.max_file_size; + if new_content.len() as u64 > max_file_size as u64 { + anyhow::bail!( + "Resulting file would be {} bytes, over the configured max_file_size ({} bytes); split the edit into smaller calls", + new_content.len(), + max_file_size + ); + } + + if !exists + && let Some(parent) = canonical.parent() + && !parent.exists() + { + anyhow::bail!("Parent directory does not exist: {:?}", parent); + } + + // Acquire the same lock index_codebase uses, BEFORE writing, so the write + // and the reindex that picks it up happen atomically with respect to any + // other indexing operation on this root. + let lock = match self.try_acquire_index_lock(&root).await? { + IndexLockResult::Acquired(lock) => lock, + IndexLockResult::WaitForResult(_) | IndexLockResult::WaitForFilesystemLock(_) => { + anyhow::bail!( + "Another indexing operation is in progress for '{}'; retry the edit shortly", + root + ); + } + }; + + std::fs::write(&canonical, &new_content) + .with_context(|| format!("Failed to write file: {}", request.file_path))?; + + let reindex_result = crate::client::indexing::do_index_smart_inner( + self, + root.clone(), + request.project.clone(), + vec![], + vec![], + max_file_size, + None, + None, + tokio_util::sync::CancellationToken::new(), + ) + .await; + + let (reindexed, warning) = match &reindex_result { + Ok(response) => { + lock.broadcast_result(response); + (true, None) + } + Err(e) => { + tracing::error!("Reindex after edit failed for root '{}': {}", root, e); + let error_response = IndexResponse { + mode: IndexingMode::Incremental, + files_indexed: 0, + chunks_created: 0, + embeddings_generated: 0, + duration_ms: 0, + errors: vec![format!("Reindex failed: {}", e)], + files_updated: 0, + files_removed: 0, + }; + lock.broadcast_result(&error_response); + ( + false, + Some(format!( + "File was written but reindexing failed ({}); index for '{}' is marked dirty and will self-heal on the next index_codebase call", + e, root + )), + ) + } + }; + lock.release().await; + + Ok(EditFileResponse { + status: "ok".to_string(), + file_hash: Some(sha256_hex(&new_content)), + total_lines: Some(total_lines), + expected_hash: None, + actual_hash: None, + reindexed, + warning, + duration_ms: start.elapsed().as_millis() as u64, + }) + } +} + +#[cfg(test)] +mod tests { + use super::*; + use tempfile::TempDir; + + async fn create_test_client() -> (RagClient, TempDir) { + let temp_dir = TempDir::new().unwrap(); + let db_path = temp_dir.path().join("db").to_string_lossy().to_string(); + let cache_path = temp_dir.path().join("cache.json"); + let client = RagClient::new_with_db_path(&db_path, cache_path) + .await + .unwrap(); + (client, temp_dir) + } + + async fn index_dir(client: &RagClient, dir: &Path) { + let response = client + .index_codebase(IndexRequest { + path: dir.to_string_lossy().to_string(), + project: None, + include_patterns: vec![], + exclude_patterns: vec![], + max_file_size: 1_048_576, + }) + .await + .unwrap(); + assert!( + response.errors.is_empty(), + "indexing errors: {:?}", + response.errors + ); + } + + #[test] + fn test_is_probably_binary() { + assert!(!is_probably_binary(b"")); + assert!(!is_probably_binary(b"fn main() {}\n")); + assert!(is_probably_binary(&[ + 0u8, 1, 2, 3, 255, 254, 253, 252, 0, 1 + ])); + } + + #[test] + fn test_split_lines_and_count() { + assert_eq!(split_lines("").len(), 0); + assert_eq!(count_lines("a\nb\nc\n"), 3); + assert_eq!(count_lines("a\nb\nc"), 3); + assert_eq!(count_lines("a"), 1); + } + + #[test] + fn test_build_new_content_whole_file() { + let req = EditFileRequest { + file_path: "x.rs".to_string(), + content: "fn main() {}\n".to_string(), + start_line: None, + end_line: None, + expected_hash: None, + project: None, + }; + let (content, total) = build_new_content(None, &req).unwrap(); + assert_eq!(content, "fn main() {}\n"); + assert_eq!(total, 1); + } + + #[test] + fn test_build_new_content_replace_range() { + let original = "line1\nline2\nline3\n"; + let req = EditFileRequest { + file_path: "x.rs".to_string(), + content: "REPLACED\n".to_string(), + start_line: Some(2), + end_line: Some(2), + expected_hash: None, + project: None, + }; + let (content, total) = build_new_content(Some(original), &req).unwrap(); + assert_eq!(content, "line1\nREPLACED\nline3\n"); + assert_eq!(total, 3); + } + + #[test] + fn test_build_new_content_insert_only() { + let original = "line1\nline2\n"; + // start_line == end_line + 1 -> insert before line 2, delete nothing + let req = EditFileRequest { + file_path: "x.rs".to_string(), + content: "INSERTED\n".to_string(), + start_line: Some(2), + end_line: Some(1), + expected_hash: None, + project: None, + }; + let (content, _total) = build_new_content(Some(original), &req).unwrap(); + assert_eq!(content, "line1\nINSERTED\nline2\n"); + } + + #[test] + fn test_build_new_content_append_past_eof_no_trailing_newline() { + // File's last line has no trailing newline; appending past EOF must not + // glue the new content onto the end of that line. + let original = "line1\nline2"; + let req = EditFileRequest { + file_path: "x.rs".to_string(), + content: "line3\n".to_string(), + start_line: Some(3), + end_line: Some(2), + expected_hash: None, + project: None, + }; + let (content, total) = build_new_content(Some(original), &req).unwrap(); + assert_eq!(content, "line1\nline2\nline3\n"); + assert_eq!(total, 3); + } + + #[test] + fn test_build_new_content_delete_range() { + let original = "line1\nline2\nline3\n"; + let req = EditFileRequest { + file_path: "x.rs".to_string(), + content: String::new(), + start_line: Some(2), + end_line: Some(2), + expected_hash: None, + project: None, + }; + let (content, _total) = build_new_content(Some(original), &req).unwrap(); + assert_eq!(content, "line1\nline3\n"); + } + + #[test] + fn test_build_new_content_out_of_range_errors() { + let original = "line1\n"; + let req = EditFileRequest { + file_path: "x.rs".to_string(), + content: "x".to_string(), + start_line: Some(5), + end_line: Some(5), + expected_hash: None, + project: None, + }; + assert!(build_new_content(Some(original), &req).is_err()); + } + + #[tokio::test] + async fn test_read_file_outside_indexed_root_errors() { + let (client, temp_dir) = create_test_client().await; + let file = temp_dir.path().join("orphan.rs"); + std::fs::write(&file, "fn main() {}").unwrap(); + + let result = client + .read_file(ReadFileRequest { + file_path: file.to_string_lossy().to_string(), + start_line: None, + end_line: None, + }) + .await; + assert!(result.is_err()); + } + + #[tokio::test] + async fn test_read_file_whole_file() { + let (client, temp_dir) = create_test_client().await; + let data_dir = temp_dir.path().join("data"); + std::fs::create_dir(&data_dir).unwrap(); + let file = data_dir.join("a.rs"); + std::fs::write(&file, "fn a() {}\nfn b() {}\nfn c() {}\n").unwrap(); + index_dir(&client, &data_dir).await; + + let response = client + .read_file(ReadFileRequest { + file_path: file.to_string_lossy().to_string(), + start_line: None, + end_line: None, + }) + .await + .unwrap(); + + assert_eq!(response.total_lines, 3); + assert_eq!(response.start_line, 1); + assert_eq!(response.end_line, 3); + assert!(!response.truncated); + assert_eq!(response.content, "fn a() {}\nfn b() {}\nfn c() {}\n"); + assert_eq!( + response.file_hash, + sha256_hex("fn a() {}\nfn b() {}\nfn c() {}\n") + ); + } + + #[tokio::test] + async fn test_read_file_range_clamped() { + let (client, temp_dir) = create_test_client().await; + let data_dir = temp_dir.path().join("data"); + std::fs::create_dir(&data_dir).unwrap(); + let file = data_dir.join("a.rs"); + std::fs::write(&file, "1\n2\n3\n").unwrap(); + index_dir(&client, &data_dir).await; + + let response = client + .read_file(ReadFileRequest { + file_path: file.to_string_lossy().to_string(), + start_line: Some(2), + end_line: Some(100), + }) + .await + .unwrap(); + + assert_eq!(response.start_line, 2); + assert_eq!(response.end_line, 3); + assert!(response.truncated); + assert_eq!(response.content, "2\n3\n"); + } + + #[tokio::test] + async fn test_read_file_not_found() { + let (client, temp_dir) = create_test_client().await; + let data_dir = temp_dir.path().join("data"); + std::fs::create_dir(&data_dir).unwrap(); + std::fs::write(data_dir.join("a.rs"), "fn a() {}\n").unwrap(); + index_dir(&client, &data_dir).await; + + let missing = data_dir.join("missing.rs"); + let result = client + .read_file(ReadFileRequest { + file_path: missing.to_string_lossy().to_string(), + start_line: None, + end_line: None, + }) + .await; + assert!(result.is_err()); + } + + #[tokio::test] + async fn test_edit_file_create_new_file() { + let (client, temp_dir) = create_test_client().await; + let data_dir = temp_dir.path().join("data"); + std::fs::create_dir(&data_dir).unwrap(); + std::fs::write(data_dir.join("seed.rs"), "fn seed() {}\n").unwrap(); + index_dir(&client, &data_dir).await; + + let new_file = data_dir.join("new.rs"); + let response = client + .edit_file(EditFileRequest { + file_path: new_file.to_string_lossy().to_string(), + content: "fn brand_new() {}\n".to_string(), + start_line: None, + end_line: None, + expected_hash: None, + project: None, + }) + .await + .unwrap(); + + assert_eq!(response.status, "ok"); + assert!(response.reindexed); + assert_eq!(response.total_lines, Some(1)); + assert_eq!( + std::fs::read_to_string(&new_file).unwrap(), + "fn brand_new() {}\n" + ); + } + + #[tokio::test] + async fn test_edit_file_replace_range_and_reindex() { + let (client, temp_dir) = create_test_client().await; + let data_dir = temp_dir.path().join("data"); + std::fs::create_dir(&data_dir).unwrap(); + let file = data_dir.join("a.rs"); + std::fs::write(&file, "fn a() {}\nfn b() {}\nfn c() {}\n").unwrap(); + index_dir(&client, &data_dir).await; + + let response = client + .edit_file(EditFileRequest { + file_path: file.to_string_lossy().to_string(), + content: "fn b_renamed() {}\n".to_string(), + start_line: Some(2), + end_line: Some(2), + expected_hash: None, + project: None, + }) + .await + .unwrap(); + + assert_eq!(response.status, "ok"); + assert!(response.reindexed); + let on_disk = std::fs::read_to_string(&file).unwrap(); + assert_eq!(on_disk, "fn a() {}\nfn b_renamed() {}\nfn c() {}\n"); + assert_eq!(response.file_hash, Some(sha256_hex(&on_disk))); + } + + #[tokio::test] + async fn test_edit_file_hash_conflict() { + let (client, temp_dir) = create_test_client().await; + let data_dir = temp_dir.path().join("data"); + std::fs::create_dir(&data_dir).unwrap(); + let file = data_dir.join("a.rs"); + std::fs::write(&file, "fn a() {}\n").unwrap(); + index_dir(&client, &data_dir).await; + + let response = client + .edit_file(EditFileRequest { + file_path: file.to_string_lossy().to_string(), + content: "fn changed() {}\n".to_string(), + start_line: None, + end_line: None, + expected_hash: Some("stale-hash-that-does-not-match".to_string()), + project: None, + }) + .await + .unwrap(); + + assert_eq!(response.status, "hash_conflict"); + assert!(!response.reindexed); + // File on disk must be untouched + assert_eq!(std::fs::read_to_string(&file).unwrap(), "fn a() {}\n"); + } + + #[tokio::test] + async fn test_edit_file_matching_hash_succeeds() { + let (client, temp_dir) = create_test_client().await; + let data_dir = temp_dir.path().join("data"); + std::fs::create_dir(&data_dir).unwrap(); + let file = data_dir.join("a.rs"); + let original = "fn a() {}\n"; + std::fs::write(&file, original).unwrap(); + index_dir(&client, &data_dir).await; + + let response = client + .edit_file(EditFileRequest { + file_path: file.to_string_lossy().to_string(), + content: "fn changed() {}\n".to_string(), + start_line: None, + end_line: None, + expected_hash: Some(sha256_hex(original)), + project: None, + }) + .await + .unwrap(); + + assert_eq!(response.status, "ok"); + assert_eq!(std::fs::read_to_string(&file).unwrap(), "fn changed() {}\n"); + } + + #[tokio::test] + async fn test_edit_file_content_too_large_rejected() { + let (client, temp_dir) = create_test_client().await; + let data_dir = temp_dir.path().join("data"); + std::fs::create_dir(&data_dir).unwrap(); + let file = data_dir.join("a.rs"); + std::fs::write(&file, "fn a() {}\n").unwrap(); + index_dir(&client, &data_dir).await; + + let huge = "x".repeat(client.config().indexing.max_file_size + 1); + let result = client + .edit_file(EditFileRequest { + file_path: file.to_string_lossy().to_string(), + content: huge, + start_line: None, + end_line: None, + expected_hash: None, + project: None, + }) + .await; + assert!(result.is_err()); + // Original content must be untouched + assert_eq!(std::fs::read_to_string(&file).unwrap(), "fn a() {}\n"); + } + + #[tokio::test] + async fn test_edit_file_outside_indexed_root_errors() { + let (client, temp_dir) = create_test_client().await; + let file = temp_dir.path().join("orphan.rs"); + std::fs::write(&file, "fn main() {}").unwrap(); + + let result = client + .edit_file(EditFileRequest { + file_path: file.to_string_lossy().to_string(), + content: "fn changed() {}\n".to_string(), + start_line: None, + end_line: None, + expected_hash: None, + project: None, + }) + .await; + assert!(result.is_err()); + } + + #[tokio::test] + async fn test_edit_file_binary_rejected() { + let (client, temp_dir) = create_test_client().await; + let data_dir = temp_dir.path().join("data"); + std::fs::create_dir(&data_dir).unwrap(); + let file = data_dir.join("a.rs"); + std::fs::write(&file, "fn a() {}\n").unwrap(); + index_dir(&client, &data_dir).await; + + // Overwrite on disk (outside the tool) with binary content, then try to edit it + std::fs::write(&file, [0u8, 1, 2, 3, 255, 254, 253, 252, 0, 1]).unwrap(); + + let result = client + .edit_file(EditFileRequest { + file_path: file.to_string_lossy().to_string(), + content: "fn changed() {}\n".to_string(), + start_line: None, + end_line: None, + expected_hash: None, + project: None, + }) + .await; + assert!(result.is_err()); + } +} diff --git a/src/client/find_unused.rs b/src/client/find_unused.rs new file mode 100644 index 0000000..ca76765 --- /dev/null +++ b/src/client/find_unused.rs @@ -0,0 +1,1143 @@ +//! find_unused: report import bindings and symbol definitions that nothing +//! references. +//! +//! The analysis is text-based on top of AST extraction, so it errs on the side +//! of "used": any word-boundary mention of a name -- call, type, comment, +//! import elsewhere -- counts as usage. What it CANNOT see is dynamic dispatch, +//! macro expansion, reflection and framework wiring, which is why every +//! candidate carries a confidence level and the tool never edits anything. +//! +//! Two checks: +//! - **imports**: an import binding is unused when its name never appears in +//! the importing file outside import statements. File-local, needs no index. +//! C/C++ `#include` is verified indirectly: the header is located among the +//! scanned files or resolved on disk next to the including file or the scan +//! root, and the question becomes "do any symbols it defines appear in this +//! file?". An include that cannot be resolved is skipped -- counted in +//! `unverifiable_import_details`, never flagged -- as are system `<...>` +//! includes, C# namespace usings and Swift module imports. +//! - **symbols**: a definition is a dead-code candidate when its name appears +//! nowhere else in the scanned corpus outside import lines and same-name +//! definition spans. When only part of an indexed root is scanned, the +//! BM25/hybrid index is probed for outside usage before flagging anything. + +use super::RagClient; +use crate::indexer::{FileInfo, FileWalker}; +use crate::relations::repomap::language_name_for_extension; +use crate::relations::{Definition, RelationsProvider, SymbolKind, Visibility}; +use crate::types::{ + FindUnusedRequest, FindUnusedResponse, SymbolRejections, UnusedCandidate, UnverifiableImport, +}; +use anyhow::{Context, Result}; +use rayon::prelude::*; +use regex::Regex; +use std::collections::{HashMap, HashSet}; +use std::path::{Path, PathBuf}; +use std::sync::Arc; +use std::time::Instant; + +/// Names that runtimes, traits and frameworks invoke implicitly; a zero-mention +/// count for these means nothing, so they are never flagged. +const IMPLICITLY_INVOKED: &[&str] = &[ + "main", + "new", + "default", + "fmt", + "drop", + "clone", + "eq", + "ne", + "cmp", + "partial_cmp", + "hash", + "next", + "from", + "into", + "try_from", + "deref", + "deref_mut", + "index", + "index_mut", + "serialize", + "deserialize", + "to_string", + "from_str", + "__init__", + "__str__", + "__repr__", + "__eq__", + "__hash__", + "__len__", + "toString", + "equals", + "hashCode", + "constructor", + "render", +]; + +/// Cap on cross-index BM25 probes per call. Symbols past the budget are +/// conservatively treated as used and `probes_exhausted` is set. +const MAX_BM25_PROBES: usize = 100; +const PROBE_FILES_PER_NAME: usize = 10; + +/// Cap on entries in `unverifiable_import_details`; the `unverifiable_imports` +/// count stays authoritative past it. +const MAX_UNVERIFIABLE_DETAILS: usize = 200; +/// Cap on header-symbol names spelled out in a candidate's probe string. +const MAX_PROBE_SYMBOLS_SHOWN: usize = 10; + +/// Everything the pure, per-file part of the analysis produces. +struct Analyzed { + files: Vec, + /// Definitions per file, parallel to `files` + defs: Vec>, + /// identifier -> 1-based lines it appears on, per file + idents: Vec>>, + /// (start_line, end_line) of every import statement, per file + import_spans: Vec>, + /// name -> definition spans, per file; a mention inside a same-name + /// definition (its own body, its impl block) is not usage + def_spans_by_name: Vec>>, + /// Definition nodes the extractor recognised but could not name; those + /// symbols were never considered at all + skipped_definitions: usize, + /// Canonicalized paths of scanned files, for excluding them from probes + scanned_paths: HashSet, +} + +/// Import candidates plus every binding that could not be verified, with the +/// reason it could not be. +#[derive(Default)] +struct ImportScan { + candidates: Vec, + unverifiable: Vec, +} + +impl RagClient { + /// Scan a file or directory for unused imports and unused symbols. + pub async fn find_unused(&self, request: FindUnusedRequest) -> Result { + let start = Instant::now(); + request.validate().map_err(|e| anyhow::anyhow!(e))?; + + let normalized = Self::normalize_path(&request.path)?; + + // Symbol verification needs the index: without it a partial scan cannot + // be told apart from a full one, and the cross-file probe has nothing to + // search, so everything would look unused -- the dangerous direction. + let indexed_root = self.find_indexed_root(&normalized).await; + if request.check_symbols() { + let Some(ref root) = indexed_root else { + anyhow::bail!( + "'{}' is not inside an indexed root; run index_codebase first, \ + or use check: \"imports\" for index-free import analysis", + request.path + ); + }; + self.check_path_not_dirty(Some(root)).await?; + } + // Probes are needed only when the scan covers less than the indexed + // root; scanning the whole root makes the in-memory corpus authoritative. + let partial_scan = indexed_root.as_deref() != Some(normalized.as_str()); + + // Gather and analyze files on a blocking thread (I/O + tree-sitter + regex). + let provider = self.relations_provider.clone(); + let single_file = Path::new(&normalized).is_file(); + let files = if single_file { + vec![self.create_file_info(&normalized, request.project.clone())?] + } else { + let walker = FileWalker::new(&normalized, request.max_file_size) + .with_project(request.project.clone()); + tokio::task::spawn_blocking(move || walker.walk()) + .await + .context("File walker task panicked")? + .context("Failed to walk directory")? + }; + + // Include resolution reads and parses headers from disk, so the import + // scan shares the analysis' blocking task. + let scan_root = { + let p = PathBuf::from(&normalized); + if single_file { + p.parent().map(|q| q.to_path_buf()).unwrap_or(p) + } else { + p + } + }; + let check_imports = request.check_imports(); + let resolver_provider = provider.clone(); + let resolver_project = request.project.clone(); + let (analyzed, import_scan) = tokio::task::spawn_blocking(move || { + let analyzed = analyze(files, provider); + let import_scan = if check_imports { + let mut resolver = IncludeResolver { + scan_root, + project: resolver_project, + provider: resolver_provider, + cache: HashMap::new(), + }; + import_candidates(&analyzed, &mut resolver) + } else { + ImportScan::default() + }; + (analyzed, import_scan) + }) + .await + .context("Analysis task panicked")?; + + let definitions_checked = analyzed.defs.iter().map(|d| d.len()).sum(); + + let mut candidates = import_scan.candidates; + let unverifiable_imports = import_scan.unverifiable.len(); + let mut unverifiable_import_details = import_scan.unverifiable; + unverifiable_import_details.truncate(MAX_UNVERIFIABLE_DETAILS); + + let mut probes_exhausted = false; + let mut symbol_rejections = SymbolRejections::default(); + if request.check_symbols() { + let (pending, rejections) = symbol_candidates(&analyzed); + symbol_rejections = rejections; + if partial_scan { + let (confirmed, exhausted, probe_errors) = self + .probe_unmentioned(pending, request.project.clone(), &analyzed.scanned_paths) + .await; + candidates.extend(confirmed); + probes_exhausted = exhausted; + symbol_rejections.probe_errors = probe_errors; + } else { + // Full-root scan: the corpus already proved these unmentioned. + candidates.extend(pending.into_iter().map(|(_, c)| c)); + } + } + + // One entry per (file, name): a Rust struct and its impl block are two + // definitions of the same name, and flagging both is noise. + candidates.sort_by(|a, b| (&a.file_path, a.line).cmp(&(&b.file_path, b.line))); + let mut seen = HashSet::new(); + candidates.retain(|c| seen.insert((c.file_path.clone(), c.name.clone()))); + + let total_candidates = candidates.len(); + let truncated = total_candidates > request.limit; + candidates.truncate(request.limit); + + Ok(FindUnusedResponse { + scanned_root: normalized, + files_scanned: analyzed.files.len(), + definitions_checked, + candidates, + total_candidates, + unverifiable_imports, + unverifiable_import_details, + symbol_rejections, + skipped_definitions: analyzed.skipped_definitions, + truncated, + probes_exhausted, + precision: "medium".to_string(), + duration_ms: start.elapsed().as_millis() as u64, + }) + } + + /// The indexed root that contains `normalized`, if any. + async fn find_indexed_root(&self, normalized: &str) -> Option { + let cache = self.hash_cache.read().await; + cache + .roots + .keys() + .find(|root| { + normalized == root.as_str() + || normalized + .strip_prefix(root.as_str()) + .is_some_and(|rest| rest.starts_with(['/', '\\'])) + }) + .cloned() + } + + /// Probe the index for usage of each pending name; return the candidates + /// whose names are mentioned nowhere, whether the budget ran out, and how + /// many probes errored (those names are conservatively treated as used). + async fn probe_unmentioned( + &self, + pending: Vec<(String, UnusedCandidate)>, + project: Option, + scanned_paths: &HashSet, + ) -> (Vec, bool, usize) { + // Group candidates by name so each name is probed once. + let mut by_name: HashMap> = HashMap::new(); + for (name, candidate) in pending { + by_name.entry(name).or_default().push(candidate); + } + let mut names: Vec = by_name.keys().cloned().collect(); + names.sort(); + + let exhausted = names.len() > MAX_BM25_PROBES; + let mut confirmed = Vec::new(); + let mut probe_errors = 0usize; + + for name in names.into_iter().take(MAX_BM25_PROBES) { + let mut used = false; + let probe_files = match self + .files_mentioning(&name, project.clone(), PROBE_FILES_PER_NAME) + .await + { + Ok(f) => f, + Err(e) => { + // Cannot verify -- treat as used rather than flag blindly. + tracing::debug!("Probe failed for {}: {}", name, e); + probe_errors += 1; + continue; + } + }; + for file in probe_files { + let canonical = tokio::fs::canonicalize(&file) + .await + .unwrap_or_else(|_| file.clone()); + if scanned_paths.contains(&canonical) { + continue; // already covered by the in-memory corpus check + } + match tokio::fs::read_to_string(&file).await { + Ok(content) if mentions_identifier(&content, &name) => { + used = true; + break; + } + _ => {} + } + } + if !used { + confirmed.extend(by_name.remove(&name).unwrap_or_default()); + } + } + (confirmed, exhausted, probe_errors) + } +} + +/// Pure per-file analysis: definitions, identifier occurrence maps, import and +/// definition spans. +fn analyze( + files: Vec, + provider: Arc, +) -> Analyzed { + let ident_re = Regex::new(r"\b[a-zA-Z_][a-zA-Z0-9_]*\b").expect("static regex"); + + let reported: Vec<(Vec, usize)> = files + .par_iter() + .map(|file| { + provider + .extract_definitions_reporting(file) + .map(|(defs, skipped)| (defs, skipped.len())) + .unwrap_or_else(|e| { + tracing::debug!( + "Definition extraction failed for {}: {}", + file.relative_path, + e + ); + (Vec::new(), 0) + }) + }) + .collect(); + let skipped_definitions = reported.iter().map(|(_, skipped)| skipped).sum(); + let defs: Vec> = reported.into_iter().map(|(defs, _)| defs).collect(); + + let idents: Vec>> = files + .par_iter() + .map(|file| identifier_lines(&file.content, &ident_re)) + .collect(); + + let import_spans: Vec> = defs + .iter() + .map(|file_defs| { + file_defs + .iter() + .filter(|d| d.kind() == SymbolKind::Import) + .map(|d| { + // tree-sitter ends a preproc_include past its newline, at + // column 0 of the NEXT row; unclamped, that span swallows + // the first code line after an include and hides every + // identifier on it from the usage check. + let end = if d.end_col == 0 && d.end_line > d.start_line() { + d.end_line - 1 + } else { + d.end_line + }; + (d.start_line(), end) + }) + .collect() + }) + .collect(); + + let def_spans_by_name: Vec>> = defs + .iter() + .map(|file_defs| { + let mut spans: HashMap> = HashMap::new(); + for d in file_defs { + spans + .entry(d.name().to_string()) + .or_default() + .push((d.start_line(), d.end_line)); + } + spans + }) + .collect(); + + let scanned_paths = files + .iter() + .map(|f| std::fs::canonicalize(&f.path).unwrap_or_else(|_| f.path.clone())) + .collect(); + + Analyzed { + files, + defs, + idents, + import_spans, + def_spans_by_name, + skipped_definitions, + scanned_paths, + } +} + +/// Resolves quoted include paths on disk and enumerates the symbols the +/// resolved header defines. Parse results are cached per canonical path. +struct IncludeResolver { + /// Base directory includes resolve against besides the including file's own + scan_root: PathBuf, + project: Option, + provider: Arc, + cache: HashMap>>, +} + +impl IncludeResolver { + /// `None` = not resolvable on disk (or unreadable / unparseable). + /// `Some` with an empty list = resolved, but nothing extractable is + /// defined in it. + fn header_symbols(&mut self, including_file: &Path, needle: &str) -> Option>> { + for base in [including_file.parent(), Some(self.scan_root.as_path())] + .into_iter() + .flatten() + { + let candidate = base.join(needle); + let Ok(canonical) = std::fs::canonicalize(&candidate) else { + continue; + }; + if !canonical.is_file() { + continue; + } + if let Some(cached) = self.cache.get(&canonical) { + return Some(cached.clone()); + } + let Ok(info) = + RagClient::build_file_info(&canonical.to_string_lossy(), self.project.clone()) + else { + return None; + }; + let Ok((defs, _skipped)) = self.provider.extract_definitions_reporting(&info) else { + return None; + }; + let names: Vec = defs + .iter() + .filter(|d| d.kind() != SymbolKind::Import && d.name().len() >= 2) + .map(|d| d.name().to_string()) + .collect(); + let names = Arc::new(names); + self.cache.insert(canonical, names.clone()); + return Some(names); + } + None + } +} + +/// Unused-import candidates plus the bindings that could not be verified. +fn import_candidates(analyzed: &Analyzed, resolver: &mut IncludeResolver) -> ImportScan { + let mut scan = ImportScan::default(); + + for (i, file_defs) in analyzed.defs.iter().enumerate() { + // The extractor's taxonomy, from the extension -- NOT FileInfo.language. + // The display taxonomy calls headers "C/C++ Header", which would route + // every #include into the generic arm below and probe for a filename no + // identifier token can ever match. + let language = analyzed.files[i] + .extension + .as_deref() + .and_then(language_name_for_extension); + let file_path = &analyzed.files[i].relative_path; + for def in file_defs.iter().filter(|d| d.kind() == SymbolKind::Import) { + match language { + Some("C") | Some("C++") => match check_include(analyzed, i, def, resolver) { + IncludeVerdict::Unused(candidate) => scan.candidates.push(candidate), + IncludeVerdict::Used => {} + IncludeVerdict::Unverifiable(reason) => { + scan.unverifiable.push(UnverifiableImport { + file_path: file_path.clone(), + name: def.name().to_string(), + reason, + }) + } + }, + // Swift imports bind a module whose name need not appear in code. + Some("Swift") => scan.unverifiable.push(UnverifiableImport { + file_path: file_path.clone(), + name: def.name().to_string(), + reason: "Swift module import; the module name need not appear in code" + .to_string(), + }), + // A plain C# `using Namespace;` makes members usable WITHOUT the + // namespace name appearing; only alias usings are checkable. + Some("C#") if !def.signature.contains('=') => { + scan.unverifiable.push(UnverifiableImport { + file_path: file_path.clone(), + name: def.name().to_string(), + reason: "C# namespace using; members are usable without the namespace name" + .to_string(), + }) + } + // No language means no extraction should have produced imports; + // never emit a candidate that no probe can support. + None => scan.unverifiable.push(UnverifiableImport { + file_path: file_path.clone(), + name: def.name().to_string(), + reason: "file language unknown; import not checkable".to_string(), + }), + Some(_) => { + if !binding_used(analyzed, i, def.name()) { + let (confidence, note) = if language == Some("Rust") { + ( + "medium", + "; note: Rust trait imports can be used implicitly via method calls", + ) + } else { + ("high", "") + }; + scan.candidates.push(make_candidate( + def, + file_path, + confidence, + format!( + "imported name '{}' is never referenced in this file{}", + def.name(), + note + ), + format!( + "whole-word search for '{}' in this file outside import lines", + def.name() + ), + )); + } + } + } + } + } + scan +} + +enum IncludeVerdict { + Unused(UnusedCandidate), + Used, + /// Why the include could not be checked; travels into + /// `unverifiable_import_details`. + Unverifiable(String), +} + +/// Verify a C/C++ `#include` indirectly: is any symbol defined by the included +/// header referenced in the including file? The header is located among the +/// scanned files first, then resolved on disk relative to the including file +/// and the scan root. An include that cannot be resolved is skipped +/// (unverifiable), never flagged. +fn check_include( + analyzed: &Analyzed, + i: usize, + def: &Definition, + resolver: &mut IncludeResolver, +) -> IncludeVerdict { + // Only quoted local includes are resolvable; `<...>` system headers live on + // include paths this tool does not know. + if !def.signature.contains('"') { + return IncludeVerdict::Unverifiable("system include (<...>); not resolvable".to_string()); + } + + let needle = def.name().trim_start_matches("./").replace('\\', "/"); + let target = (0..analyzed.files.len()).find(|&j| { + j != i && { + let p = analyzed.files[j].relative_path.replace('\\', "/"); + p == needle || p.ends_with(&format!("/{}", needle)) + } + }); + + let header_symbols: Vec = match target { + Some(j) => analyzed.defs[j] + .iter() + .filter(|d| d.kind() != SymbolKind::Import && d.name().len() >= 2) + .map(|d| d.name().to_string()) + .collect(), + None => match resolver.header_symbols(&analyzed.files[i].path, &needle) { + Some(names) => names.as_ref().clone(), + None => { + return IncludeVerdict::Unverifiable( + "include not resolved in scan set or on disk".to_string(), + ); + } + }, + }; + if header_symbols.is_empty() { + return IncludeVerdict::Unverifiable( + "resolved header has no extractable definitions".to_string(), + ); + } + + if header_symbols.iter().any(|n| binding_used(analyzed, i, n)) { + IncludeVerdict::Used + } else { + let shown = header_symbols + .iter() + .take(MAX_PROBE_SYMBOLS_SHOWN) + .map(String::as_str) + .collect::>() + .join(", "); + let extra = header_symbols.len().saturating_sub(MAX_PROBE_SYMBOLS_SHOWN); + let probe = if extra > 0 { + format!( + "searched this file for {} symbols defined by the header: {}, +{} more", + header_symbols.len(), + shown, + extra + ) + } else { + format!( + "searched this file for {} symbols defined by the header: {}", + header_symbols.len(), + shown + ) + }; + IncludeVerdict::Unused(make_candidate( + def, + &analyzed.files[i].relative_path, + "medium", + format!( + "none of the {} symbols defined by '{}' are referenced in this file", + header_symbols.len(), + def.name() + ), + probe, + )) + } +} + +/// True if `name` appears in file `i` on any line outside every import statement. +fn binding_used(analyzed: &Analyzed, i: usize, name: &str) -> bool { + analyzed.idents[i].get(name).is_some_and(|lines| { + lines + .iter() + .any(|&line| !line_in_spans(line, &analyzed.import_spans[i])) + }) +} + +/// Symbol definitions with zero mentions anywhere in the scanned corpus, paired +/// with their name for probe grouping, plus counts of why the rest were +/// rejected. +fn symbol_candidates(analyzed: &Analyzed) -> (Vec<(String, UnusedCandidate)>, SymbolRejections) { + let mut pending = Vec::new(); + let mut rejections = SymbolRejections::default(); + + for (i, file_defs) in analyzed.defs.iter().enumerate() { + for def in file_defs { + if !symbol_eligible(def) { + rejections.ineligible += 1; + continue; + } + let name = def.name(); + + // A mention inside any same-name definition (its own body, its impl + // block) is not usage. + let own_spans = analyzed.def_spans_by_name[i] + .get(name) + .map(Vec::as_slice) + .unwrap_or(&[]); + let used_here = analyzed.idents[i] + .get(name) + .is_some_and(|lines| lines.iter().any(|&l| !line_in_spans(l, own_spans))); + if used_here { + rejections.used_here += 1; + continue; + } + + // In other files, mentions on import lines and inside same-name + // definition spans do not count either: importing a symbol is not + // using it, and another definition of the same name is not a + // reference to this one. + let used_elsewhere = (0..analyzed.files.len()).any(|j| { + if j == i { + return false; + } + let Some(lines) = analyzed.idents[j].get(name) else { + return false; + }; + let spans = analyzed.def_spans_by_name[j] + .get(name) + .map(Vec::as_slice) + .unwrap_or(&[]); + lines.iter().any(|&l| { + !line_in_spans(l, &analyzed.import_spans[j]) && !line_in_spans(l, spans) + }) + }); + if used_elsewhere { + rejections.used_elsewhere += 1; + continue; + } + + let (confidence, reason) = if def.visibility == Visibility::Public { + ( + "low", + format!( + "no references to '{}' found; symbol is public and may be used externally", + name + ), + ) + } else { + ( + "medium", + format!("no references to '{}' found in the scanned corpus", name), + ) + }; + let probe = format!( + "whole-word search for '{}' across {} scanned files, excluding import lines and same-name definition spans", + name, + analyzed.files.len() + ); + pending.push(( + name.to_string(), + make_candidate( + def, + &analyzed.files[i].relative_path, + confidence, + reason, + probe, + ), + )); + } + } + (pending, rejections) +} + +/// Kinds and names worth checking for dead code. +fn symbol_eligible(def: &Definition) -> bool { + let kind_ok = matches!( + def.kind(), + SymbolKind::Function + | SymbolKind::Method + | SymbolKind::Class + | SymbolKind::Struct + | SymbolKind::Interface + | SymbolKind::Trait + | SymbolKind::Enum + | SymbolKind::TypeAlias + | SymbolKind::Constant + | SymbolKind::Variable + | SymbolKind::Module + ); + let name = def.name(); + kind_ok + && name.len() >= 3 + && !IMPLICITLY_INVOKED.contains(&name) + && !name.starts_with("test_") + && !name.starts_with("Test") +} + +fn make_candidate( + def: &Definition, + file_path: &str, + confidence: &str, + reason: String, + probe: String, +) -> UnusedCandidate { + UnusedCandidate { + file_path: file_path.to_string(), + name: def.name().to_string(), + kind: def.kind(), + line: def.start_line(), + confidence: confidence.to_string(), + reason, + signature: def.signature.clone(), + probe, + } +} + +/// identifier -> 1-based lines it appears on. +fn identifier_lines(content: &str, re: &Regex) -> HashMap> { + let mut map: HashMap> = HashMap::new(); + for (idx, line) in content.lines().enumerate() { + for m in re.find_iter(line) { + map.entry(m.as_str().to_string()).or_default().push(idx + 1); + } + } + map +} + +fn line_in_spans(line: usize, spans: &[(usize, usize)]) -> bool { + spans.iter().any(|&(s, e)| line >= s && line <= e) +} + +/// Word-boundary check: `name` appears in `text` as a whole identifier. +fn mentions_identifier(text: &str, name: &str) -> bool { + if name.is_empty() { + return false; + } + let bytes = text.as_bytes(); + let is_ident = |b: u8| b.is_ascii_alphanumeric() || b == b'_'; + for (pos, _) in text.match_indices(name) { + let before_ok = pos == 0 || !is_ident(bytes[pos - 1]); + let after = pos + name.len(); + let after_ok = after >= bytes.len() || !is_ident(bytes[after]); + if before_ok && after_ok { + return true; + } + } + false +} +#[cfg(test)] +mod tests { + use super::*; + use crate::types::FindUnusedRequest; + use tempfile::TempDir; + + #[test] + fn test_mentions_identifier_boundaries() { + assert!(mentions_identifier("let x = foo();", "foo")); + assert!(mentions_identifier("foo", "foo")); + assert!(!mentions_identifier("food()", "foo")); + assert!(!mentions_identifier("my_foo", "foo")); + assert!(!mentions_identifier("foo1", "foo")); + assert!(mentions_identifier("a.foo.b", "foo")); + assert!(!mentions_identifier("", "foo")); + } + + #[test] + fn test_line_in_spans() { + let spans = [(1, 1), (5, 8)]; + assert!(line_in_spans(1, &spans)); + assert!(line_in_spans(6, &spans)); + assert!(!line_in_spans(2, &spans)); + assert!(!line_in_spans(9, &spans)); + } + + #[test] + fn test_identifier_lines() { + let re = Regex::new(r"\b[a-zA-Z_][a-zA-Z0-9_]*\b").unwrap(); + let map = identifier_lines("fn foo() {\n bar();\n foo();\n}\n", &re); + assert_eq!(map.get("foo"), Some(&vec![1, 3])); + assert_eq!(map.get("bar"), Some(&vec![2])); + assert!(!map.contains_key("baz")); + } + + async fn create_test_client() -> (RagClient, TempDir) { + let temp_dir = TempDir::new().unwrap(); + let db_path = temp_dir.path().join("db").to_string_lossy().to_string(); + let cache_path = temp_dir.path().join("cache.json"); + let client = RagClient::new_with_db_path(&db_path, cache_path) + .await + .unwrap(); + (client, temp_dir) + } + + fn make_request(path: &str, check: &str) -> FindUnusedRequest { + FindUnusedRequest { + path: path.to_string(), + project: None, + check: check.to_string(), + limit: 100, + max_file_size: 1_048_576, + } + } + + #[tokio::test] + async fn test_symbols_check_requires_indexed_root() { + let (client, temp_dir) = create_test_client().await; + let data_dir = temp_dir.path().join("data"); + std::fs::create_dir(&data_dir).unwrap(); + std::fs::write(data_dir.join("a.rs"), "fn lonely_helper() {}\n").unwrap(); + + let result = client + .find_unused(make_request(&data_dir.to_string_lossy(), "symbols")) + .await; + assert!(result.is_err()); + assert!(format!("{:#}", result.unwrap_err()).contains("index_codebase")); + } + + #[tokio::test] + async fn test_imports_check_works_without_index() { + let (client, temp_dir) = create_test_client().await; + let data_dir = temp_dir.path().join("data"); + std::fs::create_dir(&data_dir).unwrap(); + std::fs::write( + data_dir.join("a.py"), + "import os\nimport json\n\nprint(json.dumps({}))\n", + ) + .unwrap(); + + let response = client + .find_unused(make_request(&data_dir.to_string_lossy(), "imports")) + .await + .unwrap(); + + let names: Vec<&str> = response + .candidates + .iter() + .map(|c| c.name.as_str()) + .collect(); + assert!( + names.contains(&"os"), + "unused 'import os' should be flagged" + ); + assert!(!names.contains(&"json"), "'json' is used"); + assert_eq!(response.candidates[0].confidence, "high"); + } + + #[tokio::test] + async fn test_find_unused_end_to_end() { + let (client, temp_dir) = create_test_client().await; + let data_dir = temp_dir.path().join("data"); + std::fs::create_dir(&data_dir).unwrap(); + std::fs::write( + data_dir.join("lib.rs"), + "use std::collections::HashMap;\n\ + pub fn used_helper() -> u32 { 41 }\n\ + fn orphan_helper() -> u32 { 42 }\n", + ) + .unwrap(); + std::fs::write( + data_dir.join("main.rs"), + "fn main() { let _ = crate::used_helper(); }\n", + ) + .unwrap(); + + let index_req = crate::types::IndexRequest { + path: data_dir.to_string_lossy().to_string(), + project: None, + include_patterns: vec![], + exclude_patterns: vec![], + max_file_size: 1_048_576, + }; + client.index_codebase(index_req).await.unwrap(); + + let response = client + .find_unused(make_request(&data_dir.to_string_lossy(), "all")) + .await + .unwrap(); + + assert_eq!(response.files_scanned, 2); + let names: Vec<&str> = response + .candidates + .iter() + .map(|c| c.name.as_str()) + .collect(); + assert!( + names.contains(&"HashMap"), + "unused 'use HashMap' should be flagged, got: {:?}", + names + ); + assert!( + names.contains(&"orphan_helper"), + "unreferenced fn should be flagged, got: {:?}", + names + ); + assert!(!names.contains(&"used_helper"), "used_helper is referenced"); + assert!(!names.contains(&"main"), "entry points are never flagged"); + + let orphan = response + .candidates + .iter() + .find(|c| c.name == "orphan_helper") + .unwrap(); + assert_eq!(orphan.confidence, "medium"); + let import = response + .candidates + .iter() + .find(|c| c.name == "HashMap") + .unwrap(); + assert_eq!(import.kind, SymbolKind::Import); + } + + fn write(dir: &Path, name: &str, content: &str) -> PathBuf { + let p = dir.join(name); + std::fs::write(&p, content).unwrap(); + p + } + + #[tokio::test] + async fn test_cpp_used_class_header_not_flagged() { + let (client, temp_dir) = create_test_client().await; + let data_dir = temp_dir.path().join("data"); + std::fs::create_dir(&data_dir).unwrap(); + write( + &data_dir, + "Kiosk.Notify.h", + "class KioskNotify {\npublic:\n void fire();\n};\n", + ); + write( + &data_dir, + "main.cpp", + "#include \"Kiosk.Notify.h\"\nint main() { KioskNotify n; n.fire(); return 0; }\n", + ); + + let response = client + .find_unused(make_request(&data_dir.to_string_lossy(), "imports")) + .await + .unwrap(); + + assert!( + response.candidates.is_empty(), + "a used header must not be flagged, got: {:?}", + response.candidates + ); + assert_eq!(response.unverifiable_imports, 0); + assert_eq!(response.symbol_rejections.used_elsewhere, 0); + } + + #[tokio::test] + async fn test_cpp_unresolvable_include_unverifiable() { + let (client, temp_dir) = create_test_client().await; + let data_dir = temp_dir.path().join("data"); + std::fs::create_dir(&data_dir).unwrap(); + write( + &data_dir, + "main.cpp", + "#include \"no_such.h\"\nint main() { return 0; }\n", + ); + + let response = client + .find_unused(make_request(&data_dir.to_string_lossy(), "imports")) + .await + .unwrap(); + + assert!( + response.candidates.is_empty(), + "an unresolvable include is skipped, never flagged, got: {:?}", + response.candidates + ); + assert_eq!(response.unverifiable_imports, 1); + assert_eq!(response.unverifiable_import_details.len(), 1); + assert_eq!(response.unverifiable_import_details[0].name, "no_such.h"); + assert!( + response.unverifiable_import_details[0] + .reason + .contains("not resolved") + ); + } + + #[tokio::test] + async fn test_cpp_system_include_unverifiable() { + let (client, temp_dir) = create_test_client().await; + let data_dir = temp_dir.path().join("data"); + std::fs::create_dir(&data_dir).unwrap(); + write( + &data_dir, + "main.cpp", + "#include \nint main() { std::unique_ptr p; return 0; }\n", + ); + + let response = client + .find_unused(make_request(&data_dir.to_string_lossy(), "imports")) + .await + .unwrap(); + + assert!( + response.candidates.is_empty(), + "got: {:?}", + response.candidates + ); + assert_eq!(response.unverifiable_imports, 1); + assert!( + response.unverifiable_import_details[0] + .reason + .contains("system include") + ); + } + + #[tokio::test] + async fn test_cpp_resolved_header_unused_flagged_medium() { + let (client, temp_dir) = create_test_client().await; + let data_dir = temp_dir.path().join("data"); + std::fs::create_dir(&data_dir).unwrap(); + write(&data_dir, "unused.h", "class UnusedThing {};\n"); + write( + &data_dir, + "main.cpp", + "#include \"unused.h\"\nint main() { return 0; }\n", + ); + + let response = client + .find_unused(make_request(&data_dir.to_string_lossy(), "imports")) + .await + .unwrap(); + + assert_eq!( + response.candidates.len(), + 1, + "got: {:?}", + response.candidates + ); + let candidate = &response.candidates[0]; + assert_eq!(candidate.name, "unused.h"); + assert_eq!(candidate.confidence, "medium"); + assert!( + candidate.probe.contains("UnusedThing"), + "the probe must disclose the symbols searched, got: {}", + candidate.probe + ); + } + + #[tokio::test] + async fn test_cpp_single_file_scan_resolves_sibling_header() { + let (client, temp_dir) = create_test_client().await; + let data_dir = temp_dir.path().join("data"); + std::fs::create_dir(&data_dir).unwrap(); + write( + &data_dir, + "Kiosk.Notify.h", + "class KioskNotify {\npublic:\n void fire();\n};\n", + ); + let main_cpp = write( + &data_dir, + "main.cpp", + "#include \"Kiosk.Notify.h\"\nint main() { KioskNotify n; n.fire(); return 0; }\n", + ); + + // Single-file scan: the header is outside the scan set and must be + // resolved on disk next to the including file. + let response = client + .find_unused(make_request(&main_cpp.to_string_lossy(), "imports")) + .await + .unwrap(); + + assert_eq!(response.files_scanned, 1); + assert!( + response.candidates.is_empty(), + "got: {:?}", + response.candidates + ); + assert_eq!(response.unverifiable_imports, 0); + } + + #[test] + fn test_symbol_flagged_when_other_file_only_imports_it() { + let make = |name: &str, content: &str| FileInfo { + path: PathBuf::from(name), + relative_path: name.to_string(), + root_path: "/test".to_string(), + project: None, + extension: Some("rs".to_string()), + language: None, + content: content.to_string(), + hash: "test_hash".to_string(), + }; + let files = vec![ + make("a.rs", "fn ghost_fn() -> u32 { 7 }\n"), + make("b.rs", "use crate::ghost_fn;\n"), + ]; + let provider = Arc::new(crate::relations::HybridRelationsProvider::new(false).unwrap()); + let analyzed = analyze(files, provider); + + let (pending, rejections) = symbol_candidates(&analyzed); + assert!( + pending.iter().any(|(name, _)| name == "ghost_fn"), + "an import elsewhere is not usage; got: {:?}", + pending.iter().map(|(n, _)| n).collect::>() + ); + assert_eq!(rejections.used_elsewhere, 0); + } +} diff --git a/src/client/fs_lock.rs b/src/client/fs_lock.rs index f08e61d..1230d9a 100644 --- a/src/client/fs_lock.rs +++ b/src/client/fs_lock.rs @@ -68,7 +68,15 @@ impl FsLockGuard { // Open/create lock file let file = File::create(&lock_path).context("Failed to create lock file")?; - // Try non-blocking exclusive lock + // Try non-blocking exclusive lock. + // + // fs2 signals lock contention differently per platform: on Unix it's + // EWOULDBLOCK, whose io::ErrorKind is WouldBlock; on Windows it's + // ERROR_LOCK_VIOLATION, which std does not map to WouldBlock at all, so a + // `.kind() == WouldBlock` check silently misses every contended lock on + // Windows and turns "someone else is indexing" into a hard error instead of + // the intended Ok(None). Compare raw OS error codes against fs2's own + // `lock_contended_error()` instead -- that's what it exists for. match file.try_lock_exclusive() { Ok(()) => { tracing::debug!( @@ -81,7 +89,7 @@ impl FsLockGuard { _path: lock_path, })) } - Err(e) if e.kind() == std::io::ErrorKind::WouldBlock => { + Err(e) if e.raw_os_error() == fs2::lock_contended_error().raw_os_error() => { tracing::debug!( "Filesystem lock blocked (another holder) for: {} (lock_file={:?})", normalized_path, @@ -112,10 +120,7 @@ impl FsLockGuard { loop { match Self::try_acquire(normalized_path)? { Some(guard) => { - tracing::info!( - "Acquired filesystem lock after {:?}", - start.elapsed() - ); + tracing::info!("Acquired filesystem lock after {:?}", start.elapsed()); return Ok(Some(guard)); } None => { @@ -214,10 +219,12 @@ mod tests { let lock2 = lock_file_path(path2); let lock1_dup = lock_file_path(path1_dup); - assert_ne!(lock1, lock2, "Different paths should have different lock files"); + assert_ne!( + lock1, lock2, + "Different paths should have different lock files" + ); assert_eq!(lock1, lock1_dup, "Same path should have same lock file"); } -} #[tokio::test] async fn test_concurrent_lock_fails_async() { @@ -225,20 +232,24 @@ mod tests { // Acquire lock in spawn_blocking (simulating what RagClient does) let path1 = path.to_string(); - let guard1 = tokio::task::spawn_blocking(move || { - FsLockGuard::try_acquire(&path1).unwrap() - }).await.unwrap(); - + let guard1 = tokio::task::spawn_blocking(move || FsLockGuard::try_acquire(&path1).unwrap()) + .await + .unwrap(); + assert!(guard1.is_some(), "First lock should succeed"); - + // Hold the guard in this task let _held_guard = guard1.unwrap(); // Try to acquire again from spawn_blocking let path2 = path.to_string(); - let guard2 = tokio::task::spawn_blocking(move || { - FsLockGuard::try_acquire(&path2).unwrap() - }).await.unwrap(); + let guard2 = tokio::task::spawn_blocking(move || FsLockGuard::try_acquire(&path2).unwrap()) + .await + .unwrap(); - assert!(guard2.is_none(), "Second lock should fail because first is held"); + assert!( + guard2.is_none(), + "Second lock should fail because first is held" + ); } +} diff --git a/src/client/git_indexing/mod.rs b/src/client/git_indexing/mod.rs index df496c8..da1ecc4 100644 --- a/src/client/git_indexing/mod.rs +++ b/src/client/git_indexing/mod.rs @@ -246,7 +246,9 @@ where if diff_section.starts_with("Diff:") { let diff_content = diff_section.strip_prefix("Diff:\n").unwrap_or(diff_section); if diff_content.len() > 500 { - format!("{}...", &diff_content[..500]) + // Byte cap: slicing a str at a non-boundary offset panics. + let end = crate::git::floor_char_boundary(diff_content, 500); + format!("{}...", &diff_content[..end]) } else { diff_content.to_string() } diff --git a/src/client/index_lock.rs b/src/client/index_lock.rs index c792321..9d34363 100644 --- a/src/client/index_lock.rs +++ b/src/client/index_lock.rs @@ -7,11 +7,11 @@ use super::fs_lock::FsLockGuard; use crate::types::IndexResponse; use std::collections::HashMap; -use std::sync::atomic::{AtomicBool, Ordering}; use std::sync::Arc; +use std::sync::atomic::{AtomicBool, Ordering}; use std::time::{Duration, Instant}; -use tokio::sync::broadcast; use tokio::sync::RwLock; +use tokio::sync::broadcast; /// Maximum time an indexing operation can run before being considered stale (30 minutes) /// This handles cases where the process crashes or panics without proper cleanup @@ -124,7 +124,9 @@ impl Drop for IndexLockGuard { chunks_created: 0, embeddings_generated: 0, duration_ms: 0, - errors: vec!["Indexing operation was interrupted (panic or early return)".to_string()], + errors: vec![ + "Indexing operation was interrupted (panic or early return)".to_string(), + ], files_updated: 0, files_removed: 0, }; diff --git a/src/client/indexing/mod.rs b/src/client/indexing/mod.rs index be1fd4f..18198d5 100644 --- a/src/client/indexing/mod.rs +++ b/src/client/indexing/mod.rs @@ -1,14 +1,16 @@ use super::RagClient; use crate::embedding::EmbeddingProvider; -use crate::indexer::{CodeChunk, FileWalker}; +use crate::indexer::{CodeChunk, FileInfo, FileWalker}; +use crate::relations::RelationsProvider; +use crate::relations::storage::RelationsStore; use crate::types::{ChunkMetadata, IndexResponse}; use crate::vector_db::VectorDatabase; use anyhow::{Context, Result}; use rayon::prelude::*; use rmcp::{Peer, RoleServer, model::ProgressNotificationParam, model::ProgressToken}; use std::collections::HashMap; -use std::sync::atomic::{AtomicBool, Ordering}; use std::sync::Arc; +use std::sync::atomic::{AtomicBool, Ordering}; use std::time::Instant; use tokio_util::sync::CancellationToken; @@ -22,6 +24,54 @@ macro_rules! check_cancelled { }; } +/// Extract symbol definitions (functions, classes, imports, ...) from the given +/// files and persist them in the relations store. +/// +/// Storage is idempotent per file, so calling this for modified files replaces +/// their old symbols. Best-effort: a failure degrades relations queries but must +/// not fail the indexing run, so it is reported through `errors` instead. +async fn extract_and_store_definitions( + client: &RagClient, + files: &[FileInfo], + root_path: &str, + errors: &mut Vec, +) -> usize { + let provider = client.relations_provider.clone(); + let definitions: Vec<_> = files + .par_iter() + .flat_map(|file| { + provider.extract_definitions(file).unwrap_or_else(|e| { + tracing::debug!( + "Definition extraction failed for {}: {}", + file.relative_path, + e + ); + Vec::new() + }) + }) + .collect(); + + if definitions.is_empty() { + return 0; + } + + match client + .relations_store + .store_definitions(definitions, root_path) + .await + { + Ok(stored) => { + tracing::info!("Stored {} definitions for {} files", stored, files.len()); + stored + } + Err(e) => { + tracing::warn!("Failed to store definitions: {:#}", e); + errors.push(format!("Failed to store definitions: {:#}", e)); + 0 + } + } +} + /// Result of embedding generation with cancellation support struct EmbeddingResult { embeddings: Vec>, @@ -88,11 +138,8 @@ async fn generate_embeddings_with_cancellation( let provider = client.embedding_provider.clone(); let embed_future = tokio::task::spawn_blocking(move || provider.embed_batch(texts)); - match tokio::time::timeout( - std::time::Duration::from_secs(timeout_secs), - embed_future, - ) - .await + match tokio::time::timeout(std::time::Duration::from_secs(timeout_secs), embed_future) + .await { Ok(Ok(Ok(embeddings))) => { batch_embeddings.extend(embeddings); @@ -126,8 +173,8 @@ async fn generate_embeddings_with_cancellation( // Send progress during embedding if let (Some(peer), Some(token)) = (peer, progress_token) { - let progress = - progress_start + ((batch_idx + 1) as f64 / total_batches as f64) * (progress_end - progress_start); + let progress = progress_start + + ((batch_idx + 1) as f64 / total_batches as f64) * (progress_end - progress_start); let _ = peer .notify_progress(ProgressNotificationParam { progress_token: token.clone(), @@ -295,7 +342,10 @@ pub async fn do_index( .iter() .map(|c| c.metadata.clone()) .collect(); - let contents: Vec = successful_chunks.iter().map(|c| c.content.clone()).collect(); + let contents: Vec = successful_chunks + .iter() + .map(|c| c.content.clone()) + .collect(); // Sanity check: ensure all arrays have the same length to prevent RecordBatch errors debug_assert_eq!( @@ -320,6 +370,10 @@ pub async fn do_index( .context("Failed to store embeddings")?; } + // Persist symbol definitions (functions, classes, imports) alongside the + // embeddings so relations queries can be served from the database. + extract_and_store_definitions(client, &files, &path, &mut errors).await; + // Send progress before saving cache if let (Some(peer), Some(token)) = (&peer, &progress_token) { let _ = peer @@ -516,6 +570,9 @@ pub async fn do_incremental_update( if let Err(e) = client.vector_db.delete_by_file(old_file).await { tracing::warn!("Failed to delete embeddings for removed file: {}", e); } + if let Err(e) = client.relations_store.delete_by_file(old_file).await { + tracing::warn!("Failed to delete relations for removed file: {}", e); + } } } @@ -597,7 +654,10 @@ pub async fn do_incremental_update( .iter() .map(|c| c.metadata.clone()) .collect(); - let contents: Vec = successful_chunks.iter().map(|c| c.content.clone()).collect(); + let contents: Vec = successful_chunks + .iter() + .map(|c| c.content.clone()) + .collect(); if !all_embeddings.is_empty() { client @@ -607,6 +667,10 @@ pub async fn do_incremental_update( .context("Failed to store embeddings")?; } + // Refresh stored definitions for the changed files. Storage replaces rows + // per file, so modified files do not accumulate stale symbols. + extract_and_store_definitions(client, &files_to_index, &path, &mut Vec::new()).await; + (all_embeddings.len(), embed_result.errors) } else { (0, vec![]) @@ -703,7 +767,10 @@ pub async fn do_index_smart( match lock_result { IndexLockResult::WaitForResult(mut receiver) => { // Another task in THIS PROCESS is indexing, wait for its result via broadcast - tracing::info!("Waiting for existing indexing operation in this process to complete for: {}", path); + tracing::info!( + "Waiting for existing indexing operation in this process to complete for: {}", + path + ); // Send progress notification if we have a peer if let (Some(peer), Some(token)) = (&peer, &progress_token) { @@ -712,7 +779,9 @@ pub async fn do_index_smart( progress_token: token.clone(), progress: 0.0, total: Some(100.0), - message: Some("Waiting for existing indexing operation to complete...".into()), + message: Some( + "Waiting for existing indexing operation to complete...".into(), + ), }) .await; } @@ -948,7 +1017,7 @@ async fn validate_dirty_flag( /// Inner implementation of smart indexing (called when we have the lock) #[allow(clippy::too_many_arguments)] -async fn do_index_smart_inner( +pub(crate) async fn do_index_smart_inner( client: &RagClient, path: String, project: Option, @@ -994,7 +1063,10 @@ async fn do_index_smart_inner( progress_token: token.clone(), progress: 0.0, total: Some(100.0), - message: Some(format!("Corrupted index detected ({}), clearing...", reason)), + message: Some(format!( + "Corrupted index detected ({}), clearing...", + reason + )), }) .await; } @@ -1044,7 +1116,10 @@ async fn do_index_smart_inner( let mut cache = client.hash_cache.write().await; cache.clear_dirty(&normalized_path); if let Err(e) = cache.save(&client.cache_path) { - tracing::warn!("Failed to save cache after clearing stale dirty flag: {}", e); + tracing::warn!( + "Failed to save cache after clearing stale dirty flag: {}", + e + ); } drop(cache); // Proceed with incremental update @@ -1067,7 +1142,9 @@ async fn do_index_smart_inner( progress_token: token.clone(), progress: 0.0, total: Some(100.0), - message: Some("Index appears complete, clearing stale dirty flag...".into()), + message: Some( + "Index appears complete, clearing stale dirty flag...".into(), + ), }) .await; } @@ -1149,7 +1226,10 @@ async fn do_index_smart_inner( let mut cache = client.hash_cache.write().await; cache.clear_dirty(&normalized_path); if let Err(e) = cache.save(&client.cache_path) { - tracing::warn!("Failed to clear dirty flag after successful indexing: {}", e); + tracing::warn!( + "Failed to clear dirty flag after successful indexing: {}", + e + ); // Don't fail the whole operation for this } tracing::debug!("Cleared dirty flag for: {}", normalized_path); @@ -1177,10 +1257,17 @@ async fn clear_path_data(client: &RagClient, normalized_path: &str) -> Result<() .unwrap_or_default(); drop(cache); - // Delete embeddings for each file + // Delete embeddings and stored relations for each file for file_path in file_paths { if let Err(e) = client.vector_db.delete_by_file(&file_path).await { - tracing::warn!("Failed to delete embeddings for file '{}': {}", file_path, e); + tracing::warn!( + "Failed to delete embeddings for file '{}': {}", + file_path, + e + ); + } + if let Err(e) = client.relations_store.delete_by_file(&file_path).await { + tracing::warn!("Failed to delete relations for file '{}': {}", file_path, e); } } diff --git a/src/client/indexing/tests.rs b/src/client/indexing/tests.rs index 101e17d..a7b4767 100644 --- a/src/client/indexing/tests.rs +++ b/src/client/indexing/tests.rs @@ -980,8 +980,14 @@ async fn test_concurrent_index_same_path_waits_for_result() { // - Other task waits for filesystem lock, then returns (files_indexed = 0 since it waited) // // The important thing is both succeed without errors - assert!(response1.errors.is_empty(), "Task 1 should succeed without errors"); - assert!(response2.errors.is_empty(), "Task 2 should succeed without errors"); + assert!( + response1.errors.is_empty(), + "Task 1 should succeed without errors" + ); + assert!( + response2.errors.is_empty(), + "Task 2 should succeed without errors" + ); // At least one should have done actual indexing let total = response1.files_indexed + response2.files_indexed; @@ -1047,8 +1053,14 @@ async fn test_concurrent_index_different_paths_both_run() { // Both should succeed independently let (result1, result2) = tokio::join!(handle1, handle2); - assert!(result1.unwrap().is_ok(), "First path should index successfully"); - assert!(result2.unwrap().is_ok(), "Second path should index successfully"); + assert!( + result1.unwrap().is_ok(), + "First path should index successfully" + ); + assert!( + result2.unwrap().is_ok(), + "Second path should index successfully" + ); } // ===== Cancellation Tests ===== @@ -1277,10 +1289,16 @@ async fn test_uncancelled_token_completes_normally() { async fn test_cancel_token_cancellation_is_detected() { // Test that our check_cancelled macro works correctly let cancel_token = CancellationToken::new(); - assert!(!cancel_token.is_cancelled(), "Should not be cancelled initially"); + assert!( + !cancel_token.is_cancelled(), + "Should not be cancelled initially" + ); cancel_token.cancel(); - assert!(cancel_token.is_cancelled(), "Should be cancelled after cancel()"); + assert!( + cancel_token.is_cancelled(), + "Should be cancelled after cancel()" + ); } #[tokio::test] @@ -1296,7 +1314,9 @@ async fn test_cancellation_during_embedding_batch() { data_dir.join(format!("file{}.rs", i)), format!( "fn func_{} () {{\n let x = {};\n let y = {};\n println!(\"test\");\n}}", - i, i, i * 2 + i, + i, + i * 2 ), ) .unwrap(); diff --git a/src/client/mod.rs b/src/client/mod.rs index 467dedf..8a111b3 100644 --- a/src/client/mod.rs +++ b/src/client/mod.rs @@ -8,6 +8,7 @@ use crate::config::Config; use crate::embedding::{EmbeddingProvider, FastEmbedManager}; use crate::git_cache::GitCache; use crate::indexer::{CodeChunker, FileInfo, detect_language}; +use crate::relations::storage::{LanceRelationsStore, RelationsStore}; use crate::relations::{ DefinitionResult, HybridRelationsProvider, ReferenceResult, RelationsProvider, }; @@ -37,6 +38,12 @@ pub(crate) use fs_lock::FsLockGuard; mod index_lock; pub(crate) use index_lock::{IndexLockGuard, IndexLockResult, IndexingOperation}; +// read_file/edit_file: single-file read and write-then-reindex operations +mod file_ops; + +// find_unused: unused import and dead-symbol candidate detection +mod find_unused; + /// Main client for interacting with the RAG system /// /// This client provides a high-level API for indexing codebases and performing @@ -87,6 +94,8 @@ pub struct RagClient { pub(crate) indexing_ops: Arc>>, // Relations provider for code navigation (find definition, references, call graph) pub(crate) relations_provider: Arc, + // Persistent store for extracted definitions/references (shares the LanceDB directory) + pub(crate) relations_store: Arc, } impl RagClient { @@ -194,6 +203,14 @@ impl RagClient { .context("Failed to initialize relations provider")?, ); + // Relations store lives in the same LanceDB directory as the embeddings + // table, so one database directory holds everything the index knows. + let relations_store = Arc::new( + LanceRelationsStore::new(config.vector_db.lancedb_path.clone()) + .await + .context("Failed to initialize relations store")?, + ); + Ok(Self { embedding_provider, vector_db, @@ -205,6 +222,7 @@ impl RagClient { config: Arc::new(config), indexing_ops: Arc::new(RwLock::new(HashMap::new())), relations_provider, + relations_store, }) } @@ -222,6 +240,13 @@ impl RagClient { /// Create FileInfo from a file path for relations analysis fn create_file_info(&self, file_path: &str, project: Option) -> Result { + Self::build_file_info(file_path, project) + } + + /// Associated form of create_file_info: it uses no client state, and the + /// include resolver in find_unused needs it inside a spawn_blocking closure + /// that cannot borrow the client. + pub(crate) fn build_file_info(file_path: &str, project: Option) -> Result { use std::path::Path; let path = Path::new(file_path); @@ -236,12 +261,10 @@ impl RagClient { .and_then(|e| e.to_str()) .map(|s| s.to_string()); - let language = extension.as_ref().and_then(|ext| { - detect_language(ext) - }); + let language = extension.as_ref().and_then(|ext| detect_language(ext)); // Compute file hash - use sha2::{Sha256, Digest}; + use sha2::{Digest, Sha256}; let mut hasher = Sha256::new(); hasher.update(content.as_bytes()); let hash = format!("{:x}", hasher.finalize()); @@ -484,6 +507,7 @@ impl RagClient { /// let request = QueryRequest { /// query: "authentication logic".to_string(), /// project: Some("my-project".to_string()), + /// path: None, /// limit: 10, /// min_score: 0.7, /// hybrid: true, @@ -660,22 +684,32 @@ impl RagClient { .await .context("Failed to get statistics")?; + // Relations counts are best-effort: statistics must not fail just + // because the relations tables are unreadable. + let relations_stats = self.relations_store.get_stats().await.unwrap_or_else(|e| { + tracing::warn!("Failed to get relations statistics: {:#}", e); + Default::default() + }); + let language_breakdown = stats .language_breakdown .into_iter() - .map(|(language, count)| LanguageStats { - language, - file_count: count, - chunk_count: count, + .map(|entry| LanguageStats { + language: entry.language, + file_count: entry.file_count, + chunk_count: entry.chunk_count, }) .collect(); Ok(StatisticsResponse { - total_files: stats.total_points, - total_chunks: stats.total_vectors, + total_files: stats.total_files, + total_chunks: stats.total_points, total_embeddings: stats.total_vectors, - database_size_bytes: 0, + database_size_bytes: stats.database_size_bytes, language_breakdown, + total_definitions: relations_stats.definition_count, + total_references: relations_stats.reference_count, + files_with_definitions: relations_stats.files_with_definitions, }) } @@ -716,6 +750,11 @@ impl RagClient { tracing::warn!("Failed to save cleared git cache: {}", e); } + // Also clear stored definitions/references + if let Err(e) = self.relations_store.clear().await { + tracing::warn!("Failed to clear relations store: {}", e); + } + if let Err(e) = self .vector_db .initialize(self.embedding_provider.dimension()) @@ -810,7 +849,136 @@ impl RagClient { /// # Returns /// /// A response containing the definition if found, along with precision info - pub async fn find_definition(&self, request: FindDefinitionRequest) -> Result { + /// The identifier token sitting at a 1-based `line` / 0-based `column`. + /// + /// Symbol resolution used to work purely by range containment: take the first + /// definition whose start..end spans the line. On a call site that is always the + /// ENCLOSING function, so asking about `SendNotification2(...)` inside + /// `ProcessDeviceNotifyCache` resolved to `ProcessDeviceNotifyCache`. Reading the + /// actual token under the cursor is what the caller meant by "the symbol here". + fn identifier_at(content: &str, line: usize, column: usize) -> Option { + let text = content.lines().nth(line.checked_sub(1)?)?; + let bytes = text.as_bytes(); + let is_ident = |b: u8| b.is_ascii_alphanumeric() || b == b'_'; + + if bytes.is_empty() { + return None; + } + // Clamp into the line; a column past the end just anchors at the last character. + let mut idx = column.min(bytes.len() - 1); + // A cursor resting just after a token (or on its opening delimiter) should still + // resolve that token. + if !is_ident(bytes[idx]) && idx > 0 && is_ident(bytes[idx - 1]) { + idx -= 1; + } + if !is_ident(bytes[idx]) { + return None; + } + + let mut start = idx; + while start > 0 && is_ident(bytes[start - 1]) { + start -= 1; + } + let mut end = idx; + while end + 1 < bytes.len() && is_ident(bytes[end + 1]) { + end += 1; + } + Some(text[start..=end].to_string()) + } + + /// Resolve which definition a cursor position refers to. + /// + /// Preference order: + /// 1. the identifier under the cursor, if it names a definition in this file + /// 2. the INNERMOST definition whose range contains the line + /// + /// Step 2 used to be "the first definition that contains the line", which picked + /// whichever happened to be earliest in extraction order -- normally the enclosing + /// class or function rather than the nested one being asked about. + fn resolve_symbol_at<'a>( + definitions: &'a [crate::relations::Definition], + content: &str, + line: usize, + column: usize, + callable_only: bool, + ) -> Option<&'a crate::relations::Definition> { + let is_candidate = |def: &crate::relations::Definition| { + !callable_only + || matches!( + def.symbol_id.kind, + crate::relations::SymbolKind::Function | crate::relations::SymbolKind::Method + ) + }; + + if let Some(name) = Self::identifier_at(content, line, column) { + // Prefer a real definition over an import binding of the same name: + // `use foo::helper;` plus `fn helper()` in one file must resolve to the + // function. Import defs span one line, so on span alone they would win. + let exact = definitions + .iter() + .filter(|d| d.symbol_id.name == name && is_candidate(d)) + .min_by_key(|d| { + ( + d.symbol_id.kind == crate::relations::SymbolKind::Import, + d.end_line.saturating_sub(d.symbol_id.start_line), + ) + }); + if exact.is_some() { + return exact; + } + } + + definitions + .iter() + .filter(|d| is_candidate(d) && line >= d.symbol_id.start_line && line <= d.end_line) + .min_by_key(|d| d.end_line.saturating_sub(d.symbol_id.start_line)) + } + + /// Files that plausibly mention `symbol`, newest-ranked first. + /// + /// References live wherever the identifier appears, which is generally NOT the file + /// that defines it -- the previous implementation only ever scanned the definition's + /// own file, so any cross-file reference was invisible. Rather than parse the whole + /// corpus, shortlist with keyword search: a reference must contain the literal token, + /// so BM25 surfaces exactly the right files and tree-sitter only runs on those. + async fn files_mentioning( + &self, + symbol: &str, + project: Option, + limit: usize, + ) -> Result> { + let embedding = self + .embedding_provider + .embed_batch(vec![symbol.to_string()]) + .context("Failed to embed symbol name")? + .into_iter() + .next() + .ok_or_else(|| anyhow::anyhow!("No embedding generated for symbol"))?; + + let results = self + .vector_db + .search(embedding, symbol, limit, 0.0, project, None, true) + .await + .context("Failed to search for candidate files")?; + + let mut seen = std::collections::HashSet::new(); + let mut files = Vec::new(); + for r in results { + let full = match &r.root_path { + Some(root) => std::path::Path::new(root).join(&r.file_path), + None => std::path::PathBuf::from(&r.file_path), + }; + if seen.insert(full.clone()) { + files.push(full); + } + } + Ok(files) + } + + pub async fn find_definition( + &self, + request: FindDefinitionRequest, + ) -> Result { let start = Instant::now(); // Validate request @@ -830,13 +998,14 @@ impl RagClient { .context("Failed to extract definitions")?; // Find the definition at the requested position - let definition = definitions.into_iter().find(|def| { - request.line >= def.symbol_id.start_line - && request.line <= def.end_line - && (request.column == 0 || request.column >= def.symbol_id.start_col) - }); - - let result = definition.map(|def| DefinitionResult::from(&def)); + let result = Self::resolve_symbol_at( + &definitions, + &file_info.content, + request.line, + request.column, + false, + ) + .map(DefinitionResult::from); Ok(FindDefinitionResponse { definition: result, @@ -857,7 +1026,10 @@ impl RagClient { /// # Returns /// /// A response containing the list of references found - pub async fn find_references(&self, request: FindReferencesRequest) -> Result { + pub async fn find_references( + &self, + request: FindReferencesRequest, + ) -> Result { let start = Instant::now(); // Validate request @@ -877,11 +1049,13 @@ impl RagClient { .context("Failed to extract definitions")?; // Find the symbol at the requested position - let target_symbol = definitions.iter().find(|def| { - request.line >= def.symbol_id.start_line - && request.line <= def.end_line - && (request.column == 0 || request.column >= def.symbol_id.start_col) - }); + let target_symbol = Self::resolve_symbol_at( + &definitions, + &file_info.content, + request.line, + request.column, + false, + ); let symbol_name = target_symbol.map(|def| def.symbol_id.name.clone()); @@ -898,32 +1072,81 @@ impl RagClient { let symbol_name_str = symbol_name.clone().unwrap(); - // Build symbol index from definitions + // Index ONLY the target symbol. ReferenceFinder matches identifiers against this + // map, so restricting it keeps the scan of other files cheap and on-topic. + let target_defs: Vec = definitions + .iter() + .filter(|d| d.symbol_id.name == symbol_name_str) + .cloned() + .collect(); let mut symbol_index: std::collections::HashMap> = std::collections::HashMap::new(); - for def in definitions { - symbol_index - .entry(def.symbol_id.name.clone()) - .or_default() - .push(def); + if !target_defs.is_empty() { + symbol_index.insert(symbol_name_str.clone(), target_defs); } - // Find references in the same file - let references = self - .relations_provider - .extract_references(&file_info, &symbol_index) - .context("Failed to extract references")?; + // Scan the defining file plus every other file the index says mentions the symbol. + // Searching only the defining file is why this returned nothing for anything called + // from elsewhere, which is the normal case for a public API. + let mut scan_targets: Vec = vec![file_info.path.clone()]; + match self + .files_mentioning( + &symbol_name_str, + request.project.clone(), + request.limit.max(20), + ) + .await + { + Ok(found) => { + for f in found { + if !scan_targets.iter().any(|p| p == &f) { + scan_targets.push(f); + } + } + } + Err(e) => tracing::warn!( + "Candidate lookup failed, scanning defining file only: {}", + e + ), + } - // Filter to references matching our target symbol - let matching_refs: Vec = references - .iter() - .filter(|r| { - // Check if this reference points to our target symbol - r.target_symbol_id.contains(&symbol_name_str) - }) - .take(request.limit) - .map(|r| ReferenceResult::from(r)) - .collect(); + let mut matching_refs: Vec = Vec::new(); + for target in &scan_targets { + if matching_refs.len() >= request.limit { + break; + } + let scan_info = if target == &file_info.path { + file_info.clone() + } else { + match self.create_file_info(&target.to_string_lossy(), request.project.clone()) { + Ok(fi) => fi, + Err(e) => { + tracing::debug!("Skipping unreadable candidate {:?}: {}", target, e); + continue; + } + } + }; + + let references = match self + .relations_provider + .extract_references(&scan_info, &symbol_index) + { + Ok(r) => r, + Err(e) => { + tracing::debug!("Reference extraction failed for {:?}: {}", target, e); + continue; + } + }; + + for r in references.iter() { + if matching_refs.len() >= request.limit { + break; + } + if r.target_symbol_id.contains(&symbol_name_str) { + matching_refs.push(ReferenceResult::from(r)); + } + } + } let total_count = matching_refs.len(); @@ -936,6 +1159,72 @@ impl RagClient { }) } + /// Control-flow and cast keywords that are followed by a parenthesis but are not + /// calls. Without this, `if (` is reported as a callee whenever some file happens + /// to carry a bogus definition of that name. + fn is_call_like_keyword(name: &str) -> bool { + matches!( + name, + "if" | "for" + | "while" + | "switch" + | "catch" + | "return" + | "sizeof" + | "do" + | "else" + | "new" + | "delete" + | "throw" + | "defined" + | "static_cast" + | "dynamic_cast" + | "reinterpret_cast" + | "const_cast" + ) + } + /// Identifiers that appear immediately before an opening parenthesis inside the + /// given 1-based line span -- that is, plausible call sites. + /// + /// Used to widen the callee symbol index beyond the defining file. Deliberately + /// crude: over-reporting costs one extra lookup, under-reporting loses a callee. + fn call_identifiers_in_span(content: &str, start_line: usize, end_line: usize) -> Vec { + let mut out: Vec = Vec::new(); + let mut seen = std::collections::HashSet::new(); + for (idx, line) in content.lines().enumerate() { + let n = idx + 1; + if n < start_line || n > end_line { + continue; + } + let bytes = line.as_bytes(); + let mut i = 0usize; + while i < bytes.len() { + if bytes[i].is_ascii_alphabetic() || bytes[i] == b'_' { + let start = i; + while i < bytes.len() && (bytes[i].is_ascii_alphanumeric() || bytes[i] == b'_') + { + i += 1; + } + let mut j = i; + while j < bytes.len() && bytes[j].is_ascii_whitespace() { + j += 1; + } + if j < bytes.len() && bytes[j] == b'(' { + let name = &line[start..i]; + if name.len() > 1 + && !Self::is_call_like_keyword(name) + && seen.insert(name.to_string()) + { + out.push(name.to_string()); + } + } + } else { + i += 1; + } + } + } + out + } /// Get the call graph for a function at a given file location /// /// This method returns the callers (incoming calls) and callees (outgoing calls) @@ -948,7 +1237,10 @@ impl RagClient { /// # Returns /// /// A response containing the root symbol and its call graph - pub async fn get_call_graph(&self, request: GetCallGraphRequest) -> Result { + pub async fn get_call_graph( + &self, + request: GetCallGraphRequest, + ) -> Result { let start = Instant::now(); // Validate request @@ -968,15 +1260,13 @@ impl RagClient { .context("Failed to extract definitions")?; // Find the function at the requested position - let target_function = definitions.iter().find(|def| { - // Only consider functions/methods - matches!( - def.symbol_id.kind, - crate::relations::SymbolKind::Function | crate::relations::SymbolKind::Method - ) && request.line >= def.symbol_id.start_line - && request.line <= def.end_line - && (request.column == 0 || request.column >= def.symbol_id.start_col) - }); + let target_function = Self::resolve_symbol_at( + &definitions, + &file_info.content, + request.line, + request.column, + true, + ); // If no function found at position, return empty result let root_symbol = match target_function { @@ -1011,39 +1301,169 @@ impl RagClient { .push(def.clone()); } - // Find references in the same file to identify callers + // References in this file, used for the callee side. + // + // The index handed to extract_references decides what is even visible: a + // reference is emitted only when its identifier is a key in that map. Building + // it from this file alone therefore drops every callee defined in another + // translation unit, and TForm1 alone is spread over five .cpp files. So widen + // it first with definitions of the names actually called inside the target + // span. Bounded on both axes so a large function cannot fan out forever. + const MAX_CALLEE_PROBES: usize = 40; + const FILES_PER_NAME: usize = 5; + + let mut callee_index = symbol_index.clone(); + let mut probed_files: std::collections::HashSet = + std::collections::HashSet::from([file_info.path.clone()]); + + let called_names = Self::call_identifiers_in_span( + &file_info.content, + root_symbol.start_line, + root_symbol.end_line, + ); + + for name in called_names + .iter() + .filter(|n| !symbol_index.contains_key(*n)) + .take(MAX_CALLEE_PROBES) + { + let candidates = match self + .files_mentioning(name, request.project.clone(), FILES_PER_NAME) + .await + { + Ok(f) => f, + Err(e) => { + tracing::debug!("Callee candidate lookup failed for {}: {}", name, e); + continue; + } + }; + for f in candidates { + if !probed_files.insert(f.clone()) { + continue; + } + let fi = match self.create_file_info(&f.to_string_lossy(), request.project.clone()) + { + Ok(fi) => fi, + Err(e) => { + tracing::debug!("Skipping unreadable callee candidate {:?}: {}", f, e); + continue; + } + }; + match self.relations_provider.extract_definitions(&fi) { + Ok(defs) => { + for d in defs { + callee_index + .entry(d.symbol_id.name.clone()) + .or_default() + .push(d); + } + } + Err(e) => tracing::debug!("Definition extraction failed for {:?}: {}", f, e), + } + } + } + let references = self .relations_provider - .extract_references(&file_info, &symbol_index) + .extract_references(&file_info, &callee_index) .context("Failed to extract references")?; - // Find callers (references with Call kind pointing to our function) + // Callers can live anywhere, so scan the defining file plus every file the index + // says mentions the function. Restricting this to the defining file is why the + // caller list came back empty for anything with an external call site. + let caller_index: std::collections::HashMap> = + std::collections::HashMap::from([( + function_name.clone(), + definitions + .iter() + .filter(|d| d.symbol_id.name == function_name) + .cloned() + .collect(), + )]); + + let mut scan_targets: Vec = vec![file_info.path.clone()]; + match self + .files_mentioning(&function_name, request.project.clone(), 20) + .await + { + Ok(found) => { + for f in found { + if !scan_targets.iter().any(|p| p == &f) { + scan_targets.push(f); + } + } + } + Err(e) => tracing::warn!( + "Candidate lookup failed, scanning defining file only: {}", + e + ), + } + let mut seen_callers = std::collections::HashSet::new(); - let callers: Vec = references - .iter() - .filter(|r| { + let mut callers: Vec = Vec::new(); + + for target in &scan_targets { + let (scan_info, scan_defs) = if target == &file_info.path { + (file_info.clone(), definitions.clone()) + } else { + match self.create_file_info(&target.to_string_lossy(), request.project.clone()) { + Ok(fi) => { + let defs = self + .relations_provider + .extract_definitions(&fi) + .unwrap_or_default(); + (fi, defs) + } + Err(e) => { + tracing::debug!("Skipping unreadable candidate {:?}: {}", target, e); + continue; + } + } + }; + + let refs = match self + .relations_provider + .extract_references(&scan_info, &caller_index) + { + Ok(r) => r, + Err(e) => { + tracing::debug!("Reference extraction failed for {:?}: {}", target, e); + continue; + } + }; + + for r in refs.iter().filter(|r| { r.reference_kind == crate::relations::ReferenceKind::Call && r.target_symbol_id.contains(&function_name) - }) - .filter_map(|r| { - // Try to find which function contains this call - definitions.iter().find(|def| { - matches!( - def.symbol_id.kind, - crate::relations::SymbolKind::Function | crate::relations::SymbolKind::Method - ) && r.start_line >= def.symbol_id.start_line - && r.start_line <= def.end_line - }) - }) - .filter(|def| seen_callers.insert(def.symbol_id.name.clone())) - .map(|def| crate::relations::CallGraphNode { - name: def.symbol_id.name.clone(), - kind: def.symbol_id.kind.clone(), - file_path: request.file_path.clone(), - line: def.symbol_id.start_line, - children: Vec::new(), - }) - .collect(); + }) { + // Attribute the call to the innermost function containing it, in the file + // the call was actually found in. + let enclosing = scan_defs + .iter() + .filter(|def| { + matches!( + def.symbol_id.kind, + crate::relations::SymbolKind::Function + | crate::relations::SymbolKind::Method + ) && r.start_line >= def.symbol_id.start_line + && r.start_line <= def.end_line + }) + .min_by_key(|def| def.end_line.saturating_sub(def.symbol_id.start_line)); + + if let Some(def) = enclosing + && seen_callers + .insert((scan_info.relative_path.clone(), def.symbol_id.name.clone())) + { + callers.push(crate::relations::CallGraphNode { + name: def.symbol_id.name.clone(), + kind: def.symbol_id.kind.clone(), + file_path: scan_info.relative_path.clone(), + line: def.symbol_id.start_line, + children: Vec::new(), + }); + } + } + } // Find callees (calls made from within our function) let target_func = target_function.unwrap(); @@ -1056,23 +1476,36 @@ impl RagClient { && r.start_line <= target_func.end_line }) .filter_map(|r| { - // Extract the called function name from target_symbol_id - let parts: Vec<&str> = r.target_symbol_id.split(':').collect(); - if parts.len() >= 2 { - Some(parts[1].to_string()) - } else { - None - } + // Extract the called function name from target_symbol_id. + // target_symbol_id is a Definition id -- `def:::` -- + // NOT a SymbolId id (`:::`). Parsing it with the + // wrong layout, or with a forward split that yields the file path, is why + // callees were always empty and assumed unimplemented. + crate::relations::Definition::name_from_storage_id(&r.target_symbol_id) + .map(|s| s.to_string()) }) + .filter(|name| !Self::is_call_like_keyword(name)) .filter(|name| seen_callees.insert(name.clone())) .filter_map(|name| { - // Find the definition of the called function - symbol_index.get(&name).and_then(|defs| defs.first()).cloned() + // Resolve against the widened index so a callee defined in another + // translation unit still resolves to a definition. Skip past import + // bindings: the callee should be shown at its real definition, not at + // the `use`/`import` line that pulled it into this file. + callee_index + .get(&name) + .and_then(|defs| { + defs.iter() + .find(|d| d.symbol_id.kind != crate::relations::SymbolKind::Import) + .or_else(|| defs.first()) + }) + .cloned() }) .map(|def| crate::relations::CallGraphNode { name: def.symbol_id.name.clone(), kind: def.symbol_id.kind.clone(), - file_path: request.file_path.clone(), + // The definition own file, not the requested one: a cross-TU callee + // does not live in request.file_path. + file_path: def.symbol_id.file_path.clone(), line: def.symbol_id.start_line, children: Vec::new(), }) @@ -1086,6 +1519,55 @@ impl RagClient { duration_ms: start.elapsed().as_millis() as u64, }) } + + /// List every symbol defined in a single file. + /// + /// Returns definitions only -- name, kind, line span, signature -- and never chunk + /// content, so enumerating a large file stays cheap. This is the enumeration + /// primitive the other tools lack: query_codebase and search_by_filters are + /// relevance-ranked with a limit, and find_definition / find_references need a + /// position the caller already has. + pub async fn list_symbols(&self, request: ListSymbolsRequest) -> Result { + let start = Instant::now(); + + request.validate().map_err(|e| anyhow::anyhow!(e))?; + + let file_info = self.create_file_info(&request.file_path, request.project.clone())?; + let language = file_info.language.as_deref().unwrap_or("Unknown"); + let precision = self.relations_provider.precision_level(language); + + let (definitions, skipped) = self + .relations_provider + .extract_definitions_reporting(&file_info) + .context("Failed to extract definitions")?; + + let wanted: Vec = request.kinds.iter().map(|k| k.to_lowercase()).collect(); + let mut symbols: Vec = definitions + .iter() + .filter(|d| { + wanted.is_empty() + || wanted.contains(&format!("{:?}", d.symbol_id.kind).to_lowercase()) + }) + .map(|d| crate::relations::SymbolInfo { + name: d.symbol_id.name.clone(), + kind: d.symbol_id.kind.clone(), + file_path: file_info.relative_path.clone(), + start_line: d.symbol_id.start_line, + end_line: d.end_line, + signature: d.signature.clone(), + }) + .collect(); + symbols.sort_by_key(|s| s.start_line); + + Ok(ListSymbolsResponse { + file_path: file_info.relative_path.clone(), + total_count: symbols.len(), + symbols, + precision: format!("{:?}", precision).to_lowercase(), + skipped, + duration_ms: start.elapsed().as_millis() as u64, + }) + } } // Indexing operations module diff --git a/src/client/tests.rs b/src/client/tests.rs index d868f13..1571917 100644 --- a/src/client/tests.rs +++ b/src/client/tests.rs @@ -78,7 +78,9 @@ fn test_normalize_path_absolute() { let result = RagClient::normalize_path(&path); assert!(result.is_ok()); let normalized = result.unwrap(); - assert!(normalized.starts_with('/')); + // `starts_with('/')` doesn't hold on Windows, where canonicalize() yields + // `\\?\C:\...`; `Path::is_absolute()` is the real, cross-platform invariant. + assert!(std::path::Path::new(&normalized).is_absolute()); } // ===== index_codebase Tests ===== @@ -478,9 +480,17 @@ async fn test_search_with_filters_language_filter() { // Create files in different languages let data_dir = temp_dir.path().join("data"); std::fs::create_dir(&data_dir).unwrap(); - std::fs::write(data_dir.join("main.rs"), "fn main() { println!(\"Hello\"); }").unwrap(); + std::fs::write( + data_dir.join("main.rs"), + "fn main() { println!(\"Hello\"); }", + ) + .unwrap(); std::fs::write(data_dir.join("main.py"), "def main(): print('Hello')").unwrap(); - std::fs::write(data_dir.join("main.js"), "function main() { console.log('Hello'); }").unwrap(); + std::fs::write( + data_dir.join("main.js"), + "function main() { console.log('Hello'); }", + ) + .unwrap(); let index_req = IndexRequest { path: data_dir.to_string_lossy().to_string(), @@ -528,7 +538,11 @@ async fn test_search_with_filters_path_pattern() { std::fs::create_dir_all(&src_dir).unwrap(); std::fs::create_dir_all(&tests_dir).unwrap(); - std::fs::write(src_dir.join("lib.rs"), "pub fn add(a: i32, b: i32) -> i32 { a + b }").unwrap(); + std::fs::write( + src_dir.join("lib.rs"), + "pub fn add(a: i32, b: i32) -> i32 { a + b }", + ) + .unwrap(); std::fs::write( tests_dir.join("test_lib.rs"), "fn test_add() { assert_eq!(add(1, 2), 3); }", @@ -560,10 +574,13 @@ async fn test_search_with_filters_path_pattern() { assert!(result.is_ok()); let response = result.unwrap(); - // All results should be from src directory + // All results should be from src directory. Compare with '/'-normalized + // separators since `file_path` uses the OS-native separator, which is '\' + // on Windows. for result in &response.results { + let normalized = result.file_path.replace('\\', "/"); assert!( - result.file_path.contains("src/") || result.file_path.starts_with("src/"), + normalized.contains("src/") || normalized.starts_with("src/"), "Expected path to contain src/, got: {}", result.file_path ); @@ -627,15 +644,18 @@ async fn test_search_with_filters_combined_filters() { assert!(result.is_ok()); let response = result.unwrap(); - // All results should be Rust files in src directory + // All results should be Rust files in src directory. Compare with + // '/'-normalized separators since `file_path` uses the OS-native separator, + // which is '\' on Windows. for result in &response.results { assert!( result.file_path.ends_with(".rs"), "Expected .rs file, got: {}", result.file_path ); + let normalized = result.file_path.replace('\\', "/"); assert!( - result.file_path.contains("src/") || result.file_path.starts_with("src/"), + normalized.contains("src/") || normalized.starts_with("src/"), "Expected path to contain src/, got: {}", result.file_path ); @@ -998,7 +1018,11 @@ async fn test_index_lock_prevents_duplicate_indexing() { // Create data to index let data_dir = temp_dir.path().join("data"); std::fs::create_dir(&data_dir).unwrap(); - std::fs::write(data_dir.join("test.rs"), "fn main() { println!(\"test\"); }").unwrap(); + std::fs::write( + data_dir.join("test.rs"), + "fn main() { println!(\"test\"); }", + ) + .unwrap(); let path = data_dir.to_string_lossy().to_string(); @@ -1014,7 +1038,10 @@ async fn test_index_lock_prevents_duplicate_indexing() { // With cross-process locking, this could be WaitForResult (same process, in-memory) // or WaitForFilesystemLock (different process holding filesystem lock) assert!( - matches!(lock_result2, IndexLockResult::WaitForResult(_) | IndexLockResult::WaitForFilesystemLock(_)), + matches!( + lock_result2, + IndexLockResult::WaitForResult(_) | IndexLockResult::WaitForFilesystemLock(_) + ), "Second call should wait for the first operation (got: {:?})", match &lock_result2 { IndexLockResult::Acquired(_) => "Acquired", @@ -1134,7 +1161,10 @@ async fn test_index_lock_path_normalization() { // Both WaitForResult and WaitForFilesystemLock indicate the lock is shared let lock_result2 = client.try_acquire_index_lock(&path2).await.unwrap(); assert!( - matches!(lock_result2, IndexLockResult::WaitForResult(_) | IndexLockResult::WaitForFilesystemLock(_)), + matches!( + lock_result2, + IndexLockResult::WaitForResult(_) | IndexLockResult::WaitForFilesystemLock(_) + ), "Equivalent paths should share the same lock" ); @@ -1276,12 +1306,21 @@ async fn test_concurrent_index_calls_share_result() { // waits for filesystem lock then returns immediately (files_indexed = 0) // // The important thing is both succeed without errors - assert!(resp1.errors.is_empty(), "Task 1 should succeed without errors"); - assert!(resp2.errors.is_empty(), "Task 2 should succeed without errors"); + assert!( + resp1.errors.is_empty(), + "Task 1 should succeed without errors" + ); + assert!( + resp2.errors.is_empty(), + "Task 2 should succeed without errors" + ); // At least one should have done the actual indexing let total_indexed = resp1.files_indexed + resp2.files_indexed; - assert!(total_indexed >= 1, "At least one task should have indexed files"); + assert!( + total_indexed >= 1, + "At least one task should have indexed files" + ); } #[tokio::test] @@ -1388,3 +1427,110 @@ async fn test_index_lock_can_reacquire_after_drop_without_release() { guard.release().await; } } + +// ===== Regression tests for the get_call_graph callee path ===== + +#[test] +fn callee_fix_definition_storage_id_parsing() { + // Reference::target_symbol_id holds a DEFINITION id -- `def:::` + // (Definition::to_storage_id) -- NOT a SymbolId id, which is + // `:::`. Callees stayed empty because the name was parsed + // with the wrong layout. Fields are taken from the right so a Windows drive-letter + // colon in the path cannot shift them. + use crate::relations::Definition; + + let plain = "def:Unit1.cpp:WndProc:16359"; + assert_eq!(Definition::name_from_storage_id(plain), Some("WndProc")); + + let drive = r"def:D:\Work\nft\PPSKiosk\Unit1.cpp:WndProc:16359"; + assert_eq!(Definition::name_from_storage_id(drive), Some("WndProc")); + + // The SymbolId parser cannot read this layout -- it expects a trailing column and + // would try to parse the name as a line number. This is the original defect. + assert!( + crate::relations::SymbolId::from_storage_id(plain) + .map(|s| s.name) + .as_deref() + != Some("WndProc"), + "the SymbolId parser must not be used for Definition ids" + ); + + // Malformed input is rejected rather than guessed at. + assert_eq!( + Definition::name_from_storage_id("Unit1.cpp:WndProc:16359"), + None + ); + assert_eq!(Definition::name_from_storage_id("def:nocolons"), None); +} + +#[test] +fn callee_fix_call_identifiers_in_span() { + let src = concat!( + "void __fastcall TForm1::WndProc(TMessage& msg)\n", + "{\n", + " EnterServiceMenu();\n", + " if (flag) { CheckTerminalState(); }\n", + " int total = alpha + beta;\n", + "}\n", + "void TForm1::Other() { NotInSpan(); }\n", + ); + + let names = RagClient::call_identifiers_in_span(src, 1, 6); + assert!(names.contains(&"EnterServiceMenu".to_string())); + assert!(names.contains(&"CheckTerminalState".to_string())); + + // Plain operands are not call sites. + assert!(!names.contains(&"total".to_string())); + assert!(!names.contains(&"alpha".to_string())); + + // The span bound is honoured in both directions. + assert!(!names.contains(&"NotInSpan".to_string())); + let tail = RagClient::call_identifiers_in_span(src, 7, 7); + assert!(tail.contains(&"NotInSpan".to_string())); + assert!(!tail.contains(&"EnterServiceMenu".to_string())); +} + +/// Smoke test for list_symbols against a real source tree. +/// +/// Skipped unless PROJECT_RAG_SMOKE_FILE is set, because it depends on a file outside +/// this repository. list_symbols touches no vector DB, so this exercises the real +/// extraction path without an index. +#[tokio::test] +async fn list_symbols_smoke_on_real_file() { + let path = match std::env::var("PROJECT_RAG_SMOKE_FILE") { + Ok(p) => p, + Err(_) => return, + }; + let (client, _tmp) = create_test_client().await; + let resp = client + .list_symbols(ListSymbolsRequest { + file_path: path, + project: None, + kinds: Vec::new(), + }) + .await + .expect("list_symbols should succeed"); + + eprintln!( + "SMOKE total_count={} precision={}", + resp.total_count, resp.precision + ); + for s in resp.symbols.iter().take(12) { + eprintln!("SMOKE {:>6} {:?} {}", s.start_line, s.kind, s.name); + } + for want in [ + "WndProc", + "BindCommands", + "AttachPinPadEventHandlers", + "IsSmartCardPresent", + ] { + let hit = resp.symbols.iter().find(|s| s.name == want); + eprintln!( + "SMOKE want {:<26} -> {:?}", + want, + hit.map(|s| s.start_line) + ); + } + + assert!(resp.total_count > 0, "expected at least one symbol"); +} diff --git a/src/git/chunker.rs b/src/git/chunker.rs index ef534e5..98a1b0c 100644 --- a/src/git/chunker.rs +++ b/src/git/chunker.rs @@ -66,7 +66,9 @@ impl CommitChunker { // Truncate if too long if content.len() > self.max_content_length { - content.truncate(self.max_content_length); + // Byte cap: floor to a character boundary before truncating. + let end = crate::git::floor_char_boundary(&content, self.max_content_length); + content.truncate(end); content.push_str("\n\n[... content truncated for embedding ...]"); } @@ -126,6 +128,28 @@ mod tests { } } + #[test] + fn test_commit_to_chunk_truncates_multibyte_content_without_panicking() { + // Regression: the length cap was applied with String::truncate on a + // raw byte offset, which panics when that offset lands inside a + // multi-byte character. A diff of Cyrillic text reproduces it. + let mut commit = create_test_commit(); + commit.diff_content = "привет мир ".repeat(2000); + + let chunker = CommitChunker::with_max_length(6000); + let chunk = chunker + .commit_to_chunk(&commit, "/repo/path", None) + .expect("Should convert commit to chunk"); + + assert!( + chunk.content.contains("content truncated"), + "content should have been truncated" + ); + // Surviving this assertion at all proves the cut landed on a + // character boundary: an invalid cut would have panicked above. + assert!(chunk.content.len() < commit.diff_content.len()); + } + #[test] fn test_commit_to_chunk() { let chunker = CommitChunker::new(); diff --git a/src/git/mod.rs b/src/git/mod.rs index 89f4d25..468e5fb 100644 --- a/src/git/mod.rs +++ b/src/git/mod.rs @@ -10,3 +10,65 @@ pub mod walker; pub use chunker::CommitChunker; pub use walker::GitWalker; + +/// Largest index `<= max` that lies on a UTF-8 character boundary in `s`. +/// +/// `String::truncate` and `&s[..n]` take **byte** offsets and panic unless the +/// offset falls on a character boundary. Commit messages and diffs routinely +/// carry non-ASCII text, so every byte-length cap applied to them has to be +/// floored through here first. +pub fn floor_char_boundary(s: &str, max: usize) -> usize { + if max >= s.len() { + return s.len(); + } + let mut end = max; + while end > 0 && !s.is_char_boundary(end) { + end -= 1; + } + end +} + +#[cfg(test)] +mod tests { + use super::floor_char_boundary; + + #[test] + fn floors_offsets_onto_character_boundaries() { + // Each Cyrillic character below is two bytes in UTF-8. + let s = "аб"; + assert_eq!(s.len(), 4, "test string should be 4 bytes"); + assert_eq!(floor_char_boundary(s, 0), 0); + assert_eq!( + floor_char_boundary(s, 1), + 0, + "offset 1 splits the first char" + ); + assert_eq!(floor_char_boundary(s, 2), 2); + assert_eq!( + floor_char_boundary(s, 3), + 2, + "offset 3 splits the second char" + ); + assert_eq!(floor_char_boundary(s, 4), 4); + } + + #[test] + fn clamps_offsets_past_the_end() { + assert_eq!(floor_char_boundary("abc", 99), 3); + assert_eq!(floor_char_boundary("", 5), 0); + } + + #[test] + fn every_floored_offset_is_safe_to_truncate_at() { + // Regression guard for the panic this helper exists to prevent: + // String::truncate asserts is_char_boundary, so a raw byte cap + // crashed on any text with multi-byte characters. + let s = "программа mixed with ASCII — and more"; + for i in 0..=s.len() { + let mut owned = s.to_string(); + let end = floor_char_boundary(s, i); + owned.truncate(end); + assert!(owned.len() <= i); + } + } +} diff --git a/src/git/walker.rs b/src/git/walker.rs index 4528581..daacb48 100644 --- a/src/git/walker.rs +++ b/src/git/walker.rs @@ -245,7 +245,9 @@ impl GitWalker { // Truncate if too large and add marker if diff_content.len() > 8000 { - diff_content.truncate(8000); + // 8000 is a byte cap and may land inside a multi-byte character. + let end = crate::git::floor_char_boundary(&diff_content, 8000); + diff_content.truncate(end); diff_content.push_str("\n\n[... diff truncated ...]"); tracing::warn!("Truncated large diff for commit {}", commit.id()); } diff --git a/src/indexer/file_walker/mod.rs b/src/indexer/file_walker/mod.rs index 5f9bdd9..d3e1996 100644 --- a/src/indexer/file_walker/mod.rs +++ b/src/indexer/file_walker/mod.rs @@ -8,8 +8,8 @@ use ignore::WalkBuilder; use sha2::{Digest, Sha256}; use std::fs; use std::path::{Path, PathBuf}; -use std::sync::atomic::{AtomicBool, Ordering}; use std::sync::Arc; +use std::sync::atomic::{AtomicBool, Ordering}; pub struct FileWalker { pub(crate) root: PathBuf, @@ -197,7 +197,12 @@ impl FileWalker { /// Check if file matches include/exclude patterns pub(crate) fn matches_patterns(&self, path: &Path) -> bool { - let path_str = path.to_string_lossy(); + // Match against the path relative to the walk root, not the absolute path: + // matching the absolute path lets any ancestor directory name (a username, + // a temp-dir suffix, anything containing the pattern as a substring) produce + // false positives that have nothing to do with the file itself. + let relative = path.strip_prefix(&self.root).unwrap_or(path); + let path_str = relative.to_string_lossy(); // If include patterns are specified, file must match at least one if !self.include_patterns.is_empty() { diff --git a/src/indexer/file_walker/tests.rs b/src/indexer/file_walker/tests.rs index 542e4c7..60feb32 100644 --- a/src/indexer/file_walker/tests.rs +++ b/src/indexer/file_walker/tests.rs @@ -184,8 +184,7 @@ fn test_walk_file_info_fields() { let file_path = temp_dir.path().join("test.rs"); fs::write(&file_path, "fn main() {}").unwrap(); - let walker = - FileWalker::new(temp_dir.path(), 1024).with_project(Some("test-proj".to_string())); + let walker = FileWalker::new(temp_dir.path(), 1024).with_project(Some("test-proj".to_string())); let files = walker.walk().unwrap(); assert_eq!(files.len(), 1); @@ -278,8 +277,7 @@ fn test_matches_patterns_include_multiple() { #[test] fn test_matches_patterns_exclude_match() { - let walker = - FileWalker::new("/tmp", 1024).with_patterns(vec![], vec!["target".to_string()]); + let walker = FileWalker::new("/tmp", 1024).with_patterns(vec![], vec!["target".to_string()]); assert!(walker.matches_patterns(Path::new("/tmp/src/main.rs"))); assert!(!walker.matches_patterns(Path::new("/tmp/target/debug/main"))); } diff --git a/src/lib.rs b/src/lib.rs index 7d3a63f..b99e6da 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -50,6 +50,7 @@ //! let query_req = QueryRequest { //! query: "authentication logic".to_string(), //! project: Some("my-project".to_string()), +//! path: None, //! limit: 10, //! min_score: 0.7, //! hybrid: true, @@ -72,11 +73,9 @@ //! //! #[tokio::main] //! async fn main() -> anyhow::Result<()> { -//! // Create server (internally creates a RagClient) -//! let server = RagMcpServer::new().await?; -//! -//! // Serve over stdio (MCP protocol) -//! server.serve_stdio().await?; +//! // Serve over stdio (MCP protocol). This associated function creates the +//! // server (and its RagClient) internally and runs until the client disconnects. +//! RagMcpServer::serve_stdio().await?; //! //! Ok(()) //! } @@ -86,6 +85,7 @@ //! //! ```no_run //! use project_rag::{RagClient, mcp_server::RagMcpServer}; +//! use rmcp::ServiceExt; //! use std::sync::Arc; //! //! #[tokio::main] @@ -93,10 +93,9 @@ //! // Create client with custom configuration //! let client = RagClient::new().await?; //! -//! // Wrap client in MCP server +//! // Wrap client in MCP server and serve it over stdio //! let server = RagMcpServer::with_client(Arc::new(client))?; -//! -//! server.serve_stdio().await?; +//! server.serve(rmcp::transport::io::stdio()).await?.waiting().await?; //! Ok(()) //! } //! ``` diff --git a/src/main.rs b/src/main.rs index b16144f..225e7dd 100644 --- a/src/main.rs +++ b/src/main.rs @@ -24,8 +24,12 @@ enum Commands { #[tokio::main] async fn main() -> Result<()> { - // Initialize tracing - tracing_subscriber::fmt::init(); + // Initialize tracing. + // Must write to stderr: stdout carries the JSON-RPC stream in stdio MCP mode, + // and log lines interleaved there corrupt it. + tracing_subscriber::fmt() + .with_writer(std::io::stderr) + .init(); // Parse CLI arguments let cli = Cli::parse(); diff --git a/src/mcp_server.rs b/src/mcp_server.rs index 81c2975..649bd20 100644 --- a/src/mcp_server.rs +++ b/src/mcp_server.rs @@ -225,7 +225,9 @@ impl RagMcpServer { serde_json::to_string_pretty(&response).map_err(|e| format!("Serialization failed: {}", e)) } - #[tool(description = "Find the definition of a symbol at a given file location (line and column)")] + #[tool( + description = "Find the definition of a symbol at a given file location (line and column)" + )] async fn find_definition( &self, Parameters(req): Parameters, @@ -259,7 +261,9 @@ impl RagMcpServer { serde_json::to_string_pretty(&response).map_err(|e| format!("Serialization failed: {}", e)) } - #[tool(description = "Get the call graph for a function at a given file location (callers and callees)")] + #[tool( + description = "Get the call graph for a function at a given file location (callers and callees)" + )] async fn get_call_graph( &self, Parameters(req): Parameters, @@ -275,6 +279,79 @@ impl RagMcpServer { serde_json::to_string_pretty(&response).map_err(|e| format!("Serialization failed: {}", e)) } + + #[tool( + description = "List every symbol defined in one file (name, kind, line span, signature) with no file content. Use this to enumerate a file: query_codebase and search_by_filters are relevance-ranked with a limit and cannot enumerate, while find_definition and find_references need a position you already have." + )] + async fn list_symbols( + &self, + Parameters(req): Parameters, + ) -> Result { + // Validate request inputs + req.validate()?; + + let response = self + .client + .list_symbols(req) + .await + .map_err(|e| format!("{:#}", e))?; + + serde_json::to_string_pretty(&response).map_err(|e| format!("Serialization failed: {}", e)) + } + + #[tool( + description = "Read a slice (or all) of a file's current on-disk content. The file must be inside an already-indexed project root. Returns a SHA256 file_hash to pass as expected_hash to edit_file so edits can be rejected if the file changed since this read. Large ranges are capped per call (truncated: true) rather than dropped silently - page through with start_line/end_line." + )] + async fn read_file( + &self, + Parameters(req): Parameters, + ) -> Result { + req.validate()?; + + let response = self + .client + .read_file(req) + .await + .map_err(|e| format!("{:#}", e))?; + + serde_json::to_string_pretty(&response).map_err(|e| format!("Serialization failed: {}", e)) + } + + #[tool( + description = "Replace a line range (or the whole file) in a file inside an already-indexed project root, then automatically reindex that root so search results stay current. Pass expected_hash (from read_file) to guard against overwriting a change you haven't seen - a mismatch returns status: \"hash_conflict\" instead of applying the edit. Omit start_line/end_line to replace or create the whole file. Set start_line to end_line + 1 to insert content without deleting anything." + )] + async fn edit_file( + &self, + Parameters(req): Parameters, + ) -> Result { + req.validate()?; + + let response = self + .client + .edit_file(req) + .await + .map_err(|e| format!("{:#}", e))?; + + serde_json::to_string_pretty(&response).map_err(|e| format!("Serialization failed: {}", e)) + } + + #[tool( + description = "Find unused imports and dead-code candidates in a file or directory. check: \"imports\" flags import/use/include bindings never referenced in their file (index-free); \"symbols\" flags definitions nothing references, using the index to verify cross-file usage (requires index_codebase first); \"all\" (default) does both. Candidates carry a confidence level (high/medium/low) - the analysis is text-based, so dynamic dispatch, macros and framework wiring are invisible to it. Treat results as leads to verify, never as safe to auto-delete." + )] + async fn find_unused( + &self, + Parameters(req): Parameters, + ) -> Result { + req.validate()?; + + let response = self + .client + .find_unused(req) + .await + .map_err(|e| format!("{:#}", e))?; + + serde_json::to_string_pretty(&response).map_err(|e| format!("Serialization failed: {}", e)) + } } // Prompts for slash commands @@ -443,6 +520,61 @@ impl RagMcpServer { ), )]) } + + #[prompt( + name = "read", + description = "Read a slice (or all) of a file's current content" + )] + async fn read_prompt( + &self, + Parameters(args): Parameters, + ) -> Result, McpError> { + let file = args.get("file").and_then(|v| v.as_str()).unwrap_or(""); + + Ok(vec![PromptMessage::new_text( + PromptMessageRole::User, + format!("Please read the file '{}'.", file), + )]) + } + + #[prompt( + name = "edit", + description = "Edit a file inside an indexed project and reindex it automatically" + )] + async fn edit_prompt( + &self, + Parameters(args): Parameters, + ) -> Result, McpError> { + let file = args.get("file").and_then(|v| v.as_str()).unwrap_or(""); + + Ok(vec![PromptMessage::new_text( + PromptMessageRole::User, + format!( + "Please edit the file '{}'. First read it with read_file to get its current content and file_hash, then call edit_file with the new content and that expected_hash.", + file + ), + )]) + } + + #[prompt( + name = "unused", + description = "Find unused imports and dead-code candidates in a file or directory" + )] + async fn unused_prompt( + &self, + Parameters(args): Parameters, + ) -> Result, McpError> { + let path = args.get("path").and_then(|v| v.as_str()).unwrap_or("."); + + Ok(vec![PromptMessage::new_text( + PromptMessageRole::User, + format!( + "Please run the find_unused tool on '{}' and summarize the findings grouped by confidence. \ + Remind me that candidates are leads to verify, not guaranteed dead code.", + path + ), + )]) + } } #[tool_handler(router = self.tool_router)] @@ -465,7 +597,10 @@ impl ServerHandler for RagMcpServer { instructions: Some( "RAG-based codebase indexing and semantic search. \ Use index_codebase to create embeddings (automatically performs full or incremental indexing), \ - query_codebase to search, and search_by_filters for advanced queries." + query_codebase to search, and search_by_filters for advanced queries. \ + Use read_file and edit_file to read and modify files inside an indexed project; \ + edit_file automatically reindexes the affected file. \ + Use find_unused to surface unused imports and dead-code candidates for cleanup." .into(), ), } diff --git a/src/mcp_server/tests.rs b/src/mcp_server/tests.rs index bf993f7..c329023 100644 --- a/src/mcp_server/tests.rs +++ b/src/mcp_server/tests.rs @@ -19,6 +19,113 @@ async fn test_new_creates_server() { assert!(client.is_ok(), "Server creation should succeed"); } +/// Prove every tool and prompt is actually REACHABLE through the routers, not +/// merely defined. A handler written outside the `#[tool_router]` / +/// `#[prompt_router]` impl block compiles fine but is never exposed to MCP +/// clients; enumerating the routers is the only check that catches that. The +/// exact counts are asserted so adding a handler without routing it (or +/// forgetting to update the docs' tool count) fails here instead of silently. +#[test] +fn test_all_tools_and_prompts_are_routed() { + let tool_names: Vec = RagMcpServer::tool_router() + .list_all() + .iter() + .map(|t| t.name.to_string()) + .collect(); + for expected in [ + "index_codebase", + "query_codebase", + "get_statistics", + "clear_index", + "search_by_filters", + "search_git_history", + "find_definition", + "find_references", + "get_call_graph", + "list_symbols", + "read_file", + "edit_file", + "find_unused", + ] { + assert!( + tool_names.iter().any(|n| n == expected), + "tool '{}' is not routed; routed tools: {:?}", + expected, + tool_names + ); + } + assert_eq!( + tool_names.len(), + 13, + "unexpected tool count: {:?}", + tool_names + ); + + let prompt_names: Vec = RagMcpServer::prompt_router() + .list_all() + .iter() + .map(|p| p.name.to_string()) + .collect(); + assert!( + prompt_names.iter().any(|n| n == "unused"), + "prompt 'unused' is not routed; routed prompts: {:?}", + prompt_names + ); + assert_eq!( + prompt_names.len(), + 12, + "unexpected prompt count: {:?}", + prompt_names + ); +} + +/// The MCP input schema for find_unused is generated from FindUnusedRequest's +/// JsonSchema derive; this pins the contract a client actually sees: all five +/// parameters present, only `path` required (the rest have serde defaults), +/// and doc comments surfaced as descriptions. +#[test] +fn test_find_unused_tool_schema() { + let router = RagMcpServer::tool_router(); + let tools = router.list_all(); + let tool = tools + .iter() + .find(|t| t.name == "find_unused") + .expect("find_unused not routed"); + + let schema = serde_json::to_value(&*tool.input_schema).unwrap(); + eprintln!( + "find_unused input_schema:\n{}", + serde_json::to_string_pretty(&schema).unwrap() + ); + + let properties = schema["properties"] + .as_object() + .expect("schema has no properties"); + for field in ["path", "project", "check", "limit", "max_file_size"] { + assert!(properties.contains_key(field), "schema missing '{}'", field); + assert!( + properties[field]["description"].is_string(), + "'{}' has no description", + field + ); + } + + let required: Vec<&str> = schema["required"] + .as_array() + .expect("schema has no required list") + .iter() + .filter_map(|v| v.as_str()) + .collect(); + assert_eq!(required, vec!["path"], "only 'path' should be required"); + + assert!( + tool.description + .as_deref() + .is_some_and(|d| d.contains("never as safe to auto-delete")), + "tool description lost its safety warning" + ); +} + #[tokio::test] async fn test_get_info() { let temp_dir = TempDir::new().unwrap(); @@ -358,6 +465,9 @@ async fn test_tool_get_statistics_with_data() { assert!(response.total_files > 0); assert!(response.total_chunks > 0); assert!(response.total_embeddings > 0); + // Indexing stores definitions in the relations store; `fn main` is one. + assert!(response.total_definitions > 0); + assert!(response.files_with_definitions > 0); } #[tokio::test] diff --git a/src/relations/mod.rs b/src/relations/mod.rs index eebcd37..0636bc2 100644 --- a/src/relations/mod.rs +++ b/src/relations/mod.rs @@ -34,7 +34,8 @@ use anyhow::Result; pub use types::{ CallEdge, CallGraphNode, Definition, DefinitionResult, PrecisionLevel, Reference, - ReferenceKind, ReferenceResult, SymbolId, SymbolInfo, SymbolKind, Visibility, + ReferenceKind, ReferenceResult, SkippedDefinition, SymbolId, SymbolInfo, SymbolKind, + Visibility, }; use crate::indexer::FileInfo; @@ -51,6 +52,20 @@ pub trait RelationsProvider: Send + Sync { /// found in the given file. fn extract_definitions(&self, file_info: &FileInfo) -> Result>; + /// Extract definitions, and also report the ones that were recognised but could + /// not be named. + /// + /// A definition node whose name cannot be extracted is omitted from the result. + /// This method reports those omissions so a caller can tell an incomplete listing + /// from a complete one; the default implementation reports none, which is correct + /// only for providers that cannot skip. + fn extract_definitions_reporting( + &self, + file_info: &FileInfo, + ) -> Result<(Vec, Vec)> { + Ok((self.extract_definitions(file_info)?, Vec::new())) + } + /// Extract references from a file. /// /// `symbol_index` maps symbol names to their definitions, used for @@ -141,6 +156,17 @@ impl RelationsProvider for HybridRelationsProvider { .extract_definitions(file_info) } + fn extract_definitions_reporting( + &self, + file_info: &FileInfo, + ) -> Result<(Vec, Vec)> { + // Without this override the default trait impl answers with an empty + // skipped list, hiding every omission behind the hybrid dispatch. + let language = file_info.language.as_deref().unwrap_or("Unknown"); + self.provider_for_language(language) + .extract_definitions_reporting(file_info) + } + fn extract_references( &self, file_info: &FileInfo, diff --git a/src/relations/repomap/import_extractor.rs b/src/relations/repomap/import_extractor.rs new file mode 100644 index 0000000..1618330 --- /dev/null +++ b/src/relations/repomap/import_extractor.rs @@ -0,0 +1,512 @@ +//! Import statement extraction. +//! +//! Turns import/use/include nodes into `SymbolKind::Import` definitions, one per +//! BOUND NAME rather than one per statement: `use a::{B, C as D};` yields `B` and +//! `D`, because those are the identifiers the rest of the file can actually +//! reference. That is the granularity an unused-import check needs. +//! +//! A statement that binds no checkable name (glob imports like `use x::*`, +//! side-effect imports like `import "./polyfill"`) yields nothing here; the caller +//! records it as a skipped definition so the omission is visible. + +use chrono::Utc; +use tree_sitter::Node; + +use crate::indexer::FileInfo; +use crate::relations::types::{Definition, SymbolId, SymbolKind, Visibility}; + +/// Check if a node kind represents an import statement for the given language. +pub fn is_import_node(kind: &str, language: &str) -> bool { + match language { + "Rust" => matches!(kind, "use_declaration" | "extern_crate_declaration"), + "Python" => matches!(kind, "import_statement" | "import_from_statement"), + "JavaScript" | "TypeScript" => kind == "import_statement", + "Go" | "Java" | "Swift" => kind == "import_declaration", + "C" | "C++" => kind == "preproc_include", + "C#" => kind == "using_directive", + "PHP" => kind == "namespace_use_declaration", + _ => false, + } +} + +/// Extract one `Definition` per name the import binds into scope. +/// +/// Returns an empty vector when no bound name could be extracted -- either because +/// the statement genuinely binds none (globs, side-effect imports) or because the +/// grammar shape was not recognised. The caller must surface that as a skipped +/// definition rather than dropping it silently. +pub fn extract_imports( + node: Node, + source: &str, + language: &str, + file_info: &FileInfo, + parent_id: &Option, +) -> Vec { + let mut names = binding_names(node, source, language); + + // A statement can mention the same name twice; one definition per name. + let mut seen = std::collections::HashSet::new(); + names.retain(|n| !n.trim().is_empty() && seen.insert(n.clone())); + + let start_pos = node.start_position(); + let end_pos = node.end_position(); + let text = node_text(node, source); + let signature = first_line(text); + let visibility = Visibility::from_keywords(&signature); + + names + .into_iter() + .map(|name| Definition { + symbol_id: SymbolId::new( + &file_info.relative_path, + name, + SymbolKind::Import, + start_pos.row + 1, + start_pos.column, + ), + root_path: Some(file_info.root_path.clone()), + project: file_info.project.clone(), + end_line: end_pos.row + 1, + end_col: end_pos.column, + signature: signature.clone(), + doc_comment: None, + visibility, + parent_id: parent_id.clone(), + indexed_at: Utc::now().timestamp(), + }) + .collect() +} + +/// The names an import statement binds, per language. +fn binding_names(node: Node, source: &str, language: &str) -> Vec { + match language { + "Rust" => rust_bindings(node, source), + "Python" => python_bindings(node, source), + "JavaScript" | "TypeScript" => js_bindings(node, source), + "Go" => go_bindings(node, source), + "Java" => java_bindings(node, source), + "Swift" => swift_bindings(node, source), + "C" | "C++" => c_include_bindings(node, source), + "C#" => csharp_bindings(node, source), + "PHP" => php_bindings(node, source), + _ => Vec::new(), + } +} + +fn node_text<'a>(node: Node, source: &'a str) -> &'a str { + source.get(node.start_byte()..node.end_byte()).unwrap_or("") +} + +fn first_line(text: &str) -> String { + text.lines().next().unwrap_or("").trim().to_string() +} + +/// Rust: walk the use clause tree. `use_as_clause` binds its alias, a +/// `scoped_identifier` binds its final segment, `use_list` recurses, and +/// `use_wildcard` binds nothing checkable. +fn rust_bindings(node: Node, source: &str) -> Vec { + fn walk(node: Node, source: &str, out: &mut Vec) { + match node.kind() { + "identifier" => out.push(node_text(node, source).to_string()), + "scoped_identifier" => { + if let Some(name) = node.child_by_field_name("name") { + out.push(node_text(name, source).to_string()); + } + } + "use_as_clause" => { + if let Some(alias) = node.child_by_field_name("alias") { + out.push(node_text(alias, source).to_string()); + } + } + "use_list" => { + let mut cursor = node.walk(); + for child in node.named_children(&mut cursor) { + walk(child, source, out); + } + } + "scoped_use_list" => { + if let Some(list) = node.child_by_field_name("list") { + walk(list, source, out); + } + } + "use_wildcard" => {} // binds unknowable names + _ => {} + } + } + + let mut out = Vec::new(); + if node.kind() == "extern_crate_declaration" { + // `extern crate foo;` or `extern crate foo as bar;` + let bound = node + .child_by_field_name("alias") + .or_else(|| node.child_by_field_name("name")); + if let Some(n) = bound { + out.push(node_text(n, source).to_string()); + } + return out; + } + if let Some(argument) = node.child_by_field_name("argument") { + walk(argument, source, &mut out); + } + out +} + +/// Python: `import a.b` binds `a` (the top-level module); `from m import x` +/// binds `x`. Aliases win in both forms. Both grammars put the imported items +/// in the `name` field, and `from` puts the module in `module_name`. +fn python_bindings(node: Node, source: &str) -> Vec { + let from_import = node.kind() == "import_from_statement"; + let mut out = Vec::new(); + let mut cursor = node.walk(); + for item in node.children_by_field_name("name", &mut cursor) { + match item.kind() { + "aliased_import" => { + if let Some(alias) = item.child_by_field_name("alias") { + out.push(node_text(alias, source).to_string()); + } + } + "dotted_name" => { + let text = node_text(item, source); + let segment = if from_import { + text.rsplit('.').next() + } else { + text.split('.').next() + }; + if let Some(s) = segment { + out.push(s.to_string()); + } + } + _ => {} + } + } + out +} + +/// JS/TS: default import, `* as ns`, and named specifiers (alias wins). +fn js_bindings(node: Node, source: &str) -> Vec { + let mut out = Vec::new(); + let mut cursor = node.walk(); + for child in node.named_children(&mut cursor) { + if child.kind() != "import_clause" { + continue; + } + let mut clause_cursor = child.walk(); + for part in child.named_children(&mut clause_cursor) { + match part.kind() { + "identifier" => out.push(node_text(part, source).to_string()), + "namespace_import" => { + if let Some(id) = first_descendant_of_kind(part, "identifier") { + out.push(node_text(id, source).to_string()); + } + } + "named_imports" => { + let mut spec_cursor = part.walk(); + for spec in part.named_children(&mut spec_cursor) { + if spec.kind() != "import_specifier" { + continue; + } + let bound = spec + .child_by_field_name("alias") + .or_else(|| spec.child_by_field_name("name")); + if let Some(n) = bound { + out.push(node_text(n, source).trim_matches(['"', '\'']).to_string()); + } + } + } + _ => {} + } + } + } + out +} + +/// Go: each `import_spec` binds its explicit package name, or the final path +/// segment of the import string. `_` and `.` imports bind nothing checkable. +fn go_bindings(node: Node, source: &str) -> Vec { + let mut out = Vec::new(); + let mut specs = Vec::new(); + collect_descendants_of_kind(node, "import_spec", &mut specs); + for spec in specs { + if let Some(name) = spec.child_by_field_name("name") { + let text = node_text(name, source); + if text != "_" && text != "." { + out.push(text.to_string()); + } + continue; + } + if let Some(path) = spec.child_by_field_name("path") { + let text = node_text(path, source).trim_matches(['"', '`']).to_string(); + if let Some(segment) = text.rsplit('/').next() + && !segment.is_empty() + { + out.push(segment.to_string()); + } + } + } + out +} + +/// Java: `import java.util.List;` binds `List`. Wildcard imports bind +/// unknowable names and yield nothing. +fn java_bindings(node: Node, source: &str) -> Vec { + let mut cursor = node.walk(); + for child in node.children(&mut cursor) { + if child.kind() == "asterisk" { + return Vec::new(); + } + } + let mut out = Vec::new(); + let mut cursor = node.walk(); + for child in node.named_children(&mut cursor) { + match child.kind() { + "scoped_identifier" => { + if let Some(name) = child.child_by_field_name("name") { + out.push(node_text(name, source).to_string()); + } + } + "identifier" => out.push(node_text(child, source).to_string()), + _ => {} + } + } + out +} + +/// Swift: `import Foundation` binds the module name. +fn swift_bindings(node: Node, source: &str) -> Vec { + for kind in ["simple_identifier", "identifier"] { + if let Some(id) = first_descendant_of_kind(node, kind) { + return vec![node_text(id, source).to_string()]; + } + } + Vec::new() +} + +/// C/C++: the "name" of an include is the header path itself, quotes and +/// angle brackets stripped: `#include ` binds `stdio.h`. +fn c_include_bindings(node: Node, source: &str) -> Vec { + let Some(path) = node.child_by_field_name("path") else { + return Vec::new(); + }; + let text = node_text(path, source) + .trim() + .trim_matches(['"', '<', '>']) + .to_string(); + if text.is_empty() { + Vec::new() + } else { + vec![text] + } +} + +/// C#: an alias directive binds its alias; a plain `using System.Text;` is +/// recorded under its final segment. That final segment is a namespace, not a +/// usable identifier, so unused-detection for C# usings stays heuristic. +fn csharp_bindings(node: Node, source: &str) -> Vec { + let mut ids = Vec::new(); + collect_descendants_of_kind(node, "identifier", &mut ids); + + // An alias directive (`using Foo = System.Bar;`) is recognisable by the bare + // `=` token; the alias is the identifier BEFORE it. Depending on grammar + // version the alias may or may not be wrapped in a name_equals node, so key + // off the token rather than the wrapper. + let mut cursor = node.walk(); + let is_alias = node.children(&mut cursor).any(|c| c.kind() == "=") + || first_descendant_of_kind(node, "name_equals").is_some(); + + let bound = if is_alias { ids.first() } else { ids.last() }; + match bound { + Some(id) => vec![node_text(*id, source).to_string()], + None => Vec::new(), + } +} + +/// PHP: each use clause binds its alias or the final segment of the +/// qualified name. +fn php_bindings(node: Node, source: &str) -> Vec { + let mut out = Vec::new(); + let mut clauses = Vec::new(); + collect_descendants_of_kind(node, "namespace_use_clause", &mut clauses); + for clause in clauses { + if let Some(aliasing) = first_descendant_of_kind(clause, "namespace_aliasing_clause") + && let Some(name) = first_descendant_of_kind(aliasing, "name") + { + out.push(node_text(name, source).to_string()); + continue; + } + let mut names = Vec::new(); + collect_descendants_of_kind(clause, "name", &mut names); + if let Some(last) = names.last() { + out.push(node_text(*last, source).to_string()); + } + } + out +} + +fn first_descendant_of_kind<'a>(node: Node<'a>, kind: &str) -> Option> { + if node.kind() == kind { + return Some(node); + } + let mut cursor = node.walk(); + for child in node.children(&mut cursor) { + if let Some(found) = first_descendant_of_kind(child, kind) { + return Some(found); + } + } + None +} + +fn collect_descendants_of_kind<'a>(node: Node<'a>, kind: &str, out: &mut Vec>) { + if node.kind() == kind { + out.push(node); + // A clause of some kind never nests inside itself in the grammars used here. + return; + } + let mut cursor = node.walk(); + for child in node.children(&mut cursor) { + collect_descendants_of_kind(child, kind, out); + } +} + +#[cfg(test)] +mod tests { + use super::super::symbol_extractor::SymbolExtractor; + use crate::indexer::FileInfo; + use crate::relations::types::{SymbolKind, Visibility}; + use std::path::PathBuf; + + fn make_file_info(content: &str, extension: &str) -> FileInfo { + FileInfo { + path: PathBuf::from(format!("test.{}", extension)), + relative_path: format!("test.{}", extension), + root_path: "/test".to_string(), + project: None, + extension: Some(extension.to_string()), + language: None, + content: content.to_string(), + hash: "test_hash".to_string(), + } + } + + fn import_names(content: &str, extension: &str) -> Vec { + let file_info = make_file_info(content, extension); + let extractor = SymbolExtractor::new(); + let definitions = extractor.extract_definitions(&file_info).unwrap(); + definitions + .iter() + .filter(|d| d.kind() == SymbolKind::Import) + .map(|d| d.name().to_string()) + .collect() + } + + #[test] + fn test_rust_use_bindings() { + let names = import_names( + "use std::collections::HashMap;\nuse foo::bar as baz;\nuse a::{B, C as D};\n", + "rs", + ); + assert_eq!(names, vec!["HashMap", "baz", "B", "D"]); + } + + #[test] + fn test_rust_pub_use_is_public() { + let file_info = make_file_info("pub use foo::Bar;\n", "rs"); + let defs = SymbolExtractor::new() + .extract_definitions(&file_info) + .unwrap(); + let import = defs + .iter() + .find(|d| d.kind() == SymbolKind::Import) + .unwrap(); + assert_eq!(import.name(), "Bar"); + assert_eq!(import.visibility, Visibility::Public); + } + + #[test] + fn test_rust_glob_import_is_reported_skipped() { + let file_info = make_file_info("use foo::*;\n", "rs"); + let (defs, skipped) = SymbolExtractor::new() + .extract_definitions_reporting(&file_info) + .unwrap(); + assert!(defs.iter().all(|d| d.kind() != SymbolKind::Import)); + assert_eq!(skipped.len(), 1); + assert_eq!(skipped[0].kind, "use_declaration"); + } + + #[test] + fn test_python_import_bindings() { + let names = import_names( + "import os\nimport numpy as np\nfrom collections import OrderedDict\nfrom x import a as b, c\nimport os.path\n", + "py", + ); + assert_eq!(names, vec!["os", "np", "OrderedDict", "b", "c", "os"]); + } + + #[test] + fn test_javascript_import_bindings() { + let names = import_names( + "import React from 'react';\nimport { useState, useEffect as ue } from 'react';\nimport * as path from 'path';\nimport './side-effect.css';\n", + "js", + ); + assert_eq!(names, vec!["React", "useState", "ue", "path"]); + } + + #[test] + fn test_typescript_import_bindings() { + let names = import_names("import { Component } from '@angular/core';\n", "ts"); + assert_eq!(names, vec!["Component"]); + } + + #[test] + fn test_go_import_bindings() { + let names = import_names( + "package main\n\nimport (\n\tf \"fmt\"\n\t\"strings\"\n\t\"net/http\"\n\t_ \"embed\"\n)\n", + "go", + ); + assert_eq!(names, vec!["f", "strings", "http"]); + } + + #[test] + fn test_java_import_bindings() { + let names = import_names( + "import java.util.List;\nimport java.util.*;\n\nclass Foo {}\n", + "java", + ); + assert_eq!(names, vec!["List"]); + } + + #[test] + fn test_c_include_bindings() { + let names = import_names("#include \n#include \"myheader.h\"\n", "c"); + assert_eq!(names, vec!["stdio.h", "myheader.h"]); + } + + #[test] + fn test_cpp_include_bindings() { + let names = import_names("#include \n", "cpp"); + assert_eq!(names, vec!["vector"]); + } + + #[test] + fn test_csharp_using_bindings() { + let names = import_names( + "using System.Text;\nusing Foo = System.Bar;\n\nclass C {}\n", + "cs", + ); + assert_eq!(names, vec!["Text", "Foo"]); + } + + #[test] + fn test_php_use_bindings() { + let names = import_names( + " Result<(Vec, Vec)> { + self.symbol_extractor + .extract_definitions_reporting(file_info) + } + fn extract_references( &self, file_info: &FileInfo, diff --git a/src/relations/repomap/reference_finder.rs b/src/relations/repomap/reference_finder.rs index b9b741d..7ad5b8f 100644 --- a/src/relations/repomap/reference_finder.rs +++ b/src/relations/repomap/reference_finder.rs @@ -58,9 +58,15 @@ impl ReferenceFinder { // Determine reference kind based on context let reference_kind = self.determine_reference_kind(line, mat.start(), name); - // Get the best matching definition - // For now, just use the first one (could be improved with scope analysis) - if let Some(def) = definitions.first() { + // Get the best matching definition. Prefer a real definition over + // an import binding of the same name: an import is where a symbol + // ENTERS a file, not where it is defined, so a reference resolved + // to the import would point at the wrong place. + let target = definitions + .iter() + .find(|d| d.kind() != crate::relations::types::SymbolKind::Import) + .or_else(|| definitions.first()); + if let Some(def) = target { references.push(Reference { file_path: file_info.relative_path.clone(), root_path: Some(file_info.root_path.clone()), @@ -96,12 +102,7 @@ impl ReferenceFinder { } /// Determine the kind of reference based on context - fn determine_reference_kind( - &self, - line: &str, - position: usize, - name: &str, - ) -> ReferenceKind { + fn determine_reference_kind(&self, line: &str, position: usize, name: &str) -> ReferenceKind { // Get text before the identifier let before = &line[..position]; @@ -257,7 +258,11 @@ fn greet(name: &str) { // First occurrence is a write, second is a read assert!(references.len() >= 1); - assert!(references.iter().any(|r| r.reference_kind == ReferenceKind::Write)); + assert!( + references + .iter() + .any(|r| r.reference_kind == ReferenceKind::Write) + ); } #[test] @@ -275,7 +280,11 @@ fn greet(name: &str) { let references = finder.find_references(&file_info, &symbol_index).unwrap(); assert!(!references.is_empty()); - assert!(references.iter().any(|r| r.reference_kind == ReferenceKind::Import)); + assert!( + references + .iter() + .any(|r| r.reference_kind == ReferenceKind::Import) + ); } #[test] @@ -293,7 +302,11 @@ fn greet(name: &str) { let references = finder.find_references(&file_info, &symbol_index).unwrap(); assert!(!references.is_empty()); - assert!(references.iter().any(|r| r.reference_kind == ReferenceKind::Instantiation)); + assert!( + references + .iter() + .any(|r| r.reference_kind == ReferenceKind::Instantiation) + ); } #[test] diff --git a/src/relations/repomap/symbol_extractor.rs b/src/relations/repomap/symbol_extractor.rs index 66922fe..bac634f 100644 --- a/src/relations/repomap/symbol_extractor.rs +++ b/src/relations/repomap/symbol_extractor.rs @@ -8,7 +8,7 @@ use chrono::Utc; use tree_sitter::{Language, Node, Parser}; use crate::indexer::FileInfo; -use crate::relations::types::{Definition, SymbolId, SymbolKind, Visibility}; +use crate::relations::types::{Definition, SkippedDefinition, SymbolId, SymbolKind, Visibility}; /// Extracts symbol definitions from source code using AST parsing. pub struct SymbolExtractor { @@ -23,12 +23,26 @@ impl SymbolExtractor { /// Extract all symbol definitions from a file pub fn extract_definitions(&self, file_info: &FileInfo) -> Result> { + let (definitions, _skipped) = self.extract_definitions_reporting(file_info)?; + Ok(definitions) + } + + /// Extract all symbol definitions, and report every node that was recognised as a + /// definition but could not be named. + /// + /// Those nodes are omitted from the returned definitions -- they used to be dropped + /// with no diagnostic at all, which made an incomplete listing indistinguishable + /// from a complete one. + pub fn extract_definitions_reporting( + &self, + file_info: &FileInfo, + ) -> Result<(Vec, Vec)> { let extension = file_info.extension.as_deref().unwrap_or(""); // Get language and parser let (language, language_name) = match get_language_for_extension(extension) { Some(lang) => lang, - None => return Ok(Vec::new()), // Unsupported language + None => return Ok((Vec::new(), Vec::new())), // Unsupported language }; let mut parser = Parser::new(); @@ -42,6 +56,7 @@ impl SymbolExtractor { let root_node = tree.root_node(); let mut definitions = Vec::new(); + let mut skipped = Vec::new(); // Extract definitions recursively self.extract_from_node( @@ -51,9 +66,19 @@ impl SymbolExtractor { file_info, None, &mut definitions, + &mut skipped, ); - Ok(definitions) + if !skipped.is_empty() { + tracing::warn!( + file = %file_info.relative_path, + skipped = skipped.len(), + found = definitions.len(), + "symbol extraction omitted definition nodes it could not name; the symbol list for this file is incomplete" + ); + } + + Ok((definitions, skipped)) } /// Extract definitions from a node and its children @@ -65,12 +90,34 @@ impl SymbolExtractor { file_info: &FileInfo, parent_id: Option, result: &mut Vec, + skipped: &mut Vec, ) { let kind = node.kind(); + // Import nodes bind names into scope; each bound name becomes its own + // SymbolKind::Import definition (`use a::{B, C}` yields two). A statement + // that binds no checkable name (globs, side-effect imports) is recorded as + // skipped so the listing is visibly incomplete rather than silently short. + if super::import_extractor::is_import_node(kind, language) { + let imports = super::import_extractor::extract_imports( + node, source, language, file_info, &parent_id, + ); + if imports.is_empty() { + skipped.push(skipped_from_node( + node, + source, + "could not extract bound names from this import", + )); + } else { + result.extend(imports); + } + return; // nothing definable nests inside an import statement + } + // Check if this node is a definition we care about if is_definition_node(kind, language) { - if let Some(def) = self.node_to_definition(node, source, language, file_info, &parent_id) + if let Some(def) = + self.node_to_definition(node, source, language, file_info, &parent_id) { let new_parent_id = Some(def.to_storage_id()); result.push(def); @@ -85,16 +132,34 @@ impl SymbolExtractor { file_info, new_parent_id.clone(), result, + skipped, ); } return; } + + // This node IS a definition but no name could be extracted from it, so it + // will not appear in the symbol list. Record it rather than dropping it + // silently -- the caller cannot otherwise tell that the listing is short. + skipped.push(skipped_from_node( + node, + source, + "could not extract a name from this node", + )); } // Recurse into children let mut cursor = node.walk(); for child in node.children(&mut cursor) { - self.extract_from_node(child, source, language, file_info, parent_id.clone(), result); + self.extract_from_node( + child, + source, + language, + file_info, + parent_id.clone(), + result, + skipped, + ); } } @@ -154,31 +219,71 @@ impl Default for SymbolExtractor { } } +/// Build the skipped-definition record for a node whose name (or bound names) +/// could not be extracted. +fn skipped_from_node(node: Node, source: &str, reason: &str) -> SkippedDefinition { + let snippet = source + .get(node.start_byte()..node.end_byte().min(source.len())) + .unwrap_or("") + .lines() + .next() + .unwrap_or("") + .trim() + .chars() + .take(120) + .collect::(); + SkippedDefinition { + line: node.start_position().row + 1, + kind: node.kind().to_string(), + reason: reason.to_string(), + snippet, + } +} + +/// Extractor-taxonomy language name for a file extension. +/// +/// Single source of truth for language dispatch: get_language_for_extension +/// derives its grammar from this name, so a file whose imports were extracted +/// under one language name can never be usage-checked under another. Headers +/// map to "C++" -- that grammar accepts C structs and enums and additionally +/// yields classes and namespaces, which the C grammar cannot. +pub fn language_name_for_extension(extension: &str) -> Option<&'static str> { + Some(match extension.to_lowercase().as_str() { + "rs" => "Rust", + "py" => "Python", + "js" | "mjs" | "cjs" | "jsx" => "JavaScript", + "ts" | "tsx" => "TypeScript", + "go" => "Go", + "java" => "Java", + "swift" => "Swift", + "c" => "C", + "h" | "cpp" | "cc" | "cxx" | "hpp" | "hxx" | "hh" => "C++", + "cs" => "C#", + "rb" => "Ruby", + "php" => "PHP", + _ => return None, + }) +} + /// Get the tree-sitter language for a file extension fn get_language_for_extension(extension: &str) -> Option<(Language, String)> { - match extension.to_lowercase().as_str() { - "rs" => Some((tree_sitter_rust::LANGUAGE.into(), "Rust".to_string())), - "py" => Some((tree_sitter_python::LANGUAGE.into(), "Python".to_string())), - "js" | "mjs" | "cjs" | "jsx" => Some(( - tree_sitter_javascript::LANGUAGE.into(), - "JavaScript".to_string(), - )), - "ts" | "tsx" => Some(( - tree_sitter_typescript::LANGUAGE_TYPESCRIPT.into(), - "TypeScript".to_string(), - )), - "go" => Some((tree_sitter_go::LANGUAGE.into(), "Go".to_string())), - "java" => Some((tree_sitter_java::LANGUAGE.into(), "Java".to_string())), - "swift" => Some((tree_sitter_swift::LANGUAGE.into(), "Swift".to_string())), - "c" | "h" => Some((tree_sitter_c::LANGUAGE.into(), "C".to_string())), - "cpp" | "cc" | "cxx" | "hpp" | "hxx" | "hh" => { - Some((tree_sitter_cpp::LANGUAGE.into(), "C++".to_string())) - } - "cs" => Some((tree_sitter_c_sharp::LANGUAGE.into(), "C#".to_string())), - "rb" => Some((tree_sitter_ruby::LANGUAGE.into(), "Ruby".to_string())), - "php" => Some((tree_sitter_php::LANGUAGE_PHP.into(), "PHP".to_string())), - _ => None, - } + let name = language_name_for_extension(extension)?; + let language: Language = match name { + "Rust" => tree_sitter_rust::LANGUAGE.into(), + "Python" => tree_sitter_python::LANGUAGE.into(), + "JavaScript" => tree_sitter_javascript::LANGUAGE.into(), + "TypeScript" => tree_sitter_typescript::LANGUAGE_TYPESCRIPT.into(), + "Go" => tree_sitter_go::LANGUAGE.into(), + "Java" => tree_sitter_java::LANGUAGE.into(), + "Swift" => tree_sitter_swift::LANGUAGE.into(), + "C" => tree_sitter_c::LANGUAGE.into(), + "C++" => tree_sitter_cpp::LANGUAGE.into(), + "C#" => tree_sitter_c_sharp::LANGUAGE.into(), + "Ruby" => tree_sitter_ruby::LANGUAGE.into(), + "PHP" => tree_sitter_php::LANGUAGE_PHP.into(), + _ => return None, + }; + Some((language, name.to_string())) } /// Check if a node kind represents a definition @@ -354,11 +459,21 @@ fn find_name_node<'a>(node: Node<'a>, language: &str) -> Option> { "C" | "C++" => { // C/C++: declarator contains the name if let Some(declarator) = node.child_by_field_name("declarator") { - // Navigate through possible pointer/reference declarators - return find_innermost_identifier(declarator); + // Navigate through possible pointer/reference declarators. + // Only return on success: returning None here would skip the generic + // identifier fallback at the end of this function, which is what made + // unnameable-but-valid definitions disappear without a trace. + if let Some(id) = find_innermost_identifier(declarator) { + return Some(id); + } } - // For struct/class, name is in the type specifier - if kind == "struct_specifier" || kind == "class_specifier" || kind == "enum_specifier" { + // For struct/class/enum/namespace, the name is its own field. The + // namespace name node's kind is namespace_identifier, which the + // generic identifier fallback below does not match. + if matches!( + kind, + "struct_specifier" | "class_specifier" | "enum_specifier" | "namespace_definition" + ) { if let Some(name_node) = node.child_by_field_name("name") { return Some(name_node); } @@ -403,9 +518,12 @@ fn find_innermost_identifier<'a>(node: Node<'a>) -> Option> { return Some(node); } - // Check for name field + // Check for name field. Only return on success -- an unconditional return here + // skips the child scan below, which is the same defect as in find_name_node. if let Some(name_node) = node.child_by_field_name("declarator") { - return find_innermost_identifier(name_node); + if let Some(id) = find_innermost_identifier(name_node) { + return Some(id); + } } // Fallback: look through children @@ -627,4 +745,26 @@ class Calculator { let storage_id = def.to_storage_id(); assert!(storage_id.contains("foo")); } + + #[test] + fn test_language_name_for_extension_c_family() { + assert_eq!(language_name_for_extension("c"), Some("C")); + for ext in ["h", "hh", "hxx", "hpp", "cpp", "cc", "cxx"] { + assert_eq!(language_name_for_extension(ext), Some("C++"), "{}", ext); + } + assert_eq!(language_name_for_extension("xyz"), None); + } + + #[test] + fn test_header_extracted_with_cpp_grammar() { + let source = "class KioskNotify {\npublic:\n void fire();\n};\nnamespace kiosk {\nstruct S {};\n}\n"; + let file_info = make_file_info(source, "h"); + let extractor = SymbolExtractor::new(); + let definitions = extractor.extract_definitions(&file_info).unwrap(); + + // The C grammar yielded no class or namespace nodes for headers. + assert!(definitions.iter().any(|d| d.name() == "KioskNotify")); + assert!(definitions.iter().any(|d| d.name() == "kiosk")); + assert!(definitions.iter().any(|d| d.name() == "S")); + } } diff --git a/src/relations/stack_graphs/mod.rs b/src/relations/stack_graphs/mod.rs new file mode 100644 index 0000000..91d5768 --- /dev/null +++ b/src/relations/stack_graphs/mod.rs @@ -0,0 +1,58 @@ +//! High-precision name resolution via stack-graphs (Python, TypeScript, Java, Ruby). +//! +//! **Status: not yet implemented.** The `stack-graphs` feature flag and this module +//! exist so [`crate::relations::HybridRelationsProvider`] has a real type to hold and +//! a real fallback path to exercise, but no stack-graphs crate is wired in yet. +//! [`StackGraphsProvider::new`] always returns an error, which `HybridRelationsProvider` +//! already handles by logging a warning and falling back to [`crate::relations::repomap::RepoMapProvider`] +//! for every language -- so enabling this feature today changes nothing observable. + +use anyhow::{Result, bail}; +use std::collections::HashMap; + +use crate::indexer::FileInfo; +use crate::relations::{Definition, PrecisionLevel, Reference, RelationsProvider}; + +/// Placeholder for the future stack-graphs-backed provider. +/// +/// Not constructible via the normal path: [`StackGraphsProvider::new`] always errors, +/// so [`crate::relations::HybridRelationsProvider`] never actually holds one of these +/// today. The type exists to keep the feature-gated field and call sites in +/// `relations/mod.rs` compiling against a real API shape. +pub struct StackGraphsProvider { + _private: (), +} + +impl StackGraphsProvider { + /// Always fails: stack-graphs support has not been implemented yet. + pub fn new() -> Result { + bail!("stack-graphs support is not yet implemented") + } + + /// No languages are supported yet. + pub fn supports_language(&self, _language: &str) -> bool { + false + } +} + +impl RelationsProvider for StackGraphsProvider { + fn extract_definitions(&self, _file_info: &FileInfo) -> Result> { + bail!("stack-graphs support is not yet implemented") + } + + fn extract_references( + &self, + _file_info: &FileInfo, + _symbol_index: &HashMap>, + ) -> Result> { + bail!("stack-graphs support is not yet implemented") + } + + fn supports_language(&self, _language: &str) -> bool { + false + } + + fn precision_level(&self, _language: &str) -> PrecisionLevel { + PrecisionLevel::High + } +} diff --git a/src/relations/storage/lance_store.rs b/src/relations/storage/lance_store.rs deleted file mode 100644 index 3a80abc..0000000 --- a/src/relations/storage/lance_store.rs +++ /dev/null @@ -1,176 +0,0 @@ -//! LanceDB-based storage for code relationships. - -use anyhow::{Context, Result}; -use async_trait::async_trait; -use std::path::PathBuf; -use std::sync::Arc; -use tokio::sync::RwLock; - -use super::{RelationsStats, RelationsStore}; -use crate::relations::types::{CallEdge, Definition, Reference}; - -/// LanceDB-based relations store. -/// -/// Stores definitions and references in separate LanceDB tables for efficient querying. -pub struct LanceRelationsStore { - /// Path to the database directory - db_path: PathBuf, - /// Database connection (lazy initialized) - db: Arc>>, -} - -impl LanceRelationsStore { - /// Create a new LanceDB relations store - pub async fn new(db_path: PathBuf) -> Result { - // Ensure directory exists - tokio::fs::create_dir_all(&db_path) - .await - .context("Failed to create relations database directory")?; - - Ok(Self { - db_path, - db: Arc::new(RwLock::new(None)), - }) - } - - /// Get or create the database connection - async fn get_connection(&self) -> Result { - let mut db_guard = self.db.write().await; - - if let Some(ref db) = *db_guard { - return Ok(db.clone()); - } - - let db = lancedb::connect(self.db_path.to_string_lossy().as_ref()) - .execute() - .await - .context("Failed to connect to LanceDB")?; - - *db_guard = Some(db.clone()); - Ok(db) - } - - /// Ensure definitions table exists - async fn ensure_definitions_table(&self) -> Result<()> { - let _db = self.get_connection().await?; - // Table will be created on first insert - // LanceDB creates tables lazily - Ok(()) - } - - /// Ensure references table exists - async fn ensure_references_table(&self) -> Result<()> { - let _db = self.get_connection().await?; - // Table will be created on first insert - Ok(()) - } -} - -#[async_trait] -impl RelationsStore for LanceRelationsStore { - async fn store_definitions( - &self, - definitions: Vec, - _root_path: &str, - ) -> Result { - if definitions.is_empty() { - return Ok(0); - } - - self.ensure_definitions_table().await?; - - // TODO: Implement actual LanceDB storage - // For now, just return the count - let count = definitions.len(); - - tracing::debug!("Stored {} definitions", count); - Ok(count) - } - - async fn store_references(&self, references: Vec, _root_path: &str) -> Result { - if references.is_empty() { - return Ok(0); - } - - self.ensure_references_table().await?; - - // TODO: Implement actual LanceDB storage - let count = references.len(); - - tracing::debug!("Stored {} references", count); - Ok(count) - } - - async fn find_definition_at( - &self, - _file_path: &str, - _line: usize, - _column: usize, - ) -> Result> { - // TODO: Implement query - Ok(None) - } - - async fn find_definitions_by_name(&self, _name: &str) -> Result> { - // TODO: Implement query - Ok(Vec::new()) - } - - async fn find_references(&self, _target_symbol_id: &str) -> Result> { - // TODO: Implement query - Ok(Vec::new()) - } - - async fn get_callers(&self, _symbol_id: &str) -> Result> { - // TODO: Implement call graph query - Ok(Vec::new()) - } - - async fn get_callees(&self, _symbol_id: &str) -> Result> { - // TODO: Implement call graph query - Ok(Vec::new()) - } - - async fn delete_by_file(&self, _file_path: &str) -> Result { - // TODO: Implement deletion - Ok(0) - } - - async fn clear(&self) -> Result<()> { - // TODO: Drop and recreate tables - Ok(()) - } - - async fn get_stats(&self) -> Result { - // TODO: Query actual counts - Ok(RelationsStats::default()) - } -} - -#[cfg(test)] -mod tests { - use super::*; - use tempfile::TempDir; - - #[tokio::test] - async fn test_store_creation() { - let temp_dir = TempDir::new().unwrap(); - let store = LanceRelationsStore::new(temp_dir.path().to_path_buf()) - .await - .unwrap(); - - let stats = store.get_stats().await.unwrap(); - assert_eq!(stats.definition_count, 0); - } - - #[tokio::test] - async fn test_store_empty_definitions() { - let temp_dir = TempDir::new().unwrap(); - let store = LanceRelationsStore::new(temp_dir.path().to_path_buf()) - .await - .unwrap(); - - let count = store.store_definitions(Vec::new(), "/test").await.unwrap(); - assert_eq!(count, 0); - } -} diff --git a/src/relations/storage/lance_store/codec.rs b/src/relations/storage/lance_store/codec.rs new file mode 100644 index 0000000..aa948ce --- /dev/null +++ b/src/relations/storage/lance_store/codec.rs @@ -0,0 +1,375 @@ +//! Arrow schemas and row codecs for the relations tables. +//! +//! Definitions and references are flat rows; enums travel as their serde +//! snake_case string form so the stored value matches what a `only_if` filter +//! written from Rust enum values will compare against. + +use anyhow::{Context, Result}; +use arrow_array::{Array, Int64Array, RecordBatch, StringArray, UInt32Array}; +use arrow_schema::{DataType, Field, Schema}; +use std::sync::Arc; + +use crate::relations::types::{ + Definition, Reference, ReferenceKind, SymbolId, SymbolKind, Visibility, +}; + +/// Schema of the definitions table. +pub fn definitions_schema() -> Arc { + Arc::new(Schema::new(vec![ + Field::new("id", DataType::Utf8, false), + Field::new("file_path", DataType::Utf8, false), + Field::new("root_path", DataType::Utf8, true), + Field::new("project", DataType::Utf8, true), + Field::new("name", DataType::Utf8, false), + Field::new("kind", DataType::Utf8, false), + Field::new("start_line", DataType::UInt32, false), + Field::new("start_col", DataType::UInt32, false), + Field::new("end_line", DataType::UInt32, false), + Field::new("end_col", DataType::UInt32, false), + Field::new("signature", DataType::Utf8, false), + Field::new("doc_comment", DataType::Utf8, true), + Field::new("visibility", DataType::Utf8, false), + Field::new("parent_id", DataType::Utf8, true), + Field::new("indexed_at", DataType::Int64, false), + ])) +} + +/// Schema of the references table. +pub fn references_schema() -> Arc { + Arc::new(Schema::new(vec![ + Field::new("id", DataType::Utf8, false), + Field::new("file_path", DataType::Utf8, false), + Field::new("root_path", DataType::Utf8, true), + Field::new("project", DataType::Utf8, true), + Field::new("start_line", DataType::UInt32, false), + Field::new("end_line", DataType::UInt32, false), + Field::new("start_col", DataType::UInt32, false), + Field::new("end_col", DataType::UInt32, false), + Field::new("target_symbol_id", DataType::Utf8, false), + Field::new("reference_kind", DataType::Utf8, false), + Field::new("indexed_at", DataType::Int64, false), + ])) +} + +/// Serde snake_case form of an enum value, without the JSON quotes. +pub fn enum_to_str(value: &T) -> String { + serde_json::to_string(value) + .unwrap_or_default() + .trim_matches('"') + .to_string() +} + +fn enum_from_str(s: &str) -> Option { + serde_json::from_str(&format!("\"{}\"", s)).ok() +} + +/// Escape a string for use inside a single-quoted SQL literal. +pub fn escape_sql(s: &str) -> String { + s.replace('\'', "''") +} + +/// Quoted, escaped, comma-joined list for an `IN (...)` filter. +pub fn sql_in_list>(values: &[S]) -> String { + values + .iter() + .map(|v| format!("'{}'", escape_sql(v.as_ref()))) + .collect::>() + .join(", ") +} + +pub fn definitions_to_batch(definitions: &[Definition]) -> Result { + let ids = StringArray::from( + definitions + .iter() + .map(|d| d.to_storage_id()) + .collect::>(), + ); + let file_paths = StringArray::from( + definitions + .iter() + .map(|d| d.file_path()) + .collect::>(), + ); + let root_paths = StringArray::from( + definitions + .iter() + .map(|d| d.root_path.as_deref()) + .collect::>(), + ); + let projects = StringArray::from( + definitions + .iter() + .map(|d| d.project.as_deref()) + .collect::>(), + ); + let names = StringArray::from(definitions.iter().map(|d| d.name()).collect::>()); + let kinds = StringArray::from( + definitions + .iter() + .map(|d| enum_to_str(&d.symbol_id.kind)) + .collect::>(), + ); + let start_lines = UInt32Array::from( + definitions + .iter() + .map(|d| d.symbol_id.start_line as u32) + .collect::>(), + ); + let start_cols = UInt32Array::from( + definitions + .iter() + .map(|d| d.symbol_id.start_col as u32) + .collect::>(), + ); + let end_lines = UInt32Array::from( + definitions + .iter() + .map(|d| d.end_line as u32) + .collect::>(), + ); + let end_cols = UInt32Array::from( + definitions + .iter() + .map(|d| d.end_col as u32) + .collect::>(), + ); + let signatures = StringArray::from( + definitions + .iter() + .map(|d| d.signature.as_str()) + .collect::>(), + ); + let doc_comments = StringArray::from( + definitions + .iter() + .map(|d| d.doc_comment.as_deref()) + .collect::>(), + ); + let visibilities = StringArray::from( + definitions + .iter() + .map(|d| enum_to_str(&d.visibility)) + .collect::>(), + ); + let parent_ids = StringArray::from( + definitions + .iter() + .map(|d| d.parent_id.as_deref()) + .collect::>(), + ); + let indexed_ats = + Int64Array::from(definitions.iter().map(|d| d.indexed_at).collect::>()); + + RecordBatch::try_new( + definitions_schema(), + vec![ + Arc::new(ids), + Arc::new(file_paths), + Arc::new(root_paths), + Arc::new(projects), + Arc::new(names), + Arc::new(kinds), + Arc::new(start_lines), + Arc::new(start_cols), + Arc::new(end_lines), + Arc::new(end_cols), + Arc::new(signatures), + Arc::new(doc_comments), + Arc::new(visibilities), + Arc::new(parent_ids), + Arc::new(indexed_ats), + ], + ) + .context("Failed to build definitions RecordBatch") +} + +pub fn references_to_batch(references: &[Reference]) -> Result { + let ids = StringArray::from( + references + .iter() + .map(|r| r.to_storage_id()) + .collect::>(), + ); + let file_paths = StringArray::from( + references + .iter() + .map(|r| r.file_path.as_str()) + .collect::>(), + ); + let root_paths = StringArray::from( + references + .iter() + .map(|r| r.root_path.as_deref()) + .collect::>(), + ); + let projects = StringArray::from( + references + .iter() + .map(|r| r.project.as_deref()) + .collect::>(), + ); + let start_lines = UInt32Array::from( + references + .iter() + .map(|r| r.start_line as u32) + .collect::>(), + ); + let end_lines = UInt32Array::from( + references + .iter() + .map(|r| r.end_line as u32) + .collect::>(), + ); + let start_cols = UInt32Array::from( + references + .iter() + .map(|r| r.start_col as u32) + .collect::>(), + ); + let end_cols = UInt32Array::from( + references + .iter() + .map(|r| r.end_col as u32) + .collect::>(), + ); + let targets = StringArray::from( + references + .iter() + .map(|r| r.target_symbol_id.as_str()) + .collect::>(), + ); + let kinds = StringArray::from( + references + .iter() + .map(|r| enum_to_str(&r.reference_kind)) + .collect::>(), + ); + let indexed_ats = Int64Array::from(references.iter().map(|r| r.indexed_at).collect::>()); + + RecordBatch::try_new( + references_schema(), + vec![ + Arc::new(ids), + Arc::new(file_paths), + Arc::new(root_paths), + Arc::new(projects), + Arc::new(start_lines), + Arc::new(end_lines), + Arc::new(start_cols), + Arc::new(end_cols), + Arc::new(targets), + Arc::new(kinds), + Arc::new(indexed_ats), + ], + ) + .context("Failed to build references RecordBatch") +} + +fn str_col<'a>(batch: &'a RecordBatch, name: &str) -> Result<&'a StringArray> { + batch + .column_by_name(name) + .with_context(|| format!("Missing column {}", name))? + .as_any() + .downcast_ref::() + .with_context(|| format!("Column {} is not Utf8", name)) +} + +fn u32_col<'a>(batch: &'a RecordBatch, name: &str) -> Result<&'a UInt32Array> { + batch + .column_by_name(name) + .with_context(|| format!("Missing column {}", name))? + .as_any() + .downcast_ref::() + .with_context(|| format!("Column {} is not UInt32", name)) +} + +fn i64_col<'a>(batch: &'a RecordBatch, name: &str) -> Result<&'a Int64Array> { + batch + .column_by_name(name) + .with_context(|| format!("Missing column {}", name))? + .as_any() + .downcast_ref::() + .with_context(|| format!("Column {} is not Int64", name)) +} + +fn opt_str(array: &StringArray, i: usize) -> Option { + if array.is_null(i) { + None + } else { + Some(array.value(i).to_string()) + } +} + +pub fn batch_to_definitions(batch: &RecordBatch) -> Result> { + let file_paths = str_col(batch, "file_path")?; + let root_paths = str_col(batch, "root_path")?; + let projects = str_col(batch, "project")?; + let names = str_col(batch, "name")?; + let kinds = str_col(batch, "kind")?; + let start_lines = u32_col(batch, "start_line")?; + let start_cols = u32_col(batch, "start_col")?; + let end_lines = u32_col(batch, "end_line")?; + let end_cols = u32_col(batch, "end_col")?; + let signatures = str_col(batch, "signature")?; + let doc_comments = str_col(batch, "doc_comment")?; + let visibilities = str_col(batch, "visibility")?; + let parent_ids = str_col(batch, "parent_id")?; + let indexed_ats = i64_col(batch, "indexed_at")?; + + let mut out = Vec::with_capacity(batch.num_rows()); + for i in 0..batch.num_rows() { + let kind = enum_from_str::(kinds.value(i)).unwrap_or(SymbolKind::Unknown); + let visibility = enum_from_str::(visibilities.value(i)).unwrap_or_default(); + out.push(Definition { + symbol_id: SymbolId::new( + file_paths.value(i), + names.value(i), + kind, + start_lines.value(i) as usize, + start_cols.value(i) as usize, + ), + root_path: opt_str(root_paths, i), + project: opt_str(projects, i), + end_line: end_lines.value(i) as usize, + end_col: end_cols.value(i) as usize, + signature: signatures.value(i).to_string(), + doc_comment: opt_str(doc_comments, i), + visibility, + parent_id: opt_str(parent_ids, i), + indexed_at: indexed_ats.value(i), + }); + } + Ok(out) +} + +pub fn batch_to_references(batch: &RecordBatch) -> Result> { + let file_paths = str_col(batch, "file_path")?; + let root_paths = str_col(batch, "root_path")?; + let projects = str_col(batch, "project")?; + let start_lines = u32_col(batch, "start_line")?; + let end_lines = u32_col(batch, "end_line")?; + let start_cols = u32_col(batch, "start_col")?; + let end_cols = u32_col(batch, "end_col")?; + let targets = str_col(batch, "target_symbol_id")?; + let kinds = str_col(batch, "reference_kind")?; + let indexed_ats = i64_col(batch, "indexed_at")?; + + let mut out = Vec::with_capacity(batch.num_rows()); + for i in 0..batch.num_rows() { + let reference_kind = + enum_from_str::(kinds.value(i)).unwrap_or(ReferenceKind::Unknown); + out.push(Reference { + file_path: file_paths.value(i).to_string(), + root_path: opt_str(root_paths, i), + project: opt_str(projects, i), + start_line: start_lines.value(i) as usize, + end_line: end_lines.value(i) as usize, + start_col: start_cols.value(i) as usize, + end_col: end_cols.value(i) as usize, + target_symbol_id: targets.value(i).to_string(), + reference_kind, + indexed_at: indexed_ats.value(i), + }); + } + Ok(out) +} diff --git a/src/relations/storage/lance_store/mod.rs b/src/relations/storage/lance_store/mod.rs new file mode 100644 index 0000000..db92a0f --- /dev/null +++ b/src/relations/storage/lance_store/mod.rs @@ -0,0 +1,431 @@ +//! LanceDB-based storage for code relationships. +//! +//! Definitions and references live in two tables (`relations_definitions`, +//! `relations_references`) inside the same LanceDB directory as the embeddings +//! table, so one database directory holds everything the index knows. +//! +//! Writes are idempotent per file: storing rows for a file first deletes +//! whatever that file had, so re-indexing never accumulates duplicates. + +mod codec; + +use anyhow::{Context, Result}; +use arrow_array::{RecordBatch, RecordBatchIterator, StringArray}; +use arrow_schema::Schema; +use async_trait::async_trait; +use futures::stream::TryStreamExt; +use lancedb::query::{ExecutableQuery, QueryBase}; +use lancedb::{Connection, Table}; +use std::collections::{HashMap, HashSet}; +use std::path::PathBuf; +use std::sync::Arc; +use tokio::sync::RwLock; + +use super::{RelationsStats, RelationsStore}; +use crate::relations::types::{CallEdge, Definition, Reference, ReferenceKind, SymbolKind}; + +const DEFINITIONS_TABLE: &str = "relations_definitions"; +const REFERENCES_TABLE: &str = "relations_references"; + +/// Delete filters are built as `file_path IN (...)`; chunked so a large batch +/// of files cannot produce an absurdly long filter string. +const DELETE_CHUNK: usize = 400; + +/// LanceDB-based relations store. +pub struct LanceRelationsStore { + /// Path to the database directory + db_path: PathBuf, + /// Database connection (lazy initialized) + db: Arc>>, +} + +impl LanceRelationsStore { + /// Create a new LanceDB relations store + pub async fn new(db_path: PathBuf) -> Result { + tokio::fs::create_dir_all(&db_path) + .await + .context("Failed to create relations database directory")?; + + Ok(Self { + db_path, + db: Arc::new(RwLock::new(None)), + }) + } + + /// Get or create the database connection + async fn get_connection(&self) -> Result { + let mut db_guard = self.db.write().await; + + if let Some(ref db) = *db_guard { + return Ok(db.clone()); + } + + let db = lancedb::connect(self.db_path.to_string_lossy().as_ref()) + .execute() + .await + .context("Failed to connect to LanceDB")?; + + *db_guard = Some(db.clone()); + Ok(db) + } + + /// Open a table, creating it empty with the given schema if it does not exist. + async fn open_or_create(&self, name: &str, schema: Arc) -> Result { + let db = self.get_connection().await?; + + if let Ok(table) = db.open_table(name).execute().await { + return Ok(table); + } + + let empty = RecordBatch::new_empty(schema.clone()); + let batches = RecordBatchIterator::new(vec![empty].into_iter().map(Ok), schema); + match db.create_table(name, Box::new(batches)).execute().await { + Ok(table) => Ok(table), + // Lost a creation race; the table exists now, so open it. + Err(_) => db + .open_table(name) + .execute() + .await + .with_context(|| format!("Failed to open or create table {}", name)), + } + } + + async fn definitions_table(&self) -> Result
{ + self.open_or_create(DEFINITIONS_TABLE, codec::definitions_schema()) + .await + } + + async fn references_table(&self) -> Result
{ + self.open_or_create(REFERENCES_TABLE, codec::references_schema()) + .await + } + + async fn collect_batches(table: &Table, filter: &str) -> Result> { + let stream = table + .query() + .only_if(filter) + .execute() + .await + .with_context(|| format!("Failed to query with filter: {}", filter))?; + stream + .try_collect() + .await + .context("Failed to collect query results") + } + + async fn query_definitions(&self, filter: &str) -> Result> { + let table = self.definitions_table().await?; + let batches = Self::collect_batches(&table, filter).await?; + let mut out = Vec::new(); + for batch in &batches { + out.extend(codec::batch_to_definitions(batch)?); + } + Ok(out) + } + + async fn query_references(&self, filter: &str) -> Result> { + let table = self.references_table().await?; + let batches = Self::collect_batches(&table, filter).await?; + let mut out = Vec::new(); + for batch in &batches { + out.extend(codec::batch_to_references(batch)?); + } + Ok(out) + } + + /// Delete every row belonging to the given files. + async fn delete_files(table: &Table, files: &[String]) -> Result<()> { + for chunk in files.chunks(DELETE_CHUNK) { + let filter = format!("file_path IN ({})", codec::sql_in_list(chunk)); + table + .delete(&filter) + .await + .context("Failed to delete rows by file")?; + } + Ok(()) + } + + /// The innermost function or method in `definitions` whose span contains `line`. + fn enclosing_function(definitions: &[Definition], line: usize) -> Option<&Definition> { + definitions + .iter() + .filter(|d| { + matches!(d.symbol_id.kind, SymbolKind::Function | SymbolKind::Method) + && line >= d.symbol_id.start_line + && line <= d.end_line + }) + .min_by_key(|d| d.end_line.saturating_sub(d.symbol_id.start_line)) + } +} + +#[async_trait] +impl RelationsStore for LanceRelationsStore { + async fn store_definitions( + &self, + definitions: Vec, + _root_path: &str, + ) -> Result { + if definitions.is_empty() { + return Ok(0); + } + + let table = self.definitions_table().await?; + + // Idempotent per file: replace whatever rows those files had. + let files: Vec = definitions + .iter() + .map(|d| d.file_path().to_string()) + .collect::>() + .into_iter() + .collect(); + Self::delete_files(&table, &files).await?; + + let batch = codec::definitions_to_batch(&definitions)?; + let count = batch.num_rows(); + let batches = + RecordBatchIterator::new(vec![batch].into_iter().map(Ok), codec::definitions_schema()); + table + .add(Box::new(batches)) + .execute() + .await + .context("Failed to store definitions")?; + + tracing::debug!("Stored {} definitions for {} files", count, files.len()); + Ok(count) + } + + async fn store_references( + &self, + references: Vec, + _root_path: &str, + ) -> Result { + if references.is_empty() { + return Ok(0); + } + + let table = self.references_table().await?; + + let files: Vec = references + .iter() + .map(|r| r.file_path.clone()) + .collect::>() + .into_iter() + .collect(); + Self::delete_files(&table, &files).await?; + + let batch = codec::references_to_batch(&references)?; + let count = batch.num_rows(); + let batches = + RecordBatchIterator::new(vec![batch].into_iter().map(Ok), codec::references_schema()); + table + .add(Box::new(batches)) + .execute() + .await + .context("Failed to store references")?; + + tracing::debug!("Stored {} references for {} files", count, files.len()); + Ok(count) + } + + async fn find_definition_at( + &self, + file_path: &str, + line: usize, + _column: usize, + ) -> Result> { + let filter = format!( + "file_path = '{}' AND start_line <= {} AND end_line >= {}", + codec::escape_sql(file_path), + line, + line + ); + let matches = self.query_definitions(&filter).await?; + // Innermost definition wins: the nested symbol, not its container. + Ok(matches + .into_iter() + .min_by_key(|d| d.end_line.saturating_sub(d.symbol_id.start_line))) + } + + async fn find_definitions_by_name(&self, name: &str) -> Result> { + let filter = format!("name = '{}'", codec::escape_sql(name)); + self.query_definitions(&filter).await + } + + async fn find_references(&self, target_symbol_id: &str) -> Result> { + let filter = format!( + "target_symbol_id = '{}'", + codec::escape_sql(target_symbol_id) + ); + self.query_references(&filter).await + } + + async fn get_callers(&self, symbol_id: &str) -> Result> { + let filter = format!( + "target_symbol_id = '{}' AND reference_kind = '{}'", + codec::escape_sql(symbol_id), + codec::enum_to_str(&ReferenceKind::Call) + ); + let call_refs = self.query_references(&filter).await?; + if call_refs.is_empty() { + return Ok(Vec::new()); + } + + // Attribute each call site to the innermost function containing it in + // the file where the call occurs. + let files: Vec = call_refs + .iter() + .map(|r| r.file_path.clone()) + .collect::>() + .into_iter() + .collect(); + let defs_filter = format!("file_path IN ({})", codec::sql_in_list(&files)); + let defs = self.query_definitions(&defs_filter).await?; + + let mut defs_by_file: HashMap<&str, Vec> = HashMap::new(); + for def in &defs { + defs_by_file + .entry(def.file_path()) + .or_default() + .push(def.clone()); + } + + let mut seen = HashSet::new(); + let mut edges = Vec::new(); + for r in &call_refs { + let enclosing = defs_by_file + .get(r.file_path.as_str()) + .and_then(|file_defs| Self::enclosing_function(file_defs, r.start_line)); + if let Some(def) = enclosing { + let caller_id = def.to_storage_id(); + if seen.insert((caller_id.clone(), r.start_line)) { + edges.push(CallEdge { + caller_id, + callee_id: symbol_id.to_string(), + call_site_file: r.file_path.clone(), + call_site_line: r.start_line, + call_site_col: r.start_col, + }); + } + } + } + Ok(edges) + } + + async fn get_callees(&self, symbol_id: &str) -> Result> { + let filter = format!("id = '{}'", codec::escape_sql(symbol_id)); + let defs = self.query_definitions(&filter).await?; + let Some(def) = defs.first() else { + return Ok(Vec::new()); + }; + + let refs_filter = format!( + "file_path = '{}' AND reference_kind = '{}' AND start_line >= {} AND start_line <= {}", + codec::escape_sql(def.file_path()), + codec::enum_to_str(&ReferenceKind::Call), + def.start_line(), + def.end_line + ); + let call_refs = self.query_references(&refs_filter).await?; + + let mut seen = HashSet::new(); + Ok(call_refs + .into_iter() + .filter(|r| seen.insert((r.target_symbol_id.clone(), r.start_line))) + .map(|r| CallEdge { + caller_id: symbol_id.to_string(), + callee_id: r.target_symbol_id, + call_site_file: r.file_path, + call_site_line: r.start_line, + call_site_col: r.start_col, + }) + .collect()) + } + + async fn delete_by_file(&self, file_path: &str) -> Result { + let filter = format!("file_path = '{}'", codec::escape_sql(file_path)); + + let defs_table = self.definitions_table().await?; + let refs_table = self.references_table().await?; + + // LanceDB's delete does not report a count, so count first. + let removed = defs_table + .count_rows(Some(filter.clone())) + .await + .unwrap_or(0) + + refs_table + .count_rows(Some(filter.clone())) + .await + .unwrap_or(0); + + defs_table + .delete(&filter) + .await + .context("Failed to delete definitions for file")?; + refs_table + .delete(&filter) + .await + .context("Failed to delete references for file")?; + + Ok(removed) + } + + async fn clear(&self) -> Result<()> { + let db = self.get_connection().await?; + for name in [DEFINITIONS_TABLE, REFERENCES_TABLE] { + if let Err(e) = db.drop_table(name, &[]).await { + // Dropping a table that was never created is not an error worth failing on. + tracing::debug!("Dropping relations table {} failed: {}", name, e); + } + } + Ok(()) + } + + async fn get_stats(&self) -> Result { + let defs_table = self.definitions_table().await?; + let refs_table = self.references_table().await?; + + let definition_count = defs_table + .count_rows(None) + .await + .context("Failed to count definitions")?; + let reference_count = refs_table + .count_rows(None) + .await + .context("Failed to count references")?; + + // Distinct files with definitions. + let stream = defs_table + .query() + .select(lancedb::query::Select::Columns(vec![ + "file_path".to_string(), + ])) + .execute() + .await + .context("Failed to query definition files")?; + let batches: Vec = stream + .try_collect() + .await + .context("Failed to collect definition files")?; + + let mut files = HashSet::new(); + for batch in &batches { + if let Some(paths) = batch + .column_by_name("file_path") + .and_then(|c| c.as_any().downcast_ref::()) + { + for i in 0..batch.num_rows() { + files.insert(paths.value(i).to_string()); + } + } + } + + Ok(RelationsStats { + definition_count, + reference_count, + files_with_definitions: files.len(), + }) + } +} + +#[cfg(test)] +mod tests; diff --git a/src/relations/storage/lance_store/tests.rs b/src/relations/storage/lance_store/tests.rs new file mode 100644 index 0000000..1a2fcde --- /dev/null +++ b/src/relations/storage/lance_store/tests.rs @@ -0,0 +1,263 @@ +use super::*; +use crate::relations::types::{SymbolId, Visibility}; +use tempfile::TempDir; + +fn make_def(name: &str, file: &str, start: usize, end: usize, kind: SymbolKind) -> Definition { + Definition { + symbol_id: SymbolId::new(file, name, kind, start, 0), + root_path: Some("/test".to_string()), + project: Some("proj".to_string()), + end_line: end, + end_col: 1, + signature: format!("fn {}()", name), + doc_comment: None, + visibility: Visibility::Public, + parent_id: None, + indexed_at: 42, + } +} + +fn make_call_ref(target_id: &str, file: &str, line: usize) -> Reference { + Reference { + file_path: file.to_string(), + root_path: Some("/test".to_string()), + project: Some("proj".to_string()), + start_line: line, + end_line: line, + start_col: 4, + end_col: 10, + target_symbol_id: target_id.to_string(), + reference_kind: ReferenceKind::Call, + indexed_at: 42, + } +} + +async fn make_store() -> (TempDir, LanceRelationsStore) { + let temp_dir = TempDir::new().unwrap(); + let store = LanceRelationsStore::new(temp_dir.path().to_path_buf()) + .await + .unwrap(); + (temp_dir, store) +} + +#[tokio::test] +async fn test_store_creation() { + let (_dir, store) = make_store().await; + let stats = store.get_stats().await.unwrap(); + assert_eq!(stats.definition_count, 0); + assert_eq!(stats.reference_count, 0); +} + +#[tokio::test] +async fn test_store_empty_definitions() { + let (_dir, store) = make_store().await; + let count = store.store_definitions(Vec::new(), "/test").await.unwrap(); + assert_eq!(count, 0); +} + +#[tokio::test] +async fn test_definitions_roundtrip() { + let (_dir, store) = make_store().await; + + let defs = vec![ + make_def("greet", "src/lib.rs", 10, 20, SymbolKind::Function), + make_def("Person", "src/lib.rs", 30, 50, SymbolKind::Struct), + ]; + let count = store.store_definitions(defs, "/test").await.unwrap(); + assert_eq!(count, 2); + + let found = store.find_definitions_by_name("greet").await.unwrap(); + assert_eq!(found.len(), 1); + let def = &found[0]; + assert_eq!(def.name(), "greet"); + assert_eq!(def.kind(), SymbolKind::Function); + assert_eq!(def.file_path(), "src/lib.rs"); + assert_eq!(def.start_line(), 10); + assert_eq!(def.end_line, 20); + assert_eq!(def.visibility, Visibility::Public); + assert_eq!(def.project.as_deref(), Some("proj")); + assert_eq!(def.indexed_at, 42); +} + +#[tokio::test] +async fn test_store_is_idempotent_per_file() { + let (_dir, store) = make_store().await; + + let defs = vec![make_def( + "greet", + "src/lib.rs", + 10, + 20, + SymbolKind::Function, + )]; + store + .store_definitions(defs.clone(), "/test") + .await + .unwrap(); + store.store_definitions(defs, "/test").await.unwrap(); + + let stats = store.get_stats().await.unwrap(); + assert_eq!( + stats.definition_count, 1, + "re-storing the same file must not duplicate rows" + ); +} + +#[tokio::test] +async fn test_find_definition_at_innermost() { + let (_dir, store) = make_store().await; + + // A method nested inside a class: line 12 is inside both. + let defs = vec![ + make_def("MyClass", "src/lib.rs", 1, 100, SymbolKind::Class), + make_def("helper", "src/lib.rs", 10, 15, SymbolKind::Method), + ]; + store.store_definitions(defs, "/test").await.unwrap(); + + let found = store + .find_definition_at("src/lib.rs", 12, 0) + .await + .unwrap() + .expect("should find a definition"); + assert_eq!(found.name(), "helper", "innermost definition must win"); + + let outer = store + .find_definition_at("src/lib.rs", 50, 0) + .await + .unwrap() + .expect("should find the class"); + assert_eq!(outer.name(), "MyClass"); + + let none = store + .find_definition_at("src/lib.rs", 200, 0) + .await + .unwrap(); + assert!(none.is_none()); +} + +#[tokio::test] +async fn test_references_roundtrip_and_delete_by_file() { + let (_dir, store) = make_store().await; + + let target = make_def("greet", "src/lib.rs", 10, 20, SymbolKind::Function); + let target_id = target.to_storage_id(); + store + .store_definitions(vec![target], "/test") + .await + .unwrap(); + store + .store_references( + vec![ + make_call_ref(&target_id, "src/main.rs", 5), + make_call_ref(&target_id, "src/other.rs", 7), + ], + "/test", + ) + .await + .unwrap(); + + let refs = store.find_references(&target_id).await.unwrap(); + assert_eq!(refs.len(), 2); + assert!(refs.iter().all(|r| r.reference_kind == ReferenceKind::Call)); + + // Deleting one file removes its references but not the other file's. + let removed = store.delete_by_file("src/main.rs").await.unwrap(); + assert_eq!(removed, 1); + let refs = store.find_references(&target_id).await.unwrap(); + assert_eq!(refs.len(), 1); + assert_eq!(refs[0].file_path, "src/other.rs"); + + // Deleting the defining file removes the definition. + let removed = store.delete_by_file("src/lib.rs").await.unwrap(); + assert_eq!(removed, 1); + assert!( + store + .find_definitions_by_name("greet") + .await + .unwrap() + .is_empty() + ); +} + +#[tokio::test] +async fn test_callers_and_callees() { + let (_dir, store) = make_store().await; + + // callee `greet` in lib.rs; caller `main` in main.rs calls it at line 5. + let greet = make_def("greet", "src/lib.rs", 10, 20, SymbolKind::Function); + let main_fn = make_def("main", "src/main.rs", 1, 30, SymbolKind::Function); + let greet_id = greet.to_storage_id(); + let main_id = main_fn.to_storage_id(); + + store + .store_definitions(vec![greet, main_fn], "/test") + .await + .unwrap(); + store + .store_references(vec![make_call_ref(&greet_id, "src/main.rs", 5)], "/test") + .await + .unwrap(); + + let callers = store.get_callers(&greet_id).await.unwrap(); + assert_eq!(callers.len(), 1); + assert_eq!(callers[0].caller_id, main_id); + assert_eq!(callers[0].callee_id, greet_id); + assert_eq!(callers[0].call_site_file, "src/main.rs"); + assert_eq!(callers[0].call_site_line, 5); + + let callees = store.get_callees(&main_id).await.unwrap(); + assert_eq!(callees.len(), 1); + assert_eq!(callees[0].caller_id, main_id); + assert_eq!(callees[0].callee_id, greet_id); + + // A symbol with no calls has neither callers nor callees. + let callees = store.get_callees(&greet_id).await.unwrap(); + assert!(callees.is_empty()); +} + +#[tokio::test] +async fn test_clear_and_stats() { + let (_dir, store) = make_store().await; + + store + .store_definitions( + vec![ + make_def("a", "src/a.rs", 1, 5, SymbolKind::Function), + make_def("b", "src/b.rs", 1, 5, SymbolKind::Function), + ], + "/test", + ) + .await + .unwrap(); + store + .store_references( + vec![make_call_ref("def:src/a.rs:a:1", "src/b.rs", 3)], + "/test", + ) + .await + .unwrap(); + + let stats = store.get_stats().await.unwrap(); + assert_eq!(stats.definition_count, 2); + assert_eq!(stats.reference_count, 1); + assert_eq!(stats.files_with_definitions, 2); + + store.clear().await.unwrap(); + + let stats = store.get_stats().await.unwrap(); + assert_eq!(stats.definition_count, 0); + assert_eq!(stats.reference_count, 0); + assert_eq!(stats.files_with_definitions, 0); +} + +#[tokio::test] +async fn test_sql_escaping_in_paths() { + let (_dir, store) = make_store().await; + + // A path containing a single quote must not break the delete filter. + let defs = vec![make_def("f", "src/it's.rs", 1, 5, SymbolKind::Function)]; + store.store_definitions(defs, "/test").await.unwrap(); + + let removed = store.delete_by_file("src/it's.rs").await.unwrap(); + assert_eq!(removed, 1); +} diff --git a/src/relations/types.rs b/src/relations/types.rs index 114ccb9..d6d979f 100644 --- a/src/relations/types.rs +++ b/src/relations/types.rs @@ -127,6 +127,17 @@ impl SymbolKind { | "type_declaration" // Go => Self::TypeAlias, + // Imports + "use_declaration" // Rust + | "extern_crate_declaration" // Rust + | "import_statement" // Python, JS/TS + | "import_from_statement" // Python + | "import_declaration" // Go, Java, Swift + | "preproc_include" // C/C++ + | "using_directive" // C# + | "namespace_use_declaration" // PHP + => Self::Import, + _ => Self::Unknown, } } @@ -324,6 +335,22 @@ pub struct Definition { } impl Definition { + /// Extract the symbol name from an id produced by [`Definition::to_storage_id`]. + /// + /// The layout is `def:::` -- note this differs from + /// [`SymbolId::to_storage_id`], which is `:::`. Reaching for + /// the wrong one is what kept get_call_graph callees permanently empty. + /// + /// Fields are taken from the RIGHT because `file_path` may itself contain a colon + /// (a Windows drive letter). + pub fn name_from_storage_id(id: &str) -> Option<&str> { + let rest = id.strip_prefix("def:")?; + let mut parts = rest.rsplitn(3, ':'); + let _line = parts.next()?; + let name = parts.next()?; + if name.is_empty() { None } else { Some(name) } + } + /// Generate a unique storage ID for this definition pub fn to_storage_id(&self) -> String { format!( @@ -518,6 +545,24 @@ pub struct CallGraphNode { } /// Symbol info for call graph root +/// A node the extractor recognised as a definition but could not name, and +/// therefore left out of the symbol list. +/// +/// Before this existed such nodes were dropped silently, so a caller had no way to +/// tell an empty or short symbol list from a complete one. A non-empty vector of +/// these means the listing is INCOMPLETE. +#[derive(Debug, Clone, Serialize, Deserialize, JsonSchema)] +pub struct SkippedDefinition { + /// 1-based line the definition starts on + pub line: usize, + /// tree-sitter node kind, e.g. "function_definition" + pub kind: String, + /// Why it was skipped + pub reason: String, + /// First line of the node text, truncated -- enough to identify it by eye + pub snippet: String, +} + #[derive(Debug, Clone, Serialize, Deserialize, JsonSchema)] pub struct SymbolInfo { /// Symbol name @@ -544,10 +589,22 @@ mod tests { #[test] fn test_symbol_kind_from_ast_kind() { - assert_eq!(SymbolKind::from_ast_kind("function_item"), SymbolKind::Function); - assert_eq!(SymbolKind::from_ast_kind("class_definition"), SymbolKind::Class); - assert_eq!(SymbolKind::from_ast_kind("method_definition"), SymbolKind::Method); - assert_eq!(SymbolKind::from_ast_kind("unknown_node"), SymbolKind::Unknown); + assert_eq!( + SymbolKind::from_ast_kind("function_item"), + SymbolKind::Function + ); + assert_eq!( + SymbolKind::from_ast_kind("class_definition"), + SymbolKind::Class + ); + assert_eq!( + SymbolKind::from_ast_kind("method_definition"), + SymbolKind::Method + ); + assert_eq!( + SymbolKind::from_ast_kind("unknown_node"), + SymbolKind::Unknown + ); } #[test] @@ -560,9 +617,18 @@ mod tests { #[test] fn test_visibility_from_keywords() { assert_eq!(Visibility::from_keywords("pub fn foo"), Visibility::Public); - assert_eq!(Visibility::from_keywords("public void bar"), Visibility::Public); - assert_eq!(Visibility::from_keywords("protected int x"), Visibility::Protected); - assert_eq!(Visibility::from_keywords("fn private_func"), Visibility::Private); + assert_eq!( + Visibility::from_keywords("public void bar"), + Visibility::Public + ); + assert_eq!( + Visibility::from_keywords("protected int x"), + Visibility::Protected + ); + assert_eq!( + Visibility::from_keywords("fn private_func"), + Visibility::Private + ); } #[test] diff --git a/src/types/file_ops.rs b/src/types/file_ops.rs new file mode 100644 index 0000000..d63bbd1 --- /dev/null +++ b/src/types/file_ops.rs @@ -0,0 +1,337 @@ +//! Request/response types for reading and editing files that live inside an +//! already-indexed project root. + +use schemars::JsonSchema; +use serde::{Deserialize, Serialize}; + +/// Request to read a slice (or all) of a file's current on-disk content +#[derive(Debug, Clone, Serialize, Deserialize, JsonSchema)] +pub struct ReadFileRequest { + /// File path (relative or absolute). Must be inside an already-indexed project root. + pub file_path: String, + /// First line to read, 1-indexed inclusive. Omit to read from the start of the file. + #[serde(default)] + pub start_line: Option, + /// Last line to read, 1-indexed inclusive. Omit to read to the end of the file. + #[serde(default)] + pub end_line: Option, +} + +impl ReadFileRequest { + /// Validate the read file request + pub fn validate(&self) -> Result<(), String> { + if self.file_path.is_empty() { + return Err("file_path cannot be empty".to_string()); + } + if self.start_line == Some(0) { + return Err("start_line must be >= 1".to_string()); + } + if let (Some(start), Some(end)) = (self.start_line, self.end_line) + && start > end + { + return Err("start_line must be <= end_line".to_string()); + } + Ok(()) + } +} + +/// Response from read_file +#[derive(Debug, Clone, Serialize, Deserialize, JsonSchema)] +pub struct ReadFileResponse { + /// The file content for the requested (and possibly clamped/capped) line range + pub content: String, + /// First line actually returned, 1-indexed inclusive. 0 if the file is empty. + pub start_line: usize, + /// Last line actually returned, 1-indexed inclusive. 0 if the file is empty. + pub end_line: usize, + /// Total number of lines in the file + pub total_lines: usize, + /// True if the requested range was clamped to file bounds or capped to the + /// per-call line limit; if true, `start_line`/`end_line` show what was actually returned + pub truncated: bool, + /// SHA256 hash of the full current file content. Pass this as `expected_hash` + /// to edit_file to guard against editing a file that changed since this read. + pub file_hash: String, + /// Detected language, if any + pub language: Option, +} + +/// Request to replace a line range (or the whole file) with new content +#[derive(Debug, Clone, Serialize, Deserialize, JsonSchema)] +pub struct EditFileRequest { + /// File path (relative or absolute). Must be inside an already-indexed project root. + pub file_path: String, + /// New content. Replaces the given line range, or the entire file (creating it if it + /// doesn't exist yet) when start_line/end_line are omitted. + pub content: String, + /// First line to replace, 1-indexed inclusive. Omit (with end_line) to replace/create + /// the whole file. Set to `end_line + 1` to insert `content` before that line without + /// deleting anything. + #[serde(default)] + pub start_line: Option, + /// Last line to replace, 1-indexed inclusive. Omit (with start_line) to replace/create + /// the whole file. + #[serde(default)] + pub end_line: Option, + /// SHA256 hash the file is expected to currently have (from a prior read_file/edit_file + /// call). If it doesn't match the file's actual current hash, the edit is rejected + /// instead of applied. Omit only when creating a brand new file. + #[serde(default)] + pub expected_hash: Option, + /// Project name to tag re-indexed embeddings with; should match what index_codebase + /// used for this root, if anything. + #[serde(default)] + pub project: Option, +} + +impl EditFileRequest { + /// Validate the edit file request + pub fn validate(&self) -> Result<(), String> { + if self.file_path.is_empty() { + return Err("file_path cannot be empty".to_string()); + } + match (self.start_line, self.end_line) { + (Some(start), Some(end)) => { + if start == 0 { + return Err("start_line must be >= 1".to_string()); + } + if start > end + 1 { + return Err( + "start_line must be <= end_line + 1 (use end_line + 1 to insert without deleting)" + .to_string(), + ); + } + } + (None, None) => {} + _ => { + return Err("start_line and end_line must both be set or both omitted".to_string()); + } + } + Ok(()) + } +} + +/// Response from edit_file +#[derive(Debug, Clone, Serialize, Deserialize, JsonSchema)] +pub struct EditFileResponse { + /// Outcome of the request: "ok" or "hash_conflict" + pub status: String, + /// SHA256 hash of the file after the edit (only set when status == "ok") + #[serde(default)] + pub file_hash: Option, + /// Total number of lines in the file after the edit (only set when status == "ok") + #[serde(default)] + pub total_lines: Option, + /// The hash the caller expected the file to have (only set when status == "hash_conflict") + #[serde(default)] + pub expected_hash: Option, + /// The file's actual current hash, or null if the file doesn't exist yet + /// (only set when status == "hash_conflict") + #[serde(default)] + pub actual_hash: Option, + /// Whether the index was successfully refreshed for this file after the write + pub reindexed: bool, + /// Set if the write succeeded but reindexing failed; the affected root is marked + /// dirty and will be repaired by the next index_codebase call + #[serde(default)] + pub warning: Option, + /// Time taken in milliseconds + pub duration_ms: u64, +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn test_read_file_request_validate_empty_path() { + let req = ReadFileRequest { + file_path: String::new(), + start_line: None, + end_line: None, + }; + assert!(req.validate().is_err()); + } + + #[test] + fn test_read_file_request_validate_zero_start() { + let req = ReadFileRequest { + file_path: "a.rs".to_string(), + start_line: Some(0), + end_line: None, + }; + assert!(req.validate().is_err()); + } + + #[test] + fn test_read_file_request_validate_start_after_end() { + let req = ReadFileRequest { + file_path: "a.rs".to_string(), + start_line: Some(10), + end_line: Some(5), + }; + assert!(req.validate().is_err()); + } + + #[test] + fn test_read_file_request_validate_ok() { + let req = ReadFileRequest { + file_path: "a.rs".to_string(), + start_line: Some(1), + end_line: Some(5), + }; + assert!(req.validate().is_ok()); + } + + #[test] + fn test_read_file_request_serde_roundtrip() { + let req = ReadFileRequest { + file_path: "a.rs".to_string(), + start_line: Some(1), + end_line: Some(5), + }; + let json = serde_json::to_string(&req).unwrap(); + let deserialized: ReadFileRequest = serde_json::from_str(&json).unwrap(); + assert_eq!(req.file_path, deserialized.file_path); + assert_eq!(req.start_line, deserialized.start_line); + assert_eq!(req.end_line, deserialized.end_line); + } + + #[test] + fn test_read_file_request_defaults_when_omitted() { + let json = r#"{"file_path":"a.rs"}"#; + let req: ReadFileRequest = serde_json::from_str(json).unwrap(); + assert_eq!(req.start_line, None); + assert_eq!(req.end_line, None); + } + + #[test] + fn test_edit_file_request_validate_empty_path() { + let req = EditFileRequest { + file_path: String::new(), + content: "x".to_string(), + start_line: None, + end_line: None, + expected_hash: None, + project: None, + }; + assert!(req.validate().is_err()); + } + + #[test] + fn test_edit_file_request_validate_whole_file_ok() { + let req = EditFileRequest { + file_path: "a.rs".to_string(), + content: "x".to_string(), + start_line: None, + end_line: None, + expected_hash: None, + project: None, + }; + assert!(req.validate().is_ok()); + } + + #[test] + fn test_edit_file_request_validate_mixed_none_some_rejected() { + let req = EditFileRequest { + file_path: "a.rs".to_string(), + content: "x".to_string(), + start_line: Some(1), + end_line: None, + expected_hash: None, + project: None, + }; + assert!(req.validate().is_err()); + } + + #[test] + fn test_edit_file_request_validate_range_ok() { + let req = EditFileRequest { + file_path: "a.rs".to_string(), + content: "x".to_string(), + start_line: Some(2), + end_line: Some(5), + expected_hash: None, + project: None, + }; + assert!(req.validate().is_ok()); + } + + #[test] + fn test_edit_file_request_validate_insert_only_ok() { + // start_line == end_line + 1 means "insert before start_line, delete nothing" + let req = EditFileRequest { + file_path: "a.rs".to_string(), + content: "x".to_string(), + start_line: Some(6), + end_line: Some(5), + expected_hash: None, + project: None, + }; + assert!(req.validate().is_ok()); + } + + #[test] + fn test_edit_file_request_validate_start_too_far_past_end_rejected() { + let req = EditFileRequest { + file_path: "a.rs".to_string(), + content: "x".to_string(), + start_line: Some(7), + end_line: Some(5), + expected_hash: None, + project: None, + }; + assert!(req.validate().is_err()); + } + + #[test] + fn test_edit_file_request_validate_zero_start_rejected() { + let req = EditFileRequest { + file_path: "a.rs".to_string(), + content: "x".to_string(), + start_line: Some(0), + end_line: Some(0), + expected_hash: None, + project: None, + }; + assert!(req.validate().is_err()); + } + + #[test] + fn test_edit_file_response_serde_roundtrip() { + let resp = EditFileResponse { + status: "ok".to_string(), + file_hash: Some("abc123".to_string()), + total_lines: Some(42), + expected_hash: None, + actual_hash: None, + reindexed: true, + warning: None, + duration_ms: 12, + }; + let json = serde_json::to_string(&resp).unwrap(); + let deserialized: EditFileResponse = serde_json::from_str(&json).unwrap(); + assert_eq!(resp.status, deserialized.status); + assert_eq!(resp.file_hash, deserialized.file_hash); + assert_eq!(resp.total_lines, deserialized.total_lines); + assert_eq!(resp.reindexed, deserialized.reindexed); + } + + #[test] + fn test_read_file_response_serde_roundtrip() { + let resp = ReadFileResponse { + content: "fn main() {}\n".to_string(), + start_line: 1, + end_line: 1, + total_lines: 1, + truncated: false, + file_hash: "abc123".to_string(), + language: Some("Rust".to_string()), + }; + let json = serde_json::to_string(&resp).unwrap(); + let deserialized: ReadFileResponse = serde_json::from_str(&json).unwrap(); + assert_eq!(resp.content, deserialized.content); + assert_eq!(resp.file_hash, deserialized.file_hash); + assert_eq!(resp.language, deserialized.language); + } +} diff --git a/src/types/find_unused.rs b/src/types/find_unused.rs new file mode 100644 index 0000000..0ba964d --- /dev/null +++ b/src/types/find_unused.rs @@ -0,0 +1,286 @@ +//! Request/response types for the find_unused tool: report import bindings and +//! symbol definitions that nothing references. + +use schemars::JsonSchema; +use serde::{Deserialize, Serialize}; + +use crate::relations::SymbolKind; + +fn default_check() -> String { + "all".to_string() +} + +fn default_limit() -> usize { + 100 +} + +fn default_max_file_size() -> usize { + 1_048_576 +} + +/// Request to scan a file or directory for unused imports and symbols +#[derive(Debug, Clone, Serialize, Deserialize, JsonSchema)] +pub struct FindUnusedRequest { + /// File or directory to scan. Directories are walked with .gitignore support. + pub path: String, + /// Optional project name (used when probing the index for cross-file usage) + #[serde(default)] + pub project: Option, + /// What to check: "imports" (unused import/use/include bindings), + /// "symbols" (definitions nothing references), or "all" (default) + #[serde(default = "default_check")] + pub check: String, + /// Maximum number of candidates to return (default 100) + #[serde(default = "default_limit")] + pub limit: usize, + /// Maximum file size in bytes to scan (default 1 MB) + #[serde(default = "default_max_file_size")] + pub max_file_size: usize, +} + +impl FindUnusedRequest { + /// Validate the find unused request + pub fn validate(&self) -> Result<(), String> { + if self.path.is_empty() { + return Err("path cannot be empty".to_string()); + } + if !matches!(self.check.as_str(), "imports" | "symbols" | "all") { + return Err(format!( + "check must be one of 'imports', 'symbols', 'all' (got '{}')", + self.check + )); + } + if self.limit == 0 { + return Err("limit must be >= 1".to_string()); + } + Ok(()) + } + + /// Whether unused imports should be checked + pub fn check_imports(&self) -> bool { + matches!(self.check.as_str(), "imports" | "all") + } + + /// Whether unused symbols should be checked + pub fn check_symbols(&self) -> bool { + matches!(self.check.as_str(), "symbols" | "all") + } +} + +/// A definition or import binding that appears to be unused +#[derive(Debug, Clone, Serialize, Deserialize, JsonSchema)] +pub struct UnusedCandidate { + /// File path (relative to the scanned root) containing the candidate + pub file_path: String, + /// The unused name (import binding, function, class, ...) + pub name: String, + /// Symbol kind; `import` for unused imports + pub kind: SymbolKind, + /// Line the definition or import starts on (1-based) + pub line: usize, + /// How much to trust this finding: "high", "medium", or "low". + /// This tool is text-based: dynamic dispatch, macros, reflection and + /// framework wiring are invisible to it, so treat candidates as leads to + /// verify, not facts. Never delete code from this list automatically. + pub confidence: String, + /// Why this was flagged + pub reason: String, + /// The definition/import line, for eyeballing without opening the file + pub signature: String, + /// What the analysis actually searched for before flagging: the identifier + /// probed, or the header symbols probed for a C/C++ include + #[serde(default)] + pub probe: String, +} + +/// An import binding that could not be verified and was therefore NOT flagged +#[derive(Debug, Clone, Default, Serialize, Deserialize, JsonSchema)] +pub struct UnverifiableImport { + /// File path (relative to the scanned root) containing the import + pub file_path: String, + /// The imported name as written + pub name: String, + /// Why it could not be verified + pub reason: String, +} + +/// Why symbol definitions were NOT flagged, by rejection cause. Large +/// `used_elsewhere` counts on C/C++ scans are expected: header declarations +/// count as usage, so paired .h/.cpp symbols are rejected there. +#[derive(Debug, Clone, Default, Serialize, Deserialize, JsonSchema)] +pub struct SymbolRejections { + /// Name appears in its own file outside import lines and its own definition + pub used_here: usize, + /// Name appears in another scanned file outside import lines and + /// same-name definition spans + pub used_elsewhere: usize, + /// Definition kind/shape is not checkable (imports, entry points, ...) + pub ineligible: usize, + /// Cross-index probes that errored; those symbols were conservatively + /// treated as used + pub probe_errors: usize, +} + +/// Response from find_unused +#[derive(Debug, Clone, Serialize, Deserialize, JsonSchema)] +pub struct FindUnusedResponse { + /// The root that was scanned (normalized) + pub scanned_root: String, + /// Number of files scanned + pub files_scanned: usize, + /// Total definitions extracted and considered + pub definitions_checked: usize, + /// Unused candidates found, ordered by file then line + pub candidates: Vec, + /// Total candidates found (may exceed candidates.len() when truncated) + pub total_candidates: usize, + /// Import bindings that could not be verified and were therefore NOT + /// reported: system/`<...>` includes, unresolvable local headers, C# + /// namespace usings, Swift module imports, and files whose language is + /// unknown. Always the authoritative count. + pub unverifiable_imports: usize, + /// Per-import detail for the unverifiable count, capped at 200 entries + #[serde(default)] + pub unverifiable_import_details: Vec, + /// Why symbol definitions were rejected rather than flagged + #[serde(default)] + pub symbol_rejections: SymbolRejections, + /// Definition nodes the extractor recognised but could not name in the + /// scanned files; those symbols were never considered at all + #[serde(default)] + pub skipped_definitions: usize, + /// True if candidates were cut to `limit` + pub truncated: bool, + /// True if the cross-index probe budget ran out; symbols past the budget + /// were conservatively treated as used + pub probes_exhausted: bool, + /// Precision class of the method (text-based AST extraction + whole-word + /// matching), not a per-run quality measure; see symbol_rejections, + /// unverifiable_import_details and skipped_definitions for this run's + /// completeness + pub precision: String, + /// Time taken in milliseconds + pub duration_ms: u64, +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn test_request_defaults_when_omitted() { + let json = r#"{"path":"src"}"#; + let req: FindUnusedRequest = serde_json::from_str(json).unwrap(); + assert_eq!(req.check, "all"); + assert_eq!(req.limit, 100); + assert_eq!(req.max_file_size, 1_048_576); + assert!(req.project.is_none()); + assert!(req.check_imports()); + assert!(req.check_symbols()); + } + + #[test] + fn test_request_validate_empty_path() { + let req = FindUnusedRequest { + path: String::new(), + project: None, + check: "all".to_string(), + limit: 100, + max_file_size: 1_048_576, + }; + assert!(req.validate().is_err()); + } + + #[test] + fn test_request_validate_bad_check() { + let req = FindUnusedRequest { + path: "src".to_string(), + project: None, + check: "everything".to_string(), + limit: 100, + max_file_size: 1_048_576, + }; + assert!(req.validate().is_err()); + } + + #[test] + fn test_request_validate_zero_limit() { + let req = FindUnusedRequest { + path: "src".to_string(), + project: None, + check: "imports".to_string(), + limit: 0, + max_file_size: 1_048_576, + }; + assert!(req.validate().is_err()); + } + + #[test] + fn test_request_check_flags() { + let mut req = FindUnusedRequest { + path: "src".to_string(), + project: None, + check: "imports".to_string(), + limit: 10, + max_file_size: 1_048_576, + }; + assert!(req.validate().is_ok()); + assert!(req.check_imports()); + assert!(!req.check_symbols()); + + req.check = "symbols".to_string(); + assert!(!req.check_imports()); + assert!(req.check_symbols()); + } + + #[test] + fn test_response_serde_roundtrip() { + let resp = FindUnusedResponse { + scanned_root: "/proj".to_string(), + files_scanned: 10, + definitions_checked: 50, + candidates: vec![UnusedCandidate { + file_path: "src/lib.rs".to_string(), + name: "HashMap".to_string(), + kind: SymbolKind::Import, + line: 3, + confidence: "medium".to_string(), + reason: "imported name is never referenced in this file".to_string(), + signature: "use std::collections::HashMap;".to_string(), + probe: "whole-word search for 'HashMap'".to_string(), + }], + total_candidates: 1, + unverifiable_imports: 2, + unverifiable_import_details: vec![UnverifiableImport { + file_path: "src/main.c".to_string(), + name: "stdio.h".to_string(), + reason: "system include (<...>); not resolvable".to_string(), + }], + symbol_rejections: SymbolRejections { + used_here: 4, + used_elsewhere: 3, + ineligible: 2, + probe_errors: 1, + }, + skipped_definitions: 1, + truncated: false, + probes_exhausted: false, + precision: "medium".to_string(), + duration_ms: 12, + }; + let json = serde_json::to_string(&resp).unwrap(); + let deserialized: FindUnusedResponse = serde_json::from_str(&json).unwrap(); + assert_eq!(deserialized.candidates.len(), 1); + assert_eq!(deserialized.candidates[0].name, "HashMap"); + assert_eq!(deserialized.candidates[0].kind, SymbolKind::Import); + assert_eq!( + deserialized.candidates[0].probe, + "whole-word search for 'HashMap'" + ); + assert_eq!(deserialized.unverifiable_imports, 2); + assert_eq!(deserialized.unverifiable_import_details.len(), 1); + assert_eq!(deserialized.unverifiable_import_details[0].name, "stdio.h"); + assert_eq!(deserialized.symbol_rejections.used_elsewhere, 3); + assert_eq!(deserialized.skipped_definitions, 1); + } +} diff --git a/src/types/mod.rs b/src/types/mod.rs index 308168e..83509fa 100644 --- a/src/types/mod.rs +++ b/src/types/mod.rs @@ -1,6 +1,13 @@ use schemars::JsonSchema; use serde::{Deserialize, Serialize}; +mod file_ops; +pub use file_ops::{EditFileRequest, EditFileResponse, ReadFileRequest, ReadFileResponse}; +mod find_unused; +pub use find_unused::{ + FindUnusedRequest, FindUnusedResponse, SymbolRejections, UnusedCandidate, UnverifiableImport, +}; + /// Request to index a codebase #[derive(Debug, Clone, Serialize, Deserialize, JsonSchema)] pub struct IndexRequest { @@ -150,6 +157,16 @@ pub struct StatisticsResponse { pub database_size_bytes: u64, /// Breakdown by programming language pub language_breakdown: Vec, + /// Total symbol definitions in the relations store (populated during indexing) + #[serde(default)] + pub total_definitions: usize, + /// Total symbol references in the relations store (populated on demand, + /// e.g. by tools that verify usage; 0 unless references were stored) + #[serde(default)] + pub total_references: usize, + /// Number of files with at least one stored definition + #[serde(default)] + pub files_with_definitions: usize, } #[derive(Debug, Clone, Serialize, Deserialize, JsonSchema)] @@ -395,7 +412,10 @@ impl FindReferencesRequest { } const MAX_LIMIT: usize = 10000; if self.limit > MAX_LIMIT { - return Err(format!("limit too large: {} (max: {})", self.limit, MAX_LIMIT)); + return Err(format!( + "limit too large: {} (max: {})", + self.limit, MAX_LIMIT + )); } Ok(()) } @@ -458,7 +478,10 @@ impl GetCallGraphRequest { } const MAX_DEPTH: usize = 10; if self.depth > MAX_DEPTH { - return Err(format!("depth too large: {} (max: {})", self.depth, MAX_DEPTH)); + return Err(format!( + "depth too large: {} (max: {})", + self.depth, MAX_DEPTH + )); } Ok(()) } @@ -479,6 +502,49 @@ pub struct GetCallGraphResponse { pub duration_ms: u64, } +/// Request to list every symbol defined in one file +#[derive(Debug, Clone, Serialize, Deserialize, JsonSchema)] +pub struct ListSymbolsRequest { + /// File path (relative or absolute) + pub file_path: String, + /// Optional project name to filter by + #[serde(default)] + pub project: Option, + /// Restrict to these symbol kinds (e.g. ["function", "method"]); empty means all + #[serde(default)] + pub kinds: Vec, +} + +impl ListSymbolsRequest { + /// Validate the list symbols request + pub fn validate(&self) -> Result<(), String> { + if self.file_path.is_empty() { + return Err("file_path cannot be empty".to_string()); + } + Ok(()) + } +} + +/// Response from list_symbols +#[derive(Debug, Clone, Serialize, Deserialize, JsonSchema)] +pub struct ListSymbolsResponse { + /// File the symbols were extracted from, relative to the indexed root + pub file_path: String, + /// Every definition found, ordered by start_line. Never includes file content. + pub symbols: Vec, + /// Number of symbols returned + pub total_count: usize, + /// Precision level of the extractor for this language + pub precision: String, + /// Definition nodes that were recognised but could not be named, and are therefore + /// MISSING from `symbols`. Empty is the normal case; anything here means this + /// listing is incomplete and must not be treated as an inventory. + #[serde(default, skip_serializing_if = "Vec::is_empty")] + pub skipped: Vec, + /// Time taken in milliseconds + pub duration_ms: u64, +} + /// Metadata stored with each code chunk #[derive(Debug, Clone, Serialize, Deserialize)] pub struct ChunkMetadata { diff --git a/src/types/tests.rs b/src/types/tests.rs index 00b5818..df9c6e2 100644 --- a/src/types/tests.rs +++ b/src/types/tests.rs @@ -159,11 +159,16 @@ fn test_statistics_response() { chunk_count: 100, }, ], + total_definitions: 250, + total_references: 0, + files_with_definitions: 90, }; assert_eq!(stats.total_files, 100); assert_eq!(stats.language_breakdown.len(), 2); assert_eq!(stats.language_breakdown[0].language, "Rust"); + assert_eq!(stats.total_definitions, 250); + assert_eq!(stats.files_with_definitions, 90); } // ===== Validation Tests ===== @@ -607,6 +612,9 @@ fn test_statistics_response_serialization() { file_count: 100, chunk_count: 500, }], + total_definitions: 250, + total_references: 10, + files_with_definitions: 90, }; let json = serde_json::to_string(&response).unwrap(); @@ -618,6 +626,15 @@ fn test_statistics_response_serialization() { response.language_breakdown.len(), deserialized.language_breakdown.len() ); + assert_eq!(response.total_definitions, deserialized.total_definitions); + assert_eq!(response.total_references, deserialized.total_references); + + // Old payloads without the relations fields must still deserialize. + let legacy = r#"{"total_files":1,"total_chunks":2,"total_embeddings":2,"database_size_bytes":10,"language_breakdown":[]}"#; + let parsed: StatisticsResponse = serde_json::from_str(legacy).unwrap(); + assert_eq!(parsed.total_definitions, 0); + assert_eq!(parsed.total_references, 0); + assert_eq!(parsed.files_with_definitions, 0); } #[test] diff --git a/src/vector_db/lance_client/mod.rs b/src/vector_db/lance_client/mod.rs index 4df0c7b..bf20390 100644 --- a/src/vector_db/lance_client/mod.rs +++ b/src/vector_db/lance_client/mod.rs @@ -11,7 +11,7 @@ use crate::bm25_search::BM25Search; use crate::glob_utils; use crate::types::{ChunkMetadata, SearchResult}; -use crate::vector_db::{DatabaseStats, VectorDatabase}; +use crate::vector_db::{DatabaseStats, LanguageBreakdown, VectorDatabase}; use anyhow::{Context, Result}; use arrow_array::{ Array, FixedSizeListArray, Float32Array, RecordBatch, RecordBatchIterator, StringArray, @@ -23,7 +23,8 @@ use lancedb::Table; use lancedb::connection::Connection; use lancedb::query::{ExecutableQuery, QueryBase}; use sha2::{Digest, Sha256}; -use std::collections::HashMap; +use std::collections::{HashMap, HashSet}; +use std::path::Path; use std::sync::{Arc, RwLock}; /// LanceDB vector database implementation (embedded, no server required) @@ -37,6 +38,21 @@ pub struct LanceVectorDB { bm25_indexes: Arc>>, } +/// Total on-disk size of every file under the given directory. +/// +/// LanceDB stores a directory tree rather than a single file. Entries that +/// cannot be read are skipped rather than failing the whole statistics call, +/// so this is a best-effort figure. +fn directory_size_bytes(path: &Path) -> u64 { + walkdir::WalkDir::new(path) + .into_iter() + .filter_map(|entry| entry.ok()) + .filter_map(|entry| entry.metadata().ok()) + .filter(|metadata| metadata.is_file()) + .map(|metadata| metadata.len()) + .sum() +} + impl LanceVectorDB { /// Create a new LanceDB instance with default path pub async fn new() -> Result { @@ -127,6 +143,19 @@ impl LanceVectorDB { Ok(()) } + /// The chunk's stable identity, and the fusion key shared by the vector table's `id` + /// column and the BM25 index. Both arms MUST derive it here; deriving it in two places + /// is how they silently drifted apart and killed hybrid search. + /// + /// `file_hash` is part of the key because git commits are stored as chunks whose + /// file_path is `git://` and whose start_line is 0 -- identical for EVERY commit + /// in a repository. Keyed on path and line alone they would all collapse onto one id + /// and the BM25 index would hold a single document for the entire history. For code + /// chunks the hash is constant within a file, so start_line still provides uniqueness. + fn chunk_id(meta: &ChunkMetadata) -> String { + format!("{}:{}:{}", meta.file_path, meta.start_line, meta.file_hash) + } + /// Create schema for the embeddings table fn create_schema(dimension: usize) -> Arc { Arc::new(Schema::new(vec![ @@ -182,7 +211,7 @@ impl LanceVectorDB { // Create arrays for each field let id_array = StringArray::from( (0..num_rows) - .map(|i| format!("{}:{}", metadata[i].file_path, metadata[i].start_line)) + .map(|i| Self::chunk_id(&metadata[i])) .collect::>(), ); let file_path_array = StringArray::from( @@ -319,9 +348,7 @@ impl VectorDatabase for LanceVectorDB { let dimension = embeddings[0].len(); let schema = Self::create_schema(dimension); - // Get current row count to use as starting ID for BM25 let table = self.get_table().await?; - let current_count = table.count_rows(None).await.unwrap_or(0) as u64; let batch = Self::create_record_batch( embeddings, @@ -342,11 +369,17 @@ impl VectorDatabase for LanceVectorDB { // Ensure BM25 index exists for this root path self.get_or_create_bm25(root_path)?; - // Add documents to per-project BM25 index with file_path for deletion tracking + // Add documents to per-project BM25 index, keyed by the SAME stable chunk id the + // vector table stores in its `id` column -- see Self::chunk_id. The previous + // `count_rows() + i` scheme was not merely a different key space, it was unstable: + // incremental re-indexing deletes and re-adds rows, so the row numbers drifted. let bm25_docs: Vec<_> = (0..count) .map(|i| { - let id = current_count + i as u64; - (id, contents[i].clone(), metadata[i].file_path.clone()) + ( + Self::chunk_id(&metadata[i]), + contents[i].clone(), + metadata[i].file_path.clone(), + ) }) .collect(); @@ -408,172 +441,234 @@ impl VectorDatabase for LanceVectorDB { .await .context("Failed to collect search results")?; - // Build vector results with row-based IDs - let mut vector_results = Vec::new(); - let mut row_offset = 0u64; - - // Store original scores for later reporting - let mut original_scores: HashMap)> = HashMap::new(); - - for batch in &results { + // Build vector results keyed by the chunk's stable `id` column. + // + // This used to key on `row_offset + i`, a position within THIS query's result + // batches, while the BM25 arm keyed on a table row number assigned at index + // time. The two spaces coincide only for the first insert into an empty table, + // so in any real index RRF fused two disjoint key sets: every vector hit scored + // exactly 1/(60+rank), keyword_score never populated, and BM25-only hits were + // dropped. Both arms now use `file_path:start_line`. + let mut vector_results: Vec<(String, f32)> = Vec::new(); + let mut original_scores: HashMap)> = HashMap::new(); + // chunk id -> (index into `results`, row within that batch) + let mut chunk_pos: HashMap = HashMap::new(); + + for (batch_idx, batch) in results.iter().enumerate() { let distance_array = batch .column_by_name("_distance") .context("Missing _distance column")? .as_any() .downcast_ref::() .context("Invalid _distance type")?; + let id_array = batch + .column_by_name("id") + .context("Missing id column")? + .as_any() + .downcast_ref::() + .context("Invalid id type")?; for i in 0..batch.num_rows() { let distance = distance_array.value(i); let score = 1.0 / (1.0 + distance); - let id = row_offset + i as u64; + let chunk_id = id_array.value(i).to_string(); // For hybrid search, don't filter by min_score before RRF // RRF will combine weak vector + strong keyword (or vice versa) // Filtering happens after RRF based on the combined ranking - vector_results.push((id, score)); - original_scores.insert(id, (score, None)); + vector_results.push((chunk_id.clone(), score)); + original_scores.insert(chunk_id.clone(), (score, None)); + chunk_pos.insert(chunk_id, (batch_idx, i)); } - row_offset += batch.num_rows() as u64; } - // BM25 keyword search across all per-project indexes - let bm25_indexes = self - .bm25_indexes - .read() - .map_err(|e| anyhow::anyhow!("Failed to acquire BM25 read lock: {}", e))?; + // BM25 keyword search across all per-project indexes. + // + // Scoped so the RwLockReadGuard is released at the end of the block: this + // function now awaits further down, and a guard held across an await makes the + // whole future non-Send, which the VectorDatabase trait requires. An explicit + // drop() is not enough -- the generator transform still captures it. + let bm25_results = { + let bm25_indexes = self + .bm25_indexes + .read() + .map_err(|e| anyhow::anyhow!("Failed to acquire BM25 read lock: {}", e))?; + + let mut all_bm25_results = Vec::new(); + for (root_hash, bm25) in bm25_indexes.iter() { + tracing::debug!("Searching BM25 index for root hash: {}", root_hash); + let results = bm25 + .search(query_text, search_limit) + .context("Failed to search BM25 index")?; + + // Store BM25 scores (don't filter - let RRF combine them) + // BM25 scores are not normalized to 0-1 range, so min_score doesn't apply + for result in &results { + original_scores + .entry(result.chunk_id.clone()) + .and_modify(|e| e.1 = Some(result.score)) + .or_insert((0.0, Some(result.score))); // No vector score, only keyword + } - let mut all_bm25_results = Vec::new(); - for (root_hash, bm25) in bm25_indexes.iter() { - tracing::debug!("Searching BM25 index for root hash: {}", root_hash); - let results = bm25 - .search(query_text, search_limit) - .context("Failed to search BM25 index")?; - - // Store BM25 scores (don't filter - let RRF combine them) - // BM25 scores are not normalized to 0-1 range, so min_score doesn't apply - for result in &results { - original_scores - .entry(result.id) - .and_modify(|e| e.1 = Some(result.score)) - .or_insert((0.0, Some(result.score))); // No vector score, only keyword + all_bm25_results.extend(results); } - - all_bm25_results.extend(results); - } - drop(bm25_indexes); - - let bm25_results = all_bm25_results; + all_bm25_results + }; // Combine results with Reciprocal Rank Fusion // RRF produces scores ~0.01-0.03, so don't apply min_score to combined scores let combined = crate::bm25_search::reciprocal_rank_fusion(vector_results, bm25_results, limit); - // Build final results by looking up the combined IDs in the vector results + // Build final results from the fused ranking. + // + // Vector-arm rows are materialised from the batches already in hand. Ids that + // ONLY BM25 matched are fetched from the table below -- without that step a + // pure keyword hit, which is the exact-symbol case hybrid search exists for, + // would be ranked and then silently dropped. + let missing: Vec = combined + .iter() + .map(|(id, _)| id) + .filter(|id| !chunk_pos.contains_key(*id)) + .cloned() + .collect(); + + let extra_batches: Vec = if missing.is_empty() { + Vec::new() + } else { + let quoted: Vec = missing + .iter() + .map(|id| format!("'{}'", id.replace('\'', "''"))) + .collect(); + match table + .query() + .only_if(format!("id IN ({})", quoted.join(", "))) + .execute() + .await + { + Ok(stream) => stream.try_collect().await.unwrap_or_else(|e| { + tracing::warn!("Failed to collect keyword-only rows: {}", e); + Vec::new() + }), + Err(e) => { + tracing::warn!("Failed to fetch keyword-only rows: {}", e); + Vec::new() + } + } + }; + + let mut extra_pos: HashMap = HashMap::new(); + for (batch_idx, batch) in extra_batches.iter().enumerate() { + if let Some(id_array) = batch + .column_by_name("id") + .and_then(|c| c.as_any().downcast_ref::()) + { + for i in 0..batch.num_rows() { + extra_pos.insert(id_array.value(i).to_string(), (batch_idx, i)); + } + } + } + let mut search_results = Vec::new(); - for (id, combined_score) in combined { - // Find this result in the original batch results - let mut found = false; - let mut batch_offset = 0u64; - - for batch in &results { - if id >= batch_offset && id < batch_offset + batch.num_rows() as u64 { - let idx = (id - batch_offset) as usize; - - let file_path_array = batch - .column_by_name("file_path") - .and_then(|c| c.as_any().downcast_ref::()); - let root_path_array = batch - .column_by_name("root_path") - .and_then(|c| c.as_any().downcast_ref::()); - let start_line_array = batch - .column_by_name("start_line") - .and_then(|c| c.as_any().downcast_ref::()); - let end_line_array = batch - .column_by_name("end_line") - .and_then(|c| c.as_any().downcast_ref::()); - let language_array = batch - .column_by_name("language") - .and_then(|c| c.as_any().downcast_ref::()); - let content_array = batch - .column_by_name("content") - .and_then(|c| c.as_any().downcast_ref::()); - let project_array = batch - .column_by_name("project") - .and_then(|c| c.as_any().downcast_ref::()); - - if let ( - Some(fp), - Some(rp), - Some(sl), - Some(el), - Some(lang), - Some(cont), - Some(proj), - ) = ( - file_path_array, - root_path_array, - start_line_array, - end_line_array, - language_array, - content_array, - project_array, - ) { - // Look up original scores for filtering and reporting - let (vector_score, keyword_score) = - original_scores.get(&id).copied().unwrap_or((0.0, None)); - - // For hybrid search, apply min_score intelligently: - // Accept if EITHER vector or keyword score meets threshold - // This allows pure keyword matches (weak vector) and pure semantic matches (weak keyword) - let passes_filter = vector_score >= min_score - || keyword_score.is_some_and(|k| k >= min_score); - - if passes_filter { - let result_root_path = if rp.is_null(idx) { - None - } else { - Some(rp.value(idx).to_string()) - }; - - // Filter by root_path if specified - if let Some(ref filter_path) = root_path { - if result_root_path.as_ref() != Some(filter_path) { - found = true; - break; - } - } - - // Use RRF combined score as the main score for ranking - // But report original vector/keyword scores for transparency - search_results.push(SearchResult { - score: combined_score, // RRF score for ranking - vector_score, // Original vector score - keyword_score, // Original BM25 score - file_path: fp.value(idx).to_string(), - root_path: result_root_path, - start_line: sl.value(idx) as usize, - end_line: el.value(idx) as usize, - language: lang.value(idx).to_string(), - content: cont.value(idx).to_string(), - project: if proj.is_null(idx) { - None - } else { - Some(proj.value(idx).to_string()) - }, - }); - } - found = true; - break; + for (chunk_id, combined_score) in combined { + let (batch, idx) = match chunk_pos.get(&chunk_id) { + Some(&(b, i)) => (&results[b], i), + None => match extra_pos.get(&chunk_id) { + Some(&(b, i)) => (&extra_batches[b], i), + None => { + tracing::warn!("Could not materialise fused result {}", chunk_id); + continue; } + }, + }; + + let file_path_array = batch + .column_by_name("file_path") + .and_then(|c| c.as_any().downcast_ref::()); + let root_path_array = batch + .column_by_name("root_path") + .and_then(|c| c.as_any().downcast_ref::()); + let start_line_array = batch + .column_by_name("start_line") + .and_then(|c| c.as_any().downcast_ref::()); + let end_line_array = batch + .column_by_name("end_line") + .and_then(|c| c.as_any().downcast_ref::()); + let language_array = batch + .column_by_name("language") + .and_then(|c| c.as_any().downcast_ref::()); + let content_array = batch + .column_by_name("content") + .and_then(|c| c.as_any().downcast_ref::()); + let project_array = batch + .column_by_name("project") + .and_then(|c| c.as_any().downcast_ref::()); + + if let ( + Some(fp), + Some(rp), + Some(sl), + Some(el), + Some(lang), + Some(cont), + Some(proj), + ) = ( + file_path_array, + root_path_array, + start_line_array, + end_line_array, + language_array, + content_array, + project_array, + ) { + // Look up original scores for filtering and reporting + let (vector_score, keyword_score) = original_scores + .get(&chunk_id) + .copied() + .unwrap_or((0.0, None)); + + // For hybrid search, apply min_score intelligently: + // Accept if EITHER vector or keyword score meets threshold + // This allows pure keyword matches (weak vector) and pure semantic matches (weak keyword) + let passes_filter = + vector_score >= min_score || keyword_score.is_some_and(|k| k >= min_score); + + if !passes_filter { + continue; + } + + let result_root_path = if rp.is_null(idx) { + None + } else { + Some(rp.value(idx).to_string()) + }; + + // Filter by root_path if specified + if let Some(ref filter_path) = root_path + && result_root_path.as_ref() != Some(filter_path) + { + continue; } - batch_offset += batch.num_rows() as u64; - } - if !found { - tracing::warn!("Could not find result for RRF ID {}", id); + // Use RRF combined score as the main score for ranking + // But report original vector/keyword scores for transparency + search_results.push(SearchResult { + score: combined_score, // RRF score for ranking + vector_score, // Original vector score + keyword_score, // Original BM25 score + file_path: fp.value(idx).to_string(), + root_path: result_root_path, + start_line: sl.value(idx) as usize, + end_line: el.value(idx) as usize, + language: lang.value(idx).to_string(), + content: cont.value(idx).to_string(), + project: if proj.is_null(idx) { + None + } else { + Some(proj.value(idx).to_string()) + }, + }); } } @@ -714,8 +809,24 @@ impl VectorDatabase for LanceVectorDB { languages: Vec, path_patterns: Vec, ) -> Result> { - // Get more results than requested to account for filtering - let search_limit = limit * 3; + // These filters are applied AFTER the search, so the candidate pool has to be big + // enough that the surviving rows can actually fill `limit`. With the old `limit * 3` + // this silently returned nothing whenever the wanted rows were a small minority of + // the corpus -- search_git_history is exactly that shape: a few hundred commits + // sharing a table with tens of thousands of code chunks, so no commit ever reached + // the top-N and the language filter then emptied the list every time. + // + // The proper fix is predicate pushdown into the LanceDB query; until then, widen + // the pool when a filter is actually present. The keyword arm helps here too now + // that fusion works: a commit whose message contains the query terms is surfaced by + // BM25 directly rather than having to win on vector distance. + let filtered = + !file_extensions.is_empty() || !languages.is_empty() || !path_patterns.is_empty(); + let search_limit = if filtered { + (limit * 20).max(200) + } else { + limit * 3 + }; // Do basic search with hybrid support let mut results = self @@ -837,6 +948,8 @@ impl VectorDatabase for LanceVectorDB { .query() .select(lancedb::query::Select::Columns(vec![ "language".to_string(), + "file_path".to_string(), + "root_path".to_string(), ])) .execute() .await @@ -847,7 +960,9 @@ impl VectorDatabase for LanceVectorDB { .await .context("Failed to collect language data")?; - let mut language_counts: HashMap = HashMap::new(); + let mut chunk_counts: HashMap = HashMap::new(); + let mut files_by_language: HashMap> = HashMap::new(); + let mut all_files: HashSet<(String, String)> = HashSet::new(); for batch in query_result { let language_array = batch @@ -857,18 +972,60 @@ impl VectorDatabase for LanceVectorDB { .downcast_ref::() .context("Invalid language type")?; + let file_path_array = batch + .column_by_name("file_path") + .context("Missing file_path column")? + .as_any() + .downcast_ref::() + .context("Invalid file_path type")?; + + let root_path_array = batch + .column_by_name("root_path") + .context("Missing root_path column")? + .as_any() + .downcast_ref::() + .context("Invalid root_path type")?; + for i in 0..batch.num_rows() { let language = language_array.value(i); - *language_counts.entry(language.to_string()).or_insert(0) += 1; + + // file_path is stored relative to root_path, so only the pair + // identifies a file: the same relative path can exist under + // two different indexed roots. + let root = if root_path_array.is_null(i) { + String::new() + } else { + root_path_array.value(i).to_string() + }; + let file_key = (root, file_path_array.value(i).to_string()); + + *chunk_counts.entry(language.to_string()).or_insert(0) += 1; + files_by_language + .entry(language.to_string()) + .or_default() + .insert(file_key.clone()); + all_files.insert(file_key); } } - let mut language_breakdown: Vec<(String, usize)> = language_counts.into_iter().collect(); - language_breakdown.sort_by(|a, b| b.1.cmp(&a.1)); + let mut language_breakdown: Vec = chunk_counts + .into_iter() + .map(|(language, chunk_count)| { + let file_count = files_by_language.get(&language).map_or(0, |f| f.len()); + LanguageBreakdown { + language, + file_count, + chunk_count, + } + }) + .collect(); + language_breakdown.sort_by(|a, b| b.chunk_count.cmp(&a.chunk_count)); Ok(DatabaseStats { + total_files: all_files.len(), total_points: count_result, total_vectors: count_result, + database_size_bytes: directory_size_bytes(Path::new(&self.db_path)), language_breakdown, }) } diff --git a/src/vector_db/lance_client/tests.rs b/src/vector_db/lance_client/tests.rs index 812879d..5be2eae 100644 --- a/src/vector_db/lance_client/tests.rs +++ b/src/vector_db/lance_client/tests.rs @@ -193,6 +193,130 @@ mod tests { assert!(results[0].keyword_score.is_some()); } + /// Regression: hybrid search across MORE THAN ONE insert batch. + /// + /// `test_search_hybrid` above stores a single batch into a fresh table, and that is + /// precisely why it never caught the bug this test exists for. The BM25 arm used to key + /// documents by `count_rows() + i` (a table row number) while the vector arm keyed by + /// position within the query's result batches. For the FIRST insert into an empty table + /// both are 0..n, so fusion appeared to work; from the second insert onward the two key + /// spaces diverged and RRF fused nothing -- every score collapsed to 1/(60+rank), + /// keyword_score was never populated, and keyword-only hits were dropped entirely. + /// + /// Both arms now key on the chunk's stable `file_path:start_line`. + #[tokio::test] + async fn test_search_hybrid_across_multiple_batches() { + let temp_dir = TempDir::new().unwrap(); + let db_path = temp_dir + .path() + .join("lancedb") + .to_string_lossy() + .to_string(); + let db = LanceVectorDB::with_path(&db_path).await.unwrap(); + db.initialize(384).await.unwrap(); + + // The keyword target goes in FIRST, so under the old scheme it held row id 0 -- but + // its vector is deliberately far from the query, so the vector arm ranks it LAST. + // That is what breaks the accidental agreement: insertion order and distance order + // must differ, or row ids and batch positions coincide and the bug hides. + db.store_embeddings( + vec![vec![0.9; 384]], + vec![create_test_metadata("target.rs", 100, 110)], + vec!["fn zzzuniquesymbol() { /* distinctive */ }".to_string()], + "/test/root", + ) + .await + .unwrap(); + + // Two rows close to the query vector, inserted second. The vector arm returns these + // at positions 0 and 1, so the old code fused BM25's id 0 (target.rs) with whatever + // sat at position 0 -- a different document entirely. + db.store_embeddings( + vec![vec![0.1; 384], vec![0.1; 384]], + vec![ + create_test_metadata("near1.rs", 1, 10), + create_test_metadata("near2.rs", 20, 30), + ], + vec!["fn alpha() {}".to_string(), "fn beta() {}".to_string()], + "/test/root", + ) + .await + .unwrap(); + + // Query text matches ONLY target.rs; query vector is closest to the near*.rs rows. + let results = db + .search(vec![0.1; 384], "zzzuniquesymbol", 10, 0.0, None, None, true) + .await + .unwrap(); + + let hit = results + .iter() + .find(|r| r.file_path == "target.rs") + .expect("keyword hit must survive fusion and be materialised as the RIGHT row"); + + assert!( + hit.keyword_score.is_some(), + "keyword_score must be populated for a BM25 match; None means the two arms \ + keyed on different id spaces again" + ); + assert_eq!(hit.start_line, 100); + assert_eq!(hit.end_line, 110); + } + + /// Git commits all share `file_path = git://` and `start_line = 0`, so a fusion + /// key of path+line alone collapses an entire history onto one id and the BM25 index + /// holds a single document for it. `file_hash` (the commit hash) is what separates + /// them. Guards that chunks differing ONLY by file_hash stay individually retrievable. + #[tokio::test] + async fn test_commit_chunks_are_not_collapsed_by_shared_path() { + let temp_dir = TempDir::new().unwrap(); + let db_path = temp_dir + .path() + .join("lancedb") + .to_string_lossy() + .to_string(); + let db = LanceVectorDB::with_path(&db_path).await.unwrap(); + db.initialize(384).await.unwrap(); + + let commit_meta = |hash: &str| { + let mut m = create_test_metadata("git://repo", 0, 0); + m.file_hash = hash.to_string(); + m.language = Some("git-commit".to_string()); + m + }; + + db.store_embeddings( + vec![vec![0.5; 384], vec![0.5; 384]], + vec![commit_meta("aaaa1111"), commit_meta("bbbb2222")], + vec![ + "Commit Message:\nfix the alpha subsystem".to_string(), + "Commit Message:\nrewrite zzzuniquecommit handling".to_string(), + ], + "/test/repo", + ) + .await + .unwrap(); + + let results = db + .search(vec![0.5; 384], "zzzuniquecommit", 10, 0.0, None, None, true) + .await + .unwrap(); + + assert_eq!( + results.len(), + 2, + "both commits must remain distinct rows, not one collapsed id" + ); + let matched = results + .iter() + .find(|r| r.content.contains("zzzuniquecommit")) + .expect("the commit matching the query text must be retrievable"); + assert!( + matched.keyword_score.is_some(), + "commit chunks must participate in keyword fusion" + ); + } + #[tokio::test] async fn test_search_with_min_score() { let temp_dir = TempDir::new().unwrap(); @@ -489,6 +613,7 @@ mod tests { let stats = db.get_statistics().await.unwrap(); assert_eq!(stats.total_points, 0); assert_eq!(stats.total_vectors, 0); + assert_eq!(stats.total_files, 0); assert_eq!(stats.language_breakdown.len(), 0); } @@ -526,13 +651,65 @@ mod tests { let stats = db.get_statistics().await.unwrap(); assert_eq!(stats.total_points, 3); assert_eq!(stats.total_vectors, 3); + assert_eq!(stats.total_files, 3, "three distinct files were stored"); + assert!( + stats.database_size_bytes > 0, + "on-disk size should be reported, not hardcoded to 0" + ); assert_eq!(stats.language_breakdown.len(), 2); - // Verify language counts (sorted by count descending) - assert_eq!(stats.language_breakdown[0].0, "Rust"); - assert_eq!(stats.language_breakdown[0].1, 2); - assert_eq!(stats.language_breakdown[1].0, "Python"); - assert_eq!(stats.language_breakdown[1].1, 1); + // Verify language counts (sorted by chunk count descending) + assert_eq!(stats.language_breakdown[0].language, "Rust"); + assert_eq!(stats.language_breakdown[0].chunk_count, 2); + assert_eq!(stats.language_breakdown[0].file_count, 2); + assert_eq!(stats.language_breakdown[1].language, "Python"); + assert_eq!(stats.language_breakdown[1].chunk_count, 1); + assert_eq!(stats.language_breakdown[1].file_count, 1); + } + + #[tokio::test] + async fn test_get_statistics_counts_files_distinctly_from_chunks() { + // Regression guard: file counts used to be filled with the row count, + // so one file split into several chunks was reported as several files. + let temp_dir = TempDir::new().unwrap(); + let db_path = temp_dir + .path() + .join("lancedb") + .to_string_lossy() + .to_string(); + let db = LanceVectorDB::with_path(&db_path).await.unwrap(); + db.initialize(384).await.unwrap(); + + // Three chunks that all come from the same file. + let embeddings = vec![vec![0.1; 384], vec![0.2; 384], vec![0.3; 384]]; + let mut meta1 = create_test_metadata("big.rs", 1, 10); + meta1.language = Some("Rust".to_string()); + let mut meta2 = create_test_metadata("big.rs", 11, 20); + meta2.language = Some("Rust".to_string()); + let mut meta3 = create_test_metadata("big.rs", 21, 30); + meta3.language = Some("Rust".to_string()); + + let contents = vec![ + "fn a() {}".to_string(), + "fn b() {}".to_string(), + "fn c() {}".to_string(), + ]; + + db.store_embeddings( + embeddings, + vec![meta1, meta2, meta3], + contents, + "/test/root", + ) + .await + .unwrap(); + + let stats = db.get_statistics().await.unwrap(); + assert_eq!(stats.total_points, 3, "three chunks were stored"); + assert_eq!(stats.total_files, 1, "but they all came from one file"); + assert_eq!(stats.language_breakdown.len(), 1); + assert_eq!(stats.language_breakdown[0].chunk_count, 3); + assert_eq!(stats.language_breakdown[0].file_count, 1); } #[tokio::test] diff --git a/src/vector_db/mod.rs b/src/vector_db/mod.rs index 52b2474..023e875 100644 --- a/src/vector_db/mod.rs +++ b/src/vector_db/mod.rs @@ -77,9 +77,27 @@ pub trait VectorDatabase: Send + Sync { async fn get_indexed_files(&self, root_path: &str) -> Result>; } +/// Per-language index statistics. +/// +/// The file count counts distinct files, the chunk count counts stored rows. +/// These are different numbers, because a file is split into many chunks. +/// Reporting the row count as both is what made the old statistics unusable. +#[derive(Debug, Clone)] +pub struct LanguageBreakdown { + pub language: String, + pub file_count: usize, + pub chunk_count: usize, +} + #[derive(Debug, Clone)] pub struct DatabaseStats { + /// Distinct files with at least one chunk indexed. + pub total_files: usize, + /// Total stored rows, one per chunk. pub total_points: usize, + /// Total stored embedding vectors, one per chunk row. pub total_vectors: usize, - pub language_breakdown: Vec<(String, usize)>, + /// On-disk size of the database, 0 when the backend cannot report it. + pub database_size_bytes: u64, + pub language_breakdown: Vec, } diff --git a/src/vector_db/qdrant_client.rs b/src/vector_db/qdrant_client.rs index e1a85c8..0c3e182 100644 --- a/src/vector_db/qdrant_client.rs +++ b/src/vector_db/qdrant_client.rs @@ -501,8 +501,14 @@ impl VectorDatabase for QdrantVectorDB { // For language breakdown, we'd need to scroll through all points // For now, return a simplified version Ok(DatabaseStats { + // This backend does not scan payloads, so distinct-file and + // on-disk-size figures are unavailable. They stay 0 rather than + // being filled with the row count, which is exactly the + // substitution that made the statistics misleading before. + total_files: 0, total_points: points_count as usize, total_vectors: points_count as usize, + database_size_bytes: 0, language_breakdown: vec![], }) } diff --git a/tests/simple_integration.rs b/tests/simple_integration.rs index c05ac5c..a2a55cc 100644 --- a/tests/simple_integration.rs +++ b/tests/simple_integration.rs @@ -38,7 +38,10 @@ async fn test_path_normalization() -> Result<()> { // Test path normalization with current directory let normalized = RagMcpServer::normalize_path(".")?; assert!(normalized.len() > 1); - assert!(normalized.starts_with('/') || normalized.chars().nth(1) == Some(':')); + // Canonicalization must yield an absolute path. Checked via Path rather + // than string shape: on Windows the result is the verbatim form + // (`\\?\C:\...`), which has neither a leading '/' nor ':' at index 1. + assert!(std::path::Path::new(&normalized).is_absolute()); Ok(()) }