From e6db5c610cc077ff3779103369bc271c41849098 Mon Sep 17 00:00:00 2001 From: Jordan Coin Jackson Date: Thu, 5 Nov 2026 22:17:16 -0500 Subject: [PATCH 1/2] fix(go): list a multi-file Go package as one hub, not one per file Go import edges are package-level, so every non-test file of a package carries the same importer set. Hub lists ranked those files as separate hubs (get_hubs on this repo: 95 entries, 20 of them scanner/*.go at 73 importers each; the handoff prefix filled its 15 slots with scanner files), and --importers raised HUB FILE on a file whose count belongs to its package. - scanner: CollapseGoPackageHubs groups .go files of one directory that share an importer set into one "dir/ (package, F files, N importers)" entry. It keys on the importer set rather than go.mod, so it also works on the daemon-state importer map. IsFileHub/SharedGoPackage make the per-file hub flag false for files of a multi-file package; a one-file package keeps it. - get_hubs, get_structure, session-start hub list and the handoff prefix use the collapsed list (HubSummary gains an omitempty files field). - --importers (CLI and MCP) says "Package X is imported by N files" with no HUB FILE warning; is_hub is false in the JSON report; "Imports N hub(s)" counts collapsed entries. - find labels a package-level count "(of package X)" instead of "(hub)". Non-Go files and one-file Go packages behave as before. HubFiles, the daemon state and context TopHubs still list files. Closes #285 Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01FWQw8AjoCA1yjN3T59vk46 --- blast_radius.go | 17 ++++++-- cmd/hooks.go | 15 ++++--- find/find.go | 12 +++++- handoff/build.go | 29 +++++++------ handoff/build_more_test.go | 21 ++++++++++ handoff/render.go | 4 +- handoff/types.go | 11 +++++ importers_go_package_test.go | 54 +++++++++++++++++++++++- mcp/analysis_output.go | 2 +- mcp/main.go | 34 ++++++++------- scanner/filegraph.go | 81 ++++++++++++++++++++++++++++++++++++ scanner/gopackage_test.go | 53 +++++++++++++++++++++++ 12 files changed, 293 insertions(+), 40 deletions(-) diff --git a/blast_radius.go b/blast_radius.go index 29575957..a22e359e 100644 --- a/blast_radius.go +++ b/blast_radius.go @@ -1468,7 +1468,17 @@ func pluralFiles(n int, noun string) string { } func renderImportersReport(w io.Writer, report scanner.ImportersReport) { - if scanner.CountHubImporters(report.Importers) >= scanner.HubThreshold { + if report.Package != "" && report.PackageSiblings > 0 && len(report.Importers) > 0 { + fmt.Fprintf(w, "📍 File: %s\n", report.File) + fmt.Fprintf(w, " Package %s is imported by %s (Go imports name packages, not files)\n", report.Package, pluralFiles(len(report.Importers), "file")) + for i, imp := range report.Importers { + if i >= 5 { + fmt.Fprintf(w, " ... and %d more\n", len(report.Importers)-5) + break + } + fmt.Fprintf(w, " • %s\n", imp) + } + } else if scanner.CountHubImporters(report.Importers) >= scanner.HubThreshold { fmt.Fprintf(w, "⚠️ HUB FILE: %s\n", report.File) fmt.Fprintf(w, " Imported by %d files%s - changes have wide impact!\n", len(report.Importers), importersGranularity(report)) fmt.Fprintln(w) @@ -1494,7 +1504,8 @@ func renderImportersReport(w io.Writer, report scanner.ImportersReport) { } // Every non-test file of a hub Go package is a hub, so list the // package once with its file count rather than each file. - fmt.Fprintf(w, " Imports %d hub(s): %s\n", len(report.HubImports), strings.Join(scanner.CollapseGoPackageFiles(report.HubImports), ", ")) + hubs := scanner.CollapseGoPackageFiles(report.HubImports) + fmt.Fprintf(w, " Imports %d hub(s): %s\n", len(hubs), strings.Join(hubs, ", ")) } renderCoverage(w, report.CoverageStatus, report.CoverageNotes) @@ -1554,7 +1565,7 @@ func buildImportersReportFromGraph(root, file string, fg *scanner.FileGraph) (sc Importers: importers, Imports: imports, ImporterCount: len(importers), - IsHub: fg.IsHub(file), + IsHub: fg.IsFileHub(file), NotIndexed: !fg.Indexed(file), CoverageStatus: string(coverage.EffectiveStatus()), CoverageNotes: append([]string(nil), coverage.Notes...), diff --git a/cmd/hooks.go b/cmd/hooks.go index 0d9c181f..7a875dd5 100644 --- a/cmd/hooks.go +++ b/cmd/hooks.go @@ -478,13 +478,17 @@ func hookSessionStart(root string) error { info := getHubInfo(root) if info != nil && len(info.Hubs) > 0 { fmt.Println("⚠️ High-impact files (hubs):") - for i, hub := range info.Hubs { + entries := scanner.CollapseGoPackageHubs(info.Hubs, info.Importers) + for i, entry := range entries { if i >= maxHubs { - fmt.Printf(" ... and %d more\n", len(info.Hubs)-maxHubs) + fmt.Printf(" ... and %d more\n", len(entries)-maxHubs) break } - importers := len(info.Importers[hub]) - fmt.Printf(" ⚠️ HUB FILE: %s (imported by %s)\n", hub, filesText(importers)) + if entry.IsPackage() { + fmt.Printf(" ⚠️ HUB PACKAGE: %s (%d files, imported by %s)\n", entry.Path, len(entry.Files), filesText(entry.Importers)) + continue + } + fmt.Printf(" ⚠️ HUB FILE: %s (imported by %s)\n", entry.Path, filesText(entry.Importers)) } } else if shouldSkipHubAnalysis(fileCount, fileCountKnown) { fmt.Printf("ℹ️ Hub analysis skipped for large repo (%d files)\n", fileCount) @@ -2274,7 +2278,8 @@ func checkFileImportersWithPhase(root, filePath, phase string) error { } } if len(hubImports) > 0 { - fmt.Printf(" Imports %d hub(s): %s\n", len(hubImports), strings.Join(scanner.CollapseGoPackageFiles(hubImports), ", ")) + hubs := scanner.CollapseGoPackageFiles(hubImports) + fmt.Printf(" Imports %d hub(s): %s\n", len(hubs), strings.Join(hubs, ", ")) fmt.Println() } diff --git a/find/find.go b/find/find.go index 8f3316d3..d441c3ac 100644 --- a/find/find.go +++ b/find/find.go @@ -35,6 +35,9 @@ type Hit struct { Matched []string `json:"matched"` Importers int `json:"importers"` Hub bool `json:"hub"` + // Package is set when Importers counts the importers of the file's + // multi-file Go package rather than of the file itself. + Package string `json:"package,omitempty"` } type Report struct { @@ -58,7 +61,10 @@ func Run(ctx context.Context, root string, filters scanner.Filters, query string for i := range report.Hits { importers := fg.Importers[report.Hits[i].Path] report.Hits[i].Importers = len(importers) - report.Hits[i].Hub = scanner.CountHubImporters(importers) >= scanner.HubThreshold + report.Hits[i].Hub = fg.IsFileHub(report.Hits[i].Path) + if pkg, shared := fg.SharedGoPackage(report.Hits[i].Path); shared { + report.Hits[i].Package = pkg + } } } report.Coverage = scanner.CoverageFromSources(sources) @@ -138,7 +144,9 @@ func Render(w io.Writer, r Report) { fmt.Fprintf(w, " matched: %s\n", strings.Join(h.Matched, ", ")) } hub := "" - if h.Hub { + if h.Package != "" { + hub = " (of package " + h.Package + ")" + } else if h.Hub { hub = " (hub)" } fmt.Fprintf(w, " importers: %d%s\n", h.Importers, hub) diff --git a/handoff/build.go b/handoff/build.go index 30b77d9b..c81b8b7c 100644 --- a/handoff/build.go +++ b/handoff/build.go @@ -364,24 +364,29 @@ func summarizeHubs(importersByFile map[string][]string, maxHubs int) []HubSummar return []HubSummary{} } - hubs := make([]HubSummary, 0, len(importersByFile)) + paths := make([]string, 0, len(importersByFile)) for path, importers := range importersByFile { - if len(importers) < 3 { - continue + if len(importers) >= 3 { + paths = append(paths, path) } - hubs = append(hubs, HubSummary{ - Path: path, - Importers: len(importers), - }) } - - sort.Slice(hubs, func(i, j int) bool { - if hubs[i].Importers != hubs[j].Importers { - return hubs[i].Importers > hubs[j].Importers + sort.Slice(paths, func(i, j int) bool { + if len(importersByFile[paths[i]]) != len(importersByFile[paths[j]]) { + return len(importersByFile[paths[i]]) > len(importersByFile[paths[j]]) } - return hubs[i].Path < hubs[j].Path + return paths[i] < paths[j] }) + entries := scanner.CollapseGoPackageHubs(paths, importersByFile) + hubs := make([]HubSummary, 0, len(entries)) + for _, entry := range entries { + hub := HubSummary{Path: entry.Path, Importers: entry.Importers} + if entry.IsPackage() { + hub.Files = len(entry.Files) + } + hubs = append(hubs, hub) + } + if maxHubs > 0 && len(hubs) > maxHubs { hubs = hubs[:maxHubs] } diff --git a/handoff/build_more_test.go b/handoff/build_more_test.go index 6d4d6ad6..3e9750db 100644 --- a/handoff/build_more_test.go +++ b/handoff/build_more_test.go @@ -3,6 +3,7 @@ package handoff import ( "context" "reflect" + "strings" "testing" "time" @@ -137,6 +138,26 @@ func TestSummarizeHubsFiltersSortsAndCaps(t *testing.T) { } } +// Every file of a Go package carries the package's importers (#285), so the +// prefix lists the package once instead of one identical row per file. +func TestSummarizeHubsListsGoPackageOnce(t *testing.T) { + pkg := []string{"cmd/a.go", "cmd/b.go", "main.go"} + importers := map[string][]string{ + "scanner/filegraph.go": pkg, + "scanner/types.go": pkg, + "scanner/walker.go": pkg, + "web/app.ts": pkg, + } + got := summarizeHubs(importers, 0) + want := []HubSummary{{Path: "scanner/", Importers: 3, Files: 3}, {Path: "web/app.ts", Importers: 3}} + if !reflect.DeepEqual(got, want) { + t.Fatalf("summarizeHubs = %v, want %v", got, want) + } + if out := RenderPrefixMarkdown(PrefixSnapshot{Hubs: got}); !strings.Contains(out, "- `scanner/` (package, 3 files, 3 importers)\n") { + t.Fatalf("prefix markdown missing the package row:\n%s", out) + } +} + func TestGitCurrentBranchContextErrors(t *testing.T) { if _, err := gitCurrentBranchContext(context.Background(), t.TempDir()); err == nil { t.Fatal("expected error outside a git repository") diff --git a/handoff/render.go b/handoff/render.go index 1e6df99d..8054e705 100644 --- a/handoff/render.go +++ b/handoff/render.go @@ -30,7 +30,7 @@ func RenderMarkdown(a *Artifact) string { b.WriteString(fmt.Sprintf("- ... and %d more\n", len(a.Prefix.Hubs)-15)) break } - b.WriteString(fmt.Sprintf("- `%s` (%d importers)\n", hub.Path, hub.Importers)) + b.WriteString("- " + hub.String() + "\n") } } @@ -113,7 +113,7 @@ func RenderPrefixMarkdown(p PrefixSnapshot) string { } b.WriteString(fmt.Sprintf("- Hub files: %d\n", len(p.Hubs))) for _, hub := range p.Hubs { - b.WriteString(fmt.Sprintf("- `%s` (%d importers)\n", hub.Path, hub.Importers)) + b.WriteString("- " + hub.String() + "\n") } return b.String() } diff --git a/handoff/types.go b/handoff/types.go index 8daaee50..f44213ce 100644 --- a/handoff/types.go +++ b/handoff/types.go @@ -2,6 +2,7 @@ package handoff import ( "context" + "fmt" "time" "codemap/watch" @@ -17,6 +18,16 @@ const ( type HubSummary struct { Path string `json:"path"` Importers int `json:"importers"` + // Files is set when Path is a multi-file Go package directory listed once. + Files int `json:"files,omitempty"` +} + +// String is the markdown list form of the hub. +func (h HubSummary) String() string { + if h.Files > 0 { + return fmt.Sprintf("`%s` (package, %d files, %d importers)", h.Path, h.Files, h.Importers) + } + return fmt.Sprintf("`%s` (%d importers)", h.Path, h.Importers) } // FileStub is a lightweight file descriptor for lazy detail loading. diff --git a/importers_go_package_test.go b/importers_go_package_test.go index 7c007e06..88cf6b78 100644 --- a/importers_go_package_test.go +++ b/importers_go_package_test.go @@ -2,6 +2,7 @@ package main import ( "context" + "os" "path/filepath" "strings" "testing" @@ -40,7 +41,7 @@ func TestImportersCLIReportsGoPackageGranularity(t *testing.T) { if report.Package != "example.com/demo/multi" || report.PackageSiblings != 1 { t.Fatalf("report package = %q siblings = %d, want example.com/demo/multi with 1 sibling", report.Package, report.PackageSiblings) } - want := " Imported by 1 file(s) (package example.com/demo/multi: Go imports name packages, not files)\n • app/main.go\n" + want := " Package example.com/demo/multi is imported by 1 file (Go imports name packages, not files)\n • app/main.go\n" if !strings.Contains(out, want) { t.Fatalf("--importers multi/a.go output:\n%q\nwant it to contain:\n%q", out, want) } @@ -62,6 +63,57 @@ func TestImportersCLIReportsGoPackageGranularity(t *testing.T) { } } +// Three importers of a two-file package make the package a hub, not the file: +// --importers names the package as the thing imported and raises no HUB FILE +// warning, while a one-file package at the same count still does (#285). +func TestImportersCLIGoPackageHubIsNotAFileHub(t *testing.T) { + root := t.TempDir() + files := map[string]string{ + "go.mod": "module example.com/hub\n\ngo 1.21\n", + "multi/a.go": "package multi\n\nfunc A() {}\n", + "multi/b.go": "package multi\n\nfunc B() {}\n", + "single/single.go": "package single\n\nfunc S() {}\n", + } + for _, user := range []string{"u1", "u2", "u3"} { + files[user+"/"+user+".go"] = "package " + user + "\n\nimport (\n\t\"example.com/hub/multi\"\n\t\"example.com/hub/single\"\n)\n\nfunc F() { multi.A(); single.S() }\n" + } + for rel, body := range files { + path := filepath.Join(root, filepath.FromSlash(rel)) + if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(path, []byte(body), 0o644); err != nil { + t.Fatal(err) + } + } + fg, err := scanner.BuildFileGraph(context.Background(), root, scanner.Filters{}) + if err != nil { + t.Fatalf("BuildFileGraph: %v", err) + } + render := func(file string) (scanner.ImportersReport, string) { + t.Helper() + report, err := buildImportersReportFromGraph(root, file, fg) + if err != nil { + t.Fatalf("buildImportersReportFromGraph(%s): %v", file, err) + } + var buf strings.Builder + renderImportersReportCLI(&buf, report) + return report, buf.String() + } + + report, out := render("multi/a.go") + if report.IsHub || strings.Contains(out, "HUB") { + t.Fatalf("multi/a.go flagged as a hub file (is_hub=%v):\n%s", report.IsHub, out) + } + if !strings.Contains(out, "Package example.com/hub/multi is imported by 3 files") { + t.Fatalf("--importers multi/a.go must name the package as imported:\n%s", out) + } + report, out = render("single/single.go") + if !report.IsHub || !strings.Contains(out, "HUB FILE: single/single.go") { + t.Fatalf("one-file package lost its hub warning (is_hub=%v):\n%s", report.IsHub, out) + } +} + // End to end through the binary, as the issue reproduced it. func TestImportersCommandListsMultiFilePackageImporter(t *testing.T) { root, err := filepath.Abs("testdata/go-multi-file-package") diff --git a/mcp/analysis_output.go b/mcp/analysis_output.go index 2eef1e90..60aa6eef 100644 --- a/mcp/analysis_output.go +++ b/mcp/analysis_output.go @@ -101,7 +101,7 @@ func newImportersOutput(root, file string, graph *scanner.FileGraph) ImportersOu return ImportersOutput{ Kind: kind, Root: root, Mode: "importers", File: file, Importers: boundedImporters, Imports: boundedImports, HubImports: boundedHubs, - ImporterCount: len(importers), IsHub: graph.IsHub(file), + ImporterCount: len(importers), IsHub: graph.IsFileHub(file), CoverageStatus: string(coverage.EffectiveStatus()), CoverageNotes: nonNilCopy(coverage.Notes), Truncated: omittedCount > 0, OmittedCount: omittedCount, } diff --git a/mcp/main.go b/mcp/main.go index 452ce56a..ed4cdf09 100644 --- a/mcp/main.go +++ b/mcp/main.go @@ -527,12 +527,13 @@ func handleGetStructure(ctx context.Context, req *mcp.CallToolRequest, input Str } return hubs[i] < hubs[j] }) - for i, hub := range hubs { + entries := scanner.CollapseGoPackageHubs(hubs, importers) + for i, entry := range entries { if i >= 5 { - output += fmt.Sprintf(" ... and %d more hubs\n", len(hubs)-5) + output += fmt.Sprintf(" ... and %d more hubs\n", len(entries)-5) break } - output += fmt.Sprintf(" %s (%d importers)\n", hub, len(importers[hub])) + output += " " + entry.String() + "\n" } } @@ -973,13 +974,14 @@ func handleGetImporters(ctx context.Context, req *mcp.CallToolRequest, input Imp return textResult(empty + mcpCoverageText(fg.FileCoverage(file))), structured, nil } - isHub := scanner.CountHubImporters(importers) >= scanner.HubThreshold - hubNote := "" - if isHub { - hubNote = " ⚠️ HUB FILE" + header := fmt.Sprintf("%d files import '%s'%s:", len(importers), file, granularity) + if pkg, shared := fg.SharedGoPackage(file); shared { + header = fmt.Sprintf("Package %s (of '%s') is imported by %d file(s); Go imports name packages, not files:", pkg, file, len(importers)) + } else if scanner.CountHubImporters(importers) >= scanner.HubThreshold { + header += " ⚠️ HUB FILE" } - return textResult(fmt.Sprintf("%d files import '%s'%s:%s\n%s%s", len(importers), file, granularity, hubNote, strings.Join(importers, "\n"), mcpCoverageText(fg.FileCoverage(file)))), structured, nil + return textResult(header + "\n" + strings.Join(importers, "\n") + mcpCoverageText(fg.FileCoverage(file))), structured, nil } func handleFind(ctx context.Context, req *mcp.CallToolRequest, input FindQueryInput) (*mcp.CallToolResult, any, error) { @@ -1441,15 +1443,17 @@ func handleGetHubs(ctx context.Context, req *mcp.CallToolRequest, input PathInpu } return hubs[i] < hubs[j] }) + entries := scanner.CollapseGoPackageHubs(hubs, fg.Importers) var sb strings.Builder - sb.WriteString(fmt.Sprintf("=== Hub Files (%d total) ===\n", len(hubs))) - sb.WriteString("These files are imported by 3+ other files. Changes here have wide impact.\n\n") + sb.WriteString(fmt.Sprintf("=== Hub Files (%d total) ===\n", len(entries))) + sb.WriteString("These files are imported by 3+ other files. Changes here have wide impact.\n") + sb.WriteString("A Go package is listed once: Go imports name packages, not files.\n\n") - for _, hub := range hubs { - importers := append([]string(nil), fg.Importers[hub]...) + for _, entry := range entries { + importers := append([]string(nil), fg.Importers[entry.Files[0]]...) sort.Strings(importers) - sb.WriteString(fmt.Sprintf(" %s (%d importers)\n", hub, len(importers))) + sb.WriteString(" " + entry.String() + "\n") // Show first few importers for i, imp := range importers { if i >= 3 { @@ -1483,7 +1487,7 @@ func handleGetFileContext(ctx context.Context, req *mcp.CallToolRequest, input I file := normalizeImporterFile(absRoot, input.File) imports := append([]string(nil), fg.Imports[file]...) importers := append([]string(nil), fg.Importers[file]...) - isHub := fg.IsHub(file) + isHub := fg.IsFileHub(file) connected := fg.ConnectedFiles(file) sort.Strings(imports) sort.Strings(importers) @@ -1506,6 +1510,8 @@ func handleGetFileContext(ctx context.Context, req *mcp.CallToolRequest, input I if isHub { sb.WriteString(fmt.Sprintf("⚠️ HUB FILE - %d files depend on this\n", len(importers))) sb.WriteString(" Changes here affect many parts of the codebase.\n\n") + } else if pkg, shared := fg.SharedGoPackage(file); shared && fg.IsHub(file) { + sb.WriteString(fmt.Sprintf("📦 HUB PACKAGE %s - %d files import the package, not necessarily this file\n\n", pkg, len(importers))) } // What this file imports diff --git a/scanner/filegraph.go b/scanner/filegraph.go index 113dd1a6..9e12225e 100644 --- a/scanner/filegraph.go +++ b/scanner/filegraph.go @@ -1787,6 +1787,24 @@ func (fg *FileGraph) IsHub(path string) bool { return CountHubImporters(fg.Importers[path]) >= HubThreshold } +// IsFileHub is IsHub for a claim about path itself. A file of a multi-file +// Go package shares the package's importers, so the count says nothing about +// the file and it never qualifies; CollapseGoPackageHubs lists the package. +func (fg *FileGraph) IsFileHub(path string) bool { + if _, shared := fg.SharedGoPackage(path); shared { + return false + } + return fg.IsHub(path) +} + +// SharedGoPackage returns the import path of path's Go package when that +// package has two or more files, so path's importer count is the package's +// rather than its own. +func (fg *FileGraph) SharedGoPackage(path string) (string, bool) { + pkg, files, ok := fg.GoPackage(path) + return pkg, ok && len(files) > 1 +} + // GoPackage reports the Go package that path belongs to when this graph // resolved Go imports at package granularity: the package's import path and // its non-test files, which include path itself unless path is a _test.go @@ -1849,6 +1867,69 @@ func CollapseGoPackageFiles(paths []string) []string { return out } +// HubEntry is one row of a hub list: a file, or a multi-file Go package +// whose files share one importer set because Go imports name packages. +type HubEntry struct { + Path string // the file, or "dir/" for a package + Files []string // the hub files behind this row; more than one means a package + Importers int +} + +// IsPackage reports whether the entry stands for a multi-file Go package. +func (e HubEntry) IsPackage() bool { return len(e.Files) > 1 } + +// String is "path (N importers)", or "dir/ (package, F files, N importers)". +func (e HubEntry) String() string { + if e.IsPackage() { + return fmt.Sprintf("%s (package, %d files, %d importers)", e.Path, len(e.Files), e.Importers) + } + return fmt.Sprintf("%s (%d importers)", e.Path, e.Importers) +} + +// CollapseGoPackageHubs turns a ranked hub list into one entry per file, +// except that two or more .go files of one directory with the same importer +// set become a single package entry at the first one's position. The same set +// is the signature of a package-level edge, and needs no go.mod lookup, so +// this also works on importer maps read back from the daemon state. Every +// other path passes through unchanged. +func CollapseGoPackageHubs(hubs []string, importers map[string][]string) []HubEntry { + groupKey := func(path string) string { + if !strings.EqualFold(filepath.Ext(path), ".go") { + return "" + } + set := append([]string(nil), importers[path]...) + sort.Strings(set) + return pathpkg.Dir(filepath.ToSlash(path)) + "\x00" + strings.Join(set, "\x00") + } + groups := make(map[string][]string, len(hubs)) + for _, path := range hubs { + if key := groupKey(path); key != "" { + groups[key] = append(groups[key], path) + } + } + out := make([]HubEntry, 0, len(hubs)) + emitted := make(map[string]bool, len(groups)) + for _, path := range hubs { + key := groupKey(path) + files := groups[key] + if len(files) < 2 { + out = append(out, HubEntry{Path: path, Files: []string{path}, Importers: len(importers[path])}) + continue + } + if emitted[key] { + continue + } + emitted[key] = true + dir := pathpkg.Dir(filepath.ToSlash(path)) + entry := HubEntry{Path: dir + "/", Files: files, Importers: len(importers[path])} + if dir == "." { + entry.Path = "/" + } + out = append(out, entry) + } + return out +} + // HubFiles returns all files that qualify as hubs under IsHub, ordered by // non-test importer count descending and then by path. Map iteration order is // random, so callers that truncate or display the head of this list would diff --git a/scanner/gopackage_test.go b/scanner/gopackage_test.go index 1e80898a..504306a6 100644 --- a/scanner/gopackage_test.go +++ b/scanner/gopackage_test.go @@ -101,6 +101,59 @@ func TestCollapseGoPackageFiles(t *testing.T) { } } +// writeGoHubFixture writes a module where three packages import a two-file +// package hub/multi and a one-file package hub/single, so both are hubs. +func writeGoHubFixture(t *testing.T) string { + t.Helper() + root := t.TempDir() + files := map[string]string{ + "go.mod": "module example.com/hub\n\ngo 1.21\n", + "multi/a.go": "package multi\n\nfunc A() {}\n", + "multi/b.go": "package multi\n\nfunc B() {}\n", + "single/single.go": "package single\n\nfunc S() {}\n", + } + for _, user := range []string{"u1", "u2", "u3"} { + files[user+"/"+user+".go"] = "package " + user + "\n\nimport (\n\t\"example.com/hub/multi\"\n\t\"example.com/hub/single\"\n)\n\nfunc F() { multi.A(); single.S() }\n" + } + for rel, body := range files { + path := filepath.Join(root, filepath.FromSlash(rel)) + if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(path, []byte(body), 0o644); err != nil { + t.Fatal(err) + } + } + return root +} + +// A multi-file Go package is one hub, not one per file, and none of its files +// is a hub on its own: the count belongs to the package (#285). A one-file +// package is the file, so it keeps the file-level flag. +func TestGoPackageHubsListedOncePerPackage(t *testing.T) { + fg, err := BuildFileGraph(context.Background(), writeGoHubFixture(t), Filters{}) + if err != nil { + t.Fatalf("BuildFileGraph: %v", err) + } + got := CollapseGoPackageHubs(fg.HubFiles(), fg.Importers) + want := []HubEntry{ + {Path: "multi/", Files: []string{"multi/a.go", "multi/b.go"}, Importers: 3}, + {Path: "single/single.go", Files: []string{"single/single.go"}, Importers: 3}, + } + if !reflect.DeepEqual(got, want) { + t.Fatalf("CollapseGoPackageHubs(HubFiles) = %+v, want %+v", got, want) + } + if got[0].String() != "multi/ (package, 2 files, 3 importers)" { + t.Fatalf("package entry text = %q", got[0].String()) + } + if fg.IsFileHub("multi/a.go") || !fg.IsHub("multi/a.go") { + t.Fatalf("multi/a.go: IsFileHub=%v IsHub=%v, want false/true", fg.IsFileHub("multi/a.go"), fg.IsHub("multi/a.go")) + } + if !fg.IsFileHub("single/single.go") { + t.Fatalf("single/single.go lost its file-level hub flag") + } +} + // goListPackage is the subset of `go list -json` this oracle reads. type goListPackage struct { ImportPath string From d4a8006142efeaea0eed373ad800ec50a2d2ff95 Mon Sep 17 00:00:00 2001 From: Jordan Coin Jackson Date: Fri, 6 Nov 2026 10:28:26 -0500 Subject: [PATCH 2/2] fix(mcp): carry Go package granularity in structured results get_importers' structured result now says what importer_count counts: granularity "package" plus the package import path for a Go file in a module graph ("file" otherwise), and package_hub when a multi-file package is a hub although is_hub stays false for the file. get_structure's structured result gains hub_entries, the same package-collapsed list the text output renders; hubs stays the file list so existing consumers keep working. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01FWQw8AjoCA1yjN3T59vk46 --- mcp/analysis_output.go | 30 +++++++++++++++++++++++------- mcp/main.go | 16 ++++++++-------- mcp/structured_contract_test.go | 33 +++++++++++++++++++++++++++++++++ scanner/filegraph.go | 6 +++--- 4 files changed, 67 insertions(+), 18 deletions(-) diff --git a/mcp/analysis_output.go b/mcp/analysis_output.go index 60aa6eef..7f1dcfaa 100644 --- a/mcp/analysis_output.go +++ b/mcp/analysis_output.go @@ -10,11 +10,14 @@ import ( const maxStructuredItems = 500 type StructureOutput struct { - Kind string `json:"kind"` - Project scanner.Project `json:"project"` - Hubs []string `json:"hubs"` - Truncated bool `json:"truncated"` - OmittedCount int `json:"omitted_count"` + Kind string `json:"kind"` + Project scanner.Project `json:"project"` + Hubs []string `json:"hubs"` + // HubEntries is Hubs as the text output lists them: a multi-file Go + // package is one entry whose files share one importer set. + HubEntries []scanner.HubEntry `json:"hub_entries"` + Truncated bool `json:"truncated"` + OmittedCount int `json:"omitted_count"` } type DiffOutput struct { @@ -35,6 +38,12 @@ type ImportersOutput struct { HubImports []string `json:"hub_imports"` ImporterCount int `json:"importer_count"` IsHub bool `json:"is_hub"` + // Granularity is "package" when importer_count counts importers of the + // Go package (Package) that File belongs to, else "file". PackageHub is + // true when that package is a hub though File itself is not claimed one. + Granularity string `json:"granularity"` + Package string `json:"package,omitempty"` + PackageHub bool `json:"package_hub,omitempty"` // NotIndexed mirrors scanner.ImportersReport.NotIndexed: true when the // queried file was never part of the scanned set, so importer_count 0 is // a coverage gap, not a confirmed zero. NotIndexedReason names the one @@ -67,10 +76,11 @@ func boundedProject(project scanner.Project) (scanner.Project, int) { return bounded, omittedCount } -func newStructureOutput(project scanner.Project, hubs []string) StructureOutput { +func newStructureOutput(project scanner.Project, hubs []string, entries []scanner.HubEntry) StructureOutput { bounded, omittedCount := boundedProject(project) boundedHubs, omittedCount := addBounded(hubs, omittedCount) - return StructureOutput{Kind: "structure", Project: bounded, Hubs: boundedHubs, Truncated: omittedCount > 0, OmittedCount: omittedCount} + boundedEntries, omittedCount := addMirrorBounded(entries, len(hubs), omittedCount) + return StructureOutput{Kind: "structure", Project: bounded, Hubs: boundedHubs, HubEntries: boundedEntries, Truncated: omittedCount > 0, OmittedCount: omittedCount} } func newDiffOutput(kind, ref string, project scanner.Project) DiffOutput { @@ -98,10 +108,16 @@ func newImportersOutput(root, file string, graph *scanner.FileGraph) ImportersOu kind = "empty" } coverage := graph.FileCoverage(file) + granularity, pkg, packageHub := "file", "", false + if goPkg, pkgFiles, ok := graph.GoPackage(file); ok { + granularity, pkg = "package", goPkg + packageHub = len(pkgFiles) > 1 && graph.IsHub(file) + } return ImportersOutput{ Kind: kind, Root: root, Mode: "importers", File: file, Importers: boundedImporters, Imports: boundedImports, HubImports: boundedHubs, ImporterCount: len(importers), IsHub: graph.IsFileHub(file), + Granularity: granularity, Package: pkg, PackageHub: packageHub, CoverageStatus: string(coverage.EffectiveStatus()), CoverageNotes: nonNilCopy(coverage.Notes), Truncated: omittedCount > 0, OmittedCount: omittedCount, } diff --git a/mcp/main.go b/mcp/main.go index ed4cdf09..d98b6873 100644 --- a/mcp/main.go +++ b/mcp/main.go @@ -519,15 +519,15 @@ func handleGetStructure(ctx context.Context, req *mcp.CallToolRequest, input Str output += "\nℹ️ Hub analysis skipped for large repo (request get_hubs for explicit analysis)\n" } + sort.Slice(hubs, func(i, j int) bool { + if len(importers[hubs[i]]) != len(importers[hubs[j]]) { + return len(importers[hubs[i]]) > len(importers[hubs[j]]) + } + return hubs[i] < hubs[j] + }) + entries := scanner.CollapseGoPackageHubs(hubs, importers) if len(hubs) > 0 { output += "\n⚠️ HUB FILES (high-impact, 3+ dependents):\n" - sort.Slice(hubs, func(i, j int) bool { - if len(importers[hubs[i]]) != len(importers[hubs[j]]) { - return len(importers[hubs[i]]) > len(importers[hubs[j]]) - } - return hubs[i] < hubs[j] - }) - entries := scanner.CollapseGoPackageHubs(hubs, importers) for i, entry := range entries { if i >= 5 { output += fmt.Sprintf(" ... and %d more hubs\n", len(entries)-5) @@ -537,7 +537,7 @@ func handleGetStructure(ctx context.Context, req *mcp.CallToolRequest, input Str } } - return textResult(output), newStructureOutput(project, hubs), nil + return textResult(output), newStructureOutput(project, hubs, entries), nil } func handleGetDependencies(ctx context.Context, req *mcp.CallToolRequest, input PathInput) (*mcp.CallToolResult, any, error) { diff --git a/mcp/structured_contract_test.go b/mcp/structured_contract_test.go index 82b00289..2bcab974 100644 --- a/mcp/structured_contract_test.go +++ b/mcp/structured_contract_test.go @@ -294,3 +294,36 @@ func TestNewImportersOutputSortsDeterministically(t *testing.T) { t.Fatalf("hub_imports = %v, want %v", out.HubImports, wantHubs) } } + +// #292: the structured answers carry the package granularity the text states. +func TestStructuredOutputsCarryGoPackageGranularity(t *testing.T) { + importers := []string{"a.go", "b.go", "c.go"} + graph := &scanner.FileGraph{ + Module: "example.com/m", + Packages: map[string][]string{"example.com/m/pkg": {"pkg/x.go", "pkg/y.go"}, "example.com/m/solo": {"solo/z.go"}}, + Importers: map[string][]string{ + "pkg/x.go": importers, "pkg/y.go": importers, "solo/z.go": importers, + }, + Imports: map[string][]string{}, + } + out := newImportersOutput("/repo", "pkg/x.go", graph) + if out.Granularity != "package" || out.Package != "example.com/m/pkg" || out.IsHub || !out.PackageHub { + t.Fatalf("multi-file package: granularity=%q package=%q is_hub=%v package_hub=%v", out.Granularity, out.Package, out.IsHub, out.PackageHub) + } + out = newImportersOutput("/repo", "solo/z.go", graph) + if out.Granularity != "package" || !out.IsHub || out.PackageHub { + t.Fatalf("single-file package: granularity=%q is_hub=%v package_hub=%v", out.Granularity, out.IsHub, out.PackageHub) + } + if out := newImportersOutput("/repo", "web/app.ts", graph); out.Granularity != "file" || out.Package != "" { + t.Fatalf("non-Go file: granularity=%q package=%q, want file and none", out.Granularity, out.Package) + } + + hubs := []string{"pkg/x.go", "pkg/y.go", "solo/z.go"} + structure := newStructureOutput(scanner.Project{}, hubs, scanner.CollapseGoPackageHubs(hubs, graph.Importers)) + if len(structure.HubEntries) != 2 || structure.HubEntries[0].Path != "pkg/" || len(structure.HubEntries[0].Files) != 2 || structure.HubEntries[1].Path != "solo/z.go" { + t.Fatalf("hub_entries = %+v, want pkg/ (2 files) then solo/z.go", structure.HubEntries) + } + if !slices.Equal(structure.Hubs, hubs) { + t.Fatalf("hubs = %v, want the file list unchanged", structure.Hubs) + } +} diff --git a/scanner/filegraph.go b/scanner/filegraph.go index 9e12225e..191c2cd4 100644 --- a/scanner/filegraph.go +++ b/scanner/filegraph.go @@ -1870,9 +1870,9 @@ func CollapseGoPackageFiles(paths []string) []string { // HubEntry is one row of a hub list: a file, or a multi-file Go package // whose files share one importer set because Go imports name packages. type HubEntry struct { - Path string // the file, or "dir/" for a package - Files []string // the hub files behind this row; more than one means a package - Importers int + Path string `json:"path"` // the file, or "dir/" for a package + Files []string `json:"files"` // the hub files behind this row; more than one means a package + Importers int `json:"importer_count"` } // IsPackage reports whether the entry stands for a multi-file Go package.