From edc9a78011d2cf06d02526161beb19933a5ea731 Mon Sep 17 00:00:00 2001 From: flupkede Date: Sat, 17 Oct 2026 21:32:01 +0200 Subject: [PATCH] =?UTF-8?q?fix:=20reconcile=20the=20line-number=20conventi?= =?UTF-8?q?on=20=E2=80=94=20chunkers=20emit=201-based,=20responses=20pass?= =?UTF-8?q?=20through?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The PR #308 merge combined two internally-consistent conventions into an inconsistent tree: develop's chunkers emit 1-based editor lines (the find -> find_impact position round-trip fix), while the pilot batch's emit boundaries added +1 on top of the then-0-based chunkers. Every response reported line N as N+1 for real chunks, and get_chunk's context_before included the chunk's own first line. Reconciled to: chunker emits 1-based, emit boundaries pass through. - stripped the +1 at all 22 emit sites (find, explore, get_chunk, graph, search funnel, CLI renderer) — all introduced by the pilot batch, verified against both parents - get_chunk's context windows restored to the 1-based reading (the before-window's saturating_sub(1) was correct all along; the pin test had hand-built a 0-based fixture no real chunk matches) - response-level fixtures moved to 1-based values so they represent real chunks - Chunk doc comment now states the shipped convention - new end-to-end test ties the real SemanticChunker to the get_chunk response (producer and emitter can no longer drift apart silently) - CHANGELOG entry describes the shipped mechanism --- CHANGELOG.md | 2 +- src/chunker/mod.rs | 7 +-- src/mcp/explore.rs | 8 +-- src/mcp/find.rs | 8 +-- src/mcp/get_chunk.rs | 13 +++-- src/mcp/graph.rs | 12 ++--- src/mcp/search.rs | 4 +- src/search/mod.rs | 8 +-- src/serve/tests.rs | 121 ++++++++++++++++++++++++++++++++++++++++--- 9 files changed, 146 insertions(+), 37 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c246e8f7..4b8dd42a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -121,7 +121,7 @@ finalized in place with a date — no renaming/migration step needed. - **Group fan-out hits resolve in their origin store, with the alias-prefixed path.** Chunk ids are per-repo counters, so across a group every repo answers for id 0 and any two non-empty repos collide. The fan-out helpers deduplicated by (alias, chunk_id) but flattened to bare results, so every downstream resolver probed all stores and kept the first get_chunk(id) hit — the alphabetically-first repo stole the hit, the true definition died in the KINDS filter, unrelated content rode the stolen score, and group paths never picked up their alias prefix. MultiReadOutcome now carries SourcedResult{alias, result} from the helpers themselves: find definition/usages, literal search (BM25 and scan paths), group semantic in all three modes ((alias, chunk_id) interned to unique synthetic ids for the bare-u32 RRF fusion), graph imports/dependents/similar, explore outline dedup and get_chunk auto-routing all resolve the chunk only in its origin store and prefix the path from that store's alias; merge_exact_into_fts gains a (alias, chunk_id)-keyed sibling for the group lexical path. - **Mixed-model groups warn that scores are not comparable.** A group whose repos embed with different models answers with cosine distances from different vector spaces, which the merged ranking presents as one scale — an agent reading the top hit's score as "strongest match overall" is comparing coordinates from unrelated spaces. The group path now collects the distinct models that actually answered and appends a warning naming every one of them; RRF fusion in auto/hybrid partially hides the problem by fusing ranks only, so semantic_mode's raw cosine scores are where it bites hardest. - **Literal search relaxes to OR when the exact AND query matches nothing.** Business queries arrive as sentences, and a chunk holding most of their terms is a better answer than silence. When the conjunctive pass returns nothing and the query carries at least two significant (non-stopword) terms, the search retries as a disjunction and keeps candidates covering 60% of those terms (minimum two); coverage counts terms in the stored searchable_text (signature + docstring + content — the same fields the relaxed pass queries and rescues by), with a bare-content fallback for legacy blobs, so a candidate rescued BY its signature is no longer dropped by a gate that only read its content. The response flags relaxed_fallback and explains the mode in a note. Single-term queries never relax, so an empty answer for an unknown identifier stays an honest refusal. -- **All responses emit 1-based line numbers.** Chunks carry 0-indexed start/end lines by design and every consumer emitted them raw — a symbol defined on the first line of a file showed start_line 0. The conversion happens exactly once at each final emit boundary (semantic funnel, literal match lines, find, explore outline, get_chunk, similar chunks, imports, CLI renderer); intermediate vectordb results, store metadata, chunk-internal context slicing and SCIP-based positions stay untouched, and the Chunk doc comment pins the convention so the offset cannot be applied twice. +- **All responses emit 1-based line numbers.** A symbol defined on the first line of a file used to show start_line 0. The chunkers emit 1-based editor lines — the same convention the SCIP position index and find_impact position queries use — and every response surface passes them through unchanged (semantic funnel, literal match lines, find, explore outline, get_chunk, similar chunks, imports, CLI renderer); intermediate vectordb results, store metadata, chunk-internal context slicing and SCIP-based positions stay untouched. The Chunk doc comment pins the convention, and an end-to-end test ties the real chunker to the get_chunk response so the two ends cannot drift apart again (an earlier draft of this batch added a +1 at every emit boundary on top of 0-based chunkers; once the chunkers moved to 1-based that double-converted every line, which the merge reconciled to emit-passthrough). - **get_chunk reads surrounding context from the owning repo's root.** Chunk paths are stored project-relative, and the group-scoped answer rewrites them with the alias prefix before the context read — which then joined the prefixed path onto the serve process's own project root, a location that never exists. Every group-mode get_chunk with context_lines therefore answered "source file not readable, returning indexed content only", always. The context read now resolves the pre-prefix stored path against the owning alias's root from the routing context (`alias_roots`), keeping the historical join only for stdio single-repo mode where it was already correct. - **Optional min_score threshold for semantic search, with an explicit refusal.** Nearest-neighbour search always answers something, so junk queries got confidently scored garbage and negative probes looked like weak hits. Hits below the opt-in threshold are dropped in the single funnel every local mode shares, and an answer emptied by the filter carries a note naming the threshold instead of reading as "no match in the index"; scores are mode-specific (cosine similarity for semantic_mode="semantic", RRF points for auto/hybrid/lexical) and the parameter docs spell out both scales. Federated group/project queries now forward the threshold to every leg and surface each leg's refusal note in `warnings` (the local leg hardcoded None and the peer body omitted the field, so both silently ignored it); literal mode has no calibratable scale and refuses the parameter by name instead of silently dropping it. Default behaviour is unchanged. - **Debounced config persist no longer clobbers racing registrations.** The persist worker cloned its config snapshot and wrote repos.json without the config lock, so a POST /repos landing in that window existed only in memory while the stale snapshot hit disk — every reader-side reload_if_changed then trusted the fresh mtime as an external edit and swapped the disk copy in, silently dropping the registration (alias 404s in /info while its build ended with "repo removed or cancelled mid-index"). persist_config now adopts its own write's mtime so an internal persist never masquerades as an external edit, and the persist pass re-verifies memory against the snapshot after each write, rewriting until the file converges. diff --git a/src/chunker/mod.rs b/src/chunker/mod.rs index 07589dad..1ba145a3 100644 --- a/src/chunker/mod.rs +++ b/src/chunker/mod.rs @@ -24,11 +24,12 @@ pub struct Chunk { /// The actual content of the chunk pub content: String, - /// Starting line number (0-indexed; responses convert to 1-based editor - /// lines at the emit boundary) + /// Starting line number (1-based editor line, as emitted by every + /// chunker; responses pass it through unchanged) pub start_line: usize, - /// Ending line number (0-indexed; converted to 1-based on emit) + /// Ending line number (1-based editor line, as emitted by every + /// chunker; responses pass it through unchanged) pub end_line: usize, /// Type of chunk diff --git a/src/mcp/explore.rs b/src/mcp/explore.rs index 395c304f..0c53e010 100644 --- a/src/mcp/explore.rs +++ b/src/mcp/explore.rs @@ -109,8 +109,8 @@ impl CodesearchService { chunk_id: c.id, kind: c.kind, signature: c.signature, - start_line: c.start_line + 1, - end_line: c.end_line + 1, + start_line: c.start_line, + end_line: c.end_line, }); } } @@ -133,8 +133,8 @@ impl CodesearchService { chunk_id: c.id, kind: c.kind, signature: c.signature, - start_line: c.start_line + 1, - end_line: c.end_line + 1, + start_line: c.start_line, + end_line: c.end_line, }) .collect(); out.sort_by_key(|i| i.start_line); diff --git a/src/mcp/find.rs b/src/mcp/find.rs index 0a894817..301bf137 100644 --- a/src/mcp/find.rs +++ b/src/mcp/find.rs @@ -211,7 +211,7 @@ impl CodesearchService { items.push(ReferenceItem { chunk_id: fts_hit.result.chunk_id, path: ctx.prefix_sourced_path(&fts_hit.alias, &chunk.path), - line: chunk.start_line + 1, + line: chunk.start_line, kind: chunk.kind, signature: chunk.signature, score: fts_hit.result.score, @@ -264,7 +264,7 @@ impl CodesearchService { Some(ReferenceItem { chunk_id, path: chunk.path, - line: chunk.start_line + 1, + line: chunk.start_line, kind: chunk.kind, signature: chunk.signature, score, @@ -420,7 +420,7 @@ impl CodesearchService { items.push(ReferenceItem { chunk_id: fts_hit.result.chunk_id, path: ctx.prefix_sourced_path(&fts_hit.alias, &chunk.path), - line: chunk.start_line + 1, + line: chunk.start_line, kind: chunk.kind, signature: chunk.signature, score: fts_hit.result.score, @@ -467,7 +467,7 @@ impl CodesearchService { Some(ReferenceItem { chunk_id, path: chunk.path, - line: chunk.start_line + 1, + line: chunk.start_line, kind: chunk.kind, signature: chunk.signature, score, diff --git a/src/mcp/get_chunk.rs b/src/mcp/get_chunk.rs index 1a9ebb2e..9afa59c4 100644 --- a/src/mcp/get_chunk.rs +++ b/src/mcp/get_chunk.rs @@ -347,12 +347,11 @@ impl CodesearchService { Ok(src) => { let lines: Vec<&str> = src.lines().collect(); if !lines.is_empty() { - // Chunk lines are 0-based (the 1-based conversion - // happens once at the emit boundary below); the - // slice ends at the row before the chunk's first - // line, which is start_line itself. - let before_end = chunk.start_line.min(lines.len()); + // Chunk lines are 1-based; slices need the 0-based + // row before the chunk's first line. + let before_end = chunk.start_line.saturating_sub(1); let before_start = before_end.saturating_sub(context_lines); + let before_end = before_end.min(lines.len()); if before_start < before_end { context_before = Some(lines[before_start..before_end].join("\n")); } @@ -375,8 +374,8 @@ impl CodesearchService { let response = GetChunkResponse { chunk_id: request.chunk_id, path: chunk.path, - start_line: chunk.start_line + 1, - end_line: chunk.end_line + 1, + start_line: chunk.start_line, + end_line: chunk.end_line, kind: chunk.kind, signature: chunk.signature, content: chunk.content, diff --git a/src/mcp/graph.rs b/src/mcp/graph.rs index db90acf0..be5d3770 100644 --- a/src/mcp/graph.rs +++ b/src/mcp/graph.rs @@ -453,7 +453,7 @@ impl CodesearchService { out.push(DependentItem { path: ctx.prefix_sourced_path(&f.alias, &chunk.path), - line: chunk.start_line + 1, + line: chunk.start_line, import_statement, }); } @@ -509,7 +509,7 @@ impl CodesearchService { out.push(DependentItem { path: chunk.path, - line: chunk.start_line + 1, + line: chunk.start_line, import_statement, }); @@ -669,8 +669,8 @@ impl CodesearchService { all_results.push(SearchResultItem { chunk_id: Some(r.id), path: ctx.prefix_sourced_path(alias, &r.path), - start_line: r.start_line + 1, - end_line: r.end_line + 1, + start_line: r.start_line, + end_line: r.end_line, kind: r.kind, score: r.score, signature: r.signature, @@ -726,8 +726,8 @@ impl CodesearchService { .map(|r| SearchResultItem { chunk_id: Some(r.id), path: r.path, - start_line: r.start_line + 1, - end_line: r.end_line + 1, + start_line: r.start_line, + end_line: r.end_line, kind: r.kind, score: r.score, signature: r.signature, diff --git a/src/mcp/search.rs b/src/mcp/search.rs index 06b4f99b..f491a2cf 100644 --- a/src/mcp/search.rs +++ b/src/mcp/search.rs @@ -1322,8 +1322,8 @@ impl CodesearchService { path: r.path, // Chunks store 0-indexed lines; every response emits 1-based // editor lines, so the conversion happens once, here. - start_line: r.start_line + 1, - end_line: r.end_line + 1, + start_line: r.start_line, + end_line: r.end_line, kind: r.kind, score: r.score, signature: r.signature, diff --git a/src/search/mod.rs b/src/search/mod.rs index a779affb..3011bb49 100644 --- a/src/search/mod.rs +++ b/src/search/mod.rs @@ -935,8 +935,8 @@ pub async fn search(query: &str, path: Option, options: SearchOptions) .iter() .map(|r| JsonResult { path: r.path.clone(), - start_line: r.start_line + 1, - end_line: r.end_line + 1, + start_line: r.start_line, + end_line: r.end_line, kind: r.kind.clone(), content: if compact { None @@ -1276,8 +1276,8 @@ fn print_result( // Show location and kind let location = format!( " Lines {}-{} • {}", - result.start_line + 1, - result.end_line + 1, + result.start_line, + result.end_line, sanitize_for_terminal(&result.kind) ); println!("{}", location.dimmed()); diff --git a/src/serve/tests.rs b/src/serve/tests.rs index b950ca57..673f6a6e 100644 --- a/src/serve/tests.rs +++ b/src/serve/tests.rs @@ -5287,7 +5287,7 @@ async fn seed_collision_repo( { let mut vs = stores.vector_store.write().await; let mut chunk = - crate::chunker::Chunk::new(content.to_string(), 0, 4, kind, rel_path.to_string()); + crate::chunker::Chunk::new(content.to_string(), 1, 4, kind, rel_path.to_string()); chunk.signature = signature.map(str::to_string); vs.insert_chunks(vec![crate::embed::EmbeddedChunk::new( chunk, @@ -5415,7 +5415,7 @@ async fn group_find_definition_attributes_hit_to_origin_repo_on_id_collision() { assert_eq!(items[0]["kind"], "Interface", "got: {text}"); assert_eq!( items[0]["line"], 1, - "responses must emit 1-based editor lines (chunk starts at 0-based line 0): {text}" + "responses must emit 1-based editor lines (the interface sits on editor line 1): {text}" ); } @@ -5444,7 +5444,8 @@ async fn group_get_chunk_resolves_context_against_the_owning_repo_root() { .unwrap(); // Repo A: five filler chunks (ids 0..=4) then the target at id 5, - // spanning 0-based lines [2, 4] of the file above. + // spanning editor lines [3, 5] of the file above (chunk lines are + // 1-based — the same convention the real chunker emits). let db_a = root_a.join(DB_DIR_NAME); std::fs::create_dir_all(&db_a).unwrap(); std::fs::write( @@ -5470,8 +5471,8 @@ async fn group_get_chunk_resolves_context_against_the_owning_repo_root() { } let mut target = crate::chunker::Chunk::new( "TARGET LINE THREE\nTARGET LINE FOUR\nTARGET LINE FIVE".to_string(), - 2, - 4, + 3, + 5, crate::chunker::ChunkKind::Function, "src/service/TargetService.java".to_string(), ); @@ -5550,7 +5551,7 @@ async fn group_get_chunk_resolves_context_against_the_owning_repo_root() { "context above must come from the owning repo's real file: {text}" ); assert_eq!( - parsed["context_after"], "TARGET LINE FIVE\nline six", + parsed["context_after"], "line six\nline seven", "context below must come from the owning repo's real file: {text}" ); assert!( @@ -5559,6 +5560,114 @@ async fn group_get_chunk_resolves_context_against_the_owning_repo_root() { ); } +/// End-to-end line-convention tie: a chunk produced by the REAL chunker +/// (1-based editor lines) must come back through get_chunk with that same +/// line — no emit boundary may re-convert it. The PR #308 merge paired +/// develop's 1-based chunker with the PR's +1 emits and every response +/// reported line N as N+1; the two green-but-contradictory halves (the +/// chunker's own test vs response fixtures hand-building 0-based chunks) +/// could not see it. This test ties producer to emitter so they cannot +/// drift apart again. +#[tokio::test] +async fn get_chunk_reports_the_real_chunker_editor_line_unchanged() { + use rmcp::handler::server::wrapper::Parameters; + + let tmp = tempfile::tempdir().unwrap(); + let root = tmp.path().join("solo"); + std::fs::create_dir_all(&root).unwrap(); + let source = "// line one\n// line two\nfn target_fn() {\n let _ = 1;\n}\n"; + std::fs::write(root.join("lib.rs"), source).unwrap(); + + // Real chunker: whatever editor line it says the fn starts on is the + // truth the response must repeat (its 1-based-ness is pinned by the + // chunker's own tests). + let mut chunker = crate::chunker::SemanticChunker::new(100, 4000, 2); + let chunks = chunker + .chunk_semantic( + crate::file::Language::Rust, + std::path::Path::new("lib.rs"), + source, + ) + .expect("real chunker must parse the source"); + let target = chunks + .iter() + .find(|c| c.content.contains("fn target_fn")) + .expect("the fn must be chunked"); + let true_line = target.start_line; + let source_lines: Vec<&str> = source.lines().collect(); + let expected_before = if true_line >= 2 { + Some(source_lines[true_line - 2]) + } else { + None + }; + + // Store it exactly as the indexer would. + let db_path = root.join(DB_DIR_NAME); + std::fs::create_dir_all(&db_path).unwrap(); + std::fs::write( + db_path.join("metadata.json"), + r#"{"schema_version":1,"dimensions":2,"model_short_name":"minilm-l6-q"}"#, + ) + .unwrap(); + let stores = crate::index::SharedStores::new(&db_path, 2).expect("shared stores"); + { + let mut vs = stores.vector_store.write().await; + vs.insert_chunks(vec![crate::embed::EmbeddedChunk::new( + target.clone(), + vec![0.5, 0.5], + )]) + .expect("insert chunk"); + vs.build_index().expect("build index"); + } + { + let mut fts = stores.fts_store.write().await; + fts.add_chunk( + 0, + &target.content, + &target.path, + target.signature.as_deref(), + &format!("{:?}", target.kind), + ) + .expect("fts doc"); + fts.commit().expect("fts commit"); + } + drop(stores); + + let mut config = ReposConfig::default(); + config + .register_with_alias(root.clone(), Some("solo".to_string())) + .unwrap(); + let config_file = tmp.path().join("repos.json"); + config.save_to(&config_file).unwrap(); + let state = std::sync::Arc::new(ServeState::new(config, Some(config_file))); + let service = crate::mcp::CodesearchService::new_for_serve(state).unwrap(); + + let req = crate::mcp::types::GetChunkRequest { + chunk_id: 0, + chunk_ref: None, + context_lines: Some(1), + project: None, + group: None, + }; + let res = service + .get_chunk(Parameters(req)) + .await + .expect("get_chunk must succeed"); + let text = tool_text(&res); + let parsed: serde_json::Value = + serde_json::from_str(&text).unwrap_or_else(|_| panic!("non-JSON answer: {text}")); + assert_eq!( + parsed["start_line"], true_line, + "the emitted line must equal the chunker's 1-based editor line, got: {text}" + ); + if let Some(expected_before) = expected_before { + assert_eq!( + parsed["context_before"], expected_before, + "context_before must be the editor line above the chunk, got: {text}" + ); + } +} + // ═══ Interactive fan-outs skip busy stores instead of queueing behind the // write lock (review cluster B). The priming call opens the group's stores // so `get_opened_stores` hands back the SAME SharedStores the handlers