From 4338e28ce0341c487a5e32ce167f05bd942f203a Mon Sep 17 00:00:00 2001 From: endergebnis Date: Fri, 4 Sep 2026 18:49:06 +0200 Subject: [PATCH] Fix six open issues: stale imports, MCP validation, import-graph gaps, dup importers, I/O errors, scope docs - pipeline: re-resolve imports for every file on update (not just changed), so add/remove/rename of a target fixes unchanged importers; propagate hash and read errors instead of treating unreadable files as empty - discovery: report unreadable walk entries to stderr instead of dropping them - imports: strip `pub` visibility so `pub use` re-exports resolve; share extension/module-entry probe lists (adds cc,cxx,hpp,hh,hxx,mts,cts,jsx,mjs,cjs) - store: DISTINCT + ORDER in importers_of to dedup repeated edges - mcp: validate required tool args (explicit isError), emit JSON-RPC -32700 parse errors and -32600 invalid-request instead of silently skipping - README: document the 13-tool surface vs toktoken's 27 and known gaps - tests: regression coverage for update re-resolution, pub-use re-exports, importer dedup, and MCP error reporting Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- README.md | 23 ++++++++ src/discovery/mod.rs | 10 +++- src/index/imports.rs | 33 +++++++++-- src/index/pipeline.rs | 33 +++++++---- src/index/store.rs | 4 +- src/mcp/mod.rs | 85 +++++++++++++++++---------- tests/cli.rs | 132 +++++++++++++++++++++++++++++++++++++++++- 7 files changed, 271 insertions(+), 49 deletions(-) diff --git a/README.md b/README.md index 356b331..83f4b58 100644 --- a/README.md +++ b/README.md @@ -138,6 +138,29 @@ Notes: - Hardware cycle counters were not available (`perf` tools missing on this kernel), so cycle figures are estimates: CPU time × 3.2 GHz nominal. +## Scope and known gaps + +`rustytokens` covers the same *core* retrieval surface for Java/Python/Rust/ +C/C++/HTML as `toktoken`, but intentionally implements a smaller tool set: 13 +tools vs `toktoken`'s 27. The following `toktoken` tools have **no equivalent +here yet** (deliberately, not an oversight): + +- `index_file` (reindex a single file) +- `find_callers`, `find_references` (cross-reference search) +- `inspect_bundle`, `inspect_dependencies`, `inspect_hierarchy` (graph views) +- `search_similar`, `search_cooccurrence` (similarity search) +- GitHub indexing and centrality ranking + +Add them only if the agent workflow actually needs them; they are not required +for the "roughly cover Java/Python/Rust/C/C++/HTML" goal. + +Known limits that are by design, not bugs: + +- `.css` files are discovered and import-tracked, but `ctags` has no CSS + parser — `search symbols` returns nothing for `.css`. +- `find dead` over-counts for C/C++ header-only entry points (documented + conservatism); treat results as candidates, not verdicts. + ## Development ```bash diff --git a/src/discovery/mod.rs b/src/discovery/mod.rs index 2f797c3..40aebe0 100644 --- a/src/discovery/mod.rs +++ b/src/discovery/mod.rs @@ -32,10 +32,15 @@ pub fn discover(project: &Path) -> Result> { }); let mut out = Vec::new(); + let mut skipped = Vec::new(); for entry in builder.build() { let entry = match entry { Ok(e) => e, - Err(_) => continue, + Err(e) => { + // Report unreadable entries instead of dropping them silently. + skipped.push(e.to_string()); + continue; + } }; let path = entry.path(); if entry.file_type().is_some_and(|t| t.is_dir()) { @@ -54,6 +59,9 @@ pub fn discover(project: &Path) -> Result> { language, }); } + for s in &skipped { + eprintln!("rustytokens: skipped unreadable entry: {s}"); + } out.sort_by(|a, b| a.rel.cmp(&b.rel)); Ok(out) } diff --git a/src/index/imports.rs b/src/index/imports.rs index 521f30a..20f2670 100644 --- a/src/index/imports.rs +++ b/src/index/imports.rs @@ -75,10 +75,22 @@ fn extract_python(content: &str) -> Vec { out } +/// Strip a leading visibility modifier (`pub`, `pub(crate)`, `pub(super)`, …) +/// so `pub use`/`pub mod` parse the same as their private counterparts. +fn strip_visibility(l: &str) -> &str { + if let Some(r) = l.strip_prefix("pub ") { + r + } else if let Some(r) = l.strip_prefix("pub(") { + r.find(") ").map(|i| &r[i + 2..]).unwrap_or(l) + } else { + l + } +} + fn extract_rust(content: &str) -> Vec { let mut out = Vec::new(); for line in content.lines() { - let l = line.trim(); + let l = strip_visibility(line.trim()); if let Some(rest) = l.strip_prefix("use ") { let rest = rest .trim_end_matches(';') @@ -293,6 +305,17 @@ fn clean(target: &str) -> Option { Some(t.to_string()) } +/// Code file extensions probed when resolving extensionless import targets. +/// Mirrors `config::language_for` for code languages (html/css assets are +/// always imported with an explicit extension, so they need no probing). +const RESOLVE_EXTS: &[&str] = &[ + "rs", "py", "ts", "tsx", "mts", "cts", "js", "jsx", "mjs", "cjs", "c", "h", "cc", "cpp", "cxx", + "hpp", "hh", "hxx", "java", +]; + +/// Module-directory entry files tried for `dir/mod` style resolution. +const MODULE_ENTRIES: &[&str] = &["mod.rs", "__init__.py", "index.ts", "index.js", "index.tsx"]; + impl Resolver { pub fn build(files: Vec<(String, i64)>, crate_name: Option<&str>) -> Self { let mut by_path = HashMap::new(); @@ -380,12 +403,12 @@ impl Resolver { if let Some(id) = self.by_path.get(&format!("{base}{t}")) { return Some(*id); } - for ext in ["rs", "py", "ts", "tsx", "js", "cpp", "c", "h", "java"] { + for ext in RESOLVE_EXTS { if let Some(id) = self.by_path.get(&format!("{base}{t}.{ext}")) { return Some(*id); } } - for entry in ["mod.rs", "__init__.py", "index.ts", "index.js", "index.tsx"] { + for entry in MODULE_ENTRIES { if let Some(id) = self.by_path.get(&format!("{base}{t}/{entry}")) { return Some(*id); } @@ -394,12 +417,12 @@ impl Resolver { } fn try_module(&self, base: &str) -> Option { - for ext in ["rs", "py", "ts", "tsx", "js", "cpp", "c", "h", "java"] { + for ext in RESOLVE_EXTS { if let Some(id) = self.by_path.get(&format!("{base}.{ext}")) { return Some(*id); } } - for entry in ["mod.rs", "__init__.py", "index.ts", "index.js", "index.tsx"] { + for entry in MODULE_ENTRIES { if let Some(id) = self.by_path.get(&format!("{base}/{entry}")) { return Some(*id); } diff --git a/src/index/pipeline.rs b/src/index/pipeline.rs index 494414c..12ec7c5 100644 --- a/src/index/pipeline.rs +++ b/src/index/pipeline.rs @@ -56,15 +56,17 @@ pub fn run(project: &Path, db_path: &Path, recreate: bool) -> Result .map(|f| (f.path.clone(), (f.id, f.content_hash.clone()))) .collect(); - // Hash all files in parallel. + // Hash all files in parallel. Propagate read errors instead of silently + // treating an unreadable file as an empty (all-zero) hash. let hashed: Vec<(String, String, i64, &'static str)> = files .par_iter() .map(|f| { - let h = hashing::hash_file(&f.path).unwrap_or_else(|_| "0000000000000000".to_string()); + let h = hashing::hash_file(&f.path) + .with_context(|| format!("cannot hash {}", f.path.display()))?; let m = hashing::mtime_secs(&f.path); - (f.rel.clone(), h, m, f.language) + Ok((f.rel.clone(), h, m, f.language)) }) - .collect(); + .collect::>>()?; let by_rel: HashMap<&str, &SourceFile> = files.iter().map(|f| (f.rel.as_str(), f)).collect(); let abs_to_rel: HashMap = files @@ -112,11 +114,27 @@ pub fn run(project: &Path, db_path: &Path, recreate: bool) -> Result read_crate_name(project).as_deref(), ); + // Symbols + content FTS: only files whose content changed. Skipping ctags + // for unchanged files is the whole point of the hash optimization. for f in &changed { let fid = file_ids[&f.rel]; - let content = std::fs::read_to_string(&f.path).unwrap_or_default(); + let content = std::fs::read_to_string(&f.path) + .with_context(|| format!("cannot read {}", f.path.display()))?; let tags = tags_by_rel.get(&f.rel).cloned().unwrap_or_default(); store::replace_symbols(&conn, fid, &tags_to_symbols(&tags, &content))?; + conn.execute("DELETE FROM content_fts WHERE rowid = ?1", [fid])?; + conn.execute( + "INSERT INTO content_fts(rowid, path, content) VALUES (?1, ?2, ?3)", + params![fid, f.rel, content], + )?; + } + + // Imports: re-resolve for EVERY file. Adding/removing/renaming a file can + // change whether an unchanged importer resolves, so resolution must cover + // the whole file set, not just the changed set. (Cheap: read + regex, no + // ctags subprocess.) + for f in &files { + let fid = file_ids[&f.rel]; store::clear_imports_for_file(&conn, fid)?; let dir = f .rel @@ -131,11 +149,6 @@ pub fn run(project: &Path, db_path: &Path, recreate: bool) -> Result store::insert_import(&conn, fid, to, &imp.import_type)?; } } - conn.execute("DELETE FROM content_fts WHERE rowid = ?1", [fid])?; - conn.execute( - "INSERT INTO content_fts(rowid, path, content) VALUES (?1, ?2, ?3)", - params![fid, f.rel, content], - )?; } conn.execute_batch("COMMIT;")?; diff --git a/src/index/store.rs b/src/index/store.rs index 2e4ecbb..c10b7b0 100644 --- a/src/index/store.rs +++ b/src/index/store.rs @@ -200,7 +200,9 @@ pub fn symbols_for_file(conn: &Connection, file_id: i64) -> Result Result> { - let mut stmt = conn.prepare("SELECT from_file_id FROM imports WHERE to_file_id = ?1")?; + let mut stmt = conn.prepare( + "SELECT DISTINCT from_file_id FROM imports WHERE to_file_id = ?1 ORDER BY from_file_id", + )?; let rows = stmt.query_map([file_id], |r| r.get(0))?; Ok(rows.collect::, _>>()?) } diff --git a/src/mcp/mod.rs b/src/mcp/mod.rs index a6e1e04..792f379 100644 --- a/src/mcp/mod.rs +++ b/src/mcp/mod.rs @@ -22,7 +22,22 @@ pub fn serve(project: &Path) -> Result<()> { } let msg: Value = match serde_json::from_str(&line) { Ok(m) => m, - Err(_) => continue, + Err(_) => { + // Unparseable input is a JSON-RPC parse error, not something to + // drop silently. + let mut out = stdout.lock(); + serde_json::to_writer( + &mut out, + &json!({ + "jsonrpc": "2.0", + "id": null, + "error": { "code": -32700, "message": "parse error" } + }), + )?; + out.write_all(b"\n")?; + out.flush()?; + continue; + } }; if let Some(resp) = handle(&msg, project) { let mut out = stdout.lock(); @@ -35,12 +50,20 @@ pub fn serve(project: &Path) -> Result<()> { } fn handle(msg: &Value, project: &Path) -> Option { - let method = msg.get("method")?.as_str()?; let id = msg.get("id").cloned(); // Notifications have no id -> no response. let id = id.as_ref()?; + // Requests must carry a string method name. + let Some(method) = msg.get("method").and_then(|m| m.as_str()) else { + return Some(json!({ + "jsonrpc": "2.0", + "id": id, + "error": { "code": -32600, "message": "invalid request" } + })); + }; + match method { "initialize" => Some(json!({ "jsonrpc": "2.0", @@ -93,38 +116,38 @@ fn call_tool(name: &str, args: &Value, project: &Path) -> Value { fn dispatch(name: &str, args: &Value, project: &Path) -> Result { use crate::ops; - let s = |k: &str| args.get(k).and_then(|v| v.as_str()).unwrap_or(""); + + fn req_str<'a>(args: &'a Value, key: &str, tool: &str) -> Result<&'a str> { + args.get(key) + .and_then(|v| v.as_str()) + .filter(|s| !s.is_empty()) + .ok_or_else(|| anyhow::anyhow!("{tool}: missing or invalid argument '{key}'")) + } + fn req_i64(args: &Value, key: &str, tool: &str) -> Result { + args.get(key) + .and_then(|v| v.as_i64()) + .ok_or_else(|| anyhow::anyhow!("{tool}: missing or invalid argument '{key}'")) + } + let opt_str = |k: &str| args.get(k).and_then(|v| v.as_str()); + let opt_depth = |k: &str| args.get(k).and_then(|v| v.as_u64()).map(|d| d as usize); + match name { - "index_create" => ops::index_create(Path::new(s("path"))), - "index_update" => ops::index_update(Path::new(s("path"))), - "search_symbols" => ops::search_symbols(project, s("query")), - "search_text" => ops::search_text(project, s("query")), - "inspect_symbol" => ops::inspect_symbol( - project, - args.get("id").and_then(|v| v.as_i64()).unwrap_or(0), - ), - "inspect_outline" => ops::inspect_outline(project, s("file")), - "inspect_file" => ops::inspect_file( - project, - s("file"), - args.get("lines").and_then(|v| v.as_str()), - ), - "inspect_tree" => ops::inspect_tree( - project, - args.get("depth") - .and_then(|v| v.as_u64()) - .map(|d| d as usize), - ), - "find_importers" => ops::find_importers(project, s("file")), + "index_create" => ops::index_create(Path::new(req_str(args, "path", name)?)), + "index_update" => ops::index_update(Path::new(req_str(args, "path", name)?)), + "search_symbols" => ops::search_symbols(project, req_str(args, "query", name)?), + "search_text" => ops::search_text(project, req_str(args, "query", name)?), + "inspect_symbol" => ops::inspect_symbol(project, req_i64(args, "id", name)?), + "inspect_outline" => ops::inspect_outline(project, req_str(args, "file", name)?), + "inspect_file" => { + ops::inspect_file(project, req_str(args, "file", name)?, opt_str("lines")) + } + "inspect_tree" => ops::inspect_tree(project, opt_depth("depth")), + "find_importers" => ops::find_importers(project, req_str(args, "file", name)?), "find_dead" => ops::find_dead(project), "inspect_cycles" => ops::inspect_cycles(project), - "inspect_blast_radius" => ops::inspect_blast_radius( - project, - s("file"), - args.get("depth") - .and_then(|v| v.as_u64()) - .map(|d| d as usize), - ), + "inspect_blast_radius" => { + ops::inspect_blast_radius(project, req_str(args, "file", name)?, opt_depth("depth")) + } "stats" => ops::stats(project), _ => anyhow::bail!("unknown tool: {name}"), } diff --git a/tests/cli.rs b/tests/cli.rs index dec88b0..b8c2e8b 100644 --- a/tests/cli.rs +++ b/tests/cli.rs @@ -144,7 +144,10 @@ fn inspect_blast_radius_finds_affected_files() { .collect(); // lib.rs imports util.rs directly; main.rs reaches it via the crate name. assert!(affected.contains(&"src/lib.rs"), "lib.rs imports util.rs"); - assert!(affected.contains(&"src/main.rs"), "main.rs → lib.rs → util.rs"); + assert!( + affected.contains(&"src/main.rs"), + "main.rs → lib.rs → util.rs" + ); } #[test] @@ -496,3 +499,130 @@ fn index_update_reindexes_changed_files() { "index update picks up the new symbol" ); } + +#[test] +fn index_update_reimports_unchanged_file_and_dedups() { + let tmp = cache_dir().join("update-reimport"); + let _ = std::fs::remove_dir_all(&tmp); + let src = tmp.join("src"); + std::fs::create_dir_all(&src).unwrap(); + // Two `use` lines targeting the same (not-yet-existing) module. + std::fs::write( + src.join("main.rs"), + "use util::hello;\nuse util::world;\n\nfn main() {}\n", + ) + .unwrap(); + + run_json(&["index", "create", tmp.to_str().unwrap()]); + + std::fs::write( + src.join("util.rs"), + "pub fn hello() {}\npub fn world() {}\n", + ) + .unwrap(); + run_json(&["index", "update", tmp.to_str().unwrap()]); + + let after = run_json(&[ + "--project", + tmp.to_str().unwrap(), + "find", + "importers", + "src/util.rs", + ]); + let names: Vec<&str> = after + .as_array() + .unwrap() + .iter() + .map(|v| v.as_str().unwrap()) + .collect(); + assert_eq!( + names, + vec!["src/main.rs"], + "unchanged main.rs re-resolves to the new util.rs, deduplicated" + ); +} + +#[test] +fn rust_pub_use_re_exports_resolve() { + let tmp = cache_dir().join("pub-use"); + let _ = std::fs::remove_dir_all(&tmp); + let src = tmp.join("src"); + std::fs::create_dir_all(&src).unwrap(); + std::fs::write(src.join("inner.rs"), "pub fn x() {}\n").unwrap(); + std::fs::write(src.join("re.rs"), "pub use inner;\n").unwrap(); + + run_json(&["index", "create", tmp.to_str().unwrap()]); + + let importers = run_json(&[ + "--project", + tmp.to_str().unwrap(), + "find", + "importers", + "src/inner.rs", + ]); + let names: Vec<&str> = importers + .as_array() + .unwrap() + .iter() + .map(|v| v.as_str().unwrap()) + .collect(); + assert!( + names.contains(&"src/re.rs"), + "pub use in re.rs re-exports inner.rs" + ); +} + +#[test] +fn mcp_reports_invalid_params_and_parse_errors() { + let src = fixture("rustdemo"); + let tmp = cache_dir().join("mcp-errors"); + let _ = std::fs::remove_dir_all(&tmp); + copy_dir(&src, &tmp); + run_json(&["index", "create", tmp.to_str().unwrap()]); + + let mut child = Command::new(bin()) + .args(["--project", tmp.to_str().unwrap(), "serve"]) + .env("XDG_CACHE_HOME", cache_dir()) + .stdin(Stdio::piped()) + .stdout(Stdio::piped()) + .stderr(Stdio::null()) + .spawn() + .expect("spawn serve"); + + { + let mut stdin = child.stdin.take().unwrap(); + // Missing required `query` argument -> tool-level isError. + writeln!( + stdin, + r#"{{"jsonrpc":"2.0","id":2,"method":"tools/call","params":{{"name":"search_symbols","arguments":{{}}}}}}"# + ) + .unwrap(); + // Malformed JSON -> JSON-RPC parse error (id null, code -32700). + writeln!(stdin, "this is not json").unwrap(); + } + + let out = child.wait_with_output().unwrap(); + let responses: Vec = String::from_utf8_lossy(&out.stdout) + .lines() + .filter_map(|l| serde_json::from_str(l).ok()) + .collect(); + + let call = responses + .iter() + .find(|r| r["id"] == 2) + .expect("tools/call response"); + assert_eq!(call["result"]["isError"], true, "missing arg is an error"); + assert!( + call["result"]["content"][0]["text"] + .as_str() + .unwrap() + .contains("query"), + "error names the missing argument" + ); + + let parse = responses + .iter() + .find(|r| r["error"]["code"] == -32700) + .expect("parse error response"); + assert_eq!(parse["error"]["message"], "parse error"); +}