From c2940ad07b71428f3dad4ed49103ba09c0e3440b Mon Sep 17 00:00:00 2001 From: Stephane Segning Lambou Date: Fri, 7 Aug 2026 03:40:56 +0200 Subject: [PATCH 1/2] fix: extract bodiless trait methods and stop binary content reaching chunks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #1 and #2 — two gaps in the extraction path, both pre-existing. #1 Trait methods declared without a default body parse as `function_signature_item`, not `function_item`, and were classified as nothing at all — so a Rust trait interface contributed zero callable symbols to the graph AND no chunk to semantic search. They are now definitions: a node, a `method` edge from the trait, and a chunk, with the same `chunk_type` as a bodied function (a new value would leak into every consumer's stored data for no reader benefit). They are deliberately NOT call targets. The resolver is precision-favouring: several same-named candidates with no qualifier are dropped, not fanned out. Had declarations joined the candidate set, a trait with exactly one impl would have gone from one candidate to two and every call to that method would have stopped resolving — fixing a missing-symbol bug by silently deleting `calls` edges. A call dispatches to an implementation, never to a declaration. #2 `chunk_file` rejected binary content via a NUL sniff, but the graph-enabled walk never calls it — it parses the tree itself and falls back straight to `chunk_text`/`window_chunks`, which had no guard. A binary blob that happens to be valid UTF-8 (NUL is a legal codepoint) was windowed with raw NUL bytes in `Chunk::content`, on the one path production runs. PostgreSQL's `text` rejects NUL outright, so this failed at persist time far from its cause. The guard is now a shared `is_binary` applied where a chunk is produced (`chunk_text`) and in the walk before either consumer — so the graph never ingests binary either, not just the chunker. The sample-repo golden gains exactly one node and one `method` edge (the `Shape::describe` declaration it always contained). Additions only: no edge removed, no `calls` edge lost. Coverage 93.8% -> 95.4%. Co-Authored-By: Claude Opus 5 --- src/chunk.rs | 68 +++++++++++++++++- src/graph/emit.rs | 106 ++++++++++++++++++++++++++-- src/graph/tests.rs | 50 +++++++++++++ src/walk.rs | 6 ++ tests/golden/sample-repo.graph.json | 11 +++ tests/robustness.rs | 80 +++++++++++++-------- 6 files changed, 285 insertions(+), 36 deletions(-) diff --git a/src/chunk.rs b/src/chunk.rs index b9b6df7..a4df543 100644 --- a/src/chunk.rs +++ b/src/chunk.rs @@ -38,8 +38,9 @@ pub fn chunk_file( if source.len() > MAX_FILE_BYTES { return Vec::new(); } - // Detect binary content by scanning the first 512 bytes for null bytes. - if source.as_bytes().iter().take(512).any(|&b| b == 0) { + // Cheap early-out before we pay for a parse. The authoritative guard lives in `chunk_text` + // (and in the walk, before the graph sees the bytes) so it cannot be bypassed — see `is_binary`. + if is_binary(source) { return Vec::new(); } @@ -84,9 +85,27 @@ pub fn chunk_tree( /// Chunk free text (e.g. PDF-extracted text) through the windowed path text files already take. #[must_use] pub fn chunk_text(file_path: &str, text: &str, language: &str, tuning: IndexTuning) -> Vec { + // The guard belongs HERE, at the point a chunk is produced — not only at `chunk_file`'s door. + // `chunk_file` used to be the sole holder of the binary check, and the graph-enabled walk path + // never calls it (it parses the tree itself and falls back straight to windowing), so raw NUL + // bytes reached `Chunk::content` on the one path production actually runs. + if is_binary(text) { + return Vec::new(); + } window_chunks(file_path, text, language, tuning) } +/// Binary-content sniff: a NUL byte within the first 512 bytes. +/// +/// NUL is a perfectly legal Unicode scalar, so a blob can be valid UTF-8 — passing every +/// `read_to_string` check — and still be binary. It has to be caught by content, not by encoding. +/// This matters downstream and not just aesthetically: PostgreSQL's `text` type rejects the NUL +/// codepoint outright, so a contaminated chunk fails at persist time, far from its cause. +#[must_use] +pub(crate) fn is_binary(source: &str) -> bool { + source.as_bytes().iter().take(512).any(|&b| b == 0) +} + /// Recursively collect interesting nodes. We walk the full tree (not just top-level children) so that /// methods inside `impl` blocks, nested functions, and inner classes are captured. fn collect_items( @@ -154,6 +173,14 @@ pub(crate) fn interesting_node( let (kind, name_field) = match node.kind() { // Rust "function_item" => ("function", Some("name")), + // A trait method declared WITHOUT a default body (`fn greet(&self);`) parses as + // `function_signature_item`, not `function_item`. Omitting it made trait interfaces — + // arguably the most important symbols in a Rust codebase — invisible to both the chunker + // and the graph. Same `kind` as a bodied function on purpose: it is the same thing to a + // reader searching for it, and a new `chunk_type` value would leak into every consumer's + // stored data. The graph draws the one distinction that matters (it is not a call target) + // from the tree-sitter node kind instead — see `Classifier::is_call_target`. + "function_signature_item" => ("function", Some("name")), "impl_item" => ("impl", None), "struct_item" => ("struct", Some("name")), "enum_item" => ("enum", Some("name")), @@ -306,4 +333,41 @@ mod tests { ); assert!(chunks.iter().all(|c| c.chunk_type == "window")); } + + #[test] + fn signature_only_trait_method_is_chunked_like_a_bodied_one() { + // REGRESSION: `fn greet(&self);` parses as `function_signature_item`, which was classified + // as nothing — so a trait interface produced no chunk and was invisible to semantic search. + let src = "pub trait Greeter {\n fn greet(&self) -> String;\n fn shout(&self) -> String { String::new() }\n}\n"; + let chunks = chunk_file("g.rs", src, "rust", IndexTuning::default()); + let names: Vec<&str> = chunks + .iter() + .filter_map(|c| c.symbol_name.as_deref()) + .collect(); + assert!( + names.contains(&"greet"), + "the bodiless declaration must be chunked; got {names:?}" + ); + assert!( + names.contains(&"shout"), + "the default-bodied method must still be chunked; got {names:?}" + ); + // Same chunk_type as a bodied function on purpose — a new value would leak into every + // consumer's stored data for no reader-visible benefit. + let greet = chunks + .iter() + .find(|c| c.symbol_name.as_deref() == Some("greet")) + .unwrap(); + assert_eq!(greet.chunk_type, "function"); + } + + #[test] + fn is_binary_detects_a_nul_only_within_the_sniff_window() { + assert!(is_binary("a\0b")); + assert!(!is_binary("plain text")); + // NUL beyond the 512-byte sniff window is deliberately not detected — the guard is a cheap + // prefix sniff, not a full scan. Documented so the bound is a decision, not an accident. + let late = format!("{}\0", "x".repeat(512)); + assert!(!is_binary(&late)); + } } diff --git a/src/graph/emit.rs b/src/graph/emit.rs index e048685..e8b7e1f 100644 --- a/src/graph/emit.rs +++ b/src/graph/emit.rs @@ -115,8 +115,13 @@ fn walk( source_file: source_file.to_string(), start_line, }); - // Functions/methods are callable — record for resolution, tagged with their type scope. - if is_callable && let Some(n) = name.clone() { + // Record for resolution, tagged with type scope. Note this is `is_call_target`, NOT + // `is_callable`: a bodiless trait method is callable-shaped (it earned the `()` label + // and the `method` edge above) but must not compete with its own implementations for + // the name — see `Classifier::is_call_target`. + if classifier.is_call_target(&child, kind) + && let Some(n) = name.clone() + { facts.callables.push(Callable { name: n, node_id: node_id.clone(), @@ -199,6 +204,33 @@ impl Classifier<'_> { } } + /// Whether a definition is a **call target** — something a call site may resolve to. + /// + /// This is deliberately narrower than "is callable-shaped". A callable-shaped def gets the `()` + /// label suffix and a `method` edge; a *call target* additionally competes for a name in + /// [`super::resolve::resolve`]. A trait method DECLARATION is the first but not the second: a + /// call dispatches to an implementation, never to the declaration. + /// + /// Keeping declarations out of the candidate set is load-bearing, not tidiness. The resolver is + /// precision-favouring — several same-named candidates with no disambiguating qualifier are + /// dropped, not fanned out. A trait with exactly one impl would otherwise go from one candidate + /// to two the moment declarations were indexed, and every call to that method would stop + /// resolving. Fixing the missing-symbol bug would then have silently deleted `calls` edges. + fn is_call_target(self, node: &Node<'_>, kind: &str) -> bool { + if !matches!(kind, "function" | "method") { + return false; + } + match self { + // The one Rust node kind that is callable-shaped but bodiless. + Classifier::Rust => node.kind() != "function_signature_item", + // Tags-driven languages capture definitions, and an abstract/interface method that the + // grammar's `tags.scm` reports is still reported as a definition; leave them as targets + // rather than guess per-grammar. Revisit if a language shows the same duplicate-name + // regression Rust would have had. + Classifier::Tagged(_) => true, + } + } + /// The type name a container def introduces for its children — used only to disambiguate several /// same-named callables (e.g. two classes each with `run`). `None` for non-containers. fn container_scope( @@ -329,9 +361,7 @@ mod tests { #[test] fn trait_container_emits_a_method_edge_for_its_fn() { - // A default-bodied trait method: interesting_node only classifies `function_item`, which is - // what a body-bearing method parses as. (A signature-only `fn f(&self);` parses as - // `function_signature_item` and is NOT classified at all — see the crate-level bug report.) + // A default-bodied trait method (`function_item`). let src = "trait T {\n fn f(&self) {}\n}\n"; let facts = facts_for("rust", "src/t.rs", src); let trait_node = facts.nodes.iter().find(|n| n.label == "T").unwrap(); @@ -345,6 +375,72 @@ mod tests { ); } + #[test] + fn signature_only_trait_method_is_a_node_with_a_method_edge() { + // REGRESSION: a trait method with no default body parses as `function_signature_item`, not + // `function_item`, and used to be classified as nothing at all — so a trait interface + // contributed zero callable symbols to the graph or to semantic search. + let src = "trait T {\n fn f(&self);\n}\n"; + let facts = facts_for("rust", "src/t.rs", src); + let trait_node = facts.nodes.iter().find(|n| n.label == "T").unwrap(); + let f = facts + .nodes + .iter() + .find(|n| n.label == "f()") + .expect("signature-only trait method must produce a node"); + assert_eq!(f.start_line, 2); + assert!( + facts.contains.iter().any(|e| e.relation == "method" + && e.source == trait_node.node_id + && e.target == f.node_id), + "trait → f must be a `method` edge; got {:?}", + facts.contains + ); + } + + #[test] + fn signature_only_trait_method_is_not_a_call_target() { + // It is callable-SHAPED (it earned the `()` label and the `method` edge above) but must not + // enter the resolver's candidate set: a call dispatches to an implementation, never to the + // declaration. See `Classifier::is_call_target`. + let src = "trait T {\n fn f(&self);\n}\n"; + let facts = facts_for("rust", "src/t.rs", src); + assert!( + facts.callables.is_empty(), + "a bodiless declaration must not be a call target; got {:?}", + facts.callables + ); + } + + #[test] + fn a_bodied_method_next_to_its_declaration_is_still_the_only_call_target() { + // The whole reason declarations are excluded from the candidate set. A trait with exactly + // ONE impl would otherwise go from one candidate to two the moment declarations were + // indexed, and the precision-favouring resolver would drop every call to that method — + // so fixing the missing-symbol bug would have silently deleted `calls` edges. + let src = "trait T {\n fn f(&self);\n}\n\ + struct S;\n\ + impl T for S {\n fn f(&self) {}\n}\n"; + let facts = facts_for("rust", "src/t.rs", src); + let targets: Vec<_> = facts.callables.iter().map(|c| c.node_id.as_str()).collect(); + assert_eq!( + targets.len(), + 1, + "exactly one call target (the impl) must survive; got {targets:?}" + ); + assert!( + targets[0].ends_with(":f"), + "the surviving target must be the bodied impl method; got {targets:?}" + ); + // Both defs still exist as nodes — the declaration is indexed, just not dispatched to. + assert_eq!( + facts.nodes.iter().filter(|n| n.label == "f()").count(), + 2, + "declaration and implementation must BOTH be nodes; got {:?}", + facts.nodes + ); + } + #[test] fn tagged_class_container_emits_a_method_edge_for_its_method() { let src = "class C:\n def m(self):\n pass\n"; diff --git a/src/graph/tests.rs b/src/graph/tests.rs index 7951419..dc755fd 100644 --- a/src/graph/tests.rs +++ b/src/graph/tests.rs @@ -651,3 +651,53 @@ class Builder { Object build() { return Circle.make(); } } "→ Circle.make only" ); } + +#[test] +fn a_call_to_a_single_impl_trait_method_still_resolves_to_the_impl() { + // The end-to-end guard for the decision in `Classifier::is_call_target`. Indexing trait method + // DECLARATIONS as nodes (so trait interfaces are searchable) must not make them compete with + // their own implementations for the name: the resolver is precision-favouring and drops a bare + // name matching several candidates. If declarations were call targets, this call would go from + // one candidate to two and the `calls` edge below would silently disappear. + let g = graph_of(&[ + ( + "src/shape.rs", + "trait Shape {\n fn describe(&self) -> f64;\n}\n", + ), + ( + "src/circle.rs", + "struct Circle;\nimpl Shape for Circle {\n fn describe(&self) -> f64 {\n 1.0\n }\n}\n", + ), + ( + "src/main.rs", + "fn run(c: &Circle) -> f64 {\n c.describe()\n}\n", + ), + ]); + + // Both the declaration and the implementation are indexed as nodes. + let describes: Vec<&GraphNode> = g.nodes.iter().filter(|n| n.label == "describe()").collect(); + assert_eq!( + describes.len(), + 2, + "declaration and implementation must both be nodes; got {describes:?}" + ); + assert!(describes.iter().any(|n| n.source_file == "src/shape.rs")); + + // ...but the call resolves, and resolves to the IMPLEMENTATION, not the declaration. + let caller = node(&g, "run()").node_id.clone(); + let targets: Vec<&str> = g + .edges + .iter() + .filter(|e| e.relation == "calls" && e.source == caller) + .map(|e| e.target.as_str()) + .collect(); + assert_eq!( + targets.len(), + 1, + "the call must still resolve to exactly one target; got {targets:?}" + ); + assert!( + targets[0].starts_with("src/circle.rs"), + "must resolve to the impl in circle.rs, not the declaration; got {targets:?}" + ); +} diff --git a/src/walk.rs b/src/walk.rs index 47c0b13..0ddad72 100644 --- a/src/walk.rs +++ b/src/walk.rs @@ -157,6 +157,12 @@ pub fn walk_checkout(root: &Path, options: &WalkOptions) -> anyhow::Result MAX_FILE_BYTES { continue; // over the byte cap } + // Binary content, caught by CONTENT rather than encoding: NUL is a legal Unicode scalar, so + // a binary blob can decode as valid UTF-8 above and still be junk. Rejected here — before + // either consumer — so the graph never ingests it either, not just the chunker. + if chunk::is_binary(&source) { + continue; + } let file_chunks = if options.build_graph && lang::has_graph(language) { // Parse ONCE and feed both the chunker and the graph builder (ADR-0086 "parse once"). diff --git a/tests/golden/sample-repo.graph.json b/tests/golden/sample-repo.graph.json index 9ec87ff..757f2d2 100644 --- a/tests/golden/sample-repo.graph.json +++ b/tests/golden/sample-repo.graph.json @@ -78,6 +78,12 @@ "source_file": "src/shapes.rs", "start_line": 30 }, + { + "node_id": "src/shapes.rs#31:describe", + "label": "describe()", + "source_file": "src/shapes.rs", + "start_line": 31 + }, { "node_id": "src/shapes.rs#34:impl", "label": "impl", @@ -196,6 +202,11 @@ "target": "src/shapes.rs#25:new", "relation": "method" }, + { + "source": "src/shapes.rs#30:Shape", + "target": "src/shapes.rs#31:describe", + "relation": "method" + }, { "source": "src/shapes.rs#34:impl", "target": "src/shapes.rs#35:describe", diff --git a/tests/robustness.rs b/tests/robustness.rs index 43557a4..60419e7 100644 --- a/tests/robustness.rs +++ b/tests/robustness.rs @@ -95,20 +95,15 @@ fn valid_utf8_binary_looking_content_with_a_source_extension_does_not_panic() { } #[test] -fn build_graph_path_leaks_raw_nul_bytes_into_a_window_chunk_flagged_not_fixed() { - // REAL BUG, confirmed by inspection and deliberately NOT fixed here (out of scope for a - // black-box test suite — `src/**` is off limits for this change): `chunk_file` explicitly - // rejects binary content by scanning the first 512 bytes for a NUL (`src/chunk.rs`, - // `chunk_file`). But `walk_checkout`'s graph-enabled branch (`build_graph == true` and the - // language `has_graph`) never calls `chunk_file` — it parses the tree once and, when tree-sitter - // finds no "interesting" definitions at all (as here: no recognizable Rust syntax), falls back - // directly to `chunk::chunk_text` → `window_chunks`, which has NO binary/NUL guard at all. The - // result: a byte-for-byte binary blob (as long as it happens to be valid UTF-8 — NUL is a legal - // codepoint) with a graphed-language extension gets windowed into a chunk whose `content` field - // contains raw NUL bytes, something `chunk_file` (the non-graph path, and `chunk_file` called - // directly) would have rejected outright. This is a real behavioural gap between the two chunking - // entry points, not a crash — the walk completes fine — so it's asserted here as CURRENT - // behaviour, not desired behaviour. +fn build_graph_path_rejects_a_nul_laden_blob_exactly_as_chunk_file_does() { + // REGRESSION TEST for the bug this replaces. `chunk_file` has always rejected binary content by + // scanning the first 512 bytes for a NUL, but `walk_checkout`'s graph-enabled branch never calls + // `chunk_file` — it parses the tree itself and, when tree-sitter finds no interesting definitions + // (as here: no recognisable Rust syntax), falls back straight to `chunk_text` -> `window_chunks`, + // which had no guard at all. A binary blob that happens to be valid UTF-8 (NUL is a legal + // codepoint) therefore got windowed with raw NUL bytes in `Chunk::content` — on the ONE path + // production actually runs. The two entry points now agree: the guard sits where the chunk is + // produced, so it cannot be bypassed by choosing a different door. let dir = tempfile::tempdir().unwrap(); let root = dir.path(); let garbage: Vec = vec![0u8, 1, 2, 3, 0u8, 5, 6, 0u8]; @@ -117,23 +112,17 @@ fn build_graph_path_leaks_raw_nul_bytes_into_a_window_chunk_flagged_not_fixed() let options = WalkOptions::builder().build_graph(true).build(); let out = walk_checkout(root, &options).unwrap(); - let chunk = out - .chunks - .iter() - .find(|c| c.file_path == "src/garbage.rs") - .expect( - "current behaviour: the NUL-laden blob IS chunked on the build_graph path, unlike \ - chunk_file's binary guard would allow", - ); - assert_eq!(chunk.chunk_type, "window"); assert!( - chunk.content.contains('\0'), - "current behaviour: raw NUL bytes end up in chunk content: {:?}", - chunk.content + !chunked(&out.chunks, "src/garbage.rs"), + "binary blob must not be chunked on the build_graph path: {:?}", + out.chunks + ); + assert!( + !out.chunks.iter().any(|c| c.content.contains('\0')), + "no chunk may carry a raw NUL byte (PostgreSQL `text` rejects it outright)" ); - // Contrast: the SAME bytes through `chunk_file` directly (the path a non-graphed language, or - // `build_graph == false`, actually takes) are correctly rejected as binary. + // The two entry points must agree about identical bytes — that agreement is the actual fix. let direct = lci_codegraph::chunk_file( "src/garbage.rs", std::str::from_utf8(&garbage).unwrap(), @@ -142,7 +131,40 @@ fn build_graph_path_leaks_raw_nul_bytes_into_a_window_chunk_flagged_not_fixed() ); assert!( direct.is_empty(), - "chunk_file's binary guard rejects the same bytes; chunk_tree/window_chunks does not" + "chunk_file and the graph-enabled walk must reach the same verdict on the same bytes" + ); +} + +#[test] +fn binary_blob_is_kept_out_of_the_graph_too_not_just_the_chunks() { + // The guard runs in the walk BEFORE either consumer, so a binary blob never reaches the graph + // builder either. Guarding only inside the chunker would have left tree-sitter parsing garbage + // into `:Symbol` nodes. + let dir = tempfile::tempdir().unwrap(); + let root = dir.path(); + let mut body = vec![0u8, 0u8]; + // Real, parseable Rust *after* the NUL prefix: without a content-based guard this would emit a + // perfectly ordinary-looking `victim()` node sourced from a file that is actually binary. + body.extend_from_slice(b"fn victim() {}\n"); + common::write_bytes(root, "src/blob.rs", &body); + common::write(root, "src/real.rs", "fn real() {}\n"); + + let options = WalkOptions::builder().build_graph(true).build(); + let out = walk_checkout(root, &options).unwrap(); + + assert!( + !out.graph + .nodes + .iter() + .any(|n| n.source_file == "src/blob.rs"), + "binary file contributed graph nodes: {:?}", + out.graph.nodes + ); + // ...and a clean file in the same walk is unaffected. + assert!( + out.graph.nodes.iter().any(|n| n.label == "real()"), + "the clean sibling must still be extracted: {:?}", + out.graph.nodes ); } From daeece80e2ffaa7a6e87215ff04764b2435bf5f6 Mon Sep 17 00:00:00 2001 From: Stephane Segning Lambou Date: Fri, 7 Aug 2026 04:01:24 +0200 Subject: [PATCH 2/2] fix: count binary-skipped files in WalkStats Raised by adversarial review of this PR: a file rejected by the new is_binary guard incremented no counter at all, so it vanished from the walk summary. Before this PR the same file WAS counted into files_chunked (with NUL-contaminated content), so the fix traded a data bug for an observability gap. Without the counter, 'this repo has fewer indexable files than expected' and 'a binary asset is misnamed with a source extension' look identical from the summary line, and only the second is actionable. Co-Authored-By: Claude Opus 5 --- src/walk.rs | 10 ++++++++++ tests/robustness.rs | 9 +++++++++ 2 files changed, 19 insertions(+) diff --git a/src/walk.rs b/src/walk.rs index 0ddad72..ff86370 100644 --- a/src/walk.rs +++ b/src/walk.rs @@ -48,6 +48,13 @@ pub struct WalkStats { pub paths_ignored: usize, pub pdfs_extracted: usize, pub pdfs_skipped: usize, + /// Files rejected as binary by the content sniff (a NUL byte in the first 512 bytes), despite + /// decoding as valid UTF-8 and carrying an indexable extension. + /// + /// Counted rather than silently dropped: without it, "this repo has fewer indexable files than + /// I expected" and "a generated/binary asset is misnamed with a source extension" look exactly + /// the same from the summary line — and the second is an actionable repo problem. + pub files_skipped_binary: usize, } /// The output of a walk: chunks to embed and the resolved structural graph. @@ -161,6 +168,8 @@ pub fn walk_checkout(root: &Path, options: &WalkOptions) -> anyhow::Result anyhow::Result