From a1466876175f1fb1e080b0100f48c420ec3a44f1 Mon Sep 17 00:00:00 2001 From: sunerpy Date: Fri, 31 Jul 2026 02:39:07 +0800 Subject: [PATCH 01/18] feat(resolve): name the .tres marker node from script_class A custom resource class was not a queryable symbol: parse_tres named the resource marker node from the [gd_resource type="..."] engine type, so a .tres declaring script_class="Buff" surfaced as "Resource" and a query for "Buff" missed it. Read script_class after type, each behind a non-empty guard, so a declared class wins regardless of header attribute order while the fallback chain still degrades to type and then to the default "resource" name. Golden-neutral: no fixture or golden corpus contains a .tres file. --- .../src/frameworks/godot_resource.rs | 9 +++- .../codegraph-resolve/tests/godot_resource.rs | 50 +++++++++++++++++++ docs/godot.md | 19 +++---- 3 files changed, 67 insertions(+), 11 deletions(-) diff --git a/crates/codegraph-resolve/src/frameworks/godot_resource.rs b/crates/codegraph-resolve/src/frameworks/godot_resource.rs index 870e960..6438cab 100644 --- a/crates/codegraph-resolve/src/frameworks/godot_resource.rs +++ b/crates/codegraph-resolve/src/frameworks/godot_resource.rs @@ -120,8 +120,8 @@ pub(crate) fn parse_tres( // so a `key = ExtResource("id")` line resolves inline (same as T4). let mut ext_resources: HashMap = HashMap::new(); - // The resource's own type, from `[gd_resource type="..."]`, used to name the - // marker node. Defaults if absent or unnamed. + // The resource's declared class (falling back to its engine type), used to + // name the marker node. Defaults if both are absent or unnamed. let mut resource_type = DEFAULT_RESOURCE_NAME.to_string(); // The marker node, created lazily on the first reference to anchor. Once // created it is reused for every subsequent reference. @@ -147,6 +147,11 @@ pub(crate) fn parse_tres( resource_type = ty.to_string(); } } + if let Some(script_class) = attr(&header.attrs, "script_class") { + if !script_class.is_empty() { + resource_type = script_class.to_string(); + } + } } "ext_resource" => record_ext_resource(&header.attrs, &mut ext_resources), "resource" => in_resource = true, diff --git a/crates/codegraph-resolve/tests/godot_resource.rs b/crates/codegraph-resolve/tests/godot_resource.rs index bd3a366..63e007d 100644 --- a/crates/codegraph-resolve/tests/godot_resource.rs +++ b/crates/codegraph-resolve/tests/godot_resource.rs @@ -61,12 +61,62 @@ duration = 5.0 .first() .expect("a resource marker node must exist"); assert_eq!(marker.kind, NodeKind::Constant, "resource-marker kind"); + assert_eq!(marker.name, "Buff", "script_class must name the marker"); assert_eq!( script_ref.from_node_id, marker.id, "script ref must originate from the resource marker node" ); } +#[test] +fn resource_script_class_before_type_still_names_marker() { + // Given a `.tres` whose header lists script_class before type and a reference + // that causes its marker node to be created, + let content = "\ +[gd_resource script_class=\"Buff\" type=\"Resource\" load_steps=2 format=3] + +[ext_resource type=\"Script\" path=\"res://buff.gd\" id=\"1\"] + +[resource] +script = ExtResource(\"1\") +"; + // When extracting, + let result = extract("data/reverse_order_buff.tres", content); + + // Then script_class still wins regardless of its position in the header. + let marker = result + .nodes + .first() + .expect("a resource marker node must exist"); + assert_eq!(marker.name, "Buff", "script_class must name the marker"); +} + +#[test] +fn resource_without_script_class_uses_resource_type_for_marker_name() { + // Given a `.tres` with a resource type but no script_class and a reference + // that causes its marker node to be created, + let content = "\ +[gd_resource type=\"StatusEffect\" load_steps=2 format=3] + +[ext_resource type=\"Script\" path=\"res://status_effect.gd\" id=\"1\"] + +[resource] +script = ExtResource(\"1\") +"; + // When extracting, + let result = extract("data/status_effect.tres", content); + + // Then the existing type-derived fallback still names the marker. + let marker = result + .nodes + .first() + .expect("a resource marker node must exist"); + assert_eq!( + marker.name, "StatusEffect", + "type must name the marker when script_class is absent" + ); +} + #[test] fn resource_property_ext_resource_emits_resource_reference() { // Given a `.tres` whose [resource] property points at another resource via diff --git a/docs/godot.md b/docs/godot.md index f5d5aed..15d2800 100644 --- a/docs/godot.md +++ b/docs/godot.md @@ -68,15 +68,16 @@ see [Honesty signals](#honesty-signals) below). ### `.tres` — text resources -Each `.tres` file emits a single resource marker node (typed from the -`[gd_resource type="..."]` header) and `References` edges for every -`ExtResource(…)` it references: - -| Extracted item | Graph representation | -| ------------------------------- | ----------------------------------------------------------- | -| `[gd_resource type="T"]` | One `Constant` marker node named after the resource type | -| `script = ExtResource(…)` | `References` edge: marker → repo-relative `.gd` path | -| Any property `= ExtResource(…)` | `References` edge: marker → referenced `.tres`/`.tscn` path | +Each `.tres` file with an external reference emits a single resource marker node, +named from a non-empty `script_class` first, then `type`, and finally the default +`"resource"` (regardless of header attribute order), plus `References` edges for +every `ExtResource(…)` it references: + +| Extracted item | Graph representation | +| ----------------------------------------- | --------------------------------------------------------------------- | +| `[gd_resource script_class="C" type="T"]` | One `Constant` marker named `C`; falls back to `T`, then `"resource"` | +| `script = ExtResource(…)` | `References` edge: marker → repo-relative `.gd` path | +| Any property `= ExtResource(…)` | `References` edge: marker → referenced `.tres`/`.tscn` path | A self-contained `.tres` with no external references produces no extra nodes and no edges — just the file node from ingestion. From 9ab08301358aba17f1fa128e353ffb7741b83686 Mon Sep 17 00:00:00 2001 From: sunerpy Date: Fri, 31 Jul 2026 02:39:07 +0800 Subject: [PATCH 02/18] feat(graph): rank an exact whole-name match above every non-exact row An exact-name candidate was injected with score = max_fts_score, only TYING the best FTS hit, and the shared bonus pass then let a non-exact row overtake it on kind_bonus plus path relevance. Observed on a two-node corpus: the exact symbol ranked 2 at 65.0000020754717 against 65.00000209923664. Add exact_name_bonus, whose value is derived from the measured ceilings it must beat (80 name_match_bonus + 10 kind_bonus + 15 path relevance per query word, plus a strict-dominance margin), and lift an exact row only when it sits at or below the best non-exact score. Rows that already win keep their score, which leaves the ordered search goldens byte-stable. --- crates/codegraph-graph/src/query/mod.rs | 20 +++++++ crates/codegraph-graph/src/query/scoring.rs | 32 ++++++++++ crates/codegraph-graph/tests/search.rs | 66 +++++++++++++++++++++ 3 files changed, 118 insertions(+) diff --git a/crates/codegraph-graph/src/query/mod.rs b/crates/codegraph-graph/src/query/mod.rs index 7d678f7..83eb7ab 100644 --- a/crates/codegraph-graph/src/query/mod.rs +++ b/crates/codegraph-graph/src/query/mod.rs @@ -116,6 +116,26 @@ pub fn search_nodes( ) + scoring::name_match_bonus(&result.node.name, scoring_query); } + + // Preserve the established score ABI when an exact match already wins, but + // rescue any exact candidate that FTS (or exact-name injection) left below a + // non-exact row. The shared pass is intentionally source-independent, so it + // also covers candidates appended by multi-segment definer seeding. + let best_non_exact_score = results + .iter() + .filter(|result| scoring::exact_name_bonus(&result.node.name, scoring_query) == 0.0) + .map(|result| result.score) + .reduce(f64::max); + if let Some(best_non_exact_score) = best_non_exact_score { + for result in &mut results { + let exact_bonus = scoring::exact_name_bonus(&result.node.name, scoring_query); + if exact_bonus > 0.0 && result.score <= best_non_exact_score { + // Cover any FTS base-score gap first; the arithmetic-derived bonus + // then puts the exact row decisively above the best non-exact row. + result.score += best_non_exact_score - result.score + exact_bonus; + } + } + } sort_by_score_desc(&mut results); if results.len() > limit as usize { results.truncate(limit as usize); diff --git a/crates/codegraph-graph/src/query/scoring.rs b/crates/codegraph-graph/src/query/scoring.rs index ca1cab1..51540d6 100644 --- a/crates/codegraph-graph/src/query/scoring.rs +++ b/crates/codegraph-graph/src/query/scoring.rs @@ -633,6 +633,31 @@ pub fn name_match_bonus(node_name: &str, query: &str) -> f64 { 0.0 } +/// Return a source-independent rank rescue for a case-insensitive whole-name match. +/// +/// A non-exact row can earn at most 80 from [`name_match_bonus`] and 10 from +/// [`kind_bonus`]. [`score_path_relevance`] has no query-independent ceiling: for +/// each whitespace-delimited query word it can add at most 10 for the filename +/// plus 5 for the immediate directory (the test-file adjustment only subtracts). +/// Scale that 15-point ceiling by the query length, then add one so an exact name +/// strictly dominates every non-exact bonus above the best candidate's base score. +pub fn exact_name_bonus(node_name: &str, query: &str) -> f64 { + if node_name.to_lowercase() != query.to_lowercase() { + return 0.0; + } + + const MAX_NAME_MATCH_BONUS: f64 = 80.0; + const MAX_KIND_BONUS: f64 = 10.0; + const MAX_PATH_BONUS_PER_QUERY_WORD: f64 = 15.0; + const STRICT_DOMINANCE_MARGIN: f64 = 1.0; + + let query_word_count = query.split_whitespace().count() as f64; + MAX_NAME_MATCH_BONUS + + MAX_KIND_BONUS + + MAX_PATH_BONUS_PER_QUERY_WORD * query_word_count + + STRICT_DOMINANCE_MARGIN +} + pub fn kind_bonus(kind: NodeKind) -> f64 { match kind { NodeKind::Function => 10.0, @@ -937,6 +962,13 @@ mod tests { assert_eq!(name_match_bonus("foo", "zzz"), 0.0); } + #[test] + fn exact_name_bonus_is_case_insensitive_and_exceeds_other_bonus_ceilings() { + assert_eq!(exact_name_bonus("LongSymbol", "longsymbol"), 106.0); + assert_eq!(exact_name_bonus("Long Symbol", "LONG SYMBOL"), 121.0); + assert_eq!(exact_name_bonus("LongerSymbol", "longsymbol"), 0.0); + } + #[test] fn kind_bonus_covers_every_variant() { for kind in NodeKind::ALL { diff --git a/crates/codegraph-graph/tests/search.rs b/crates/codegraph-graph/tests/search.rs index 1eedcbc..6570baa 100644 --- a/crates/codegraph-graph/tests/search.rs +++ b/crates/codegraph-graph/tests/search.rs @@ -460,3 +460,69 @@ fn search_parse_query_rust_qualifier_stays_in_text() { assert_eq!(parsed.text, "Counter::increment"); assert!(parsed.kinds.is_empty()); } + +#[test] +fn search_exact_long_name_ranks_first_deterministically() { + let mut store = Store::open(&temp_db_path("exact-long-name-rank")).expect("open temp store"); + store + .upsert_nodes(&[ + node( + "parameter:exact", + NodeKind::Parameter, + "LoadProjectSettingsFromConfiguredGodotResourceIdentifierLookup", + "Runtime::parameter", + "tests/runtime.rs", + Language::Rust, + 1, + 1, + None, + None, + false, + ), + node( + "function:competing", + NodeKind::Function, + "LoadProjectSettingsFromConfiguredGodotResourceIdentifierLookupX", + "LoadProjectSettingsFromConfiguredGodotResourceIdentifierLookupX::function", + "loadprojectsettingsfromconfiguredgodotresourceidentifierlookup/loadprojectsettingsfromconfiguredgodotresourceidentifierlookup.rs", + Language::Rust, + 1, + 1, + None, + None, + false, + ), + ]) + .expect("insert ranking corpus"); + + let project_tokens = HashSet::new(); + let query = "LoadProjectSettingsFromConfiguredGodotResourceIdentifierLookup"; + let first = search_nodes(&store, query, &SearchOptions::default(), &project_tokens) + .expect("first search"); + let second = search_nodes(&store, query, &SearchOptions::default(), &project_tokens) + .expect("second search"); + + let ordered = |results: &[codegraph_store::queries::SearchResult]| { + results + .iter() + .map(|result| (result.node.id.clone(), result.score.to_bits())) + .collect::>() + }; + assert_eq!(ordered(&first), ordered(&second), "search ordering changed"); + + let exact_rank = first + .iter() + .position(|result| result.node.id == "parameter:exact") + .expect("exact symbol returned") + + 1; + let exact_score = first[exact_rank - 1].score; + let competing_score = first + .iter() + .find(|result| result.node.id == "function:competing") + .expect("competing symbol returned") + .score; + assert_eq!( + exact_rank, 1, + "exact symbol ranked {exact_rank} with score {exact_score}; competing row scored {competing_score}" + ); +} From 4f32ad791e057759e9541e93c049abfcee99eb26 Mon Sep 17 00:00:00 2001 From: sunerpy Date: Fri, 31 Jul 2026 02:39:07 +0800 Subject: [PATCH 03/18] docs(mcp): recommend streamable-HTTP for Zed remote development The Zed-over-SSH section still presented the stdio bridge as the working workaround and carried no HTTP recipe, contradicting README.md and editors/zed/README.md, which already name HTTP the recommended remote transport. Lead with serve --http over a forwarded port, explain why forwarding beats exposing the port, and keep the SSH bridge as the documented fallback. --- docs/mcp.md | 63 ++++++++++++++++++++++++++++++++++++++++++++++------- 1 file changed, 55 insertions(+), 8 deletions(-) diff --git a/docs/mcp.md b/docs/mcp.md index 97f2d8e..e738b6b 100644 --- a/docs/mcp.md +++ b/docs/mcp.md @@ -104,10 +104,56 @@ open: the `command: "codegraph"` runs locally, cannot reach the remote the server on the remote host — is not yet implemented in Zed (tracked in Zed's GitHub issues; status as of mid-2026). -**Working workaround.** Make Zed's local `command` be `ssh` into the remote host -and run codegraph there. SSH proxies stdin/stdout transparently, so the MCP -JSON-RPC stream flows through the tunnel without any change to the codegraph -binary itself. +**Recommended fix — streamable-HTTP over a forwarded port.** Run the MCP server +on the remote host with the HTTP transport, forward its port through SSH, and give +Zed a `url` instead of a `command`. Zed then talks to a local port that SSH pipes +to the remote server, so the process reading the index runs where the index lives. + +On the **remote** host: + +```bash +codegraph serve --http --detach --path /abs/path/to/project +``` + +`--http` binds `127.0.0.1:8111` by default (override with +`--http-addr :`); `--detach` runs it in the background and prints its +pid and log path. `codegraph http list` shows running servers and +`codegraph http stop 127.0.0.1:8111` terminates one. With `--path`, the project +must already be indexed — run `codegraph init /abs/path/to/project` first. + +Forward the port from your **local** machine: + +```bash +ssh -N -L 8111:127.0.0.1:8111 +``` + +Then in `.zed/settings.json` on the **local** machine: + +```jsonc +{ + "context_servers": { + "codegraph": { + "url": "http://localhost:8111/mcp", + }, + }, +} +``` + +**Why forward rather than expose the port.** The default bind is loopback-only, +so nothing is reachable from the network — the SSH tunnel is what makes the remote +server visible, and it makes the endpoint genuinely _local_ from Zed's point of +view. That matters because MCP hosts commonly permit plain `http` only for +localhost and require `https` for anything remote (Kiro enforces exactly this). +Forwarding keeps you on the localhost side of that rule without terminating TLS. + +`codegraph install --target=zed` writes this HTTP entry into your `settings.json` +as a `//`-commented alternative next to the active stdio entry, marked RECOMMENDED +for remote — uncomment it rather than typing it out. + +**Fallback — SSH stdio bridge.** If you cannot forward a port, make Zed's local +`command` be `ssh` into the remote host and run codegraph there. SSH proxies +stdin/stdout transparently, so the MCP JSON-RPC stream flows through the tunnel +without any change to the codegraph binary itself. In your project's `.zed/settings.json` on the **local** machine: @@ -145,11 +191,12 @@ In your project's `.zed/settings.json` on the **local** machine: explicitly, so resolution never depends on the remote cwd or the MCP roots handshake over the tunnel. -**Caveats.** Each Zed window opens a fresh SSH session (the shared daemon is not -reused across them), so startup is slightly slower than a local connection. The +**Bridge caveats.** Each Zed window opens a fresh SSH session (the shared daemon is +not reused across them), so startup is slightly slower than a local connection. The remote codegraph daemon does still run for the duration of that session and serves -queries normally. This is a bridge solution until Zed ships native remote MCP -support. +queries normally. The HTTP transport avoids this — one detached server handles every +window — which is why it is the recommended path. Both are stopgaps until Zed ships +native remote MCP support. --- From e3787e93952a5d2f6accf6432d98b5aa868a9f83 Mon Sep 17 00:00:00 2001 From: sunerpy Date: Fri, 31 Jul 2026 02:47:37 +0800 Subject: [PATCH 04/18] docs(godot): document exact-name search recall The exact-name ranking guarantee added in 9ab0830 was undocumented, so a user had no way to know that querying a full symbol name now returns it first. State the behavior, both of its boundaries (the query must be the name as typed, and an already-winning row keeps its score), and that ordering is deterministic. --- docs/godot.md | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/docs/godot.md b/docs/godot.md index 15d2800..50944eb 100644 --- a/docs/godot.md +++ b/docs/godot.md @@ -33,6 +33,24 @@ dropping `addons`: ignore_dirs = [".godot", "node_modules", "target", "dist", ".venv"] ``` +### Exact-name search recall + +When your query string equals a symbol's name — compared case-insensitively over +the whole name, not as a substring — that symbol is returned as the **first** +result, ahead of any row that would otherwise outscore it on file-path relevance +or symbol kind. Long, precise GDScript names benefit most: querying +`apply_status_effect` puts that `func` at the top instead of burying it under +shorter partial matches from busier files. + +Two details worth knowing. The query has to be the name typed as-is; a multi-word +paraphrase never satisfies the whole-name comparison, since a query containing +whitespace cannot equal a symbol name that has none. And the rescue only lifts an +exact match that was tied with or below the best non-exact result — a row already +ranked first keeps its original score untouched. Ordering is deterministic, so +repeating the same query returns the same order every time. + +This applies to every language CodeGraph indexes, not just Godot. + ### `project.godot` — autoload singletons, input actions, plugins | Extracted item | Graph representation | From 2ab415d58a550f5cb3b9542cbcd61f3000d6059d Mon Sep 17 00:00:00 2001 From: sunerpy Date: Fri, 31 Jul 2026 03:20:35 +0800 Subject: [PATCH 05/18] fix(graph): group exact name matches by sort key instead of score The exact-name rescue mutated scores, which broke two ways. It lifted every needy exact row to the same value, erasing their score differences so their final order rested on the unordered SELECT in nodes_by_exact_name_nocase; and because a row already above the best non-exact score kept its original score, a weaker exact match could be lifted past a stronger one. Reproduced: a Parameter in a test path outranked a same-named Function in a matching path. Sort by exact-name status first and score second, via two stable passes, and mutate no scores. Every exact row precedes every non-exact row, exact rows keep their relative score order, no new ties are created, and the ordered search goldens stay byte-stable because the recorded scores never move. exact_name_bonus becomes the predicate it always was, is_exact_name_match; the derived dominance constants it needed are now dead and removed. --- crates/codegraph-graph/src/query/mod.rs | 32 +++---- crates/codegraph-graph/src/query/scoring.rs | 38 +++----- crates/codegraph-graph/tests/search.rs | 98 +++++++++++++++++++++ 3 files changed, 122 insertions(+), 46 deletions(-) diff --git a/crates/codegraph-graph/src/query/mod.rs b/crates/codegraph-graph/src/query/mod.rs index 83eb7ab..8b71b5c 100644 --- a/crates/codegraph-graph/src/query/mod.rs +++ b/crates/codegraph-graph/src/query/mod.rs @@ -117,26 +117,7 @@ pub fn search_nodes( + scoring::name_match_bonus(&result.node.name, scoring_query); } - // Preserve the established score ABI when an exact match already wins, but - // rescue any exact candidate that FTS (or exact-name injection) left below a - // non-exact row. The shared pass is intentionally source-independent, so it - // also covers candidates appended by multi-segment definer seeding. - let best_non_exact_score = results - .iter() - .filter(|result| scoring::exact_name_bonus(&result.node.name, scoring_query) == 0.0) - .map(|result| result.score) - .reduce(f64::max); - if let Some(best_non_exact_score) = best_non_exact_score { - for result in &mut results { - let exact_bonus = scoring::exact_name_bonus(&result.node.name, scoring_query); - if exact_bonus > 0.0 && result.score <= best_non_exact_score { - // Cover any FTS base-score gap first; the arithmetic-derived bonus - // then puts the exact row decisively above the best non-exact row. - result.score += best_non_exact_score - result.score + exact_bonus; - } - } - } - sort_by_score_desc(&mut results); + sort_by_exact_name_then_score_desc(&mut results, scoring_query); if results.len() > limit as usize { results.truncate(limit as usize); } @@ -168,6 +149,17 @@ fn sort_by_score_desc(results: &mut [SearchResult]) { }); } +fn sort_by_exact_name_then_score_desc(results: &mut [SearchResult], query: &str) { + // Both sorts are stable: establish score order first, then group exact whole-name + // matches ahead of non-exact rows without mutating the externally visible scores. + sort_by_score_desc(results); + results.sort_by(|a, b| { + let a_is_exact = scoring::is_exact_name_match(&a.node.name, query); + let b_is_exact = scoring::is_exact_name_match(&b.node.name, query); + b_is_exact.cmp(&a_is_exact) + }); +} + /// Multi-hump field-name seeding (upstream `1de7e8f` #1319). /// /// A query token that names a multi-SEGMENT field (`userProfileId`, diff --git a/crates/codegraph-graph/src/query/scoring.rs b/crates/codegraph-graph/src/query/scoring.rs index 51540d6..2c13d1f 100644 --- a/crates/codegraph-graph/src/query/scoring.rs +++ b/crates/codegraph-graph/src/query/scoring.rs @@ -633,29 +633,15 @@ pub fn name_match_bonus(node_name: &str, query: &str) -> f64 { 0.0 } -/// Return a source-independent rank rescue for a case-insensitive whole-name match. +/// Whether `node_name` equals the WHOLE `query`, compared case-insensitively. /// -/// A non-exact row can earn at most 80 from [`name_match_bonus`] and 10 from -/// [`kind_bonus`]. [`score_path_relevance`] has no query-independent ceiling: for -/// each whitespace-delimited query word it can add at most 10 for the filename -/// plus 5 for the immediate directory (the test-file adjustment only subtracts). -/// Scale that 15-point ceiling by the query length, then add one so an exact name -/// strictly dominates every non-exact bonus above the best candidate's base score. -pub fn exact_name_bonus(node_name: &str, query: &str) -> f64 { - if node_name.to_lowercase() != query.to_lowercase() { - return 0.0; - } - - const MAX_NAME_MATCH_BONUS: f64 = 80.0; - const MAX_KIND_BONUS: f64 = 10.0; - const MAX_PATH_BONUS_PER_QUERY_WORD: f64 = 15.0; - const STRICT_DOMINANCE_MARGIN: f64 = 1.0; - - let query_word_count = query.split_whitespace().count() as f64; - MAX_NAME_MATCH_BONUS - + MAX_KIND_BONUS - + MAX_PATH_BONUS_PER_QUERY_WORD * query_word_count - + STRICT_DOMINANCE_MARGIN +/// Used as a sort key, not a score: the search pass groups exact whole-name +/// matches ahead of every non-exact row while leaving scores untouched, so a +/// precise symbol cannot be out-ranked by a longer partial match that happens to +/// earn more from [`kind_bonus`] or [`score_path_relevance`]. Whitespace is +/// significant — a multi-word query never equals a whitespace-free name. +pub fn is_exact_name_match(node_name: &str, query: &str) -> bool { + node_name.to_lowercase() == query.to_lowercase() } pub fn kind_bonus(kind: NodeKind) -> f64 { @@ -963,10 +949,10 @@ mod tests { } #[test] - fn exact_name_bonus_is_case_insensitive_and_exceeds_other_bonus_ceilings() { - assert_eq!(exact_name_bonus("LongSymbol", "longsymbol"), 106.0); - assert_eq!(exact_name_bonus("Long Symbol", "LONG SYMBOL"), 121.0); - assert_eq!(exact_name_bonus("LongerSymbol", "longsymbol"), 0.0); + fn exact_name_match_is_full_string_and_case_insensitive() { + assert!(is_exact_name_match("LongSymbol", "longsymbol")); + assert!(is_exact_name_match("Long Symbol", "LONG SYMBOL")); + assert!(!is_exact_name_match("LongerSymbol", "longsymbol")); } #[test] diff --git a/crates/codegraph-graph/tests/search.rs b/crates/codegraph-graph/tests/search.rs index 6570baa..cdac3b4 100644 --- a/crates/codegraph-graph/tests/search.rs +++ b/crates/codegraph-graph/tests/search.rs @@ -526,3 +526,101 @@ fn search_exact_long_name_ranks_first_deterministically() { "exact symbol ranked {exact_rank} with score {exact_score}; competing row scored {competing_score}" ); } + +#[test] +fn search_multiple_exact_matches_preserve_score_order_deterministically() { + let mut store = + Store::open(&temp_db_path("multiple-exact-score-order")).expect("open temp store"); + let query = "LoadProjectSettingsFromConfiguredGodotResourceIdentifierLookup"; + let scoring_path = format!("{query}/{query}.rs"); + store + .upsert_nodes(&[ + node( + "function:strong-exact", + NodeKind::Function, + query, + "Runtime::strong_exact", + &scoring_path, + Language::Rust, + 1, + 1, + None, + None, + false, + ), + node( + "parameter:weak-exact", + NodeKind::Parameter, + &query.to_lowercase(), + "Runtime::weak_exact", + "tests/runtime.rs", + Language::Rust, + 2, + 2, + None, + None, + false, + ), + node( + "function:competing", + NodeKind::Function, + &format!("{query}X"), + "Runtime::competing", + &scoring_path, + Language::Rust, + 3, + 3, + None, + None, + false, + ), + ]) + .expect("insert ranking corpus"); + + let project_tokens = HashSet::new(); + let first = search_nodes(&store, query, &SearchOptions::default(), &project_tokens) + .expect("first search"); + let second = search_nodes(&store, query, &SearchOptions::default(), &project_tokens) + .expect("second search"); + let ordered = |results: &[codegraph_store::queries::SearchResult]| { + results + .iter() + .map(|result| (result.node.id.clone(), result.score.to_bits())) + .collect::>() + }; + assert_eq!(ordered(&first), ordered(&second), "search ordering changed"); + + let exact = first + .iter() + .filter(|result| result.node.name.eq_ignore_ascii_case(query)) + .collect::>(); + assert_eq!(exact.len(), 2, "expected both exact-name rows"); + assert_eq!(exact[0].node.id, "function:strong-exact"); + assert_eq!(exact[1].node.id, "parameter:weak-exact"); + assert!( + exact[0].score > exact[1].score, + "strong exact score {} must remain above weak exact score {}", + exact[0].score, + exact[1].score + ); + + let limited = search_nodes( + &store, + query, + &SearchOptions { + limit: Some(2), + ..SearchOptions::default() + }, + &project_tokens, + ) + .expect("limited search"); + let limited_ids = limited + .iter() + .map(|result| result.node.id.as_str()) + .collect::>(); + assert_eq!( + limited_ids, + ["function:strong-exact", "parameter:weak-exact"], + "limit must retain exact rows in score order" + ); +} From 74eb775c6b1eeb1097cc2b57edeef8b97291afe6 Mon Sep 17 00:00:00 2001 From: sunerpy Date: Fri, 31 Jul 2026 03:20:35 +0800 Subject: [PATCH 06/18] fix(watch): ignore .godot and addons without relying on gitignore The watcher kept its own copy of the default ignore list, and that copy had drifted from the scan's: .godot and addons were absent, so live watching only skipped them when a project's .gitignore happened to list them. A Godot project that commits .godot got those trees re-indexed after any edit even though codegraph init had excluded them. Derive the shared entries from IndexingConfig::default().ignore_dirs so the two lists cannot diverge again, keep the watch-only entries in their own list, and place .godot and addons after the built-in-skip boundary so an explicit include can still opt a first-party addon back in, matching the scan. collect_watch_dirs_honors_gitignore_pruning now uses .generated/, a directory genuinely absent from the defaults, so it still proves gitignore merging rather than passing for the wrong reason. --- crates/codegraph-watch/src/policy.rs | 119 +++++++++++++------------- crates/codegraph-watch/src/watcher.rs | 16 ++-- 2 files changed, 69 insertions(+), 66 deletions(-) diff --git a/crates/codegraph-watch/src/policy.rs b/crates/codegraph-watch/src/policy.rs index ef20b2f..258f7f3 100644 --- a/crates/codegraph-watch/src/policy.rs +++ b/crates/codegraph-watch/src/policy.rs @@ -6,59 +6,7 @@ use codegraph_extract::{ExtensionOverrides, detect_language_with}; pub const CODEGRAPH_NO_WATCH: &str = "CODEGRAPH_NO_WATCH"; -const DEFAULT_IGNORE_DIRS: &[&str] = &[ - "node_modules", - "bower_components", - "jspm_packages", - "web_modules", - ".yarn", - ".pnpm-store", - ".next", - ".nuxt", - ".svelte-kit", - ".turbo", - ".vite", - ".parcel-cache", - ".angular", - ".docusaurus", - "storybook-static", - ".vinxi", - ".nitro", - "out-tsc", - ".vercel", - ".netlify", - ".wrangler", - "dist", - "build", - "out", - ".output", - "coverage", - ".nyc_output", - "__pycache__", - "__pypackages__", - ".venv", - "venv", - ".pixi", - ".pdm-build", - ".mypy_cache", - ".pytest_cache", - ".ruff_cache", - ".tox", - ".nox", - ".hypothesis", - ".ipynb_checkpoints", - ".eggs", - "target", - ".gradle", - "obj", - "vendor", - ".build", - "Pods", - "Carthage", - "DerivedData", - ".swiftpm", - ".dart_tool", - ".pub-cache", +const WATCH_ONLY_DEFAULT_IGNORE_DIRS: &[&str] = &[ ".cxx", ".externalNativeBuild", "vcpkg_installed", @@ -71,6 +19,8 @@ const DEFAULT_IGNORE_DIRS: &[&str] = &[ ".cache", ]; +const REINCLUDABLE_SCAN_DEFAULT_IGNORE_DIRS: &[&str] = &[".godot", "addons"]; + #[derive(Debug, Clone)] struct IgnoreRule { pattern: String, @@ -108,8 +58,12 @@ impl WatchPolicy { // `upstream extraction/index.ts:117-161,242-246` so // `.gitignore` negations can opt default-excluded dirs back in. let root = root.as_ref().to_path_buf(); - let mut rules = DEFAULT_IGNORE_DIRS + let scan_ignore_dirs = codegraph_core::config::IndexingConfig::default().ignore_dirs; + let mut rules = scan_ignore_dirs .iter() + .map(String::as_str) + .filter(|dir| !REINCLUDABLE_SCAN_DEFAULT_IGNORE_DIRS.contains(dir)) + .chain(WATCH_ONLY_DEFAULT_IGNORE_DIRS.iter().copied()) .map(|dir| IgnoreRule { pattern: format!("{dir}/"), negated: false, @@ -129,11 +83,19 @@ impl WatchPolicy { negated: false, }, ]); - // The built-in DEFAULT_IGNORE_DIRS + egg/cmake/bazel rules are the - // "built-in skip" set that `include` must NEVER re-watch; count them so - // the include override below can tell a built-in skip from a `.gitignore` - // rule. Only `.gitignore` (and default-path) exclusions are overridable. + // The hard scan defaults, watch-only defaults, and egg/cmake/bazel rules + // are the "built-in skip" set that `include` must NEVER re-watch. Godot's + // configurable defaults are appended after this boundary because the scan + // permits dropping them from `indexing.ignore_dirs`. let builtin_rule_count = rules.len(); + rules.extend( + REINCLUDABLE_SCAN_DEFAULT_IGNORE_DIRS + .iter() + .map(|dir| IgnoreRule { + pattern: format!("{dir}/"), + negated: false, + }), + ); rules.extend(read_gitignore_rules(&root)); Self { root, @@ -561,6 +523,47 @@ mod tests { assert!(policy.should_watch_dir("src/components")); } + #[test] + fn default_godot_dirs_are_ignored_without_gitignore() { + // Given: a project with no `.gitignore`, so only the default policy applies. + let dir = crate::sync::tests::TestDir::new("watch-policy-godot-defaults"); + assert!(!dir.path().join(".gitignore").exists()); + + // When: the watcher evaluates Godot noise and normal business paths. + let policy = WatchPolicy::new(dir.path()); + + // Then: both Godot noise trees are pruned, while normal source stays watched. + assert!(!policy.should_watch_dir(".godot")); + assert!(!policy.should_handle_file(".godot/cache_junk.gd")); + assert!(!policy.should_watch_dir("addons")); + assert!(!policy.should_handle_file("addons/vendor_plugin/plugin.gd")); + assert!(policy.should_watch_dir("src/gameplay")); + assert!(policy.should_handle_file("src/gameplay/player.gd")); + } + + #[test] + fn with_config_include_reincludes_addons_default_ignore() { + // Given: no `.gitignore`, and a first-party addon explicitly included by config. + let dir = crate::sync::tests::TestDir::new("watch-policy-addons-include"); + assert!(!dir.path().join(".gitignore").exists()); + let default_policy = WatchPolicy::new(dir.path()); + let include = ["addons/first_party/**".to_string()]; + let included = WatchPolicy::with_config(dir.path(), &include, &[]); + // Excludes use the shared matcher, where `dir/` covers its subtree; + // unlike the include-only helper, that matcher does not expand `dir/**`. + let excluded = + WatchPolicy::with_config(dir.path(), &include, &["addons/first_party/".to_string()]); + + // When/Then: the default is ignored, include opts in only its subtree, + // and an explicit exclude still wins over that include. + assert!(!default_policy.should_watch_dir("addons")); + assert!(included.should_watch_dir("addons")); + assert!(included.should_watch_dir("addons/first_party")); + assert!(included.should_handle_file("addons/first_party/tool.gd")); + assert!(!included.should_handle_file("addons/vendor_plugin/plugin.gd")); + assert!(!excluded.should_handle_file("addons/first_party/tool.gd")); + } + #[test] fn partial_segment_names_are_not_false_positives() { let dir = crate::sync::tests::TestDir::new("watch-policy-partial"); diff --git a/crates/codegraph-watch/src/watcher.rs b/crates/codegraph-watch/src/watcher.rs index c7a08c3..e1f41c7 100644 --- a/crates/codegraph-watch/src/watcher.rs +++ b/crates/codegraph-watch/src/watcher.rs @@ -2290,15 +2290,15 @@ mod tests { #[test] fn collect_watch_dirs_honors_gitignore_pruning() { - // `.godot/` is not in DEFAULT_IGNORE_DIRS, but a real Godot project lists - // it in .gitignore, which WatchPolicy::new merges. This proves the watch - // set is pruned by the SAME merged policy (not a second hardcoded list), - // so project-specific ignores keep us out of generated trees too. + // `.generated/` is deliberately absent from the default ignores. This + // proves the watch set is pruned by the SAME merged policy (not a second + // hardcoded list), so project-specific ignores keep us out of generated + // trees too. let dir = crate::sync::tests::TestDir::new("watch-collect-gitignore"); let root = dir.path(); - fs::write(root.join(".gitignore"), ".godot/\n").unwrap(); + fs::write(root.join(".gitignore"), ".generated/\n").unwrap(); fs::create_dir_all(root.join("scenes")).unwrap(); - fs::create_dir_all(root.join(".godot/imported")).unwrap(); + fs::create_dir_all(root.join(".generated/cache")).unwrap(); let policy = WatchPolicy::new(root); let dirs = collect_watch_dirs(root, &policy); @@ -2309,8 +2309,8 @@ mod tests { assert!(got.contains(&"scenes".to_string()), "source dir kept"); assert!( - !got.iter().any(|p| p.starts_with(".godot")), - ".godot subtree must be pruned via .gitignore, got {got:?}" + !got.iter().any(|p| p.starts_with(".generated")), + ".generated subtree must be pruned via .gitignore, got {got:?}" ); } From c604b67a83630ca3a915717cc12475abbfc92524 Mon Sep 17 00:00:00 2001 From: sunerpy Date: Fri, 31 Jul 2026 04:18:56 +0800 Subject: [PATCH 07/18] fix(watch): take ignore_dirs from project config as a structural skip MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The watcher read IndexingConfig::default().ignore_dirs and placed .godot and addons outside its built-in boundary, so include could resurface them. The scan does neither: scan_dir prunes options.ignore_dirs structurally before any include check, and ExtractOptions::for_project fills that list from the project's own config. The two disagreed in both directions — include re-included under the watcher but not the scan, and dropping addons from indexing.ignore_dirs re-included under the scan but not the watcher — either of which breaks sync output equalling index --force byte-for-byte. Thread the project's configured ignore_dirs through WatchOptions into WatchPolicy and keep every entry inside the built-in boundary. Overriding indexing.ignore_dirs is now the single re-inclusion surface on both sides, the same contract scan_reincludes_addons_when_override_drops_it already documents; gitignore-derived rules stay re-includable and exclude still beats include. with_config_include_reincludes_addons_default_ignore encoded the inverted contract and is replaced by a non-re-inclusion assertion plus a new configured-override test. --- crates/codegraph-watch/src/policy.rs | 116 ++++++++++-------- crates/codegraph-watch/src/sync.rs | 5 +- crates/codegraph-watch/src/watcher.rs | 30 +++-- .../tests/include_scan_watch_parity.rs | 4 +- 4 files changed, 90 insertions(+), 65 deletions(-) diff --git a/crates/codegraph-watch/src/policy.rs b/crates/codegraph-watch/src/policy.rs index 258f7f3..534c9ae 100644 --- a/crates/codegraph-watch/src/policy.rs +++ b/crates/codegraph-watch/src/policy.rs @@ -19,8 +19,6 @@ const WATCH_ONLY_DEFAULT_IGNORE_DIRS: &[&str] = &[ ".cache", ]; -const REINCLUDABLE_SCAN_DEFAULT_IGNORE_DIRS: &[&str] = &[".godot", "addons"]; - #[derive(Debug, Clone)] struct IgnoreRule { pattern: String, @@ -44,25 +42,27 @@ pub struct WatchPolicy { impl WatchPolicy { pub fn new(root: impl AsRef) -> Self { - Self::with_config(root, &[], &[]) - } - - /// Like [`new`](Self::new) but threads the `codegraph.json`/`config.toml` - /// `include` and `exclude` path patterns (#1063) so the watcher's scope - /// matches the scan's: an included gitignored dir is WATCHED and its file - /// events HANDLED, while a built-in skip is never re-watched and an explicit - /// `exclude` always wins. Empty `include`+`exclude` is byte-identical to the - /// pre-#1063 policy. - pub fn with_config(root: impl AsRef, include: &[String], exclude: &[String]) -> Self { + let indexing = codegraph_core::config::IndexingConfig::default(); + Self::with_config(root, &indexing.ignore_dirs, &[], &[]) + } + + /// Like [`new`](Self::new) but threads the project's configured `ignore_dirs`, + /// `include`, and `exclude` scope into the watcher. Configured ignored dirs + /// mirror the scan's structural prune and cannot be re-included; an included + /// gitignored dir is watched, while an explicit `exclude` always wins. + pub fn with_config( + root: impl AsRef, + ignore_dirs: &[String], + include: &[String], + exclude: &[String], + ) -> Self { // Mirrors the upstream built-in ignore seed and root .gitignore merge from // `upstream extraction/index.ts:117-161,242-246` so // `.gitignore` negations can opt default-excluded dirs back in. let root = root.as_ref().to_path_buf(); - let scan_ignore_dirs = codegraph_core::config::IndexingConfig::default().ignore_dirs; - let mut rules = scan_ignore_dirs + let mut rules = ignore_dirs .iter() .map(String::as_str) - .filter(|dir| !REINCLUDABLE_SCAN_DEFAULT_IGNORE_DIRS.contains(dir)) .chain(WATCH_ONLY_DEFAULT_IGNORE_DIRS.iter().copied()) .map(|dir| IgnoreRule { pattern: format!("{dir}/"), @@ -83,19 +83,9 @@ impl WatchPolicy { negated: false, }, ]); - // The hard scan defaults, watch-only defaults, and egg/cmake/bazel rules - // are the "built-in skip" set that `include` must NEVER re-watch. Godot's - // configurable defaults are appended after this boundary because the scan - // permits dropping them from `indexing.ignore_dirs`. + // The configured scan ignores, watch-only defaults, and egg/cmake/bazel + // rules are structural skips that `include` must NEVER re-watch. let builtin_rule_count = rules.len(); - rules.extend( - REINCLUDABLE_SCAN_DEFAULT_IGNORE_DIRS - .iter() - .map(|dir| IgnoreRule { - pattern: format!("{dir}/"), - negated: false, - }), - ); rules.extend(read_gitignore_rules(&root)); Self { root, @@ -542,26 +532,42 @@ mod tests { } #[test] - fn with_config_include_reincludes_addons_default_ignore() { - // Given: no `.gitignore`, and a first-party addon explicitly included by config. + fn with_config_include_does_not_reinclude_configured_ignore_dir() { + // Given: no `.gitignore`, the default ignored dirs, and a first-party addon + // explicitly included by config. let dir = crate::sync::tests::TestDir::new("watch-policy-addons-include"); assert!(!dir.path().join(".gitignore").exists()); - let default_policy = WatchPolicy::new(dir.path()); + let ignore_dirs = codegraph_core::config::IndexingConfig::default().ignore_dirs; let include = ["addons/first_party/**".to_string()]; - let included = WatchPolicy::with_config(dir.path(), &include, &[]); - // Excludes use the shared matcher, where `dir/` covers its subtree; - // unlike the include-only helper, that matcher does not expand `dir/**`. - let excluded = - WatchPolicy::with_config(dir.path(), &include, &["addons/first_party/".to_string()]); - - // When/Then: the default is ignored, include opts in only its subtree, - // and an explicit exclude still wins over that include. - assert!(!default_policy.should_watch_dir("addons")); - assert!(included.should_watch_dir("addons")); - assert!(included.should_watch_dir("addons/first_party")); - assert!(included.should_handle_file("addons/first_party/tool.gd")); + let included = WatchPolicy::with_config(dir.path(), &ignore_dirs, &include, &[]); + + // When/Then: configured ignored dirs are structural scan skips, so include + // cannot resurface the directory or any source file below it. + assert!(!included.should_watch_dir("addons")); + assert!(!included.should_watch_dir("addons/first_party")); + assert!(!included.should_handle_file("addons/first_party/tool.gd")); assert!(!included.should_handle_file("addons/vendor_plugin/plugin.gd")); - assert!(!excluded.should_handle_file("addons/first_party/tool.gd")); + } + + #[test] + fn configured_ignore_dirs_override_reincludes_addons() { + // Given: the project drops `addons` from its configured ignore dirs while + // retaining `.godot`, mirroring the scan-side override contract. + let dir = crate::sync::tests::TestDir::new("watch-policy-addons-override"); + assert!(!dir.path().join(".gitignore").exists()); + let mut ignore_dirs = codegraph_core::config::IndexingConfig::default().ignore_dirs; + ignore_dirs.retain(|entry| entry != "addons"); + + // When: the watcher builds its policy from that project's configured list. + let policy = WatchPolicy::with_config(dir.path(), &ignore_dirs, &[], &[]); + + // Then: first-party addons are watched, while the retained Godot cache skip + // remains structurally ignored. + assert!(policy.should_watch_dir("addons")); + assert!(policy.should_watch_dir("addons/first_party")); + assert!(policy.should_handle_file("addons/first_party/tool.gd")); + assert!(!policy.should_watch_dir(".godot")); + assert!(!policy.should_handle_file(".godot/cache_junk.gd")); } #[test] @@ -744,8 +750,10 @@ mod tests { // exclude still wins. let dir = crate::sync::tests::TestDir::new("watch-policy-include"); fs::write(dir.path().join(".gitignore"), "Tools/\nLocal/\n").unwrap(); + let ignore_dirs = codegraph_core::config::IndexingConfig::default().ignore_dirs; let policy = WatchPolicy::with_config( dir.path(), + &ignore_dirs, &["Tools/".to_string(), "Local/ts/app.ts".to_string()], &[], ); @@ -760,23 +768,33 @@ mod tests { assert!(policy.should_handle_file("Local/ts/app.ts")); assert!(!policy.should_handle_file("Local/ts/other.ts")); // A built-in skip is NEVER re-watched, even if named in include. - let builtin = WatchPolicy::with_config(dir.path(), &["node_modules/".to_string()], &[]); + let builtin = WatchPolicy::with_config( + dir.path(), + &ignore_dirs, + &["node_modules/".to_string()], + &[], + ); assert!(!builtin.should_watch_dir("node_modules")); assert!(!builtin.should_handle_file("node_modules/pkg/index.ts")); // An explicit exclude wins over include. - let excluded = - WatchPolicy::with_config(dir.path(), &["Tools/".to_string()], &["Tools/".to_string()]); + let excluded = WatchPolicy::with_config( + dir.path(), + &ignore_dirs, + &["Tools/".to_string()], + &["Tools/".to_string()], + ); assert!(!excluded.should_watch_dir("Tools")); assert!(!excluded.should_handle_file("Tools/helper.ts")); } #[test] fn empty_include_leaves_policy_byte_identical() { - // #1063: WatchPolicy::new == with_config(root, &[], &[]); an empty include - // must not change any watch/handle decision vs the pre-#1063 policy. + // #1063: WatchPolicy::new equals with_config using the default ignore dirs + // and empty include/exclude; an empty include changes no policy decision. let dir = crate::sync::tests::TestDir::new("watch-policy-include-empty"); fs::write(dir.path().join(".gitignore"), "vendor/\n").unwrap(); - let policy = WatchPolicy::with_config(dir.path(), &[], &[]); + let ignore_dirs = codegraph_core::config::IndexingConfig::default().ignore_dirs; + let policy = WatchPolicy::with_config(dir.path(), &ignore_dirs, &[], &[]); assert!(!policy.should_watch_dir("vendor")); assert!(!policy.should_handle_file("vendor/dep.ts")); assert!(!policy.should_watch_dir("node_modules")); diff --git a/crates/codegraph-watch/src/sync.rs b/crates/codegraph-watch/src/sync.rs index 3784684..a49bcb9 100644 --- a/crates/codegraph-watch/src/sync.rs +++ b/crates/codegraph-watch/src/sync.rs @@ -374,8 +374,9 @@ fn sync_paths_with_store( started: std::time::Instant, mut on_progress: impl FnMut(usize, usize), ) -> Result { - let policy = WatchPolicy::with_config(project_root, include, exclude) - .with_extension_overrides(Arc::clone(&scope.options.extensions)); + let policy = + WatchPolicy::with_config(project_root, &scope.options.ignore_dirs, include, exclude) + .with_extension_overrides(Arc::clone(&scope.options.extensions)); let mut outcome = SyncOutcome::default(); let mut changed = false; let mut seen = HashSet::new(); diff --git a/crates/codegraph-watch/src/watcher.rs b/crates/codegraph-watch/src/watcher.rs index e1f41c7..4451ea2 100644 --- a/crates/codegraph-watch/src/watcher.rs +++ b/crates/codegraph-watch/src/watcher.rs @@ -301,9 +301,9 @@ pub struct WatchOptions { /// Called for a non-degrading watch/sync error (e.g. inotify watch-count /// exhaustion). May fire more than once; the watcher keeps running. pub on_sync_error: Option, - /// The addressed project's `config.toml` `include`/`exclude` path patterns - /// (#1063). Threaded into the [`WatchPolicy`] and the default incremental-sync - /// fn so the watcher's scope matches the scan's. Empty = pre-#1063 behavior. + /// The addressed project's `config.toml` indexing scope. Threaded into the + /// [`WatchPolicy`] and default sync functions so watcher scope matches scan. + pub ignore_dirs: Vec, pub include: Vec, pub exclude: Vec, /// The addressed project's custom extension→language overrides (its @@ -322,6 +322,7 @@ pub struct WatchOptions { impl Default for WatchOptions { fn default() -> Self { + let indexing = codegraph_core::config::IndexingConfig::default(); Self { // Upstream default debounce is 2000ms (`watch-policy.ts` notes and // `watcher.ts:86-90,220-223`); env override is clamped [100ms, 60s]. @@ -332,8 +333,9 @@ impl Default for WatchOptions { on_sync_complete: None, on_degraded: None, on_sync_error: None, - include: Vec::new(), - exclude: Vec::new(), + ignore_dirs: indexing.ignore_dirs, + include: indexing.include, + exclude: indexing.exclude, extensions: ExtensionOverrides::empty(), sync_fn: None, full_sync_fn: None, @@ -346,11 +348,9 @@ impl WatchOptions { /// Build watch options from ONE project's immutable [`Config`] and its own /// extension overrides. /// - /// `include`/`exclude` and the debounce window come from that project, and - /// `watch.enabled = false` disables watching for it — so two projects served - /// by one process can watch different scopes (or one not at all). An explicit - /// `CODEGRAPH_WATCH_DEBOUNCE_MS` still wins, keeping the documented env escape - /// hatch authoritative over config. + /// Indexing scope and the debounce window come from that project, and + /// `watch.enabled = false` disables watching for it. An explicit + /// `CODEGRAPH_WATCH_DEBOUNCE_MS` still wins over config. /// Share `cancel` with this watcher's default sync closures so a shutdown /// can cancel queued and running lease loops. #[must_use] @@ -364,6 +364,7 @@ impl WatchOptions { Self { debounce: debounce_from_env_or(config.watch.debounce_ms), no_watch: !config.watch.enabled, + ignore_dirs: config.indexing.ignore_dirs.clone(), include: config.indexing.include.clone(), exclude: config.indexing.exclude.clone(), extensions, @@ -421,8 +422,13 @@ impl ProjectWatcher { if watch_disabled_reason(&project_root, options.no_watch).is_some() { return Ok(None); } - let policy = WatchPolicy::with_config(&project_root, &options.include, &options.exclude) - .with_extension_overrides(Arc::clone(&options.extensions)); + let policy = WatchPolicy::with_config( + &project_root, + &options.ignore_dirs, + &options.include, + &options.exclude, + ) + .with_extension_overrides(Arc::clone(&options.extensions)); let db_path = match options.db_path.clone() { Some(db) => db, None => default_db_path(&project_root)?, diff --git a/crates/codegraph-watch/tests/include_scan_watch_parity.rs b/crates/codegraph-watch/tests/include_scan_watch_parity.rs index e6f5555..7da4ec4 100644 --- a/crates/codegraph-watch/tests/include_scan_watch_parity.rs +++ b/crates/codegraph-watch/tests/include_scan_watch_parity.rs @@ -66,7 +66,7 @@ fn scan_and_watch_agree_on_include_file_verdicts() { ..ExtractOptions::default() }; let scanned = scan_project(&project, &options).expect("scan"); - let policy = WatchPolicy::with_config(&project, &include, &[]); + let policy = WatchPolicy::with_config(&project, &options.ignore_dirs, &include, &[]); for file in candidate_files { let in_scan = scanned.iter().any(|f| f == file); @@ -99,7 +99,7 @@ fn gen_glob_indexes_and_watches_gitignored_file() { "gen* must index gen/helper.ts: {scanned:?}" ); - let policy = WatchPolicy::with_config(&project, &include, &[]); + let policy = WatchPolicy::with_config(&project, &options.ignore_dirs, &include, &[]); assert!( policy.should_handle_file("gen/helper.ts"), "gen* must watch gen/helper.ts (parity with scan)" From eb9c2d15cc88809613342e37eab1ab8e18f7ac01 Mon Sep 17 00:00:00 2001 From: sunerpy Date: Fri, 31 Jul 2026 09:08:04 +0800 Subject: [PATCH 08/18] fix(watch): apply exclude unconditionally and prune ignore_dirs structurally MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit is_ignored returned early when include was empty, so a configured exclude was never read in the common case: the scan dropped src/generated.ts while the watcher handled it. And gitignore rules shared one last-match-wins vector with the configured ignore_dirs, so a !addons/ line re-included a directory scan_dir prunes with a bare continue before any pattern set runs. Either way sync stopped equalling index --force. Split the single rules vector into structural_ignores and gitignore_rules so the tier boundary is a type distinction rather than an index convention, and give is_ignored the scan's three stages in the scan's order: structural prune, then a negotiable exclude-then-gitignore stream evaluated for every path, then include force-inclusion. matches_negotiable seeds its verdict from exclude before folding gitignore rules, which is what preserves both the scan's set order and each side's own glob semantics — exclude keeps the shared whole-path matcher, gitignore keeps rule_matches. gitignore_negation_reincludes_default_ignored_dir asserted that !vendor/ re-includes vendor, but vendor is in default_ignore_dirs, so the assertion was the defect. It now contrasts a negotiable Tools/ negation against structural vendor and node_modules. --- crates/codegraph-watch/src/policy.rs | 133 +++++++++++------- .../tests/include_scan_watch_parity.rs | 96 +++++++++++++ 2 files changed, 176 insertions(+), 53 deletions(-) diff --git a/crates/codegraph-watch/src/policy.rs b/crates/codegraph-watch/src/policy.rs index 534c9ae..3b3549e 100644 --- a/crates/codegraph-watch/src/policy.rs +++ b/crates/codegraph-watch/src/policy.rs @@ -28,10 +28,18 @@ struct IgnoreRule { #[derive(Debug, Clone)] pub struct WatchPolicy { root: PathBuf, - rules: Vec, - /// Number of leading `rules` that are the built-in skip set (never - /// re-includable by `include`); the rest are `.gitignore`-derived. - builtin_rule_count: usize, + /// STRUCTURAL skips, mirroring the scan's pre-include prune in `scan_dir`: + /// the project's configured `ignore_dirs`, the watch-only defaults, and the + /// egg/cmake/bazel forms. Matched with the watcher's `.gitignore`-style + /// [`rule_matches`]; nothing can undo one — not a `.gitignore` negation, not + /// `include` — exactly as `scan_dir` `continue`s on them before it evaluates + /// any pattern set. + structural_ignores: Vec, + /// The root `.gitignore` rules in file order — the NEGOTIABLE tail of the + /// last-match-wins stream (`exclude` first, these second, mirroring the + /// scan's `pattern_sets`), so a `!pattern` line here re-includes what an + /// `exclude` or an earlier `.gitignore` line dropped. + gitignore_rules: Vec, include: Vec, exclude: Vec, /// The addressed project's custom extension→language overrides, so a file the @@ -57,40 +65,26 @@ impl WatchPolicy { exclude: &[String], ) -> Self { // Mirrors the upstream built-in ignore seed and root .gitignore merge from - // `upstream extraction/index.ts:117-161,242-246` so - // `.gitignore` negations can opt default-excluded dirs back in. + // `upstream extraction/index.ts:117-161,242-246`, split into the scan's + // two tiers: the structural prune nothing can undo, then the negotiable + // `.gitignore` stream whose negations can opt an excluded path back in. let root = root.as_ref().to_path_buf(); - let mut rules = ignore_dirs + let structural_ignores = ignore_dirs .iter() .map(String::as_str) .chain(WATCH_ONLY_DEFAULT_IGNORE_DIRS.iter().copied()) - .map(|dir| IgnoreRule { - pattern: format!("{dir}/"), - negated: false, - }) + .map(|dir| format!("{dir}/")) + .chain( + ["*.egg-info/", "cmake-build-*/", "bazel-*/"] + .into_iter() + .map(str::to_string), + ) .collect::>(); - rules.extend([ - IgnoreRule { - pattern: "*.egg-info/".to_string(), - negated: false, - }, - IgnoreRule { - pattern: "cmake-build-*/".to_string(), - negated: false, - }, - IgnoreRule { - pattern: "bazel-*/".to_string(), - negated: false, - }, - ]); - // The configured scan ignores, watch-only defaults, and egg/cmake/bazel - // rules are structural skips that `include` must NEVER re-watch. - let builtin_rule_count = rules.len(); - rules.extend(read_gitignore_rules(&root)); + let gitignore_rules = read_gitignore_rules(&root); Self { root, - rules, - builtin_rule_count, + structural_ignores, + gitignore_rules, include: include.to_vec(), exclude: exclude.to_vec(), extensions: ExtensionOverrides::empty(), @@ -143,25 +137,29 @@ impl WatchPolicy { top == ".git" || top == ".codegraph" || top.starts_with(".codegraph-") } + /// The watcher's counterpart to the scan's three-stage decision in + /// `scan_project`/`scan_dir`, in the SAME order, so `sync`/watch keeps + /// exactly the files `index --force` keeps. fn is_ignored(&self, relative: &str, is_dir: bool) -> bool { - let base = self.matches_rules(&self.rules, relative, is_dir); - // Empty include short-circuits the #1063 override → pre-#1063 behavior - // byte-identical (the override only ever flips an ignored path back in). - if self.include.is_empty() || !base { - return base; + // 1. Structural prune. `scan_dir` `continue`s on a configured ignore dir + // before any pattern set or include runs, so neither a `.gitignore` + // negation nor `include` can resurface one here either. + if self.matches_structural(relative, is_dir) { + return true; } - // base == ignored: consider the include force-inclusion override. - // Precedence: a built-in skip is NEVER re-watched; an explicit `exclude` - // wins over `include`; otherwise an included dir (or an ancestor of an - // included path) is watched and an included file is handled. - if self.matches_rules(&self.rules[..self.builtin_rule_count], relative, is_dir) { + // 2. The negotiable last-match-wins stream, evaluated for EVERY path — + // which is what makes a configured `exclude` apply whether or not + // `include` is set. + if !self.matches_negotiable(relative, is_dir) { + return false; + } + // 3. Post-model include force-inclusion (#1063), mirroring + // `IncludeSet::forces`/`wants_descend`: a stream-ignored path returns + // iff `include` matches and no explicit `exclude` knocks it out. + if self.include.is_empty() { return true; } - if self - .exclude - .iter() - .any(|pattern| codegraph_extract::include_exclude_pattern_matches(pattern, relative)) - { + if self.matches_exclude(relative) { return true; } let force = if is_dir { @@ -176,15 +174,31 @@ impl WatchPolicy { !force } - fn matches_rules(&self, rules: &[IgnoreRule], relative: &str, is_dir: bool) -> bool { - let mut ignored = false; - for rule in rules { + fn matches_structural(&self, relative: &str, is_dir: bool) -> bool { + self.structural_ignores + .iter() + .any(|pattern| rule_matches(pattern, relative, is_dir)) + } + + /// Last-match-wins over `exclude` then `.gitignore`, the watcher's port of + /// the scan's `is_path_ignored` over its ordered `pattern_sets`. Each side + /// keeps its own matcher: `exclude` uses the SHARED whole-path matcher the + /// scan uses, `.gitignore` the watcher's [`rule_matches`] glob semantics. + fn matches_negotiable(&self, relative: &str, is_dir: bool) -> bool { + let mut ignored = self.matches_exclude(relative); + for rule in &self.gitignore_rules { if rule_matches(&rule.pattern, relative, is_dir) { ignored = !rule.negated; } } ignored } + + fn matches_exclude(&self, relative: &str) -> bool { + self.exclude + .iter() + .any(|pattern| codegraph_extract::include_exclude_pattern_matches(pattern, relative)) + } } /// Whether an include `pattern` matches the FILE at root-relative `relative` @@ -494,11 +508,21 @@ mod tests { } #[test] - fn gitignore_negation_reincludes_default_ignored_dir() { + fn gitignore_negation_reincludes_a_gitignored_dir_but_not_a_structural_skip() { + // A `.gitignore` negation is part of the NEGOTIABLE last-match-wins + // stream, so it flips back what an earlier `.gitignore` line dropped. It + // can NOT resurface a configured `ignore_dirs` entry (`vendor`, + // `node_modules`): `scan_dir` prunes those before any pattern set runs, + // so the watcher must prune them too. let dir = crate::sync::tests::TestDir::new("watch-policy-negation"); - fs::write(dir.path().join(".gitignore"), "!vendor/\n").unwrap(); + fs::write( + dir.path().join(".gitignore"), + "Tools/\n!Tools/\n!vendor/\n!node_modules/\n", + ) + .unwrap(); let policy = WatchPolicy::new(dir.path()); - assert!(policy.should_handle_file("vendor/first_party.ts")); + assert!(policy.should_handle_file("Tools/first_party.ts")); + assert!(!policy.should_handle_file("vendor/first_party.ts")); assert!(!policy.should_handle_file("node_modules/pkg/index.ts")); } @@ -790,7 +814,10 @@ mod tests { #[test] fn empty_include_leaves_policy_byte_identical() { // #1063: WatchPolicy::new equals with_config using the default ignore dirs - // and empty include/exclude; an empty include changes no policy decision. + // and empty include/exclude; an empty include adds no force-inclusion, so + // it changes no policy decision. It does NOT disable the `exclude` tier — + // that is evaluated for every path (see + // `configured_exclude_applies_with_empty_include` in tests/). let dir = crate::sync::tests::TestDir::new("watch-policy-include-empty"); fs::write(dir.path().join(".gitignore"), "vendor/\n").unwrap(); let ignore_dirs = codegraph_core::config::IndexingConfig::default().ignore_dirs; diff --git a/crates/codegraph-watch/tests/include_scan_watch_parity.rs b/crates/codegraph-watch/tests/include_scan_watch_parity.rs index 7da4ec4..9974e3a 100644 --- a/crates/codegraph-watch/tests/include_scan_watch_parity.rs +++ b/crates/codegraph-watch/tests/include_scan_watch_parity.rs @@ -109,3 +109,99 @@ fn gen_glob_indexes_and_watches_gitignored_file() { "gen* must keep the gen/ dir watchable" ); } + +#[test] +fn configured_exclude_applies_with_empty_include() { + let project = unique_project("exclude_empty_include"); + touch( + &project, + "src/generated.ts", + "export const generated = true;", + ); + + let exclude = vec!["src/generated.ts".to_string()]; + let options = ExtractOptions { + exclude: exclude.clone(), + ..ExtractOptions::default() + }; + let scanned = scan_project(&project, &options).expect("scan"); + let policy = WatchPolicy::with_config(&project, &options.ignore_dirs, &[], &exclude); + + assert!(!scanned.iter().any(|file| file == "src/generated.ts")); + assert!( + !policy.should_handle_file("src/generated.ts"), + "configured exclude must apply even when include is empty" + ); +} + +/// A configured `ignore_dirs` entry is STRUCTURAL: `scan_dir` prunes it with a +/// bare `continue` before any pattern set is evaluated, so a `.gitignore` +/// negation can never resurface it. The watcher must agree. +#[test] +fn gitignore_negation_cannot_reinclude_a_configured_ignore_dir() { + let project = unique_project("negate_ignore_dir"); + touch(&project, ".gitignore", "!addons/\n"); + touch(&project, "src/app.ts", "export const a = 1;"); + touch( + &project, + "addons/vendor_plugin/plugin.ts", + "export const p = 1;", + ); + + let options = ExtractOptions::default(); + assert!( + options.ignore_dirs.iter().any(|dir| dir == "addons"), + "this test needs `addons` to be a configured ignore dir" + ); + let scanned = scan_project(&project, &options).expect("scan"); + let policy = WatchPolicy::with_config(&project, &options.ignore_dirs, &[], &[]); + + assert!(scanned.iter().any(|file| file == "src/app.ts")); + assert!(policy.should_handle_file("src/app.ts")); + assert!( + !scanned + .iter() + .any(|file| file == "addons/vendor_plugin/plugin.ts"), + "scan must keep pruning a configured ignore dir despite `!addons/`: {scanned:?}" + ); + assert!( + !policy.should_watch_dir("addons"), + "a .gitignore negation must NOT re-include a configured ignore dir (parity with scan)" + ); + assert!( + !policy.should_handle_file("addons/vendor_plugin/plugin.ts"), + "a .gitignore negation must NOT re-include files under a configured ignore dir" + ); +} + +/// The precision boundary of the rule above: `exclude` and `.gitignore` DO share +/// one last-match-wins stream on both sides, so a later `!pattern` still +/// overrides an earlier `exclude` match. Guards against over-correcting the +/// structural rule into "nothing can ever be negated". +#[test] +fn gitignore_negation_still_overrides_a_configured_exclude() { + let project = unique_project("negate_exclude"); + touch(&project, ".gitignore", "!Tools/\n"); + touch(&project, "Tools/helper.ts", "export const t = 1;"); + + let exclude = vec!["Tools/".to_string()]; + let options = ExtractOptions { + exclude: exclude.clone(), + ..ExtractOptions::default() + }; + let scanned = scan_project(&project, &options).expect("scan"); + let policy = WatchPolicy::with_config(&project, &options.ignore_dirs, &[], &exclude); + + assert!( + scanned.iter().any(|file| file == "Tools/helper.ts"), + "a .gitignore negation must override an earlier exclude in the scan: {scanned:?}" + ); + assert!( + policy.should_watch_dir("Tools"), + "a .gitignore negation must override an earlier exclude for the watcher too" + ); + assert!( + policy.should_handle_file("Tools/helper.ts"), + "a .gitignore negation must override an earlier exclude for files as well" + ); +} From 870fcc72c5f2d0fd9cd33e73939112c84deb3923 Mon Sep 17 00:00:00 2001 From: sunerpy Date: Fri, 31 Jul 2026 09:19:57 +0800 Subject: [PATCH 09/18] fix(watch): honor the configured ignore_paths pattern set The watcher had no ignore_paths tier at all, so the nine default Android res/values* style patterns applied only to the scan. XML is extractable here, so res/values/strings.xml was excluded by index --force yet watched and synced, breaking sync output equalling index --force. Add ignore_paths as the FIRST set of the negotiable stream, ahead of exclude and gitignore, mirroring the scan's pattern_sets, and thread it from project config through WatchOptions the same way ignore_dirs already travels. Match it with the shared whole-path matcher rather than rule_matches: rule_matches compares basenames for a * pattern, so res/values* could never match res/values/strings.xml, which is the same divergence the shared matcher was introduced to end. A gitignore negation and an include can still override an ignore_paths match, as they can on the scan side. Also correct default_ignore_dirs' comment: a .gitignore negation cannot re-include .godot or addons, because scan_dir prunes every ignore_dirs entry before evaluating any pattern set. Dropping the entry from a custom indexing.ignore_dirs is the only re-inclusion path. --- crates/codegraph-core/src/config.rs | 6 +- crates/codegraph-watch/src/policy.rs | 89 ++++++-- crates/codegraph-watch/src/sync.rs | 11 +- crates/codegraph-watch/src/watcher.rs | 4 + .../tests/include_scan_watch_parity.rs | 199 +++++++++++++++++- 5 files changed, 277 insertions(+), 32 deletions(-) diff --git a/crates/codegraph-core/src/config.rs b/crates/codegraph-core/src/config.rs index b61654e..daa419b 100644 --- a/crates/codegraph-core/src/config.rs +++ b/crates/codegraph-core/src/config.rs @@ -161,8 +161,10 @@ fn default_ignore_dirs() -> Vec { ".dart_tool".to_string(), ".pub-cache".to_string(), // Godot — .godot is the regenerated engine import/cache dir (never source); - // addons holds vendored third-party editor plugins / GDScript. Both are - // re-includable via a .gitignore negation or a custom indexing.ignore_dirs. + // addons holds vendored third-party editor plugins / GDScript. Re-include + // either one by dropping it from a custom indexing.ignore_dirs; a + // .gitignore negation cannot, because scan_dir prunes every ignore_dirs + // entry before it evaluates any pattern set. ".godot".to_string(), "addons".to_string(), ] diff --git a/crates/codegraph-watch/src/policy.rs b/crates/codegraph-watch/src/policy.rs index 3b3549e..9b85530 100644 --- a/crates/codegraph-watch/src/policy.rs +++ b/crates/codegraph-watch/src/policy.rs @@ -35,10 +35,16 @@ pub struct WatchPolicy { /// `include` — exactly as `scan_dir` `continue`s on them before it evaluates /// any pattern set. structural_ignores: Vec, + /// The project's configured `ignore_paths` — the FIRST set of the NEGOTIABLE + /// last-match-wins stream, ahead of `exclude` and `.gitignore`, mirroring the + /// scan's `pattern_sets`. Matched with the SHARED whole-path matcher the scan + /// uses, never [`rule_matches`] — whose `*` form compares BASENAMES, so it + /// could never match a `res/values*` prefix against `res/values/strings.xml`. + ignore_paths: Vec, /// The root `.gitignore` rules in file order — the NEGOTIABLE tail of the - /// last-match-wins stream (`exclude` first, these second, mirroring the - /// scan's `pattern_sets`), so a `!pattern` line here re-includes what an - /// `exclude` or an earlier `.gitignore` line dropped. + /// last-match-wins stream (`ignore_paths` and `exclude` first, these last, + /// mirroring the scan's `pattern_sets`), so a `!pattern` line here re-includes + /// what an `ignore_paths`, `exclude`, or earlier `.gitignore` line dropped. gitignore_rules: Vec, include: Vec, exclude: Vec, @@ -51,16 +57,25 @@ pub struct WatchPolicy { impl WatchPolicy { pub fn new(root: impl AsRef) -> Self { let indexing = codegraph_core::config::IndexingConfig::default(); - Self::with_config(root, &indexing.ignore_dirs, &[], &[]) + Self::with_config( + root, + &indexing.ignore_dirs, + &indexing.ignore_paths, + &[], + &[], + ) } /// Like [`new`](Self::new) but threads the project's configured `ignore_dirs`, - /// `include`, and `exclude` scope into the watcher. Configured ignored dirs - /// mirror the scan's structural prune and cannot be re-included; an included - /// gitignored dir is watched, while an explicit `exclude` always wins. + /// `ignore_paths`, `include`, and `exclude` scope into the watcher. The two + /// ignore params are ordered by scan PRECEDENCE, not by kind: `ignore_dirs` + /// mirrors the scan's structural prune and cannot be re-included, while + /// `ignore_paths` opens the negotiable pattern stream a `.gitignore` negation + /// or an `include` can still override. An explicit `exclude` always wins. pub fn with_config( root: impl AsRef, ignore_dirs: &[String], + ignore_paths: &[String], include: &[String], exclude: &[String], ) -> Self { @@ -84,6 +99,7 @@ impl WatchPolicy { Self { root, structural_ignores, + ignore_paths: ignore_paths.to_vec(), gitignore_rules, include: include.to_vec(), exclude: exclude.to_vec(), @@ -180,12 +196,15 @@ impl WatchPolicy { .any(|pattern| rule_matches(pattern, relative, is_dir)) } - /// Last-match-wins over `exclude` then `.gitignore`, the watcher's port of - /// the scan's `is_path_ignored` over its ordered `pattern_sets`. Each side - /// keeps its own matcher: `exclude` uses the SHARED whole-path matcher the - /// scan uses, `.gitignore` the watcher's [`rule_matches`] glob semantics. + /// Last-match-wins over `ignore_paths`, then `exclude`, then `.gitignore` — + /// the watcher's port of the scan's `is_path_ignored` over its ordered + /// `pattern_sets`. Each side keeps its own matcher: the two CONFIG sets use + /// the SHARED whole-path matcher the scan uses, `.gitignore` the watcher's + /// [`rule_matches`] glob semantics. The two config sets are combined with + /// `||` because neither is negation-bearing here, which makes their relative + /// order unobservable; only the `.gitignore` fold has to run last. fn matches_negotiable(&self, relative: &str, is_dir: bool) -> bool { - let mut ignored = self.matches_exclude(relative); + let mut ignored = self.matches_ignore_paths(relative) || self.matches_exclude(relative); for rule in &self.gitignore_rules { if rule_matches(&rule.pattern, relative, is_dir) { ignored = !rule.negated; @@ -194,6 +213,15 @@ impl WatchPolicy { ignored } + /// Deliberately matched WITHOUT `is_dir`, exactly as the scan feeds a bare + /// `relative` to `is_path_ignored`, so a `res/values*` prefix drops the + /// `res/values` DIRECTORY and its files on identical terms. + fn matches_ignore_paths(&self, relative: &str) -> bool { + self.ignore_paths + .iter() + .any(|pattern| codegraph_extract::include_exclude_pattern_matches(pattern, relative)) + } + fn matches_exclude(&self, relative: &str) -> bool { self.exclude .iter() @@ -561,9 +589,15 @@ mod tests { // explicitly included by config. let dir = crate::sync::tests::TestDir::new("watch-policy-addons-include"); assert!(!dir.path().join(".gitignore").exists()); - let ignore_dirs = codegraph_core::config::IndexingConfig::default().ignore_dirs; + let indexing = codegraph_core::config::IndexingConfig::default(); let include = ["addons/first_party/**".to_string()]; - let included = WatchPolicy::with_config(dir.path(), &ignore_dirs, &include, &[]); + let included = WatchPolicy::with_config( + dir.path(), + &indexing.ignore_dirs, + &indexing.ignore_paths, + &include, + &[], + ); // When/Then: configured ignored dirs are structural scan skips, so include // cannot resurface the directory or any source file below it. @@ -579,11 +613,13 @@ mod tests { // retaining `.godot`, mirroring the scan-side override contract. let dir = crate::sync::tests::TestDir::new("watch-policy-addons-override"); assert!(!dir.path().join(".gitignore").exists()); - let mut ignore_dirs = codegraph_core::config::IndexingConfig::default().ignore_dirs; + let indexing = codegraph_core::config::IndexingConfig::default(); + let mut ignore_dirs = indexing.ignore_dirs.clone(); ignore_dirs.retain(|entry| entry != "addons"); // When: the watcher builds its policy from that project's configured list. - let policy = WatchPolicy::with_config(dir.path(), &ignore_dirs, &[], &[]); + let policy = + WatchPolicy::with_config(dir.path(), &ignore_dirs, &indexing.ignore_paths, &[], &[]); // Then: first-party addons are watched, while the retained Godot cache skip // remains structurally ignored. @@ -774,10 +810,11 @@ mod tests { // exclude still wins. let dir = crate::sync::tests::TestDir::new("watch-policy-include"); fs::write(dir.path().join(".gitignore"), "Tools/\nLocal/\n").unwrap(); - let ignore_dirs = codegraph_core::config::IndexingConfig::default().ignore_dirs; + let indexing = codegraph_core::config::IndexingConfig::default(); let policy = WatchPolicy::with_config( dir.path(), - &ignore_dirs, + &indexing.ignore_dirs, + &indexing.ignore_paths, &["Tools/".to_string(), "Local/ts/app.ts".to_string()], &[], ); @@ -794,7 +831,8 @@ mod tests { // A built-in skip is NEVER re-watched, even if named in include. let builtin = WatchPolicy::with_config( dir.path(), - &ignore_dirs, + &indexing.ignore_dirs, + &indexing.ignore_paths, &["node_modules/".to_string()], &[], ); @@ -803,7 +841,8 @@ mod tests { // An explicit exclude wins over include. let excluded = WatchPolicy::with_config( dir.path(), - &ignore_dirs, + &indexing.ignore_dirs, + &indexing.ignore_paths, &["Tools/".to_string()], &["Tools/".to_string()], ); @@ -820,8 +859,14 @@ mod tests { // `configured_exclude_applies_with_empty_include` in tests/). let dir = crate::sync::tests::TestDir::new("watch-policy-include-empty"); fs::write(dir.path().join(".gitignore"), "vendor/\n").unwrap(); - let ignore_dirs = codegraph_core::config::IndexingConfig::default().ignore_dirs; - let policy = WatchPolicy::with_config(dir.path(), &ignore_dirs, &[], &[]); + let indexing = codegraph_core::config::IndexingConfig::default(); + let policy = WatchPolicy::with_config( + dir.path(), + &indexing.ignore_dirs, + &indexing.ignore_paths, + &[], + &[], + ); assert!(!policy.should_watch_dir("vendor")); assert!(!policy.should_handle_file("vendor/dep.ts")); assert!(!policy.should_watch_dir("node_modules")); diff --git a/crates/codegraph-watch/src/sync.rs b/crates/codegraph-watch/src/sync.rs index a49bcb9..d0ac18d 100644 --- a/crates/codegraph-watch/src/sync.rs +++ b/crates/codegraph-watch/src/sync.rs @@ -374,9 +374,14 @@ fn sync_paths_with_store( started: std::time::Instant, mut on_progress: impl FnMut(usize, usize), ) -> Result { - let policy = - WatchPolicy::with_config(project_root, &scope.options.ignore_dirs, include, exclude) - .with_extension_overrides(Arc::clone(&scope.options.extensions)); + let policy = WatchPolicy::with_config( + project_root, + &scope.options.ignore_dirs, + &scope.options.ignore_paths, + include, + exclude, + ) + .with_extension_overrides(Arc::clone(&scope.options.extensions)); let mut outcome = SyncOutcome::default(); let mut changed = false; let mut seen = HashSet::new(); diff --git a/crates/codegraph-watch/src/watcher.rs b/crates/codegraph-watch/src/watcher.rs index 4451ea2..d6ab89b 100644 --- a/crates/codegraph-watch/src/watcher.rs +++ b/crates/codegraph-watch/src/watcher.rs @@ -304,6 +304,7 @@ pub struct WatchOptions { /// The addressed project's `config.toml` indexing scope. Threaded into the /// [`WatchPolicy`] and default sync functions so watcher scope matches scan. pub ignore_dirs: Vec, + pub ignore_paths: Vec, pub include: Vec, pub exclude: Vec, /// The addressed project's custom extension→language overrides (its @@ -334,6 +335,7 @@ impl Default for WatchOptions { on_degraded: None, on_sync_error: None, ignore_dirs: indexing.ignore_dirs, + ignore_paths: indexing.ignore_paths, include: indexing.include, exclude: indexing.exclude, extensions: ExtensionOverrides::empty(), @@ -365,6 +367,7 @@ impl WatchOptions { debounce: debounce_from_env_or(config.watch.debounce_ms), no_watch: !config.watch.enabled, ignore_dirs: config.indexing.ignore_dirs.clone(), + ignore_paths: config.indexing.ignore_paths.clone(), include: config.indexing.include.clone(), exclude: config.indexing.exclude.clone(), extensions, @@ -425,6 +428,7 @@ impl ProjectWatcher { let policy = WatchPolicy::with_config( &project_root, &options.ignore_dirs, + &options.ignore_paths, &options.include, &options.exclude, ) diff --git a/crates/codegraph-watch/tests/include_scan_watch_parity.rs b/crates/codegraph-watch/tests/include_scan_watch_parity.rs index 9974e3a..957a07b 100644 --- a/crates/codegraph-watch/tests/include_scan_watch_parity.rs +++ b/crates/codegraph-watch/tests/include_scan_watch_parity.rs @@ -66,7 +66,13 @@ fn scan_and_watch_agree_on_include_file_verdicts() { ..ExtractOptions::default() }; let scanned = scan_project(&project, &options).expect("scan"); - let policy = WatchPolicy::with_config(&project, &options.ignore_dirs, &include, &[]); + let policy = WatchPolicy::with_config( + &project, + &options.ignore_dirs, + &options.ignore_paths, + &include, + &[], + ); for file in candidate_files { let in_scan = scanned.iter().any(|f| f == file); @@ -99,7 +105,13 @@ fn gen_glob_indexes_and_watches_gitignored_file() { "gen* must index gen/helper.ts: {scanned:?}" ); - let policy = WatchPolicy::with_config(&project, &options.ignore_dirs, &include, &[]); + let policy = WatchPolicy::with_config( + &project, + &options.ignore_dirs, + &options.ignore_paths, + &include, + &[], + ); assert!( policy.should_handle_file("gen/helper.ts"), "gen* must watch gen/helper.ts (parity with scan)" @@ -125,7 +137,13 @@ fn configured_exclude_applies_with_empty_include() { ..ExtractOptions::default() }; let scanned = scan_project(&project, &options).expect("scan"); - let policy = WatchPolicy::with_config(&project, &options.ignore_dirs, &[], &exclude); + let policy = WatchPolicy::with_config( + &project, + &options.ignore_dirs, + &options.ignore_paths, + &[], + &exclude, + ); assert!(!scanned.iter().any(|file| file == "src/generated.ts")); assert!( @@ -154,7 +172,13 @@ fn gitignore_negation_cannot_reinclude_a_configured_ignore_dir() { "this test needs `addons` to be a configured ignore dir" ); let scanned = scan_project(&project, &options).expect("scan"); - let policy = WatchPolicy::with_config(&project, &options.ignore_dirs, &[], &[]); + let policy = WatchPolicy::with_config( + &project, + &options.ignore_dirs, + &options.ignore_paths, + &[], + &[], + ); assert!(scanned.iter().any(|file| file == "src/app.ts")); assert!(policy.should_handle_file("src/app.ts")); @@ -174,6 +198,165 @@ fn gitignore_negation_cannot_reinclude_a_configured_ignore_dir() { ); } +/// The `ignore_paths` tier — the FIRST set in the scan's ordered `pattern_sets` +/// (`ignore_paths` → `exclude` → `.gitignore`). XML is an extractable language +/// here (Tier-2 embedded / MyBatis mapper), so without this tier the watcher +/// would keep syncing an Android `res/values/strings.xml` that `index --force` +/// never indexed — breaking "sync == index --force". +#[test] +fn default_ignore_paths_exclude_android_res_from_scan_and_watch() { + let project = unique_project("ignore_paths_res"); + touch(&project, "src/main/java/App.java", "class App {}\n"); + touch( + &project, + "res/values/strings.xml", + "\n", + ); + + let options = ExtractOptions::default(); + assert!( + options.ignore_paths.iter().any(|p| p == "res/values*"), + "this test needs the default `res/values*` ignore path: {:?}", + options.ignore_paths + ); + let scanned = scan_project(&project, &options).expect("scan"); + let policy = WatchPolicy::with_config( + &project, + &options.ignore_dirs, + &options.ignore_paths, + &[], + &[], + ); + + assert!( + scanned.iter().any(|file| file == "src/main/java/App.java"), + "a normal source file must still be indexed: {scanned:?}" + ); + assert!( + policy.should_handle_file("src/main/java/App.java"), + "a normal source file must still be watched" + ); + assert!( + !scanned.iter().any(|file| file == "res/values/strings.xml"), + "the default `res/values*` ignore path must exclude it from the scan: {scanned:?}" + ); + assert!( + !policy.should_handle_file("res/values/strings.xml"), + "the default `res/values*` ignore path must also stop the watcher (parity with scan)" + ); +} + +/// The precision boundary of the tier above: `default_ignore_paths` deliberately +/// preserves `res/raw/` (real assets) and MyBatis mapper XML under +/// `src/main/resources/`. Guards against over-excluding with a broader rule. +#[test] +fn default_ignore_paths_preserve_res_raw_and_resources_in_scan_and_watch() { + let project = unique_project("ignore_paths_preserve"); + touch(&project, "res/raw/config.xml", "\n"); + touch( + &project, + "src/main/resources/mapper/UserMapper.xml", + "\n", + ); + + let options = ExtractOptions::default(); + let scanned = scan_project(&project, &options).expect("scan"); + let policy = WatchPolicy::with_config( + &project, + &options.ignore_dirs, + &options.ignore_paths, + &[], + &[], + ); + + for file in [ + "res/raw/config.xml", + "src/main/resources/mapper/UserMapper.xml", + ] { + assert!( + scanned.iter().any(|scanned_file| scanned_file == file), + "{file} must stay indexed: {scanned:?}" + ); + assert!( + policy.should_handle_file(file), + "{file} must stay watched (parity with scan)" + ); + } +} + +/// `ignore_paths` is a PATTERN SET, not a structural prune: it shares the +/// negotiable last-match-wins stream with `.gitignore`, so a later `!res/values/` +/// re-includes what the default `res/values*` dropped — on both sides. +#[test] +fn gitignore_negation_still_overrides_a_default_ignore_path() { + let project = unique_project("negate_ignore_path"); + touch(&project, ".gitignore", "!res/values/\n"); + touch( + &project, + "res/values/strings.xml", + "\n", + ); + + let options = ExtractOptions::default(); + let scanned = scan_project(&project, &options).expect("scan"); + let policy = WatchPolicy::with_config( + &project, + &options.ignore_dirs, + &options.ignore_paths, + &[], + &[], + ); + + assert!( + scanned.iter().any(|file| file == "res/values/strings.xml"), + "`!res/values/` must override the default ignore path in the scan: {scanned:?}" + ); + assert!( + policy.should_watch_dir("res/values"), + "`!res/values/` must keep the dir watchable for the watcher too" + ); + assert!( + policy.should_handle_file("res/values/strings.xml"), + "`!res/values/` must override the default ignore path for the watcher too" + ); +} + +/// The other half of "pattern set, not structural prune": `include` force-inclusion +/// still wins over an `ignore_paths` match, because `IncludeSet::forces` re-checks +/// only `exclude`. A configured `ignore_dirs` entry stays non-re-includable. +#[test] +fn include_force_includes_a_default_ignore_path_match() { + let project = unique_project("include_ignore_path"); + touch( + &project, + "res/values/strings.xml", + "\n", + ); + + let include = vec!["res/values/**".to_string()]; + let options = ExtractOptions { + include: include.clone(), + ..ExtractOptions::default() + }; + let scanned = scan_project(&project, &options).expect("scan"); + let policy = WatchPolicy::with_config( + &project, + &options.ignore_dirs, + &options.ignore_paths, + &include, + &[], + ); + + assert!( + scanned.iter().any(|file| file == "res/values/strings.xml"), + "`include` must force-include an ignore_paths match in the scan: {scanned:?}" + ); + assert!( + policy.should_handle_file("res/values/strings.xml"), + "`include` must force-include an ignore_paths match for the watcher too" + ); +} + /// The precision boundary of the rule above: `exclude` and `.gitignore` DO share /// one last-match-wins stream on both sides, so a later `!pattern` still /// overrides an earlier `exclude` match. Guards against over-correcting the @@ -190,7 +373,13 @@ fn gitignore_negation_still_overrides_a_configured_exclude() { ..ExtractOptions::default() }; let scanned = scan_project(&project, &options).expect("scan"); - let policy = WatchPolicy::with_config(&project, &options.ignore_dirs, &[], &exclude); + let policy = WatchPolicy::with_config( + &project, + &options.ignore_dirs, + &options.ignore_paths, + &[], + &exclude, + ); assert!( scanned.iter().any(|file| file == "Tools/helper.ts"), From db421e49c64c1eac78ef19fad5729abb34cfb6ac Mon Sep 17 00:00:00 2001 From: sunerpy Date: Fri, 31 Jul 2026 09:40:59 +0800 Subject: [PATCH 10/18] fix(watch): honor pattern negation in every config ignore set matches_negotiable ORed the ignore_paths and exclude sets before folding gitignore, so a leading ! was honored only in gitignore and the order between the two config sets was unobservable. The scan's is_path_ignored strips ! in every set and lets the last matching pattern decide, so ignore_paths of ["gen/", "!gen/"] kept gen/helper.ts for index --force while the watcher pruned it. Fold the two config sets and gitignore through one accumulator in the scan's order, stripping ! per pattern and re-matching with that set's own matcher. The matcher split stays load-bearing and is now documented as such: config sets use the shared whole-path matcher, gitignore keeps the basename-glob rule_matches. matches_exclude keeps its plain any-match, because its remaining caller is the include stage, and the scan's IncludeSet::forces and wants_descend likewise any-match the raw exclude patterns without stripping !. A negated exclude has already cleared the verdict in the fold, so it never reaches that stage. --- crates/codegraph-watch/src/policy.rs | 56 ++++--- .../tests/include_scan_watch_parity.rs | 137 ++++++++++++++++++ 2 files changed, 176 insertions(+), 17 deletions(-) diff --git a/crates/codegraph-watch/src/policy.rs b/crates/codegraph-watch/src/policy.rs index 9b85530..9b0623d 100644 --- a/crates/codegraph-watch/src/policy.rs +++ b/crates/codegraph-watch/src/policy.rs @@ -196,15 +196,41 @@ impl WatchPolicy { .any(|pattern| rule_matches(pattern, relative, is_dir)) } - /// Last-match-wins over `ignore_paths`, then `exclude`, then `.gitignore` — - /// the watcher's port of the scan's `is_path_ignored` over its ordered - /// `pattern_sets`. Each side keeps its own matcher: the two CONFIG sets use - /// the SHARED whole-path matcher the scan uses, `.gitignore` the watcher's - /// [`rule_matches`] glob semantics. The two config sets are combined with - /// `||` because neither is negation-bearing here, which makes their relative - /// order unobservable; only the `.gitignore` fold has to run last. + /// The watcher's port of the scan's `is_path_ignored` fold over its ordered + /// `pattern_sets`: ONE accumulator, sets in order (`ignore_paths` → + /// `exclude` → `.gitignore`), patterns within a set in order, and the LAST + /// matching pattern decides. A leading `!` is therefore honored in EVERY + /// set, not just `.gitignore` — `ignore_paths = ["gen/", "!gen/"]` keeps + /// `gen/helper.ts` — and the set order is observable, so a `!` in `exclude` + /// re-includes an `ignore_paths` match but not the reverse. + /// + /// The ONLY deviation from the scan is which matcher each set uses, and it + /// is LOAD-BEARING — do not "simplify" this into a single matcher: + /// the two CONFIG sets use the SHARED whole-path + /// [`codegraph_extract::include_exclude_pattern_matches`] the scan itself + /// uses (its trailing-`*` form is a whole-path prefix, so `res/values*` + /// matches `res/values/strings.xml`), while `.gitignore` keeps the + /// watcher's basename-glob [`rule_matches`]. A `!` in a config set is + /// stripped and then re-matched with that set's OWN matcher, mirroring how + /// the scan strips then calls `pattern_matches`. The config sets are also + /// matched WITHOUT `is_dir`, exactly as the scan feeds a bare `relative` to + /// `is_path_ignored`; only the `.gitignore` fold is `is_dir`-aware. The + /// `.gitignore` rules arrive pre-parsed into [`IgnoreRule`] by + /// [`read_gitignore_rules`], so their `!` is already split off, whereas the + /// config sets are raw patterns with the `!` still inline. fn matches_negotiable(&self, relative: &str, is_dir: bool) -> bool { - let mut ignored = self.matches_ignore_paths(relative) || self.matches_exclude(relative); + let mut ignored = false; + for set in [&self.ignore_paths, &self.exclude] { + for pattern in set { + if let Some(negated) = pattern.strip_prefix('!') { + if codegraph_extract::include_exclude_pattern_matches(negated, relative) { + ignored = false; + } + } else if codegraph_extract::include_exclude_pattern_matches(pattern, relative) { + ignored = true; + } + } + } for rule in &self.gitignore_rules { if rule_matches(&rule.pattern, relative, is_dir) { ignored = !rule.negated; @@ -213,15 +239,11 @@ impl WatchPolicy { ignored } - /// Deliberately matched WITHOUT `is_dir`, exactly as the scan feeds a bare - /// `relative` to `is_path_ignored`, so a `res/values*` prefix drops the - /// `res/values` DIRECTORY and its files on identical terms. - fn matches_ignore_paths(&self, relative: &str) -> bool { - self.ignore_paths - .iter() - .any(|pattern| codegraph_extract::include_exclude_pattern_matches(pattern, relative)) - } - + /// The scan's `IncludeSet::forces`/`wants_descend` knockout check, which is a + /// PLAIN any-match over the raw `exclude` patterns — no `!` stripping and no + /// ordering, unlike the negotiable fold above. Keep it that way: a negated + /// `exclude` entry already cleared `ignored` in the fold, so such a path + /// never reaches the include stage at all. fn matches_exclude(&self, relative: &str) -> bool { self.exclude .iter() diff --git a/crates/codegraph-watch/tests/include_scan_watch_parity.rs b/crates/codegraph-watch/tests/include_scan_watch_parity.rs index 957a07b..3609326 100644 --- a/crates/codegraph-watch/tests/include_scan_watch_parity.rs +++ b/crates/codegraph-watch/tests/include_scan_watch_parity.rs @@ -394,3 +394,140 @@ fn gitignore_negation_still_overrides_a_configured_exclude() { "a .gitignore negation must override an earlier exclude for files as well" ); } + +/// `ignore_paths` is a `.gitignore`-STYLE ordered set, not a flat any-match: +/// `is_path_ignored` strips a leading `!` in EVERY pattern set it folds, so a +/// later `!gen/` re-includes what an earlier `gen/` in the SAME set dropped. +#[test] +fn negated_ignore_path_reincludes_within_the_same_set() { + let project = unique_project("negate_within_ignore_paths"); + touch(&project, "gen/helper.ts", "export const g = 1;"); + + let ignore_paths = vec!["gen/".to_string(), "!gen/".to_string()]; + let options = ExtractOptions { + ignore_paths, + ..ExtractOptions::default() + }; + let scanned = scan_project(&project, &options).expect("scan"); + let policy = WatchPolicy::with_config( + &project, + &options.ignore_dirs, + &options.ignore_paths, + &[], + &[], + ); + + assert!( + scanned.iter().any(|file| file == "gen/helper.ts"), + "a later `!gen/` in ignore_paths must re-include it in the scan: {scanned:?}" + ); + assert!( + policy.should_watch_dir("gen"), + "a later `!gen/` in ignore_paths must keep the gen/ dir watchable (parity with scan)" + ); + assert!( + policy.should_handle_file("gen/helper.ts"), + "a later `!gen/` in ignore_paths must re-include the file for the watcher too" + ); +} + +/// Same ordered-set contract for the SECOND pattern set: a `!` is honored inside +/// `exclude` too, so `exclude = ["gen/", "!gen/"]` leaves `gen/helper.ts` in. +#[test] +fn negated_exclude_reincludes_within_the_same_set() { + let project = unique_project("negate_within_exclude"); + touch(&project, "gen/helper.ts", "export const g = 1;"); + + let exclude = vec!["gen/".to_string(), "!gen/".to_string()]; + let options = ExtractOptions { + exclude: exclude.clone(), + ..ExtractOptions::default() + }; + let scanned = scan_project(&project, &options).expect("scan"); + let policy = WatchPolicy::with_config( + &project, + &options.ignore_dirs, + &options.ignore_paths, + &[], + &exclude, + ); + + assert!( + scanned.iter().any(|file| file == "gen/helper.ts"), + "a later `!gen/` in exclude must re-include it in the scan: {scanned:?}" + ); + assert!( + policy.should_watch_dir("gen"), + "a later `!gen/` in exclude must keep the gen/ dir watchable (parity with scan)" + ); + assert!( + policy.should_handle_file("gen/helper.ts"), + "a later `!gen/` in exclude must re-include the file for the watcher too" + ); +} + +/// The set ORDER is observable, which a `||` of the two config sets cannot +/// express: `ignore_paths` folds before `exclude`, so a `!` in `exclude` +/// re-includes an `ignore_paths` match, while the mirrored config (`!` in +/// `ignore_paths`, plain pattern in `exclude`) stays excluded. +#[test] +fn config_set_order_decides_which_negation_wins() { + let project = unique_project("negate_across_sets"); + touch(&project, "gen/helper.ts", "export const g = 1;"); + touch(&project, "out/helper.ts", "export const o = 1;"); + + let later_negation = ExtractOptions { + ignore_paths: vec!["gen/".to_string(), "out/".to_string()], + exclude: vec!["!gen/".to_string()], + ..ExtractOptions::default() + }; + let scanned = scan_project(&project, &later_negation).expect("scan"); + let policy = WatchPolicy::with_config( + &project, + &later_negation.ignore_dirs, + &later_negation.ignore_paths, + &[], + &later_negation.exclude, + ); + + assert!( + scanned.iter().any(|file| file == "gen/helper.ts"), + "a `!gen/` in the later exclude set must re-include the ignore_paths match: {scanned:?}" + ); + assert!( + policy.should_handle_file("gen/helper.ts"), + "a `!gen/` in the later exclude set must re-include it for the watcher too" + ); + assert!( + !scanned.iter().any(|file| file == "out/helper.ts"), + "the un-negated `out/` ignore path must stay excluded from the scan: {scanned:?}" + ); + assert!( + !policy.should_handle_file("out/helper.ts"), + "the un-negated `out/` ignore path must stay excluded for the watcher too" + ); + + let earlier_negation = ExtractOptions { + ignore_paths: vec!["!gen/".to_string()], + exclude: vec!["gen/".to_string()], + ..ExtractOptions::default() + }; + let scanned = scan_project(&project, &earlier_negation).expect("scan"); + let policy = WatchPolicy::with_config( + &project, + &earlier_negation.ignore_dirs, + &earlier_negation.ignore_paths, + &[], + &earlier_negation.exclude, + ); + + assert!( + !scanned.iter().any(|file| file == "gen/helper.ts"), + "a `!gen/` in the EARLIER ignore_paths set must not survive the later exclude: {scanned:?}" + ); + assert!( + !policy.should_handle_file("gen/helper.ts"), + "a `!gen/` in the EARLIER ignore_paths set must not survive the later exclude for the \ + watcher either" + ); +} From 9e3005fda0cc1665542754fbac5f320db0a3eb19 Mon Sep 17 00:00:00 2001 From: sunerpy Date: Fri, 31 Jul 2026 11:46:13 +0800 Subject: [PATCH 11/18] fix(cli): count Godot resource referrers in impact edgeCount impact --json collected edgeCount only from the graph traversal, while the Godot reverse referrers were appended to affected through a separate path-keyed lane. A resource target therefore reported affected=13 with edgeCount=0, so an automation reading edgeCount to decide whether a dependency path exists saw none where twelve .tres files referenced the script. Take the referrer count from the already sorted and deduped set that feeds affected, add it to the traversal edges, and report it separately as resourceEdgeCount. Sharing one deduped set keeps edgeCount from ever contradicting affected, and a pure-code target keeps its previous edgeCount value with resourceEdgeCount zero. Reported by the Godot team against 0.40.4 as a JSON contract problem rather than a missing edge. --- crates/codegraph-cli/src/main.rs | 4 +- .../codegraph-cli/tests/impact_edge_count.rs | 133 ++++++++++++++++++ 2 files changed, 136 insertions(+), 1 deletion(-) create mode 100644 crates/codegraph-cli/tests/impact_edge_count.rs diff --git a/crates/codegraph-cli/src/main.rs b/crates/codegraph-cli/src/main.rs index 8eb97a2..226e064 100644 --- a/crates/codegraph-cli/src/main.rs +++ b/crates/codegraph-cli/src/main.rs @@ -3147,6 +3147,7 @@ fn cmd_impact( } godot_referrers.sort(); godot_referrers.dedup(); + let resource_edge_count = godot_referrers.len(); for from_file in godot_referrers { affected.push(NodeSummary { name: from_file.clone(), @@ -3161,7 +3162,8 @@ fn cmd_impact( "symbol": symbol, "depth": depth, "nodeCount": affected.len(), - "edgeCount": edge_keys.len(), + "edgeCount": edge_keys.len() + resource_edge_count, + "resourceEdgeCount": resource_edge_count, "affected": affected, "godotDynamic": godot.as_json(), }))?; diff --git a/crates/codegraph-cli/tests/impact_edge_count.rs b/crates/codegraph-cli/tests/impact_edge_count.rs new file mode 100644 index 0000000..2dd41ff --- /dev/null +++ b/crates/codegraph-cli/tests/impact_edge_count.rs @@ -0,0 +1,133 @@ +use std::fs; +use std::path::{Path, PathBuf}; +use std::process::Command; + +struct TestDir { + path: PathBuf, +} + +impl TestDir { + fn new(label: &str) -> Self { + let path = std::env::temp_dir().join(format!( + "codegraph-cli-impact-edge-count-{label}-{}-{}", + std::process::id(), + std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .unwrap() + .as_nanos() + )); + fs::create_dir_all(&path).unwrap(); + Self { path } + } + + fn path(&self) -> &Path { + &self.path + } +} + +impl Drop for TestDir { + fn drop(&mut self) { + let _ = fs::remove_dir_all(&self.path); + } +} + +fn cli(args: &[&str]) -> (String, String, bool) { + let output = Command::new(env!("CARGO_BIN_EXE_codegraph")) + .args(args) + .env("CODEGRAPH_NO_DAEMON", "1") + .env("CODEGRAPH_NO_WATCH", "1") + .output() + .expect("run codegraph binary"); + ( + String::from_utf8_lossy(&output.stdout).into_owned(), + String::from_utf8_lossy(&output.stderr).into_owned(), + output.status.success(), + ) +} + +fn index_project(project: &Path) { + let p = project.to_str().unwrap(); + let (out, err, ok) = cli(&["init", p]); + assert!(ok, "init failed: stdout={out} stderr={err}"); + let (out, err, ok) = cli(&["index", "--force", p]); + assert!(ok, "index --force failed: stdout={out} stderr={err}"); +} + +fn impact_json(project: &Path, symbol: &str) -> serde_json::Value { + let p = project.to_str().unwrap(); + let (stdout, err, ok) = cli(&["impact", symbol, "-p", p, "--depth", "4", "--json"]); + assert!(ok, "impact failed: stdout={stdout} stderr={err}"); + serde_json::from_str(&stdout).expect("impact emits valid JSON on stdout") +} + +#[test] +fn godot_resource_referrers_count_as_impact_edges() { + let dir = TestDir::new("godot"); + let project = dir.path().join("godot"); + fs::create_dir_all(&project).unwrap(); + fs::write( + project.join("project.godot"), + "[application]\nconfig/name=\"Impact edge count\"\n", + ) + .unwrap(); + fs::write( + project.join("weapon_data.gd"), + "class_name WeaponData\nextends Resource\n", + ) + .unwrap(); + for index in 0..12 { + fs::write( + project.join(format!("weapon_{index}.tres")), + "[gd_resource type=\"Resource\" load_steps=2 format=3]\n\ + \n\ + [ext_resource type=\"Script\" path=\"res://weapon_data.gd\" id=\"1_script\"]\n\ + \n\ + [resource]\n\ + script = ExtResource(\"1_script\")\n", + ) + .unwrap(); + } + index_project(&project); + + let value = impact_json(&project, "weapon_data.gd"); + let resource_referrers = value["affected"] + .as_array() + .expect("affected is an array") + .iter() + .filter(|node| { + node["filePath"] + .as_str() + .is_some_and(|path| path.ends_with(".tres")) + }) + .count() as u64; + let edge_count = value["edgeCount"].as_u64().expect("edgeCount is a number"); + + assert_eq!(edge_count, resource_referrers); + assert!(edge_count > 0, "resource referrers must contribute edges"); + assert_eq!( + value["resourceEdgeCount"] + .as_u64() + .expect("resourceEdgeCount is a number"), + resource_referrers + ); +} + +#[test] +fn pure_code_edge_count_is_unchanged() { + let dir = TestDir::new("pure-code"); + let project = dir.path().join("typescript"); + fs::create_dir_all(&project).unwrap(); + fs::write( + project.join("chain.ts"), + "export function helper(): number { return 1; }\n\ + export function caller(): number { return helper(); }\n\ + export function outer(): number { return caller(); }\n", + ) + .unwrap(); + index_project(&project); + + let value = impact_json(&project, "helper"); + + assert_eq!(value["edgeCount"].as_u64(), Some(2)); + assert_eq!(value["resourceEdgeCount"].as_u64(), Some(0)); +} From a89364ecb4e8e4c81786946c992245a449c7352f Mon Sep 17 00:00:00 2001 From: sunerpy Date: Fri, 31 Jul 2026 12:05:17 +0800 Subject: [PATCH 12/18] feat(daemon): register foreground stdio MCP processes in a PID-keyed registry A stdio serve --mcp process registered nowhere. The per-project daemon has daemon.pid and an HTTP server has an addr-keyed entry, but a foreground stdio process had no rendezvous, so neither codegraph http list nor codegraph status could name the processes holding an index. A partner report on Windows traced a failing index --force to three such invisible processes, and this machine accumulated thirteen. Key by pid, one .json per process, in a global state dir resolved like the HTTP registry but under an mcp leaf. Reads distinguish a missing directory, which means nobody has registered yet, from a directory that cannot be read, because the caller must be able to tell an empty registry from an outage. Dead-pid and unparseable entries are pruned on read. Registration covers the Direct and SpawnOrProxy modes, the foreground processes a user sees, and never BeDaemon, which already holds the daemon lock. A registry failure warns and serving continues, since MCP availability outranks observability, and no signal handler is installed because a crashed process's stale entry is what pruning exists for. The registry offers no terminate path: a pid outliving a crash may have been reused, and this crate cannot prove instance identity portably, so project, startedAt and version are recorded for a human to recognize a stale row. --- crates/codegraph-cli/src/main.rs | 51 +++ .../codegraph-cli/tests/rmcp_serve_direct.rs | 90 ++++ crates/codegraph-daemon/src/lib.rs | 1 + crates/codegraph-daemon/src/mcp_registry.rs | 398 ++++++++++++++++++ 4 files changed, 540 insertions(+) create mode 100644 crates/codegraph-daemon/src/mcp_registry.rs diff --git a/crates/codegraph-cli/src/main.rs b/crates/codegraph-cli/src/main.rs index 226e064..1265d0d 100644 --- a/crates/codegraph-cli/src/main.rs +++ b/crates/codegraph-cli/src/main.rs @@ -1918,6 +1918,7 @@ fn cmd_serve( emit_serve_startup_debug(&project_root, explicit_path, has_codegraph, &mode); match mode { ServeMode::Direct => { + let _registry = register_stdio_mcp_process(project.as_deref()); return serve_direct(project, &project_root, no_watch, explicit_path); } ServeMode::BeDaemon => { @@ -1939,6 +1940,7 @@ fn cmd_serve( .context("running as detached MCP daemon"); } ServeMode::SpawnOrProxy => { + let _registry = register_stdio_mcp_process(project.as_deref()); return serve_spawn_or_proxy(project, &project_root, no_watch, explicit_path); } } @@ -1949,6 +1951,55 @@ fn cmd_serve( Ok(()) } +/// Register THIS process in the global PID-keyed MCP registry and return the +/// guard that removes the entry when serving ends (stdin EOF is the ordinary +/// shutdown path). Registered from `Direct` and `SpawnOrProxy` — the FOREGROUND +/// stdio processes a user sees in their process list — and never from +/// `BeDaemon`, which already holds the per-project `daemon.pid` lock and would +/// otherwise be counted twice. +/// +/// A registry failure NEVER breaks `serve --mcp`: MCP availability outranks +/// observability, so an unwritable state dir is warned about and serving +/// continues unregistered. No signal handlers are installed either — a killed +/// or crashed process leaves a stale entry on purpose, which is exactly what +/// [`codegraph_daemon::mcp_registry::prune_dead`] self-heals on the next read. +fn register_stdio_mcp_process(project: Option<&Path>) -> Option { + use codegraph_daemon::mcp_registry::{self, McpServerInfo}; + + let pid = std::process::id(); + let info = McpServerInfo { + pid, + project: project.map(|path| path.display().to_string()), + transport: "stdio".to_string(), + started_at: mcp_registry::now_millis(), + version: VERSION.to_string(), + }; + match mcp_registry::write_entry(&info) { + Ok(_) => Some(McpRegistryGuard { pid }), + Err(error) => { + tracing::warn!( + %error, + "could not register this stdio MCP process; serving anyway (`codegraph mcp list` \ + will not show it)" + ); + None + } + } +} + +/// Best-effort removal of this process's own MCP registry entry on scope exit. +/// A crash is covered by the next read's prune, so this is a courtesy, not a +/// correctness requirement. +struct McpRegistryGuard { + pid: u32, +} + +impl Drop for McpRegistryGuard { + fn drop(&mut self) { + let _ = codegraph_daemon::mcp_registry::remove_entry(self.pid); + } +} + /// `serve --http`: serve MCP over streamable-HTTP (rmcp). Two modes selected by /// `--path`. With `--path`: PINNED — resolve the project (find-up), REQUIRE an /// on-disk index (hard-error otherwise — never self-index), and pin it as the diff --git a/crates/codegraph-cli/tests/rmcp_serve_direct.rs b/crates/codegraph-cli/tests/rmcp_serve_direct.rs index c0e0f59..260794c 100644 --- a/crates/codegraph-cli/tests/rmcp_serve_direct.rs +++ b/crates/codegraph-cli/tests/rmcp_serve_direct.rs @@ -7,6 +7,10 @@ //! non-empty, non-error tool result — proving the rmcp direct path serves the //! tools. Then closes stdin and confirms the process exits (stdin EOF → rmcp //! serve ends → block_on returns → exit). +//! +//! A second case reuses the same harness to cover the PID-keyed MCP registry: a +//! LIVE `serve --mcp` publishes a `.json` entry describing itself, and +//! stdin EOF removes it. #![cfg(unix)] use std::io::{BufRead, BufReader, Write}; @@ -197,6 +201,92 @@ fn serve_mcp_direct_routes_through_rmcp_handler() { ); } +#[test] +fn serve_mcp_direct_registers_its_pid_and_unregisters_on_stdin_eof() { + // GIVEN an indexed project served directly, with the PID-keyed MCP registry + // pointed at an isolated temp dir (never the developer's real state dir). + let home = TestDir::new("registry"); + let indexed = indexed_project(&home); + let registry_dir = home.path().join("mcp-registry"); + + let mut child = Command::new(bin()) + .args(["serve", "--mcp", "--path", indexed.to_str().unwrap()]) + .env("CODEGRAPH_NO_DAEMON", "1") + .env("CODEGRAPH_NO_WATCH", "1") + .env("CODEGRAPH_MCP_REGISTRY_DIR", ®istry_dir) + .stdin(Stdio::piped()) + .stdout(Stdio::piped()) + .stderr(Stdio::null()) + .spawn() + .expect("spawn serve --mcp (rmcp)"); + + let pid = child.id(); + let entry_path = registry_dir.join(format!("{pid}.json")); + let mut stdin = child.stdin.take().expect("child stdin"); + let mut stdout = BufReader::new(child.stdout.take().expect("child stdout")); + let deadline = Instant::now() + Duration::from_secs(20); + + // WHEN the server has answered `initialize` — proof it is past startup, so + // registration (which happens before the serve loop) has already landed. + let init = json!({ + "jsonrpc": "2.0", + "id": 1, + "method": "initialize", + "params": { + "protocolVersion": "2024-11-05", + "capabilities": {}, + "clientInfo": { "name": "rmcp-registry-test", "version": "0" } + } + }); + writeln!(stdin, "{init}").unwrap(); + stdin.flush().unwrap(); + let init_resp = read_json_line_with_id(&mut stdout, 1, deadline).expect("initialize result"); + assert_eq!( + init_resp["result"]["serverInfo"]["name"], + json!("codegraph") + ); + + // THEN a registry entry keyed by the live process's own pid describes it. + let raw = std::fs::read_to_string(&entry_path).unwrap_or_else(|error| { + panic!( + "a live `serve --mcp` must register {}: {error}", + entry_path.display() + ) + }); + let entry: Value = serde_json::from_str(&raw).expect("registry entry is valid JSON"); + assert_eq!( + entry["pid"], + json!(pid), + "the entry must carry the serving process's own pid: {raw}" + ); + assert_eq!(entry["transport"], json!("stdio"), "{raw}"); + let project = entry["project"].as_str().unwrap_or_default(); + assert!( + project.ends_with("mini"), + "the entry must record the resolved project path: {raw}" + ); + assert!( + entry["version"].as_str().is_some_and(|v| !v.is_empty()), + "the entry must record the binary version: {raw}" + ); + assert!( + entry["startedAt"].as_u64().is_some_and(|ms| ms > 0), + "startedAt is camelCase epoch millis: {raw}" + ); + + // AND closing stdin (EOF → serve ends → process exits) unregisters it. + drop(stdin); + assert!( + wait_with_timeout(&mut child, Duration::from_secs(10)), + "serve --mcp must exit after stdin EOF" + ); + assert!( + !entry_path.exists(), + "the registry entry must be removed on graceful shutdown: {}", + entry_path.display() + ); +} + /// Poll for process exit within `timeout`, returning whether it exited (killing /// it on timeout so the test process never leaks a child). fn wait_with_timeout(child: &mut std::process::Child, timeout: Duration) -> bool { diff --git a/crates/codegraph-daemon/src/lib.rs b/crates/codegraph-daemon/src/lib.rs index 3deac02..a965c1b 100644 --- a/crates/codegraph-daemon/src/lib.rs +++ b/crates/codegraph-daemon/src/lib.rs @@ -8,6 +8,7 @@ pub mod control; pub mod http_registry; mod lock; +pub mod mcp_registry; mod paths; mod process; pub mod proxy; diff --git a/crates/codegraph-daemon/src/mcp_registry.rs b/crates/codegraph-daemon/src/mcp_registry.rs new file mode 100644 index 0000000..d80c88e --- /dev/null +++ b/crates/codegraph-daemon/src/mcp_registry.rs @@ -0,0 +1,398 @@ +//! Global, PID-keyed registry for foreground stdio MCP processes. +//! +//! `serve --mcp` speaks MCP over stdio in the FOREGROUND — the process a user +//! actually sees in their process list. Unlike the per-project daemon (keyed by +//! `.codegraph-v2/daemon.pid` under the project root) and unlike an HTTP MCP +//! server (keyed by bind addr, see [`crate::http_registry`]), a stdio process +//! has no natural rendezvous of its own: several may serve the same project at +//! once, and one may serve no resolvable project at all. So it is keyed by PID, +//! one `.json` per process, in a GLOBAL state directory. +//! +//! This registry is PURE OBSERVABILITY — "who is running, since when, against +//! which project". It deliberately offers no terminate path: a PID whose entry +//! outlived a crash may have been reused by an unrelated process, and this +//! crate has no portable way to prove instance identity, so killing by +//! registered PID could hit an innocent process. `project` / `started_at` / +//! `version` exist so a HUMAN can recognize a stale row instead. +//! +//! Registry directory resolution (mirrors [`crate::http_registry`], with an +//! `mcp` leaf so the two registries never share a directory): +//! 1. `CODEGRAPH_MCP_REGISTRY_DIR` (explicit override — used by tests and +//! power users); +//! 2. `XDG_STATE_HOME/codegraph/mcp` when `XDG_STATE_HOME` is set; +//! 3. `$HOME/.local/state/codegraph/mcp` (unix / XDG fallback); +//! 4. `%LOCALAPPDATA%\codegraph\mcp` (windows), falling back to `USERPROFILE`. +//! +//! Reads distinguish "nobody registered yet" from "the registry could not be +//! read" — see [`RegistryRead`]. A MISSING directory is the former: it is the +//! normal state before the first `serve --mcp` ever runs. + +use std::fs; +use std::path::{Path, PathBuf}; +use std::time::{SystemTime, UNIX_EPOCH}; + +use anyhow::{Context, Result, anyhow}; +use serde::{Deserialize, Serialize}; + +use crate::process::is_process_alive; + +/// Explicit override for the registry directory (highest precedence). Tests set +/// this to an isolated temp dir so they never touch a developer's real state. +pub const CODEGRAPH_MCP_REGISTRY_DIR: &str = "CODEGRAPH_MCP_REGISTRY_DIR"; + +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +#[serde(rename_all = "camelCase")] +pub struct McpServerInfo { + pub pid: u32, + #[serde(skip_serializing_if = "Option::is_none", default)] + pub project: Option, + pub transport: String, + pub started_at: u64, + pub version: String, +} + +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum RegistryRead { + Available(Vec), + Unavailable { path: PathBuf, error: String }, +} + +/// Resolve the global registry directory (see module docs for precedence). +/// Does NOT create it — call [`ensure_registry_dir`] for that. +#[must_use] +pub fn registry_dir() -> PathBuf { + if let Some(explicit) = std::env::var_os(CODEGRAPH_MCP_REGISTRY_DIR) { + let raw = PathBuf::from(explicit); + if !raw.as_os_str().is_empty() { + return raw; + } + } + base_state_dir().join("codegraph").join("mcp") +} + +#[cfg(unix)] +fn base_state_dir() -> PathBuf { + if let Some(xdg) = non_empty_env("XDG_STATE_HOME") { + return PathBuf::from(xdg); + } + if let Some(home) = non_empty_env("HOME") { + return PathBuf::from(home).join(".local").join("state"); + } + // Last resort: a temp-dir bucket so we never write to `/`. + std::env::temp_dir().join("codegraph-state") +} + +#[cfg(windows)] +fn base_state_dir() -> PathBuf { + if let Some(local) = non_empty_env("LOCALAPPDATA") { + return PathBuf::from(local); + } + if let Some(profile) = non_empty_env("USERPROFILE") { + return PathBuf::from(profile).join("AppData").join("Local"); + } + std::env::temp_dir().join("codegraph-state") +} + +fn non_empty_env(key: &str) -> Option { + std::env::var(key).ok().filter(|value| !value.is_empty()) +} + +/// Create the registry directory (idempotent) and return it. +pub fn ensure_registry_dir() -> Result { + let dir = registry_dir(); + fs::create_dir_all(&dir) + .with_context(|| format!("creating MCP registry dir {}", dir.display()))?; + Ok(dir) +} + +/// Absolute path of the registry file for `pid` under `dir`. +#[must_use] +pub fn registry_file(dir: &Path, pid: u32) -> PathBuf { + dir.join(format!("{pid}.json")) +} + +/// Current epoch milliseconds (0 on a pre-1970 clock, never panics). +#[must_use] +pub fn now_millis() -> u64 { + SystemTime::now() + .duration_since(UNIX_EPOCH) + .map(|elapsed| u64::try_from(elapsed.as_millis()).unwrap_or(u64::MAX)) + .unwrap_or(0) +} + +/// Write (create/overwrite) the registry entry for `info.pid`. Creates the +/// registry dir if needed. Atomic-ish: writes a temp file then renames over the +/// final path so a concurrent reader never sees a partial record. +pub fn write_entry(info: &McpServerInfo) -> Result { + let dir = ensure_registry_dir()?; + let path = registry_file(&dir, info.pid); + let payload = format!("{}\n", serde_json::to_string_pretty(info)?); + let tmp = path.with_extension(format!("json.{}.tmp", std::process::id())); + fs::write(&tmp, &payload).with_context(|| format!("writing {}", tmp.display()))?; + fs::rename(&tmp, &path).with_context(|| format!("publishing {}", path.display()))?; + Ok(path) +} + +/// Read a single registry entry by pid, if present and parseable. +#[must_use] +pub fn read_entry(pid: u32) -> Option { + read_entry_file(®istry_file(®istry_dir(), pid)) +} + +fn read_entry_file(path: &Path) -> Option { + let raw = fs::read_to_string(path).ok()?; + serde_json::from_str::(raw.trim()).ok() +} + +/// What a directory scan found. `Missing` is NOT a failure: no registry +/// directory means no stdio MCP process has ever registered, which reads as an +/// empty-but-available registry. +enum DirScan { + Missing, + Files(Vec), + Failed(String), +} + +fn scan_registry_dir(dir: &Path) -> DirScan { + match fs::read_dir(dir) { + Ok(read) => { + let mut files: Vec = read + .flatten() + .map(|entry| entry.path()) + .filter(|path| path.extension().is_some_and(|ext| ext == "json")) + .collect(); + files.sort(); + DirScan::Files(files) + } + Err(error) if error.kind() == std::io::ErrorKind::NotFound => DirScan::Missing, + // Anything else (a FILE where the dir should be, a permission denial, an + // unreadable mount) is a genuine outage the caller must be able to tell + // apart from "nothing registered". + Err(error) => DirScan::Failed(error.to_string()), + } +} + +/// List EVERY registry entry (live or stale) without pruning. Sorted by pid for +/// deterministic output. +#[must_use] +pub fn list_entries() -> RegistryRead { + let dir = registry_dir(); + match scan_registry_dir(&dir) { + DirScan::Missing => RegistryRead::Available(Vec::new()), + DirScan::Failed(error) => RegistryRead::Unavailable { path: dir, error }, + DirScan::Files(files) => { + let mut entries: Vec = files + .iter() + .filter_map(|path| read_entry_file(path)) + .collect(); + entries.sort_by_key(|entry| entry.pid); + RegistryRead::Available(entries) + } + } +} + +/// Prune every registry entry whose pid is no longer alive (self-heal), plus any +/// unparseable file. Returns the pids that were pruned, sorted. A live entry is +/// never touched. `Err` means the registry directory itself could not be read. +pub fn prune_dead() -> Result> { + let dir = registry_dir(); + let files = match scan_registry_dir(&dir) { + DirScan::Missing => return Ok(Vec::new()), + DirScan::Failed(error) => { + return Err(anyhow!( + "reading MCP registry dir {}: {error}", + dir.display() + )); + } + DirScan::Files(files) => files, + }; + let mut pruned = Vec::new(); + for path in files { + match read_entry_file(&path) { + Some(info) if is_process_alive(info.pid) => {} + Some(info) => { + if fs::remove_file(&path).is_ok() { + pruned.push(info.pid); + } + } + None => { + // Unparseable/corrupt file: remove it so it stops shadowing a + // future healthy entry for the same pid. + let _ = fs::remove_file(&path); + } + } + } + pruned.sort_unstable(); + Ok(pruned) +} + +/// List entries after pruning dead ones — the canonical "what is running now". +#[must_use] +pub fn live_entries() -> RegistryRead { + // Pruning is best-effort here: [`list_entries`] below owns the + // Available-vs-Unavailable classification, so an unreadable registry is + // reported once, from one place, instead of twice with two error strings. + let _ = prune_dead(); + list_entries() +} + +/// Remove the registry entry for `pid` (best-effort; a missing file is already +/// the desired end state). Returns true when a file was removed. +pub fn remove_entry(pid: u32) -> bool { + fs::remove_file(registry_file(®istry_dir(), pid)).is_ok() +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::is_process_alive; + use std::fs; + use std::sync::{Mutex, MutexGuard}; + use std::time::{SystemTime, UNIX_EPOCH}; + + static ENV_LOCK: Mutex<()> = Mutex::new(()); + + struct TempRegistry { + dir: PathBuf, + _guard: MutexGuard<'static, ()>, + previous: Option, + } + + impl TempRegistry { + fn new(label: &str) -> Self { + let guard = ENV_LOCK.lock().unwrap_or_else(|error| error.into_inner()); + let unique = SystemTime::now() + .duration_since(UNIX_EPOCH) + .unwrap() + .as_nanos(); + let dir = std::env::temp_dir().join(format!( + "cg-mcp-reg-{label}-{}-{unique}", + std::process::id() + )); + fs::create_dir_all(&dir).unwrap(); + let previous = std::env::var_os(CODEGRAPH_MCP_REGISTRY_DIR); + // SAFETY: guarded by ENV_LOCK for the guard's lifetime. + unsafe { std::env::set_var(CODEGRAPH_MCP_REGISTRY_DIR, &dir) }; + Self { + dir, + _guard: guard, + previous, + } + } + } + + impl Drop for TempRegistry { + fn drop(&mut self) { + // SAFETY: guarded by ENV_LOCK for the guard's lifetime. + unsafe { + match &self.previous { + Some(value) => std::env::set_var(CODEGRAPH_MCP_REGISTRY_DIR, value), + None => std::env::remove_var(CODEGRAPH_MCP_REGISTRY_DIR), + } + } + if self.dir.is_dir() { + let _ = fs::remove_dir_all(&self.dir); + } else { + let _ = fs::remove_file(&self.dir); + } + } + } + + fn sample(pid: u32) -> McpServerInfo { + McpServerInfo { + pid, + project: Some("/work/project".to_string()), + transport: "stdio".to_string(), + started_at: 1_700_000_000_123, + version: "1.2.3-test".to_string(), + } + } + + fn available(result: RegistryRead) -> Vec { + match result { + RegistryRead::Available(entries) => entries, + RegistryRead::Unavailable { path, error } => { + panic!( + "registry unexpectedly unavailable at {}: {error}", + path.display() + ) + } + } + } + + fn pick_dead_pid() -> u32 { + let candidate = 4_000_000_000u32; + if is_process_alive(candidate) { + 0 + } else { + candidate + } + } + + #[test] + fn write_then_read_roundtrip_preserves_every_field() { + let _registry = TempRegistry::new("roundtrip"); + let info = sample(std::process::id()); + + write_entry(&info).unwrap(); + + assert_eq!(read_entry(info.pid), Some(info)); + } + + #[test] + fn live_entries_prunes_an_entry_whose_pid_is_dead() { + let registry = TempRegistry::new("dead"); + let dead_pid = pick_dead_pid(); + write_entry(&sample(dead_pid)).unwrap(); + + assert!(available(live_entries()).is_empty()); + assert!(!registry_file(®istry.dir, dead_pid).exists()); + } + + #[test] + fn corrupt_pid_json_does_not_poison_the_whole_list() { + let registry = TempRegistry::new("corrupt"); + let live = sample(std::process::id()); + write_entry(&live).unwrap(); + let corrupt = registry.dir.join("424242.json"); + fs::write(&corrupt, b"{ not valid json").unwrap(); + + assert_eq!(available(live_entries()), vec![live]); + assert!(!corrupt.exists(), "corrupt registry file must be pruned"); + } + + #[test] + fn missing_registry_directory_reads_as_available_and_empty() { + let registry = TempRegistry::new("missing"); + fs::remove_dir_all(®istry.dir).unwrap(); + + assert_eq!(live_entries(), RegistryRead::Available(Vec::new())); + } + + #[test] + fn unreadable_registry_path_reads_as_unavailable_not_empty() { + let registry = TempRegistry::new("unavailable"); + fs::remove_dir_all(®istry.dir).unwrap(); + fs::write(®istry.dir, b"not a directory").unwrap(); + + match live_entries() { + RegistryRead::Unavailable { path, error } => { + assert_eq!(path, registry.dir); + assert!(!error.is_empty()); + } + RegistryRead::Available(entries) => { + panic!("unreadable registry was reported as available: {entries:?}") + } + } + } + + #[test] + fn remove_entry_deletes_the_pid_file() { + let _registry = TempRegistry::new("remove"); + let pid = std::process::id(); + write_entry(&sample(pid)).unwrap(); + + assert!(remove_entry(pid)); + assert_eq!(read_entry(pid), None); + } +} From 77d8c02b190d888127ea3c038a73810fd60a1e6e Mon Sep 17 00:00:00 2001 From: sunerpy Date: Fri, 31 Jul 2026 12:21:00 +0800 Subject: [PATCH 13/18] feat(cli): add codegraph mcp list to surface stdio MCP processes The registry landed in the previous commit but nothing read it, so a user still had no way to answer the question the partner report raised: which process is holding this index. http list and status both stay silent about stdio servers. Add a mcp subcommand group mirroring http's wiring, with list printing pid, start time, version and project, plus the platform-appropriate command to stop a listed process. Guidance rather than a stop subcommand is deliberate: entries are pid-keyed, so a stale entry whose pid was reused would let us terminate an innocent process. McpAction stays an enum so stop can be added once process instance identity is provable portably. Also register the too-broad-root guard's exit, which serves tools off an existing index in the foreground exactly like Direct and was the one stdio exit left invisible. That is the IDE-launched CWD=$HOME case this command exists to surface. Every branch exits 0, including an unreadable registry, because failing a diagnostic command while the user is already debugging is hostile. The JSON keeps servers as an array in both cases and marks an outage with a separate key so a consumer never has to guess the shape. --- crates/codegraph-cli/src/main.rs | 121 ++++- .../codegraph-cli/tests/mcp_registry_cli.rs | 493 ++++++++++++++++++ 2 files changed, 610 insertions(+), 4 deletions(-) create mode 100644 crates/codegraph-cli/tests/mcp_registry_cli.rs diff --git a/crates/codegraph-cli/src/main.rs b/crates/codegraph-cli/src/main.rs index 1265d0d..8e66eba 100644 --- a/crates/codegraph-cli/src/main.rs +++ b/crates/codegraph-cli/src/main.rs @@ -479,6 +479,11 @@ enum Command { #[command(subcommand)] action: HttpAction, }, + /// Inspect the foreground stdio MCP processes started by `serve --mcp`. + Mcp { + #[command(subcommand)] + action: McpAction, + }, /// Print the codegraph version. Version, /// Generate shell completion scripts (bash, zsh, fish, powershell, elvish). @@ -584,6 +589,20 @@ enum HttpAction { Stop { addr: String }, } +/// Deliberately a single-variant enum: by decision A of the rev55 plan `stop` is +/// absent this round (a PID-keyed entry whose pid was reused would let us +/// terminate an innocent process), and the enum shape leaves room to add it once +/// process instance identity can be proven portably. +#[derive(Debug, Subcommand)] +enum McpAction { + /// List the running foreground stdio MCP processes (`serve --mcp`). + List { + /// Emit machine-readable JSON instead of the table. + #[arg(long)] + json: bool, + }, +} + fn run(cli: Cli) -> Result<()> { match cli.command { Command::Init { path, target } => cmd_init(path, &target), @@ -770,6 +789,9 @@ fn run(cli: Cli) -> Result<()> { HttpAction::Status { addr } => cmd_http_status(addr), HttpAction::Stop { addr } => cmd_http_stop(&addr), }, + Command::Mcp { action } => match action { + McpAction::List { json } => cmd_mcp_list(json), + }, Command::Version => { println!("codegraph {VERSION}"); Ok(()) @@ -1909,6 +1931,11 @@ fn cmd_serve( %reason, "no project root; tools still answer off an existing index if present" ); + // A FOREGROUND stdio process like `Direct`: it answers tool calls off + // any existing index, opening the DB per request, so it must be + // visible to `codegraph mcp list` too — this is exactly the + // IDE-launched `CWD=$HOME` case that command exists to surface. + let _registry = register_stdio_mcp_process(project.as_deref()); return serve_direct_no_services(project, &project_root, no_watch); } let has_codegraph = codegraph_dir(&project_root) @@ -1953,10 +1980,11 @@ fn cmd_serve( /// Register THIS process in the global PID-keyed MCP registry and return the /// guard that removes the entry when serving ends (stdin EOF is the ordinary -/// shutdown path). Registered from `Direct` and `SpawnOrProxy` — the FOREGROUND -/// stdio processes a user sees in their process list — and never from -/// `BeDaemon`, which already holds the per-project `daemon.pid` lock and would -/// otherwise be counted twice. +/// shutdown path). Registered from all THREE foreground stdio exits — `Direct`, +/// `SpawnOrProxy`, and the too-broad-root guard's `serve_direct_no_services` — +/// the processes a user sees in their process list, and never from `BeDaemon`, +/// which already holds the per-project `daemon.pid` lock and would otherwise be +/// counted twice. /// /// A registry failure NEVER breaks `serve --mcp`: MCP availability outranks /// observability, so an unwritable state dir is warned about and serving @@ -2302,6 +2330,91 @@ fn cmd_http_stop(addr: &str) -> Result<()> { Ok(()) } +/// `codegraph mcp list`: prune dead entries, then report the live FOREGROUND +/// stdio MCP processes (`serve --mcp`) — pid, project, start time, version — so +/// a user can tell WHO is holding an index and HOW to stop it. +/// +/// We deliberately print stop GUIDANCE instead of offering `mcp stop` (decision +/// A of the rev55 plan): entries are PID-keyed, so a stale entry whose pid was +/// reused by an unrelated process would make us terminate an innocent process, +/// and this workspace has no portable way to prove process instance identity. +/// +/// Every branch exits 0, including the unreadable-registry one: this is a +/// diagnostic command, and failing hard while the user is already debugging +/// would be hostile. +fn cmd_mcp_list(json: bool) -> Result<()> { + match codegraph_daemon::mcp_registry::live_entries() { + codegraph_daemon::mcp_registry::RegistryRead::Available(servers) => { + if json { + print_json_pretty(&json!({ "servers": servers })) + } else { + print_mcp_table(&servers); + Ok(()) + } + } + codegraph_daemon::mcp_registry::RegistryRead::Unavailable { path, error } => { + let path = path.display().to_string(); + if json { + // `servers` stays an array so a consumer never has to guess the + // shape; the extra key is what marks the outage. + print_json_pretty(&json!({ + "servers": [], + "registryUnavailable": { "path": path, "error": error }, + })) + } else { + println!("registry unavailable at {path}: {error}"); + println!( + "Cannot tell which stdio MCP servers are running. Find them with your OS \ + process tools (look for `codegraph serve --mcp`), then stop one with {}.", + mcp_stop_command() + ); + Ok(()) + } + } + } +} + +/// Render the live stdio MCP processes, or a friendly empty state. PROJECT goes +/// LAST and is never truncated: unlike the HTTP table (whose selector, ADDR, is +/// both short and first), here the project path is the field a human reads to +/// recognize a stale row, so clipping it would defeat the purpose. +fn print_mcp_table(servers: &[codegraph_daemon::mcp_registry::McpServerInfo]) { + if servers.is_empty() { + println!("No stdio MCP servers registered."); + println!( + "Note: older codegraph versions do not register; find those with your OS process \ + tools (look for `codegraph serve --mcp`)." + ); + return; + } + println!("{:>7} {:<27} {:<12} PROJECT", "PID", "STARTED", "VERSION"); + for info in servers { + println!( + "{:>7} {:<27} {:<12} {}", + info.pid, + format_started_at(info.started_at), + info.version, + info.project.as_deref().unwrap_or(""), + ); + } + println!( + "({} stdio MCP server(s) running). To stop one: {} — closing the client that launched it \ + is cleaner.", + servers.len(), + mcp_stop_command() + ); +} + +/// The platform-appropriate command a HUMAN runs to stop a listed process. See +/// [`cmd_mcp_list`] for why we only ever print this. +fn mcp_stop_command() -> &'static str { + if cfg!(windows) { + "taskkill /PID /F" + } else { + "kill " + } +} + /// Render the running-servers table, or a "none" line when empty. fn print_http_table(servers: &[codegraph_daemon::http_registry::HttpServerInfo]) { if servers.is_empty() { diff --git a/crates/codegraph-cli/tests/mcp_registry_cli.rs b/crates/codegraph-cli/tests/mcp_registry_cli.rs new file mode 100644 index 0000000..fbc0502 --- /dev/null +++ b/crates/codegraph-cli/tests/mcp_registry_cli.rs @@ -0,0 +1,493 @@ +//! `codegraph mcp list` — the READ side of the PID-keyed stdio MCP registry. +//! +//! Drives the real `codegraph` binary against an ISOLATED registry dir +//! (`CODEGRAPH_MCP_REGISTRY_DIR`) so it never touches a developer's real state. +//! Four cases cover the command itself: two live `serve --mcp` processes are +//! BOTH listed; an empty registry prints a friendly empty state and exits 0; an +//! UNREADABLE registry prints a visibly different diagnostic and still exits 0; +//! and `--json` emits a stable, parseable shape. +//! +//! A fifth case covers the fourth stdio exit of `cmd_serve`: the too-broad-root +//! ("home guard") early return also serves tools off any existing index in the +//! foreground, so it must register itself too — otherwise `mcp list` silently +//! misses exactly the Kiro-style `CWD=$HOME` launches this command exists to +//! surface. +//! +//! By decision A of the rev55 plan there is no `mcp stop`: entries are +//! PID-keyed, and a stale entry whose PID was reused would let us terminate an +//! innocent process. `list` therefore only PRINTS platform-appropriate stop +//! guidance, which the first case asserts. +#![cfg(unix)] + +use std::io::{BufRead, BufReader, Read, Write}; +use std::path::{Path, PathBuf}; +use std::process::{Child, Command, Stdio}; +use std::time::{Duration, Instant}; + +use serde_json::{Value, json}; + +fn bin() -> PathBuf { + PathBuf::from(env!("CARGO_BIN_EXE_codegraph")) +} + +fn workspace_root() -> PathBuf { + Path::new(env!("CARGO_MANIFEST_DIR")) + .parent() + .and_then(Path::parent) + .expect("codegraph-cli is under crates/") + .to_path_buf() +} + +fn mini_fixture() -> PathBuf { + workspace_root().join("crates/codegraph-bench/fixtures/mini") +} + +struct TestDir { + path: PathBuf, +} + +impl TestDir { + fn new(label: &str) -> Self { + let path = std::env::temp_dir().join(format!( + "codegraph-mcplist-{label}-{}-{}", + std::process::id(), + std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .unwrap() + .as_nanos() + )); + std::fs::create_dir_all(&path).unwrap(); + Self { path } + } + fn path(&self) -> &Path { + &self.path + } +} + +impl Drop for TestDir { + fn drop(&mut self) { + let _ = std::fs::remove_dir_all(&self.path); + } +} + +fn copy_tree(src: &Path, dst: &Path) { + std::fs::create_dir_all(dst).unwrap(); + for entry in std::fs::read_dir(src).unwrap() { + let entry = entry.unwrap(); + let from = entry.path(); + let to = dst.join(entry.file_name()); + if from.is_dir() { + copy_tree(&from, &to); + } else { + std::fs::copy(&from, &to).unwrap(); + } + } +} + +/// Copy the mini fixture to `/` and index it. Each live server in the +/// multi-instance case gets its OWN project so their background catch-up threads +/// never contend on one database. +fn indexed_project(dir: &TestDir, name: &str) -> PathBuf { + let project = dir.path().join(name); + copy_tree(&mini_fixture(), &project); + let status = Command::new(bin()) + .args(["init", project.to_str().unwrap()]) + .stdout(Stdio::null()) + .stderr(Stdio::null()) + .status() + .expect("run codegraph init"); + assert!(status.success(), "init failed for {}", project.display()); + project +} + +/// Run a `codegraph` command with the isolated MCP registry dir set. +fn run_cli(registry_dir: &Path, args: &[&str]) -> std::process::Output { + Command::new(bin()) + .args(args) + .env("CODEGRAPH_MCP_REGISTRY_DIR", registry_dir) + .env("CODEGRAPH_NO_DAEMON", "1") + .stdin(Stdio::null()) + .stdout(Stdio::piped()) + .stderr(Stdio::piped()) + .output() + .expect("run codegraph") +} + +fn read_json_line_with_id( + reader: &mut impl BufRead, + want_id: i64, + deadline: Instant, +) -> Option { + loop { + if Instant::now() > deadline { + return None; + } + let mut line = String::new(); + match reader.read_line(&mut line) { + Ok(0) => return None, + Ok(_) => { + let trimmed = line.trim(); + if trimmed.is_empty() { + continue; + } + if let Ok(value) = serde_json::from_str::(trimmed) + && value.get("id").and_then(Value::as_i64) == Some(want_id) + { + return Some(value); + } + } + Err(_) => return None, + } + } +} + +/// Poll for process exit within `timeout`, returning whether it exited (killing +/// it on timeout so the test process never leaks a child). +fn wait_with_timeout(child: &mut Child, timeout: Duration) -> bool { + let deadline = Instant::now() + timeout; + while Instant::now() < deadline { + match child.try_wait() { + Ok(Some(_)) => return true, + Ok(None) => std::thread::sleep(Duration::from_millis(50)), + Err(_) => return false, + } + } + let _ = child.kill(); + let _ = child.wait(); + false +} + +/// A spawned `serve --mcp` whose `initialize` round-trip has already completed — +/// the barrier proving startup (and therefore registration) has landed. No +/// sleeps, no polling on the filesystem. +struct LiveServer { + child: Child, + stdin: Option, +} + +impl LiveServer { + fn spawn(project: &Path, registry_dir: &Path) -> Self { + let child = Command::new(bin()) + .args(["serve", "--mcp", "--path", project.to_str().unwrap()]) + .env("CODEGRAPH_NO_DAEMON", "1") + .env("CODEGRAPH_NO_WATCH", "1") + .env("CODEGRAPH_MCP_REGISTRY_DIR", registry_dir) + .stdin(Stdio::piped()) + .stdout(Stdio::piped()) + .stderr(Stdio::null()) + .spawn() + .expect("spawn serve --mcp"); + Self::handshake(child) + } + + fn handshake(mut child: Child) -> Self { + let mut stdin = child.stdin.take().expect("child stdin"); + let mut stdout = BufReader::new(child.stdout.take().expect("child stdout")); + let init = json!({ + "jsonrpc": "2.0", + "id": 1, + "method": "initialize", + "params": { + "protocolVersion": "2024-11-05", + "capabilities": {}, + "clientInfo": { "name": "mcp-list-test", "version": "0" } + } + }); + writeln!(stdin, "{init}").unwrap(); + stdin.flush().unwrap(); + let deadline = Instant::now() + Duration::from_secs(20); + let response = + read_json_line_with_id(&mut stdout, 1, deadline).expect("initialize must be answered"); + assert_eq!(response["result"]["serverInfo"]["name"], json!("codegraph")); + Self { + child, + stdin: Some(stdin), + } + } + + fn pid(&self) -> u32 { + self.child.id() + } + + /// Close stdin (EOF → serve ends → process exits) and confirm it exited. + fn shutdown(mut self) -> bool { + self.stdin.take(); + wait_with_timeout(&mut self.child, Duration::from_secs(10)) + } +} + +impl Drop for LiveServer { + fn drop(&mut self) { + self.stdin.take(); + let _ = self.child.kill(); + let _ = self.child.wait(); + } +} + +/// PIDs read out of the text table's FIRST column (exact tokens, so a pid never +/// matches by accident inside a longer number). +fn table_pids(stdout: &str) -> Vec { + stdout + .lines() + .filter_map(|line| line.split_whitespace().next()) + .filter_map(|token| token.parse::().ok()) + .collect() +} + +fn seed_entry(registry_dir: &Path, pid: u32, project: &str) { + std::fs::create_dir_all(registry_dir).unwrap(); + let entry = json!({ + "pid": pid, + "project": project, + "transport": "stdio", + "startedAt": 1_700_000_000_123u64, + "version": "1.2.3-test", + }); + std::fs::write( + registry_dir.join(format!("{pid}.json")), + format!("{entry}\n"), + ) + .unwrap(); +} + +/// (a) TWO live `serve --mcp` processes are BOTH listed, with the platform's +/// stop command printed as guidance (we never stop them ourselves). +#[test] +fn list_shows_every_live_stdio_server() { + let home = TestDir::new("two-live"); + let registry_dir = home.path().join("mcp-registry"); + let project_a = indexed_project(&home, "mini-a"); + let project_b = indexed_project(&home, "mini-b"); + + let server_a = LiveServer::spawn(&project_a, ®istry_dir); + let server_b = LiveServer::spawn(&project_b, ®istry_dir); + let (pid_a, pid_b) = (server_a.pid(), server_b.pid()); + + let out = run_cli(®istry_dir, &["mcp", "list"]); + let stdout = String::from_utf8_lossy(&out.stdout).into_owned(); + assert!( + out.status.success(), + "mcp list must exit 0: stdout={stdout} stderr={}", + String::from_utf8_lossy(&out.stderr) + ); + + let pids = table_pids(&stdout); + assert!( + pids.contains(&pid_a) && pids.contains(&pid_b), + "mcp list must show BOTH live servers ({pid_a}, {pid_b}); saw {pids:?}: {stdout}" + ); + assert!( + stdout.contains("mini-a") && stdout.contains("mini-b"), + "each row must name the project it serves: {stdout}" + ); + assert!( + stdout.contains(env!("CARGO_PKG_VERSION")), + "each row must carry the binary version: {stdout}" + ); + // Decision A: guidance only, never a terminate path. + assert!( + stdout.contains("kill"), + "list must print the platform's stop command as GUIDANCE: {stdout}" + ); + + assert!(server_a.shutdown(), "server a must exit on stdin EOF"); + assert!(server_b.shutdown(), "server b must exit on stdin EOF"); +} + +/// (b) An EMPTY (but readable) registry is the normal pre-first-serve state: a +/// friendly empty line, exit 0, and none of the outage wording. +#[test] +fn list_empty_registry_is_friendly_and_exits_zero() { + let home = TestDir::new("empty"); + let registry_dir = home.path().join("mcp-registry"); + std::fs::create_dir_all(®istry_dir).unwrap(); + + let out = run_cli(®istry_dir, &["mcp", "list"]); + let stdout = String::from_utf8_lossy(&out.stdout).into_owned(); + assert!( + out.status.success(), + "mcp list on an empty registry must exit 0: stderr={}", + String::from_utf8_lossy(&out.stderr) + ); + assert!( + stdout.contains("No stdio MCP servers"), + "an empty registry must print a friendly empty state: {stdout:?}" + ); + assert!( + !stdout.contains("registry unavailable"), + "an EMPTY registry must not be reported as an outage: {stdout}" + ); +} + +/// (c) An UNREADABLE registry (a file where the directory should be) is a +/// distinguishable diagnostic — and still exits 0, because failing hard while +/// the user is already debugging would be hostile. +#[test] +fn list_unavailable_registry_is_distinguishable_and_exits_zero() { + let home = TestDir::new("unavailable"); + let registry_dir = home.path().join("mcp-registry"); + std::fs::write(®istry_dir, b"not a directory").unwrap(); + + let out = run_cli(®istry_dir, &["mcp", "list"]); + let stdout = String::from_utf8_lossy(&out.stdout).into_owned(); + assert!( + out.status.success(), + "mcp list is a diagnostic: an unreadable registry must still exit 0: stdout={stdout} stderr={}", + String::from_utf8_lossy(&out.stderr) + ); + assert!( + stdout.contains("registry unavailable") && stdout.contains(registry_dir.to_str().unwrap()), + "an unreadable registry must be reported with its path: {stdout:?}" + ); + assert!( + !stdout.contains("No stdio MCP servers"), + "an outage must NOT read like an empty registry: {stdout}" + ); +} + +/// (d) `--json` is a stable, parseable shape: always a `servers` array of the +/// registry's own camelCase fields, plus a `registryUnavailable` object ONLY +/// when the registry could not be read. +#[test] +fn list_json_shape_is_stable_and_parseable() { + let home = TestDir::new("json"); + let registry_dir = home.path().join("mcp-registry"); + // The test process's own pid is guaranteed alive, so the seeded entry + // survives the read-time prune. + let pid = std::process::id(); + seed_entry(®istry_dir, pid, "/work/project"); + + let out = run_cli(®istry_dir, &["mcp", "list", "--json"]); + let stdout = String::from_utf8_lossy(&out.stdout).into_owned(); + assert!( + out.status.success(), + "mcp list --json must exit 0: stdout={stdout} stderr={}", + String::from_utf8_lossy(&out.stderr) + ); + let value: Value = + serde_json::from_str(&stdout).expect("mcp list --json emits valid JSON on stdout"); + let servers = value["servers"] + .as_array() + .unwrap_or_else(|| panic!("`servers` must always be an array: {stdout}")); + assert_eq!(servers.len(), 1, "one live entry was seeded: {stdout}"); + assert_eq!(servers[0]["pid"], json!(pid), "{stdout}"); + assert_eq!(servers[0]["project"], json!("/work/project"), "{stdout}"); + assert_eq!(servers[0]["transport"], json!("stdio"), "{stdout}"); + assert_eq!( + servers[0]["startedAt"], + json!(1_700_000_000_123u64), + "startedAt stays camelCase epoch millis: {stdout}" + ); + assert_eq!(servers[0]["version"], json!("1.2.3-test"), "{stdout}"); + assert!( + value.get("registryUnavailable").is_none(), + "a readable registry must not carry the outage key: {stdout}" + ); + + // The SAME command on an unreadable registry keeps `servers` an array and + // adds the outage key, so a consumer never has to guess the shape. + let broken = TestDir::new("json-broken"); + let broken_dir = broken.path().join("mcp-registry"); + std::fs::write(&broken_dir, b"not a directory").unwrap(); + let out = run_cli(&broken_dir, &["mcp", "list", "--json"]); + let stdout = String::from_utf8_lossy(&out.stdout).into_owned(); + assert!(out.status.success(), "unreadable registry still exits 0"); + let value: Value = + serde_json::from_str(&stdout).expect("the outage branch is JSON too: {stdout}"); + assert_eq!( + value["servers"], + json!([]), + "`servers` stays an array in the outage branch: {stdout}" + ); + assert_eq!( + value["registryUnavailable"]["path"], + json!(broken_dir.to_str().unwrap()), + "{stdout}" + ); + assert!( + value["registryUnavailable"]["error"] + .as_str() + .is_some_and(|error| !error.is_empty()), + "the outage must carry a non-empty error: {stdout}" + ); +} + +/// (e) The FOURTH stdio exit — the too-broad-root ("home guard") early return — +/// is a foreground process serving tools off any existing index, so it must +/// register itself just like `Direct` does. Without this, `mcp list` misses the +/// Kiro-style `CWD=$HOME` launches it exists to surface. +#[test] +fn home_guard_serve_registers_itself() { + let home = TestDir::new("home-guard"); + let registry_dir = home.path().join("mcp-registry"); + // `too_broad_root_reason` fires on an EXACT $HOME match, so point HOME at + // the temp dir and serve that same dir. + let mut child = Command::new(bin()) + .args(["serve", "--mcp", "--path", home.path().to_str().unwrap()]) + .env("HOME", home.path()) + .env("CODEGRAPH_NO_DAEMON", "1") + .env("CODEGRAPH_NO_WATCH", "1") + .env("CODEGRAPH_MCP_REGISTRY_DIR", ®istry_dir) + .stdin(Stdio::piped()) + .stdout(Stdio::piped()) + .stderr(Stdio::piped()) + .spawn() + .expect("spawn serve --mcp at $HOME"); + + let pid = child.id(); + let entry_path = registry_dir.join(format!("{pid}.json")); + let mut stderr = child.stderr.take().expect("child stderr"); + let stderr_reader = std::thread::spawn(move || { + let mut buf = String::new(); + let _ = stderr.read_to_string(&mut buf); + buf + }); + let mut stdin = child.stdin.take().expect("child stdin"); + let mut stdout = BufReader::new(child.stdout.take().expect("child stdout")); + + let init = json!({ + "jsonrpc": "2.0", + "id": 1, + "method": "initialize", + "params": { + "protocolVersion": "2024-11-05", + "capabilities": {}, + "clientInfo": { "name": "mcp-home-guard-test", "version": "0" } + } + }); + writeln!(stdin, "{init}").unwrap(); + stdin.flush().unwrap(); + let deadline = Instant::now() + Duration::from_secs(20); + let response = read_json_line_with_id(&mut stdout, 1, deadline) + .expect("the home-guard path must still answer initialize"); + assert_eq!(response["result"]["serverInfo"]["name"], json!("codegraph")); + + // THEN it registered itself, exactly like the Direct arm does. + let raw = std::fs::read_to_string(&entry_path).unwrap_or_else(|error| { + panic!( + "the too-broad-root serve path must register {}: {error}", + entry_path.display() + ) + }); + let entry: Value = serde_json::from_str(&raw).expect("registry entry is valid JSON"); + assert_eq!(entry["pid"], json!(pid), "{raw}"); + assert_eq!(entry["transport"], json!("stdio"), "{raw}"); + + drop(stdin); + assert!( + wait_with_timeout(&mut child, Duration::from_secs(10)), + "serve --mcp must exit after stdin EOF" + ); + assert!( + !entry_path.exists(), + "the entry must be removed on graceful shutdown: {}", + entry_path.display() + ); + + // Proof the HOME-guard branch (not the Direct arm) served this session. + let logs = stderr_reader.join().expect("stderr reader thread"); + assert!( + logs.contains("home directory"), + "this session must have taken the too-broad-root branch: {logs}" + ); +} From 7a5712641a6a9d43707f28d629c0716e80e9bff6 Mon Sep 17 00:00:00 2001 From: sunerpy Date: Fri, 31 Jul 2026 13:56:30 +0800 Subject: [PATCH 14/18] feat(cli): warn before a rebuild when a stdio MCP server may hold the index index deletes codegraph.db and its sidecars before rewriting them, and on Windows that delete cannot succeed while another process has the file open. The partner report hit exactly this: three invisible serve --mcp processes held the database and the only clue was a bare remove_file error naming the path. Generate the who-holds-it guidance in a pure function so both of its branches are unit-testable, then call it from two places. Before the destructive rebuild, index warns when a registered stdio server could reach this project, listing each pid with the platform command to stop it. After a RemoveDatabase failure, the same text is appended to the CLI's presentation, leaving the store layer's statement of fact and the error chain untouched. A pinned entry only matches when the target lives at or under its project root; an entry with no project is always included, since it resolves per request and can open any database. Both sides canonicalize leniently so a symlinked ancestor does not split one project in two. The warning is strictly additive: it prints nothing when there is no holder to name, honors --quiet, and never changes the exit code, so a successful index against an empty registry emits its previous bytes exactly. An unreadable registry stays silent here and is surfaced on the failure path instead, where it is actionable. The RemoveDatabase wiring carries no integration test by design: RebuildFault exposes no database-removal hook, and a black-box attempt fails earlier, at database open. --- crates/codegraph-cli/src/main.rs | 299 ++++++++++++++++++ .../tests/index_holder_warning.rs | 281 ++++++++++++++++ 2 files changed, 580 insertions(+) create mode 100644 crates/codegraph-cli/tests/index_holder_warning.rs diff --git a/crates/codegraph-cli/src/main.rs b/crates/codegraph-cli/src/main.rs index 8e66eba..9b52a72 100644 --- a/crates/codegraph-cli/src/main.rs +++ b/crates/codegraph-cli/src/main.rs @@ -182,6 +182,9 @@ fn main() { if let Err(err) = run(cli) { eprintln!("Error: {err:#}"); + if let Some(guidance) = index_removal_holder_guidance(&err) { + eprint!("{guidance}"); + } std::process::exit(1); } } @@ -1433,6 +1436,9 @@ fn cmd_index(path: Option, force: bool, quiet: bool, verbose: bool) -> // reachable; authorization is still decided later under the exclusive lease. let project = resolve_required_rebuild_project(path)?; guard_indexable_root(&project)?; + if !quiet { + warn_if_stdio_mcp_may_hold_index(&project); + } // `--force` no longer removes the DB up front: the rebuild layer performs the // destructive removal itself, AFTER publishing `phase=building` under the one // outer exclusive lease, so an interruption can never leave a bare DB with no @@ -2415,6 +2421,299 @@ fn mcp_stop_command() -> &'static str { } } +/// PURE text generator for "a stdio MCP server may be holding this index +/// database" guidance. Inputs in, `String` out — no IO, no environment read, no +/// clock — which is what makes both of its branches unit-testable even though +/// one of its two call sites (the `RemoveDatabase` failure) cannot be driven +/// from a test (decision B of the rev55 plan: `RebuildFault` exposes no DB +/// removal hook, and a black-box attempt fails EARLIER, at database open). +/// +/// `holders` is the caller's already-narrowed set of registry entries that could +/// hold the database in question; `registry_unavailable` carries `(path, error)` +/// when the registry could not be read at all. +/// +/// An unreadable registry deliberately renders the SAME branch as an empty one: +/// informationally both mean "we found no holder and cannot prove there is +/// none", so the actionable advice is identical. The outage only adds a line +/// naming the path, so the user can still tell the two apart. +/// +/// Timestamps are NOT rendered here: [`format_started_at`] reads the local UTC +/// offset, which would make this function impure and its output TZ-dependent. +/// The text points at `codegraph mcp list` for start times instead. +fn index_holder_guidance( + holders: &[codegraph_daemon::mcp_registry::McpServerInfo], + registry_unavailable: Option<(&str, &str)>, +) -> String { + let mut out = String::new(); + if holders.is_empty() { + out.push_str( + "No registered codegraph stdio MCP server matches this project, but that does not \ + prove none is holding its index database:\n", + ); + if let Some((path, error)) = registry_unavailable { + out.push_str(&format!( + " - the stdio MCP registry at {path} could not be read ({error}), so this check \ + cannot tell either way;\n - even a readable registry can be empty because \ + nothing has registered yet;\n" + )); + } else { + out.push_str(" - nothing has registered yet, so the registry is empty;\n"); + } + out.push_str( + " - codegraph 0.40.x and earlier never register at all, so they never appear here.\n", + ); + } else { + out.push_str( + "Registered codegraph stdio MCP server(s) that may hold this project's index database \ + open:\n", + ); + for info in holders { + let scope = match info.project.as_deref() { + Some(project) => format!("project {project}"), + None => "any project (started without --path)".to_string(), + }; + out.push_str(&format!( + " - pid {} — {scope}, codegraph {}\n", + info.pid, info.version + )); + } + out.push_str( + "That list only covers servers that register: codegraph 0.40.x and earlier never did.\n", + ); + } + out.push_str(&format!( + "Look for `codegraph serve --mcp` with your OS process tools (`pgrep -af 'codegraph serve \ + --mcp'` on unix, `tasklist` or Task Manager on Windows) and stop the holder with `{}` — \ + closing the client that launched it is cleaner. `codegraph mcp list` shows every \ + registered server with its start time.\n", + mcp_stop_command() + )); + out +} + +/// Resolve `path` through the filesystem when possible, falling back to its +/// lexical form. `index` and `serve --mcp` both record LEXICALLY normalized +/// absolute paths, so without this a symlinked ancestor (`/tmp` -> +/// `/private/tmp`) would make one and the same project compare unequal. +fn canonicalize_lenient(path: &Path) -> PathBuf { + path.canonicalize() + .unwrap_or_else(|_| path.components().collect::()) +} + +/// The live stdio MCP registry entries that could be holding the index database +/// at `target`, plus `(path, error)` when the registry could not be read at all. +/// +/// An entry with NO project was launched without `--path` (the Kiro / Qoder +/// shape): it resolves its project per request, so it can open ANY project's +/// database and is always included — excluding it would hide exactly the launches +/// this check exists to surface. A pinned entry can only reach databases at or +/// under its own project root, so `target` must live beneath it; +/// `Path::starts_with` compares whole components, so `/w/proj` never matches +/// `/w/proj2`. +fn mcp_index_holders( + target: &Path, +) -> ( + Vec, + Option<(String, String)>, +) { + let canonical_target = canonicalize_lenient(target); + match codegraph_daemon::mcp_registry::live_entries() { + codegraph_daemon::mcp_registry::RegistryRead::Available(entries) => { + let holders = entries + .into_iter() + .filter(|entry| match entry.project.as_deref() { + None => true, + Some(project) => { + canonical_target.starts_with(canonicalize_lenient(Path::new(project))) + } + }) + .collect(); + (holders, None) + } + codegraph_daemon::mcp_registry::RegistryRead::Unavailable { path, error } => { + (Vec::new(), Some((path.display().to_string(), error))) + } + } +} + +/// Warn BEFORE the destructive rebuild when a registered stdio MCP server may +/// hold this project's index database. Advisory only: it never fails, never +/// changes the exit code, and prints nothing when there is no holder to name, so +/// a successful `index` against an empty registry emits its previous bytes +/// exactly. +/// +/// An unreadable registry stays SILENT here (a debug event only). It reports +/// nothing about a holder, and a line on every single `index` run would be noise +/// the user cannot act on; the same outage IS surfaced on the failure path, where +/// it finally matters. +fn warn_if_stdio_mcp_may_hold_index(project: &Path) { + let (holders, unavailable) = mcp_index_holders(project); + if holders.is_empty() { + if let Some((path, error)) = unavailable { + tracing::debug!( + %path, + %error, + "could not read the stdio MCP registry before rebuilding this index" + ); + } + return; + } + eprintln!( + "Warning: rebuilding this index deletes its database files, and a process still holding \ + them makes that delete fail (the Windows failure mode)." + ); + eprint!("{}", index_holder_guidance(&holders, None)); +} + +/// Append holder guidance to the CLI's presentation of a `RemoveDatabase` +/// failure — the exact Windows failure point, where `std::fs::remove_file` on an +/// open `codegraph.db` cannot succeed. The store-layer message +/// (`cannot remove v2 database artifact {path}: {source}`) is a statement of fact +/// and is left untouched; this only adds "who may be holding it, and how to stop +/// it" AFTER it, and the original error chain is neither wrapped nor swallowed. +/// +/// The holder set is narrowed with the failing artifact's own path, so a server +/// pinned to an unrelated project is still excluded here. +/// +/// Deliberately NOT integration-tested (decision B of the rev55 plan): the +/// private `RebuildFault` hook cannot inject a DB removal failure, and a +/// black-box attempt fails EARLIER, at database open. The text is covered by +/// [`index_holder_guidance`]'s unit tests; this wiring is confirmed by a +/// hands-on walk-through. +fn index_removal_holder_guidance(err: &anyhow::Error) -> Option { + let artifact = err.chain().find_map(|cause| { + match cause.downcast_ref::() { + Some(codegraph_store::RebuildError::RemoveDatabase { path, .. }) => Some(path.clone()), + _ => None, + } + })?; + let (holders, unavailable) = mcp_index_holders(&artifact); + let unavailable = unavailable + .as_ref() + .map(|(path, error)| (path.as_str(), error.as_str())); + Some(index_holder_guidance(&holders, unavailable)) +} + +#[cfg(test)] +mod index_holder_guidance_tests { + use super::{index_holder_guidance, mcp_stop_command}; + use codegraph_daemon::mcp_registry::McpServerInfo; + + fn entry(pid: u32, project: Option<&str>) -> McpServerInfo { + McpServerInfo { + pid, + project: project.map(str::to_string), + transport: "stdio".to_string(), + started_at: 1_700_000_000_123, + version: "0.41.0".to_string(), + } + } + + /// Branch A — every holder's PID is named, alongside the platform stop + /// command a HUMAN runs (decision A: we print it, we never run it). + #[test] + fn with_holders_names_every_pid_and_the_stop_command() { + let text = + index_holder_guidance(&[entry(4321, Some("/work/alpha")), entry(9876, None)], None); + + assert!( + text.contains("4321") && text.contains("9876"), + "every holder pid must be named: {text:?}" + ); + assert!( + text.contains(mcp_stop_command()), + "the platform stop command must be offered as guidance: {text:?}" + ); + assert!( + text.contains("/work/alpha"), + "a pinned holder must name the project it serves: {text:?}" + ); + assert!( + text.contains("--path"), + "a holder with no project must be explained (started without --path): {text:?}" + ); + assert!( + text.contains("0.40.x"), + "the registered set is not exhaustive; legacy builds never register: {text:?}" + ); + assert!( + !text.contains("does not prove"), + "the found-a-holder branch must not read like the nothing-found one: {text:?}" + ); + assert!( + text.ends_with('\n'), + "guidance is printed with `eprint!`, so it must end with a newline: {text:?}" + ); + } + + /// Branch B, readable-but-empty — must name BOTH causes at once: nothing has + /// registered yet, AND `<=0.40.x` never registers, so absence is not proof. + #[test] + fn without_holders_covers_empty_and_legacy_causes() { + let text = index_holder_guidance(&[], None); + + assert!( + text.contains("does not prove"), + "absence of a registered holder must not be presented as proof: {text:?}" + ); + assert!( + text.contains("registered yet"), + "cause 1: the registry may simply be empty: {text:?}" + ); + assert!( + text.contains("0.40.x"), + "cause 2: legacy builds never register at all: {text:?}" + ); + assert!( + text.contains("codegraph serve --mcp") && text.contains("OS process tools"), + "the user must be told what to look for, and with what: {text:?}" + ); + assert!( + text.contains(mcp_stop_command()), + "the platform stop command must be offered here too: {text:?}" + ); + assert!( + !text.contains("could not be read"), + "a readable-but-empty registry must not be reported as an outage: {text:?}" + ); + assert!(text.ends_with('\n'), "must end with a newline: {text:?}"); + } + + /// Branch B, unreadable — same actionable advice, plus the registry path and + /// error so the outage is distinguishable from a genuinely empty registry. + /// All THREE causes are spelled out here (unreadable / possibly-empty / + /// legacy) so no reader concludes the check was authoritative. + #[test] + fn unreadable_registry_renders_the_nothing_found_branch_with_its_path() { + let text = index_holder_guidance( + &[], + Some(("/state/codegraph/mcp", "Not a directory (os error 20)")), + ); + + assert!( + text.contains("/state/codegraph/mcp") && text.contains("Not a directory (os error 20)"), + "the outage must name the registry path and the error: {text:?}" + ); + assert!( + text.contains("could not be read"), + "the outage must be stated as an outage: {text:?}" + ); + assert!( + text.contains("registered yet"), + "a readable registry could ALSO have been empty; say so: {text:?}" + ); + assert!( + text.contains("0.40.x"), + "legacy builds never register at all, outage or not: {text:?}" + ); + assert!( + text.contains("codegraph serve --mcp") && text.contains(mcp_stop_command()), + "the advice must stay actionable during an outage: {text:?}" + ); + assert!(text.ends_with('\n'), "must end with a newline: {text:?}"); + } +} + /// Render the running-servers table, or a "none" line when empty. fn print_http_table(servers: &[codegraph_daemon::http_registry::HttpServerInfo]) { if servers.is_empty() { diff --git a/crates/codegraph-cli/tests/index_holder_warning.rs b/crates/codegraph-cli/tests/index_holder_warning.rs new file mode 100644 index 0000000..4153fbd --- /dev/null +++ b/crates/codegraph-cli/tests/index_holder_warning.rs @@ -0,0 +1,281 @@ +//! `codegraph index` — the PRE-REBUILD warning about stdio MCP servers that may +//! hold this project's index database open (P1-b of the rev55 plan). +//! +//! A full rebuild deletes `codegraph.db`/`-wal`/`-shm` before writing the new +//! index. Unix can unlink an open file; Windows cannot, which is why the partner +//! team's `index --force` failed while stray `codegraph serve --mcp` processes +//! were holding the database. The fix for the ROOT cause shipped in v0.41.0 +//! (the MCP engine is request-scoped now); what this suite pins is the remaining +//! legibility gap — telling the user WHO may be holding the index, BEFORE the +//! destructive step, and never changing the exit code. +//! +//! Seeding uses the test process's own pid, which is guaranteed alive, so the +//! entry survives the read-time liveness prune. `CODEGRAPH_MCP_REGISTRY_DIR` +//! keeps every case off a developer's real state directory. +//! +//! This lives in its own file rather than in `mcp_registry_cli.rs`: that suite +//! documents the `mcp list` READ surface, whereas these cases drive `index`. +//! Nothing is shared between integration test files in this crate, so the small +//! harness below is duplicated by the same convention every sibling follows. +#![cfg(unix)] + +use std::path::{Path, PathBuf}; +use std::process::{Command, Stdio}; + +use serde_json::json; + +fn bin() -> PathBuf { + PathBuf::from(env!("CARGO_BIN_EXE_codegraph")) +} + +fn mini_fixture() -> PathBuf { + Path::new(env!("CARGO_MANIFEST_DIR")) + .parent() + .and_then(Path::parent) + .expect("codegraph-cli is under crates/") + .join("crates/codegraph-bench/fixtures/mini") +} + +struct TestDir { + path: PathBuf, +} + +impl TestDir { + fn new(label: &str) -> Self { + let path = std::env::temp_dir().join(format!( + "codegraph-idxwarn-{label}-{}-{}", + std::process::id(), + std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .unwrap() + .as_nanos() + )); + std::fs::create_dir_all(&path).unwrap(); + Self { path } + } + fn path(&self) -> &Path { + &self.path + } +} + +impl Drop for TestDir { + fn drop(&mut self) { + let _ = std::fs::remove_dir_all(&self.path); + } +} + +fn copy_tree(src: &Path, dst: &Path) { + std::fs::create_dir_all(dst).unwrap(); + for entry in std::fs::read_dir(src).unwrap() { + let entry = entry.unwrap(); + let from = entry.path(); + let to = dst.join(entry.file_name()); + if from.is_dir() { + copy_tree(&from, &to); + } else { + std::fs::copy(&from, &to).unwrap(); + } + } +} + +/// Copy the mini fixture to `/` and give it a v2 index namespace. +fn indexed_project(dir: &TestDir, name: &str) -> PathBuf { + let project = dir.path().join(name); + copy_tree(&mini_fixture(), &project); + let status = Command::new(bin()) + .args(["init", project.to_str().unwrap()]) + .env("CODEGRAPH_NO_DAEMON", "1") + .stdout(Stdio::null()) + .stderr(Stdio::null()) + .status() + .expect("run codegraph init"); + assert!(status.success(), "init failed for {}", project.display()); + project +} + +/// Write one `.json` registry entry in the registry's own camelCase shape. +/// `project` of `None` is the `serve --mcp` launched WITHOUT `--path` case (the +/// Kiro / Qoder shape), which resolves its project per request. +fn seed_entry(registry_dir: &Path, pid: u32, project: Option<&str>) { + std::fs::create_dir_all(registry_dir).unwrap(); + let mut entry = json!({ + "pid": pid, + "transport": "stdio", + "startedAt": 1_700_000_000_123u64, + "version": "0.41.0", + }); + if let Some(project) = project { + entry["project"] = json!(project); + } + std::fs::write( + registry_dir.join(format!("{pid}.json")), + format!("{entry}\n"), + ) + .unwrap(); +} + +struct Run { + stdout: String, + stderr: String, + success: bool, +} + +fn run_index(registry_dir: &Path, project: &Path, extra: &[&str]) -> Run { + let mut args: Vec<&str> = vec!["index", project.to_str().unwrap()]; + args.extend_from_slice(extra); + let out = Command::new(bin()) + .args(&args) + .env("CODEGRAPH_MCP_REGISTRY_DIR", registry_dir) + .env("CODEGRAPH_NO_DAEMON", "1") + .stdin(Stdio::null()) + .stdout(Stdio::piped()) + .stderr(Stdio::piped()) + .output() + .expect("run codegraph index"); + Run { + stdout: String::from_utf8_lossy(&out.stdout).into_owned(), + stderr: String::from_utf8_lossy(&out.stderr).into_owned(), + success: out.status.success(), + } +} + +/// The wording that only ever appears when a holder was found. +const WARNING_MARKER: &str = "may hold this project's index database open"; + +/// Drop the two fields that are volatile BETWEEN RUNS even without this feature: +/// the subscriber's startup timestamp on stderr, and the elapsed-duration tail of +/// the summary on stdout. What remains must match byte-for-byte, which is how the +/// "successful output is unchanged" claim is checked without a stored baseline. +fn normalize(stream: &str) -> String { + stream + .lines() + .filter(|line| !line.contains("logger initialized")) + .map(|line| match line.find(" in ") { + Some(at) if line.ends_with("ms") => line[..at].to_string(), + _ => line.to_string(), + }) + .collect::>() + .join("\n") +} + +/// A live server registered against THIS project is warned about before the +/// destructive rebuild — and the warning is advisory only: `index` still +/// succeeds, still prints its summary, and still exits 0. +#[test] +fn warns_before_rebuild_when_a_server_is_registered_for_this_project() { + let dir = TestDir::new("same-project"); + let registry_dir = dir.path().join("mcp-registry"); + let project = indexed_project(&dir, "proj"); + let pid = std::process::id(); + seed_entry(®istry_dir, pid, Some(project.to_str().unwrap())); + + let run = run_index(®istry_dir, &project, &[]); + + assert!( + run.success, + "the warning must NOT change the exit code: stdout={} stderr={}", + run.stdout, run.stderr + ); + assert!( + run.stderr.contains(WARNING_MARKER), + "index must warn that a registered stdio MCP server may hold the DB: stderr={}", + run.stderr + ); + assert!( + run.stderr.contains(&pid.to_string()), + "the warning must name the holder's pid ({pid}): stderr={}", + run.stderr + ); + assert!( + run.stderr.contains("Scanning files"), + "the warning must come BEFORE the rebuild, not replace its output: stderr={}", + run.stderr + ); + assert!( + run.stdout.contains("Indexed"), + "the rest of the success path is untouched: stdout={}", + run.stdout + ); + + // `--quiet` is the machine-readable mode; `cmd_index` gates all of its other + // human-facing output on it, and this warning follows that convention. + let quiet = run_index(®istry_dir, &project, &["--quiet"]); + assert!(quiet.success, "--quiet index must still exit 0"); + assert!( + !quiet.stderr.contains(WARNING_MARKER), + "--quiet must suppress the advisory warning: stderr={}", + quiet.stderr + ); +} + +/// A server with NO pinned project (`serve --mcp` without `--path`) resolves its +/// project per request, so it CAN hold this project's database — excluding it +/// would hide exactly the Kiro-style launches this warning exists for. +#[test] +fn warns_for_a_server_with_no_pinned_project() { + let dir = TestDir::new("no-project"); + let registry_dir = dir.path().join("mcp-registry"); + let project = indexed_project(&dir, "proj"); + seed_entry(®istry_dir, std::process::id(), None); + + let run = run_index(®istry_dir, &project, &[]); + + assert!(run.success, "still exits 0: stderr={}", run.stderr); + assert!( + run.stderr.contains(WARNING_MARKER) && run.stderr.contains("--path"), + "a project-less server must be warned about and explained: stderr={}", + run.stderr + ); +} + +/// An EMPTY registry and a registry holding only an UNRELATED project's server +/// both leave the successful `index` output unchanged — no warning, and the +/// normalized streams match the empty-registry baseline byte-for-byte. +#[test] +fn unrelated_project_and_empty_registry_leave_output_unchanged() { + let dir = TestDir::new("unrelated"); + let project = indexed_project(&dir, "proj"); + let unrelated = dir.path().join("other-proj"); + + let empty_dir = dir.path().join("registry-empty"); + std::fs::create_dir_all(&empty_dir).unwrap(); + let baseline = run_index(&empty_dir, &project, &[]); + assert!( + baseline.success, + "baseline index must succeed: stderr={}", + baseline.stderr + ); + assert!( + !baseline.stderr.contains(WARNING_MARKER), + "an empty registry has nothing to say: stderr={}", + baseline.stderr + ); + + let seeded_dir = dir.path().join("registry-unrelated"); + seed_entry( + &seeded_dir, + std::process::id(), + Some(unrelated.to_str().unwrap()), + ); + let seeded = run_index(&seeded_dir, &project, &[]); + assert!( + seeded.success, + "index must succeed: stderr={}", + seeded.stderr + ); + assert!( + !seeded.stderr.contains(WARNING_MARKER), + "a server for an UNRELATED project must not raise a false alarm: stderr={}", + seeded.stderr + ); + assert_eq!( + normalize(&seeded.stderr), + normalize(&baseline.stderr), + "stderr must be unchanged when there is no holder to report" + ); + assert_eq!( + normalize(&seeded.stdout), + normalize(&baseline.stdout), + "stdout must be unchanged when there is no holder to report" + ); +} From bc187b45d1aa343c7a18342c4bdf19d99a9f074d Mon Sep 17 00:00:00 2001 From: sunerpy Date: Fri, 31 Jul 2026 14:02:59 +0800 Subject: [PATCH 15/18] docs: document mcp list, impact edge counts, and the MCP registry cli.md gains two reference sections: impact --json's edgeCount now being the total with resourceEdgeCount as the resource share, and mcp list with both output shapes, the exit-0-on-outage behavior, the index pre-warning, and why there is no mcp stop. A reader who sees http stop but no mcp stop would otherwise assume it was forgotten rather than gated on portable instance-identity verification. godot.md notes the edge-count change where it matters, in the resource-impact section: a .gd referenced only from .tres files used to report zero edges while listing every referrer, because resource files have no grammar and so no graph edges of their own. AGENTS.md records the PID-keyed registry next to the addr-keyed HTTP one, since they are the same class of architectural fact and the difference in keying is what determines which one can offer a stop. --- AGENTS.md | 19 +++++++++ docs/cli.md | 105 ++++++++++++++++++++++++++++++++++++++++++++++++++ docs/godot.md | 8 ++++ 3 files changed, 132 insertions(+) diff --git a/AGENTS.md b/AGENTS.md index 2ba5541..52f4d1b 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -100,6 +100,25 @@ conflict (listing the running instance), and notes any other live servers when t (`stop` uses `process::terminate_pid` — SIGTERM on unix / `TerminateProcess` on Windows). None of this touches extraction/golden equivalence. +Foreground stdio `serve --mcp` gets a THIRD, PID-keyed registry +(`codegraph-daemon/src/mcp_registry.rs`): one `.json` +(`McpServerInfo { pid, project: Option, transport: "stdio", started_at, version }`, camelCase on +the wire) under the same GLOBAL state chain as the HTTP one but with an `mcp` leaf +(`$XDG_STATE_HOME/codegraph/mcp`, else `~/.local/state/codegraph/mcp`, `%LOCALAPPDATA%\codegraph\mcp` on +Windows; `CODEGRAPH_MCP_REGISTRY_DIR` overrides). PID keying is forced by the transport: a stdio process +has no addr and no per-project rendezvous, several may serve one project, and one may serve none. +Registration fires from all THREE foreground exits in `cmd_serve` (`Direct`, `SpawnOrProxy`, and the +too-broad-root home guard) and NEVER from `BeDaemon`, which already owns `.codegraph-v2/daemon.pid`. +Reads go through `RegistryRead::{Available, Unavailable}` so a MISSING directory ("nobody registered +yet", normal) is distinguishable from an unreadable one (an outage); dead-PID and unparseable entries +are pruned on read. `codegraph mcp list [--json]` renders it, and `index` pre-warns when a registered +server could reach the project it is about to rebuild. This registry is PURE OBSERVABILITY — there is no +`mcp stop` and no `terminate_pid` import, because a stale entry's PID may have been reused and this +workspace has no portable instance-identity primitive (`try_acquire_daemon_lock` is a `create_new` +placeholder plus a recorded PID, not an OS advisory lock); `list` prints `kill ` / +`taskkill /PID /F` for a human instead. CLI/daemon only; extraction and golden equivalence +untouched. + ## Agent installer (`codegraph install` / `uninstall`) `codegraph install` writes the codegraph MCP-server entry into each supported agent's config diff --git a/docs/cli.md b/docs/cli.md index 7de4bdb..67de347 100644 --- a/docs/cli.md +++ b/docs/cli.md @@ -350,6 +350,32 @@ is out of scope (that is Godot MCP Pro's job). --- +## `codegraph impact` — edge counts in `--json` + +`impact --json` emits `symbol`, `depth`, `nodeCount`, `edgeCount`, +`resourceEdgeCount`, `affected`, and `godotDynamic`. The two counts split like +this: + +- **`edgeCount`** — **all** impact edges: the graph-traversal edges reached from + the matched symbols, **plus** the Godot static resource edges (a `.tscn` / + `.tres` / `project.godot` referrer of the target file). It is the total, not + the code-only figure. +- **`resourceEdgeCount`** — just the resource share of that total. Code edges are + therefore `edgeCount - resourceEdgeCount`; no third field is needed. + +The resource count comes from the same sorted+deduped referrer set that gets +appended to `affected`, so the count and the list can never contradict each +other. A pure-code target reports `resourceEdgeCount: 0` and an `edgeCount` +identical to what earlier versions produced. + +This matters for Godot projects, where the referrers of a `.gd` script live in +resource files that have no tree-sitter grammar and so no graph edges of their +own. Such a target used to report `nodeCount: 13, edgeCount: 0` while listing 12 +referrers under `affected`; it now reports `edgeCount: 12` with +`resourceEdgeCount: 12`. See [`godot.md`](godot.md#resource-audit-codegraph-audit). + +--- + ## `codegraph export` — whole-graph export + centrality Exports the entire code graph as **NetworkX node-link JSON** @@ -540,6 +566,84 @@ codegraph completions elvish > ~/.config/codegraph/completion.elv --- +## `codegraph mcp list` — see the running stdio MCP servers + +`serve --mcp` runs in the foreground, so several stdio MCP processes can be alive +at once — one per client window — with nothing tying them to a project directory. +Each foreground `serve --mcp` registers itself in a global, PID-keyed registry, and +`mcp list` reads it back: + +```bash +codegraph mcp list # table +codegraph mcp list --json # machine-readable +``` + +The table columns are `PID`, `STARTED`, `VERSION`, `PROJECT`. `PROJECT` is last +and never truncated — it is the field a human reads to recognize a stale row. A +server launched without `--path` (the Kiro / Qoder shape, which resolves its +project per request) shows ``. + +Two other renderings: + +- Nothing registered: `No stdio MCP servers registered.` plus a note that older + codegraph versions do not register at all, so they never appear here — find + those with your OS process tools. +- Registry unreadable: `registry unavailable at : ` plus the same + process-tool fallback. A missing directory is **not** an outage; it is the + normal state before the first `serve --mcp` ever runs, and renders as the empty + case above. + +**Every branch exits 0**, the outage included. This is a diagnostic command, and +failing it while someone is debugging would only get in the way. + +`--json` output always carries `servers` as an array, so a consumer never has to +branch on shape: + +```jsonc +{ + "servers": [ + { + "pid": 41287, + "project": "/w/proj", + "startedAt": 1753900000000, + "transport": "stdio", + "version": "0.41.0", + }, + ], +} +``` + +`project` is omitted when the server was started without `--path`. On an outage +the array is empty and an extra `registryUnavailable` key appears: + +```jsonc +{ "servers": [], "registryUnavailable": { "path": "…/codegraph/mcp", "error": "…" } } +``` + +**Why there is no `codegraph mcp stop`.** The HTTP registry is keyed by bind +address and does offer `http stop`; this one is keyed by PID, and that difference +is the whole reason. An entry that outlived a crash names a PID the OS may since +have handed to an unrelated process, and codegraph has no portable way to prove +process instance identity — the daemon lock is an atomic-create placeholder plus a +recorded PID, not an OS advisory lock. Terminating by registered PID could kill an +innocent process, so `list` prints the platform-appropriate command (`kill ` +on unix, `taskkill /PID /F` on Windows) and leaves the decision to a human. +A `stop` subcommand is gated on landing instance-identity verification first, not +on anything else. Closing the client that launched the server is cleaner than +either way. + +**`index` pre-warning.** Before rebuilding an index — a destructive step that +deletes `codegraph.db`, `-wal`, and `-shm` — `codegraph index` checks the same +registry and warns when a registered stdio server could reach this project, +naming each PID and the stop command. A process still holding those files makes +the delete fail; that is the Windows-only failure mode, since unix can unlink an +open file. The warning goes to stderr, respects `-q/--quiet`, never changes the +exit code, and is silent when no holder is registered. The same guidance is +appended when the delete does fail. It is observability, not a fix: registries +only see servers that register, so an absent holder is not proof there is none. + +--- + ## Daemon, watch & environment variables ### How `serve --mcp` chooses a run mode @@ -629,6 +733,7 @@ Three escape hatches: | `CODEGRAPH_WATCH_DEBOUNCE_MS` | `2000` | 100–60000 | File-change debounce window before a re-index triggers | | `CODEGRAPH_NO_WATCH` | — | — | Disable the live file watcher (equivalent to `serve --no-watch`) | | `CODEGRAPH_FORCE_WATCH` | — | — | Override WSL2 `/mnt/` auto-disable; does not override `NO_WATCH` | +| `CODEGRAPH_MCP_REGISTRY_DIR` | — | — | Override the stdio MCP registry directory read by `mcp list` | Values outside the clamp range are silently clamped to the nearest bound. diff --git a/docs/godot.md b/docs/godot.md index 50944eb..fa963d0 100644 --- a/docs/godot.md +++ b/docs/godot.md @@ -279,6 +279,14 @@ on incoming graph edges. (`affectedFiles ⊇ affectedTests`), so for a Godot project with no test files it still LISTS the dependent scenes/resources instead of only counting them — bringing `affected` into agreement with `impact` / `audit --impact`. +- **Edge counts** (`codegraph impact --json`) — `edgeCount` now covers + **all** impact edges, the Godot static resource edges included, with a sibling + `resourceEdgeCount` reporting just the resource share (code edges are + `edgeCount - resourceEdgeCount`). Before this, a `.gd` referenced only from + `.tres` files reported `edgeCount: 0` while listing every referrer under + `affected` — those referrers have no tree-sitter grammar and so no graph edges + of their own to count. The resource count is drawn from the same sorted+deduped + referrer set appended to `affected`, so the count and the list always agree. This is a static structural report. Runtime `ResourceLoader` load-verification is out of scope (that is Godot MCP Pro's job). See [`cli.md`](cli.md) for the full From 674b655f4fada28429a64ef8408da2567760da0d Mon Sep 17 00:00:00 2001 From: sunerpy Date: Fri, 31 Jul 2026 15:01:40 +0800 Subject: [PATCH 16/18] fix(cli): correct five honesty defects in the MCP registry and impact counts A code review found five ways this round's own additions could mislead. impact counted a referrer twice when a GDScript preload gave it both a graph edge and a hit in the path-keyed resource lane: one dependency appeared twice in affected and inflated both counts. Restrict the resource lane to files the traversal did not already reach, before counting and before rendering, so disjointness is structural rather than assumed. A dangling symlink at the registry path made read_dir return NotFound, which the scan read as nobody has registered yet. Treat NotFound as missing only when symlink_metadata also fails, so a broken configuration is reported as the outage it is. live_entries trusted prune_dead's deletions to have landed, so a dead entry on a read-only registry was reported as running and made index warn about a holder that does not exist. Filter the returned entries by liveness and leave pruning as the best-effort disk self-heal it always was. The guidance asserted a listed pid was running and handed over a kill command without asking anyone to confirm what that pid is. Since we refuse to stop a process precisely because identity is unprovable, we must not imply the pid is proven either: rows now read as registered as running, and both stop hints ask for a ps or tasklist check first. The RemoveDatabase diagnostic narrowed holders by the failing artifact path, but CODEGRAPH_DIR can place the database outside the project tree, so the real holder was filtered out and the failure claimed none matched. Report every live entry on the failure path, keeping the precise narrowing only in the pre-warning, which legitimately has the project root. --- AGENTS.md | 24 +- crates/codegraph-cli/src/main.rs | 239 ++++++++++++++---- .../codegraph-cli/tests/impact_edge_count.rs | 55 ++++ .../codegraph-cli/tests/mcp_registry_cli.rs | 8 + crates/codegraph-daemon/src/mcp_registry.rs | 95 ++++++- docs/cli.md | 17 +- 6 files changed, 367 insertions(+), 71 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 52f4d1b..afb7b27 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -110,14 +110,22 @@ has no addr and no per-project rendezvous, several may serve one project, and on Registration fires from all THREE foreground exits in `cmd_serve` (`Direct`, `SpawnOrProxy`, and the too-broad-root home guard) and NEVER from `BeDaemon`, which already owns `.codegraph-v2/daemon.pid`. Reads go through `RegistryRead::{Available, Unavailable}` so a MISSING directory ("nobody registered -yet", normal) is distinguishable from an unreadable one (an outage); dead-PID and unparseable entries -are pruned on read. `codegraph mcp list [--json]` renders it, and `index` pre-warns when a registered -server could reach the project it is about to rebuild. This registry is PURE OBSERVABILITY — there is no -`mcp stop` and no `terminate_pid` import, because a stale entry's PID may have been reused and this -workspace has no portable instance-identity primitive (`try_acquire_daemon_lock` is a `create_new` -placeholder plus a recorded PID, not an OS advisory lock); `list` prints `kill ` / -`taskkill /PID /F` for a human instead. CLI/daemon only; extraction and golden equivalence -untouched. +yet", normal) is distinguishable from an unreadable one (an outage); `read_dir`'s `NotFound` only means +MISSING when `fs::symlink_metadata` also fails, so a dangling symlink at the registry path reads as an +outage instead of an empty registry. `list_entries` is the RAW on-disk view (stale entries included); +`live_entries` filters its RETURN VALUE by `is_process_alive` rather than trusting `prune_dead`'s +deletions to have landed, so a dead entry on an undeletable (read-only) registry is still never reported +as running — dead-PID and unparseable files are pruned on read as best-effort disk self-heal only. +`codegraph mcp list [--json]` renders it, and `index` pre-warns when a registered server could reach the +project it is about to rebuild; the `RemoveDatabase` FAILURE path deliberately reports ALL live entries +instead of narrowing by the failing artifact path, because with `CODEGRAPH_DIR` set the index root is a +`-v2-` sibling of the legacy root and can sit outside the project tree. This +registry is PURE OBSERVABILITY — there is no `mcp stop` and no `terminate_pid` import, because a stale +entry's PID may have been reused and this workspace has no portable instance-identity primitive +(`try_acquire_daemon_lock` is a `create_new` placeholder plus a recorded PID, not an OS advisory lock); +`list` asks a human to confirm the PID with `ps -p -o command=` / `tasklist /FI "PID eq "` +before offering `kill ` / `taskkill /PID /F`. CLI/daemon only; extraction and golden +equivalence untouched. ## Agent installer (`codegraph install` / `uninstall`) diff --git a/crates/codegraph-cli/src/main.rs b/crates/codegraph-cli/src/main.rs index 9b52a72..4f93537 100644 --- a/crates/codegraph-cli/src/main.rs +++ b/crates/codegraph-cli/src/main.rs @@ -2404,9 +2404,11 @@ fn print_mcp_table(servers: &[codegraph_daemon::mcp_registry::McpServerInfo]) { ); } println!( - "({} stdio MCP server(s) running). To stop one: {} — closing the client that launched it \ - is cleaner.", + "({} stdio MCP server(s) registered as running). A pid can be reused, so confirm one is \ + still codegraph with `{}` before stopping it with `{}` — closing the client that launched \ + it is cleaner.", servers.len(), + mcp_identity_check_command(), mcp_stop_command() ); } @@ -2421,16 +2423,43 @@ fn mcp_stop_command() -> &'static str { } } +/// The platform-appropriate command that shows WHAT a pid actually is, printed +/// wherever we offer [`mcp_stop_command`]. +/// +/// Registry entries are PID-keyed and liveness is checked by pid alone, never by +/// process identity, so a stale entry whose pid was reused names an innocent +/// process. Decision A of the rev55 plan refuses to terminate BECAUSE identity is +/// unprovable here; presenting the pid as proven while handing over a stop +/// command would contradict that, so the user is asked to confirm it first. +fn mcp_identity_check_command() -> &'static str { + if cfg!(windows) { + "tasklist /FI \"PID eq \"" + } else { + "ps -p -o command=" + } +} + +/// How the `holders` handed to [`index_holder_guidance`] were selected, which is +/// all that changes in its wording. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum HolderScope { + /// Narrowed to the servers that could reach ONE project's index — only sound + /// when the caller holds that project's root. + Project, + /// Every live registered server, unnarrowed. + AllRegistered, +} + /// PURE text generator for "a stdio MCP server may be holding this index /// database" guidance. Inputs in, `String` out — no IO, no environment read, no -/// clock — which is what makes both of its branches unit-testable even though -/// one of its two call sites (the `RemoveDatabase` failure) cannot be driven +/// clock — which is what makes every branch unit-testable even though one of its +/// two call sites (the `RemoveDatabase` failure) cannot have its FAILURE driven /// from a test (decision B of the rev55 plan: `RebuildFault` exposes no DB /// removal hook, and a black-box attempt fails EARLIER, at database open). /// -/// `holders` is the caller's already-narrowed set of registry entries that could -/// hold the database in question; `registry_unavailable` carries `(path, error)` -/// when the registry could not be read at all. +/// `holders` is the caller's set of registry entries that could hold the database +/// in question, selected as `scope` says; `registry_unavailable` carries +/// `(path, error)` when the registry could not be read at all. /// /// An unreadable registry deliberately renders the SAME branch as an empty one: /// informationally both mean "we found no holder and cannot prove there is @@ -2442,14 +2471,21 @@ fn mcp_stop_command() -> &'static str { /// The text points at `codegraph mcp list` for start times instead. fn index_holder_guidance( holders: &[codegraph_daemon::mcp_registry::McpServerInfo], + scope: HolderScope, registry_unavailable: Option<(&str, &str)>, ) -> String { let mut out = String::new(); if holders.is_empty() { - out.push_str( - "No registered codegraph stdio MCP server matches this project, but that does not \ - prove none is holding its index database:\n", - ); + out.push_str(match scope { + HolderScope::Project => { + "No registered codegraph stdio MCP server matches this project, but that does not \ + prove none is holding its index database:\n" + } + HolderScope::AllRegistered => { + "No codegraph stdio MCP server is registered at all, but that does not prove none \ + is holding this index database:\n" + } + }); if let Some((path, error)) = registry_unavailable { out.push_str(&format!( " - the stdio MCP registry at {path} could not be read ({error}), so this check \ @@ -2463,10 +2499,16 @@ fn index_holder_guidance( " - codegraph 0.40.x and earlier never register at all, so they never appear here.\n", ); } else { - out.push_str( - "Registered codegraph stdio MCP server(s) that may hold this project's index database \ - open:\n", - ); + out.push_str(match scope { + HolderScope::Project => { + "Registered codegraph stdio MCP server(s) that may hold this project's index \ + database open:\n" + } + HolderScope::AllRegistered => { + "Every registered codegraph stdio MCP server is listed below — any of them may be \ + holding this index database open, and each row names the project it serves:\n" + } + }); for info in holders { let scope = match info.project.as_deref() { Some(project) => format!("project {project}"), @@ -2483,9 +2525,11 @@ fn index_holder_guidance( } out.push_str(&format!( "Look for `codegraph serve --mcp` with your OS process tools (`pgrep -af 'codegraph serve \ - --mcp'` on unix, `tasklist` or Task Manager on Windows) and stop the holder with `{}` — \ - closing the client that launched it is cleaner. `codegraph mcp list` shows every \ - registered server with its start time.\n", + --mcp'` on unix, `tasklist` or Task Manager on Windows). A registered pid is not proof of \ + identity — it may have been reused — so confirm it with `{}` before stopping the holder \ + with `{}`; closing the client that launched it is cleaner. `codegraph mcp list` shows \ + every registered server with its start time.\n", + mcp_identity_check_command(), mcp_stop_command() )); out @@ -2500,40 +2544,53 @@ fn canonicalize_lenient(path: &Path) -> PathBuf { .unwrap_or_else(|_| path.components().collect::()) } +/// Every live stdio MCP registry entry, plus `(path, error)` when the registry +/// could not be read at all — the unnarrowed read both holder queries build on. +fn mcp_live_entries() -> ( + Vec, + Option<(String, String)>, +) { + match codegraph_daemon::mcp_registry::live_entries() { + codegraph_daemon::mcp_registry::RegistryRead::Available(entries) => (entries, None), + codegraph_daemon::mcp_registry::RegistryRead::Unavailable { path, error } => { + (Vec::new(), Some((path.display().to_string(), error))) + } + } +} + /// The live stdio MCP registry entries that could be holding the index database -/// at `target`, plus `(path, error)` when the registry could not be read at all. +/// of the PROJECT rooted at `project_root`, plus `(path, error)` when the +/// registry could not be read at all. /// /// An entry with NO project was launched without `--path` (the Kiro / Qoder /// shape): it resolves its project per request, so it can open ANY project's /// database and is always included — excluding it would hide exactly the launches /// this check exists to surface. A pinned entry can only reach databases at or -/// under its own project root, so `target` must live beneath it; +/// under its own project root, so `project_root` must live beneath it; /// `Path::starts_with` compares whole components, so `/w/proj` never matches /// `/w/proj2`. +/// +/// Only call this with a genuine PROJECT ROOT. An index ARTIFACT path is not +/// interchangeable with one: with `CODEGRAPH_DIR` set the current index root is a +/// `-v2-` sibling of the normalized legacy root, so the +/// database may sit outside the project tree entirely and would match no pinned +/// entry. Use [`mcp_live_entries`] where the project root is not in hand. fn mcp_index_holders( - target: &Path, + project_root: &Path, ) -> ( Vec, Option<(String, String)>, ) { - let canonical_target = canonicalize_lenient(target); - match codegraph_daemon::mcp_registry::live_entries() { - codegraph_daemon::mcp_registry::RegistryRead::Available(entries) => { - let holders = entries - .into_iter() - .filter(|entry| match entry.project.as_deref() { - None => true, - Some(project) => { - canonical_target.starts_with(canonicalize_lenient(Path::new(project))) - } - }) - .collect(); - (holders, None) - } - codegraph_daemon::mcp_registry::RegistryRead::Unavailable { path, error } => { - (Vec::new(), Some((path.display().to_string(), error))) - } - } + let canonical_root = canonicalize_lenient(project_root); + let (entries, unavailable) = mcp_live_entries(); + let holders = entries + .into_iter() + .filter(|entry| match entry.project.as_deref() { + None => true, + Some(project) => canonical_root.starts_with(canonicalize_lenient(Path::new(project))), + }) + .collect(); + (holders, unavailable) } /// Warn BEFORE the destructive rebuild when a registered stdio MCP server may @@ -2562,7 +2619,10 @@ fn warn_if_stdio_mcp_may_hold_index(project: &Path) { "Warning: rebuilding this index deletes its database files, and a process still holding \ them makes that delete fail (the Windows failure mode)." ); - eprint!("{}", index_holder_guidance(&holders, None)); + eprint!( + "{}", + index_holder_guidance(&holders, HolderScope::Project, None) + ); } /// Append holder guidance to the CLI's presentation of a `RemoveDatabase` @@ -2572,8 +2632,16 @@ fn warn_if_stdio_mcp_may_hold_index(project: &Path) { /// and is left untouched; this only adds "who may be holding it, and how to stop /// it" AFTER it, and the original error chain is neither wrapped nor swallowed. /// -/// The holder set is narrowed with the failing artifact's own path, so a server -/// pinned to an unrelated project is still excluded here. +/// Reports EVERY live registered server rather than narrowing by the failing +/// artifact's path. All this error hands us is a database path, and a database +/// path is not a project root: with `CODEGRAPH_DIR` set the current index root is +/// a `-v2-` SIBLING of the normalized legacy root, so the +/// artifact can live outside the project tree, match no pinned entry, and make +/// this diagnostic claim no registered server is involved while one is. An +/// over-inclusive list is strictly better than hiding the actual holder, and each +/// row names its own project so a reader can still tell which is which. The +/// PRE-WARNING path keeps its precise narrowing — it legitimately has the project +/// root. /// /// Deliberately NOT integration-tested (decision B of the rev55 plan): the /// private `RebuildFault` hook cannot inject a DB removal failure, and a @@ -2581,23 +2649,31 @@ fn warn_if_stdio_mcp_may_hold_index(project: &Path) { /// [`index_holder_guidance`]'s unit tests; this wiring is confirmed by a /// hands-on walk-through. fn index_removal_holder_guidance(err: &anyhow::Error) -> Option { - let artifact = err.chain().find_map(|cause| { - match cause.downcast_ref::() { - Some(codegraph_store::RebuildError::RemoveDatabase { path, .. }) => Some(path.clone()), + err.chain().find_map( + |cause| match cause.downcast_ref::() { + Some(codegraph_store::RebuildError::RemoveDatabase { .. }) => Some(()), _ => None, - } - })?; - let (holders, unavailable) = mcp_index_holders(&artifact); + }, + )?; + let (holders, unavailable) = mcp_live_entries(); let unavailable = unavailable .as_ref() .map(|(path, error)| (path.as_str(), error.as_str())); - Some(index_holder_guidance(&holders, unavailable)) + Some(index_holder_guidance( + &holders, + HolderScope::AllRegistered, + unavailable, + )) } #[cfg(test)] mod index_holder_guidance_tests { - use super::{index_holder_guidance, mcp_stop_command}; + use super::{ + HolderScope, index_holder_guidance, index_removal_holder_guidance, + mcp_identity_check_command, mcp_stop_command, + }; use codegraph_daemon::mcp_registry::McpServerInfo; + use std::path::PathBuf; fn entry(pid: u32, project: Option<&str>) -> McpServerInfo { McpServerInfo { @@ -2613,8 +2689,11 @@ mod index_holder_guidance_tests { /// command a HUMAN runs (decision A: we print it, we never run it). #[test] fn with_holders_names_every_pid_and_the_stop_command() { - let text = - index_holder_guidance(&[entry(4321, Some("/work/alpha")), entry(9876, None)], None); + let text = index_holder_guidance( + &[entry(4321, Some("/work/alpha")), entry(9876, None)], + HolderScope::Project, + None, + ); assert!( text.contains("4321") && text.contains("9876"), @@ -2640,6 +2719,11 @@ mod index_holder_guidance_tests { !text.contains("does not prove"), "the found-a-holder branch must not read like the nothing-found one: {text:?}" ); + assert!( + text.contains(mcp_identity_check_command()) && text.contains("reused"), + "a registered pid is not proof of identity, so the user must be told to confirm \ + it before stopping it: {text:?}" + ); assert!( text.ends_with('\n'), "guidance is printed with `eprint!`, so it must end with a newline: {text:?}" @@ -2650,7 +2734,7 @@ mod index_holder_guidance_tests { /// registered yet, AND `<=0.40.x` never registers, so absence is not proof. #[test] fn without_holders_covers_empty_and_legacy_causes() { - let text = index_holder_guidance(&[], None); + let text = index_holder_guidance(&[], HolderScope::Project, None); assert!( text.contains("does not prove"), @@ -2687,6 +2771,7 @@ mod index_holder_guidance_tests { fn unreadable_registry_renders_the_nothing_found_branch_with_its_path() { let text = index_holder_guidance( &[], + HolderScope::Project, Some(("/state/codegraph/mcp", "Not a directory (os error 20)")), ); @@ -2712,6 +2797,44 @@ mod index_holder_guidance_tests { ); assert!(text.ends_with('\n'), "must end with a newline: {text:?}"); } + + /// The FAILURE path must not narrow by the failing artifact's path. With + /// `CODEGRAPH_DIR` set, the current index root is a `-v2-` + /// SIBLING of the normalized legacy root, so the database can live entirely + /// OUTSIDE the project tree; narrowing by it would then match no pinned entry + /// and wrongly report that no registered server matches. An over-inclusive + /// diagnostic beats hiding the actual holder — each row names its own project, + /// so a reader can still tell which is which. + #[test] + fn removal_guidance_reports_a_holder_pinned_outside_the_failing_artifact_path() { + let dir = + std::env::temp_dir().join(format!("cg-mcp-removal-{}-{}", std::process::id(), line!())); + let _ = std::fs::remove_dir_all(&dir); + let mut env = crate::test_env::env_guard(); + env.set( + codegraph_daemon::mcp_registry::CODEGRAPH_MCP_REGISTRY_DIR, + &dir, + ); + + let holder = entry(std::process::id(), Some("/unrelated/elsewhere")); + codegraph_daemon::mcp_registry::write_entry(&holder).unwrap(); + let err = anyhow::Error::new(codegraph_store::RebuildError::RemoveDatabase { + path: PathBuf::from("/state/codegraph-v2-0f1e2d/codegraph.db"), + source: std::io::Error::other("the process cannot access the file"), + }); + + let text = index_removal_holder_guidance(&err); + + drop(env); + let _ = std::fs::remove_dir_all(&dir); + + let text = text.expect("a RemoveDatabase failure must carry holder guidance"); + assert!( + text.contains(&holder.pid.to_string()) && text.contains("/unrelated/elsewhere"), + "a live registered server must be reported even though the failing artifact does not \ + live under its project: {text:?}" + ); + } } /// Render the running-servers table, or a "none" line when empty. @@ -3603,14 +3726,22 @@ fn cmd_impact( godot_files.push(node.file_path.clone()); } } - let mut affected = nodes.values().map(NodeSummary::from).collect::>(); let mut godot_referrers: Vec = Vec::new(); for file in &godot_files { godot_referrers.extend(godot_reverse_referrers(&store, file)?); } godot_referrers.sort(); godot_referrers.dedup(); + // A loader-side referrer may already be represented by a graph edge (for + // example, a GDScript `preload`). Keep the path-keyed resource lane + // structurally disjoint from traversal nodes before counting or rendering. + let traversal_file_paths = nodes + .values() + .map(|node| node.file_path.as_str()) + .collect::>(); + godot_referrers.retain(|file| !traversal_file_paths.contains(file.as_str())); let resource_edge_count = godot_referrers.len(); + let mut affected = nodes.values().map(NodeSummary::from).collect::>(); for from_file in godot_referrers { affected.push(NodeSummary { name: from_file.clone(), diff --git a/crates/codegraph-cli/tests/impact_edge_count.rs b/crates/codegraph-cli/tests/impact_edge_count.rs index 2dd41ff..f2815ec 100644 --- a/crates/codegraph-cli/tests/impact_edge_count.rs +++ b/crates/codegraph-cli/tests/impact_edge_count.rs @@ -1,3 +1,4 @@ +use std::collections::HashSet; use std::fs; use std::path::{Path, PathBuf}; use std::process::Command; @@ -112,6 +113,60 @@ fn godot_resource_referrers_count_as_impact_edges() { ); } +#[test] +fn godot_referrer_already_reached_by_graph_is_not_counted_twice() { + let dir = TestDir::new("godot-overlap"); + let project = dir.path().join("godot"); + fs::create_dir_all(project.join("scripts")).unwrap(); + fs::create_dir_all(project.join("data")).unwrap(); + fs::write( + project.join("project.godot"), + "[application]\nconfig/name=\"Impact edge overlap\"\n", + ) + .unwrap(); + fs::write( + project.join("scripts/target.gd"), + "class_name Target\nextends Resource\n", + ) + .unwrap(); + fs::write( + project.join("scripts/user.gd"), + "const TargetResource = preload(\"res://scripts/target.gd\")\n", + ) + .unwrap(); + fs::write( + project.join("data/one.tres"), + "[gd_resource type=\"Resource\" load_steps=2 format=3]\n\ + \n\ + [ext_resource type=\"Script\" path=\"res://scripts/target.gd\" id=\"1_script\"]\n\ + \n\ + [resource]\n\ + script = ExtResource(\"1_script\")\n", + ) + .unwrap(); + index_project(&project); + + let value = impact_json(&project, "target.gd"); + let affected = value["affected"].as_array().expect("affected is an array"); + let file_paths = affected + .iter() + .map(|node| { + node["filePath"] + .as_str() + .expect("every affected row has a filePath") + }) + .collect::>(); + let distinct_file_paths = file_paths.iter().copied().collect::>(); + + assert_eq!( + distinct_file_paths.len(), + file_paths.len(), + "a graph-reached preload referrer must not be appended again: {value}" + ); + assert_eq!(value["edgeCount"].as_u64(), Some(2), "{value}"); + assert_eq!(value["resourceEdgeCount"].as_u64(), Some(1), "{value}"); +} + #[test] fn pure_code_edge_count_is_unchanged() { let dir = TestDir::new("pure-code"); diff --git a/crates/codegraph-cli/tests/mcp_registry_cli.rs b/crates/codegraph-cli/tests/mcp_registry_cli.rs index fbc0502..31b5ad8 100644 --- a/crates/codegraph-cli/tests/mcp_registry_cli.rs +++ b/crates/codegraph-cli/tests/mcp_registry_cli.rs @@ -289,6 +289,14 @@ fn list_shows_every_live_stdio_server() { stdout.contains("kill"), "list must print the platform's stop command as GUIDANCE: {stdout}" ); + // Liveness is checked by pid, never by process identity, so the stop command + // must not be offered as if the pid were proven to be codegraph. + let identity_probe = if cfg!(windows) { "tasklist" } else { "ps -p" }; + assert!( + stdout.contains("reused") && stdout.contains(identity_probe), + "list must tell the user to confirm the pid really is codegraph before stopping it: \ + {stdout}" + ); assert!(server_a.shutdown(), "server a must exit on stdin EOF"); assert!(server_b.shutdown(), "server b must exit on stdin EOF"); diff --git a/crates/codegraph-daemon/src/mcp_registry.rs b/crates/codegraph-daemon/src/mcp_registry.rs index d80c88e..d7eb26a 100644 --- a/crates/codegraph-daemon/src/mcp_registry.rs +++ b/crates/codegraph-daemon/src/mcp_registry.rs @@ -164,7 +164,23 @@ fn scan_registry_dir(dir: &Path) -> DirScan { files.sort(); DirScan::Files(files) } - Err(error) if error.kind() == std::io::ErrorKind::NotFound => DirScan::Missing, + Err(error) if error.kind() == std::io::ErrorKind::NotFound => { + // `NotFound` is only the benign "nobody registered yet" state when + // NOTHING is at this path. `read_dir` also reports it when the path + // itself exists but cannot be resolved to a directory — a dangling + // symlink is the common case — and reporting a broken registry + // configuration as an empty registry would hide a real outage. + // `symlink_metadata` does not follow links, so it succeeds exactly + // when something IS there; it also covers other odd inodes for free. + if fs::symlink_metadata(dir).is_ok() { + DirScan::Failed(format!( + "{error} (a filesystem object exists at this path but cannot be read as a \ + directory — a dangling symlink?)" + )) + } else { + DirScan::Missing + } + } // Anything else (a FILE where the dir should be, a permission denial, an // unreadable mount) is a genuine outage the caller must be able to tell // apart from "nothing registered". @@ -174,6 +190,10 @@ fn scan_registry_dir(dir: &Path) -> DirScan { /// List EVERY registry entry (live or stale) without pruning. Sorted by pid for /// deterministic output. +/// +/// This is the RAW on-disk view and makes no liveness claim: a stale entry left +/// by a crashed process is still returned. Callers that mean "what is running +/// now" want [`live_entries`], which filters by liveness. #[must_use] pub fn list_entries() -> RegistryRead { let dir = registry_dir(); @@ -226,14 +246,30 @@ pub fn prune_dead() -> Result> { Ok(pruned) } -/// List entries after pruning dead ones — the canonical "what is running now". +/// The canonical "what is running now": every registry entry whose pid is still +/// alive, sorted by pid. +/// +/// Liveness is enforced on the RETURNED SET, not by assuming [`prune_dead`] +/// deleted the stale files. Pruning is disk self-heal and it can legitimately +/// fail — a read-only or otherwise undeletable registry keeps a dead +/// `.json` on disk — and a caller that trusted deletion would then report a +/// process that no longer exists as running, naming a pid that may since have +/// been reused. Correctness therefore never depends on the removal succeeding. #[must_use] pub fn live_entries() -> RegistryRead { // Pruning is best-effort here: [`list_entries`] below owns the // Available-vs-Unavailable classification, so an unreadable registry is // reported once, from one place, instead of twice with two error strings. let _ = prune_dead(); - list_entries() + match list_entries() { + RegistryRead::Available(entries) => RegistryRead::Available( + entries + .into_iter() + .filter(|entry| is_process_alive(entry.pid)) + .collect(), + ), + unavailable => unavailable, + } } /// Remove the registry entry for `pid` (best-effort; a missing file is already @@ -349,6 +385,39 @@ mod tests { assert!(!registry_file(®istry.dir, dead_pid).exists()); } + /// `live_entries` must filter its RETURN VALUE by liveness instead of + /// trusting `prune_dead` to have deleted the file. On a read-only registry + /// the deletion fails, the dead `.json` survives, and a caller would + /// otherwise report a process that no longer exists as RUNNING. + /// + /// A directory mode of `r-x` is what makes the deletion fail while still + /// allowing the scan. `root` ignores mode bits, so the file may vanish + /// anyway there; the assertion is therefore on the returned set — the + /// contract being fixed — and reports whether the file actually survived. + #[cfg(unix)] + #[test] + fn live_entries_omits_a_dead_entry_whose_file_cannot_be_deleted() { + use std::os::unix::fs::PermissionsExt; + + let registry = TempRegistry::new("undeletable-dead"); + let dead_pid = pick_dead_pid(); + write_entry(&sample(dead_pid)).unwrap(); + let path = registry_file(®istry.dir, dead_pid); + let restore = fs::metadata(®istry.dir).unwrap().permissions(); + fs::set_permissions(®istry.dir, fs::Permissions::from_mode(0o500)).unwrap(); + + let entries = available(live_entries()); + let survived = path.exists(); + fs::set_permissions(®istry.dir, restore).unwrap(); + + assert!( + !entries.iter().any(|entry| entry.pid == dead_pid), + "a dead pid must be absent from live_entries' return value (its file survived \ + pruning: {survived}); reporting it would name a process that does not exist: \ + {entries:?}" + ); + } + #[test] fn corrupt_pid_json_does_not_poison_the_whole_list() { let registry = TempRegistry::new("corrupt"); @@ -386,6 +455,26 @@ mod tests { } } + #[cfg(unix)] + #[test] + fn dangling_registry_symlink_reads_as_unavailable_not_empty() { + use std::os::unix::fs::symlink; + + let registry = TempRegistry::new("dangling-symlink"); + fs::remove_dir_all(®istry.dir).unwrap(); + symlink(registry.dir.with_extension("missing-target"), ®istry.dir).unwrap(); + + match live_entries() { + RegistryRead::Unavailable { path, error } => { + assert_eq!(path, registry.dir); + assert!(!error.is_empty()); + } + RegistryRead::Available(entries) => { + panic!("dangling registry symlink was reported as available: {entries:?}") + } + } + } + #[test] fn remove_entry_deletes_the_pid_file() { let _registry = TempRegistry::new("remove"); diff --git a/docs/cli.md b/docs/cli.md index 67de347..ab88a2c 100644 --- a/docs/cli.md +++ b/docs/cli.md @@ -363,10 +363,12 @@ this: - **`resourceEdgeCount`** — just the resource share of that total. Code edges are therefore `edgeCount - resourceEdgeCount`; no third field is needed. -The resource count comes from the same sorted+deduped referrer set that gets -appended to `affected`, so the count and the list can never contradict each -other. A pure-code target reports `resourceEdgeCount: 0` and an `edgeCount` -identical to what earlier versions produced. +The resource count comes from the same referrer set that gets appended to +`affected` — sorted, deduped, and restricted to files the graph traversal did not +already reach — so the count and the list can never contradict each other, and a +referrer that also has a real graph edge (a GDScript `preload`, say) is counted +once rather than twice. A pure-code target reports `resourceEdgeCount: 0` and an +`edgeCount` identical to what earlier versions produced. This matters for Godot projects, where the referrers of a `.gd` script live in resource files that have no tree-sitter grammar and so no graph edges of their @@ -626,8 +628,11 @@ is the whole reason. An entry that outlived a crash names a PID the OS may since have handed to an unrelated process, and codegraph has no portable way to prove process instance identity — the daemon lock is an atomic-create placeholder plus a recorded PID, not an OS advisory lock. Terminating by registered PID could kill an -innocent process, so `list` prints the platform-appropriate command (`kill ` -on unix, `taskkill /PID /F` on Windows) and leaves the decision to a human. +innocent process, so `list` leaves the decision to a human: it asks you to confirm +the PID really is codegraph (`ps -p -o command=` on unix, +`tasklist /FI "PID eq "` on Windows) and only then offers the stop command +(`kill ` on unix, `taskkill /PID /F` on Windows). A listed row means +"registered, and that PID is alive" — never "that PID is proven to be codegraph". A `stop` subcommand is gated on landing instance-identity verification first, not on anything else. Closing the client that launched the server is cleaner than either way. From 37d366afba39330c58f2d7717f73498abe29b790 Mon Sep 17 00:00:00 2001 From: sunerpy Date: Fri, 31 Jul 2026 16:03:34 +0800 Subject: [PATCH 17/18] fix(cli): stop treating a registered project as a holder boundary The pre-rebuild warning filtered registered servers by the project each was launched for, which assumed that project bounds what a server can open. It does not. When a client passes an absolute projectPath, roots resolution accepts it on its own merits and consults the launch default only when no path was given, so a server started in one project routinely serves another. Filtering by the launch project therefore hid exactly the holder the warning exists to name, and index stayed silent before the delete that Windows cannot complete. Warn on every live registered server, as the RemoveDatabase path already did. Over-warning is the right bias here: it appears only before a destructive rebuild, only when not quiet, and only when something is registered, so a successful index against an empty registry still emits its previous bytes. HolderScope and the path predicate are removed rather than left unused, because the Project variant documented the very precondition this disproves. The project field is now recorded only for an explicit --path, since a bare launch has no pinned project and its cwd was never a scope; that also makes the rendering reachable instead of dead. Wording throughout now calls it the launch project and says plainly that it does not limit what a server may hold. The test that asserted a sibling project's server needs no warning encoded the defect and is inverted. Two regressions pin the premise end to end: a real bare serve --mcp registers without a project, and a server launched for one project resolves an absolute projectPath into another. --- AGENTS.md | 14 +- crates/codegraph-cli/src/main.rs | 236 ++++++++---------- .../tests/index_holder_warning.rs | 109 +++++--- .../codegraph-cli/tests/mcp_registry_cli.rs | 2 +- .../codegraph-cli/tests/rmcp_serve_direct.rs | 207 ++++++++++++++- docs/cli.md | 34 ++- 6 files changed, 423 insertions(+), 179 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index afb7b27..73dfe3d 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -116,10 +116,16 @@ outage instead of an empty registry. `list_entries` is the RAW on-disk view (sta `live_entries` filters its RETURN VALUE by `is_process_alive` rather than trusting `prune_dead`'s deletions to have landed, so a dead entry on an undeletable (read-only) registry is still never reported as running — dead-PID and unparseable files are pruned on read as best-effort disk self-heal only. -`codegraph mcp list [--json]` renders it, and `index` pre-warns when a registered server could reach the -project it is about to rebuild; the `RemoveDatabase` FAILURE path deliberately reports ALL live entries -instead of narrowing by the failing artifact path, because with `CODEGRAPH_DIR` set the index root is a -`-v2-` sibling of the legacy root and can sit outside the project tree. This +`project` records the launch `--path` and ONLY that: it is `Some` only when the user actually passed +`--path` (a bare `serve --mcp` stores `None`, not its cwd), and it is purely INFORMATIONAL — never a +capability boundary, because `roots::resolve_project_arg` probes an absolute per-call `projectPath` on its +own merits and consults the launch default only when no path was passed, so any live server can be asked +to open any indexed project's database. `codegraph mcp list [--json]` renders it as `LAUNCH PROJECT`, and +BOTH holder diagnostics — `index`'s pre-warning and the `RemoveDatabase` FAILURE path — therefore report +ALL live entries with no narrowing: filtering by `project` would drop the holder in the very case they +exist for (a server launched elsewhere that a client has since pointed at this project), and the failure +path additionally has only a DB path, which with `CODEGRAPH_DIR` set is a `-v2-` +sibling of the legacy root that can sit outside the project tree entirely. This registry is PURE OBSERVABILITY — there is no `mcp stop` and no `terminate_pid` import, because a stale entry's PID may have been reused and this workspace has no portable instance-identity primitive (`try_acquire_daemon_lock` is a `create_new` placeholder plus a recorded PID, not an OS advisory lock); diff --git a/crates/codegraph-cli/src/main.rs b/crates/codegraph-cli/src/main.rs index 4f93537..0e80f05 100644 --- a/crates/codegraph-cli/src/main.rs +++ b/crates/codegraph-cli/src/main.rs @@ -1941,7 +1941,7 @@ fn cmd_serve( // any existing index, opening the DB per request, so it must be // visible to `codegraph mcp list` too — this is exactly the // IDE-launched `CWD=$HOME` case that command exists to surface. - let _registry = register_stdio_mcp_process(project.as_deref()); + let _registry = register_stdio_mcp_process(project.as_deref(), explicit_path); return serve_direct_no_services(project, &project_root, no_watch); } let has_codegraph = codegraph_dir(&project_root) @@ -1951,7 +1951,7 @@ fn cmd_serve( emit_serve_startup_debug(&project_root, explicit_path, has_codegraph, &mode); match mode { ServeMode::Direct => { - let _registry = register_stdio_mcp_process(project.as_deref()); + let _registry = register_stdio_mcp_process(project.as_deref(), explicit_path); return serve_direct(project, &project_root, no_watch, explicit_path); } ServeMode::BeDaemon => { @@ -1973,7 +1973,7 @@ fn cmd_serve( .context("running as detached MCP daemon"); } ServeMode::SpawnOrProxy => { - let _registry = register_stdio_mcp_process(project.as_deref()); + let _registry = register_stdio_mcp_process(project.as_deref(), explicit_path); return serve_spawn_or_proxy(project, &project_root, no_watch, explicit_path); } } @@ -1992,18 +1992,32 @@ fn cmd_serve( /// which already holds the per-project `daemon.pid` lock and would otherwise be /// counted twice. /// +/// `project` is recorded ONLY when the user actually passed `--path` +/// (`explicit_path`). A bare `serve --mcp` defaults `project` to its cwd, but cwd +/// is merely where the process started: with nothing pinned it resolves a project +/// per request, so recording cwd would claim a default it does not have. The field +/// is purely informational either way — nothing filters on it, because a client +/// can ask any server to open any indexed project — so an absent value is the +/// honest one, and `codegraph mcp list` renders it as ``. +/// /// A registry failure NEVER breaks `serve --mcp`: MCP availability outranks /// observability, so an unwritable state dir is warned about and serving /// continues unregistered. No signal handlers are installed either — a killed /// or crashed process leaves a stale entry on purpose, which is exactly what /// [`codegraph_daemon::mcp_registry::prune_dead`] self-heals on the next read. -fn register_stdio_mcp_process(project: Option<&Path>) -> Option { +fn register_stdio_mcp_process( + project: Option<&Path>, + explicit_path: bool, +) -> Option { use codegraph_daemon::mcp_registry::{self, McpServerInfo}; let pid = std::process::id(); let info = McpServerInfo { pid, - project: project.map(|path| path.display().to_string()), + project: explicit_path + .then_some(project) + .flatten() + .map(|path| path.display().to_string()), transport: "stdio".to_string(), started_at: mcp_registry::now_millis(), version: VERSION.to_string(), @@ -2380,10 +2394,15 @@ fn cmd_mcp_list(json: bool) -> Result<()> { } } -/// Render the live stdio MCP processes, or a friendly empty state. PROJECT goes -/// LAST and is never truncated: unlike the HTTP table (whose selector, ADDR, is -/// both short and first), here the project path is the field a human reads to +/// Render the live stdio MCP processes, or a friendly empty state. LAUNCH PROJECT +/// goes LAST and is never truncated: unlike the HTTP table (whose selector, ADDR, +/// is both short and first), here the project path is the field a human reads to /// recognize a stale row, so clipping it would defeat the purpose. +/// +/// The column is LAUNCH PROJECT, not PROJECT, because that is all the entry knows: +/// it is the `--path` the server was started with (`` for a bare launch), +/// and a client can ask any server to open a different indexed project at any +/// time. Nothing in the CLI treats it as a scope. fn print_mcp_table(servers: &[codegraph_daemon::mcp_registry::McpServerInfo]) { if servers.is_empty() { println!("No stdio MCP servers registered."); @@ -2393,7 +2412,10 @@ fn print_mcp_table(servers: &[codegraph_daemon::mcp_registry::McpServerInfo]) { ); return; } - println!("{:>7} {:<27} {:<12} PROJECT", "PID", "STARTED", "VERSION"); + println!( + "{:>7} {:<27} {:<12} LAUNCH PROJECT", + "PID", "STARTED", "VERSION" + ); for info in servers { println!( "{:>7} {:<27} {:<12} {}", @@ -2404,9 +2426,10 @@ fn print_mcp_table(servers: &[codegraph_daemon::mcp_registry::McpServerInfo]) { ); } println!( - "({} stdio MCP server(s) registered as running). A pid can be reused, so confirm one is \ - still codegraph with `{}` before stopping it with `{}` — closing the client that launched \ - it is cleaner.", + "({} stdio MCP server(s) registered as running). LAUNCH PROJECT is the `--path` each \ + server started with, not a limit on what it can open: any of them can be asked to open \ + any indexed project. A pid can be reused, so confirm one is still codegraph with `{}` \ + before stopping it with `{}` — closing the client that launched it is cleaner.", servers.len(), mcp_identity_check_command(), mcp_stop_command() @@ -2439,17 +2462,6 @@ fn mcp_identity_check_command() -> &'static str { } } -/// How the `holders` handed to [`index_holder_guidance`] were selected, which is -/// all that changes in its wording. -#[derive(Debug, Clone, Copy, PartialEq, Eq)] -enum HolderScope { - /// Narrowed to the servers that could reach ONE project's index — only sound - /// when the caller holds that project's root. - Project, - /// Every live registered server, unnarrowed. - AllRegistered, -} - /// PURE text generator for "a stdio MCP server may be holding this index /// database" guidance. Inputs in, `String` out — no IO, no environment read, no /// clock — which is what makes every branch unit-testable even though one of its @@ -2457,13 +2469,19 @@ enum HolderScope { /// from a test (decision B of the rev55 plan: `RebuildFault` exposes no DB /// removal hook, and a black-box attempt fails EARLIER, at database open). /// -/// `holders` is the caller's set of registry entries that could hold the database -/// in question, selected as `scope` says; `registry_unavailable` carries -/// `(path, error)` when the registry could not be read at all. +/// `holders` is EVERY live registry entry, never a subset. A server's recorded +/// project is only its launch-time DEFAULT, not a capability boundary: +/// `crate::roots::resolve_project_arg` in `codegraph-mcp` probes an absolute +/// per-call `projectPath` on its own merits and consults the launch default only +/// when no path was passed, so any live server can be asked to open any indexed +/// project's database. Narrowing by that field would therefore hide real holders — +/// exactly the Windows rebuild failure this guidance exists to explain. Every row +/// names its own launch project so a reader can still tell the rows apart. /// -/// An unreadable registry deliberately renders the SAME branch as an empty one: -/// informationally both mean "we found no holder and cannot prove there is -/// none", so the actionable advice is identical. The outage only adds a line +/// `registry_unavailable` carries `(path, error)` when the registry could not be +/// read at all. An unreadable registry deliberately renders the SAME branch as an +/// empty one: informationally both mean "we found no holder and cannot prove there +/// is none", so the actionable advice is identical. The outage only adds a line /// naming the path, so the user can still tell the two apart. /// /// Timestamps are NOT rendered here: [`format_started_at`] reads the local UTC @@ -2471,21 +2489,14 @@ enum HolderScope { /// The text points at `codegraph mcp list` for start times instead. fn index_holder_guidance( holders: &[codegraph_daemon::mcp_registry::McpServerInfo], - scope: HolderScope, registry_unavailable: Option<(&str, &str)>, ) -> String { let mut out = String::new(); if holders.is_empty() { - out.push_str(match scope { - HolderScope::Project => { - "No registered codegraph stdio MCP server matches this project, but that does not \ - prove none is holding its index database:\n" - } - HolderScope::AllRegistered => { - "No codegraph stdio MCP server is registered at all, but that does not prove none \ - is holding this index database:\n" - } - }); + out.push_str( + "No codegraph stdio MCP server is registered at all, but that does not prove none is \ + holding this index database:\n", + ); if let Some((path, error)) = registry_unavailable { out.push_str(&format!( " - the stdio MCP registry at {path} could not be read ({error}), so this check \ @@ -2499,23 +2510,19 @@ fn index_holder_guidance( " - codegraph 0.40.x and earlier never register at all, so they never appear here.\n", ); } else { - out.push_str(match scope { - HolderScope::Project => { - "Registered codegraph stdio MCP server(s) that may hold this project's index \ - database open:\n" - } - HolderScope::AllRegistered => { - "Every registered codegraph stdio MCP server is listed below — any of them may be \ - holding this index database open, and each row names the project it serves:\n" - } - }); + out.push_str( + "Every registered codegraph stdio MCP server is listed below — any of them may be \ + holding this index database open. Each row names the project the server was LAUNCHED \ + for, which is only its default: a client can ask any server to open a different \ + indexed project, so its launch project does not limit which index it may hold.\n", + ); for info in holders { - let scope = match info.project.as_deref() { - Some(project) => format!("project {project}"), - None => "any project (started without --path)".to_string(), + let launched_for = match info.project.as_deref() { + Some(project) => format!("launched for {project}"), + None => "launched without --path, so it has no default project".to_string(), }; out.push_str(&format!( - " - pid {} — {scope}, codegraph {}\n", + " - pid {} (codegraph {}) — {launched_for}\n", info.pid, info.version )); } @@ -2535,17 +2542,15 @@ fn index_holder_guidance( out } -/// Resolve `path` through the filesystem when possible, falling back to its -/// lexical form. `index` and `serve --mcp` both record LEXICALLY normalized -/// absolute paths, so without this a symlinked ancestor (`/tmp` -> -/// `/private/tmp`) would make one and the same project compare unequal. -fn canonicalize_lenient(path: &Path) -> PathBuf { - path.canonicalize() - .unwrap_or_else(|_| path.components().collect::()) -} - /// Every live stdio MCP registry entry, plus `(path, error)` when the registry -/// could not be read at all — the unnarrowed read both holder queries build on. +/// could not be read at all — the ONE read behind both holder diagnostics. +/// +/// There is deliberately no project-narrowed variant. An entry's `project` is the +/// path its `serve --mcp` was LAUNCHED with, not the set of databases it can open: +/// a client passing an absolute `projectPath` reaches any indexed project +/// (`codegraph-mcp/src/roots.rs`, `resolve_project_arg`). Filtering by that field +/// would drop genuine holders, which is precisely how a Windows rebuild came to +/// fail with no holder named. fn mcp_live_entries() -> ( Vec, Option<(String, String)>, @@ -2558,53 +2563,26 @@ fn mcp_live_entries() -> ( } } -/// The live stdio MCP registry entries that could be holding the index database -/// of the PROJECT rooted at `project_root`, plus `(path, error)` when the -/// registry could not be read at all. -/// -/// An entry with NO project was launched without `--path` (the Kiro / Qoder -/// shape): it resolves its project per request, so it can open ANY project's -/// database and is always included — excluding it would hide exactly the launches -/// this check exists to surface. A pinned entry can only reach databases at or -/// under its own project root, so `project_root` must live beneath it; -/// `Path::starts_with` compares whole components, so `/w/proj` never matches -/// `/w/proj2`. -/// -/// Only call this with a genuine PROJECT ROOT. An index ARTIFACT path is not -/// interchangeable with one: with `CODEGRAPH_DIR` set the current index root is a -/// `-v2-` sibling of the normalized legacy root, so the -/// database may sit outside the project tree entirely and would match no pinned -/// entry. Use [`mcp_live_entries`] where the project root is not in hand. -fn mcp_index_holders( - project_root: &Path, -) -> ( - Vec, - Option<(String, String)>, -) { - let canonical_root = canonicalize_lenient(project_root); - let (entries, unavailable) = mcp_live_entries(); - let holders = entries - .into_iter() - .filter(|entry| match entry.project.as_deref() { - None => true, - Some(project) => canonical_root.starts_with(canonicalize_lenient(Path::new(project))), - }) - .collect(); - (holders, unavailable) -} - /// Warn BEFORE the destructive rebuild when a registered stdio MCP server may /// hold this project's index database. Advisory only: it never fails, never -/// changes the exit code, and prints nothing when there is no holder to name, so -/// a successful `index` against an empty registry emits its previous bytes +/// changes the exit code, and prints nothing when there is no live server to name, +/// so a successful `index` against an empty registry emits its previous bytes /// exactly. /// +/// Reports EVERY live server rather than the ones whose recorded project contains +/// `project`. That field is a launch-time default, not a boundary — see +/// [`mcp_live_entries`] — so narrowing by it silently drops the holder in the very +/// scenario this warning exists for: a server launched in one project that a +/// client has since asked to open this one. Over-warning costs a few stderr lines +/// before a destructive rebuild, and only when a server is actually registered; +/// under-warning costs the user the Windows failure with nobody named. +/// /// An unreadable registry stays SILENT here (a debug event only). It reports /// nothing about a holder, and a line on every single `index` run would be noise /// the user cannot act on; the same outage IS surfaced on the failure path, where /// it finally matters. fn warn_if_stdio_mcp_may_hold_index(project: &Path) { - let (holders, unavailable) = mcp_index_holders(project); + let (holders, unavailable) = mcp_live_entries(); if holders.is_empty() { if let Some((path, error)) = unavailable { tracing::debug!( @@ -2616,13 +2594,11 @@ fn warn_if_stdio_mcp_may_hold_index(project: &Path) { return; } eprintln!( - "Warning: rebuilding this index deletes its database files, and a process still holding \ - them makes that delete fail (the Windows failure mode)." - ); - eprint!( - "{}", - index_holder_guidance(&holders, HolderScope::Project, None) + "Warning: rebuilding the index for {} deletes its database files, and a process still \ + holding them makes that delete fail (the Windows failure mode).", + project.display() ); + eprint!("{}", index_holder_guidance(&holders, None)); } /// Append holder guidance to the CLI's presentation of a `RemoveDatabase` @@ -2632,16 +2608,15 @@ fn warn_if_stdio_mcp_may_hold_index(project: &Path) { /// and is left untouched; this only adds "who may be holding it, and how to stop /// it" AFTER it, and the original error chain is neither wrapped nor swallowed. /// -/// Reports EVERY live registered server rather than narrowing by the failing -/// artifact's path. All this error hands us is a database path, and a database -/// path is not a project root: with `CODEGRAPH_DIR` set the current index root is -/// a `-v2-` SIBLING of the normalized legacy root, so the -/// artifact can live outside the project tree, match no pinned entry, and make -/// this diagnostic claim no registered server is involved while one is. An -/// over-inclusive list is strictly better than hiding the actual holder, and each -/// row names its own project so a reader can still tell which is which. The -/// PRE-WARNING path keeps its precise narrowing — it legitimately has the project -/// root. +/// Reports EVERY live registered server, exactly like the PRE-WARNING path — a +/// server's recorded project is only its launch default, so neither path may +/// filter on it (see [`mcp_live_entries`]). Two independent reasons converge here: +/// all this error hands us is a database path, and a database path is not even a +/// project root (with `CODEGRAPH_DIR` set the current index root is a +/// `-v2-` SIBLING of the normalized legacy root, so the +/// artifact can live outside the project tree entirely). An over-inclusive list is +/// strictly better than hiding the actual holder, and each row names its own +/// launch project so a reader can still tell which is which. /// /// Deliberately NOT integration-tested (decision B of the rev55 plan): the /// private `RebuildFault` hook cannot inject a DB removal failure, and a @@ -2659,18 +2634,14 @@ fn index_removal_holder_guidance(err: &anyhow::Error) -> Option { let unavailable = unavailable .as_ref() .map(|(path, error)| (path.as_str(), error.as_str())); - Some(index_holder_guidance( - &holders, - HolderScope::AllRegistered, - unavailable, - )) + Some(index_holder_guidance(&holders, unavailable)) } #[cfg(test)] mod index_holder_guidance_tests { use super::{ - HolderScope, index_holder_guidance, index_removal_holder_guidance, - mcp_identity_check_command, mcp_stop_command, + index_holder_guidance, index_removal_holder_guidance, mcp_identity_check_command, + mcp_stop_command, }; use codegraph_daemon::mcp_registry::McpServerInfo; use std::path::PathBuf; @@ -2689,11 +2660,8 @@ mod index_holder_guidance_tests { /// command a HUMAN runs (decision A: we print it, we never run it). #[test] fn with_holders_names_every_pid_and_the_stop_command() { - let text = index_holder_guidance( - &[entry(4321, Some("/work/alpha")), entry(9876, None)], - HolderScope::Project, - None, - ); + let text = + index_holder_guidance(&[entry(4321, Some("/work/alpha")), entry(9876, None)], None); assert!( text.contains("4321") && text.contains("9876"), @@ -2711,6 +2679,11 @@ mod index_holder_guidance_tests { text.contains("--path"), "a holder with no project must be explained (started without --path): {text:?}" ); + assert!( + text.contains("does not limit which index"), + "a row's project is the one its server was LAUNCHED for; presenting it as a limit on \ + what that server can open is the defect this text must not repeat: {text:?}" + ); assert!( text.contains("0.40.x"), "the registered set is not exhaustive; legacy builds never register: {text:?}" @@ -2734,7 +2707,7 @@ mod index_holder_guidance_tests { /// registered yet, AND `<=0.40.x` never registers, so absence is not proof. #[test] fn without_holders_covers_empty_and_legacy_causes() { - let text = index_holder_guidance(&[], HolderScope::Project, None); + let text = index_holder_guidance(&[], None); assert!( text.contains("does not prove"), @@ -2771,7 +2744,6 @@ mod index_holder_guidance_tests { fn unreadable_registry_renders_the_nothing_found_branch_with_its_path() { let text = index_holder_guidance( &[], - HolderScope::Project, Some(("/state/codegraph/mcp", "Not a directory (os error 20)")), ); diff --git a/crates/codegraph-cli/tests/index_holder_warning.rs b/crates/codegraph-cli/tests/index_holder_warning.rs index 4153fbd..5f0ed17 100644 --- a/crates/codegraph-cli/tests/index_holder_warning.rs +++ b/crates/codegraph-cli/tests/index_holder_warning.rs @@ -139,8 +139,12 @@ fn run_index(registry_dir: &Path, project: &Path, extra: &[&str]) -> Run { } } -/// The wording that only ever appears when a holder was found. -const WARNING_MARKER: &str = "may hold this project's index database open"; +/// The wording that only ever appears when at least one live server was found. +const WARNING_MARKER: &str = "may be holding this index database open"; + +/// Vocabulary that belongs exclusively to the holder guidance, used to prove the +/// pre-warning contributes ZERO bytes when nothing is registered. +const GUIDANCE_VOCABULARY: [&str; 3] = ["stdio MCP", "0.40.x", "serve --mcp"]; /// Drop the two fields that are volatile BETWEEN RUNS even without this feature: /// the subscriber's startup timestamp on stderr, and the elapsed-duration tail of @@ -228,14 +232,64 @@ fn warns_for_a_server_with_no_pinned_project() { ); } -/// An EMPTY registry and a registry holding only an UNRELATED project's server -/// both leave the successful `index` output unchanged — no warning, and the -/// normalized streams match the empty-registry baseline byte-for-byte. +/// A server registered for ANOTHER project must STILL be warned about. The +/// registry's `project` field is only the launch-time default, never a capability +/// boundary: `resolve_project_arg` (`codegraph-mcp/src/roots.rs`) pushes an +/// absolute per-call `projectPath` straight into its candidate list and probes it +/// on its own merits, consulting the launch default ONLY when no path was passed. +/// A server launched in A therefore opens B's database the moment a client asks +/// it to — which is exactly the partner-reported Windows rebuild failure. Warning +/// about it is the whole point of this check, so over-warning is the correct bias. +#[test] +fn warns_when_a_server_is_registered_for_another_project() { + let dir = TestDir::new("other-project"); + let registry_dir = dir.path().join("mcp-registry"); + let project = indexed_project(&dir, "proj"); + let other = dir.path().join("other-proj"); + let pid = std::process::id(); + seed_entry(®istry_dir, pid, Some(other.to_str().unwrap())); + + let run = run_index(®istry_dir, &project, &[]); + + assert!( + run.success, + "the warning must NOT change the exit code: stdout={} stderr={}", + run.stdout, run.stderr + ); + assert!( + run.stderr.contains(WARNING_MARKER), + "a server launched for another project can still be asked to open this one, so it must \ + be warned about: stderr={}", + run.stderr + ); + assert!( + run.stderr.contains(&pid.to_string()), + "the warning must name the holder's pid ({pid}): stderr={}", + run.stderr + ); + assert!( + run.stderr.contains("other-proj"), + "each row must name the project the server was launched for, so a reader can tell the \ + rows apart: stderr={}", + run.stderr + ); +} + +/// An EMPTY registry and an UNREADABLE one both leave the successful `index` +/// output byte-for-byte unchanged: the pre-warning is the only new emission and +/// it is gated on a NON-EMPTY live set, so with nothing to name it adds zero +/// bytes. An outage stays silent here too — it says nothing about a holder, and a +/// line on every `index` run would be noise; it is surfaced on the FAILURE path +/// instead, where it is actionable. +/// +/// (Before the launch-project field was demoted to a mere default, this case also +/// compared against a registry holding an unrelated project's server. That is no +/// longer a no-holder scenario — see +/// `warns_when_a_server_is_registered_for_another_project`.) #[test] -fn unrelated_project_and_empty_registry_leave_output_unchanged() { - let dir = TestDir::new("unrelated"); +fn empty_and_unreadable_registries_leave_the_success_path_unchanged() { + let dir = TestDir::new("no-holder"); let project = indexed_project(&dir, "proj"); - let unrelated = dir.path().join("other-proj"); let empty_dir = dir.path().join("registry-empty"); std::fs::create_dir_all(&empty_dir).unwrap(); @@ -245,36 +299,31 @@ fn unrelated_project_and_empty_registry_leave_output_unchanged() { "baseline index must succeed: stderr={}", baseline.stderr ); - assert!( - !baseline.stderr.contains(WARNING_MARKER), - "an empty registry has nothing to say: stderr={}", - baseline.stderr - ); + for marker in GUIDANCE_VOCABULARY { + assert!( + !baseline.stderr.contains(marker) && !baseline.stdout.contains(marker), + "an empty registry must contribute ZERO bytes, but {marker:?} appeared: \ + stdout={} stderr={}", + baseline.stdout, + baseline.stderr + ); + } - let seeded_dir = dir.path().join("registry-unrelated"); - seed_entry( - &seeded_dir, - std::process::id(), - Some(unrelated.to_str().unwrap()), - ); - let seeded = run_index(&seeded_dir, &project, &[]); - assert!( - seeded.success, - "index must succeed: stderr={}", - seeded.stderr - ); + let unreadable_dir = dir.path().join("registry-unreadable"); + std::fs::write(&unreadable_dir, b"not a directory").unwrap(); + let outage = run_index(&unreadable_dir, &project, &[]); assert!( - !seeded.stderr.contains(WARNING_MARKER), - "a server for an UNRELATED project must not raise a false alarm: stderr={}", - seeded.stderr + outage.success, + "an unreadable registry must not fail the index: stderr={}", + outage.stderr ); assert_eq!( - normalize(&seeded.stderr), + normalize(&outage.stderr), normalize(&baseline.stderr), "stderr must be unchanged when there is no holder to report" ); assert_eq!( - normalize(&seeded.stdout), + normalize(&outage.stdout), normalize(&baseline.stdout), "stdout must be unchanged when there is no holder to report" ); diff --git a/crates/codegraph-cli/tests/mcp_registry_cli.rs b/crates/codegraph-cli/tests/mcp_registry_cli.rs index 31b5ad8..ee04de1 100644 --- a/crates/codegraph-cli/tests/mcp_registry_cli.rs +++ b/crates/codegraph-cli/tests/mcp_registry_cli.rs @@ -278,7 +278,7 @@ fn list_shows_every_live_stdio_server() { ); assert!( stdout.contains("mini-a") && stdout.contains("mini-b"), - "each row must name the project it serves: {stdout}" + "each row must name the project its server was LAUNCHED for: {stdout}" ); assert!( stdout.contains(env!("CARGO_PKG_VERSION")), diff --git a/crates/codegraph-cli/tests/rmcp_serve_direct.rs b/crates/codegraph-cli/tests/rmcp_serve_direct.rs index 260794c..282c1c4 100644 --- a/crates/codegraph-cli/tests/rmcp_serve_direct.rs +++ b/crates/codegraph-cli/tests/rmcp_serve_direct.rs @@ -79,8 +79,20 @@ fn copy_tree(src: &Path, dst: &Path) { } fn indexed_project(dir: &TestDir) -> PathBuf { - let project = dir.path().join("mini"); + indexed_project_with(dir, "mini", None) +} + +/// Copy the mini fixture to `/` and index it. `extra` writes one more +/// source file BEFORE indexing, which is how a second project gets a symbol the +/// first one provably does not have. +fn indexed_project_with(dir: &TestDir, name: &str, extra: Option<(&str, &str)>) -> PathBuf { + let project = dir.path().join(name); copy_tree(&mini_fixture(), &project); + if let Some((relative, contents)) = extra { + let file = project.join(relative); + std::fs::create_dir_all(file.parent().unwrap()).unwrap(); + std::fs::write(&file, contents).unwrap(); + } let status = Command::new(bin()) .args(["init", project.to_str().unwrap()]) .stdout(Stdio::null()) @@ -287,6 +299,199 @@ fn serve_mcp_direct_registers_its_pid_and_unregisters_on_stdin_eof() { ); } +/// Drive `initialize` + the spec-required `initialized` notification, so a test +/// can go straight to `tools/call`. Answering `initialize` also proves the server +/// is past startup, hence past registry registration. +fn handshake(stdin: &mut impl Write, stdout: &mut impl BufRead, deadline: Instant, client: &str) { + let init = json!({ + "jsonrpc": "2.0", + "id": 1, + "method": "initialize", + "params": { + "protocolVersion": "2024-11-05", + "capabilities": {}, + "clientInfo": { "name": client, "version": "0" } + } + }); + writeln!(stdin, "{init}").unwrap(); + stdin.flush().unwrap(); + let resp = read_json_line_with_id(stdout, 1, deadline).expect("initialize result"); + assert_eq!( + resp["result"]["serverInfo"]["name"], + json!("codegraph"), + "the rmcp handler must identify as codegraph" + ); + let initialized = json!({ "jsonrpc": "2.0", "method": "notifications/initialized" }); + writeln!(stdin, "{initialized}").unwrap(); + stdin.flush().unwrap(); +} + +fn search( + stdin: &mut impl Write, + stdout: &mut impl BufRead, + deadline: Instant, + id: i64, + arguments: Value, +) -> String { + let call = json!({ + "jsonrpc": "2.0", + "id": id, + "method": "tools/call", + "params": { "name": "codegraph_search", "arguments": arguments } + }); + writeln!(stdin, "{call}").unwrap(); + stdin.flush().unwrap(); + let resp = read_json_line_with_id(stdout, id, deadline).expect("tools/call response"); + resp["result"]["content"][0]["text"] + .as_str() + .unwrap_or("") + .to_string() +} + +/// A `--path`-pinned server WILL open a DIFFERENT project's index when a client +/// passes an absolute `projectPath` — so the launch path is a DEFAULT, not an +/// access boundary. `resolve_project_arg` (`codegraph-mcp/src/roots.rs`) pushes an +/// absolute per-call path straight into its candidate list and probes it on its +/// own merits; the launch default is consulted only when no path was passed. +/// +/// This is the premise the registry's `project` field must not be read as a +/// capability boundary — which is why `index`'s pre-warning reports every live +/// server instead of narrowing by that field. The existing +/// `resolve_project_arg_absolute_indexed_resolves` unit test does NOT cover it: it +/// builds the handler with `default_project = None`, so it never shows an absolute +/// argument WINNING OVER a pinned default. This case drives the real binary +/// end-to-end instead, and proves both directions at once. +#[test] +fn serve_mcp_resolves_an_absolute_project_path_outside_its_launch_path() { + // GIVEN two indexed projects where only the second defines `betamarker`, and a + // server pinned with `--path` to the first. + let home = TestDir::new("cross-project"); + let pinned = indexed_project_with(&home, "mini-pinned", None); + let other = indexed_project_with( + &home, + "mini-other", + Some(( + "src/beta.ts", + "export function betamarker(): number {\n return 7;\n}\n", + )), + ); + + let mut child = Command::new(bin()) + .args(["serve", "--mcp", "--path", pinned.to_str().unwrap()]) + .env("CODEGRAPH_NO_DAEMON", "1") + .env("CODEGRAPH_NO_WATCH", "1") + .stdin(Stdio::piped()) + .stdout(Stdio::piped()) + .stderr(Stdio::null()) + .spawn() + .expect("spawn serve --mcp (rmcp)"); + + let mut stdin = child.stdin.take().expect("child stdin"); + let mut stdout = BufReader::new(child.stdout.take().expect("child stdout")); + let deadline = Instant::now() + Duration::from_secs(30); + handshake(&mut stdin, &mut stdout, deadline, "rmcp-cross-project-test"); + + // WHEN the pinned default answers, THEN the other project's symbol is absent — + // proving the two indexes are distinguishable. + let default_hit = search( + &mut stdin, + &mut stdout, + deadline, + 2, + json!({ "query": "betamarker" }), + ); + assert!( + default_hit.contains("No results found"), + "the pinned project must not contain the other project's symbol: {default_hit}" + ); + + // WHEN an absolute `projectPath` names the OTHER project, THEN the server + // opens THAT index despite having been launched with `--path` elsewhere. + let cross_hit = search( + &mut stdin, + &mut stdout, + deadline, + 3, + json!({ "query": "betamarker", "projectPath": other.to_str().unwrap() }), + ); + assert!( + !cross_hit.contains("No results found") && cross_hit.contains("beta.ts"), + "a `--path`-pinned server must still resolve an absolute projectPath to another indexed \ + project — the launch path is a default, not a boundary: {cross_hit}" + ); + + drop(stdin); + assert!( + wait_with_timeout(&mut child, Duration::from_secs(10)), + "serve --mcp must exit after stdin EOF" + ); +} + +/// A BARE `serve --mcp` (no `--path`) records NO project. Its cwd is merely where +/// it happened to start: with no path pinned it resolves a project per request, so +/// recording cwd as "the project" would overstate what the entry knows. `mcp list` +/// renders the absent field as ``. +#[test] +fn serve_mcp_without_an_explicit_path_registers_without_a_project() { + // GIVEN a bare `serve --mcp` started INSIDE an indexed project. + let home = TestDir::new("bare-launch"); + let indexed = indexed_project(&home); + let registry_dir = home.path().join("mcp-registry"); + + let mut child = Command::new(bin()) + .args(["serve", "--mcp"]) + .current_dir(&indexed) + .env("CODEGRAPH_NO_DAEMON", "1") + .env("CODEGRAPH_NO_WATCH", "1") + .env("CODEGRAPH_MCP_REGISTRY_DIR", ®istry_dir) + .stdin(Stdio::piped()) + .stdout(Stdio::piped()) + .stderr(Stdio::null()) + .spawn() + .expect("spawn bare serve --mcp"); + + let pid = child.id(); + let entry_path = registry_dir.join(format!("{pid}.json")); + let mut stdin = child.stdin.take().expect("child stdin"); + let mut stdout = BufReader::new(child.stdout.take().expect("child stdout")); + let deadline = Instant::now() + Duration::from_secs(30); + handshake(&mut stdin, &mut stdout, deadline, "rmcp-bare-launch-test"); + + // THEN the entry exists but carries no project. + let raw = std::fs::read_to_string(&entry_path).unwrap_or_else(|error| { + panic!( + "a live bare `serve --mcp` must still register {}: {error}", + entry_path.display() + ) + }); + let entry: Value = serde_json::from_str(&raw).expect("registry entry is valid JSON"); + assert_eq!(entry["pid"], json!(pid), "{raw}"); + assert!( + entry.get("project").is_none(), + "a bare `serve --mcp` has no pinned project, so the field must be absent rather than \ + recording the cwd it happened to start in: {raw}" + ); + + // AND it still serves the cwd project — the demoted field is informational. + let hit = search( + &mut stdin, + &mut stdout, + deadline, + 2, + json!({ "query": "add" }), + ); + assert!( + hit.contains("add"), + "a bare launch must still resolve its cwd project per request: {hit}" + ); + + drop(stdin); + assert!( + wait_with_timeout(&mut child, Duration::from_secs(10)), + "serve --mcp must exit after stdin EOF" + ); +} + /// Poll for process exit within `timeout`, returning whether it exited (killing /// it on timeout so the test process never leaks a child). fn wait_with_timeout(child: &mut std::process::Child, timeout: Duration) -> bool { diff --git a/docs/cli.md b/docs/cli.md index ab88a2c..060b0ca 100644 --- a/docs/cli.md +++ b/docs/cli.md @@ -580,10 +580,15 @@ codegraph mcp list # table codegraph mcp list --json # machine-readable ``` -The table columns are `PID`, `STARTED`, `VERSION`, `PROJECT`. `PROJECT` is last -and never truncated — it is the field a human reads to recognize a stale row. A -server launched without `--path` (the Kiro / Qoder shape, which resolves its -project per request) shows ``. +The table columns are `PID`, `STARTED`, `VERSION`, `LAUNCH PROJECT`. +`LAUNCH PROJECT` is last and never truncated — it is the field a human reads to +recognize a stale row. A server launched without `--path` (the Kiro / Qoder shape) +shows ``. + +`LAUNCH PROJECT` is the `--path` the server started with, i.e. its **default** — +not a limit on what it can open. A client that passes an absolute `projectPath` +reaches any indexed project, so every live server is a potential holder of any +index. Nothing in the CLI filters on this field; it is there to tell rows apart. Two other renderings: @@ -615,8 +620,9 @@ branch on shape: } ``` -`project` is omitted when the server was started without `--path`. On an outage -the array is empty and an extra `registryUnavailable` key appears: +`project` is the launch `--path` and is omitted entirely when the server was +started without one. On an outage the array is empty and an extra +`registryUnavailable` key appears: ```jsonc { "servers": [], "registryUnavailable": { "path": "…/codegraph/mcp", "error": "…" } } @@ -639,13 +645,19 @@ either way. **`index` pre-warning.** Before rebuilding an index — a destructive step that deletes `codegraph.db`, `-wal`, and `-shm` — `codegraph index` checks the same -registry and warns when a registered stdio server could reach this project, -naming each PID and the stop command. A process still holding those files makes +registry and warns when **any** stdio server is registered, naming each PID, its +launch project, and the stop command. A process still holding those files makes the delete fail; that is the Windows-only failure mode, since unix can unlink an open file. The warning goes to stderr, respects `-q/--quiet`, never changes the -exit code, and is silent when no holder is registered. The same guidance is -appended when the delete does fail. It is observability, not a fix: registries -only see servers that register, so an absent holder is not proof there is none. +exit code, and is silent when the registry holds nothing. The same guidance is +appended when the delete does fail. + +The warning deliberately does **not** narrow to servers whose launch project +contains the one being rebuilt. Since any server can be asked to open any indexed +project, narrowing would hide the holder in exactly the case the check exists for — +a server launched elsewhere that a client has since pointed at this project. It is +observability, not a fix: registries only see servers that register, so an absent +holder is still not proof there is none. --- From 33364c33b0121be961e327411f303106dd2481f0 Mon Sep 17 00:00:00 2001 From: sunerpy Date: Fri, 31 Jul 2026 16:54:08 +0800 Subject: [PATCH 18/18] test(cli): normalize the index duration in either unit MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The no-holder byte-identical guard keyed its duration normalization off ends_with("ms"), but format_duration emits milliseconds only below a second and switches to {:.1}s at or above. Under the parallel load of the pre-push gate the index crossed that boundary and the guard stopped matching, so 1.6s versus 1.8s failed a comparison that was never about timing. Match the summary line by its own shape instead — it always reads "{n} nodes, {m} edges in {duration}" — and truncate at its last " in ". Relaxing the guard to ends_with('s') would have over-normalized ordinary prose, so the test also pins that a non-summary line containing " in " survives. --- .../tests/index_holder_warning.rs | 46 +++++++++++++++++-- 1 file changed, 43 insertions(+), 3 deletions(-) diff --git a/crates/codegraph-cli/tests/index_holder_warning.rs b/crates/codegraph-cli/tests/index_holder_warning.rs index 5f0ed17..952f48f 100644 --- a/crates/codegraph-cli/tests/index_holder_warning.rs +++ b/crates/codegraph-cli/tests/index_holder_warning.rs @@ -150,18 +150,58 @@ const GUIDANCE_VOCABULARY: [&str; 3] = ["stdio MCP", "0.40.x", "serve --mcp"]; /// the subscriber's startup timestamp on stderr, and the elapsed-duration tail of /// the summary on stdout. What remains must match byte-for-byte, which is how the /// "successful output is unchanged" claim is checked without a stored baseline. +/// +/// The duration is matched by the SUMMARY LINE'S SHAPE, not by the unit: the line +/// `print_index_result` emits always reads `{n} nodes, {m} edges in {duration}`, +/// while `format_duration` renders that duration in EITHER unit — `{}ms` below +/// 1000ms, `{:.1}s` at or above. Keying off the unit (e.g. `ends_with("ms")`) +/// silently stops normalizing on a loaded machine where the index crosses into +/// seconds; keying off `ends_with('s')` instead would over-normalize ordinary +/// prose. So: only lines carrying both summary fragments are truncated, at their +/// LAST " in ". fn normalize(stream: &str) -> String { stream .lines() .filter(|line| !line.contains("logger initialized")) - .map(|line| match line.find(" in ") { - Some(at) if line.ends_with("ms") => line[..at].to_string(), - _ => line.to_string(), + .map(|line| { + let is_summary = line.contains(" nodes, ") && line.contains(" edges in "); + match line.rfind(" in ") { + Some(at) if is_summary => line[..at].to_string(), + _ => line.to_string(), + } }) .collect::>() .join("\n") } +/// `normalize` must erase the duration in BOTH of `format_duration`'s units, and +/// must not touch a non-summary line that merely happens to contain " in ". +#[test] +fn normalize_erases_the_duration_in_either_unit_and_nothing_else() { + assert_eq!( + normalize("Indexed 3 files\n13 nodes, 21 edges in 1.6s"), + normalize("Indexed 3 files\n13 nodes, 21 edges in 1.8s"), + "seconds-form durations must normalize equal" + ); + assert_eq!( + normalize("Indexed 3 files\n13 nodes, 21 edges in 320ms"), + normalize("Indexed 3 files\n13 nodes, 21 edges in 410ms"), + "milliseconds-form durations must normalize equal" + ); + assert_eq!( + normalize("13 nodes, 21 edges in 1.6s"), + "13 nodes, 21 edges", + "the summary must keep its counts and lose only the duration" + ); + + let prose = "wrote the report in triplicate for the archives"; + assert_eq!( + normalize(prose), + prose, + "a non-summary line containing \" in \" must survive untouched" + ); +} + /// A live server registered against THIS project is warned about before the /// destructive rebuild — and the warning is advisory only: `index` still /// succeeds, still prints its summary, and still exits 0.