diff --git a/crates/nexus-index/src/lib.rs b/crates/nexus-index/src/lib.rs index 7c65c8c..6041a0b 100644 --- a/crates/nexus-index/src/lib.rs +++ b/crates/nexus-index/src/lib.rs @@ -24,6 +24,7 @@ pub use project::{ record_auto_reindex, touch_and_catchup, touch_queried, IndexingStatus, }; pub use queries::{ - call_graph_dot, detect_changes, detect_dead_code, get_architecture, get_file_context, - open_store, plan_query, search_code, ArchitectureSummary, QueryPlanResult, + call_graph_dot, detect_changes, detect_changes_blast_radius, detect_dead_code, + get_architecture, get_file_context, open_store, plan_query, search_code, ArchitectureSummary, + BlastRadiusResult, QueryPlanResult, }; diff --git a/crates/nexus-index/src/queries.rs b/crates/nexus-index/src/queries.rs index 3082fba..dd9a9a3 100644 --- a/crates/nexus-index/src/queries.rs +++ b/crates/nexus-index/src/queries.rs @@ -1,4 +1,4 @@ -use crate::graph::{Direction, GraphStore}; +use crate::graph::{Direction, GraphStore, TracedNode}; use crate::project::graph_db_path; use crate::{CodeSearchHit, NodeRecord}; use anyhow::{bail, Result}; @@ -156,6 +156,16 @@ pub fn search_code(repo_path: &Path, query: &str, limit: u32) -> Result Result> { + Ok(detect_changes_direct(repo_path)?.0) +} + +/// Shared setup for `detect_changes`/`detect_changes_blast_radius`: resolves +/// and authorizes `repo_path`, runs `git diff --unified=0`, and maps the +/// touched hunks to the directly-overlapping graph symbols. Returns the +/// direct symbols alongside the opened `GraphStore` so the blast-radius path +/// can reuse the same store for its `trace_calls` walk instead of opening it +/// twice. +fn detect_changes_direct(repo_path: &Path) -> Result<(Vec, GraphStore)> { // Canonicalize *before* the allowed_roots check, not after - see the // matching comment in `get_file_context` below for why the ordering // itself is the bug (issue #29). @@ -183,7 +193,66 @@ pub fn detect_changes(repo_path: &Path) -> Result> { affected.extend(store.nodes_overlapping(&file, start, end)?); } } - Ok(affected) + Ok((affected, store)) +} + +/// Result of `detect_changes_blast_radius`: the directly-changed symbols +/// (identical to plain `detect_changes`) plus everything transitively +/// affected by them (their inbound callers, direct and indirect, found by +/// walking `GraphStore::trace_calls` from each directly-changed function). +pub struct BlastRadiusResult { + pub direct: Vec, + /// Transitively-affected callers, deduped against each other and + /// against `direct` (a node already in `direct` is never repeated here, + /// even if it's also a caller of another directly-changed symbol). + /// Carries the same `TracedNode` provenance (`edge_kind`) that + /// `trace_call_path` already surfaces, tagged with whichever edge kind + /// first reached it during the BFS - see `GraphStore::trace_calls`'s doc + /// comment. + pub transitive: Vec, + /// Distinct files touched across `direct` and `transitive` combined. + pub files_touched: usize, +} + +/// Blast-radius mode for `detect_changes` (issue #89): in addition to the +/// directly-changed symbols, walks `GraphStore::trace_calls` in the Inbound +/// direction ("who calls this changed thing") from each directly-changed +/// function, at `depth`, reusing the exact same BFS/provenance machinery +/// `trace_call_path` already exposes rather than a new traversal. `depth` is +/// expected to already be clamped by the caller (mirrors `trace_call_path`'s +/// own clamping convention - this function does no clamping of its own so +/// library callers control that policy). +pub fn detect_changes_blast_radius(repo_path: &Path, depth: u32) -> Result { + let (direct, store) = detect_changes_direct(repo_path)?; + + let direct_ids: HashSet = direct.iter().map(|n| n.id).collect(); + let mut seen_transitive: HashSet = HashSet::new(); + let mut transitive: Vec = Vec::new(); + + // One `trace_calls` walk per directly-changed *function* - types don't + // have callers in the CALLS graph, `trace_calls` itself only starts from + // nodes named like a Function (see its `WHERE kind = 'Function'`), so + // non-function direct nodes simply contribute nothing here rather than + // erroring. + for node in &direct { + for traced in store.trace_calls(&node.name, Direction::Inbound, depth)? { + let id = traced.node.id; + if direct_ids.contains(&id) || !seen_transitive.insert(id) { + continue; + } + transitive.push(traced); + } + } + + let mut files: HashSet<&str> = direct.iter().map(|n| n.file_path.as_str()).collect(); + files.extend(transitive.iter().map(|t| t.node.file_path.as_str())); + let files_touched = files.len(); + + Ok(BlastRadiusResult { + direct, + transitive, + files_touched, + }) } /// Minimal unified-diff hunk parser: pulls (file, [(start_line, end_line)]) @@ -672,3 +741,205 @@ mod plan_query_tests { let _ = fs::remove_dir_all(&dir); } } + +/// Tests for issue #89: blast-radius mode. Builds a real git repo + graph +/// index (not mocked) so `detect_changes_blast_radius` exercises the actual +/// `git diff` parsing, `nodes_overlapping` mapping, and `trace_calls` BFS +/// together, the same way an MCP caller would hit them. +#[cfg(test)] +mod blast_radius_tests { + use super::*; + use std::process::Command; + + /// Isolates each test from the real user config (and from each other) + /// the same way `path_security.rs` does - `NEXUS_CONFIG_DIR` pointed at + /// a scratch dir with no `config.toml` written, so + /// `require_path_allowed` falls back to `Config::default()`'s empty + /// `allowed_roots`, which permits everything (see + /// `nexus_core::config::empty_allowed_roots_permits_everything_unrestricted`). + struct FakeConfigDir { + prev: Option, + } + + impl FakeConfigDir { + fn set(label: &str) -> Self { + let prev = std::env::var_os("NEXUS_CONFIG_DIR"); + let dir = std::env::temp_dir().join(format!( + "nexus_blast_radius_test_config_{label}_{}", + std::process::id() + )); + let _ = std::fs::remove_dir_all(&dir); + std::fs::create_dir_all(&dir).unwrap(); + std::env::set_var("NEXUS_CONFIG_DIR", &dir); + Self { prev } + } + } + + impl Drop for FakeConfigDir { + fn drop(&mut self) { + match &self.prev { + Some(v) => std::env::set_var("NEXUS_CONFIG_DIR", v), + None => std::env::remove_var("NEXUS_CONFIG_DIR"), + } + } + } + + /// Serializes every test in this module against the process-global + /// `NEXUS_CONFIG_DIR` mutation `FakeConfigDir` performs. + static ENV_LOCK: std::sync::Mutex<()> = std::sync::Mutex::new(()); + + fn run_git(dir: &Path, args: &[&str]) { + let status = Command::new("git") + .arg("-C") + .arg(dir) + .args(args) + .status() + .expect("git must be on PATH for this test"); + assert!(status.success(), "git {args:?} failed in {dir:?}"); + } + + const SRC: &str = "\ +fn helper() { + let x = 1; +} + +fn caller_a() { + helper(); +} + +fn caller_b() { + helper(); +} + +fn caller_c() { + caller_a(); +} + +fn isolated() { + let y = 2; +} +"; + + /// Sets up a committed git repo with `SRC` indexed, ready for a caller + /// to make an uncommitted edit and call `detect_changes`/ + /// `detect_changes_blast_radius`. + fn setup_repo(label: &str) -> std::path::PathBuf { + let dir = std::env::temp_dir().join(format!( + "nexus_blast_radius_test_repo_{label}_{}", + std::process::id() + )); + let _ = std::fs::remove_dir_all(&dir); + std::fs::create_dir_all(&dir).unwrap(); + + std::fs::write(dir.join("lib.rs"), SRC).unwrap(); + + run_git(&dir, &["init", "-q"]); + run_git(&dir, &["config", "user.email", "test@example.com"]); + run_git(&dir, &["config", "user.name", "test"]); + run_git(&dir, &["add", "."]); + run_git(&dir, &["commit", "-q", "-m", "initial"]); + + crate::project::index_project(&dir).expect("indexing the test repo must succeed"); + dir + } + + #[test] + fn a_widely_called_function_surfaces_its_multi_hop_caller_set() { + let _lock = ENV_LOCK.lock().unwrap_or_else(|e| e.into_inner()); + let _fake_config = FakeConfigDir::set("widely_called"); + let dir = setup_repo("widely_called"); + + // Edit only inside `helper`'s body, same line count, so every other + // function's indexed line range is untouched - the diff hunk lands + // squarely on `helper` alone. + let edited = SRC.replace("let x = 1;", "let x = 2;"); + std::fs::write(dir.join("lib.rs"), edited).unwrap(); + + let result = detect_changes_blast_radius(&dir, 3).unwrap(); + + assert_eq!(result.direct.len(), 1, "only `helper` was directly edited"); + assert_eq!(result.direct[0].name, "helper"); + + let transitive_names: HashSet<&str> = result + .transitive + .iter() + .map(|t| t.node.name.as_str()) + .collect(); + // caller_a/caller_b call helper directly (depth 1); caller_c calls + // caller_a, so it's only reachable at depth 2 - asserting it's + // present proves this is a real multi-hop walk, not just depth-1 + // direct callers. + assert!(transitive_names.contains("caller_a")); + assert!(transitive_names.contains("caller_b")); + assert!( + transitive_names.contains("caller_c"), + "caller_c is a depth-2 (transitive) caller of helper via caller_a - expected it in the blast radius, got: {transitive_names:?}" + ); + assert!(!transitive_names.contains("isolated")); + // `helper` itself must never appear in its own transitive set even + // though it's technically reachable if the graph had a cycle back + // to it - the direct/transitive sets must stay disjoint. + assert!(!transitive_names.contains("helper")); + + assert_eq!(result.direct.len() + result.transitive.len(), 4); + // Everything lives in the one file in this fixture. + assert_eq!(result.files_touched, 1); + + let _ = std::fs::remove_dir_all(&dir); + } + + #[test] + fn an_isolated_leaf_change_reports_an_empty_transitive_set_not_an_error() { + let _lock = ENV_LOCK.lock().unwrap_or_else(|e| e.into_inner()); + let _fake_config = FakeConfigDir::set("isolated_leaf"); + let dir = setup_repo("isolated_leaf"); + + let edited = SRC.replace("let y = 2;", "let y = 3;"); + std::fs::write(dir.join("lib.rs"), edited).unwrap(); + + let result = detect_changes_blast_radius(&dir, 3).unwrap(); + + assert_eq!(result.direct.len(), 1); + assert_eq!(result.direct[0].name, "isolated"); + assert!( + result.transitive.is_empty(), + "a function with no callers must not over-report a transitive set: {:?}", + result + .transitive + .iter() + .map(|t| &t.node.name) + .collect::>() + ); + assert_eq!(result.files_touched, 1); + + let _ = std::fs::remove_dir_all(&dir); + } + + #[test] + fn default_detect_changes_output_is_unchanged_by_blast_radius_existing() { + let _lock = ENV_LOCK.lock().unwrap_or_else(|e| e.into_inner()); + let _fake_config = FakeConfigDir::set("default_unchanged"); + let dir = setup_repo("default_unchanged"); + + let edited = SRC.replace("let x = 1;", "let x = 2;"); + std::fs::write(dir.join("lib.rs"), edited).unwrap(); + + // The plain call and the `.0` half of the blast-radius call must + // agree exactly - proves `detect_changes` still returns just the + // directly-changed symbols, unaffected by the blast-radius code + // path existing alongside it. + let plain = detect_changes(&dir).unwrap(); + let blast = detect_changes_blast_radius(&dir, 3).unwrap(); + + assert_eq!(plain.len(), blast.direct.len()); + for (a, b) in plain.iter().zip(blast.direct.iter()) { + assert_eq!(a.id, b.id); + assert_eq!(a.name, b.name); + assert_eq!(a.file_path, b.file_path); + assert_eq!(a.start_line, b.start_line); + assert_eq!(a.end_line, b.end_line); + } + + let _ = std::fs::remove_dir_all(&dir); + } +} diff --git a/crates/nexusd/src/tools.rs b/crates/nexusd/src/tools.rs index 69077ce..c7d62dc 100644 --- a/crates/nexusd/src/tools.rs +++ b/crates/nexusd/src/tools.rs @@ -189,10 +189,15 @@ pub fn tool_definitions() -> Value { }, { "name": "detect_changes", - "description": "Map uncommitted git changes to affected graph symbols (functions/types whose line range overlaps a diff hunk).", + "description": "Map uncommitted git changes to affected graph symbols (functions/types whose line range overlaps a diff hunk). Optional `blast_radius=true` additionally walks inbound callers (direct and transitive) of each changed function via the same BFS `trace_call_path` uses, up to `depth` hops - each transitive node carries the same `provenance`/`resolution`/`confidence` fields `trace_call_path` returns. Default (`blast_radius=false`) response and cost are unchanged from the plain symbol list.", "inputSchema": { "type": "object", - "properties": { "repo_path": { "type": "string" } }, + "properties": { + "repo_path": { "type": "string" }, + "blast_radius": { "type": "boolean", "default": false }, + "depth": { "type": "integer", "default": 3 }, + "limit": { "type": "integer", "default": 100 } + }, "required": ["repo_path"] } }, @@ -828,8 +833,38 @@ fn query_graph(args: Value) -> Result { fn detect_changes(args: Value) -> Result { let repo_path = repo_path_arg(&args)?; - let affected = index::detect_changes(&repo_path)?; - Ok(serde_json::to_string_pretty(&records_to_json(&affected))?) + let blast_radius = args + .get("blast_radius") + .and_then(|v| v.as_bool()) + .unwrap_or(false); + + // blast_radius=false (the default) must stay byte-for-byte identical to + // the tool's pre-#89 response and cost - this branch calls the same + // plain `detect_changes` it always has, never touching the BFS below. + if !blast_radius { + let affected = index::detect_changes(&repo_path)?; + return Ok(serde_json::to_string_pretty(&records_to_json(&affected))?); + } + + let depth = clamp_depth(args.get("depth").and_then(|v| v.as_u64()).unwrap_or(3) as u32); + let limit = + clamp_limit(args.get("limit").and_then(|v| v.as_u64()).unwrap_or(100) as u32) as usize; + + let result = index::detect_changes_blast_radius(&repo_path, depth)?; + let direct_count = result.direct.len(); + let transitive_total = result.transitive.len(); + let transitive_shown: Vec<_> = result.transitive.into_iter().take(limit).collect(); + Ok(serde_json::to_string_pretty(&json!({ + "direct": records_to_json(&result.direct), + "transitive_total": transitive_total, + "transitive_shown": transitive_shown.len(), + "transitive": traced_nodes_to_json(&transitive_shown), + "summary": { + "direct_count": direct_count, + "transitive_count": transitive_total, + "files_touched": result.files_touched, + } + }))?) } #[cfg(test)] diff --git a/docs/NexusContext-Wiki/ADRs/0016-blast-radius-mode-reuses-trace-calls-not-a-new-subsystem.md b/docs/NexusContext-Wiki/ADRs/0016-blast-radius-mode-reuses-trace-calls-not-a-new-subsystem.md new file mode 100644 index 0000000..d5ecd35 --- /dev/null +++ b/docs/NexusContext-Wiki/ADRs/0016-blast-radius-mode-reuses-trace-calls-not-a-new-subsystem.md @@ -0,0 +1,71 @@ +# 0016. Blast-radius mode for `detect_changes` reuses `trace_calls`/clamping, not a new subsystem + +Status: Accepted +Date: 2026-08-27 + +## Context + +Issue #89 asked for a richer `detect_changes`: instead of only "these +symbols were directly edited," an opt-in mode that also answers "what +depends on this, transitively, and how big is the blast radius." The issue +itself named the risk explicitly: don't build a new traversal or a new +test-coverage-mapping subsystem to get there - the graph can already answer +"who calls this" via `GraphStore::trace_calls` (the same BFS +`trace_call_path` exposes, with per-node `edge_kind` provenance since +[[0013-provenance-confidence-surfaced-through-trace-call-path]]), and the +depth/limit bounding this needs already exists as `clamp_depth`/ +`clamp_limit` from #58's work. + +## Decision + +`detect_changes_blast_radius(repo_path, depth)` (`crates/nexus-index/src/ +queries.rs`) is additive, not a rewrite: `detect_changes_direct`, a small +internal helper, factors out the existing git-diff-to-`NodeRecord` mapping +so both plain `detect_changes` and the new blast-radius path share it +byte-for-byte instead of diverging. Blast-radius mode then runs one +`GraphStore::trace_calls(name, Direction::Inbound, depth)` call per +directly-changed function - the exact same BFS `trace_call_path` already +runs, just started from every changed symbol instead of one caller-named +one - and unions/dedupes the results (excluding anything already in the +direct set) into a `BlastRadiusResult { direct, transitive, files_touched }`. +Each `transitive` entry is a `TracedNode`, carrying the same `edge_kind` +provenance `trace_call_path` already surfaces. + +`crates/nexusd/src/tools.rs`'s `detect_changes` MCP handler adds +`blast_radius` (bool, default `false`), plus `depth`/`limit` - named and +clamped (`clamp_depth`/`clamp_limit`) identically to `trace_call_path`'s own +parameters, deliberately, so an agent that has used one immediately +understands the other. When `blast_radius` is `false` (the default), the +handler takes the exact same code path it always has - `index::detect_changes` +- and returns the exact same response shape; the blast-radius branch (BFS, +provenance mapping, summary counts) is not reached at all, so existing +callers see no behavior or cost change. When `true`, the response adds +`transitive`/`transitive_total`/`transitive_shown` (capped by `limit`, same +total-vs-shown honesty pattern as `trace_call_path`/`detect_dead_code`) and +a `summary` (`direct_count`, `transitive_count`, `files_touched`). + +## Scope + +Per the issue's explicit instruction: no test-coverage-mapping ("which +tests cover this changed code") was attempted - that's not answerable from +the current graph's structural signals without a new subsystem, so it's +left out entirely rather than approximated. + +## Consequences + +- No new BFS, no new bounding scheme, no new provenance model - a bug fix + or improvement to `trace_calls`'s traversal or `edge_kind_provenance`'s + tagging benefits `detect_changes`'s blast-radius mode automatically. +- `BlastRadiusResult`/`detect_changes_blast_radius` are additive exports + from `nexus-index` (`crates/nexus-index/src/lib.rs`); no existing + function signature changed. +- Cost is opt-in by construction: `blast_radius=true` runs one `trace_calls` + walk per directly-changed function, so a diff touching many symbols is + proportionally more expensive - same tradeoff `trace_call_path` already + makes explicit via `depth`/`limit`, not a new unbounded-cost shape. + +## Related + +[[MCP-Tools]] · +[[0013-provenance-confidence-surfaced-through-trace-call-path]] · +[[0014-resource-observability-and-closing-out-58]] diff --git a/docs/NexusContext-Wiki/ADRs/README.md b/docs/NexusContext-Wiki/ADRs/README.md index 3961427..2909aa0 100644 --- a/docs/NexusContext-Wiki/ADRs/README.md +++ b/docs/NexusContext-Wiki/ADRs/README.md @@ -65,6 +65,7 @@ history stays legible. | [0013](0013-provenance-confidence-surfaced-through-trace-call-path.md) | `CALLS` vs `CALLS_RESOLVED` provenance/confidence is surfaced through `trace_call_path`'s JSON response | Accepted | | [0014](0014-resource-observability-and-closing-out-58.md) | Resource observability closes out issue #58, with two items explicitly deferred | Accepted | | [0015](0015-o-nofollow-defense-in-depth-not-full-toctou-proofing.md) | O_NOFOLLOW is defense-in-depth against symlink-substitution TOCTOU, not full TOCTOU-proofing | Accepted | +| [0016](0016-blast-radius-mode-reuses-trace-calls-not-a-new-subsystem.md) | Blast-radius mode for `detect_changes` reuses `trace_calls`/clamping, not a new subsystem | Accepted | ## Related diff --git a/docs/NexusContext-Wiki/MCP-Tools.md b/docs/NexusContext-Wiki/MCP-Tools.md index 65e9354..be5e082 100644 --- a/docs/NexusContext-Wiki/MCP-Tools.md +++ b/docs/NexusContext-Wiki/MCP-Tools.md @@ -26,7 +26,7 @@ deliberate, tested property rather than an assumption. | Tool | What it does | |---|---| | `detect_dead_code` | Functions with no inbound `CALLS`/`CALLS_RESOLVED` edge (excluding `main`). High false-positive rate is expected and stated in the tool's own description — treat hits as leads, not conclusions. Optional `path_prefix` (e.g. `"pkg/events"`) scopes results to a subdirectory or exact file — a real path-prefix match, not a naive string prefix, so it won't false-match a sibling directory like `pkg/events-vendor`. Fixes issue #77: on a monorepo, an unscoped call was dominated by vendored/generated code. | -| `detect_changes` | Maps uncommitted git changes to the graph symbols whose line range overlaps a diff hunk. | +| `detect_changes` | Maps uncommitted git changes to the graph symbols whose line range overlaps a diff hunk. Default response/cost is just that list, unchanged. Optional `blast_radius=true` (issue #89) additionally walks inbound callers of each changed function — direct and transitive, via the same `trace_calls` BFS `trace_call_path` uses, with the same `depth`/`limit` clamping (see [[Security-Model]]) and the same per-node `provenance`/`resolution`/`confidence` tagging. Response then has `direct` (the changed symbols), `transitive`/`transitive_total`/`transitive_shown` (its inbound callers, capped by `limit`), and a `summary` (`direct_count`, `transitive_count`, `files_touched`). | | `query_planner` | Picks the cheapest retrieval strategy (file read / symbol search / keyword fallback) instead of the agent guessing, and returns the actual answer in-band alongside which strategy it used — no second call needed. Also carries `index_freshness`. | ## Observability