From 251a920ebea08e98dc7a8f2f64b65686111137aa Mon Sep 17 00:00:00 2001 From: sting8k Date: Sun, 6 Sep 2026 15:22:02 +0700 Subject: [PATCH 1/6] fix: share the definition scan needle between single and batch discover MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Batching supported symbol queries dropped definitions that the same queries found individually, through two defects in one seam — the Aho-Corasick gate in find_definitions_batch: - the gate matched the raw query bytes, while the single search pre-scans with the plain name of a `Q.N` query, so a receiver/container-qualified term never admitted any file and the existing qualified matcher was never reached; - the gate used non-overlapping iteration, so a shorter term masked an overlapping longer one and the longer term's hit bit was never set. Both are fixed at the shared policy: definitions.rs now exports definition_scan_needle(), used by single and batch admission alike, and the batch gate iterates overlapping matches. Needles stay 1:1 with the queries, duplicates included, so a pattern id remains a query index. The original query still decides qualification in the matcher, usage matching is untouched, and single-symbol output is byte-for-byte unchanged. Adds candidate-level and CLI regressions for qualified terms, prefix overlap, shared needles, comment independence, ambiguity and no-match, plus batch cases in the US-071 selector and Windows path suites. --- CHANGELOG.md | 5 + README.md | 2 + skills/srcwalk/GUIDE.md | 2 +- src/search/symbol/batch.rs | 15 +- src/search/symbol/definitions.rs | 16 +- src/search/symbol/tests.rs | 236 +++++++++++++++++++++ tests/us071_selector_round_trip.rs | 64 ++++++ tests/us075_batch_prefilter_equivalence.rs | 209 ++++++++++++++++++ tests/windows_paths.rs | 85 ++++++++ 9 files changed, 624 insertions(+), 10 deletions(-) create mode 100644 tests/us075_batch_prefilter_equivalence.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index 1a37f21..ec3257a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,11 @@ All notable changes to srcwalk are documented here. +## [Unreleased] + +### Fixed +- Symbol batches (`discover 'a,b' --as symbol`) no longer drop definitions that the same terms find on their own. The batch definition prefilter now derives its scan needle exactly like the single-symbol search, so a receiver/container-qualified term keeps its method definition inside a batch (`'Batch.Set,helper'`), and it matches overlapping needles, so a shorter term no longer masks a longer one (`'helper,helper_extra'`). Qualification is still decided by the existing structural matcher, usage matching is unchanged, and single-symbol output stays byte-for-byte identical. + ## [1.8.0] - 2026-08-16 ### Added diff --git a/README.md b/README.md index 6ca6afc..f4a054a 100644 --- a/README.md +++ b/README.md @@ -348,6 +348,8 @@ hunks:
Discover — multi-symbol and multi-scope +Symbol batches take 2-5 comma-separated terms and report one section per term. A receiver/container-qualified term keeps the definition semantics it has as a single query, so `srcwalk discover 'NextAction.new,render_next_actions' --as symbol --scope src/evidence` returns the `NextAction.new` method definition rather than name occurrences only. + ``` $ srcwalk discover "render_next_actions, Anchor" --scope src/evidence --scope src/commands --limit 2 # Search: "render_next_actions" in 2 scopes — 2 matches (1 definitions, 1 name occurrences) diff --git a/skills/srcwalk/GUIDE.md b/skills/srcwalk/GUIDE.md index 47c8b78..1321588 100644 --- a/skills/srcwalk/GUIDE.md +++ b/skills/srcwalk/GUIDE.md @@ -28,7 +28,7 @@ Turn the request's explicit evidence questions into a short coverage list. In ea - Multi-root symbol discovery may repeat the flag: `srcwalk discover 'foo,bar' --as symbol --scope src --scope tests`. - Other routes and `discover --as text|file|access` accept one scope; use a common ancestor or run independent commands in the same model/tool round. - Keep scope as small as the evidence question allows; narrow scopes can hide definitions. -- Symbol batches accept 2-5 comma-separated symbols: `srcwalk discover 'foo,bar,baz' --as symbol --scope src`. Split larger symbol sets. One dot qualifies a method by receiver/container in a single-symbol query (`'Batch.Set'`); dotted terms inside a batch are a known matching limitation, so run that dotted term separately when exact qualification is required. +- Symbol batches accept 2-5 comma-separated symbols: `srcwalk discover 'foo,bar,baz' --as symbol --scope src`. Split larger symbol sets. One dot qualifies a method by receiver/container (`'Batch.Set'`), and a qualified term keeps its single-query definition semantics inside a batch: `srcwalk discover 'Batch.Set,helper' --as symbol --scope src`. - Text OR is separate: `srcwalk discover 'alloc,copy' --match any --as text --scope src` is literal text evidence, not a symbol batch. - Do not infer definitions, usages, callers, deps, or code paths from shell path lists, broad grep, or converted identifier paths. diff --git a/src/search/symbol/batch.rs b/src/search/symbol/batch.rs index e18d39b..4ecd088 100644 --- a/src/search/symbol/batch.rs +++ b/src/search/symbol/batch.rs @@ -12,7 +12,9 @@ use grep_searcher::Searcher; use super::super::{file_metadata, read_file_bytes, walker}; use super::comments::tag_comment_matches; -use super::definitions::{find_defs_from_outline, find_defs_heuristic_buf, find_defs_treesitter}; +use super::definitions::{ + definition_scan_needle, find_defs_from_outline, find_defs_heuristic_buf, find_defs_treesitter, +}; use super::usages::is_word_byte; /// Multi-symbol batch search. @@ -35,8 +37,11 @@ pub(super) fn search_batch( )?]); } - // Build aho-corasick automaton for byte-level any-of gate. - let ac = aho_corasick::AhoCorasick::new(queries).map_err(|e| SrcwalkError::InvalidQuery { + // Build aho-corasick automaton for byte-level any-of gate. Needles use the + // same derivation as the single definition search, and stay 1:1 with the + // queries — duplicates included — so a pattern id is a query index. + let needles: Vec<&str> = queries.iter().map(|q| definition_scan_needle(q)).collect(); + let ac = aho_corasick::AhoCorasick::new(&needles).map_err(|e| SrcwalkError::InvalidQuery { query: queries.join(","), reason: e.to_string(), })?; @@ -140,7 +145,9 @@ fn find_definitions_batch( // Single-pass any-of gate: find which queries hit this file. let mut hit_mask = vec![false; queries.len()]; let mut any_hit = false; - for m in ac.find_iter(&bytes[..]) { + // Overlapping: a shorter needle must not mask a longer one, and + // duplicate needles must each report their own pattern id. + for m in ac.find_overlapping_iter(&bytes[..]) { hit_mask[m.pattern().as_usize()] = true; any_hit = true; } diff --git a/src/search/symbol/definitions.rs b/src/search/symbol/definitions.rs index c12189b..8256924 100644 --- a/src/search/symbol/definitions.rs +++ b/src/search/symbol/definitions.rs @@ -40,6 +40,16 @@ pub(super) fn outline_def_weight(kind: OutlineKind) -> u16 { } } +/// Byte pre-scan needle admitting a file to the definition matcher. +/// +/// US-064: for a `Q.N` query the file may contain `Q` and `N` separately (Go +/// receiver + method name), so the scan uses the plain name; the original query +/// still decides qualification in the matcher. Single and batch admission share +/// this derivation so batching cannot drop definitions (US-075). +pub(super) fn definition_scan_needle(query: &str) -> &str { + split_dot_symbol_query(query).map_or(query, |(_, plain)| plain) +} + /// Find definitions using tree-sitter structural detection. /// For each file containing the query string, parse with tree-sitter and walk /// definition nodes to see if any declare the queried symbol. @@ -59,11 +69,7 @@ pub(super) fn find_definitions_with_artifact( // Relaxed is correct: walker.run() joins all threads before we read the final value. // Early-quit checks are approximate by design — one extra iteration is harmless. let found_count = AtomicUsize::new(0); - // US-064: for a `Q.N` query the file may contain `Q` and `N` separately - // (Go receiver + method name), so the byte pre-scan uses the plain name. - let needle = split_dot_symbol_query(query) - .map_or(query, |(_, plain)| plain) - .as_bytes(); + let needle = definition_scan_needle(query).as_bytes(); let walker = if artifact.enabled() { super::super::io::walker_with_artifact_dirs(scope, glob)? diff --git a/src/search/symbol/tests.rs b/src/search/symbol/tests.rs index fb931a4..657f7b3 100644 --- a/src/search/symbol/tests.rs +++ b/src/search/symbol/tests.rs @@ -567,3 +567,239 @@ fn split_dot_symbol_query_rejects_multi_dot_and_empty_sides() { assert_eq!(split_dot_symbol_query(""), None); assert_eq!(split_dot_symbol_query("Set"), None); } + +/// US-075: the batch definition prefilter must admit every file that the +/// single-symbol prefilter would admit. Overlapping needles rely on +/// `find_overlapping_iter` reporting every pattern id, including duplicates. +#[test] +fn aho_corasick_overlapping_iter_reports_duplicate_and_contained_patterns() { + let ac = aho_corasick::AhoCorasick::new(["Set", "Set", "helper", "helper_extra"]).unwrap(); + let mut ids: Vec = ac + .find_overlapping_iter(&b"func (b *Batch) Set(v int) {}\nfn helper_extra() {}\n"[..]) + .map(|m| m.pattern().as_usize()) + .collect(); + ids.sort_unstable(); + ids.dedup(); + assert_eq!(ids, vec![0, 1, 2, 3]); +} + +/// US-075 definition-identity oracle: for every accepted batch term, the batch +/// must yield the same definition candidates as the single-symbol search. +#[test] +fn batch_definitions_match_single_search_for_qualified_and_overlapping_queries() { + struct Case { + name: &'static str, + files: &'static [(&'static str, &'static str)], + glob: &'static str, + queries: &'static [&'static str], + expected_defs: &'static [usize], + } + + const GO: &[(&str, &str)] = &[( + "sample.go", + "package sample\n\ntype Batch struct{}\n\nfunc (b *Batch) Set(v int) {}\n\ntype Other struct{}\n\nfunc (o *Other) Set(v int) {}\n\nfunc helper() {}\n", + )]; + const RUST: &[(&str, &str)] = &[( + "sample.rs", + "pub struct Batch;\n\nimpl Batch {\n pub fn set(&self, v: i32) -> i32 {\n v\n }\n}\n\npub struct Other;\n\nimpl Other {\n pub fn set(&self, v: i32) -> i32 {\n v\n }\n}\n\npub fn helper() {}\n", + )]; + const PY: &[(&str, &str)] = &[( + "sample.py", + "class Batch:\n def set(self, v):\n return v\n\n\nclass Other:\n def set(self, v):\n return v\n\n\ndef helper():\n return None\n", + )]; + // The long name lives in a file that does not contain the short name, so a + // masked needle cannot be rescued by the other query's admission. + const OVERLAP: &[(&str, &str)] = &[ + ("only_extra.rs", "pub fn helper_extra() -> u8 {\n 1\n}\n"), + ("short.rs", "pub fn helper() -> u8 {\n 2\n}\n"), + ]; + + let cases = [ + Case { + name: "go_qualified", + files: GO, + glob: "*.go", + queries: &["Batch.Set", "helper"], + expected_defs: &[1, 1], + }, + Case { + name: "go_qualified_reversed", + files: GO, + glob: "*.go", + queries: &["helper", "Batch.Set"], + expected_defs: &[1, 1], + }, + Case { + name: "rust_qualified", + files: RUST, + glob: "*.rs", + queries: &["Batch.set", "helper"], + expected_defs: &[1, 1], + }, + Case { + name: "python_qualified", + files: PY, + glob: "*.py", + queries: &["Batch.set", "helper"], + expected_defs: &[1, 1], + }, + Case { + name: "prefix_overlap", + files: OVERLAP, + glob: "*.rs", + queries: &["helper", "helper_extra"], + expected_defs: &[1, 1], + }, + Case { + name: "prefix_overlap_reversed", + files: OVERLAP, + glob: "*.rs", + queries: &["helper_extra", "helper"], + expected_defs: &[1, 1], + }, + Case { + name: "shared_needle", + files: GO, + glob: "*.go", + queries: &["Batch.Set", "Other.Set", "Set"], + expected_defs: &[1, 1, 2], + }, + Case { + name: "shared_needle_reversed", + files: GO, + glob: "*.go", + queries: &["Set", "Other.Set", "Batch.Set"], + expected_defs: &[2, 1, 1], + }, + Case { + name: "max_five_terms", + files: GO, + glob: "*.go", + queries: &["Batch.Set", "Other.Set", "Set", "helper", "Nope.Set"], + expected_defs: &[1, 1, 2, 1, 0], + }, + ]; + + for case in &cases { + let dir = tempfile::tempdir().unwrap(); + for (rel, content) in case.files { + std::fs::write(dir.path().join(rel), content).unwrap(); + } + + let batch = search_batch(case.queries, dir.path(), None, None, Some(case.glob)).unwrap(); + assert_eq!(batch.len(), case.queries.len()); + + for (idx, query) in case.queries.iter().enumerate() { + let single = search(query, dir.path(), None, None, Some(case.glob)).unwrap(); + let batch_defs = definition_keys(&batch[idx], dir.path()); + let single_defs = definition_keys(&single, dir.path()); + assert_eq!( + batch_defs, single_defs, + "{}: batch definitions diverged from single search for '{query}'", + case.name + ); + assert_eq!( + batch_defs.len(), + case.expected_defs[idx], + "{}: unexpected definition count for '{query}' (got {batch_defs:?})", + case.name + ); + } + } +} + +/// US-075 AC-5: a literal comment occurrence may add occurrences but must never +/// change which definitions the batch finds. +#[test] +fn batch_definition_anchors_are_independent_of_comment_occurrences() { + const BASE: &str = "package sample\n\ntype Batch struct{}\n\nfunc (b *Batch) Set(v int) {}\n\nfunc helper() {}\n"; + let queries = ["Batch.Set", "helper"]; + + let anchors = |source: &str| { + let dir = tempfile::tempdir().unwrap(); + std::fs::write(dir.path().join("sample.go"), source).unwrap(); + let batch = search_batch(&queries, dir.path(), None, None, Some("*.go")).unwrap(); + let single = search(queries[0], dir.path(), None, None, Some("*.go")).unwrap(); + ( + definition_keys(&batch[0], dir.path()), + definition_keys(&single, dir.path()), + ) + }; + + let (without_comment, single_without) = anchors(BASE); + let (with_comment, single_with) = anchors(&format!("{BASE}\n// Batch.Set\n")); + + assert_eq!(without_comment.len(), 1, "{without_comment:?}"); + assert_eq!(without_comment, with_comment); + assert_eq!(single_without, single_with); + assert_eq!(without_comment, single_without); +} + +/// US-075 AC-6: same-name definitions in different files stay separate, and a +/// wrong qualifier never acquires a definition because a sibling query admitted +/// the file. +#[test] +fn batch_shared_needle_keeps_qualifier_ownership_and_file_identity() { + let dir = tempfile::tempdir().unwrap(); + std::fs::create_dir_all(dir.path().join("a")).unwrap(); + std::fs::create_dir_all(dir.path().join("b")).unwrap(); + std::fs::write( + dir.path().join("a/sample.go"), + "package a\n\ntype Batch struct{}\n\nfunc (b *Batch) Set(v int) {}\n", + ) + .unwrap(); + std::fs::write( + dir.path().join("b/sample.go"), + "package b\n\ntype Other struct{}\n\nfunc (o *Other) Set(v int) {}\n", + ) + .unwrap(); + + let queries = ["Batch.Set", "Other.Set", "Nope.Set"]; + let batch = search_batch(&queries, dir.path(), None, None, Some("*.go")).unwrap(); + + let batch_first = definition_keys(&batch[0], dir.path()); + let batch_second = definition_keys(&batch[1], dir.path()); + assert_eq!(batch_first.len(), 1, "{batch_first:?}"); + assert_eq!(batch_second.len(), 1, "{batch_second:?}"); + assert!(batch_first.iter().all(|k| k.0 == "a/sample.go")); + assert!(batch_second.iter().all(|k| k.0 == "b/sample.go")); + assert!( + definition_keys(&batch[2], dir.path()).is_empty(), + "wrong qualifier must not acquire a definition" + ); + + for (idx, query) in queries.iter().enumerate() { + let single = search(query, dir.path(), None, None, Some("*.go")).unwrap(); + assert_eq!( + definition_keys(&batch[idx], dir.path()), + definition_keys(&single, dir.path()), + "batch definitions diverged from single search for '{query}'" + ); + } +} + +/// Normalized definition identity: scope-relative path, anchor, range, name and +/// kind weight. Occurrences are excluded so a downgrade cannot pass as a match. +fn definition_keys( + result: &SearchResult, + root: &std::path::Path, +) -> std::collections::BTreeSet<(String, u32, Option<(u32, u32)>, Option, u16)> { + result + .matches + .iter() + .filter(|m| m.is_definition) + .map(|m| { + ( + m.path + .strip_prefix(root) + .unwrap_or(&m.path) + .to_string_lossy() + .replace('\\', "/"), + m.line, + m.def_range, + m.def_name.clone(), + m.def_weight, + ) + }) + .collect() +} diff --git a/tests/us071_selector_round_trip.rs b/tests/us071_selector_round_trip.rs index 50d7aea..3c0ec1e 100644 --- a/tests/us071_selector_round_trip.rs +++ b/tests/us071_selector_round_trip.rs @@ -466,3 +466,67 @@ fn multi_symbol_discover_round_trips_each_section_target() { assert!(!callees.contains("error"), "callees {target}:\n{callees}"); } } + +/// US-075: a batch section whose definition is restored by the shared prefilter +/// must emit the same copyable selector as a single query, and a restored +/// qualified target must still round-trip. +/// +/// A generic container whose selector carries a comma (`Cache.get`) is +/// emitted quoted here but does not replay; that defect predates this story and +/// reproduces identically on a single-symbol query, so it is not asserted. +#[test] +fn batch_sections_emit_quoted_generic_and_round_trip_qualified_targets() { + let fx = Fixture::new( + "batch_generic", + &[ + ( + "src/cache.rs", + "pub struct Cache(K, V);\n\nimpl Cache {\n pub fn get(&self) -> u8 {\n 0\n }\n}\n", + ), + ( + "src/alpha.rs", + "pub struct Alpha;\n\nimpl Alpha {\n pub fn run(&self) {}\n}\n", + ), + ], + ); + + let discovered = fx.ok(&[ + "discover", + "get,Alpha.run", + "--as", + "symbol", + "--scope", + "src", + ]); + assert!( + discovered.contains("> Next: srcwalk show 'src/cache.rs:Cache.get'"), + "a space/comma selector must stay quoted:\n{discovered}" + ); + + let emitted = emitted_targets_and_flags_all(&discovered); + assert_eq!( + emitted.len(), + 2, + "expected one target per term:\n{discovered}" + ); + let targets: Vec<&str> = emitted.iter().map(|(t, _)| t.as_str()).collect(); + assert!( + targets.iter().any(|t| t.ends_with(":Cache.get")), + "expected the generic selector among {targets:?}:\n{discovered}" + ); + assert!( + targets.iter().any(|t| t.ends_with(":Alpha.run")), + "expected the qualified selector among {targets:?}:\n{discovered}" + ); + + // The restored qualified target is copied back verbatim and reads its body. + let (qualified, flags) = emitted + .iter() + .find(|(t, _)| t.ends_with(":Alpha.run")) + .unwrap(); + let shown = fx.ok(&with_flags(&["show"], qualified, flags)); + assert!( + shown.contains("pub fn run(&self) {}"), + "show {qualified}:\n{shown}" + ); +} diff --git a/tests/us075_batch_prefilter_equivalence.rs b/tests/us075_batch_prefilter_equivalence.rs new file mode 100644 index 0000000..6b29703 --- /dev/null +++ b/tests/us075_batch_prefilter_equivalence.rs @@ -0,0 +1,209 @@ +//! US-075: batch prefilter equivalence. +//! +//! Batching supported symbol queries must not remove or downgrade definition +//! evidence that the same queries find individually. The regressions here cover +//! the two prefilter defects: a receiver/container-qualified term whose scan +//! needle differs from its literal spelling, and overlapping term names where a +//! shorter term previously masked a longer one. + +use std::fs; +use std::path::PathBuf; +use std::process::Command; + +fn srcwalk() -> Command { + Command::new(env!("CARGO_BIN_EXE_srcwalk")) +} + +struct Fixture { + dir: PathBuf, +} + +impl Fixture { + fn new(name: &str, files: &[(&str, &str)]) -> Self { + let dir = std::env::temp_dir().join(format!( + "us075_{name}_{}_{}", + std::process::id(), + std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .map_or(0, |d| d.as_nanos()) + )); + fs::create_dir_all(&dir).unwrap(); + for (rel, content) in files { + let path = dir.join(rel); + if let Some(parent) = path.parent() { + fs::create_dir_all(parent).unwrap(); + } + fs::write(path, content).unwrap(); + } + Self { dir } + } + + fn discover(&self, args: &[&str]) -> String { + let out = srcwalk() + .current_dir(&self.dir) + .args(["discover"]) + .args(args) + .output() + .unwrap(); + assert!( + out.status.success(), + "discover {:?} failed: {}", + args, + String::from_utf8_lossy(&out.stderr) + ); + String::from_utf8_lossy(&out.stdout).into_owned() + } +} + +impl Drop for Fixture { + fn drop(&mut self) { + let _ = fs::remove_dir_all(&self.dir); + } +} + +/// Per-query section of a batch packet, matched by query label rather than by +/// presentation order. +fn section<'a>(out: &'a str, query: &str) -> &'a str { + let header = format!("# Search: \"{query}\""); + let start = out + .find(&header) + .unwrap_or_else(|| panic!("no section for '{query}' in:\n{out}")); + let rest = &out[start..]; + match rest[header.len()..].find("\n---\n") { + Some(end) => &rest[..header.len() + end], + None => rest, + } +} + +const GO_FILES: &[(&str, &str)] = &[( + "sample.go", + "package sample\n\ntype Batch struct{}\n\nfunc (b *Batch) Set(v int) {}\n\ntype Other struct{}\n\nfunc (o *Other) Set(v int) {}\n\nfunc helper() {}\n", +)]; + +// The long name lives in a file that does not contain the short name, so a +// masked needle cannot be rescued by the sibling query admitting the file. +const OVERLAP_FILES: &[(&str, &str)] = &[ + ("only_extra.rs", "pub fn helper_extra() -> u8 {\n 1\n}\n"), + ("short.rs", "pub fn helper() -> u8 {\n 2\n}\n"), +]; + +#[test] +fn qualified_term_keeps_its_definition_inside_a_batch() { + let fx = Fixture::new("qualified", GO_FILES); + let single = fx.discover(&["Batch.Set", "--as", "symbol"]); + assert!(single.contains("[fn] Batch.Set sample.go:5-5"), "{single}"); + + for args in [ + ["Batch.Set,helper", "--as", "symbol"], + ["helper,Batch.Set", "--as", "symbol"], + ] { + let out = fx.discover(&args); + let qualified = section(&out, "Batch.Set"); + assert!( + qualified.contains("1 matches (1 definitions)"), + "{args:?}\n{qualified}" + ); + assert!( + qualified.contains("[fn] Batch.Set sample.go:5-5"), + "{args:?}\n{qualified}" + ); + assert!( + qualified.contains("confidence: structural syntax"), + "{args:?}\n{qualified}" + ); + assert!( + section(&out, "helper").contains("[fn] helper sample.go:11-11"), + "{args:?}\n{out}" + ); + } +} + +#[test] +fn overlapping_names_keep_their_definitions_inside_a_batch() { + let fx = Fixture::new("overlap", OVERLAP_FILES); + + for args in [ + ["helper,helper_extra", "--as", "symbol"], + ["helper_extra,helper", "--as", "symbol"], + ] { + let out = fx.discover(&args); + let long = section(&out, "helper_extra"); + assert!( + long.contains("[fn] helper_extra only_extra.rs:1-3"), + "{args:?}\n{long}" + ); + assert!( + long.contains("source: ast · kind: definition"), + "{args:?}\n{long}" + ); + let short = section(&out, "helper"); + assert!( + short.contains("[fn] helper short.rs:1-3"), + "{args:?}\n{short}" + ); + // A prefix hit must not fabricate a definition inside the longer name. + assert!( + !short.contains("only_extra.rs:1-3\n source: ast"), + "{args:?}\n{short}" + ); + } +} + +#[test] +fn shared_scan_needle_keeps_each_qualifier_and_its_own_definitions() { + let fx = Fixture::new("shared_needle", GO_FILES); + + // Maximum accepted batch size, mixing qualified, plain and absent terms. + let out = fx.discover(&["Batch.Set,Other.Set,Set,helper,Nope.Set", "--as", "symbol"]); + + let batch_set = section(&out, "Batch.Set"); + assert!( + batch_set.contains("1 matches (1 definitions)"), + "{batch_set}" + ); + assert!( + batch_set.contains("[fn] Batch.Set sample.go:5-5"), + "{batch_set}" + ); + + let other_set = section(&out, "Other.Set"); + assert!( + other_set.contains("1 matches (1 definitions)"), + "{other_set}" + ); + assert!( + other_set.contains("[fn] Other.Set sample.go:9-9"), + "{other_set}" + ); + + let plain = section(&out, "Set"); + assert!(plain.contains("2 definitions"), "{plain}"); + + // A wrong qualifier must not acquire a definition just because sibling + // queries admitted the same file. + let absent = section(&out, "Nope.Set"); + assert!(absent.contains("0 matches"), "{absent}"); +} + +#[test] +fn exact_dotted_outline_name_still_wins_inside_a_batch() { + let fx = Fixture::new( + "elixir_dotted", + &[( + "app.ex", + "defmodule Foo.Bar do\n def hello do\n :world\n end\nend\n", + )], + ); + let out = fx.discover(&["Foo.Bar,hello", "--as", "symbol"]); + + let dotted = section(&out, "Foo.Bar"); + assert!(dotted.contains("1 matches (1 definitions)"), "{dotted}"); + assert!( + dotted.contains("[definition] Foo.Bar app.ex:1-5") && !dotted.contains("[fn] Foo.Bar"), + "exact dotted-name match must win inside a batch, not a qualified fn:\n{dotted}" + ); + assert!( + section(&out, "hello").contains("[fn] Foo.Bar.hello app.ex:2-4"), + "{out}" + ); +} diff --git a/tests/windows_paths.rs b/tests/windows_paths.rs index 1c8766b..93fb6ce 100644 --- a/tests/windows_paths.rs +++ b/tests/windows_paths.rs @@ -345,3 +345,88 @@ fn windows_absolute_drive_path_symbol_splits_after_the_file_path() { let _ = fs::remove_dir_all(&dir); } + +/// US-075: a symbol batch must keep single-query definition semantics on +/// Windows path surfaces — drive-letter absolute scope, a space-containing +/// directory, and a relative backslash scope — and the target it emits must +/// replay from the discovery CWD. +#[test] +fn windows_batch_keeps_qualified_and_overlapping_definitions() { + let dir = temp_repo("windows batch prefilter"); + fs::write( + dir.join("sample.go"), + "package sample\n\ntype Batch struct{}\n\nfunc (b *Batch) Set(v int) {}\n\nfunc helper() {}\n", + ) + .unwrap(); + let pkg = dir.join("pkg"); + fs::create_dir_all(&pkg).unwrap(); + fs::write( + pkg.join("overlap.rs"), + "pub fn helper_extra() -> u8 {\n 1\n}\n", + ) + .unwrap(); + + // Drive-letter absolute scope containing a space. + let out = srcwalk() + .args(["discover", "Batch.Set,helper", "--as", "symbol", "--scope"]) + .arg(&dir) + .output() + .unwrap(); + assert!( + out.status.success(), + "absolute batch discover failed:\n{}", + String::from_utf8_lossy(&out.stderr) + ); + let stdout = String::from_utf8_lossy(&out.stdout); + assert!(stdout.contains("[fn] Batch.Set"), "{stdout}"); + assert!(stdout.contains("[fn] helper "), "{stdout}"); + + // Relative backslash scope, run from the repo root. + let out = srcwalk() + .current_dir(&dir) + .args([ + "discover", + "helper,helper_extra", + "--as", + "symbol", + "--scope", + ]) + .arg(r".\pkg") + .output() + .unwrap(); + assert!( + out.status.success(), + "relative backslash batch discover failed:\n{}", + String::from_utf8_lossy(&out.stderr) + ); + let stdout = String::from_utf8_lossy(&out.stdout); + assert!(stdout.contains("[fn] helper_extra"), "{stdout}"); + + // The emitted target replays verbatim from the discovery CWD. + let out = srcwalk() + .current_dir(&dir) + .args(["discover", "Batch.Set,helper", "--as", "symbol"]) + .output() + .unwrap(); + let stdout = String::from_utf8_lossy(&out.stdout); + let target = "sample.go:Batch.Set"; + assert!( + stdout.contains(&format!("> Next: srcwalk show {target}")), + "expected an emitted target for the qualified term:\n{stdout}" + ); + let out = srcwalk() + .current_dir(&dir) + .arg("show") + .arg(target) + .output() + .unwrap(); + assert!( + out.status.success(), + "emitted target replay failed:\n{}", + String::from_utf8_lossy(&out.stderr) + ); + let stdout = String::from_utf8_lossy(&out.stdout); + assert!(stdout.contains("func (b *Batch) Set(v int) {}"), "{stdout}"); + + let _ = fs::remove_dir_all(&dir); +} From 83b5535632f8f70971cae6088075f8637bc34a35 Mon Sep 17 00:00:00 2001 From: sting8k Date: Sun, 6 Sep 2026 15:32:34 +0700 Subject: [PATCH 2/6] test: widen the batch prefilter oracle per US-075 review Adds bounded rows to the existing single-vs-batch definition oracle rather than new fixtures or helpers: - reversed Rust and Python batches, plus a qualified term batched with its own plain name, so the plain name keeps both definitions and the qualified term keeps only its own; - suffix/contained overlap (`extra` inside `helper_extra`) and a three-term overlap chain (`help`, `helper`, `helper_extra`) in both orders, each name owning a file that contains no other query name; - the self-hosted `search`/`search_batch` repro shape that first exposed the masked needle; - a Go generic receiver (`Store[T]`) batched with a plain receiver sharing the same method needle. AC-6 now batches five terms so the shared plain name keeps both file identities while a wrong qualifier and an absent name stay empty. The suffix row passes on the base binary as well: contained needles were already reported, so it guards the semantics rather than reproducing the bug. Every other added row fails on 39c6360. --- src/search/symbol/tests.rs | 134 +++++++++++++++++++++++++++++++++++-- 1 file changed, 127 insertions(+), 7 deletions(-) diff --git a/src/search/symbol/tests.rs b/src/search/symbol/tests.rs index 657f7b3..ad27fc2 100644 --- a/src/search/symbol/tests.rs +++ b/src/search/symbol/tests.rs @@ -607,11 +607,25 @@ fn batch_definitions_match_single_search_for_qualified_and_overlapping_queries() "sample.py", "class Batch:\n def set(self, v):\n return v\n\n\nclass Other:\n def set(self, v):\n return v\n\n\ndef helper():\n return None\n", )]; - // The long name lives in a file that does not contain the short name, so a - // masked needle cannot be rescued by the other query's admission. + // A generic receiver spells the container as `Store[T]`, so the qualified + // term must survive a batch the same way a plain receiver does. + const GO_GENERIC: &[(&str, &str)] = &[( + "generic.go", + "package sample\n\ntype Store[T any] struct{}\n\nfunc (s *Store[T]) Get() {}\n\ntype Plain struct{}\n\nfunc (p Plain) Get() {}\n\nfunc helper() {}\n", + )]; + // Every overlapping name owns a file that contains no other query name, so a + // masked needle can never be rescued by a sibling query's admission. + // `help` is a prefix, `extra` is a suffix, and `helper` is both. const OVERLAP: &[(&str, &str)] = &[ ("only_extra.rs", "pub fn helper_extra() -> u8 {\n 1\n}\n"), ("short.rs", "pub fn helper() -> u8 {\n 2\n}\n"), + ("contained.rs", "pub fn extra() -> u8 {\n 3\n}\n"), + ("chain.rs", "pub fn help() -> u8 {\n 4\n}\n"), + ]; + // The self-hosted repro shape that first exposed the masked needle. + const SEARCH_REPRO: &[(&str, &str)] = &[ + ("single.rs", "pub fn search() -> u8 {\n 1\n}\n"), + ("batch.rs", "pub fn search_batch() -> u8 {\n 2\n}\n"), ]; let cases = [ @@ -643,6 +657,58 @@ fn batch_definitions_match_single_search_for_qualified_and_overlapping_queries() queries: &["Batch.set", "helper"], expected_defs: &[1, 1], }, + Case { + name: "rust_qualified_reversed", + files: RUST, + glob: "*.rs", + queries: &["helper", "Batch.set"], + expected_defs: &[1, 1], + }, + Case { + name: "python_qualified_reversed", + files: PY, + glob: "*.py", + queries: &["helper", "Batch.set"], + expected_defs: &[1, 1], + }, + // The qualified term and its own plain name in one batch: the plain name + // keeps both definitions, the qualified term keeps only its own. + Case { + name: "rust_plain_name_control", + files: RUST, + glob: "*.rs", + queries: &["Batch.set", "set"], + expected_defs: &[1, 2], + }, + Case { + name: "python_plain_name_control", + files: PY, + glob: "*.py", + queries: &["Batch.set", "set"], + expected_defs: &[1, 2], + }, + Case { + name: "go_generic_receiver", + files: GO_GENERIC, + glob: "*.go", + queries: &["Store.Get", "helper"], + expected_defs: &[1, 1], + }, + Case { + name: "go_generic_receiver_reversed", + files: GO_GENERIC, + glob: "*.go", + queries: &["helper", "Store.Get"], + expected_defs: &[1, 1], + }, + // Generic and plain receivers share the `Get` needle in one batch. + Case { + name: "go_generic_and_plain_receiver", + files: GO_GENERIC, + glob: "*.go", + queries: &["Store.Get", "Plain.Get", "Get"], + expected_defs: &[1, 1, 2], + }, Case { name: "prefix_overlap", files: OVERLAP, @@ -657,6 +723,48 @@ fn batch_definitions_match_single_search_for_qualified_and_overlapping_queries() queries: &["helper_extra", "helper"], expected_defs: &[1, 1], }, + Case { + name: "suffix_overlap", + files: OVERLAP, + glob: "*.rs", + queries: &["extra", "helper_extra"], + expected_defs: &[1, 1], + }, + Case { + name: "suffix_overlap_reversed", + files: OVERLAP, + glob: "*.rs", + queries: &["helper_extra", "extra"], + expected_defs: &[1, 1], + }, + Case { + name: "overlap_chain", + files: OVERLAP, + glob: "*.rs", + queries: &["help", "helper", "helper_extra"], + expected_defs: &[1, 1, 1], + }, + Case { + name: "overlap_chain_reversed", + files: OVERLAP, + glob: "*.rs", + queries: &["helper_extra", "helper", "help"], + expected_defs: &[1, 1, 1], + }, + Case { + name: "search_batch_repro", + files: SEARCH_REPRO, + glob: "*.rs", + queries: &["search", "search_batch"], + expected_defs: &[1, 1], + }, + Case { + name: "search_batch_repro_reversed", + files: SEARCH_REPRO, + glob: "*.rs", + queries: &["search_batch", "search"], + expected_defs: &[1, 1], + }, Case { name: "shared_needle", files: GO, @@ -735,9 +843,9 @@ fn batch_definition_anchors_are_independent_of_comment_occurrences() { assert_eq!(without_comment, single_without); } -/// US-075 AC-6: same-name definitions in different files stay separate, and a -/// wrong qualifier never acquires a definition because a sibling query admitted -/// the file. +/// US-075 AC-6: same-name definitions in different files stay separate, the +/// plain name owns both of them, and neither a wrong qualifier nor an absent +/// name acquires a definition because a sibling query admitted the file. #[test] fn batch_shared_needle_keeps_qualifier_ownership_and_file_identity() { let dir = tempfile::tempdir().unwrap(); @@ -754,7 +862,7 @@ fn batch_shared_needle_keeps_qualifier_ownership_and_file_identity() { ) .unwrap(); - let queries = ["Batch.Set", "Other.Set", "Nope.Set"]; + let queries = ["Batch.Set", "Other.Set", "Set", "Nope.Set", "absent_name"]; let batch = search_batch(&queries, dir.path(), None, None, Some("*.go")).unwrap(); let batch_first = definition_keys(&batch[0], dir.path()); @@ -763,10 +871,22 @@ fn batch_shared_needle_keeps_qualifier_ownership_and_file_identity() { assert_eq!(batch_second.len(), 1, "{batch_second:?}"); assert!(batch_first.iter().all(|k| k.0 == "a/sample.go")); assert!(batch_second.iter().all(|k| k.0 == "b/sample.go")); + + // The shared plain name keeps both file identities, not one merged anchor. + let plain: Vec = definition_keys(&batch[2], dir.path()) + .into_iter() + .map(|k| k.0) + .collect(); + assert_eq!(plain, vec!["a/sample.go", "b/sample.go"], "{plain:?}"); + assert!( - definition_keys(&batch[2], dir.path()).is_empty(), + definition_keys(&batch[3], dir.path()).is_empty(), "wrong qualifier must not acquire a definition" ); + assert!( + definition_keys(&batch[4], dir.path()).is_empty(), + "absent plain name must not acquire a definition" + ); for (idx, query) in queries.iter().enumerate() { let single = search(query, dir.path(), None, None, Some("*.go")).unwrap(); From ad9d3340134468837c7086f7923f1fb78022357d Mon Sep 17 00:00:00 2001 From: sting8k Date: Sun, 6 Sep 2026 16:06:50 +0700 Subject: [PATCH 3/6] fix: frame comma-separated target lists around generic selectors srcwalk prints `> Next: srcwalk show ':Cache.get'` and promises the printed command works unchanged, but every target-list consumer split that string at each comma, so the fragment `:Cache` before any target runs. Input without a comma is returned unvalidated, so nothing that parses today can start failing. The four framing sites now share it: show and context target lists, the show rejection-hint normalization, and section/range lists. The artifact JS/TS reader stops refusing every comma-bearing symbol and refuses only a real depth-zero list. Emission is unchanged, including generic parameters and the quoted path plus `--section` form chosen for comma paths; replaying that comma-path command still fails exactly as it does on the base, which is recorded rather than asserted as a working round-trip. Discover and query comma grammar is untouched. --- CHANGELOG.md | 1 + README.md | 7 + skills/srcwalk/GUIDE.md | 2 +- src/artifact.rs | 6 +- src/cli_run.rs | 25 ++- src/format.rs | 119 +++++++++++ src/main.rs | 5 +- src/read/section.rs | 92 ++++++++- tests/us071_selector_round_trip.rs | 30 ++- tests/us076_comma_selector_round_trip.rs | 253 +++++++++++++++++++++++ tests/windows_paths.rs | 90 ++++++++ 11 files changed, 601 insertions(+), 29 deletions(-) create mode 100644 tests/us076_comma_selector_round_trip.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index ec3257a..f5cdc65 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,7 @@ All notable changes to srcwalk are documented here. ## [Unreleased] ### Fixed +- Emitted generic selectors carrying a comma (`show 'src/cache.rs:Cache.get'`) now replay unchanged through `show`, `context`, `trace callers`, `trace callees`, and `--section`. Target lists split only at a comma outside balanced `<...>`, so such a selector stays one target and can be combined with other targets, while an unbalanced `<...>` fails once as an explicit framing error without running any target. Comma-free targets, target lists, and every discover/query comma grammar are unchanged. - Symbol batches (`discover 'a,b' --as symbol`) no longer drop definitions that the same terms find on their own. The batch definition prefilter now derives its scan needle exactly like the single-symbol search, so a receiver/container-qualified term keeps its method definition inside a batch (`'Batch.Set,helper'`), and it matches overlapping needles, so a shorter term no longer masks a longer one (`'helper,helper_extra'`). Qualification is still decided by the existing structural matcher, usage matching is unchanged, and single-symbol output stays byte-for-byte identical. ## [1.8.0] - 2026-08-16 diff --git a/README.md b/README.md index f4a054a..5fc98d6 100644 --- a/README.md +++ b/README.md @@ -128,6 +128,13 @@ srcwalk overview --scope src/ Discovery commands respect ignore files; explicit file reads can still inspect ignored paths. +An exact `:` target that srcwalk prints is reusable verbatim across `show`, `context`, `trace callers`, and `trace callees`. A comma inside a generic selector belongs to that one target, while a comma outside `<...>` separates targets in a list: + +```sh +srcwalk show 'src/cache.rs:Cache.get' # one target +srcwalk show 'src/cache.rs:Cache.get,src/auth.ts:handleAuth' # two targets +``` + Regex-dialect and path-fragment `discover` queries are translated instead of dead-ending (no regex engine runs): - `srcwalk discover 'parseGitUrl\(' --scope src/` de-escapes to literal + symbol search, labeled `interpreted as`. - `srcwalk discover 'a.*b' --scope src/` runs bounded same-line ordered co-occurrence of `a` then `b`. diff --git a/skills/srcwalk/GUIDE.md b/skills/srcwalk/GUIDE.md index 1321588..42bedcd 100644 --- a/skills/srcwalk/GUIDE.md +++ b/skills/srcwalk/GUIDE.md @@ -66,7 +66,7 @@ Use auto overview depth first; explicit `--depth N` is strict. Narrow `overview Intent inference: path-like globs infer file discovery; punctuation/path comma lists infer literal Text OR; symbol globs stay symbol search. Add `--as symbol|file|text|access` when ambiguous. After a first pass, use `--expand=3`, `--filter kind:fn`, or `--exclude 'tests/**'` only when output is too broad. Regex-style queries are translated, not executed as regex: `foo\(` de-escapes to literal + symbol search, `a.*b` runs same-line ordered co-occurrence, `models\.json` behaves like `models.json`, and an unresolved path fragment like `packages/ai` lists matching relative paths (≤20). Each translation is labeled `interpreted as`; zero-match branches print a `> Try:` recovery line. Windows drive paths and `./`/`../` paths are never treated as regex. Text Search and Text OR may add a conditional `> Note:` when a term has at least 400 matches across at least 150 files and reaches at least 1.5% of eligible files; treat it as measured spread, then consider `overview`, a narrower term or scope, or a structural route when that spread was not intentional. -If `discover` prints `## Confirmed structural targets`, run the printed `> Next:` command verbatim. The target is a canonical `:` built from parser outline primitives and proven to resolve to that one body; a numeric `:` command is the safe fallback when ambiguity prevents a unique symbol selector. A numeric range shown beside a stable symbol is evidence metadata (a bounded preview), not the preferred body address. Use `srcwalk context ` only when you need a Flow Map, scoped occurrences, or call neighborhood; do not run `context` for each hop just to read source. Reuse that `:` string unchanged across `show`, `context`, `trace callers`, and `trace callees`, keeping any `--scope` printed with it; never rebuild it from a displayed qualified name, because a namespace/module display prefix (`System.Text.Json.GetTypeInfoInternal`) is not a selector and the emitted form uses the owning container (`JsonSerializerOptions.GetTypeInfoInternal`). The path identifies the requested definition; it does not upgrade caller evidence — `trace callers` stays a direct by-name search and may include same-name definitions elsewhere. Unresolved or ambiguous targets fail explicitly instead of falling back to a bare-name search. +If `discover` prints `## Confirmed structural targets`, run the printed `> Next:` command verbatim. The target is a canonical `:` built from parser outline primitives and proven to resolve to that one body; a numeric `:` command is the safe fallback when ambiguity prevents a unique symbol selector. A numeric range shown beside a stable symbol is evidence metadata (a bounded preview), not the preferred body address. Use `srcwalk context ` only when you need a Flow Map, scoped occurrences, or call neighborhood; do not run `context` for each hop just to read source. Reuse that `:` string unchanged across `show`, `context`, `trace callers`, and `trace callees`, keeping any `--scope` printed with it; never rebuild it from a displayed qualified name, because a namespace/module display prefix (`System.Text.Json.GetTypeInfoInternal`) is not a selector and the emitted form uses the owning container (`JsonSerializerOptions.GetTypeInfoInternal`). The path identifies the requested definition; it does not upgrade caller evidence — `trace callers` stays a direct by-name search and may include same-name definitions elsewhere. A comma inside a generic selector (`Cache.get`) is part of that one target, so copy the quoted command unchanged; only a comma outside `<...>` separates targets in a `show`/`context` list. Unresolved or ambiguous targets fail explicitly instead of falling back to a bare-name search. Symbol discovery separates parser-backed definitions from text-matched name occurrences. Repeated same-name definitions receive an ambiguity caveat. Text discovery remains literal evidence; `--match all` is same-file co-occurrence, not semantic relation proof. diff --git a/src/artifact.rs b/src/artifact.rs index ec3a224..9f212a5 100644 --- a/src/artifact.rs +++ b/src/artifact.rs @@ -50,7 +50,11 @@ pub(crate) fn read_js_ts_symbol_section( symbol: &str, budget: Option, ) -> Option> { - if symbol.starts_with('#') || symbol.contains(',') || parse_line_range(symbol).is_some() { + // This reader resolves one section. A comma nested in a generic selector is + // part of that single symbol, so only a real depth-zero list is out of scope. + let is_section_list = + crate::format::split_target_list(symbol).is_ok_and(|framed| framed.len() > 1); + if symbol.starts_with('#') || is_section_list || parse_line_range(symbol).is_some() { return None; } diff --git a/src/cli_run.rs b/src/cli_run.rs index 8d25d31..52a2250 100644 --- a/src/cli_run.rs +++ b/src/cli_run.rs @@ -865,13 +865,20 @@ fn run_context( filter: Option<&str>, artifact: ArtifactMode, ) -> Result { - if !target.contains(',') { + let raw_targets = srcwalk::format::split_target_list(target).map_err(|reason| { + srcwalk::error::SrcwalkError::InvalidQuery { + query: target.to_string(), + reason: reason.to_string(), + } + })?; + // One framed target covers both a comma-free target and a selector whose + // only commas are nested in generics. + if raw_targets.len() == 1 { return srcwalk::run_flow_with_artifact( target, scope, budget, cache, depth, filter, artifact, ); } - let raw_targets: Vec<&str> = target.split(',').collect(); if raw_targets.iter().any(|part| part.trim().is_empty()) { return Err(srcwalk::error::SrcwalkError::InvalidQuery { query: target.to_string(), @@ -1222,7 +1229,15 @@ fn run_show( context_lines: Option, cache: &srcwalk::cache::OutlineCache, ) -> Result { - if !target.contains(',') { + let framed = srcwalk::format::split_target_list(target).map_err(|reason| { + srcwalk::error::SrcwalkError::InvalidQuery { + query: target.to_string(), + reason: reason.to_string(), + } + })?; + // One framed target covers both a comma-free target and a selector whose + // only commas are nested in generics. + if framed.len() == 1 { return srcwalk::run_path_exact_with_artifact_and_context( target, scope, @@ -1243,8 +1258,8 @@ fn run_show( }); } - let targets: Vec<&str> = target - .split(',') + let targets: Vec<&str> = framed + .into_iter() .map(str::trim) .filter(|part| !part.is_empty()) .collect(); diff --git a/src/format.rs b/src/format.rs index 19d6bcf..26c8949 100644 --- a/src/format.rs +++ b/src/format.rs @@ -195,6 +195,55 @@ fn is_shell_safe_path_char(c: char) -> bool { || cfg!(windows) && c == '\\' } +/// Generic angle brackets in a comma-separated target list are unbalanced, so +/// the list cannot be framed without guessing a repair. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub struct UnbalancedAngleBrackets; + +impl std::fmt::Display for UnbalancedAngleBrackets { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.write_str("unbalanced `<...>` in comma-separated target list") + } +} + +/// Frame a comma-separated list of exact targets. +/// +/// A comma inside balanced generic angle brackets is selector data, not a +/// separator: `cache.rs:Cache.get` is one target, while +/// `cache.rs:Cache.get,a.rs:A.run` is two. Slices are returned exactly as +/// written, so each consumer keeps its own trimming and empty-item policy. +/// +/// Input without a comma is one target and is returned without bracket +/// validation: there is no list to frame, so no input that parses today can +/// start failing here. +pub fn split_target_list(input: &str) -> Result, UnbalancedAngleBrackets> { + if !input.contains(',') { + return Ok(vec![input]); + } + + let mut targets = Vec::new(); + let mut depth: usize = 0; + let mut start = 0; + // ASCII `<`, `>` and `,` never occur inside a multi-byte UTF-8 sequence, so + // byte offsets are safe slice boundaries. + for (offset, byte) in input.bytes().enumerate() { + match byte { + b'<' => depth += 1, + b'>' => depth = depth.checked_sub(1).ok_or(UnbalancedAngleBrackets)?, + b',' if depth == 0 => { + targets.push(&input[start..offset]); + start = offset + 1; + } + _ => {} + } + } + if depth != 0 { + return Err(UnbalancedAngleBrackets); + } + targets.push(&input[start..]); + Ok(targets) +} + /// Split trailing footer guidance from primary output. #[must_use] pub fn split_trailing_footer(output: &str) -> Option<(&str, &str)> { @@ -268,4 +317,74 @@ mod tests { "2 matches (1 definitions, 1 text matches)" ); } + + #[test] + fn split_target_list_keeps_nested_generic_commas_in_one_target() { + assert_eq!( + split_target_list("cache.rs:Cache.get"), + Ok(vec!["cache.rs:Cache.get"]) + ); + assert_eq!( + split_target_list("cache.rs:Outer>.get"), + Ok(vec!["cache.rs:Outer>.get"]) + ); + } + + #[test] + fn split_target_list_splits_only_at_depth_zero_commas() { + assert_eq!( + split_target_list("a.rs:A.run,b.rs:B.run"), + Ok(vec!["a.rs:A.run", "b.rs:B.run"]) + ); + assert_eq!( + split_target_list("cache.rs:Cache.get,a.rs:A.run"), + Ok(vec!["cache.rs:Cache.get", "a.rs:A.run"]) + ); + assert_eq!( + split_target_list("a.rs:A.run,cache.rs:Outer>.get"), + Ok(vec!["a.rs:A.run", "cache.rs:Outer>.get"]) + ); + } + + #[test] + fn split_target_list_preserves_slices_order_and_duplicates() { + // Spacing and empty items survive verbatim; consumers own that policy. + assert_eq!( + split_target_list("a.rs:A.run, a.rs:A.run,,b.rs:B.run "), + Ok(vec!["a.rs:A.run", " a.rs:A.run", "", "b.rs:B.run "]) + ); + // A comma path still frames as separate depth-zero pieces; canonical + // emission keeps using the path + `--section` form for those. + assert_eq!( + split_target_list("a,file.rs:run"), + Ok(vec!["a", "file.rs:run"]) + ); + } + + #[test] + fn split_target_list_rejects_unbalanced_angle_brackets() { + assert_eq!( + split_target_list("cache.rs:Cache.get,a.rs:A.run"), + Err(UnbalancedAngleBrackets) + ); + assert_eq!( + UnbalancedAngleBrackets.to_string(), + "unbalanced `<...>` in comma-separated target list" + ); + } + + #[test] + fn split_target_list_returns_comma_free_input_unvalidated() { + // No comma means no list to frame, so today's inputs cannot start failing. + assert_eq!(split_target_list("a.rs:A.run"), Ok(vec!["a.rs:A.run"])); + assert_eq!( + split_target_list("weird>name.rs:run"), + Ok(vec!["weird>name.rs:run"]) + ); + assert_eq!(split_target_list(""), Ok(vec![""])); + } } diff --git a/src/main.rs b/src/main.rs index 0ca2550..de633d8 100644 --- a/src/main.rs +++ b/src/main.rs @@ -98,7 +98,10 @@ fn normalize_show_target_group(value: &OsStr) -> Option> { let mut normalized = Vec::new(); let mut shorthand_path = None; - for part in value.split(',').map(str::trim) { + // Same target-list framing as `show` itself, so a hint never rewrites a + // generic selector into fragments. + let framed = srcwalk::format::split_target_list(value).ok()?; + for part in framed.into_iter().map(str::trim) { if part.is_empty() { return None; } diff --git a/src/read/section.rs b/src/read/section.rs index b552350..7b9246d 100644 --- a/src/read/section.rs +++ b/src/read/section.rs @@ -203,7 +203,14 @@ pub(super) fn read_section_with_context( let buf = &mmap[..]; // Resolve section address: line range, focused line, heading, symbol name, - // or a comma-separated list of those addresses. + // or a comma-separated list of those addresses. A comma nested in a generic + // selector (`Cache.get`) is part of one address, not a separator. + let section_list = + crate::format::split_target_list(range).map_err(|reason| SrcwalkError::InvalidQuery { + query: range.to_string(), + reason: reason.to_string(), + })?; + let is_section_list = section_list.len() > 1; let mut focus_line = None; let requested_range = parse_requested_range(range); let (start, end) = if range.starts_with('#') { @@ -217,7 +224,7 @@ pub(super) fn read_section_with_context( (start, end) } } - None if range.contains(',') => { + None if is_section_list => { return read_multi_section(path, buf, range, budget, context_lines) } None => { @@ -236,7 +243,7 @@ pub(super) fn read_section_with_context( }); } } - } else if range.contains(',') { + } else if is_section_list { return read_multi_section(path, buf, range, budget, context_lines); } else if let Some(line) = parse_focused_line(range).filter(|_| context_lines.is_some()) { let context = context_lines.expect("checked context_lines above"); @@ -487,8 +494,12 @@ fn read_multi_section( budget: Option, context_lines: Option, ) -> Result { - let requested: Vec<&str> = range - .split(',') + let requested: Vec<&str> = crate::format::split_target_list(range) + .map_err(|reason| SrcwalkError::InvalidQuery { + query: range.to_string(), + reason: reason.to_string(), + })? + .into_iter() .map(str::trim) .filter(|s| !s.is_empty()) .collect(); @@ -1320,3 +1331,74 @@ mod precision_tests { ); } } + +#[cfg(test)] +mod section_list_tests { + use super::*; + use crate::cache::OutlineCache; + + fn fixture(name: &str, files: &[(&str, &str)]) -> std::path::PathBuf { + let dir = std::env::temp_dir().join(format!("us076_section_{name}_{}", std::process::id())); + let _ = std::fs::remove_dir_all(&dir); + std::fs::create_dir_all(&dir).unwrap(); + for (rel, content) in files { + std::fs::write(dir.join(rel), content).unwrap(); + } + dir + } + + const RUST: &str = "pub struct Cache(K, V);\n\nimpl Cache {\n pub fn get(&self) -> u8 {\n 0\n }\n}\n\npub fn plain() -> u8 {\n 1\n}\n"; + + /// US-076: a comma nested in a generic selector addresses one section. + #[test] + fn generic_selector_is_one_section_and_lists_still_split() { + let dir = fixture("generic", &[("cache.rs", RUST)]); + let path = dir.join("cache.rs"); + let cache = OutlineCache::new(); + + let one = read_section(&path, "Cache.get", None, &cache).unwrap(); + assert!(one.contains("pub fn get(&self) -> u8"), "{one}"); + assert!( + !one.contains("pub fn plain"), + "one selector must not read more:\n{one}" + ); + + let mixed = read_section(&path, "Cache.get,plain", None, &cache).unwrap(); + assert!(mixed.contains("pub fn get(&self) -> u8"), "{mixed}"); + assert!(mixed.contains("pub fn plain() -> u8"), "{mixed}"); + + let numeric = read_section(&path, "1-1,9-9", None, &cache).unwrap(); + assert!( + numeric.contains("pub struct Cache(K, V);"), + "{numeric}" + ); + assert!(numeric.contains("pub fn plain() -> u8"), "{numeric}"); + + let err = read_section(&path, "Cache` in comma-separated target list"), + "{err}" + ); + let _ = std::fs::remove_dir_all(&dir); + } + + /// A markdown heading containing a comma still resolves full-heading-first. + #[test] + fn comma_heading_still_resolves_before_list_parsing() { + let dir = fixture( + "heading", + &[("doc.md", "# Alpha\n\ntext\n\n## Beta, Gamma\n\nbody\n")], + ); + let path = dir.join("doc.md"); + let cache = OutlineCache::new(); + + let out = read_section(&path, "## Beta, Gamma", None, &cache).unwrap(); + assert!(out.contains("body"), "{out}"); + assert!( + !out.contains("text"), + "heading must win before list parsing:\n{out}" + ); + let _ = std::fs::remove_dir_all(&dir); + } +} diff --git a/tests/us071_selector_round_trip.rs b/tests/us071_selector_round_trip.rs index 3c0ec1e..9a849a7 100644 --- a/tests/us071_selector_round_trip.rs +++ b/tests/us071_selector_round_trip.rs @@ -468,14 +468,12 @@ fn multi_symbol_discover_round_trips_each_section_target() { } /// US-075: a batch section whose definition is restored by the shared prefilter -/// must emit the same copyable selector as a single query, and a restored -/// qualified target must still round-trip. +/// must emit the same copyable selector as a single query. /// -/// A generic container whose selector carries a comma (`Cache.get`) is -/// emitted quoted here but does not replay; that defect predates this story and -/// reproduces identically on a single-symbol query, so it is not asserted. +/// US-076: every emitted target replays, including a generic container whose +/// selector carries a comma (`Cache.get`). #[test] -fn batch_sections_emit_quoted_generic_and_round_trip_qualified_targets() { +fn batch_sections_emit_quoted_generic_and_round_trip_all_targets() { let fx = Fixture::new( "batch_generic", &[ @@ -519,14 +517,14 @@ fn batch_sections_emit_quoted_generic_and_round_trip_qualified_targets() { "expected the qualified selector among {targets:?}:\n{discovered}" ); - // The restored qualified target is copied back verbatim and reads its body. - let (qualified, flags) = emitted - .iter() - .find(|(t, _)| t.ends_with(":Alpha.run")) - .unwrap(); - let shown = fx.ok(&with_flags(&["show"], qualified, flags)); - assert!( - shown.contains("pub fn run(&self) {}"), - "show {qualified}:\n{shown}" - ); + // Every emitted target is copied back verbatim and reads its own body. + for (target, flags) in &emitted { + let shown = fx.ok(&with_flags(&["show"], target, flags)); + let expected = if target.ends_with(":Cache.get") { + "pub fn get(&self) -> u8" + } else { + "pub fn run(&self) {}" + }; + assert!(shown.contains(expected), "show {target}:\n{shown}"); + } } diff --git a/tests/us076_comma_selector_round_trip.rs b/tests/us076_comma_selector_round_trip.rs new file mode 100644 index 0000000..dd2ec67 --- /dev/null +++ b/tests/us076_comma_selector_round_trip.rs @@ -0,0 +1,253 @@ +//! US-076: a comma inside an emitted generic selector is selector data, not a +//! target-list separator. +//! +//! srcwalk prints `> Next: srcwalk show ':Cache.get'` and promises +//! the printed command works unchanged. Target-list consumers used to split that +//! string at every comma, so the fragment `:Cache Command { + Command::new(env!("CARGO_BIN_EXE_srcwalk")) +} + +struct Fixture { + dir: PathBuf, +} + +impl Fixture { + fn new(name: &str, files: &[(&str, &str)]) -> Self { + let dir = std::env::temp_dir().join(format!( + "us076_{name}_{}_{}", + std::process::id(), + std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .map_or(0, |d| d.as_nanos()) + )); + fs::create_dir_all(&dir).unwrap(); + for (rel, content) in files { + let path = dir.join(rel); + if let Some(parent) = path.parent() { + fs::create_dir_all(parent).unwrap(); + } + fs::write(path, content).unwrap(); + } + Self { dir } + } + + fn run(&self, args: &[&str]) -> (bool, String, String) { + let out = srcwalk() + .current_dir(&self.dir) + .args(args) + .output() + .unwrap(); + ( + out.status.success(), + String::from_utf8_lossy(&out.stdout).into_owned(), + String::from_utf8_lossy(&out.stderr).into_owned(), + ) + } + + fn ok(&self, args: &[&str]) -> String { + let (success, stdout, stderr) = self.run(args); + assert!(success, "{args:?} failed:\n{stderr}"); + stdout + } +} + +impl Drop for Fixture { + fn drop(&mut self) { + let _ = fs::remove_dir_all(&self.dir); + } +} + +/// The exact target of the first emitted `> Next: srcwalk ` line, +/// unquoted the way a shell would hand it to the process. +fn emitted_target(output: &str, command: &str) -> String { + let needle = format!("> Next: srcwalk {command} "); + let line = output + .lines() + .find(|line| line.starts_with(&needle)) + .unwrap_or_else(|| panic!("no emitted `{command}` target in:\n{output}")); + let rest = line[needle.len()..].trim(); + let target = rest.strip_prefix('\'').map_or_else( + || rest.split_whitespace().next().unwrap_or(rest).to_string(), + |unquoted| { + unquoted + .split_once('\'') + .map_or_else(|| unquoted.to_string(), |(target, _)| target.to_string()) + }, + ); + target +} + +/// Two `get` definitions in one file: without its generic container the selector +/// is ambiguous, so the comma-bearing selector is the only way to address the +/// first body. +const RUST_FILES: &[(&str, &str)] = &[ + ( + "src/cache.rs", + "pub struct Cache(K, V);\n\nimpl Cache {\n pub fn get(&self) -> u8 {\n 0\n }\n}\n\npub struct Solo(K);\n\nimpl Solo {\n pub fn get(&self) -> u8 {\n 1\n }\n}\n", + ), + ( + "src/alpha.rs", + "pub struct Alpha;\n\nimpl Alpha {\n pub fn run(&self) {}\n}\n", + ), +]; + +#[test] +fn emitted_generic_selector_replays_through_show_and_context() { + let fx = Fixture::new("replay", RUST_FILES); + let discovered = fx.ok(&["discover", "get", "--as", "symbol", "--scope", "src"]); + let target = emitted_target(&discovered, "show"); + assert_eq!(target, "src/cache.rs:Cache.get", "{discovered}"); + + let shown = fx.ok(&["show", &target]); + assert!(shown.contains("pub fn get(&self) -> u8"), "{shown}"); + assert!( + shown.contains("within fn get 4-6") && !shown.contains("12-14"), + "show must select the generic container's body, not the sibling:\n{shown}" + ); + + let context = fx.ok(&["context", &target]); + assert!( + context.contains("# Context Packet: src/cache.rs:Cache.get"), + "context must resolve the exact target, not fall back to the file:\n{context}" + ); + + // Without the generic container the same name is ambiguous, so the comma is + // load-bearing rather than decorative. + let (success, _, stderr) = fx.run(&["show", "src/cache.rs:get"]); + assert!(!success && stderr.contains("2 definitions"), "{stderr}"); +} + +#[test] +fn emitted_generic_selector_roots_trace_callers_and_callees() { + // Trace already accepted one comma-bearing target; this guards that the + // shared framing did not start splitting a trace root. + let fx = Fixture::new("trace", RUST_FILES); + let target = "src/cache.rs:Cache.get"; + + let callers = fx.ok(&["trace", "callers", target]); + assert!(callers.contains(&format!("\"{target}\"")), "{callers}"); + + let callees = fx.ok(&["trace", "callees", target]); + assert!(callees.contains(target), "{callees}"); + + for out in [&callers, &callees] { + // Every mention of the container is part of the intact target, so no + // `Cache(A, B);\n\npub struct Outer(K, V);\n\nimpl Outer> {\n pub fn deep(&self) -> u8 {\n 7\n }\n}\n", + ), + ( + "src/alpha.rs", + "pub struct Alpha;\n\nimpl Alpha {\n pub fn run(&self) {}\n}\n", + ), + ], + ); + + let discovered = fx.ok(&["discover", "deep", "--as", "symbol", "--scope", "src"]); + let target = emitted_target(&discovered, "show"); + assert_eq!( + target, "src/nested.rs:Outer>.deep", + "{discovered}" + ); + + let shown = fx.ok(&["show", &target]); + assert!(shown.contains("pub fn deep(&self) -> u8"), "{shown}"); + + let list = format!("{target},src/alpha.rs:Alpha.run"); + let shown = fx.ok(&["show", &list]); + assert!(shown.contains("# Show: 2 locations"), "{shown}"); + assert!(shown.contains(&format!("## Target: {target}")), "{shown}"); +} + +#[test] +fn unbalanced_angle_brackets_fail_once_without_running_any_target() { + let fx = Fixture::new("malformed", RUST_FILES); + // The first item is a valid target, so a partial run would be visible. + let malformed = "src/alpha.rs:Alpha.run,src/cache.rs:Cache` in comma-separated target list"), + "{command} stderr:\n{stderr}" + ); + assert!( + !stdout.contains("pub fn run(&self) {}") && !stdout.contains("## Target:"), + "{command} must not execute the valid prefix target:\n{stdout}" + ); + assert_eq!( + stderr.matches("unbalanced").count(), + 1, + "one framing error only:\n{stderr}" + ); + } +} + +#[test] +fn comma_path_keeps_section_emission_and_its_pre_existing_replay_failure() { + let fx = Fixture::new( + "comma_path", + &[("od,d/f.rs", "pub fn commapath() -> u8 {\n 9\n}\n")], + ); + + // Emission is unchanged: a comma in the *path* still routes to the quoted + // path + `--section` form rather than an inline `path:selector` target. + let discovered = fx.ok(&["discover", "commapath", "--as", "symbol"]); + assert!( + discovered.contains("> Next: srcwalk show 'od,d/f.rs' --section commapath"), + "{discovered}" + ); + + // Replaying that emitted command fails, identically on the base and on this + // change. US-076 does not add path escaping, so the failure is recorded + // rather than asserted as a working round-trip. + let (success, _, stderr) = fx.run(&["show", "od,d/f.rs", "--section", "commapath"]); + assert!( + !success && stderr.contains("--section applies to one show target"), + "pre-existing comma-path replay failure changed shape:\n{stderr}" + ); +} diff --git a/tests/windows_paths.rs b/tests/windows_paths.rs index 93fb6ce..19d616d 100644 --- a/tests/windows_paths.rs +++ b/tests/windows_paths.rs @@ -430,3 +430,93 @@ fn windows_batch_keeps_qualified_and_overlapping_definitions() { let _ = fs::remove_dir_all(&dir); } + +/// US-076: an emitted comma-generic selector must replay on Windows path +/// surfaces — drive-letter absolute scope, a space-containing directory, and a +/// relative backslash scope — and combine with a second target. +#[test] +fn windows_comma_generic_selector_replays_and_combines() { + let dir = temp_repo("windows comma selector"); + let pkg = dir.join("pkg"); + fs::create_dir_all(&pkg).unwrap(); + fs::write( + pkg.join("cache.rs"), + "pub struct Cache(K, V);\n\nimpl Cache {\n pub fn get(&self) -> u8 {\n 0\n }\n}\n", + ) + .unwrap(); + fs::write( + pkg.join("alpha.rs"), + "pub struct Alpha;\n\nimpl Alpha {\n pub fn run(&self) {}\n}\n", + ) + .unwrap(); + + // Drive-letter absolute scope containing a space. + let out = srcwalk() + .args(["discover", "get", "--as", "symbol", "--scope"]) + .arg(&pkg) + .output() + .unwrap(); + assert!( + out.status.success(), + "absolute discover failed:\n{}", + String::from_utf8_lossy(&out.stderr) + ); + let stdout = String::from_utf8_lossy(&out.stdout); + assert!(stdout.contains("Cache.get"), "{stdout}"); + + // Relative backslash scope, replayed from the repo root. + let out = srcwalk() + .current_dir(&dir) + .args(["discover", "get", "--as", "symbol", "--scope"]) + .arg(r".\pkg") + .output() + .unwrap(); + assert!( + out.status.success(), + "relative backslash discover failed:\n{}", + String::from_utf8_lossy(&out.stderr) + ); + let stdout = String::from_utf8_lossy(&out.stdout); + let target = stdout + .lines() + .find_map(|line| line.strip_prefix("> Next: srcwalk show ")) + .map(|rest| rest.trim().trim_matches('\'')) + .unwrap_or_else(|| panic!("no emitted target:\n{stdout}")) + .to_string(); + assert!(target.ends_with("Cache.get"), "{target}"); + + // The emitted target replays verbatim. + let out = srcwalk() + .current_dir(&dir) + .arg("show") + .arg(&target) + .output() + .unwrap(); + assert!( + out.status.success(), + "emitted comma-generic replay failed:\n{}", + String::from_utf8_lossy(&out.stderr) + ); + let stdout = String::from_utf8_lossy(&out.stdout); + assert!(stdout.contains("pub fn get(&self) -> u8"), "{stdout}"); + + // It combines with a second target in one list. + let second = target.replace("cache.rs", "alpha.rs"); + let second = second.replace("Cache.get", "Alpha.run"); + let out = srcwalk() + .current_dir(&dir) + .arg("show") + .arg(format!("{target},{second}")) + .output() + .unwrap(); + assert!( + out.status.success(), + "multi-target replay failed:\n{}", + String::from_utf8_lossy(&out.stderr) + ); + let stdout = String::from_utf8_lossy(&out.stdout); + assert!(stdout.contains("# Show: 2 locations"), "{stdout}"); + assert!(stdout.contains("pub fn run(&self) {}"), "{stdout}"); + + let _ = fs::remove_dir_all(&dir); +} From 714fd0a36c734906130ff94274e71bdae07b5cb9 Mon Sep 17 00:00:00 2001 From: sting8k Date: Sun, 6 Sep 2026 16:22:17 +0700 Subject: [PATCH 4/6] fix: resolve a Markdown heading before framing a section list Review finding: eager target-list framing ran before full-heading resolution, so an existing heading carrying both a comma and a literal `<` or `>` started failing as an unbalanced list (`show doc.md --section '## Compare A < B, C'` resolved on the base and exited 3 here). Framing is now lazy for the heading route: the full heading is matched first, exactly as before, and the list is framed only when no heading matches. Non-heading section input stays validated up front, so a malformed list still fails before any target runs. Also narrows the changelog wording: only a comma-separated target list is rejected for unbalanced angles; comma-free input is deliberately handed to the resolver unchanged for byte parity. --- CHANGELOG.md | 2 +- src/read/section.rs | 91 +++++++++++++++++++++++++-------------------- 2 files changed, 52 insertions(+), 41 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f5cdc65..103b618 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,7 +5,7 @@ All notable changes to srcwalk are documented here. ## [Unreleased] ### Fixed -- Emitted generic selectors carrying a comma (`show 'src/cache.rs:Cache.get'`) now replay unchanged through `show`, `context`, `trace callers`, `trace callees`, and `--section`. Target lists split only at a comma outside balanced `<...>`, so such a selector stays one target and can be combined with other targets, while an unbalanced `<...>` fails once as an explicit framing error without running any target. Comma-free targets, target lists, and every discover/query comma grammar are unchanged. +- Emitted generic selectors carrying a comma (`show 'src/cache.rs:Cache.get'`) now replay unchanged through `show`, `context`, `trace callers`, `trace callees`, and `--section`. Target lists split only at a comma outside balanced `<...>`, so such a selector stays one target and can be combined with other targets, while an unbalanced comma-separated target list fails once as an explicit framing error without running any target. Comma-free targets, target lists, and every discover/query comma grammar are unchanged. - Symbol batches (`discover 'a,b' --as symbol`) no longer drop definitions that the same terms find on their own. The batch definition prefilter now derives its scan needle exactly like the single-symbol search, so a receiver/container-qualified term keeps its method definition inside a batch (`'Batch.Set,helper'`), and it matches overlapping needles, so a shorter term no longer masks a longer one (`'helper,helper_extra'`). Qualification is still decided by the existing structural matcher, usage matching is unchanged, and single-symbol output stays byte-for-byte identical. ## [1.8.0] - 2026-08-16 diff --git a/src/read/section.rs b/src/read/section.rs index 7b9246d..0c40b75 100644 --- a/src/read/section.rs +++ b/src/read/section.rs @@ -205,45 +205,45 @@ pub(super) fn read_section_with_context( // Resolve section address: line range, focused line, heading, symbol name, // or a comma-separated list of those addresses. A comma nested in a generic // selector (`Cache.get`) is part of one address, not a separator. - let section_list = - crate::format::split_target_list(range).map_err(|reason| SrcwalkError::InvalidQuery { - query: range.to_string(), - reason: reason.to_string(), - })?; - let is_section_list = section_list.len() > 1; + // Framing is deliberately lazy: a Markdown heading is matched in full first, + // so a heading may carry both a comma and a literal `<` or `>`. + let is_section_list = |range: &str| -> Result { + crate::format::split_target_list(range) + .map(|framed| framed.len() > 1) + .map_err(|reason| SrcwalkError::InvalidQuery { + query: range.to_string(), + reason: reason.to_string(), + }) + }; let mut focus_line = None; let requested_range = parse_requested_range(range); let (start, end) = if range.starts_with('#') { // Markdown heading. Try the full heading first so headings containing // commas still work; if that fails, fall through to comma-list parsing. - match resolve_heading(buf, range) { - Some((start, end)) => { - if let Some(context) = context_lines { - expand_range(start, end, context) - } else { - (start, end) - } - } - None if is_section_list => { - return read_multi_section(path, buf, range, budget, context_lines) - } - None => { - let suggestions = suggest_headings(buf, range, 5); - let reason = if suggestions.is_empty() { - "heading not found in file".to_string() - } else { - format!( - "heading not found in file. Closest matches:\n {}", - suggestions.join("\n ") - ) - }; - return Err(SrcwalkError::InvalidQuery { - query: range.to_string(), - reason, - }); + let Some((start, end)) = resolve_heading(buf, range) else { + if is_section_list(range)? { + return read_multi_section(path, buf, range, budget, context_lines); } + let suggestions = suggest_headings(buf, range, 5); + let reason = if suggestions.is_empty() { + "heading not found in file".to_string() + } else { + format!( + "heading not found in file. Closest matches:\n {}", + suggestions.join("\n ") + ) + }; + return Err(SrcwalkError::InvalidQuery { + query: range.to_string(), + reason, + }); + }; + if let Some(context) = context_lines { + expand_range(start, end, context) + } else { + (start, end) } - } else if is_section_list { + } else if is_section_list(range)? { return read_multi_section(path, buf, range, budget, context_lines); } else if let Some(line) = parse_focused_line(range).filter(|_| context_lines.is_some()) { let context = context_lines.expect("checked context_lines above"); @@ -1383,22 +1383,33 @@ mod section_list_tests { let _ = std::fs::remove_dir_all(&dir); } - /// A markdown heading containing a comma still resolves full-heading-first. + /// A markdown heading is matched in full before any list framing, so a + /// heading may carry a comma together with a literal `<` or `>` that would + /// never balance as a generic. #[test] fn comma_heading_still_resolves_before_list_parsing() { let dir = fixture( "heading", - &[("doc.md", "# Alpha\n\ntext\n\n## Beta, Gamma\n\nbody\n")], + &[( + "doc.md", + "# Alpha\n\ntext\n\n## Beta, Gamma\n\nbody\n\n## Compare A < B, C\n\nleft\n\n## Compare X > Y, Z\n\nright\n", + )], ); let path = dir.join("doc.md"); let cache = OutlineCache::new(); - let out = read_section(&path, "## Beta, Gamma", None, &cache).unwrap(); - assert!(out.contains("body"), "{out}"); - assert!( - !out.contains("text"), - "heading must win before list parsing:\n{out}" - ); + for (heading, expected) in [ + ("## Beta, Gamma", "body"), + ("## Compare A < B, C", "left"), + ("## Compare X > Y, Z", "right"), + ] { + let out = read_section(&path, heading, None, &cache).unwrap(); + assert!(out.contains(expected), "{heading}:\n{out}"); + assert!( + !out.contains("text"), + "heading must win before list framing: {heading}\n{out}" + ); + } let _ = std::fs::remove_dir_all(&dir); } } From 4cc0825caf2b9b7ffb5c411d2379993addcf0115 Mon Sep 17 00:00:00 2001 From: sting8k Date: Sun, 6 Sep 2026 16:39:38 +0700 Subject: [PATCH 5/6] test: cover the artifact section boundary and other-language replay Review found two evidence gaps, no production behavior finding. The artifact reader's changed guard had no direct test. Its condition moves into a named private classifier so the boundary can be asserted without a JS/TS outline that emits generic arguments: a nested-comma selector reaches symbol lookup, while a real depth-zero list, a heading and a line range stay out of scope. Malformed input stays unclassified there on purpose, because the shared section framing rejects it before any body is read. The emitted-target contract also lacked language controls, so a parameterized CLI test drives TypeScript, Java and C# generic containers. Those outlines currently emit a non-generic selector, so the test asserts the actual emitted string and replays it, rather than assuming a generic spelling. --- src/artifact.rs | 50 +++++++++++++++++++++--- tests/us076_comma_selector_round_trip.rs | 44 +++++++++++++++++++++ 2 files changed, 89 insertions(+), 5 deletions(-) diff --git a/src/artifact.rs b/src/artifact.rs index 9f212a5..c175a7a 100644 --- a/src/artifact.rs +++ b/src/artifact.rs @@ -45,16 +45,24 @@ pub(crate) fn should_auto_artifact_file(path: &Path) -> bool { crate::search::io::looks_minified(&bytes) } +/// Section addresses this single-section reader cannot serve: a heading, a line +/// range, or a real target list. +/// +/// A comma nested in a generic selector (`TsCache.get`) belongs to one +/// symbol, so only a depth-zero comma marks a list. Malformed input is left to +/// the shared section framing, which rejects it before any body is read. +fn is_unsupported_section_address(symbol: &str) -> bool { + symbol.starts_with('#') + || crate::format::split_target_list(symbol).is_ok_and(|framed| framed.len() > 1) + || parse_line_range(symbol).is_some() +} + pub(crate) fn read_js_ts_symbol_section( path: &Path, symbol: &str, budget: Option, ) -> Option> { - // This reader resolves one section. A comma nested in a generic selector is - // part of that single symbol, so only a real depth-zero list is out of scope. - let is_section_list = - crate::format::split_target_list(symbol).is_ok_and(|framed| framed.len() > 1); - if symbol.starts_with('#') || is_section_list || parse_line_range(symbol).is_some() { + if is_unsupported_section_address(symbol) { return None; } @@ -595,3 +603,35 @@ fn clean_export_name(text: &str) -> Option { } Some(name) } + +#[cfg(test)] +mod section_address_tests { + use super::is_unsupported_section_address; + + /// US-076: the single-section artifact reader must classify a nested-comma + /// selector as one symbol and keep refusing a real target list. + #[test] + fn nested_generic_comma_is_one_symbol_but_a_target_list_is_refused() { + for one_symbol in [ + "TsCache.get", + "Outer>.deep", + "TsCache.get", + ] { + assert!( + !is_unsupported_section_address(one_symbol), + "{one_symbol} must reach symbol lookup" + ); + } + + for unsupported in ["a,b", "TsCache.get,other", "#heading", "10-20"] { + assert!( + is_unsupported_section_address(unsupported), + "{unsupported} must not reach symbol lookup" + ); + } + + // Malformed input is deliberately not classified here: the shared + // section framing rejects it before any body is read. + assert!(!is_unsupported_section_address("TsCache {\n get(key: K): V | undefined {\n return undefined;\n }\n}\n", + ), + ( + "src/Cache.java", + "public class JavaCache {\n public V get(K key) {\n return null;\n }\n}\n", + ), + ( + "src/Cache.cs", + "public class CsCache {\n public V Get(K key) {\n return default(V);\n }\n}\n", + ), + ], + ); + + for (query, expected_target, body) in [ + ( + "get", + "src/cache.ts:TsCache.get", + "get(key: K): V | undefined", + ), + ("get", "src/Cache.java:JavaCache.get", "public V get(K key)"), + ("Get", "src/Cache.cs:CsCache.Get", "public V Get(K key)"), + ] { + let discovered = fx.ok(&["discover", query, "--as", "symbol", "--scope", "src"]); + assert!( + discovered.contains(&format!("> Next: srcwalk show {expected_target}")), + "expected `{expected_target}` in:\n{discovered}" + ); + + let shown = fx.ok(&["show", expected_target]); + assert!(shown.contains(body), "show {expected_target}:\n{shown}"); + } +} From 48693f4fd8086d1597c77720eca48f61990669a6 Mon Sep 17 00:00:00 2001 From: sting8k Date: Sun, 6 Sep 2026 17:00:18 +0700 Subject: [PATCH 6/6] release: 1.8.1 --- CHANGELOG.md | 2 +- Cargo.lock | 2 +- Cargo.toml | 2 +- npm/package.json | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 103b618..ea3142c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,7 +2,7 @@ All notable changes to srcwalk are documented here. -## [Unreleased] +## [1.8.1] - 2026-09-06 ### Fixed - Emitted generic selectors carrying a comma (`show 'src/cache.rs:Cache.get'`) now replay unchanged through `show`, `context`, `trace callers`, `trace callees`, and `--section`. Target lists split only at a comma outside balanced `<...>`, so such a selector stays one target and can be combined with other targets, while an unbalanced comma-separated target list fails once as an explicit framing error without running any target. Comma-free targets, target lists, and every discover/query comma grammar are unchanged. diff --git a/Cargo.lock b/Cargo.lock index 8eb8113..8794d6d 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -737,7 +737,7 @@ checksum = "67b1b7a3b5fe4f1376887184045fcf45c69e92af734b7aaddc05fb777b6fbd03" [[package]] name = "srcwalk" -version = "1.8.0" +version = "1.8.1" dependencies = [ "aho-corasick", "clap", diff --git a/Cargo.toml b/Cargo.toml index 6cb3024..7388b3e 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "srcwalk" -version = "1.8.0" +version = "1.8.1" edition = "2021" description = "Tree-sitter indexed lookups — smart code reading for AI agents" license = "MIT" diff --git a/npm/package.json b/npm/package.json index 949c79c..2702b4c 100644 --- a/npm/package.json +++ b/npm/package.json @@ -1,6 +1,6 @@ { "name": "srcwalk", - "version": "1.8.0", + "version": "1.8.1", "description": "Code-intelligence CLI for AI agents — tree-sitter outlines, symbol search, caller/callee graphs, deps, overview", "bin": { "srcwalk": "run.js"