diff --git a/CHANGELOG.md b/CHANGELOG.md index be520a9..2043168 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,6 +19,9 @@ Follows the first real-agent A/B (bench/agent-ab, pull request #77), which found ### Changed - The MCP handshake instructions and `stacklit guide` use the routed wording instead of "source of truth, do not crawl" (about 100 tokens, was about 75). `STACKLIT_STEERING=d` keeps the earlier text for comparison. +### Fixed +- Python absolute imports resolve when the import root is below the scanned root, for example `app.core.db` for `apps/api/app/core/db.py`. Before, such a repo had no module edges and every module was listed as isolated. A single segment import only matches a package, so `import logging` does not resolve to a repo file called `logging.py`. + ### Documentation - `docs/SESSIONS.md` and `docs/USAGE.md` report what the first A/B measured. `--crawl-guard` and steering `d` measured worse than no Stacklit (cost 1.12x, correct 82.7% against 94.0%): off by default, not recommended. Added a "When Stacklit helps" section: large repos and locate or add questions, not exact-name lookups or small repos. diff --git a/internal/graph/modresolve.go b/internal/graph/modresolve.go index bb63337..84cd3cb 100644 --- a/internal/graph/modresolve.go +++ b/internal/graph/modresolve.go @@ -12,13 +12,16 @@ import ( ) // langResolver maps language-specific import strings (Rust paths, Java -// packages, Ruby requires) to module names. The generic resolver only knows +// packages, Python absolute imports, Ruby requires) to module names. The generic resolver only knows // directory-style paths, which these languages never use. type langResolver struct { root string fileMod map[string]string // slash path -> module of that file dirMod map[string]string // slash dir -> module of files in that dir javaPkgs map[string][]string // trailing dir path -> sorted unique modules + pyPkgs map[string][]string // trailing dir path of a .py file -> sorted unique modules + pyFiles map[string][]string // trailing path of a .py file without .py -> sorted unique modules + pyTop map[string]bool // directory names that hold an __init__.py (importable packages) rubyLibs map[string][]string // require path without .rb -> sorted unique file paths crates map[string]string // Rust crate identifier -> its source dir seenSrc map[string]bool @@ -32,11 +35,15 @@ func newLangResolver(files []*parser.FileInfo, root string, modOf map[string]str fileMod: map[string]string{}, dirMod: map[string]string{}, javaPkgs: map[string][]string{}, + pyPkgs: map[string][]string{}, + pyFiles: map[string][]string{}, + pyTop: map[string]bool{}, rubyLibs: map[string][]string{}, crates: map[string]string{}, seenSrc: map[string]bool{}, } javaSeen := map[string]map[string]bool{} + pySeen := map[string]bool{} for _, f := range files { p := filepath.ToSlash(f.Path) mod := modOf[f.Path] @@ -54,6 +61,8 @@ func newLangResolver(files []*parser.FileInfo, root string, modOf map[string]str r.javaPkgs[suf] = append(r.javaPkgs[suf], mod) } } + case ".py": + r.notePython(p, dir, mod, pySeen) case ".rb": noExt := strings.TrimSuffix(p, path.Ext(p)) for _, suf := range dirSuffixes(noExt) { @@ -66,6 +75,12 @@ func newLangResolver(files []*parser.FileInfo, root string, modOf map[string]str for k := range r.javaPkgs { sort.Strings(r.javaPkgs[k]) } + for k := range r.pyPkgs { + sort.Strings(r.pyPkgs[k]) + } + for k := range r.pyFiles { + sort.Strings(r.pyFiles[k]) + } for k := range r.rubyLibs { sort.Strings(r.rubyLibs[k]) } @@ -138,6 +153,15 @@ func (r *langResolver) resolve(f *parser.FileInfo, imp string) (mod string, hand return r.rust(p, imp), true case ".java": return r.java(p, imp), true + case ".py": + // Relative imports and anything unmatched stay with the generic resolver. + if strings.HasPrefix(imp, ".") { + return "", false + } + if m := r.python(p, imp); m != "" { + return m, true + } + return "", false case ".rb": return r.ruby(p, imp), true } @@ -250,6 +274,51 @@ func (r *langResolver) java(p, imp string) string { return "" } +// ---- Python ---- + +// notePython indexes a .py file under every trailing sub-path of its directory +// and of its own path, so `app.core.db` can find apps/api/app/core/db.py when +// the import root (apps/api) is a subdirectory of the scanned root. +func (r *langResolver) notePython(p, dir, mod string, seen map[string]bool) { + add := func(idx map[string][]string, kind, suf string) { + k := kind + "|" + suf + "|" + mod + if seen[k] { + return + } + seen[k] = true + idx[suf] = append(idx[suf], mod) + } + if path.Base(p) == "__init__.py" { + r.pyTop[path.Base(dir)] = true + } else { + for _, suf := range dirSuffixes(strings.TrimSuffix(p, ".py")) { + add(r.pyFiles, "f", suf) + } + } + for _, suf := range dirSuffixes(dir) { + add(r.pyPkgs, "d", suf) + } +} + +// python resolves an absolute dotted import (`app.core.db`, `app.core`) to a +// module by the longest leading part of the import that is the tail of a file +// or directory path. A single-segment import only matches a real package (a +// directory with an __init__.py), so `import logging` does not land on a +// repo file called logging.py. +func (r *langResolver) python(p, imp string) string { + segs := strings.Split(imp, ".") + for n := len(segs); n >= 1; n-- { + key := strings.Join(segs[:n], "/") + if mods := r.pyFiles[key]; n >= 2 && len(mods) > 0 { + return closestModule(p, mods) + } + if mods := r.pyPkgs[key]; len(mods) > 0 && (n >= 2 || r.pyTop[segs[0]]) { + return closestModule(p, mods) + } + } + return "" +} + // closestModule picks the candidate sharing the longest path prefix with p. func closestModule(p string, mods []string) string { best, bestScore := mods[0], -1 diff --git a/internal/graph/modresolve_test.go b/internal/graph/modresolve_test.go index 34f917c..090f30a 100644 --- a/internal/graph/modresolve_test.go +++ b/internal/graph/modresolve_test.go @@ -150,3 +150,57 @@ func TestModuleRankingPrefersRichFiles(t *testing.T) { t.Errorf("ExportsByRank = %v", m.ExportsByRank) } } + +// Absolute imports are written from the Python import root (apps/api), which +// is a subdirectory of the scanned root, so `app.core.db` has to match the +// tail of apps/api/app/core/db.py. +func TestPythonAbsoluteImportsFromSubdirectoryRoot(t *testing.T) { + files := []*parser.FileInfo{ + fi("apps/api/app/__init__.py"), + fi("apps/api/app/main.py", "app.core", "app.domains.spend.router", "fastapi"), + fi("apps/api/app/core/__init__.py"), + fi("apps/api/app/core/db.py", "alembic", "sqlalchemy"), + fi("apps/api/app/core/logging.py"), + fi("apps/api/app/domains/spend/__init__.py"), + fi("apps/api/app/domains/spend/router.py", "app.core.db", "app.domains.spend.service", "logging"), + fi("apps/api/app/domains/spend/service.py", "app.core.db.Session", ".router"), + fi("apps/api/alembic/env.py"), + } + g := Build(files, BuildOptions{MaxDepth: 6}) + + core := "apps/api/app/core" + spend := "apps/api/app/domains/spend" + if got, want := dependedBy(g, core), []string{"apps/api/app", spend}; !reflect.DeepEqual(got, want) { + t.Errorf("core depended_by = %v, want %v", got, want) + } + // main.py reaches spend through app.domains.spend.router (a file, not a package). + if got, want := dependedBy(g, spend), []string{"apps/api/app"}; !reflect.DeepEqual(got, want) { + t.Errorf("spend depended_by = %v, want %v", got, want) + } + // `logging` is the standard library and `alembic` is a third-party package, even though + // the repo has a logging.py and an alembic/ directory without an __init__.py. + if got := dependedBy(g, "apps/api/alembic"); len(got) != 0 { + t.Errorf("alembic depended_by = %v, want none", got) + } + for _, m := range g.Module(core).DependsOn { + t.Errorf("core should depend on nothing, got %s (stdlib logging or third-party alembic matched a repo path)", m) + } +} + +func TestPythonSingleSegmentImportNeedsAPackage(t *testing.T) { + files := []*parser.FileInfo{ + fi("svc/app/__init__.py"), + fi("svc/app/models.py"), + fi("svc/worker/run.py", "app", "app.models"), + fi("svc/util/helpers.py"), + fi("svc/worker/jobs.py", "helpers"), + } + g := Build(files, BuildOptions{}) + if got, want := dependedBy(g, "svc/app"), []string{"svc/worker"}; !reflect.DeepEqual(got, want) { + t.Errorf("app depended_by = %v, want %v", got, want) + } + // helpers is a plain file with no package around it, so a bare `import helpers` stays unresolved. + if got := dependedBy(g, "svc/util"); len(got) != 0 { + t.Errorf("util depended_by = %v, want none", got) + } +}