From 73f663a2d35699207133ff0f038243f773b0e9b1 Mon Sep 17 00:00:00 2001 From: Steve Brown Date: Thu, 22 Oct 2026 02:49:53 -0400 Subject: [PATCH] fix(mcp): resolve repository-relative chunk reads --- docs/book/src/mcp/tools.md | 11 +- src/http/handlers/context.rs | 11 +- src/main.rs | 1 + src/mcp/read_path_contract_tests.rs | 77 ++++++++ src/mcp/remote/params.rs | 1 + src/mcp/remote/params_tests.rs | 12 ++ src/mcp/server.rs | 30 ++- src/mcp/tools.rs | 4 + src/read_path.rs | 275 ++++++++++++++++++++++++++++ 9 files changed, 411 insertions(+), 11 deletions(-) create mode 100644 src/mcp/read_path_contract_tests.rs create mode 100644 src/read_path.rs diff --git a/docs/book/src/mcp/tools.md b/docs/book/src/mcp/tools.md index 8087e237..d6355a2a 100644 --- a/docs/book/src/mcp/tools.md +++ b/docs/book/src/mcp/tools.md @@ -155,11 +155,20 @@ Read a specific section of code from a file by line range. | Parameter | Type | Required | Default | Description | |-----------|------|----------|---------|-------------| -| `file` | string | yes | — | File path relative to repo root | +| `file` | string | yes | — | Repository-relative or workspace-relative file path | +| `repo` | string | no | — | Repository name from the search result; required for ambiguous relative paths | | `start_line` | integer | yes | — | Starting line number | | `end_line` | integer | yes | — | Ending line number | | `context` | integer | no | 0 | Context lines to include before and after | +For a search result, pass its repository as `repo` and its relative path as +`file`. Existing workspace paths such as `repos/alpha/src/main.rs` still work +without `repo`. When `repo` is omitted, an existing workspace-relative file +takes precedence; otherwise exactly one match under the configured repository +prefix (default: `repos/`) is required. Multiple matches refuse and list the +repository names so the caller can choose explicitly. Missing files and unknown +repositories refuse with guidance; no index rebuild is performed. + **Response fields:** `file`, `start_line`, `end_line`, `actual_start_line`, `actual_end_line`, `content`, `language` ## hotspots diff --git a/src/http/handlers/context.rs b/src/http/handlers/context.rs index c2d293bc..b143eb3c 100644 --- a/src/http/handlers/context.rs +++ b/src/http/handlers/context.rs @@ -288,6 +288,8 @@ pub(super) async fn context( #[derive(Deserialize)] pub(super) struct ReadChunkParams { + /// Repository name from the search result. + repo: Option, /// File path (relative to repo root) file: String, /// Start line @@ -317,6 +319,8 @@ pub(super) async fn read_chunk( let (content, actual_start, actual_end) = read_file_lines( &state.repo_root, ¶ms.file, + params.repo.as_deref(), + state.config.server.repo_path_prefix.as_deref(), params.start_line, params.end_line, ctx, @@ -337,14 +341,13 @@ pub(super) async fn read_chunk( fn read_file_lines( repo_root: &std::path::Path, file: &str, + repo: Option<&str>, + repo_prefix: Option<&str>, start: u32, end: u32, context: u32, ) -> anyhow::Result<(String, u32, u32)> { - let file_path = repo_root.join(file); - if !file_path.exists() { - anyhow::bail!("File not found: {}", file); - } + let file_path = crate::read_path::resolve_read_path(repo_root, file, repo, repo_prefix)?; let content = std::fs::read_to_string(&file_path)?; let lines: Vec<&str> = content.lines().collect(); diff --git a/src/main.rs b/src/main.rs index a8e5b6d7..98d1a19d 100644 --- a/src/main.rs +++ b/src/main.rs @@ -21,6 +21,7 @@ mod operational_metrics; /// asked for) — what makes retrieval cumulative rather than per-prompt. pub mod plate; pub mod reactions; +mod read_path; mod search; mod storage; pub mod tags; diff --git a/src/mcp/read_path_contract_tests.rs b/src/mcp/read_path_contract_tests.rs new file mode 100644 index 00000000..99922fe2 --- /dev/null +++ b/src/mcp/read_path_contract_tests.rs @@ -0,0 +1,77 @@ +use super::*; + +#[tokio::test] +async fn private_index_definition_is_readable_without_workspace_prefix() { + let directory = tempfile::tempdir().unwrap(); + let root = directory.path(); + Config::default().save(&Config::config_path(root)).unwrap(); + let relative = "src/example.rs"; + let content = "fn example() { /* requested content */ }"; + let source = root.join("repos/alpha").join(relative); + std::fs::create_dir_all(source.parent().unwrap()).unwrap(); + std::fs::write(&source, content).unwrap(); + let mut store = VectorStore::open(&Config::lance_path(root)).await.unwrap(); + store + .insert( + &[crate::types::Chunk { + id: "private-example".into(), + file_path: relative.into(), + chunk_type: ChunkType::Function, + name: Some("example".into()), + start_line: 1, + end_line: 1, + content: content.into(), + language: "rust".into(), + tags: String::new(), + }], + &[vec![0.1; 384]], + &[None], + "alpha", + "fixture-hash", + "2026-01-01T00:00:00Z", + ) + .await + .unwrap(); + drop(store); + let server = BobbinMcpServer::new(root.to_path_buf()).unwrap(); + let result = server + .find_refs(Parameters(FindRefsRequest { + symbol: "example".into(), + repo: Some("alpha".into()), + r#type: None, + limit: Some(5), + })) + .await + .unwrap(); + let value: serde_json::Value = + serde_json::from_str(&result.content[0].as_text().unwrap().text).unwrap(); + let definition = &value["definition"]; + assert_eq!(definition["file_path"], relative); + for repo in [None, Some("alpha".to_string())] { + let result = server + .read_chunk(Parameters(ReadChunkRequest { + file: definition["file_path"].as_str().unwrap().into(), + repo, + start_line: 1, + end_line: 1, + context: None, + })) + .await + .unwrap(); + let value: serde_json::Value = + serde_json::from_str(&result.content[0].as_text().unwrap().text).unwrap(); + assert_eq!(value["content"], content); + } + assert!(server + .read_chunk(Parameters(ReadChunkRequest { + file: relative.into(), + repo: Some("unknown".into()), + start_line: 1, + end_line: 1, + context: None, + })) + .await + .unwrap_err() + .message + .contains("Unknown repository")); +} diff --git a/src/mcp/remote/params.rs b/src/mcp/remote/params.rs index 8dca189a..6be513ee 100644 --- a/src/mcp/remote/params.rs +++ b/src/mcp/remote/params.rs @@ -87,6 +87,7 @@ pub(super) fn context_params(req: &ContextRequest) -> Params { pub(super) fn read_chunk_params(req: &ReadChunkRequest) -> Params { Params::new() + .opt_str("repo", &req.repo) .set("file", &req.file) .set("start_line", req.start_line) .set("end_line", req.end_line) diff --git a/src/mcp/remote/params_tests.rs b/src/mcp/remote/params_tests.rs index c29b35b7..829d8162 100644 --- a/src/mcp/remote/params_tests.rs +++ b/src/mcp/remote/params_tests.rs @@ -105,11 +105,13 @@ fn context_carries_every_field() { #[test] fn read_chunk_carries_every_field() { let p = read_chunk_params(&ReadChunkRequest { + repo: Some("alpha".into()), file: "src/main.rs".into(), start_line: 10, end_line: 42, context: Some(3), }); + assert_eq!(p.get("repo"), Some("alpha")); assert_eq!(p.get("file"), Some("src/main.rs")); assert_eq!(p.get("start_line"), Some("10")); assert_eq!(p.get("end_line"), Some("42")); @@ -297,3 +299,13 @@ fn feedback_params_carry_every_field() { assert_eq!(p.get("commit_hash"), Some("deadbeef")); assert_eq!(p.get("limit"), Some("2")); } + +#[test] +fn read_chunk_without_repo_remains_compatible() { + let req: ReadChunkRequest = serde_json::from_value(serde_json::json!({ + "file": "repos/alpha/src/main.rs", "start_line": 1, "end_line": 5 + })) + .unwrap(); + assert!(req.repo.is_none()); + assert_eq!(read_chunk_params(&req).get("repo"), None); +} diff --git a/src/mcp/server.rs b/src/mcp/server.rs index b7c3decf..7218fc4e 100644 --- a/src/mcp/server.rs +++ b/src/mcp/server.rs @@ -272,15 +272,23 @@ impl BobbinMcpServer { fn read_file_lines( &self, file: &str, + repo: Option<&str>, start: u32, end: u32, context: u32, ) -> Result<(String, u32, u32)> { - let file_path = self.repo_root.join(file); - - if !file_path.exists() { - anyhow::bail!("File not found: {}", file); - } + let config_path = Config::config_path(&self.repo_root); + let config = if config_path.exists() { + Config::load(&config_path)? + } else { + Config::default() + }; + let file_path = crate::read_path::resolve_read_path( + &self.repo_root, + file, + repo, + config.server.repo_path_prefix.as_deref(), + )?; let content = std::fs::read_to_string(&file_path) .with_context(|| format!("Failed to read file: {}", file))?; @@ -1352,7 +1360,13 @@ impl BobbinMcpServer { let context = req.context.unwrap_or(0); let (content, actual_start, actual_end) = self - .read_file_lines(&req.file, req.start_line, req.end_line, context) + .read_file_lines( + &req.file, + req.repo.as_deref(), + req.start_line, + req.end_line, + context, + ) .map_err(|e| McpError::internal_error(e.to_string(), None))?; let response = ReadChunkResponse { @@ -3510,3 +3524,7 @@ pub async fn run_http_server( Ok(()) } + +#[cfg(test)] +#[path = "read_path_contract_tests.rs"] +mod read_path_contract_tests; diff --git a/src/mcp/tools.rs b/src/mcp/tools.rs index 40033f5a..cc2984f2 100644 --- a/src/mcp/tools.rs +++ b/src/mcp/tools.rs @@ -439,6 +439,10 @@ pub struct SymbolItemOutput { /// Request for reading a specific code chunk #[derive(Debug, Deserialize, schemars::JsonSchema)] pub struct ReadChunkRequest { + /// Repository name from search; omit for a unique or workspace path. + #[schemars(description = "Repository name; required for ambiguous relative paths")] + pub repo: Option, + /// File path containing the chunk #[schemars(description = "File path (relative to repo root) containing the code chunk")] pub file: String, diff --git a/src/read_path.rs b/src/read_path.rs new file mode 100644 index 00000000..23252f8e --- /dev/null +++ b/src/read_path.rs @@ -0,0 +1,275 @@ +//! Resolve read requests in single-repository and multi-repository workspaces. +use std::io; +use std::path::{Component, Path, PathBuf}; + +fn failure(message: String) -> io::Error { + io::Error::new(io::ErrorKind::NotFound, message) +} + +fn is_file(path: &Path) -> io::Result { + match std::fs::metadata(path) { + Ok(metadata) => Ok(metadata.is_file()), + Err(error) + if matches!( + error.kind(), + io::ErrorKind::NotFound | io::ErrorKind::NotADirectory + ) => + { + Ok(false) + } + Err(error) => Err(error), + } +} + +/// Resolve only existing files. Never select an arbitrary repository when a +/// repository-relative path has multiple matches. +pub(crate) fn resolve_read_path( + root: &Path, + file: &str, + repo: Option<&str>, + repo_prefix: Option<&str>, +) -> io::Result { + let repos = repo_prefix + .map(|prefix| root.join(prefix)) + .unwrap_or_else(|| root.join("repos")); + if let Some(repo) = repo { + let mut parts = Path::new(repo).components(); + if !matches!(parts.next(), Some(Component::Normal(_))) || parts.next().is_some() { + return Err(failure("Invalid repository name; use one repository name and a repository-relative file path".into())); + } + let nested_repo = repos.join(repo); + let nested_exists = match std::fs::metadata(&nested_repo) { + Ok(metadata) => metadata.is_dir(), + Err(error) + if matches!( + error.kind(), + io::ErrorKind::NotFound | io::ErrorKind::NotADirectory + ) => + { + false + } + Err(error) => return Err(error), + }; + // A workspace can itself share a repository's name. Prefer its + // configured child repository before the single-repository fallback. + let repo_root = if nested_exists { + nested_repo + } else if root.file_name().and_then(|n| n.to_str()) == Some(repo) { + root.to_path_buf() + } else { + nested_repo + }; + if !repo_root.is_dir() { + return Err(failure(format!( + "Unknown repository: {repo}; use the repository name from the search result" + ))); + } + let relative = Path::new(file); + if relative.is_absolute() + || relative.components().any(|c| { + matches!( + c, + Component::ParentDir | Component::RootDir | Component::Prefix(_) + ) + }) + { + return Err(failure( + "With repo set, file must be repository-relative without parent traversal".into(), + )); + } + let candidate = repo_root.join(relative); + if is_file(&candidate)? { + // An explicit repository may not escape through a symlink. + let canonical = candidate.canonicalize()?; + if canonical.starts_with(repo_root.canonicalize()?) { + return Ok(candidate); + } + return Err(failure( + "File resolves outside the requested repository".into(), + )); + } + return Err(failure(format!("File not found in repository {repo}: {file}; use the file path from a current search result"))); + } + // Preserve existing workspace-relative (including repos//...) and + // absolute path behavior before attempting repository-relative resolution. + let direct = root.join(file); + if is_file(&direct)? { + return Ok(direct); + } + if Path::new(file).is_absolute() + || Path::new(file) + .components() + .any(|c| matches!(c, Component::ParentDir)) + { + return Err(failure(format!( + "File not found: {file}; use a current search result and its repository" + ))); + } + let mut matches = Vec::new(); + match std::fs::read_dir(&repos) { + Ok(entries) => { + for entry in entries { + let entry = entry?; + if !entry.file_type()?.is_dir() { + continue; + } + let candidate = entry.path().join(file); + if is_file(&candidate)? { + let canonical = candidate.canonicalize()?; + if canonical.starts_with(entry.path().canonicalize()?) { + matches.push((entry.file_name().to_string_lossy().into_owned(), candidate)); + } + } + } + } + Err(e) if e.kind() == io::ErrorKind::NotFound => {} + Err(e) => return Err(e), + } + matches.sort_by(|a, b| a.0.cmp(&b.0)); + match matches.len() { + 1 => Ok(matches.remove(0).1), + 0 => Err(failure(format!("File not found: {file}; pass repo with the repository-relative path from a current search result, or use repos//"))), + _ => Err(failure(format!("Ambiguous file: {file}; pass repo explicitly (matches: {})", matches.iter().map(|(repo, _)| repo.as_str()).collect::>().join(", ")))), + } +} + +#[cfg(test)] +mod tests { + use super::*; + use std::sync::atomic::{AtomicU64, Ordering}; + static NEXT: AtomicU64 = AtomicU64::new(0); + struct Fixture(PathBuf); + impl Fixture { + fn new() -> Self { + let p = std::env::temp_dir().join(format!( + "read-path-{}-{}", + std::process::id(), + NEXT.fetch_add(1, Ordering::Relaxed) + )); + std::fs::create_dir_all(&p).unwrap(); + Self(p) + } + fn file(&self, path: &str) -> PathBuf { + let p = self.0.join(path); + std::fs::create_dir_all(p.parent().unwrap()).unwrap(); + std::fs::write(&p, "requested content\n").unwrap(); + p + } + } + impl Drop for Fixture { + fn drop(&mut self) { + std::fs::remove_dir_all(&self.0).unwrap(); + } + } + #[test] + fn search_relative_path_reads_unique_content() { + let f = Fixture::new(); + f.file("repos/alpha/src/example.rs"); + let p = resolve_read_path(&f.0, "src/example.rs", None, None).unwrap(); + assert_eq!(std::fs::read_to_string(p).unwrap(), "requested content\n"); + } + #[test] + fn ambiguity_refuses_and_explicit_repo_selects() { + let f = Fixture::new(); + f.file("repos/zeta/src/example.rs"); + let p = f.file("repos/alpha/src/example.rs"); + let error = resolve_read_path(&f.0, "src/example.rs", None, None) + .unwrap_err() + .to_string(); + assert!(error.contains("pass repo explicitly (matches: alpha, zeta)")); + assert_eq!( + resolve_read_path(&f.0, "src/example.rs", Some("alpha"), None).unwrap(), + p + ); + } + #[test] + fn explicit_repo_prefers_nested_repository_over_workspace_name() { + let f = Fixture::new(); + let workspace_file = f.file("alpha/src/example.rs"); + std::fs::write( + workspace_file, + "workspace file, not the requested repository", + ) + .unwrap(); + let wanted = f.file("alpha/repos/alpha/src/example.rs"); + let workspace = f.0.join("alpha"); + let actual = resolve_read_path(&workspace, "src/example.rs", Some("alpha"), None).unwrap(); + assert_eq!(actual, wanted); + } + + #[test] + fn explicit_single_repository_root_is_preserved() { + let f = Fixture::new(); + let wanted = f.file("solo/src/example.rs"); + let actual = + resolve_read_path(&f.0.join("solo"), "src/example.rs", Some("solo"), None).unwrap(); + assert_eq!(actual, wanted); + } + + #[test] + fn workspace_paths_remain_readable() { + let f = Fixture::new(); + let p = f.file("repos/alpha/src/example.rs"); + assert_eq!( + resolve_read_path(&f.0, "repos/alpha/src/example.rs", None, None).unwrap(), + p + ); + let p = f.file("src/local.rs"); + assert_eq!( + resolve_read_path(&f.0, "src/local.rs", None, None).unwrap(), + p + ); + } + #[test] + fn missing_and_unknown_repo_have_guidance() { + let f = Fixture::new(); + f.file("repos/alpha/src/example.rs"); + assert!( + resolve_read_path(&f.0, "src/missing.rs", Some("alpha"), None) + .unwrap_err() + .to_string() + .contains("current search result") + ); + assert!( + resolve_read_path(&f.0, "src/example.rs", Some("missing"), None) + .unwrap_err() + .to_string() + .contains("Unknown repository") + ); + assert!(resolve_read_path(&f.0, "missing", None, None) + .unwrap_err() + .to_string() + .contains("pass repo")); + } + #[test] + fn configured_prefix_is_respected() { + let f = Fixture::new(); + let p = f.file("sources/alpha/src/example.rs"); + let prefix = f.0.join("sources"); + assert_eq!( + resolve_read_path(&f.0, "src/example.rs", Some("alpha"), prefix.to_str()).unwrap(), + p + ); + assert_eq!( + resolve_read_path(&f.0, "src/example.rs", None, prefix.to_str()).unwrap(), + p + ); + } + #[test] + fn explicit_repository_rejects_traversal() { + let f = Fixture::new(); + f.file("repos/alpha/src/example.rs"); + f.file("secret"); + assert!(resolve_read_path(&f.0, "../../secret", Some("alpha"), None).is_err()); + assert!(resolve_read_path(&f.0, "src/example.rs", Some("../alpha"), None).is_err()); + } + #[cfg(unix)] + #[test] + fn explicit_repository_rejects_symlink_escape() { + let f = Fixture::new(); + let secret = f.file("secret"); + f.file("repos/alpha/src/example.rs"); + std::os::unix::fs::symlink(secret, f.0.join("repos/alpha/link")).unwrap(); + assert!(resolve_read_path(&f.0, "link", Some("alpha"), None).is_err()); + } +}