Fix false call edges and dropped module-graph edges; harden pop/path guards

Two correctness fixes surfaced by a bug-hunt pass over the codebase, plus two
defensive guards for currently-unreachable panics.

Fixes:
- pythonextractor: resolveCall no longer fabricates same-module RelCalls edges
  for callable parameters, locals, and loop variables. It now mirrors
  valueRefTarget — skipping parameter names and resolving only known module-level
  defs (via idx.moduleDefs) when an index is present, with a best-effort fallback
  for the index-less single-file path. Removes spurious "used" signals, inflated
  fan-out, and misleading impact analysis.
- explainers/common: BuildModuleGraph no longer pre-filters imports through
  IsExternalImport, which misclassified single-segment internal modules
  (e.g. top-level "cmd", "config") as external and dropped real edges. The
  authoritative moduleNames gate now decides inclusion; IsExternalImport is left
  unchanged so Go stdlib / npm classification is preserved.

Hardening (defensive; not currently reachable):
- pythonextractor: popOwner/popType are now no-ops on empty stacks instead of
  panicking on a slice-bounds underflow.
- server: bestPath returns a clean not-found PathResult for empty candidate
  slices instead of indexing fromCands[0]/toCands[0] out of range.

All changes covered by unit tests. Also initializes moduleDefs in the
astExtractWithIndex test helper so it mirrors the production index.

Author: dejo1307

internal/explainers/common/common.go

@@ -107,14 +107,14 @@ func BuildModuleGraph(store *facts.Store) map[string][]string {
 			}
 			target := rel.Target
 
-			if IsExternalImport(target) {
-				continue
-			}
-
 			if strings.HasPrefix(target, ".") {
 				target = ResolveRelativeImport(sourceModule, target)
 			}
 
+			// A target is included only if it names a known internal module. This gate is
+			// authoritative; we deliberately do NOT pre-filter with IsExternalImport, which
+			// misclassifies single-segment internal modules (top-level "cmd", "config") as
+			// external and would drop real edges before this check.
 			if moduleNames[target] {
 				graph[sourceModule] = append(graph[sourceModule], target)
 			}

internal/explainers/common/common_test.go

@@ -233,3 +233,47 @@ func TestBuildModuleGraph_ExcludesTestRole(t *testing.T) {
 		t.Errorf("edge to a test-role module should be dropped, got %v", graph["src/app"])
 	}
 }
+
+// TestBuildModuleGraph_SingleSegmentInternalModule: a top-level internal module
+// with a single-segment name (e.g. "config") must be included as an import edge.
+// Before the fix, IsExternalImport("config") returned true and the edge was
+// dropped before the authoritative moduleNames gate.
+func TestBuildModuleGraph_SingleSegmentInternalModule(t *testing.T) {
+	s := facts.NewStore()
+	s.Add(facts.Fact{Kind: facts.KindModule, Name: "config"})
+	s.Add(facts.Fact{Kind: facts.KindModule, Name: "handlers"})
+	s.Add(facts.Fact{
+		Kind:      facts.KindDependency,
+		File:      "handlers/h.go",
+		Relations: []facts.Relation{{Kind: facts.RelImports, Target: "config"}},
+	})
+
+	graph := BuildModuleGraph(s)
+
+	if edges := graph["handlers"]; len(edges) != 1 || edges[0] != "config" {
+		t.Errorf("handlers edges = %v, want [config]", edges)
+	}
+}
+
+// TestBuildModuleGraph_ExternalStillDropped: single-segment names that are NOT
+// declared modules (Go stdlib "fmt", npm "react") must still be dropped — the
+// moduleNames gate remains authoritative after removing the IsExternalImport
+// pre-filter.
+func TestBuildModuleGraph_ExternalStillDropped(t *testing.T) {
+	s := facts.NewStore()
+	s.Add(facts.Fact{Kind: facts.KindModule, Name: "handlers"})
+	s.Add(facts.Fact{
+		Kind: facts.KindDependency,
+		File: "handlers/h.go",
+		Relations: []facts.Relation{
+			{Kind: facts.RelImports, Target: "fmt"},
+			{Kind: facts.RelImports, Target: "react"},
+		},
+	})
+
+	graph := BuildModuleGraph(s)
+
+	if edges := graph["handlers"]; len(edges) != 0 {
+		t.Errorf("handlers edges = %v, want none (fmt/react are not modules)", edges)
+	}
+}

internal/extractors/pythonextractor/python_ast.go

@@ -141,7 +141,11 @@ func (w *pyWalker) recordCallMetrics(target string) {
 }
 
 func (w *pyWalker) pushOwner(idx int) { w.ownerStack = append(w.ownerStack, idx) }
