diff --git a/AGENTS.md b/AGENTS.md index 2ba5541..73dfe3d 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -100,6 +100,39 @@ 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); `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. +`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); +`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`) `codegraph install` writes the codegraph MCP-server entry into each supported agent's config diff --git a/crates/codegraph-cli/src/main.rs b/crates/codegraph-cli/src/main.rs index 8eb97a2..0e80f05 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); } } @@ -479,6 +482,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 +592,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 +792,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(()) @@ -1411,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 @@ -1909,6 +1937,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(), explicit_path); return serve_direct_no_services(project, &project_root, no_watch); } let has_codegraph = codegraph_dir(&project_root) @@ -1918,6 +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(), explicit_path); return serve_direct(project, &project_root, no_watch, explicit_path); } ServeMode::BeDaemon => { @@ -1939,6 +1973,7 @@ fn cmd_serve( .context("running as detached MCP daemon"); } ServeMode::SpawnOrProxy => { + let _registry = register_stdio_mcp_process(project.as_deref(), explicit_path); return serve_spawn_or_proxy(project, &project_root, no_watch, explicit_path); } } @@ -1949,6 +1984,70 @@ 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 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. +/// +/// `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>, + explicit_path: bool, +) -> Option { + use codegraph_daemon::mcp_registry::{self, McpServerInfo}; + + let pid = std::process::id(); + let info = McpServerInfo { + pid, + 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(), + }; + 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 @@ -2251,6 +2350,465 @@ 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. 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."); + 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} LAUNCH 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) 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() + ); +} + +/// 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 " + } +} + +/// 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=" + } +} + +/// 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 +/// 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 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. +/// +/// `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 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 \ + 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( + "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 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 {} (codegraph {}) — {launched_for}\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). 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 +} + +/// Every live stdio MCP registry entry, plus `(path, error)` when the registry +/// 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)>, +) { + 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))) + } + } +} + +/// 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 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_live_entries(); + 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 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` +/// 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. +/// +/// 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 +/// 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 { + err.chain().find_map( + |cause| match cause.downcast_ref::() { + Some(codegraph_store::RebuildError::RemoveDatabase { .. }) => Some(()), + _ => None, + }, + )?; + 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)) +} + +#[cfg(test)] +mod index_holder_guidance_tests { + use super::{ + 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 { + 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("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:?}" + ); + assert!( + !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:?}" + ); + } + + /// 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:?}"); + } + + /// 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. fn print_http_table(servers: &[codegraph_daemon::http_registry::HttpServerInfo]) { if servers.is_empty() { @@ -3140,13 +3698,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(), @@ -3161,7 +3728,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..f2815ec --- /dev/null +++ b/crates/codegraph-cli/tests/impact_edge_count.rs @@ -0,0 +1,188 @@ +use std::collections::HashSet; +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 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"); + 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)); +} 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..952f48f --- /dev/null +++ b/crates/codegraph-cli/tests/index_holder_warning.rs @@ -0,0 +1,370 @@ +//! `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 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 +/// 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| { + 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. +#[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 + ); +} + +/// 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 empty_and_unreadable_registries_leave_the_success_path_unchanged() { + let dir = TestDir::new("no-holder"); + let project = indexed_project(&dir, "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 + ); + 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 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!( + outage.success, + "an unreadable registry must not fail the index: stderr={}", + outage.stderr + ); + assert_eq!( + normalize(&outage.stderr), + normalize(&baseline.stderr), + "stderr must be unchanged when there is no holder to report" + ); + assert_eq!( + 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 new file mode 100644 index 0000000..ee04de1 --- /dev/null +++ b/crates/codegraph-cli/tests/mcp_registry_cli.rs @@ -0,0 +1,501 @@ +//! `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 its server was LAUNCHED for: {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}" + ); + // 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"); +} + +/// (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}" + ); +} diff --git a/crates/codegraph-cli/tests/rmcp_serve_direct.rs b/crates/codegraph-cli/tests/rmcp_serve_direct.rs index c0e0f59..282c1c4 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}; @@ -75,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()) @@ -197,6 +213,285 @@ 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() + ); +} + +/// 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/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-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..d7eb26a --- /dev/null +++ b/crates/codegraph-daemon/src/mcp_registry.rs @@ -0,0 +1,487 @@ +//! 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 => { + // `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". + Err(error) => DirScan::Failed(error.to_string()), + } +} + +/// 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(); + 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) +} + +/// 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(); + 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 +/// 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()); + } + + /// `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"); + 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:?}") + } + } + } + + #[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"); + let pid = std::process::id(); + write_entry(&sample(pid)).unwrap(); + + assert!(remove_entry(pid)); + assert_eq!(read_entry(pid), None); + } +} diff --git a/crates/codegraph-graph/src/query/mod.rs b/crates/codegraph-graph/src/query/mod.rs index 7d678f7..8b71b5c 100644 --- a/crates/codegraph-graph/src/query/mod.rs +++ b/crates/codegraph-graph/src/query/mod.rs @@ -116,7 +116,8 @@ pub fn search_nodes( ) + scoring::name_match_bonus(&result.node.name, scoring_query); } - 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); } @@ -148,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 ca1cab1..2c13d1f 100644 --- a/crates/codegraph-graph/src/query/scoring.rs +++ b/crates/codegraph-graph/src/query/scoring.rs @@ -633,6 +633,17 @@ pub fn name_match_bonus(node_name: &str, query: &str) -> f64 { 0.0 } +/// Whether `node_name` equals the WHOLE `query`, compared case-insensitively. +/// +/// 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 { match kind { NodeKind::Function => 10.0, @@ -937,6 +948,13 @@ mod tests { assert_eq!(name_match_bonus("foo", "zzz"), 0.0); } + #[test] + 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] 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..cdac3b4 100644 --- a/crates/codegraph-graph/tests/search.rs +++ b/crates/codegraph-graph/tests/search.rs @@ -460,3 +460,167 @@ 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}" + ); +} + +#[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" + ); +} 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/crates/codegraph-watch/src/policy.rs b/crates/codegraph-watch/src/policy.rs index ef20b2f..9b0623d 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", @@ -80,10 +28,24 @@ 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 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 (`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, /// The addressed project's custom extension→language overrides, so a file the @@ -94,51 +56,51 @@ pub struct WatchPolicy { impl WatchPolicy { pub fn new(root: impl AsRef) -> Self { - Self::with_config(root, &[], &[]) + let indexing = codegraph_core::config::IndexingConfig::default(); + Self::with_config( + root, + &indexing.ignore_dirs, + &indexing.ignore_paths, + &[], + &[], + ) } - /// 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 { + /// Like [`new`](Self::new) but threads the project's configured `ignore_dirs`, + /// `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 { // 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 = DEFAULT_IGNORE_DIRS + let structural_ignores = ignore_dirs .iter() - .map(|dir| IgnoreRule { - pattern: format!("{dir}/"), - negated: false, - }) + .map(String::as_str) + .chain(WATCH_ONLY_DEFAULT_IGNORE_DIRS.iter().copied()) + .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 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. - 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, + ignore_paths: ignore_paths.to_vec(), + gitignore_rules, include: include.to_vec(), exclude: exclude.to_vec(), extensions: ExtensionOverrides::empty(), @@ -191,25 +153,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 { @@ -224,15 +190,65 @@ impl WatchPolicy { !force } - fn matches_rules(&self, rules: &[IgnoreRule], relative: &str, is_dir: bool) -> bool { + fn matches_structural(&self, relative: &str, is_dir: bool) -> bool { + self.structural_ignores + .iter() + .any(|pattern| rule_matches(pattern, relative, is_dir)) + } + + /// 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 = false; - for rule in rules { + 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; } } ignored } + + /// 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() + .any(|pattern| codegraph_extract::include_exclude_pattern_matches(pattern, relative)) + } } /// Whether an include `pattern` matches the FILE at root-relative `relative` @@ -542,11 +558,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")); } @@ -561,6 +587,71 @@ 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_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 indexing = codegraph_core::config::IndexingConfig::default(); + let include = ["addons/first_party/**".to_string()]; + 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. + 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")); + } + + #[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 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, &indexing.ignore_paths, &[], &[]); + + // 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] fn partial_segment_names_are_not_false_positives() { let dir = crate::sync::tests::TestDir::new("watch-policy-partial"); @@ -741,8 +832,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 indexing = codegraph_core::config::IndexingConfig::default(); let policy = WatchPolicy::with_config( dir.path(), + &indexing.ignore_dirs, + &indexing.ignore_paths, &["Tools/".to_string(), "Local/ts/app.ts".to_string()], &[], ); @@ -757,23 +851,44 @@ 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(), + &indexing.ignore_dirs, + &indexing.ignore_paths, + &["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(), + &indexing.ignore_dirs, + &indexing.ignore_paths, + &["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 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 policy = WatchPolicy::with_config(dir.path(), &[], &[]); + 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 3784684..d0ac18d 100644 --- a/crates/codegraph-watch/src/sync.rs +++ b/crates/codegraph-watch/src/sync.rs @@ -374,8 +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, 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 c7a08c3..d6ab89b 100644 --- a/crates/codegraph-watch/src/watcher.rs +++ b/crates/codegraph-watch/src/watcher.rs @@ -301,9 +301,10 @@ 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 ignore_paths: Vec, pub include: Vec, pub exclude: Vec, /// The addressed project's custom extension→language overrides (its @@ -322,6 +323,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 +334,10 @@ 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, + ignore_paths: indexing.ignore_paths, + include: indexing.include, + exclude: indexing.exclude, extensions: ExtensionOverrides::empty(), sync_fn: None, full_sync_fn: None, @@ -346,11 +350,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 +366,8 @@ impl WatchOptions { Self { 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, @@ -421,8 +425,14 @@ 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.ignore_paths, + &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)?, @@ -2290,15 +2300,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 +2319,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:?}" ); } diff --git a/crates/codegraph-watch/tests/include_scan_watch_parity.rs b/crates/codegraph-watch/tests/include_scan_watch_parity.rs index e6f5555..3609326 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, &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, &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)" @@ -109,3 +121,413 @@ 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, + &options.ignore_paths, + &[], + &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, + &options.ignore_paths, + &[], + &[], + ); + + 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 `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 +/// 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, + &options.ignore_paths, + &[], + &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" + ); +} + +/// `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" + ); +} diff --git a/docs/cli.md b/docs/cli.md index 7de4bdb..060b0ca 100644 --- a/docs/cli.md +++ b/docs/cli.md @@ -350,6 +350,34 @@ 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 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 +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 +568,99 @@ 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`, `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: + +- 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 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": "…" } } +``` + +**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` 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. + +**`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 **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 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. + +--- + ## Daemon, watch & environment variables ### How `serve --mcp` chooses a run mode @@ -629,6 +750,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 f5d5aed..fa963d0 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 | @@ -68,15 +86,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: +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 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 | +| 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. @@ -260,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 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. ---