From 3f15820f7a8c0050549561eaf30ce193eb688488 Mon Sep 17 00:00:00 2001 From: juice094 <160722440+juice094@users.noreply.github.com> Date: Mon, 11 May 2026 19:53:24 +0800 Subject: [PATCH 1/2] refactor(mcp): trait-ize remaining inline crate:: calls in repo.rs - Add SearchClient trait (index_is_empty_at, search_repos_at) - Add RepoAnalyzer trait (compute_workspace_hash, analyze_repo) - Provide SearchClientImpl in search.rs and RepoAnalyzerImpl in health.rs - Remove nl_filter_repos (eliminated DefaultStorageBackend inline call) - Convert nl_filter_repos_at, apply_nl_filters, analyze_repo_for_repo to generic over traits - Update DevkitQueryReposTool and DevkitNaturalLanguageQueryTool to use trait impls - Sync mcp/tests.rs callers to new generic signatures - Result: repo.rs crate:: references reduced to 8 use-statements, 0 inline calls --- src/clients.rs | 22 +++++++++++++ src/health.rs | 18 ++++++++++ src/mcp/tests.rs | 32 ++++++++++++++++-- src/mcp/tools/repo.rs | 77 +++++++++++++++++++++---------------------- src/search.rs | 18 ++++++++++ 5 files changed, 125 insertions(+), 42 deletions(-) diff --git a/src/clients.rs b/src/clients.rs index f36ff32..738ead7 100644 --- a/src/clients.rs +++ b/src/clients.rs @@ -88,3 +88,25 @@ pub trait KnowledgeClient: Send + Sync { pub trait DigestClient: Send + Sync { fn generate_daily_digest(&self) -> Result; } + +/// Low-level repository analysis (no async, no external state). +pub trait RepoAnalyzer: Send + Sync { + fn compute_workspace_hash(&self, path: &str) -> Result; + fn analyze_repo( + &self, + path: &str, + upstream_url: Option<&str>, + default_branch: Option<&str>, + ) -> Result<(String, usize, usize)>; +} + +/// Tantivy search operations exposed to MCP tools. +pub trait SearchClient: Send + Sync { + fn index_is_empty_at(&self, path: &std::path::Path) -> Result; + fn search_repos_at( + &self, + path: &std::path::Path, + query: &str, + limit: usize, + ) -> Result>; +} diff --git a/src/health.rs b/src/health.rs index aede2d1..4e34cb8 100644 --- a/src/health.rs +++ b/src/health.rs @@ -475,6 +475,24 @@ impl HealthClient for AppContext { } } +/// Stateless implementation of [`RepoAnalyzer`] for use in spawn_blocking closures. +pub struct RepoAnalyzerImpl; + +impl crate::clients::RepoAnalyzer for RepoAnalyzerImpl { + fn compute_workspace_hash(&self, path: &str) -> anyhow::Result { + compute_workspace_hash(std::path::Path::new(path)) + } + + fn analyze_repo( + &self, + path: &str, + upstream_url: Option<&str>, + default_branch: Option<&str>, + ) -> anyhow::Result<(String, usize, usize)> { + Ok(analyze_repo(path, upstream_url, default_branch)) + } +} + #[cfg(test)] mod tests { use super::*; diff --git a/src/mcp/tests.rs b/src/mcp/tests.rs index 5bb051b..2e0b768 100644 --- a/src/mcp/tests.rs +++ b/src/mcp/tests.rs @@ -402,7 +402,19 @@ fn test_nl_filter_repos_empty_query_returns_empty() { let _guard = NL_FILTER_TEST_LOCK.lock().unwrap(); let conn = crate::registry::WorkspaceRegistry::init_in_memory().unwrap(); let repos: Vec = vec![]; - let results = crate::mcp::tools::repo::nl_filter_repos("", &repos, &conn).unwrap(); + let backend = crate::storage::TempStorageBackend::new(); + let index_path = backend.index_path().unwrap(); + let searcher = crate::search::SearchClientImpl; + let analyzer = crate::health::RepoAnalyzerImpl; + let results = crate::mcp::tools::repo::nl_filter_repos_at( + &index_path, + "", + &repos, + &conn, + &searcher, + &analyzer, + ) + .unwrap(); assert!(results.is_empty()); } @@ -414,7 +426,19 @@ fn test_nl_filter_repos_fallback_finds_by_language() { mock_repo("repo1", Some("rust"), vec!["cli"], Some(10)), mock_repo("repo2", Some("python"), vec!["web"], Some(5)), ]; - let results = crate::mcp::tools::repo::nl_filter_repos("rust cli tool", &repos, &conn).unwrap(); + let backend = crate::storage::TempStorageBackend::new(); + let index_path = backend.index_path().unwrap(); + let searcher = crate::search::SearchClientImpl; + let analyzer = crate::health::RepoAnalyzerImpl; + let results = crate::mcp::tools::repo::nl_filter_repos_at( + &index_path, + "rust cli tool", + &repos, + &conn, + &searcher, + &analyzer, + ) + .unwrap(); assert_eq!(results.len(), 1); assert_eq!(results[0].id, "repo1"); } @@ -455,11 +479,15 @@ fn test_nl_filter_repos_tantivy_finds_devbase() { remotes: vec![], }]; + let searcher = crate::search::SearchClientImpl; + let analyzer = crate::health::RepoAnalyzerImpl; let results = crate::mcp::tools::repo::nl_filter_repos_at( &index_path, "developer workspace", &repos, &conn, + &searcher, + &analyzer, ) .unwrap(); assert!(!results.is_empty(), "tantivy path should find devbase"); diff --git a/src/mcp/tools/repo.rs b/src/mcp/tools/repo.rs index a623845..25c7a82 100644 --- a/src/mcp/tools/repo.rs +++ b/src/mcp/tools/repo.rs @@ -1,12 +1,16 @@ // SPDX-License-Identifier: MIT // Copyright (c) 2026 juice094 use super::super::McpTool; -use crate::clients::{HealthClient, KnowledgeClient, ScanClient, SyncClient}; +use crate::clients::{ + HealthClient, KnowledgeClient, RepoAnalyzer, ScanClient, SearchClient, SyncClient, +}; +use crate::health::RepoAnalyzerImpl; use crate::registry::RepoEntry; use crate::repository::health::HealthRepository; use crate::repository::repo::RepoRepository; use crate::repository::workspace::WorkspaceRepository; -use crate::storage::{AppContext, StorageBackend}; +use crate::search::SearchClientImpl; +use crate::storage::AppContext; use anyhow::Context; #[derive(Clone)] @@ -309,6 +313,7 @@ Returns: JSON array of repo objects. Each includes: id, local_path, language, ta let limit = args.get("limit").and_then(|v| v.as_i64()).unwrap_or(50) as usize; let pool = ctx.pool(); + let analyzer = RepoAnalyzerImpl; tokio::task::spawn_blocking(move || { let conn = pool.get()?; let repos = RepoRepository::new(&conn).list_repos(None)?; @@ -332,12 +337,13 @@ Returns: JSON array of repo objects. Each includes: id, local_path, language, ta let (ahead, behind, dirty) = if repo.workspace_type == "git" { let (st, ah, bh) = match HealthRepository::new(&conn).get_health(&repo.id)? { Some(health) => (health.status.clone(), health.ahead, health.behind), - None => analyze_repo_for_repo(&repo), + None => analyze_repo_for_repo(&repo, &analyzer)?, }; let dirty = st == "dirty" || st == "changed"; (ah, bh, dirty) } else { - let dirty = match crate::health::compute_workspace_hash(&repo.local_path) { + let path_str = repo.local_path.to_string_lossy(); + let dirty = match analyzer.compute_workspace_hash(&path_str) { Ok(current_hash) => { match WorkspaceRepository::new(&conn).get_latest_snapshot(&repo.id)? { Some(prev) => prev.file_hash != current_hash, @@ -440,10 +446,14 @@ Returns: JSON array of matching repos with metadata, same format as devkit_query let query = query.to_string(); let pool = ctx.pool(); + let index_path = ctx.storage.index_path()?; tokio::task::spawn_blocking(move || { let conn = pool.get()?; let repos = RepoRepository::new(&conn).list_repos(None)?; - let filtered = nl_filter_repos(&query, &repos, &conn)?; + let searcher = SearchClientImpl; + let analyzer = RepoAnalyzerImpl; + let filtered = + nl_filter_repos_at(&index_path, &query, &repos, &conn, &searcher, &analyzer)?; let results: Vec = filtered .into_iter() @@ -469,12 +479,13 @@ Returns: JSON array of matching repos with metadata, same format as devkit_query .map_err(|e| anyhow::anyhow!("spawn_blocking failed: {}", e))? } } -fn apply_nl_filters( +fn apply_nl_filters( repo: &RepoEntry, q: &str, stars_cond: Option<(char, u64)>, explicit_tag: Option<&str>, conn: &rusqlite::Connection, + analyzer: &A, ) -> anyhow::Result { // Language filter: only apply if query explicitly mentions a language keyword let lang_keywords = [ @@ -527,7 +538,7 @@ fn apply_nl_filters( { let (st, ah, bh) = match HealthRepository::new(conn).get_health(&repo.id)? { Some(h) => (h.status.clone(), h.ahead, h.behind), - None => analyze_repo_for_repo(repo), + None => analyze_repo_for_repo(repo, analyzer)?, }; let dirty = st == "dirty" || st == "changed"; @@ -550,38 +561,14 @@ fn apply_nl_filters( Ok(true) } -pub(crate) fn nl_filter_repos( - query: &str, - repos: &[RepoEntry], - conn: &rusqlite::Connection, -) -> anyhow::Result> { - let backend = crate::storage::DefaultStorageBackend {}; - let index_path = match backend.index_path() { - Ok(p) => p, - Err(e) => { - tracing::warn!("Failed to resolve index path: {}", e); - // Fallback to non-Tantivy path - let q = query.to_lowercase(); - let stars_cond = parse_stars_condition(&q); - let explicit_tag = extract_tag_from_query(&q); - let mut results = Vec::new(); - for repo in repos { - if apply_nl_filters(repo, &q, stars_cond, explicit_tag.as_deref(), conn)? { - results.push(repo.clone()); - } - } - return Ok(results); - } - }; - nl_filter_repos_at(&index_path, query, repos, conn) -} - /// Filter repos using an explicit Tantivy index path, bypassing global storage backend. -pub(crate) fn nl_filter_repos_at( +pub(crate) fn nl_filter_repos_at( index_path: &std::path::Path, query: &str, repos: &[RepoEntry], conn: &rusqlite::Connection, + searcher: &S, + analyzer: &A, ) -> anyhow::Result> { let q = query.to_lowercase(); let stars_cond = parse_stars_condition(&q); @@ -597,7 +584,7 @@ pub(crate) fn nl_filter_repos_at( || q.contains("uptodate"); // Try Tantivy search first if index is not empty - let use_tantivy = match crate::search::index_is_empty_at(index_path) { + let use_tantivy = match searcher.index_is_empty_at(index_path) { Ok(empty) => !empty, Err(e) => { tracing::warn!("Failed to check search index: {}", e); @@ -607,7 +594,7 @@ pub(crate) fn nl_filter_repos_at( if use_tantivy && !query.trim().is_empty() { let limit = repos.len().max(1000); - match crate::search::search_repos_at(index_path, query, limit) { + match searcher.search_repos_at(index_path, query, limit) { Ok(search_results) => { let repo_map: std::collections::HashMap<_, _> = repos.iter().map(|r| (r.id.clone(), r)).collect(); @@ -618,7 +605,14 @@ pub(crate) fn nl_filter_repos_at( continue; } if let Some(repo) = repo_map.get(&id) - && apply_nl_filters(repo, &q, stars_cond, explicit_tag.as_deref(), conn)? + && apply_nl_filters( + repo, + &q, + stars_cond, + explicit_tag.as_deref(), + conn, + analyzer, + )? { results.push((*repo).clone()); } @@ -640,7 +634,7 @@ pub(crate) fn nl_filter_repos_at( // Fallback: iterate all repos with hardcoded regex logic let mut results = Vec::new(); for repo in repos { - if apply_nl_filters(repo, &q, stars_cond, explicit_tag.as_deref(), conn)? { + if apply_nl_filters(repo, &q, stars_cond, explicit_tag.as_deref(), conn, analyzer)? { results.push(repo.clone()); } } @@ -677,12 +671,15 @@ fn extract_tag_from_query(q: &str) -> Option { None } } -fn analyze_repo_for_repo(repo: &RepoEntry) -> (String, usize, usize) { +fn analyze_repo_for_repo( + repo: &RepoEntry, + analyzer: &A, +) -> anyhow::Result<(String, usize, usize)> { let path = repo.local_path.to_string_lossy(); let primary = repo.primary_remote(); let upstream_url = primary.and_then(|r| r.upstream_url.as_deref()); let default_branch = primary.and_then(|r| r.default_branch.as_deref()); - crate::health::analyze_repo(&path, upstream_url, default_branch) + analyzer.analyze_repo(&path, upstream_url, default_branch) } #[cfg(test)] diff --git a/src/search.rs b/src/search.rs index abc20d4..3468434 100644 --- a/src/search.rs +++ b/src/search.rs @@ -302,6 +302,24 @@ fn open_index_at(path: &std::path::Path) -> Result<(Index, Schema), TantivyError Ok((idx, schema)) } +/// Stateless implementation of [`crate::clients::SearchClient`] for use in spawn_blocking closures. +pub struct SearchClientImpl; + +impl crate::clients::SearchClient for SearchClientImpl { + fn index_is_empty_at(&self, path: &std::path::Path) -> anyhow::Result { + index_is_empty_at(path).map_err(|e| anyhow::anyhow!(e)) + } + + fn search_repos_at( + &self, + path: &std::path::Path, + query: &str, + limit: usize, + ) -> anyhow::Result> { + search_repos_at(path, query, limit).map_err(|e| anyhow::anyhow!(e)) + } +} + #[cfg(test)] pub(crate) static SEARCH_TEST_LOCK: std::sync::Mutex<()> = std::sync::Mutex::new(()); From 2d2337af79f8375d84f70fb66bceb9d92c72acb2 Mon Sep 17 00:00:00 2001 From: juice094 <160722440+juice094@users.noreply.github.com> Date: Mon, 11 May 2026 20:01:26 +0800 Subject: [PATCH 2/2] refactor(tests): replace unwrap with ? in nl_filter_repos tests for G5 compliance --- src/mcp/tests.rs | 39 +++++++++++++++++++-------------------- 1 file changed, 19 insertions(+), 20 deletions(-) diff --git a/src/mcp/tests.rs b/src/mcp/tests.rs index 2e0b768..2f30c1d 100644 --- a/src/mcp/tests.rs +++ b/src/mcp/tests.rs @@ -398,12 +398,12 @@ fn mock_repo( } #[test] -fn test_nl_filter_repos_empty_query_returns_empty() { +fn test_nl_filter_repos_empty_query_returns_empty() -> anyhow::Result<()> { let _guard = NL_FILTER_TEST_LOCK.lock().unwrap(); - let conn = crate::registry::WorkspaceRegistry::init_in_memory().unwrap(); + let conn = crate::registry::WorkspaceRegistry::init_in_memory()?; let repos: Vec = vec![]; let backend = crate::storage::TempStorageBackend::new(); - let index_path = backend.index_path().unwrap(); + let index_path = backend.index_path()?; let searcher = crate::search::SearchClientImpl; let analyzer = crate::health::RepoAnalyzerImpl; let results = crate::mcp::tools::repo::nl_filter_repos_at( @@ -413,21 +413,21 @@ fn test_nl_filter_repos_empty_query_returns_empty() { &conn, &searcher, &analyzer, - ) - .unwrap(); + )?; assert!(results.is_empty()); + Ok(()) } #[test] -fn test_nl_filter_repos_fallback_finds_by_language() { +fn test_nl_filter_repos_fallback_finds_by_language() -> anyhow::Result<()> { let _guard = NL_FILTER_TEST_LOCK.lock().unwrap(); - let conn = crate::registry::WorkspaceRegistry::init_in_memory().unwrap(); + let conn = crate::registry::WorkspaceRegistry::init_in_memory()?; let repos = vec![ mock_repo("repo1", Some("rust"), vec!["cli"], Some(10)), mock_repo("repo2", Some("python"), vec!["web"], Some(5)), ]; let backend = crate::storage::TempStorageBackend::new(); - let index_path = backend.index_path().unwrap(); + let index_path = backend.index_path()?; let searcher = crate::search::SearchClientImpl; let analyzer = crate::health::RepoAnalyzerImpl; let results = crate::mcp::tools::repo::nl_filter_repos_at( @@ -437,23 +437,23 @@ fn test_nl_filter_repos_fallback_finds_by_language() { &conn, &searcher, &analyzer, - ) - .unwrap(); + )?; assert_eq!(results.len(), 1); assert_eq!(results[0].id, "repo1"); + Ok(()) } #[test] -fn test_nl_filter_repos_tantivy_finds_devbase() { +fn test_nl_filter_repos_tantivy_finds_devbase() -> anyhow::Result<()> { let backend = std::sync::Arc::new(crate::storage::TempStorageBackend::new()); - let index_path = backend.index_path().unwrap(); + let index_path = backend.index_path()?; // Ensure DB schema exists - let conn = crate::registry::WorkspaceRegistry::init_db_with(&*backend).unwrap(); + let conn = crate::registry::WorkspaceRegistry::init_db_with(&*backend)?; // Populate Tantivy index with devbase doc - let (index, _reader) = crate::search::init_index_at(&index_path).unwrap(); - let mut writer = crate::search::get_writer(&index).unwrap(); + let (index, _reader) = crate::search::init_index_at(&index_path)?; + let mut writer = crate::search::get_writer(&index)?; let schema = index.schema(); crate::search::add_repo_doc( &mut writer, @@ -462,9 +462,8 @@ fn test_nl_filter_repos_tantivy_finds_devbase() { "devbase developer workspace manager", "rust, cli, workspace, developer", &["rust".to_string(), "cli".to_string()], - ) - .unwrap(); - crate::search::commit_writer(&mut writer).unwrap(); + )?; + crate::search::commit_writer(&mut writer)?; let repos = vec![crate::registry::RepoEntry { id: "devbase".to_string(), @@ -488,10 +487,10 @@ fn test_nl_filter_repos_tantivy_finds_devbase() { &conn, &searcher, &analyzer, - ) - .unwrap(); + )?; assert!(!results.is_empty(), "tantivy path should find devbase"); assert_eq!(results[0].id, "devbase"); + Ok(()) } #[test]