From 718976fb39a0068f0f104363a14d915037e8641b Mon Sep 17 00:00:00 2001 From: ajianaz Date: Wed, 7 Oct 2026 15:18:13 +0700 Subject: [PATCH] refactor(index): single seam for resolving project root and opening the index Evolve IndexBridge into the one module that resolves the project root (via resolve_project_root), opens the index with shared PRAGMAs, and ensures the project id. Tolerant mode for review, strict mode for CLI/MCP. Index scanners and brain context take the bridge, so a review run from a subdirectory resolves the same project_id as indexing from the root. Closes #566 Co-Authored-By: Claude Sonnet 5.5 Signed-off-by: ajianaz --- src/commands/query.rs | 4 +- src/commands/routes.rs | 4 +- src/commands/scan.rs | 5 +- src/commands/serve.rs | 8 +- src/commands/watch.rs | 4 +- src/engine/db_writer.rs | 10 +- src/engine/index_bridge.rs | 265 ++++++++++++++++++++++++++++++------ src/engine/index_scanner.rs | 177 +++++++++++------------- src/engine/review.rs | 43 +++--- src/index/mod.rs | 49 +++---- src/main.rs | 103 ++++---------- src/mcp/tools.rs | 16 ++- 12 files changed, 397 insertions(+), 291 deletions(-) diff --git a/src/commands/query.rs b/src/commands/query.rs index 41deb63..502fdc3 100644 --- a/src/commands/query.rs +++ b/src/commands/query.rs @@ -211,8 +211,8 @@ pub fn execute_query_cli( json_flag: bool, limit: usize, ) -> anyhow::Result { - let conn = crate::index::open_global_index()?; - let (project_id, _root) = crate::index::resolve_project_id(&conn)?; + let (conn, project_id, _root) = + crate::engine::index_bridge::IndexBridge::open_or_create_cwd()?.into_strict_parts()?; let pattern = parse_query(pattern_str)?; let results = execute_query(&pattern, project_id, &conn, limit)?; diff --git a/src/commands/routes.rs b/src/commands/routes.rs index 9e72a6c..a84a276 100644 --- a/src/commands/routes.rs +++ b/src/commands/routes.rs @@ -98,8 +98,8 @@ pub fn execute_routes_cli( prefix: Option<&str>, json_flag: bool, ) -> anyhow::Result { - let conn = crate::index::open_global_index()?; - let (project_id, _root) = crate::index::resolve_project_id(&conn)?; + let (conn, project_id, _root) = + crate::engine::index_bridge::IndexBridge::open_or_create_cwd()?.into_strict_parts()?; let routes = list_routes(&conn, project_id, method, prefix)?; diff --git a/src/commands/scan.rs b/src/commands/scan.rs index c884ace..1dcc73c 100644 --- a/src/commands/scan.rs +++ b/src/commands/scan.rs @@ -121,8 +121,9 @@ pub async fn execute_scan( let mut index_skip = config.ignore.files.clone(); index_skip.extend(config.rules_config.index_skip_files.iter().cloned()); index_skip.dedup(); + let index_bridge = crate::engine::index_bridge::IndexBridge::open(&root_abs); let index_findings = crate::engine::index_scanner::scan_project_index( - &root_abs, + &index_bridge, &files, config.rules_config.max_findings, &index_skip, @@ -154,7 +155,7 @@ pub async fn execute_scan( crate::engine::review::build_scan_brain_context( &files, config.context_chain.impact_depth, - &root_abs, + &index_bridge, ) } else { None diff --git a/src/commands/serve.rs b/src/commands/serve.rs index e34c925..db2ebf3 100644 --- a/src/commands/serve.rs +++ b/src/commands/serve.rs @@ -3,12 +3,8 @@ /// Execute the serve command: auto-reindex the current project, then start the MCP server. pub fn execute_serve() -> anyhow::Result<()> { // 1. Auto-reindex current project (incremental — skips unchanged files) - let project_root = std::env::current_dir()?; - let project_root = - crate::index::resolve_project_root(&project_root).unwrap_or(project_root.clone()); - - let conn = crate::index::open_global_index()?; - let _project_id = crate::index::ensure_project(&conn, &project_root)?; + let (conn, _project_id, project_root) = + crate::engine::index_bridge::IndexBridge::open_or_create_cwd()?.into_strict_parts()?; let skip_patterns = crate::index::prepare_index_config(None); let stats = crate::index::index_project_with_skip( diff --git a/src/commands/watch.rs b/src/commands/watch.rs index 904d40b..a7d8880 100644 --- a/src/commands/watch.rs +++ b/src/commands/watch.rs @@ -33,7 +33,9 @@ pub fn run_watch( filter: Option<&str>, verbose: bool, ) -> Result<()> { - let conn = crate::index::open_global_index()?; + let (conn, _project_id, _root) = + crate::engine::index_bridge::IndexBridge::open_or_create(project_root)? + .into_strict_parts()?; // Load skip patterns + brain embedding backend from config let config = crate::config::loader::load_config(config_path, None, None, None, None, false).ok(); diff --git a/src/engine/db_writer.rs b/src/engine/db_writer.rs index 782e0fe..12e4888 100644 --- a/src/engine/db_writer.rs +++ b/src/engine/db_writer.rs @@ -181,14 +181,10 @@ pub fn resolve_stale_findings(project_root: &str, current_fingerprints: &[String } /// Open the global `cora.db` and ensure migrations are up to date. +/// +/// Delegates to the shared index opener so PRAGMAs live in one place. fn open_db() -> anyhow::Result { - crate::data_dir::ensure_data_dir()?; - let db_path = crate::data_dir::graph_db_path(); - let conn = Connection::open(&db_path)?; - conn.execute_batch("PRAGMA foreign_keys=ON;")?; - conn.execute_batch("PRAGMA journal_mode=WAL;")?; - schema::run_migrations(&conn)?; - Ok(conn) + crate::index::open_global_index() } /// Open cora.db in read-only mode (no migrations, no WAL). diff --git a/src/engine/index_bridge.rs b/src/engine/index_bridge.rs index b7e004f..7e6f23d 100644 --- a/src/engine/index_bridge.rs +++ b/src/engine/index_bridge.rs @@ -1,66 +1,156 @@ -//! IndexBridge — lightweight connection between the engine and the symbol index. +//! IndexBridge — the single seam for "which project am I in, and where is its index?". //! -//! Provides a single struct that wraps an optional `rusqlite::Connection` to the -//! global `cora.db` and the resolved `project_id`. When the index database does -//! not exist or cannot be opened, the bridge reports `is_available() == false` -//! and all query methods return empty results — **zero caller impact**. +//! Every entry point (CLI arms, MCP tools, review-time scanners, the context +//! resolver) goes through this module. It owns three things: //! -//! The bridge is constructed once at the start of a review/scan run and passed -//! through the context chain pipeline, replacing the ad-hoc -//! `crate::index::open_global_index()` calls scattered throughout resolver.rs. +//! 1. **Root resolution** — a start path is always normalised with +//! [`crate::index::resolve_project_root`], so a run from a subdirectory or a +//! workspace member lands on the same `project_id` as indexing from the root. +//! 2. **Opening the database** — PRAGMAs and migrations live in +//! [`crate::index::open_index_at`]; nobody else opens `cora.db` read-write. +//! 3. **Project id** — resolved once per bridge via `ensure_project`. +//! +//! Two modes: +//! - *tolerant* ([`IndexBridge::open`]): review-time. A missing database or any +//! failure yields an *unavailable* bridge; all queries return empty results. +//! - *strict* ([`IndexBridge::open_strict`]): CLI/MCP. A missing database is a +//! [`NoIndexError`]; the returned bridge is always available. +//! +//! [`IndexBridge::open_or_create`] is for writers (`index`, `serve`, `watch`). -use std::path::Path; +use std::path::{Path, PathBuf}; use rusqlite::Connection; use tracing::debug; -// ── Public API ──────────────────────────────────────────────────────── +/// Returned by [`IndexBridge::open_strict`] when no index database exists yet. +#[derive(Debug)] +pub struct NoIndexError; + +impl std::fmt::Display for NoIndexError { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!(f, "No index found. Run `cora index` first.") + } +} + +impl std::error::Error for NoIndexError {} /// Bridge to the cora symbol index. /// -/// Holds an optional SQLite connection + project_id pair. If the index is -/// unavailable (no `cora.db`, migration failure, etc.) the bridge is *unavailable* -/// but still safe to query — all lookups return `None` / empty `Vec`. -#[allow(dead_code)] +/// Holds an optional SQLite connection + project_id pair plus the resolved +/// project root. If the index is unavailable (tolerant mode: no `cora.db`, +/// migration failure, etc.) the bridge is *unavailable* but still safe to +/// query — all lookups return `None` / empty `Vec`. pub struct IndexBridge { conn: Option, project_id: Option, + root: PathBuf, } impl IndexBridge { - /// Open the global index and resolve the project id for `project_root`. + /// Normalise `start` to the project root (`.cora.yaml` / workspace / marker), + /// falling back to `start` itself when no marker is found. + pub fn resolve_root(start: &Path) -> PathBuf { + crate::index::resolve_project_root(start).unwrap_or_else(|| start.to_path_buf()) + } + + /// [`Self::resolve_root`] applied to the current working directory. + pub fn current_root() -> anyhow::Result { + Ok(Self::resolve_root(&std::env::current_dir()?)) + } + + /// Tolerant open of the global index for the project containing `start`. /// - /// Returns an `IndexBridge` regardless of whether the index exists. - /// Call `is_available()` to check. - pub fn open(project_root: &Path) -> Self { - let conn = match crate::index::open_global_index() { - Ok(c) => c, - Err(e) => { - debug!(error = %e, "index bridge: global index unavailable"); - return Self::unavailable(); - } - }; + /// Never creates the database and never fails: use [`Self::is_available`]. + pub fn open(start: &Path) -> Self { + Self::open_tolerant_at(&crate::data_dir::graph_db_path(), start) + } + + /// Tolerant open of the global index for the current working directory. + pub fn open_cwd() -> Self { + match std::env::current_dir() { + Ok(cwd) => Self::open(&cwd), + Err(_) => Self::unavailable(), + } + } - let project_id = match crate::index::ensure_project(&conn, project_root) { - Ok(id) => Some(id), + pub(crate) fn open_tolerant_at(db_path: &Path, start: &Path) -> Self { + let root = Self::resolve_root(start); + if !db_path.exists() { + debug!("index bridge: no index database"); + return Self::unavailable_for(root); + } + match Self::open_at(db_path, &root) { + Ok(b) => b, Err(e) => { - debug!(error = %e, "index bridge: failed to resolve project_id"); - return Self::unavailable(); + debug!(error = %e, "index bridge: index unavailable"); + Self::unavailable_for(root) } - }; + } + } + + /// Strict open of the global index: errors with [`NoIndexError`] if the + /// database does not exist. The returned bridge is always available. + pub fn open_strict(start: &Path) -> anyhow::Result { + Self::open_strict_at(&crate::data_dir::graph_db_path(), start) + } + + /// Strict open for the current working directory. + pub fn open_strict_cwd() -> anyhow::Result { + Self::open_strict(&std::env::current_dir()?) + } + + pub(crate) fn open_strict_at(db_path: &Path, start: &Path) -> anyhow::Result { + if !db_path.exists() { + return Err(NoIndexError.into()); + } + Self::open_at(db_path, &Self::resolve_root(start)) + } + + /// Open the global index, creating it if needed (writers: index/serve/watch). + pub fn open_or_create(start: &Path) -> anyhow::Result { + let conn = crate::index::open_global_index()?; + Self::from_connection(conn, start) + } + + /// [`Self::open_or_create`] for the current working directory. + pub fn open_or_create_cwd() -> anyhow::Result { + Self::open_or_create(&std::env::current_dir()?) + } + + /// Wrap an existing connection (tests, in-memory indexes). Resolves the + /// root from `start` and ensures the project row. + pub fn from_connection(conn: Connection, start: &Path) -> anyhow::Result { + let root = Self::resolve_root(start); + let project_id = crate::index::ensure_project(&conn, &root)?; + Ok(Self { + conn: Some(conn), + project_id: Some(project_id), + root, + }) + } + fn open_at(db_path: &Path, root: &Path) -> anyhow::Result { + let conn = crate::index::open_index_at(db_path)?; + let project_id = crate::index::ensure_project(&conn, root)?; debug!(project_id, "index bridge: opened successfully"); - Self { + Ok(Self { conn: Some(conn), - project_id, - } + project_id: Some(project_id), + root: root.to_path_buf(), + }) } /// Create an explicitly unavailable bridge (no index / cannot open). pub fn unavailable() -> Self { + Self::unavailable_for(PathBuf::new()) + } + + fn unavailable_for(root: PathBuf) -> Self { Self { conn: None, project_id: None, + root, } } @@ -77,6 +167,27 @@ impl IndexBridge { self.project_id } + /// The resolved project root (set even when the index is unavailable). + #[inline] + pub fn root(&self) -> &Path { + &self.root + } + + /// Connection and project id together, or `None` when unavailable. + #[inline] + pub fn parts(&self) -> Option<(&Connection, i64)> { + Some((self.conn.as_ref()?, self.project_id?)) + } + + /// Consume an available bridge into `(connection, project_id, root)`. + pub fn into_strict_parts(self) -> anyhow::Result<(Connection, i64, PathBuf)> { + let root = self.root; + match (self.conn, self.project_id) { + (Some(c), Some(id)) => Ok((c, id, root)), + _ => Err(NoIndexError.into()), + } + } + // ── Query helpers ──────────────────────────────────────────────────── /// Search the symbols table via FTS5 for the given query text. @@ -189,16 +300,86 @@ mod tests { assert!(bridge.connection().is_none()); } + fn init_db(dir: &Path) -> PathBuf { + let db = dir.join("cora.db"); + crate::index::open_index_at(&db).unwrap(); + db + } + + #[test] + fn tolerant_open_without_index_is_unavailable_and_creates_nothing() { + let dir = tempfile::tempdir().unwrap(); + let db = dir.path().join("cora.db"); + let bridge = IndexBridge::open_tolerant_at(&db, dir.path()); + assert!(!bridge.is_available()); + assert!(bridge.parts().is_none()); + assert!(!db.exists(), "tolerant mode must not create the database"); + } + + #[test] + fn strict_open_without_index_errors_clearly() { + let dir = tempfile::tempdir().unwrap(); + let db = dir.path().join("cora.db"); + let err = IndexBridge::open_strict_at(&db, dir.path()) + .err() + .expect("strict mode must fail without an index"); + assert!(err.downcast_ref::().is_some()); + assert!(err.to_string().contains("cora index")); + } + + #[test] + fn subdirectory_resolves_same_project_as_root() { + let dir = tempfile::tempdir().unwrap(); + let repo = dir.path().join("repo"); + let member = repo.join("crates/member/src"); + std::fs::create_dir_all(&member).unwrap(); + std::fs::create_dir_all(repo.join(".git")).unwrap(); + std::fs::write( + repo.join("Cargo.toml"), + "[workspace]\nmembers = [\"crates/member\"]\n", + ) + .unwrap(); + std::fs::write( + repo.join("crates/member/Cargo.toml"), + "[package]\nname = \"member\"\n", + ) + .unwrap(); + let db = init_db(dir.path()); + + let from_root = IndexBridge::open_strict_at(&db, &repo).unwrap(); + let from_sub = IndexBridge::open_tolerant_at(&db, &member); + assert!(from_sub.is_available()); + assert_eq!(from_root.project_id(), from_sub.project_id()); + assert_eq!(from_root.root(), from_sub.root()); + } + + #[test] + fn from_connection_resolves_root_from_subdirectory() { + let dir = tempfile::tempdir().unwrap(); + let sub = dir.path().join("a/b"); + std::fs::create_dir_all(&sub).unwrap(); + std::fs::write(dir.path().join(".cora.yaml"), "").unwrap(); + let conn = Connection::open_in_memory().unwrap(); + crate::index::schema::run_migrations(&conn).unwrap(); + let pid = crate::index::ensure_project(&conn, dir.path()).unwrap(); + let bridge = IndexBridge::from_connection(conn, &sub).unwrap(); + assert_eq!(bridge.project_id(), Some(pid)); + } + #[test] - fn open_nonexistent_project_returns_unavailable() { - // Opening with a nonexistent project root should still succeed - // (it creates the project row), but we can verify it opens. + fn pragmas_are_applied_by_the_shared_opener() { let dir = tempfile::tempdir().unwrap(); - // The bridge opens the global index — if it doesn't exist, - // the data_dir crate will create it. - let bridge = IndexBridge::open(dir.path()); - // Either available (index was created) or unavailable — both are valid. - // The key invariant: no panic, no crash. - let _ = bridge.is_available(); + let db = init_db(dir.path()); + let conn = crate::index::open_index_at(&db).unwrap(); + let fk: i64 = conn + .query_row("PRAGMA foreign_keys", [], |r| r.get(0)) + .unwrap(); + let sync: i64 = conn + .query_row("PRAGMA synchronous", [], |r| r.get(0)) + .unwrap(); + let mode: String = conn + .query_row("PRAGMA journal_mode", [], |r| r.get(0)) + .unwrap(); + assert_eq!((fk, sync, mode.as_str()), (1, 1, "wal")); } } diff --git a/src/engine/index_scanner.rs b/src/engine/index_scanner.rs index 8632cfb..1059c41 100644 --- a/src/engine/index_scanner.rs +++ b/src/engine/index_scanner.rs @@ -9,6 +9,7 @@ use tracing::debug; use crate::engine::Severity; use crate::engine::diff_parser::{DiffLineType, FileChunk}; +use crate::engine::index_bridge::IndexBridge; use crate::engine::rules::types::RuleFinding; use crate::index::graph; @@ -110,25 +111,14 @@ pub fn should_skip_file(file_path: &str, skip_patterns: &[String]) -> bool { /// /// Returns `Vec` with severity `Minor` for each unused import. pub fn scan_unused_imports( + bridge: &IndexBridge, chunks: &[FileChunk], - project_root: &std::path::Path, max_findings: usize, skip_patterns: &[String], ) -> Vec { - let conn = match crate::index::open_global_index() { - Ok(c) => c, - Err(_) => { - debug!("no global index available — skipping unused import scan"); - return Vec::new(); - } - }; - - let project_id = match crate::index::ensure_project(&conn, project_root) { - Ok(id) => id, - Err(_) => { - debug!("failed to get project_id — skipping unused import scan"); - return Vec::new(); - } + let Some((conn, project_id)) = bridge.parts() else { + debug!("no project index available — skipping unused import scan"); + return Vec::new(); }; let mut findings = Vec::new(); @@ -162,7 +152,7 @@ pub fn scan_unused_imports( } if seen_files.insert(file.to_string()) { - match graph::find_unused_imports(&conn, file, project_id) { + match graph::find_unused_imports(conn, file, project_id) { Ok(unused) => { for u in &unused { findings.push(RuleFinding { @@ -204,25 +194,14 @@ pub fn scan_unused_imports( /// /// Returns `Vec` with severity `Info` for each dead symbol. pub fn scan_dead_code_in_review( + bridge: &IndexBridge, chunks: &[FileChunk], - project_root: &std::path::Path, max_findings: usize, skip_patterns: &[String], ) -> Vec { - let conn = match crate::index::open_global_index() { - Ok(c) => c, - Err(_) => { - debug!("no global index available — skipping dead code scan"); - return Vec::new(); - } - }; - - let project_id = match crate::index::ensure_project(&conn, project_root) { - Ok(id) => id, - Err(_) => { - debug!("failed to get project_id — skipping dead code scan"); - return Vec::new(); - } + let Some((conn, project_id)) = bridge.parts() else { + debug!("no project index available — skipping dead code scan"); + return Vec::new(); }; let mut findings = Vec::new(); @@ -244,7 +223,7 @@ pub fn scan_dead_code_in_review( } if seen_files.insert(file.to_string()) { - match graph::find_dead_code_in_file(&conn, file, project_id, false) { + match graph::find_dead_code_in_file(conn, file, project_id, false) { Ok(dead) => { for d in &dead { findings.push(RuleFinding { @@ -287,37 +266,14 @@ pub fn scan_dead_code_in_review( /// /// Returns `Vec` with severity `Major` for each breaking change. pub fn scan_breaking_changes( + bridge: &IndexBridge, chunks: &[FileChunk], - project_root: &std::path::Path, - max_findings: usize, - skip_patterns: &[String], -) -> Vec { - let conn = match crate::index::open_global_index() { - Ok(c) => c, - Err(_) => { - debug!("no global index available — skipping breaking change scan"); - return Vec::new(); - } - }; - - scan_breaking_changes_with(&conn, chunks, project_root, max_findings, skip_patterns) -} - -/// [`scan_breaking_changes`] against an explicit connection — testable with an -/// in-memory index. -pub(crate) fn scan_breaking_changes_with( - conn: &rusqlite::Connection, - chunks: &[FileChunk], - project_root: &std::path::Path, max_findings: usize, skip_patterns: &[String], ) -> Vec { - let project_id = match crate::index::ensure_project(conn, project_root) { - Ok(id) => id, - Err(_) => { - debug!("failed to get project_id — skipping breaking change scan"); - return Vec::new(); - } + let Some((conn, project_id)) = bridge.parts() else { + debug!("no project index available — skipping breaking change scan"); + return Vec::new(); }; // Symbol names (re)defined by this very diff — the post-image of the change. @@ -470,7 +426,7 @@ fn collect_added_definitions(chunks: &[FileChunk]) -> HashSet { /// Designed for `cora scan` which operates on file paths, not diffs. /// Returns findings for any file in the project that has an index DB. pub fn scan_project_index( - root: &std::path::Path, + bridge: &IndexBridge, files: &[crate::engine::scanner::FileEntry], max_findings: usize, skip_patterns: &[String], @@ -479,20 +435,9 @@ pub fn scan_project_index( let mut findings = Vec::new(); - let conn = match crate::index::open_global_index() { - Ok(c) => c, - Err(_) => { - debug!("no global index available — skipping project index scan"); - return findings; - } - }; - - let project_id = match crate::index::ensure_project(&conn, root) { - Ok(id) => id, - Err(_) => { - debug!("failed to get project_id — skipping project index scan"); - return findings; - } + let Some((conn, project_id)) = bridge.parts() else { + debug!("no project index available — skipping project index scan"); + return findings; }; // Scan for unused imports across all files in the scan set @@ -502,7 +447,7 @@ pub fn scan_project_index( continue; } if seen_files.insert(entry.path.clone()) { - match graph::find_unused_imports(&conn, &entry.path, project_id) { + match graph::find_unused_imports(conn, &entry.path, project_id) { Ok(unused) => { for u in &unused { findings.push(ReviewIssue { @@ -536,7 +481,7 @@ pub fn scan_project_index( // Scan for dead code in the indexed project let opts = graph::DeadCodeOptions::default(); - match graph::find_dead_code(&conn, project_id, &opts) { + match graph::find_dead_code(conn, project_id, &opts) { Ok(dead) => { for func in dead.into_iter().take(max_findings - findings.len()) { findings.push(ReviewIssue { @@ -605,7 +550,7 @@ mod tests { fn scan_unused_imports_no_index_graceful() { // No index available — should return empty, not panic let chunks = vec![make_chunk("src/main.rs", "+use std::collections::HashMap;")]; - let findings = scan_unused_imports(&chunks, std::path::Path::new("/nonexistent"), 10, &[]); + let findings = scan_unused_imports(&IndexBridge::unavailable(), &chunks, 10, &[]); assert!( findings.is_empty(), "should gracefully return empty without index" @@ -615,8 +560,7 @@ mod tests { #[test] fn scan_dead_code_no_index_graceful() { let chunks = vec![make_chunk("src/main.rs", "+fn foo() {}")]; - let findings = - scan_dead_code_in_review(&chunks, std::path::Path::new("/nonexistent"), 10, &[]); + let findings = scan_dead_code_in_review(&IndexBridge::unavailable(), &chunks, 10, &[]); assert!( findings.is_empty(), "should gracefully return empty without index" @@ -626,8 +570,7 @@ mod tests { #[test] fn scan_breaking_changes_no_index_graceful() { let chunks = vec![make_chunk("src/main.rs", "-pub fn important_api() {}")]; - let findings = - scan_breaking_changes(&chunks, std::path::Path::new("/nonexistent"), 10, &[]); + let findings = scan_breaking_changes(&IndexBridge::unavailable(), &chunks, 10, &[]); assert!( findings.is_empty(), "should gracefully return empty without index" @@ -641,20 +584,27 @@ mod tests { "-pub fn important_api() {}\n+pub fn new_api() {}", )]; // No index, so no callers detected — but the pattern should still compile - let findings = - scan_breaking_changes(&chunks, std::path::Path::new("/nonexistent"), 10, &[]); + let findings = scan_breaking_changes(&IndexBridge::unavailable(), &chunks, 10, &[]); assert!(findings.is_empty(), "no index means no caller data"); } - // --- scan_breaking_changes_with: stale-index false-positive guard (#533) --- + // --- scan_breaking_changes: stale-index false-positive guard (#533) --- /// In-memory index with caller edges for a symbol, mirroring a populated /// global index that may be out of date relative to the diff. - fn index_with_callers(callee: &str, callers: &[(&str, &str, i64)]) -> rusqlite::Connection { + fn index_with_callers(callee: &str, callers: &[(&str, &str, i64)]) -> IndexBridge { + index_at_root(std::path::Path::new("/fixture/proj"), callee, callers) + } + + /// Same, with the project registered under `root` and the bridge opened from `start`. + fn index_at_root( + root: &std::path::Path, + callee: &str, + callers: &[(&str, &str, i64)], + ) -> IndexBridge { let conn = rusqlite::Connection::open_in_memory().expect("in-memory db"); crate::index::schema::run_migrations(&conn).expect("migrations"); - let project_id = - crate::index::schema::get_or_create_project(&conn, "/fixture/proj").expect("project"); + let project_id = crate::index::ensure_project(&conn, root).expect("project"); for (caller, file, line) in callers { conn.execute( "INSERT INTO call_graph (caller, callee, file, line, project_id) \ @@ -663,11 +613,7 @@ mod tests { ) .expect("insert call_graph"); } - conn - } - - fn project_root() -> &'static std::path::Path { - std::path::Path::new("/fixture/proj") + IndexBridge::from_connection(conn, root).expect("bridge") } /// Build a chunk the way the real diff parser does: content WITHOUT the @@ -707,7 +653,7 @@ mod tests { fn signature_drift_against_stale_index_is_not_a_removal() { // The reported FP (#533): only the signature line changed, so the old // definition shows up as a `-` line while the same symbol is re-added. - let conn = index_with_callers("build_review_prompt", &[("handler_a", "src/api.rs", 42)]); + let bridge = index_with_callers("build_review_prompt", &[("handler_a", "src/api.rs", 42)]); let chunks = vec![chunk_lines( "src/engine/llm.rs", &[ @@ -718,7 +664,7 @@ mod tests { ), ], )]; - let findings = scan_breaking_changes_with(&conn, &chunks, project_root(), 10, &[]); + let findings = scan_breaking_changes(&bridge, &chunks, 10, &[]); assert!( findings.is_empty(), "signature-only drift must not be reported as removal, got: {:?}", @@ -728,12 +674,12 @@ mod tests { #[test] fn genuine_removal_with_callers_still_fires() { - let conn = index_with_callers("important_api", &[("caller_x", "src/app.rs", 7)]); + let bridge = index_with_callers("important_api", &[("caller_x", "src/app.rs", 7)]); let chunks = vec![chunk_lines( "src/lib.rs", &[("-", "pub fn important_api() {}")], )]; - let findings = scan_breaking_changes_with(&conn, &chunks, project_root(), 10, &[]); + let findings = scan_breaking_changes(&bridge, &chunks, 10, &[]); assert_eq!(findings.len(), 1, "a true removal must still be reported"); assert_eq!(findings[0].rule_id, "index-breaking-change"); assert_eq!(findings[0].severity, Severity::Major); @@ -741,24 +687,24 @@ mod tests { #[test] fn rename_reports_only_the_old_name() { - let conn = index_with_callers("old_name", &[("caller_y", "src/app.rs", 3)]); + let bridge = index_with_callers("old_name", &[("caller_y", "src/app.rs", 3)]); let chunks = vec![chunk_lines( "src/lib.rs", &[("-", "pub fn old_name() {}"), ("+", "pub fn new_name() {}")], )]; - let findings = scan_breaking_changes_with(&conn, &chunks, project_root(), 10, &[]); + let findings = scan_breaking_changes(&bridge, &chunks, 10, &[]); assert_eq!(findings.len(), 1, "rename is still breaking for old_name"); assert!(findings[0].title.contains("old_name")); } #[test] fn cross_file_move_is_not_a_removal() { - let conn = index_with_callers("moved_fn", &[("caller_z", "src/main.rs", 11)]); + let bridge = index_with_callers("moved_fn", &[("caller_z", "src/main.rs", 11)]); let chunks = vec![ chunk_lines("src/old_location.rs", &[("-", "pub fn moved_fn() {}")]), chunk_lines("src/new_location.rs", &[("+", "pub fn moved_fn() {}")]), ]; - let findings = scan_breaking_changes_with(&conn, &chunks, project_root(), 10, &[]); + let findings = scan_breaking_changes(&bridge, &chunks, 10, &[]); assert!( findings.is_empty(), "a definition moved between files still exists post-change, got: {:?}", @@ -791,6 +737,37 @@ mod tests { // --- should_skip_file tests --- + #[test] + fn scanner_run_from_subdirectory_sees_the_indexed_project() { + // Index registered under the workspace root; the review runs from a + // member directory. Both must resolve the same project_id, otherwise + // the scanner sees an empty project and reports nothing. + let dir = tempfile::tempdir().unwrap(); + let root = dir.path().join("repo"); + let member = root.join("crates/member"); + std::fs::create_dir_all(&member).unwrap(); + std::fs::create_dir_all(root.join(".git")).unwrap(); + std::fs::write( + root.join("Cargo.toml"), + "[workspace]\nmembers = [\"crates/member\"]\n", + ) + .unwrap(); + std::fs::write(member.join("Cargo.toml"), "[package]\nname = \"member\"\n").unwrap(); + + let from_root = index_at_root(&root, "important_api", &[("caller_x", "src/app.rs", 7)]); + let (conn, root_pid, _) = from_root.into_strict_parts().unwrap(); + // Re-open the same connection as if started from the subdirectory. + let from_sub = IndexBridge::from_connection(conn, &member).unwrap(); + assert_eq!(from_sub.project_id(), Some(root_pid)); + + let chunks = vec![chunk_lines( + "src/lib.rs", + &[("-", "pub fn important_api() {}")], + )]; + let findings = scan_breaking_changes(&from_sub, &chunks, 10, &[]); + assert_eq!(findings.len(), 1); + } + #[test] fn skip_empty_patterns() { assert!(!should_skip_file("src/main.ts", &[])); diff --git a/src/engine/review.rs b/src/engine/review.rs index d6665a2..98d975c 100644 --- a/src/engine/review.rs +++ b/src/engine/review.rs @@ -172,22 +172,30 @@ async fn review_diff_inner( ); // Run index-powered scans (requires symbol graph — graceful no-op without index) + // One bridge, rooted via resolve_project_root, shared by every index-backed + // step so a run from a subdirectory agrees with `cora index` (#566). + let index_bridge = crate::engine::index_bridge::IndexBridge::open_cwd(); + let project_root = if index_bridge.root().as_os_str().is_empty() { + std::env::current_dir().unwrap_or_default() + } else { + index_bridge.root().to_path_buf() + }; let skip_patterns = &config.rules_config.index_skip_files; let index_unused_findings = crate::engine::index_scanner::scan_unused_imports( + &index_bridge, &diff_chunks, - std::env::current_dir().unwrap_or_default().as_path(), config.rules_config.max_findings, skip_patterns, ); let index_dead_findings = crate::engine::index_scanner::scan_dead_code_in_review( + &index_bridge, &diff_chunks, - std::env::current_dir().unwrap_or_default().as_path(), config.rules_config.max_findings, skip_patterns, ); let index_breaking_findings = crate::engine::index_scanner::scan_breaking_changes( + &index_bridge, &diff_chunks, - std::env::current_dir().unwrap_or_default().as_path(), config.rules_config.max_findings, skip_patterns, ); @@ -239,7 +247,7 @@ async fn review_diff_inner( let context_chain = crate::engine::context::build_context_chain( &diff_chunks, &config.context_chain, - std::env::current_dir().unwrap_or_default().as_path(), + &project_root, &config.ignore.files, ); @@ -294,7 +302,7 @@ async fn review_diff_inner( match build_brain_context( &diff_chunks, config.context_chain.impact_depth, - std::env::current_dir().unwrap_or_default().as_path(), + &index_bridge, ) { Some(brain_ctx) if !brain_ctx.is_empty() => { debug!( @@ -778,11 +786,9 @@ fn is_valid_file_path(issue_file: &str, valid_files: &[String]) -> bool { pub(crate) fn build_brain_context( diff_chunks: &[crate::engine::diff_parser::FileChunk], impact_depth: u32, - project_root: &std::path::Path, + bridge: &crate::engine::index_bridge::IndexBridge, ) -> Option { - // Try to open the global symbol index - let conn = crate::index::open_global_index().ok()?; - let project_id = crate::index::ensure_project(&conn, project_root).ok()?; + let (conn, project_id) = bridge.parts()?; // Extract defined symbols from the diff let defs = crate::engine::context::extraction::extract_definitions_from_diff(diff_chunks); @@ -799,7 +805,7 @@ pub(crate) fn build_brain_context( continue; } if let Ok(nodes) = - crate::index::graph::impact_analysis(&conn, project_id, &def.name, impact_depth) + crate::index::graph::impact_analysis(conn, project_id, &def.name, impact_depth) { if !nodes.is_empty() { impact_lines.push(format!( @@ -838,7 +844,7 @@ pub(crate) fn build_brain_context( } // Walk impact nodes, collect files containing "test" or "spec" if let Ok(nodes) = crate::index::graph::impact_analysis( - &conn, project_id, &def.name, 1, // depth 1 is enough for test detection + conn, project_id, &def.name, 1, // depth 1 is enough for test detection ) { for node in &nodes { let lower = node.file.to_lowercase(); @@ -849,7 +855,7 @@ pub(crate) fn build_brain_context( } // Also search FTS5 for test symbols matching this function name if let Ok(results) = - crate::index::brain::brain_search(&conn, project_id, &format!("test {}", def.name), 3) + crate::index::brain::brain_search(conn, project_id, &format!("test {}", def.name), 3) { for r in results { let lower = r.file.to_lowercase(); @@ -880,7 +886,7 @@ pub(crate) fn build_brain_context( if def.name.len() < 2 { continue; } - if let Ok(results) = crate::index::brain::brain_search(&conn, project_id, &def.name, 3) { + if let Ok(results) = crate::index::brain::brain_search(conn, project_id, &def.name, 3) { for r in results { // Skip results from the same file as the definition if r.file == def.file { @@ -922,10 +928,9 @@ pub(crate) fn build_brain_context( pub(crate) fn build_scan_brain_context( files: &[crate::engine::scanner::FileEntry], impact_depth: u32, - project_root: &std::path::Path, + bridge: &crate::engine::index_bridge::IndexBridge, ) -> Option { - let conn = crate::index::open_global_index().ok()?; - let project_id = crate::index::ensure_project(&conn, project_root).ok()?; + let (conn, project_id) = bridge.parts()?; // Extract function/type names from each file using simple heuristics. // For scan we don't have tree-sitter AST — we use the index's FTS5 @@ -939,7 +944,7 @@ pub(crate) fn build_scan_brain_context( let mut all_symbols: Vec = Vec::new(); for file_path in file_paths.iter().take(10) { let query = format!("file:\"{file_path}\""); - if let Ok(results) = crate::index::brain::brain_search(&conn, project_id, &query, 5) { + if let Ok(results) = crate::index::brain::brain_search(conn, project_id, &query, 5) { all_symbols.extend(results.into_iter().filter(|r| r.name.len() >= 2)); } } @@ -955,7 +960,7 @@ pub(crate) fn build_scan_brain_context( let mut impact_lines: Vec = Vec::new(); for r in &unique_symbols { if let Ok(nodes) = - crate::index::graph::impact_analysis(&conn, project_id, &r.name, impact_depth) + crate::index::graph::impact_analysis(conn, project_id, &r.name, impact_depth) { if nodes.len() > 2 { impact_lines.push(format!( @@ -979,7 +984,7 @@ pub(crate) fn build_scan_brain_context( // Reuse the same symbols — no additional brain_search calls needed. let mut test_files: std::collections::HashSet = std::collections::HashSet::new(); for r in &unique_symbols { - if let Ok(nodes) = crate::index::graph::impact_analysis(&conn, project_id, &r.name, 1) { + if let Ok(nodes) = crate::index::graph::impact_analysis(conn, project_id, &r.name, 1) { for node in &nodes { let lower = node.file.to_lowercase(); if lower.contains("test") || lower.contains("spec") || lower.contains("_test") { diff --git a/src/index/mod.rs b/src/index/mod.rs index 9579664..6779382 100644 --- a/src/index/mod.rs +++ b/src/index/mod.rs @@ -32,9 +32,25 @@ pub use symbols::{SearchResult, SymbolKind, SymbolQuery}; /// Project isolation is handled via the `project_id` foreign key. pub fn open_global_index() -> anyhow::Result { crate::data_dir::ensure_data_dir()?; - let db_path = crate::data_dir::graph_db_path(); + open_index_at(&crate::data_dir::graph_db_path()) +} + +/// Open (creating if absent) the index database at `db_path`, apply the +/// standard PRAGMAs and run migrations. +/// +/// This is the single place that opens an index connection; the production path +/// goes through [`open_global_index`], tests may point it at a temp file. +pub fn open_index_at(db_path: &Path) -> anyhow::Result { + let conn = Connection::open(db_path)?; + apply_pragmas(&conn)?; + schema::run_migrations(&conn)?; - let conn = Connection::open(&db_path)?; + debug!("Opened index at {}", db_path.display()); + Ok(conn) +} + +/// The one PRAGMA set every read-write index connection uses. +pub fn apply_pragmas(conn: &Connection) -> anyhow::Result<()> { conn.execute_batch( "PRAGMA journal_mode=WAL;\ PRAGMA foreign_keys=ON;\ @@ -43,11 +59,9 @@ pub fn open_global_index() -> anyhow::Result { PRAGMA mmap_size=268435456;\ PRAGMA temp_store=MEMORY;", )?; - schema::run_migrations(&conn)?; - - debug!("Opened global index at {}", db_path.display()); - Ok(conn) + Ok(()) } + /// Resolve the `project_id` for a given root path, creating the project row if needed. pub fn ensure_project(conn: &Connection, root: &Path) -> anyhow::Result { let root_str = root.to_string_lossy().to_string(); @@ -120,17 +134,6 @@ pub fn resolve_project_root(start: &Path) -> Option { fallback } -/// Resolve `project_id` from the current directory, using project root detection. -/// -/// Walks up from CWD to find a project root (`.cora.yaml`, `Cargo.toml`, etc.). -/// Falls back to CWD if no marker is found. -pub fn resolve_project_id(conn: &Connection) -> anyhow::Result<(i64, std::path::PathBuf)> { - let cwd = std::env::current_dir()?; - let root = resolve_project_root(&cwd).unwrap_or_else(|| cwd.clone()); - let project_id = ensure_project(conn, &root)?; - Ok((project_id, root)) -} - #[cfg(test)] /// Index a single file: extract symbols and store in the database. /// Test-only — production uses `index_project_with_id` with batch fingerprinting. @@ -930,18 +933,6 @@ pub struct AuthService { } } - #[test] - fn test_resolve_project_id_uses_project_root() { - let conn = mem_conn(); - // resolve_project_id uses CWD — which is the cora-code crate root. - let (pid, root) = resolve_project_id(&conn).unwrap(); - assert!(pid > 0); - assert!( - root.join("Cargo.toml").exists(), - "resolved root should contain Cargo.toml" - ); - } - /// Regression (#522): running `cora index` from inside a workspace member /// crate must resolve to the WORKSPACE root (the member's plain /// `Cargo.toml` is not the project root), so CLI and MCP agree on one diff --git a/src/main.rs b/src/main.rs index a9f956e..21cf0ff 100644 --- a/src/main.rs +++ b/src/main.rs @@ -639,6 +639,22 @@ enum ProfileAction { } /// Format bytes as human-readable string. +/// Strict open of the index for read-only CLI arms: prints a friendly hint and +/// exits when no index exists yet. +fn open_index_strict_or_exit() -> Result<(rusqlite::Connection, i64, std::path::PathBuf)> { + match engine::index_bridge::IndexBridge::open_strict_cwd() { + Ok(bridge) => bridge.into_strict_parts(), + Err(e) + if e.downcast_ref::() + .is_some() => + { + eprintln!("{}", "No index found. Run `cora index` first.".yellow()); + std::process::exit(1); + } + Err(e) => Err(e), + } +} + fn format_bytes(bytes: u64) -> String { if bytes < 1024 { format!("{bytes} B") @@ -697,10 +713,8 @@ async fn main() -> Result<()> { watch, verbose, } => { - let project_root = std::env::current_dir()?; - let project_root = index::resolve_project_root(&project_root).unwrap_or(project_root); - let conn = index::open_global_index()?; - let project_id = index::ensure_project(&conn, &project_root)?; + let (conn, project_id, project_root) = + engine::index_bridge::IndexBridge::open_or_create_cwd()?.into_strict_parts()?; if rebuild { // Delete all data for this project via CASCADE @@ -848,17 +862,7 @@ async fn main() -> Result<()> { limit, json, } => { - let project_root = std::env::current_dir()?; - let project_root = index::resolve_project_root(&project_root).unwrap_or(project_root); - let db_path = crate::data_dir::graph_db_path(); - - if !db_path.exists() { - eprintln!("{}", "No index found. Run `cora index` first.".yellow()); - std::process::exit(1); - } - - let conn = index::open_global_index()?; - let project_id = index::ensure_project(&conn, &project_root)?; + let (conn, project_id, _project_root) = open_index_strict_or_exit()?; let sym_kind = kind.as_deref().map(index::SymbolKind::from_str); @@ -921,15 +925,7 @@ async fn main() -> Result<()> { limit, json, } => { - let project_root = std::env::current_dir()?; - let project_root = index::resolve_project_root(&project_root).unwrap_or(project_root); - let db_path = crate::data_dir::graph_db_path(); - if !db_path.exists() { - eprintln!("{}", "No index found. Run `cora index` first.".yellow()); - std::process::exit(1); - } - let conn = index::open_global_index()?; - let project_id = index::ensure_project(&conn, &project_root)?; + let (conn, project_id, _project_root) = open_index_strict_or_exit()?; let callers = index::graph::find_callers(&conn, project_id, &symbol, limit)?; // Cross-project fallback: if no callers in current project, @@ -1002,15 +998,7 @@ async fn main() -> Result<()> { depth, json, } => { - let project_root = std::env::current_dir()?; - let project_root = index::resolve_project_root(&project_root).unwrap_or(project_root); - let db_path = crate::data_dir::graph_db_path(); - if !db_path.exists() { - eprintln!("{}", "No index found. Run 'cora index' first.".yellow()); - std::process::exit(1); - } - let conn = index::open_global_index()?; - let project_id = index::ensure_project(&conn, &project_root)?; + let (conn, project_id, _project_root) = open_index_strict_or_exit()?; let impact = index::graph::impact_analysis(&conn, project_id, &symbol, depth)?; if json { @@ -1051,15 +1039,7 @@ async fn main() -> Result<()> { depth, json, } => { - let project_root = std::env::current_dir()?; - let project_root = index::resolve_project_root(&project_root).unwrap_or(project_root); - let db_path = crate::data_dir::graph_db_path(); - if !db_path.exists() { - eprintln!("{}", "No index found. Run `cora index` first.".yellow()); - std::process::exit(1); - } - let conn = index::open_global_index()?; - let project_id = index::ensure_project(&conn, &project_root)?; + let (conn, project_id, _project_root) = open_index_strict_or_exit()?; let dir = match direction.as_str() { "incoming" => index::graph::TraceDirection::Incoming, @@ -1110,15 +1090,7 @@ async fn main() -> Result<()> { } Command::Arch { json } => { - let project_root = std::env::current_dir()?; - let project_root = index::resolve_project_root(&project_root).unwrap_or(project_root); - let db_path = crate::data_dir::graph_db_path(); - if !db_path.exists() { - eprintln!("{}", "No index found. Run `cora index` first.".yellow()); - std::process::exit(1); - } - let conn = index::open_global_index()?; - let project_id = index::ensure_project(&conn, &project_root)?; + let (conn, project_id, _project_root) = open_index_strict_or_exit()?; let overview = index::graph::arch_overview(&conn, project_id)?; @@ -1160,15 +1132,7 @@ async fn main() -> Result<()> { std::process::exit(1); } - let project_root = std::env::current_dir()?; - let project_root = index::resolve_project_root(&project_root).unwrap_or(project_root); - let db_path = crate::data_dir::graph_db_path(); - if !db_path.exists() { - eprintln!("{}", "No index found. Run `cora index` first.".yellow()); - std::process::exit(1); - } - let conn = index::open_global_index()?; - let project_id = index::ensure_project(&conn, &project_root)?; + let (conn, project_id, _project_root) = open_index_strict_or_exit()?; // Resolve embedding backend from config for query embedding let brain_cfg = crate::config::loader::load_config( @@ -1221,15 +1185,7 @@ async fn main() -> Result<()> { filter, json, } => { - let project_root = std::env::current_dir()?; - let project_root = index::resolve_project_root(&project_root).unwrap_or(project_root); - let db_path = crate::data_dir::graph_db_path(); - if !db_path.exists() { - eprintln!("{}", "No index found. Run `cora index` first.".yellow()); - std::process::exit(1); - } - let conn = index::open_global_index()?; - let project_id = index::ensure_project(&conn, &project_root)?; + let (conn, project_id, _project_root) = open_index_strict_or_exit()?; // Gather changed files let mut changed: Vec = files; @@ -1584,8 +1540,7 @@ async fn main() -> Result<()> { git_only, filter, } => { - let project_root = std::env::current_dir()?; - let project_root = index::resolve_project_root(&project_root).unwrap_or(project_root); + let project_root = engine::index_bridge::IndexBridge::current_root()?; let config_path = cli.global.config.as_deref(); commands::watch::run_watch( &project_root, @@ -1627,10 +1582,8 @@ async fn main() -> Result<()> { } => { // Resolve the project root the same way `cora index` does, so // dead-code queries the workspace the index actually built (#522). - let cwd = std::env::current_dir().with_context(|| "failed to get cwd")?; - let project_root = index::resolve_project_root(&cwd).unwrap_or(cwd.clone()); - let conn = index::open_global_index()?; - let project_id = index::ensure_project(&conn, &project_root)?; + let (conn, project_id, _project_root) = + engine::index_bridge::IndexBridge::open_or_create_cwd()?.into_strict_parts()?; // Load config for entry_point_patterns let config = crate::config::loader::load_config( diff --git a/src/mcp/tools.rs b/src/mcp/tools.rs index c5e9f81..ba36890 100644 --- a/src/mcp/tools.rs +++ b/src/mcp/tools.rs @@ -438,13 +438,17 @@ fn handle_list_profiles() -> ToolResult { /// Uses project root detection (walks up from CWD looking for markers). /// Returns helpful error if not found. fn open_index_db() -> anyhow::Result<(rusqlite::Connection, i64)> { - let db_path = crate::data_dir::graph_db_path(); - if !db_path.exists() { - anyhow::bail!("No symbol index found. Run 'cora index' first to build the index."); + use crate::engine::index_bridge::{IndexBridge, NoIndexError}; + match IndexBridge::open_strict_cwd() { + Ok(bridge) => { + let (conn, project_id, _root) = bridge.into_strict_parts()?; + Ok((conn, project_id)) + } + Err(e) if e.downcast_ref::().is_some() => { + anyhow::bail!("No symbol index found. Run 'cora index' first to build the index.") + } + Err(e) => Err(e), } - let conn = crate::index::open_global_index()?; - let (project_id, _root) = crate::index::resolve_project_id(&conn)?; - Ok((conn, project_id)) } fn handle_search_symbols(params: &serde_json::Value) -> ToolResult {