-func (w *pyWalker) popOwner()         { w.ownerStack = w.ownerStack[:len(w.ownerStack)-1] }
+func (w *pyWalker) popOwner() {
+	if len(w.ownerStack) > 0 {
+		w.ownerStack = w.ownerStack[:len(w.ownerStack)-1]
+	}
+}
 func (w *pyWalker) currentOwner() *facts.Fact {
 	if len(w.ownerStack) == 0 {
 		return nil
@@ -164,8 +168,12 @@ func (w *pyWalker) pushType(name string, methods map[string]bool) {
 }
 
 func (w *pyWalker) popType() {
-	w.typeStack = w.typeStack[:len(w.typeStack)-1]
-	w.methodSets = w.methodSets[:len(w.methodSets)-1]
+	if len(w.typeStack) > 0 {
+		w.typeStack = w.typeStack[:len(w.typeStack)-1]
+	}
+	if len(w.methodSets) > 0 {
+		w.methodSets = w.methodSets[:len(w.methodSets)-1]
+	}
 }
 
 func (w *pyWalker) currentMethods() map[string]bool {
@@ -1493,7 +1501,20 @@ func (w *pyWalker) resolveCall(name string) string {
 	if target, ok := w.importMap[name]; ok {
 		return target // "" means external → no edge
 	}
-	// Same-module top-level function.
+	// Same-module top-level function. A bare callee that shadows a parameter is the
+	// parameter, not the module-level def (e.g. def wrapper(cb): cb()). When an index
+	// is available, resolve only names that are actually module-level defs, so callable
+	// locals/params/loop vars don't fabricate edges. Without an index (single-file
+	// extraction) fall back to best-effort; production always supplies one.
+	if w.paramNames[name] {
+		return ""
+	}
+	if w.idx != nil {
+		if w.idx.moduleDefs[w.module][name] {
+			return w.module + "." + name
+		}
+		return ""
+	}
 	return w.module + "." + name
 }
 

internal/extractors/pythonextractor/python_ast_test.go

@@ -27,7 +27,7 @@ func astExtract(t *testing.T, filename, src string, isDjango bool) []facts.Fact
 // extracts facts from targetFile using that index.
 func astExtractWithIndex(t *testing.T, files map[string]string, targetFile string, isDjango bool) []facts.Fact {
 	t.Helper()
-	idx := &pySymbolIndex{classes: make(map[string]*pyClassInfo)}
+	idx := &pySymbolIndex{classes: make(map[string]*pyClassInfo), moduleDefs: make(map[string]map[string]bool)}
 	for filename, src := range files {
 		buildFileIndex([]byte(src), filename, idx)
 	}
@@ -1173,3 +1173,117 @@ def build(a, b):
 		}
 	}
 }
+
+// --- resolveCall must not fabricate edges for params/locals/loop vars (bug 05) ---
+
+// hasCallTo reports whether fact f has a RelCalls edge to target.
+func hasCallTo(f facts.Fact, target string) bool {
+	for _, c := range relsByKind(f, facts.RelCalls) {
+		if c == target {
+			return true
+		}
+	}
+	return false
+}
+
+// TestAST_ParamCall_NoEdge: a callable parameter invoked by name must not resolve
+// to a same-module symbol.
+func TestAST_ParamCall_NoEdge(t *testing.T) {
+	files := map[string]string{
+		"svc.py": `
+def wrapper(callback):
+    callback()
+`,
+	}
+	result := astExtractWithIndex(t, files, "svc.py", false)
+	idx := byName(result)
+
+	fn, ok := idx["svc.wrapper"]
+	if !ok {
+		t.Fatalf("missing svc.wrapper; keys: %v", keys(idx))
+	}
+	if hasCallTo(fn, "svc.callback") {
+		t.Errorf("param 'callback' wrongly resolved to svc.callback; calls=%v", relsByKind(fn, facts.RelCalls))
+	}
+}
+
+// TestAST_LocalCallable_NoEdge: a locally-assigned callable invoked by name must
+// not resolve to a same-module symbol (it is not a module-level def).
+func TestAST_LocalCallable_NoEdge(t *testing.T) {
+	files := map[string]string{
+		"svc.py": `
+def f():
+    fn = lambda: None
+    fn()
+`,
+	}
+	result := astExtractWithIndex(t, files, "svc.py", false)
+	idx := byName(result)
+
+	fn, ok := idx["svc.f"]
+	if !ok {
+		t.Fatalf("missing svc.f; keys: %v", keys(idx))
+	}
+	if hasCallTo(fn, "svc.fn") {
+		t.Errorf("local 'fn' wrongly resolved to svc.fn; calls=%v", relsByKind(fn, facts.RelCalls))
+	}
+}
+
+// TestAST_LoopVarCall_NoEdge: a loop variable invoked by name must not resolve to
+// a same-module symbol.
+func TestAST_LoopVarCall_NoEdge(t *testing.T) {
+	files := map[string]string{
+		"svc.py": `
+def f(handlers):
+    for handler in handlers:
+        handler()
+`,
+	}
+	result := astExtractWithIndex(t, files, "svc.py", false)
+	idx := byName(result)
+
+	fn, ok := idx["svc.f"]
+	if !ok {
+		t.Fatalf("missing svc.f; keys: %v", keys(idx))
+	}
+	if hasCallTo(fn, "svc.handler") {
+		t.Errorf("loop var 'handler' wrongly resolved to svc.handler; calls=%v", relsByKind(fn, facts.RelCalls))
+	}
+}
+
+// TestAST_SameModuleCall_StillResolves: a genuine same-module top-level def call
+// must still emit a RelCalls edge (regression guard for the bug 05 fix).
+func TestAST_SameModuleCall_StillResolves(t *testing.T) {
+	files := map[string]string{
+		"svc.py": `
+def helper():
+    pass
+
+def main():
+    helper()
+`,
+	}
+	result := astExtractWithIndex(t, files, "svc.py", false)
+	idx := byName(result)
+
+	fn, ok := idx["svc.main"]
+	if !ok {
+		t.Fatalf("missing svc.main; keys: %v", keys(idx))
+	}
+	if !hasCallTo(fn, "svc.helper") {
+		t.Errorf("main: expected RelCalls to svc.helper; got %v", relsByKind(fn, facts.RelCalls))
+	}
+}
+
+// TestAST_PopOnEmptyStack_NoPanic: popOwner/popType must be no-ops on empty
+// stacks rather than panicking with a slice-bounds underflow (defensive hardening).
+func TestAST_PopOnEmptyStack_NoPanic(t *testing.T) {
+	w := &pyWalker{}
+	// Must not panic on empty stacks.
+	w.popOwner()
+	w.popType()
+	if len(w.ownerStack) != 0 || len(w.typeStack) != 0 || len(w.methodSets) != 0 {
+		t.Fatalf("expected all stacks to remain empty, got owner=%d type=%d methodSets=%d",
+			len(w.ownerStack), len(w.typeStack), len(w.methodSets))
+	}
+}

internal/server/server.go

@@ -1963,12 +1963,26 @@ func (s *Server) pathCandidates(store *facts.Store, input, resolved string, res
 	return out
 }
 
+// firstOr returns the first element of s, or fallback when s is empty.
+func firstOr(s []string, fallback string) string {
+	if len(s) == 0 {
+		return fallback
+	}
+	return s[0]
+}
+
 // bestPath tries to connect any from-candidate to any to-candidate, expanding each
 // to-candidate with RollupSeeds (a path to a type usually ends at one of its
 // methods/constructor). It scans in ranked order, keeps the shortest path found,
 // and is bounded by maxPathAttempts (returning early on a ≤2-hop hit). When no
 // path exists it returns a not-found PathResult naming the first from/to tried.
 func (s *Server) bestPath(graph *facts.Graph, fromCands, toCands []string, relKinds []string, maxDepth int) facts.PathResult {
+	// Self-defensive: callers are expected to pass non-empty candidate slices, but
+	// guard so an empty slice yields a clean not-found result instead of indexing
+	// fromCands[0]/toCands[0] out of range.
+	if len(fromCands) == 0 || len(toCands) == 0 {
+		return facts.PathResult{From: firstOr(fromCands, ""), To: firstOr(toCands, ""), Found: false}
+	}
 	var best facts.PathResult
 	attempts := 0
 	for _, from := range fromCands {

internal/server/server_test.go

@@ -1913,6 +1913,28 @@ func TestBestPath_NoPath(t *testing.T) {
 	}
 }
 
+// TestBestPath_EmptyCandidates_NoPanic: bestPath must return a clean not-found
+// result rather than panicking on fromCands[0]/toCands[0] when either candidate
+// slice is empty (defensive hardening; callers are expected to pre-filter).
+func TestBestPath_EmptyCandidates_NoPanic(t *testing.T) {
+	store := facts.NewStore()
+	store.Add(facts.Fact{Kind: facts.KindSymbol, Name: "a.Foo", Props: map[string]any{"symbol_kind": "struct"}})
+	store.BuildGraph()
+	srv := newTestServer(store)
+
+	cases := []struct{ from, to []string }{
+		{nil, []string{"a.Foo"}},
+		{[]string{"a.Foo"}, nil},
+		{nil, nil},
+	}
+	for _, tc := range cases {
+		res := srv.bestPath(store.Graph(), tc.from, tc.to, nil, 0)
+		if res.Found {
+			t.Errorf("bestPath(from=%v, to=%v): Found = true, want false", tc.from, tc.to)
+		}
+	}
+}
+
 func TestBuildCoverageReport_Classifications(t *testing.T) {
 	store := facts.NewStore()
 	store.Add(