From 4da3d8ead5ef7e5bc5037662796e6b49a14ab66a Mon Sep 17 00:00:00 2001 From: Jordan Coin Jackson Date: Fri, 4 Sep 2026 10:25:09 -0400 Subject: [PATCH 1/5] feat: codemap collide, rank open PRs by shared-file merge-order hazard CI structurally cannot see cross-PR collisions: every PR is built against main and never against its siblings. Issue #134 measured that blind spot by merging six worktree pairs by hand, and #117/#118 shipped a miscompile through it. `codemap collide` reads open PRs through `gh pr list --json files`, intersects their changed paths, and weights each shared file by the importer count from the graph on the current checkout. The intersection is glue; the weighting is the part that needs codemap, because only the graph knows that a collision on a 23-importer hub is a different severity from one on a test fixture. Honesty rules, per the design principle in #134 (a composite inherits the honesty of its primitives and states it with more authority): - Importer counts are stated as facts only while graph coverage is complete. Degraded coverage prints "unknown importers", drops the verdict to TRUST LOW, and says ranking fell back to shared-file count. - A "no collisions" answer from a degraded graph is TRUST LOW too: a negative finding from a partial graph is as unreliable as a positive one. - Coverage attribution is whole-graph, not per-language. Narrowing "partial" to a subset of languages by matching free-text notes would hand back confidence the graph never claimed. #174's ResolvesFileLevelImports is the supported seam for per-language attribution; collideImportersKnown is the single function it belongs in. - --min-importers never hides a file whose count is unknown, and never drops a hazard silently: the hidden count and the way to see them are printed. - A file the graph carries no edges for at all (a YAML rule, a fixture) reports "not in graph" rather than a zero that reads as "nothing imports it". Tests cover the pair/shared-file computation against issue #134's measured 4-PR matrix (6 of 6 pairs, including the 2-vs-3 distinction), the ranking order, degraded coverage yielding TRUST LOW with unknown counts, and a golden human output. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01TcyheQmM3HCvxF5wRL3s5t --- README.md | 1 + collide.go | 619 ++++++++++++++++++++++++++++++++++++++++++++++++ collide_test.go | 373 +++++++++++++++++++++++++++++ main.go | 10 + 4 files changed, 1003 insertions(+) create mode 100644 collide.go create mode 100644 collide_test.go diff --git a/README.md b/README.md index ec6a6cb..f887d0f 100644 --- a/README.md +++ b/README.md @@ -155,6 +155,7 @@ codemap --diff # what changed vs main codemap --deps . # dependency flow codemap --importers f # who imports a file codemap blast-radius # review bundle: diff + deps + importers +codemap collide # rank open PRs by shared-file merge-order hazard codemap handoff . # save layered handoff for cross-agent continuation codemap context # machine-readable project context JSON codemap doctor # validate agent integrations diff --git a/collide.go b/collide.go new file mode 100644 index 0000000..4260e3a --- /dev/null +++ b/collide.go @@ -0,0 +1,619 @@ +package main + +import ( + "context" + "encoding/json" + "errors" + "flag" + "fmt" + "io" + "os" + "os/exec" + "path/filepath" + "sort" + "strings" + + "codemap/analysis" + "codemap/cmd" + "codemap/config" + "codemap/scanner" +) + +// collideSchema versions the machine-readable output so a hosted consumer can +// detect a shape change instead of guessing. +const collideSchema = "codemap.collide/v1" + +const ( + collideTrustHigh = "high" + collideTrustLow = "low" +) + +// collideUnknownWeight is the ranking weight of a file whose importer count the +// graph cannot state. It sorts below a measured zero on purpose: a pair whose +// severity is unknown must never outrank one whose severity was measured. +const collideUnknownWeight = -1 + +// collidePRFile is one changed path from `gh pr list --json files`. +type collidePRFile struct { + Path string `json:"path"` +} + +// collidePR is the slice of a pull request this command reads. Field names +// match `gh pr list --json number,title,headRefName,files` exactly. +type collidePR struct { + Number int `json:"number"` + Title string `json:"title"` + HeadRefName string `json:"headRefName"` + Files []collidePRFile `json:"files"` +} + +// collideImporters is everything the graph knows about one shared file. +// +// Known and InGraph are different kinds of ignorance and are kept apart: a file +// the graph does not track (a YAML rule file, a fixture) has a truthful answer +// of "no import edges exist for this kind of file", while a file the graph does +// track under degraded coverage has no truthful answer at all. +type collideImporters struct { + Count int + Known bool + InGraph bool +} + +// collideLookup answers importer questions for a repository-relative path. +type collideLookup func(path string) collideImporters + +// collideSharedFile is one path changed by more than one open PR. +type collideSharedFile struct { + Path string `json:"path"` + PRs []int `json:"prs"` + Language string `json:"language,omitempty"` + ImporterCount int `json:"importer_count"` + ImportersKnown bool `json:"importers_known"` + InGraph bool `json:"in_graph"` +} + +// weight is the ranking value of this file's severity. +func (f collideSharedFile) weight() int { + if !f.ImportersKnown { + return collideUnknownWeight + } + return f.ImporterCount +} + +// collidePair is two open PRs that change at least one file in common. +type collidePair struct { + A int `json:"a"` + B int `json:"b"` + SharedFileCount int `json:"shared_file_count"` + SharedFiles []string `json:"shared_files"` + TopFile string `json:"top_file"` + TopImporterCount int `json:"top_importer_count"` + TopImportersKnown bool `json:"top_importers_known"` + TopFileInGraph bool `json:"top_file_in_graph"` +} + +// collideLanguageCoverage reports graph coverage for one language present in +// the shared set, which is the only slice of the repository this verdict rests +// on. +type collideLanguageCoverage struct { + Language string `json:"language"` + Status string `json:"status"` + Files int `json:"files"` +} + +// collideCoverage carries the graph provenance behind every importer count. +type collideCoverage struct { + Status string `json:"status"` + Notes []string `json:"notes,omitempty"` + Languages []collideLanguageCoverage `json:"languages"` + Untracked int `json:"untracked_files"` +} + +// collideReportPR labels a PR number in the output. +type collideReportPR struct { + Number int `json:"number"` + Title string `json:"title"` + Head string `json:"head_ref_name,omitempty"` +} + +// collideReport is the whole answer, human rendering and JSON alike. +type collideReport struct { + Schema string `json:"schema"` + Repo string `json:"repo,omitempty"` + MinImporters int `json:"min_importers"` + Trust string `json:"trust"` + Coverage collideCoverage `json:"coverage"` + PRs []collideReportPR `json:"prs"` + SharedFiles []collideSharedFile `json:"shared_files"` + Pairs []collidePair `json:"pairs"` + HiddenByMinImporters int `json:"hidden_by_min_importers"` +} + +func runCollideSubcommand(args []string, launchDir string) int { + fs := flag.NewFlagSet("collide", flag.ContinueOnError) + fs.SetOutput(os.Stderr) + repo := fs.String("repo", "", "Repository to read open PRs from (owner/name); defaults to the current checkout") + jsonMode := fs.Bool("json", false, "Emit a single JSON object") + minImporters := fs.Int("min-importers", 1, "Hide shared files with fewer importers than this") + limit := fs.Int("limit", 50, "Maximum open PRs to read") + var help bool + fs.BoolVar(&help, "help", false, "Show collide help") + fs.BoolVar(&help, "h", false, "Show collide help") + fs.Usage = func() { printCollideUsage(fs) } + + if err := fs.Parse(args); err != nil { + if errors.Is(err, flag.ErrHelp) { + return 0 + } + return 2 + } + if help { + fs.Usage() + return 0 + } + if fs.NArg() > 0 { + fmt.Fprintf(os.Stderr, "Error: unexpected argument %q; codemap collide reads the current checkout (use -C to change it)\n", fs.Arg(0)) + return 2 + } + if *minImporters < 0 { + fmt.Fprintln(os.Stderr, "Error: --min-importers cannot be negative") + return 2 + } + if *limit <= 0 { + fmt.Fprintln(os.Stderr, "Error: --limit must be greater than zero") + return 2 + } + + absRoot, err := filepath.Abs(launchDir) + if err != nil { + fmt.Fprintf(os.Stderr, "Error resolving project root: %v\n", err) + return 1 + } + resolvedRoot, _, err := cmd.ResolveNearestGitRoot(absRoot) + if err != nil { + fmt.Fprintf(os.Stderr, "Error resolving project root: %v\n", err) + return 1 + } + if _, err := cmd.ValidateProjectPath(resolvedRoot); err != nil { + fmt.Fprintf(os.Stderr, "Error resolving project root: %v\n", err) + return 1 + } + + prs, err := collideFetchPRs(*repo, *limit) + if err != nil { + fmt.Fprintf(os.Stderr, "Error: %v\n", err) + return 1 + } + + cfg := config.Load(resolvedRoot) + filters := scanner.Filters{Only: cfg.Only, Exclude: cfg.Exclude} + fg, err := scanner.BuildFileGraph(context.Background(), resolvedRoot, filters) + if err != nil { + fmt.Fprintf(os.Stderr, "Error building file graph: %v\n", err) + return 1 + } + + report := buildCollideReport(prs, collideGraphLookup(resolvedRoot, fg), fg.Coverage, *repo, *minImporters) + + if *jsonMode { + encoder := json.NewEncoder(os.Stdout) + encoder.SetIndent("", " ") + if err := encoder.Encode(report); err != nil { + fmt.Fprintf(os.Stderr, "Error encoding JSON: %v\n", err) + return 1 + } + return 0 + } + renderCollideReport(os.Stdout, report) + return 0 +} + +func printCollideUsage(fs *flag.FlagSet) { + fmt.Fprintln(os.Stderr, "Usage: codemap collide [options]") + fmt.Fprintln(os.Stderr) + fmt.Fprintln(os.Stderr, "Rank open pull requests by the merge-order hazard they share: which pairs") + fmt.Fprintln(os.Stderr, "change the same files, weighted by how many files import them. CI cannot see") + fmt.Fprintln(os.Stderr, "this, because every PR is built against main and never against its siblings.") + fmt.Fprintln(os.Stderr) + fmt.Fprintln(os.Stderr, "Options:") + fs.PrintDefaults() +} + +// collideImportersKnown decides, once for the whole run, whether importer +// counts from this graph may be stated as fact. +// +// The rule is deliberately whole-graph rather than per-language. Coverage notes +// are free text, so narrowing "this graph is partial" down to a subset of +// languages by matching those strings would hand back confidence the graph +// never claimed — the exact failure the composite is supposed to refuse. When +// per-language attribution becomes available from the scanner (PR #174 adds +// scanner.ResolvesFileLevelImports), it belongs in this function and nowhere +// else. +func collideImportersKnown(coverage scanner.GraphCoverage) bool { + return coverage.Status == "" || coverage.Status == analysis.CoverageComplete +} + +// collideCoverageStatus renders the graph's status, spelling out the zero value +// rather than emitting an empty string a reader cannot interpret. +func collideCoverageStatus(coverage scanner.GraphCoverage) string { + if coverage.Status == "" { + return string(analysis.CoverageComplete) + } + return string(coverage.Status) +} + +// collideGraphLookup answers importer questions from a built file graph, +// reusing the same helper that backs `codemap --importers` and blast-radius so +// all three report the same number for the same file. +func collideGraphLookup(root string, fg *scanner.FileGraph) collideLookup { + known := collideImportersKnown(fg.Coverage) + return func(path string) collideImporters { + if !scanner.IsSourceExt(filepath.Ext(path)) { + // Not a dishonest zero: the import graph has no edges for this kind + // of file at all, which is a different statement from "nothing + // imports it". + return collideImporters{Known: true, InGraph: false} + } + report, err := buildImportersReportFromGraph(root, path, fg) + if err != nil { + return collideImporters{Known: false, InGraph: true} + } + return collideImporters{Count: report.ImporterCount, Known: known, InGraph: true} + } +} + +// buildCollideReport is the whole computation, kept free of gh and the scanner +// so it can be tested against fixtures. +func buildCollideReport(prs []collidePR, lookup collideLookup, coverage scanner.GraphCoverage, repo string, minImporters int) collideReport { + shared, hidden := collideSharedFiles(prs, lookup, minImporters) + report := collideReport{ + Schema: collideSchema, + Repo: repo, + MinImporters: minImporters, + PRs: collideReportPRs(prs), + SharedFiles: shared, + Pairs: collidePairs(shared), + HiddenByMinImporters: hidden, + Coverage: collideCoverageFor(shared, coverage), + } + report.Trust = collideTrust(shared, coverage) + if report.SharedFiles == nil { + report.SharedFiles = []collideSharedFile{} + } + if report.Pairs == nil { + report.Pairs = []collidePair{} + } + return report +} + +func collideReportPRs(prs []collidePR) []collideReportPR { + out := make([]collideReportPR, 0, len(prs)) + for _, pr := range prs { + out = append(out, collideReportPR{Number: pr.Number, Title: pr.Title, Head: pr.HeadRefName}) + } + sort.Slice(out, func(i, j int) bool { return out[i].Number < out[j].Number }) + return out +} + +// collideSharedFiles groups every path changed by more than one open PR and +// attaches what the graph knows about it. +// +// A file whose importer count is unknown is never hidden by --min-importers: +// the threshold is a severity filter, and a file with no known severity has not +// been shown to be below it. +func collideSharedFiles(prs []collidePR, lookup collideLookup, minImporters int) (shared []collideSharedFile, hidden int) { + byPath := make(map[string]map[int]bool) + for _, pr := range prs { + for _, file := range pr.Files { + path := strings.TrimSpace(filepath.ToSlash(file.Path)) + if path == "" { + continue + } + if byPath[path] == nil { + byPath[path] = make(map[int]bool) + } + byPath[path][pr.Number] = true + } + } + + for path, numbers := range byPath { + if len(numbers) < 2 { + continue + } + prNumbers := make([]int, 0, len(numbers)) + for number := range numbers { + prNumbers = append(prNumbers, number) + } + sort.Ints(prNumbers) + + importers := lookup(path) + if importers.Known && importers.Count < minImporters { + hidden++ + continue + } + shared = append(shared, collideSharedFile{ + Path: path, + PRs: prNumbers, + Language: scanner.DetectLanguage(path), + ImporterCount: importers.Count, + ImportersKnown: importers.Known, + InGraph: importers.InGraph, + }) + } + + sort.Slice(shared, func(i, j int) bool { + left, right := shared[i], shared[j] + if len(left.PRs) != len(right.PRs) { + return len(left.PRs) > len(right.PRs) + } + if left.weight() != right.weight() { + return left.weight() > right.weight() + } + return left.Path < right.Path + }) + return shared, hidden +} + +// collidePairs ranks every pair of PRs that share a file by the worst importer +// count among the files they share, then by how many files they share. +// +// When no shared file has a known importer count every pair weighs the same, so +// the ranking degrades to the file count rather than inventing an order the +// graph cannot support. +func collidePairs(shared []collideSharedFile) []collidePair { + type pairKey struct{ a, b int } + pairs := make(map[pairKey]*collidePair) + + for _, file := range shared { + for i := 0; i < len(file.PRs); i++ { + for j := i + 1; j < len(file.PRs); j++ { + key := pairKey{a: file.PRs[i], b: file.PRs[j]} + pair, ok := pairs[key] + if !ok { + pair = &collidePair{A: key.a, B: key.b, TopImporterCount: collideUnknownWeight} + pairs[key] = pair + } + pair.SharedFiles = append(pair.SharedFiles, file.Path) + // shared is already ordered worst-first, so the first file to + // land on a pair is its top file. + if pair.TopFile == "" { + pair.TopFile = file.Path + pair.TopImporterCount = file.ImporterCount + pair.TopImportersKnown = file.ImportersKnown + pair.TopFileInGraph = file.InGraph + } + } + } + } + + out := make([]collidePair, 0, len(pairs)) + for _, pair := range pairs { + pair.SharedFileCount = len(pair.SharedFiles) + out = append(out, *pair) + } + + weight := func(pair collidePair) int { + if !pair.TopImportersKnown { + return collideUnknownWeight + } + return pair.TopImporterCount + } + sort.Slice(out, func(i, j int) bool { + left, right := out[i], out[j] + if weight(left) != weight(right) { + return weight(left) > weight(right) + } + if left.SharedFileCount != right.SharedFileCount { + return left.SharedFileCount > right.SharedFileCount + } + if left.A != right.A { + return left.A < right.A + } + return left.B < right.B + }) + return out +} + +// collideCoverageFor reports coverage for exactly the languages the verdict +// rests on: the ones present in the shared set. +func collideCoverageFor(shared []collideSharedFile, coverage scanner.GraphCoverage) collideCoverage { + status := collideCoverageStatus(coverage) + files := make(map[string]int) + degraded := make(map[string]bool) + untracked := 0 + + for _, file := range shared { + if !file.InGraph { + untracked++ + continue + } + language := file.Language + if language == "" { + language = "unknown" + } + files[language]++ + if !file.ImportersKnown { + degraded[language] = true + } + } + + languages := make([]collideLanguageCoverage, 0, len(files)) + for language, count := range files { + languageStatus := string(analysis.CoverageComplete) + if degraded[language] { + languageStatus = status + } + languages = append(languages, collideLanguageCoverage{Language: language, Status: languageStatus, Files: count}) + } + sort.Slice(languages, func(i, j int) bool { return languages[i].Language < languages[j].Language }) + + return collideCoverage{ + Status: status, + Notes: append([]string(nil), coverage.Notes...), + Languages: languages, + Untracked: untracked, + } +} + +// collideTrust refuses a confident verdict whenever an input reports degraded +// coverage. That includes the "no collisions" answer: a negative finding from a +// partial graph is exactly as unreliable as a positive one. +func collideTrust(shared []collideSharedFile, coverage scanner.GraphCoverage) string { + if !collideImportersKnown(coverage) { + return collideTrustLow + } + for _, file := range shared { + if !file.ImportersKnown { + return collideTrustLow + } + } + return collideTrustHigh +} + +// collideImportersText states an importer count the way it is actually known. +func collideImportersText(known, inGraph bool, count int) string { + if !known { + return "unknown importers" + } + if !inGraph { + return "not in graph" + } + if count == 1 { + return "1 importer" + } + return fmt.Sprintf("%d importers", count) +} + +func renderCollideReport(w io.Writer, report collideReport) { + numbers := make([]string, 0, len(report.PRs)) + for _, pr := range report.PRs { + numbers = append(numbers, fmt.Sprintf("#%d", pr.Number)) + } + if len(numbers) == 0 { + fmt.Fprintln(w, "OPEN PRs (0): none") + } else { + fmt.Fprintf(w, "OPEN PRs (%d): %s\n", len(numbers), strings.Join(numbers, ", ")) + } + fmt.Fprintln(w) + + if len(report.SharedFiles) == 0 { + // "None" and "none left after filtering" are different answers, and + // only one of them means the open PRs are independent. + if report.HiddenByMinImporters > 0 { + fmt.Fprintf(w, "No shared files at or above --min-importers %d.\n", report.MinImporters) + } else { + fmt.Fprintln(w, "No shared files between open PRs.") + } + } else { + width := 0 + for _, file := range report.SharedFiles { + if len(file.Path) > width { + width = len(file.Path) + } + } + fmt.Fprintln(w, "SHARED FILES (each = a merge-order hazard):") + for _, file := range report.SharedFiles { + owners := make([]string, 0, len(file.PRs)) + for _, number := range file.PRs { + owners = append(owners, fmt.Sprintf("#%d", number)) + } + fmt.Fprintf(w, " %d PRs %-*s %-17s <- %s\n", + len(file.PRs), width, file.Path, + collideImportersText(file.ImportersKnown, file.InGraph, file.ImporterCount), + strings.Join(owners, ", ")) + } + } + if report.HiddenByMinImporters > 0 { + fmt.Fprintf(w, " (%d shared file(s) hidden by --min-importers %d; rerun with --min-importers 0 to see them)\n", + report.HiddenByMinImporters, report.MinImporters) + } + + if len(report.Pairs) > 0 { + fmt.Fprintln(w) + fmt.Fprintln(w, "PREDICTED COLLIDING PAIRS:") + for _, pair := range report.Pairs { + fmt.Fprintf(w, " #%d + #%d -> %d shared file(s) top: %s (%s)\n", + pair.A, pair.B, pair.SharedFileCount, pair.TopFile, + collideImportersText(pair.TopImportersKnown, pair.TopFileInGraph, pair.TopImporterCount)) + } + } + + fmt.Fprintln(w) + fmt.Fprintln(w, renderCollideCoverageLine(report)) +} + +// renderCollideCoverageLine is the last line of the human output: what the +// graph knows about the languages in the shared set, and the trust that follows +// from it. +func renderCollideCoverageLine(report collideReport) string { + var parts []string + for _, language := range report.Coverage.Languages { + noun := "files" + if language.Files == 1 { + noun = "file" + } + parts = append(parts, fmt.Sprintf("%s %s (%d %s)", language.Language, language.Status, language.Files, noun)) + } + if report.Coverage.Untracked > 0 { + noun := "files" + if report.Coverage.Untracked == 1 { + noun = "file" + } + parts = append(parts, fmt.Sprintf("%d %s not tracked by the import graph", report.Coverage.Untracked, noun)) + } + if len(parts) == 0 { + parts = append(parts, fmt.Sprintf("graph %s, no shared files to weight", report.Coverage.Status)) + } + + line := fmt.Sprintf("Graph coverage: %s. TRUST %s.", strings.Join(parts, ", "), strings.ToUpper(report.Trust)) + if report.Trust == collideTrustLow && len(report.Coverage.Notes) > 0 { + line += "\n Why: " + strings.Join(report.Coverage.Notes, "; ") + } + if report.Trust == collideTrustLow { + line += "\n Ranking is by shared-file count only; importer weighting needs complete coverage." + } + return line +} + +// collideFetchPRs shells out to gh, the only network this command uses. +func collideFetchPRs(repo string, limit int) ([]collidePR, error) { + if _, err := exec.LookPath("gh"); err != nil { + return nil, errors.New("gh not found: install GitHub CLI from https://cli.github.com, then run: gh auth login") + } + + args := []string{"pr", "list", "--state", "open", "--json", "number,title,headRefName,files", "--limit", fmt.Sprintf("%d", limit)} + if strings.TrimSpace(repo) != "" { + args = append(args, "--repo", repo) + } + + command := exec.Command("gh", args...) + var stderr strings.Builder + command.Stderr = &stderr + stdout, err := command.Output() + if err != nil { + return nil, collideGHError(stderr.String(), err) + } + + var prs []collidePR + if err := json.Unmarshal(stdout, &prs); err != nil { + return nil, fmt.Errorf("parse gh pr list output: %w", err) + } + return prs, nil +} + +// collideGHError turns a gh failure into one actionable line. +func collideGHError(stderr string, err error) error { + message := strings.TrimSpace(stderr) + if index := strings.IndexByte(message, '\n'); index >= 0 { + message = strings.TrimSpace(message[:index]) + } + lowered := strings.ToLower(message) + switch { + case strings.Contains(lowered, "auth") || strings.Contains(lowered, "logged in") || strings.Contains(lowered, "token"): + return errors.New("gh is not authenticated. Run: gh auth login") + case message == "": + return fmt.Errorf("gh pr list failed: %w", err) + default: + return fmt.Errorf("gh pr list failed: %s", message) + } +} diff --git a/collide_test.go b/collide_test.go new file mode 100644 index 0000000..c3838d0 --- /dev/null +++ b/collide_test.go @@ -0,0 +1,373 @@ +package main + +import ( + "bytes" + "errors" + "fmt" + "strings" + "testing" + + "codemap/analysis" + "codemap/scanner" +) + +// collideFixturePRs reproduces the four-PR pileup from issue #134 (#124-#127), +// whose real collision matrix was measured by merging every pair in a worktree. +// Keeping the shape means a regression here is visible against ground truth. +func collideFixturePRs() []collidePR { + rustPRFiles := []collidePRFile{ + {Path: "scanner/astgrep.go"}, + {Path: "scanner/rustgraph.go"}, + {Path: "scanner/sg-rules/rust.yml"}, + } + return []collidePR{ + {Number: 124, Title: "rust graph edges", HeadRefName: "rust-edges", Files: []collidePRFile{ + {Path: "scanner/astgrep.go"}, + {Path: "scanner/rustgraph.go"}, + {Path: "scanner/rustcargo.go"}, + }}, + {Number: 125, Title: "rust mod resolution", HeadRefName: "rust-mods", Files: rustPRFiles}, + {Number: 126, Title: "rust workspace members", HeadRefName: "rust-workspace", Files: rustPRFiles}, + {Number: 127, Title: "rust re-export edges", HeadRefName: "rust-reexports", Files: rustPRFiles}, + } +} + +// collideFixtureLookup is a stand-in for the file graph: astgrep.go is a hub, +// rustgraph.go is ordinary, and the sg-rules YAML is not a file the import +// graph carries edges for. +func collideFixtureLookup(path string) collideImporters { + switch path { + case "scanner/astgrep.go": + return collideImporters{Count: 23, Known: true, InGraph: true} + case "scanner/rustgraph.go": + return collideImporters{Count: 4, Known: true, InGraph: true} + case "scanner/rustcargo.go": + return collideImporters{Count: 0, Known: true, InGraph: true} + case "scanner/sg-rules/rust.yml": + return collideImporters{Known: true, InGraph: false} + default: + return collideImporters{Known: true, InGraph: true} + } +} + +func collideCompleteCoverage() scanner.GraphCoverage { + return scanner.GraphCoverage{Status: analysis.CoverageComplete} +} + +func TestCollideSharedFilesGroupsEveryPRTouchingAPath(t *testing.T) { + shared, hidden := collideSharedFiles(collideFixturePRs(), collideFixtureLookup, 0) + if hidden != 0 { + t.Fatalf("hidden = %d, want 0 with --min-importers 0", hidden) + } + + type want struct { + path string + prs []int + } + wants := []want{ + {path: "scanner/astgrep.go", prs: []int{124, 125, 126, 127}}, + {path: "scanner/rustgraph.go", prs: []int{124, 125, 126, 127}}, + {path: "scanner/sg-rules/rust.yml", prs: []int{125, 126, 127}}, + } + if len(shared) != len(wants) { + t.Fatalf("shared files = %d (%+v), want %d", len(shared), shared, len(wants)) + } + for i, expected := range wants { + if shared[i].Path != expected.path { + t.Errorf("shared[%d].Path = %q, want %q", i, shared[i].Path, expected.path) + } + if len(shared[i].PRs) != len(expected.prs) { + t.Fatalf("shared[%d].PRs = %v, want %v", i, shared[i].PRs, expected.prs) + } + for j, number := range expected.prs { + if shared[i].PRs[j] != number { + t.Errorf("shared[%d].PRs = %v, want %v", i, shared[i].PRs, expected.prs) + break + } + } + } + + // rustcargo.go is changed by one PR only: a file nobody else touches is not + // a merge-order hazard, however many importers it has. + for _, file := range shared { + if file.Path == "scanner/rustcargo.go" { + t.Errorf("single-PR file %q reported as shared", file.Path) + } + } +} + +// The pair set and the 2-vs-3 distinction are the part issue #134 verified +// against six real worktree merges. +func TestCollidePairsMatchTheMeasuredMatrix(t *testing.T) { + shared, _ := collideSharedFiles(collideFixturePRs(), collideFixtureLookup, 0) + pairs := collidePairs(shared) + + got := make(map[string]int, len(pairs)) + for _, pair := range pairs { + got[pairLabel(pair.A, pair.B)] = pair.SharedFileCount + } + want := map[string]int{ + "#124+#125": 2, + "#124+#126": 2, + "#124+#127": 2, + "#125+#126": 3, + "#125+#127": 3, + "#126+#127": 3, + } + if len(got) != len(want) { + t.Fatalf("pairs = %v, want %v", got, want) + } + for label, count := range want { + if got[label] != count { + t.Errorf("pair %s shared file count = %d, want %d", label, got[label], count) + } + } +} + +// Ranking is importer count first, shared-file count second: a pair colliding +// on one hub outranks a pair colliding on three fixtures. +func TestCollidePairsRankByImporterCountThenSharedFiles(t *testing.T) { + prs := []collidePR{ + {Number: 1, Files: []collidePRFile{{Path: "hub.go"}}}, + {Number: 2, Files: []collidePRFile{{Path: "hub.go"}}}, + {Number: 3, Files: []collidePRFile{{Path: "a.go"}, {Path: "b.go"}, {Path: "c.go"}}}, + {Number: 4, Files: []collidePRFile{{Path: "a.go"}, {Path: "b.go"}, {Path: "c.go"}}}, + {Number: 5, Files: []collidePRFile{{Path: "a.go"}}}, + } + lookup := func(path string) collideImporters { + if path == "hub.go" { + return collideImporters{Count: 40, Known: true, InGraph: true} + } + return collideImporters{Count: 2, Known: true, InGraph: true} + } + + pairs := collidePairs(mustSharedFiles(t, prs, lookup, 0)) + if len(pairs) < 2 { + t.Fatalf("pairs = %+v, want at least 2", pairs) + } + if label := pairLabel(pairs[0].A, pairs[0].B); label != "#1+#2" { + t.Errorf("top pair = %s (top file %s, %d importers), want #1+#2 on the hub", + label, pairs[0].TopFile, pairs[0].TopImporterCount) + } + if pairs[0].TopFile != "hub.go" || pairs[0].TopImporterCount != 40 { + t.Errorf("top pair top file = %s (%d importers), want hub.go (40)", pairs[0].TopFile, pairs[0].TopImporterCount) + } + if label := pairLabel(pairs[1].A, pairs[1].B); label != "#3+#4" { + t.Errorf("second pair = %s, want #3+#4 (three shared files beats one)", label) + } +} + +// --min-importers is a severity filter, so it may only hide files whose +// severity is known to be below it. +func TestCollideMinImportersHidesLowCountsButNeverUnknownOnes(t *testing.T) { + prs := []collidePR{ + {Number: 1, Files: []collidePRFile{{Path: "quiet.go"}, {Path: "busy.go"}, {Path: "opaque.swift"}}}, + {Number: 2, Files: []collidePRFile{{Path: "quiet.go"}, {Path: "busy.go"}, {Path: "opaque.swift"}}}, + } + lookup := func(path string) collideImporters { + switch path { + case "busy.go": + return collideImporters{Count: 7, Known: true, InGraph: true} + case "quiet.go": + return collideImporters{Count: 0, Known: true, InGraph: true} + default: + return collideImporters{Known: false, InGraph: true} + } + } + + shared, hidden := collideSharedFiles(prs, lookup, 1) + if hidden != 1 { + t.Errorf("hidden = %d, want 1 (quiet.go is measured below the threshold)", hidden) + } + paths := make([]string, 0, len(shared)) + for _, file := range shared { + paths = append(paths, file.Path) + } + if strings.Join(paths, ",") != "busy.go,opaque.swift" { + t.Errorf("shared paths = %v, want busy.go and opaque.swift (an unknown count is not below a threshold)", paths) + } +} + +// Degraded inputs must not be laundered into a confident answer. +func TestCollideDegradedCoverageYieldsUnknownCountsAndTrustLow(t *testing.T) { + prs := []collidePR{ + {Number: 10, Files: []collidePRFile{{Path: "App/Model.swift"}}}, + {Number: 11, Files: []collidePRFile{{Path: "App/Model.swift"}}}, + } + coverage := scanner.GraphCoverage{ + Status: analysis.CoveragePartial, + Notes: []string{"Swift: imports name modules, not files"}, + } + lookup := func(string) collideImporters { + return collideImporters{Count: 0, Known: collideImportersKnown(coverage), InGraph: true} + } + + report := buildCollideReport(prs, lookup, coverage, "", 1) + if report.Trust != collideTrustLow { + t.Errorf("trust = %q, want %q", report.Trust, collideTrustLow) + } + if len(report.SharedFiles) != 1 { + t.Fatalf("shared files = %+v, want the Swift file kept despite --min-importers 1", report.SharedFiles) + } + if report.SharedFiles[0].ImportersKnown { + t.Error("shared file reports a known importer count from partial coverage") + } + if len(report.Pairs) != 1 || report.Pairs[0].TopImportersKnown { + t.Errorf("pairs = %+v, want one pair with an unknown top count", report.Pairs) + } + + var buf bytes.Buffer + renderCollideReport(&buf, report) + output := buf.String() + if !strings.Contains(output, "unknown importers") { + t.Errorf("human output states a count it cannot know:\n%s", output) + } + if !strings.Contains(output, "TRUST LOW") { + t.Errorf("human output missing TRUST LOW:\n%s", output) + } + if !strings.Contains(output, "Swift: imports name modules, not files") { + t.Errorf("human output does not say why trust is low:\n%s", output) + } + if strings.Contains(output, "0 importers") { + t.Errorf("human output printed a fabricated zero:\n%s", output) + } +} + +// A "no collisions" answer from a partial graph is exactly as unreliable as a +// positive one, so it carries the same verdict. +func TestCollideNoSharedFilesStillReportsCoverage(t *testing.T) { + prs := []collidePR{ + {Number: 1, Files: []collidePRFile{{Path: "a.go"}}}, + {Number: 2, Files: []collidePRFile{{Path: "b.go"}}}, + } + lookup := func(string) collideImporters { return collideImporters{Known: false, InGraph: true} } + + report := buildCollideReport(prs, lookup, scanner.GraphCoverage{Status: analysis.CoverageUnavailable}, "", 1) + if report.Trust != collideTrustLow { + t.Errorf("trust = %q, want %q for an empty answer from an unavailable graph", report.Trust, collideTrustLow) + } + var buf bytes.Buffer + renderCollideReport(&buf, report) + if !strings.Contains(buf.String(), "No shared files between open PRs.") { + t.Errorf("missing empty-result line:\n%s", buf.String()) + } + if !strings.Contains(buf.String(), "TRUST LOW") { + t.Errorf("empty result skipped its trust verdict:\n%s", buf.String()) + } +} + +// An empty graph status is complete knowledge, but a consumer cannot read an +// empty string, so it is spelled out. +func TestCollideEmptyCoverageStatusRendersComplete(t *testing.T) { + if got := collideCoverageStatus(scanner.GraphCoverage{}); got != string(analysis.CoverageComplete) { + t.Errorf("collideCoverageStatus(zero) = %q, want %q", got, analysis.CoverageComplete) + } + if !collideImportersKnown(scanner.GraphCoverage{}) { + t.Error("zero coverage treated as degraded; it means nothing was reported against the graph") + } + for _, status := range []analysis.CoverageStatus{analysis.CoveragePartial, analysis.CoverageUnavailable} { + if collideImportersKnown(scanner.GraphCoverage{Status: status}) { + t.Errorf("coverage %q treated as knowable", status) + } + } +} + +func TestCollideHumanOutputGolden(t *testing.T) { + report := buildCollideReport(collideFixturePRs(), collideFixtureLookup, collideCompleteCoverage(), "JordanCoin/codemap", 0) + + want := `OPEN PRs (4): #124, #125, #126, #127 + +SHARED FILES (each = a merge-order hazard): + 4 PRs scanner/astgrep.go 23 importers <- #124, #125, #126, #127 + 4 PRs scanner/rustgraph.go 4 importers <- #124, #125, #126, #127 + 3 PRs scanner/sg-rules/rust.yml not in graph <- #125, #126, #127 + +PREDICTED COLLIDING PAIRS: + #125 + #126 -> 3 shared file(s) top: scanner/astgrep.go (23 importers) + #125 + #127 -> 3 shared file(s) top: scanner/astgrep.go (23 importers) + #126 + #127 -> 3 shared file(s) top: scanner/astgrep.go (23 importers) + #124 + #125 -> 2 shared file(s) top: scanner/astgrep.go (23 importers) + #124 + #126 -> 2 shared file(s) top: scanner/astgrep.go (23 importers) + #124 + #127 -> 2 shared file(s) top: scanner/astgrep.go (23 importers) + +Graph coverage: go complete (2 files), 1 file not tracked by the import graph. TRUST HIGH. +` + + var buf bytes.Buffer + renderCollideReport(&buf, report) + if got := buf.String(); got != want { + t.Errorf("human output mismatch\n--- got ---\n%s\n--- want ---\n%s", got, want) + } +} + +func TestCollideHiddenFilesAreAnnouncedNotSilent(t *testing.T) { + prs := []collidePR{ + {Number: 1, Files: []collidePRFile{{Path: "quiet.go"}}}, + {Number: 2, Files: []collidePRFile{{Path: "quiet.go"}}}, + } + lookup := func(string) collideImporters { return collideImporters{Count: 0, Known: true, InGraph: true} } + + report := buildCollideReport(prs, lookup, collideCompleteCoverage(), "", 1) + if report.HiddenByMinImporters != 1 { + t.Fatalf("hidden = %d, want 1", report.HiddenByMinImporters) + } + var buf bytes.Buffer + renderCollideReport(&buf, report) + output := buf.String() + if !strings.Contains(output, "hidden by --min-importers 1") { + t.Errorf("a hazard was dropped without saying so:\n%s", output) + } + // "No shared files between open PRs" would be a false negative here: the + // PRs do share a file, it was filtered. + if strings.Contains(output, "No shared files between open PRs.") { + t.Errorf("filtered result claims the PRs share nothing:\n%s", output) + } + if !strings.Contains(output, "No shared files at or above --min-importers 1.") { + t.Errorf("filtered empty result does not say the threshold caused it:\n%s", output) + } +} + +func TestCollideGHErrorIsOneActionableLine(t *testing.T) { + cases := []struct { + name string + stderr string + want string + }{ + { + name: "unauthenticated", + stderr: "gh: To use GitHub CLI in a GitHub Actions workflow, set the GH_TOKEN environment variable.\nmore noise", + want: "gh is not authenticated. Run: gh auth login", + }, + { + name: "other failure", + stderr: "GraphQL: Could not resolve to a Repository\nsecond line", + want: "gh pr list failed: GraphQL: Could not resolve to a Repository", + }, + { + name: "silent failure", + stderr: "", + want: "gh pr list failed: exit status 1", + }, + } + for _, testCase := range cases { + t.Run(testCase.name, func(t *testing.T) { + got := collideGHError(testCase.stderr, errors.New("exit status 1")) + if got.Error() != testCase.want { + t.Errorf("collideGHError() = %q, want %q", got, testCase.want) + } + if strings.Contains(got.Error(), "\n") { + t.Errorf("error spans multiple lines: %q", got) + } + }) + } +} + +func mustSharedFiles(t *testing.T, prs []collidePR, lookup collideLookup, minImporters int) []collideSharedFile { + t.Helper() + shared, _ := collideSharedFiles(prs, lookup, minImporters) + return shared +} + +func pairLabel(a, b int) string { + return fmt.Sprintf("#%d+#%d", a, b) +} diff --git a/main.go b/main.go index e14ae1b..9f28f2a 100644 --- a/main.go +++ b/main.go @@ -211,6 +211,15 @@ func main() { return } + // Handle "collide" subcommand before global flag parsing + if len(os.Args) >= 2 && os.Args[1] == "collide" { + root, _ := os.Getwd() + if code := runCollideSubcommand(os.Args[2:], root); code != 0 { + os.Exit(code) + } + return + } + skylineMode := flag.Bool("skyline", false, "Enable skyline visualization mode") animateMode := flag.Bool("animate", false, "Enable animation (use with --skyline)") depsMode := flag.Bool("deps", false, "Enable dependency graph mode (function/import analysis)") @@ -293,6 +302,7 @@ func main() { fmt.Println(" codemap hook session-stop # Session summary") fmt.Println(" codemap handoff [path] # Build handoff artifact for agent switching") fmt.Println(" codemap blast-radius [path] # Compact bounded blast-radius bundle") + fmt.Println(" codemap collide # Rank open PRs by shared-file merge-order hazard") fmt.Println() fmt.Println("Project config:") fmt.Println(" codemap config init # Create .codemap/config.json (auto-detects extensions)") From a84f614638853745a029b0d2632a705f6fb4228a Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 4 Sep 2026 14:06:19 +0000 Subject: [PATCH 2/5] fix(scanner): Keep the Rust fallback when cargo metadata times out Ported verbatim from @reneleonhardt's open PR #171, which fixes this already. Carrying it here so this PR can go green rather than waiting on that one to merge; it becomes a no-op once main has it. buildRustWorkspaceIndex shadowed its caller's ctx with the cargo-metadata deadline, so once that deadline passed ctx.Err() returned DeadlineExceeded and the whole graph build failed with a bare "context deadline exceeded" instead of falling back to the manually derived Rust workspace. On a cold or loaded runner three seconds is not always enough for cargo metadata, and mcp/TestRustGraphContextHandlersDisclosePartialCoverage has now failed this way on three separate pull requests. Separating the metadata context from the caller's lets an expired deadline break out of the loop and keep the fallback index, which is what the test asserts and what a consumer needs: partial coverage disclosed, not a failed graph. Relates to #147, #172 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01PEUvjGsJemDFSV8nbxvxBo (cherry picked from commit 24af8fb155ea216b392cc699be06f8f177763eb2) --- scanner/rustcargo.go | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/scanner/rustcargo.go b/scanner/rustcargo.go index 9f08133..90db77d 100644 --- a/scanner/rustcargo.go +++ b/scanner/rustcargo.go @@ -100,6 +100,10 @@ func parseCargoMetadata(data []byte) (cargoMetadata, error) { } func buildRustWorkspaceIndex(ctx context.Context, root string, analyses []FileAnalysis, files []FileInfo, loader cargoMetadataLoader) (*rustWorkspaceIndex, *ScanSourceOutcome, error) { + return buildRustWorkspaceIndexWithTimeout(ctx, root, analyses, files, loader, cargoMetadataTimeout) +} + +func buildRustWorkspaceIndexWithTimeout(ctx context.Context, root string, analyses []FileAnalysis, files []FileInfo, loader cargoMetadataLoader, metadataTimeout time.Duration) (*rustWorkspaceIndex, *ScanSourceOutcome, error) { index := buildRustFallbackWorkspaceIndex(root, analyses) manifestPaths, err := discoverCargoManifests(ctx, root, files) if err != nil { @@ -115,7 +119,7 @@ func buildRustWorkspaceIndex(ctx context.Context, root string, analyses []FileAn outcome := cargoMetadataOutcome(0, len(manifestPaths)) return index, &outcome, nil } - ctx, cancel := context.WithTimeout(ctx, cargoMetadataTimeout) + metadataCtx, cancel := context.WithTimeout(ctx, metadataTimeout) defer cancel() packagesByRoot := make(map[string]rustPackage, len(index.packages)) @@ -129,11 +133,14 @@ func buildRustWorkspaceIndex(ctx context.Context, root string, analyses []FileAn if err := ctx.Err(); err != nil { return nil, nil, err } + if metadataCtx.Err() != nil { + break + } manifestPath = filepath.Clean(manifestPath) if handledManifests[manifestPath] { continue } - data, err := loader(ctx, manifestPath) + data, err := loader(metadataCtx, manifestPath) if err != nil { continue } From f4c29f4c2f2af88b40ed9f999eb8b31324cdf692 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 4 Sep 2026 14:03:02 +0000 Subject: [PATCH 3/5] fix(watch): Treat an unparseable readiness file as not-ready-yet MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit waitWatchReadiness returned on the first successful read, so a readiness file caught mid-write — existing but empty or partial — failed json.Unmarshal and aborted the wait immediately, reporting a startup failure for a daemon that had not finished writing. Only os.ErrNotExist counted as "not ready". Measured against the real function: an empty file returns "reading daemon readiness: unexpected end of JSON input" after 0s, without waiting out any part of the 30s timeout. Keep polling on a parse failure until the deadline, and surface the last parse error when the deadline passes, so a file that never becomes valid still says why rather than only that it timed out. publishWatchReadiness already renames its payload into place atomically, so codemap's own daemon does not open this window; the reader was brittle to any writer that is not atomic. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01PEUvjGsJemDFSV8nbxvxBo (cherry picked from commit 35d2af3063da80800674cf2a9da9ba4b76d9c0c6) --- main.go | 23 +++++++++----- watch_readiness_test.go | 66 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 82 insertions(+), 7 deletions(-) create mode 100644 watch_readiness_test.go diff --git a/main.go b/main.go index 9f28f2a..f784c5c 100644 --- a/main.go +++ b/main.go @@ -1163,22 +1163,31 @@ func publishWatchReadiness(path string, readinessErr error) error { func waitWatchReadiness(path string, timeout time.Duration) error { deadline := time.Now().Add(timeout) + // A readiness file that does not parse is a file still being written, not + // a daemon that failed: publishWatchReadiness renames its payload into + // place atomically, but nothing guarantees every writer does, and treating + // the first unparseable read as fatal reported a startup failure for a + // daemon that was starting fine. Keep the last parse error so a file that + // never becomes valid says why, rather than only that it timed out. + var lastParseErr error for { data, err := os.ReadFile(path) if err == nil { var status watchReadiness - if err := json.Unmarshal(data, &status); err != nil { - return fmt.Errorf("reading daemon readiness: %w", err) - } - if status.Error != "" { + if parseErr := json.Unmarshal(data, &status); parseErr != nil { + lastParseErr = parseErr + } else if status.Error != "" { return errors.New(status.Error) + } else { + return nil } - return nil - } - if !errors.Is(err, os.ErrNotExist) { + } else if !errors.Is(err, os.ErrNotExist) { return fmt.Errorf("reading daemon readiness: %w", err) } if time.Now().After(deadline) { + if lastParseErr != nil { + return fmt.Errorf("reading daemon readiness: %w", lastParseErr) + } return fmt.Errorf("daemon readiness timed out after %s", timeout) } time.Sleep(10 * time.Millisecond) diff --git a/watch_readiness_test.go b/watch_readiness_test.go new file mode 100644 index 0000000..e2a10cb --- /dev/null +++ b/watch_readiness_test.go @@ -0,0 +1,66 @@ +package main + +import ( + "os" + "path/filepath" + "strings" + "testing" + "time" +) + +// A readiness file caught mid-write reads as zero or partial bytes. Treating +// that as fatal reported a startup failure for a daemon that was starting +// fine, and made TestRunWatchStartWaitsForChildReadinessFailure fail whenever +// the machine was loaded enough to land in the window. +func TestWaitWatchReadinessWaitsThroughPartialWrite(t *testing.T) { + path := filepath.Join(t.TempDir(), "ready.json") + if err := os.WriteFile(path, nil, 0o644); err != nil { + t.Fatal(err) + } + go func() { + time.Sleep(50 * time.Millisecond) + _ = os.WriteFile(path, []byte(`{"error":"claim rejected"}`), 0o644) + }() + + err := waitWatchReadiness(path, 5*time.Second) + if err == nil || !strings.Contains(err.Error(), "claim rejected") { + t.Fatalf("waitWatchReadiness() = %v, want the daemon's own error once the file is complete", err) + } +} + +func TestWaitWatchReadinessSucceedsAfterPartialWrite(t *testing.T) { + path := filepath.Join(t.TempDir(), "ready.json") + if err := os.WriteFile(path, []byte(`{"err`), 0o644); err != nil { + t.Fatal(err) + } + go func() { + time.Sleep(50 * time.Millisecond) + _ = os.WriteFile(path, []byte(`{}`), 0o644) + }() + + if err := waitWatchReadiness(path, 5*time.Second); err != nil { + t.Fatalf("waitWatchReadiness() = %v, want success once the file is complete", err) + } +} + +// A file that never becomes valid still has to say why, rather than reporting +// only that it timed out. +func TestWaitWatchReadinessReportsPersistentGarbage(t *testing.T) { + path := filepath.Join(t.TempDir(), "ready.json") + if err := os.WriteFile(path, []byte("not json at all"), 0o644); err != nil { + t.Fatal(err) + } + err := waitWatchReadiness(path, 200*time.Millisecond) + if err == nil || !strings.Contains(err.Error(), "reading daemon readiness") { + t.Fatalf("waitWatchReadiness() = %v, want the parse failure surfaced", err) + } +} + +// A missing file must still time out rather than hang. +func TestWaitWatchReadinessTimesOutWhenAbsent(t *testing.T) { + path := filepath.Join(t.TempDir(), "never-written.json") + err := waitWatchReadiness(path, 100*time.Millisecond) + if err == nil || !strings.Contains(err.Error(), "timed out") { + t.Fatalf("waitWatchReadiness() = %v, want a timeout", err) + } +} From 1e3ef7e7ce38dea9b5f6ab4e7cebdc464e8d499d Mon Sep 17 00:00:00 2001 From: Jordan Coin Jackson Date: Fri, 4 Sep 2026 10:43:06 -0400 Subject: [PATCH 4/5] fix(collide): Weight Go collisions at the granularity Go actually resolves MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `--min-importers 1` hid every hazard on this repository, and the reason was not the threshold. Go resolves imports at package level, and BuildFileGraph deliberately drops an import that resolves to more than one file rather than fanning it into an edge per file, so a file inside a multi-file Go package has zero file-level importers by construction. `scanner/filegraph.go` scored 0 and read as harmless. So did every same-package collision, including all six pairs issue #134 verified by hand. A shared file whose language resolves at package granularity is now weighted as the files outside its package that import the package, plus the package's other files, and the count is labelled `package importers` so it is not read as a file-level number. Languages whose imports name files keep the file-level count and the plain label. FileGraph.Packages is populated for Go and nothing else, which is exactly the set this is correct for. The cross-package term cannot come from the graph — the edges are the ones that were dropped — so collide now keeps the scan outcome it was already paying for and counts the raw import strings. ScanForDeps plus BuildFileGraphFromOutcome is the same single scan BuildFileGraph was doing. Real effect on this repository, where the default previously printed nothing: 6 PRs scanner/filegraph.go 73 package importers <- #171, #174, #175, ... 3 PRs config/config.go 38 package importers <- #171, #181, #182 3 PRs main.go 6 package importers <- #175, #179, #180 --min-importers now defaults to 0. A file two open PRs both change is a hazard whatever its weight, and a default that hides hazards answers "no collisions" on a repository full of them. The flag stays for narrowing a long list. Three tests added: a Go fixture where two same-package files collide and carry a non-zero package weight (with the self-import and third-party cases held out of the count), a file-resolved language keeping file scope and its plain label, and #134's six measured pairs proved unchanged by the weighting — reordering them is allowed, adding or dropping one is not. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01TcyheQmM3HCvxF5wRL3s5t --- collide.go | 209 ++++++++++++++++++++++++++++++++++++++++++++---- collide_test.go | 184 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 377 insertions(+), 16 deletions(-) diff --git a/collide.go b/collide.go index 4260e3a..fe79e4f 100644 --- a/collide.go +++ b/collide.go @@ -33,6 +33,31 @@ const ( // severity is unknown must never outrank one whose severity was measured. const collideUnknownWeight = -1 +// collideDefaultMinImporters hides nothing. Every file two open PRs both change +// is a merge-order hazard whatever its importer count, so a threshold that +// filters by default answers "no collisions" on a repository that is full of +// them — which is exactly what a default of 1 did here, where Go's +// package-level resolution puts most shared files at a file-level 0. The flag +// remains, for narrowing a long list to the worst of it. +const collideDefaultMinImporters = 0 + +// The two scopes an importer count can be measured at. They are reported +// separately because they are not the same measurement and must not be read as +// if they were. +const ( + // collideScopeFile counts the files that import this exact file. It is the + // right answer for every language whose imports name a file: TypeScript, + // JavaScript, Python. + collideScopeFile = "file" + // collideScopePackage counts what a change to this file can reach when the + // language resolves imports at package granularity. Go is the case that + // matters here: nothing imports `scanner/astgrep.go`, things import + // `codemap/scanner`, so a file-level count of a Go file inside a multi-file + // package is structurally always 0 and reads as "harmless" for the busiest + // files in the repository. + collideScopePackage = "package" +) + // collidePRFile is one changed path from `gh pr list --json files`. type collidePRFile struct { Path string `json:"path"` @@ -57,6 +82,18 @@ type collideImporters struct { Count int Known bool InGraph bool + // Scope says which question Count answers. The empty value means + // collideScopeFile so a caller that never had a choice reads correctly. + Scope string +} + +// scope normalises the zero value rather than emitting an empty string a +// consumer cannot interpret. +func (i collideImporters) scope() string { + if i.Scope == collideScopePackage { + return collideScopePackage + } + return collideScopeFile } // collideLookup answers importer questions for a repository-relative path. @@ -68,6 +105,7 @@ type collideSharedFile struct { PRs []int `json:"prs"` Language string `json:"language,omitempty"` ImporterCount int `json:"importer_count"` + ImporterScope string `json:"importer_scope"` ImportersKnown bool `json:"importers_known"` InGraph bool `json:"in_graph"` } @@ -88,6 +126,7 @@ type collidePair struct { SharedFiles []string `json:"shared_files"` TopFile string `json:"top_file"` TopImporterCount int `json:"top_importer_count"` + TopImporterScope string `json:"top_importer_scope"` TopImportersKnown bool `json:"top_importers_known"` TopFileInGraph bool `json:"top_file_in_graph"` } @@ -134,7 +173,7 @@ func runCollideSubcommand(args []string, launchDir string) int { fs.SetOutput(os.Stderr) repo := fs.String("repo", "", "Repository to read open PRs from (owner/name); defaults to the current checkout") jsonMode := fs.Bool("json", false, "Emit a single JSON object") - minImporters := fs.Int("min-importers", 1, "Hide shared files with fewer importers than this") + minImporters := fs.Int("min-importers", collideDefaultMinImporters, "Hide shared files with fewer importers than this (0 shows every hazard)") limit := fs.Int("limit", 50, "Maximum open PRs to read") var help bool fs.BoolVar(&help, "help", false, "Show collide help") @@ -187,13 +226,22 @@ func runCollideSubcommand(args []string, launchDir string) int { cfg := config.Load(resolvedRoot) filters := scanner.Filters{Only: cfg.Only, Exclude: cfg.Exclude} - fg, err := scanner.BuildFileGraph(context.Background(), resolvedRoot, filters) + // The scan outcome is kept rather than discarded: a package-resolved + // language needs the raw import strings, which the file graph does not + // preserve. BuildFileGraphFromOutcome reuses this same scan, so keeping it + // costs nothing — this is one scan either way. + outcome, err := scanner.ScanForDeps(context.Background(), resolvedRoot, filters) + if err != nil { + fmt.Fprintf(os.Stderr, "Error scanning project: %v\n", err) + return 1 + } + fg, err := scanner.BuildFileGraphFromOutcome(context.Background(), resolvedRoot, outcome, filters) if err != nil { fmt.Fprintf(os.Stderr, "Error building file graph: %v\n", err) return 1 } - report := buildCollideReport(prs, collideGraphLookup(resolvedRoot, fg), fg.Coverage, *repo, *minImporters) + report := buildCollideReport(prs, collideGraphLookup(resolvedRoot, fg, outcome.Analyses), fg.Coverage, *repo, *minImporters) if *jsonMode { encoder := json.NewEncoder(os.Stdout) @@ -215,6 +263,12 @@ func printCollideUsage(fs *flag.FlagSet) { fmt.Fprintln(os.Stderr, "change the same files, weighted by how many files import them. CI cannot see") fmt.Fprintln(os.Stderr, "this, because every PR is built against main and never against its siblings.") fmt.Fprintln(os.Stderr) + fmt.Fprintln(os.Stderr, "Weights are labelled with the granularity they were measured at. A language") + fmt.Fprintln(os.Stderr, "whose imports name files (TypeScript, JavaScript, Python) reports importers of") + fmt.Fprintln(os.Stderr, "that file. Go imports name packages, so a Go file reports package importers:") + fmt.Fprintln(os.Stderr, "the files outside its package that import the package, plus its siblings") + fmt.Fprintln(os.Stderr, "inside it, because two PRs editing one package are editing one unit.") + fmt.Fprintln(os.Stderr) fmt.Fprintln(os.Stderr, "Options:") fs.PrintDefaults() } @@ -245,23 +299,128 @@ func collideCoverageStatus(coverage scanner.GraphCoverage) string { // collideGraphLookup answers importer questions from a built file graph, // reusing the same helper that backs `codemap --importers` and blast-radius so // all three report the same number for the same file. -func collideGraphLookup(root string, fg *scanner.FileGraph) collideLookup { +func collideGraphLookup(root string, fg *scanner.FileGraph, analyses []scanner.FileAnalysis) collideLookup { known := collideImportersKnown(fg.Coverage) + packageImporters := collidePackageImporterCounts(fg, analyses) return func(path string) collideImporters { if !scanner.IsSourceExt(filepath.Ext(path)) { // Not a dishonest zero: the import graph has no edges for this kind // of file at all, which is a different statement from "nothing // imports it". - return collideImporters{Known: true, InGraph: false} + return collideImporters{Known: true, InGraph: false, Scope: collideScopeFile} + } + if pkg, ok := collidePackageOf(fg, path); ok { + return collideImporters{ + Count: collidePackageWeight(packageImporters, pkg, len(fg.Packages[pkg])), + Known: known, + InGraph: true, + Scope: collideScopePackage, + } } report, err := buildImportersReportFromGraph(root, path, fg) if err != nil { - return collideImporters{Known: false, InGraph: true} + return collideImporters{Known: false, InGraph: true, Scope: collideScopeFile} } - return collideImporters{Count: report.ImporterCount, Known: known, InGraph: true} + return collideImporters{Count: report.ImporterCount, Known: known, InGraph: true, Scope: collideScopeFile} } } +// collidePackageOf names the package a file belongs to, when the graph resolves +// that file's language at package granularity. +// +// FileGraph.Packages is populated for Go and nothing else, which is exactly the +// set of languages this treatment is correct for — a language whose imports +// name files needs no package hop, and inventing one would inflate its counts. +// Test files are not members of the index (buildFileIndex skips them) but do +// live in the package, so they are matched by directory and counted as +// siblings of it rather than falling back to a file-level 0. +func collidePackageOf(fg *scanner.FileGraph, path string) (string, bool) { + if fg == nil || fg.Module == "" || len(fg.Packages) == 0 { + return "", false + } + if !strings.EqualFold(filepath.Ext(path), ".go") { + return "", false + } + pkg := fg.Module + if dir := filepath.ToSlash(filepath.Dir(filepath.FromSlash(path))); dir != "" && dir != "." { + pkg += "/" + dir + } + if _, ok := fg.Packages[pkg]; !ok { + return "", false + } + return pkg, true +} + +// collidePackageImporterCounts counts, per package, the files that import it. +// +// This cannot be read off FileGraph.Importers. BuildFileGraph deliberately +// drops a Go import that resolves to more than one file rather than fanning one +// import into an edge per file (it would inflate every hub count in the +// repository), so a multi-file package has no file-level edges at all and the +// graph's own answer for "who imports scanner/filegraph.go" is a structural +// zero. The raw import strings do carry it, which is why the scan outcome is +// kept. +// +// A file importing its own package is not counted: a package is not a second +// party to a collision inside itself. Its siblings are counted separately. +func collidePackageImporterCounts(fg *scanner.FileGraph, analyses []scanner.FileAnalysis) map[string]int { + if fg == nil || fg.Module == "" || len(fg.Packages) == 0 { + return nil + } + importers := make(map[string]map[string]bool) + for _, a := range analyses { + if !strings.EqualFold(filepath.Ext(a.Path), ".go") { + continue + } + ownPackage, hasOwn := collidePackageOf(fg, a.Path) + for _, imported := range a.Imports { + // Only a package this module actually contains is in fg.Packages, + // so this lookup also rejects every third-party import. + pkg := strings.Trim(imported, "\"'`") + if _, tracked := fg.Packages[pkg]; !tracked { + continue + } + if hasOwn && pkg == ownPackage { + continue + } + if importers[pkg] == nil { + importers[pkg] = make(map[string]bool) + } + importers[pkg][filepath.ToSlash(a.Path)] = true + } + } + + counts := make(map[string]int, len(importers)) + for pkg, files := range importers { + counts[pkg] = len(files) + } + return counts +} + +// collidePackageWeight is the severity of a collision on a file inside a +// package-resolved package: how many files a change to it can reach. +// +// It is the sum of two counts: +// +// - the files outside the package that import the package, which is the +// cross-package blast radius a file-level count structurally cannot see; and +// - the file's siblings in the same package, which is the in-package hazard +// — two PRs editing two files of one package are editing one compilation +// unit, and that is the collision `codemap collide` exists to name. +// +// The sibling term is the package's size minus one for every file in it, test +// files included. Test files are absent from the package index, so counting +// only actual members would score a _test.go file one higher than the +// implementation file beside it — an ordering artefact of the index, not a real +// difference in hazard. +func collidePackageWeight(packageImporters map[string]int, pkg string, packageSize int) int { + siblings := packageSize - 1 + if siblings < 0 { + siblings = 0 + } + return packageImporters[pkg] + siblings +} + // buildCollideReport is the whole computation, kept free of gh and the scanner // so it can be tested against fixtures. func buildCollideReport(prs []collidePR, lookup collideLookup, coverage scanner.GraphCoverage, repo string, minImporters int) collideReport { @@ -336,6 +495,7 @@ func collideSharedFiles(prs []collidePR, lookup collideLookup, minImporters int) PRs: prNumbers, Language: scanner.DetectLanguage(path), ImporterCount: importers.Count, + ImporterScope: importers.scope(), ImportersKnown: importers.Known, InGraph: importers.InGraph, }) @@ -379,6 +539,7 @@ func collidePairs(shared []collideSharedFile) []collidePair { if pair.TopFile == "" { pair.TopFile = file.Path pair.TopImporterCount = file.ImporterCount + pair.TopImporterScope = file.ImporterScope pair.TopImportersKnown = file.ImportersKnown pair.TopFileInGraph = file.InGraph } @@ -470,18 +631,25 @@ func collideTrust(shared []collideSharedFile, coverage scanner.GraphCoverage) st return collideTrustHigh } -// collideImportersText states an importer count the way it is actually known. -func collideImportersText(known, inGraph bool, count int) string { +// collideImportersText states an importer count the way it is actually known, +// including at which granularity it was measured. "23 package importers" and +// "23 importers" are different claims and a reader must be able to tell them +// apart without knowing which languages resolve how. +func collideImportersText(known, inGraph bool, count int, scope string) string { if !known { return "unknown importers" } if !inGraph { return "not in graph" } + noun := "importers" if count == 1 { - return "1 importer" + noun = "importer" } - return fmt.Sprintf("%d importers", count) + if scope == collideScopePackage { + noun = "package " + noun + } + return fmt.Sprintf("%d %s", count, noun) } func renderCollideReport(w io.Writer, report collideReport) { @@ -506,20 +674,29 @@ func renderCollideReport(w io.Writer, report collideReport) { } } else { width := 0 - for _, file := range report.SharedFiles { + // A package-scoped label is longer than a file-scoped one, so the + // weight column is measured rather than assumed; a hard-coded width + // silently ragged the output the moment a count grew. + weightWidth := 17 + weights := make([]string, len(report.SharedFiles)) + for i, file := range report.SharedFiles { if len(file.Path) > width { width = len(file.Path) } + weights[i] = collideImportersText(file.ImportersKnown, file.InGraph, file.ImporterCount, file.ImporterScope) + if len(weights[i]) > weightWidth { + weightWidth = len(weights[i]) + } } fmt.Fprintln(w, "SHARED FILES (each = a merge-order hazard):") - for _, file := range report.SharedFiles { + for i, file := range report.SharedFiles { owners := make([]string, 0, len(file.PRs)) for _, number := range file.PRs { owners = append(owners, fmt.Sprintf("#%d", number)) } - fmt.Fprintf(w, " %d PRs %-*s %-17s <- %s\n", + fmt.Fprintf(w, " %d PRs %-*s %-*s <- %s\n", len(file.PRs), width, file.Path, - collideImportersText(file.ImportersKnown, file.InGraph, file.ImporterCount), + weightWidth, weights[i], strings.Join(owners, ", ")) } } @@ -534,7 +711,7 @@ func renderCollideReport(w io.Writer, report collideReport) { for _, pair := range report.Pairs { fmt.Fprintf(w, " #%d + #%d -> %d shared file(s) top: %s (%s)\n", pair.A, pair.B, pair.SharedFileCount, pair.TopFile, - collideImportersText(pair.TopImportersKnown, pair.TopFileInGraph, pair.TopImporterCount)) + collideImportersText(pair.TopImportersKnown, pair.TopFileInGraph, pair.TopImporterCount, pair.TopImporterScope)) } } diff --git a/collide_test.go b/collide_test.go index c3838d0..81f57b3 100644 --- a/collide_test.go +++ b/collide_test.go @@ -272,6 +272,173 @@ func TestCollideEmptyCoverageStatusRendersComplete(t *testing.T) { } } +// The default hides nothing. A merge-order hazard the tool measured at zero +// file-level importers is still a hazard, and the previous default of 1 hid +// every one of them on this repository. +func TestCollideDefaultMinImportersHidesNothing(t *testing.T) { + if got := collideDefaultMinImporters; got != 0 { + t.Fatalf("collideDefaultMinImporters = %d, want 0", got) + } + prs := []collidePR{ + {Number: 1, Files: []collidePRFile{{Path: "quiet.go"}}}, + {Number: 2, Files: []collidePRFile{{Path: "quiet.go"}}}, + } + lookup := func(string) collideImporters { return collideImporters{Count: 0, Known: true, InGraph: true} } + + report := buildCollideReport(prs, lookup, collideCompleteCoverage(), "", collideDefaultMinImporters) + if report.HiddenByMinImporters != 0 || len(report.SharedFiles) != 1 { + t.Fatalf("default run hid %d file(s) and kept %d, want 0 hidden and 1 kept", + report.HiddenByMinImporters, len(report.SharedFiles)) + } +} + +// Go resolves imports at package level, so nothing imports an individual file +// inside a multi-file package and a file-level count is structurally 0 there. +// Two PRs editing two files of one package are editing one compilation unit, +// and that must not read as weightless. +func TestCollideGoSamePackageCollisionCarriesPackageWeight(t *testing.T) { + fg := &scanner.FileGraph{ + Root: "/repo", + Module: "example.com/app", + Imports: map[string][]string{}, + // The multi-file package carries no file-level edges at all: a Go + // import resolving to three files is dropped rather than fanned out. + // A file-level count here is a structural zero, which is the bug. + Importers: map[string][]string{}, + Packages: map[string][]string{ + "example.com/app/pkg": {"pkg/one.go", "pkg/two.go", "pkg/three.go"}, + "example.com/app/cmd": {"cmd/main.go"}, + }, + Coverage: collideCompleteCoverage(), + } + analyses := []scanner.FileAnalysis{ + {Path: "cmd/main.go", Language: "go", Imports: []string{"example.com/app/pkg"}}, + // Quoted, and from a directory the package index does not carry: still + // an importer. + {Path: "other/x.go", Language: "go", Imports: []string{`"example.com/app/pkg"`}}, + // A file importing its own package is not a second party to a + // collision inside it; it is counted once, as a sibling. + {Path: "pkg/two.go", Language: "go", Imports: []string{"example.com/app/pkg"}}, + // Third-party imports are not this module's packages. + {Path: "pkg/three.go", Language: "go", Imports: []string{"github.com/other/thing"}}, + } + lookup := collideGraphLookup("/repo", fg, analyses) + + got := lookup("pkg/one.go") + if !got.Known || !got.InGraph { + t.Fatalf("lookup(pkg/one.go) = %+v, want a known in-graph answer", got) + } + if got.scope() != collideScopePackage { + t.Errorf("lookup(pkg/one.go) scope = %q, want %q", got.scope(), collideScopePackage) + } + if got.Count != 4 { + t.Errorf("lookup(pkg/one.go) count = %d, want 4 (2 package importers + 2 siblings)", got.Count) + } + + // A _test.go file is not a member of the package index but does live in the + // package, so it is weighted with it rather than given a file-level zero — + // and identically to the file beside it, since the index's exclusion of + // test files is not a difference in hazard. + if got := lookup("pkg/one_test.go"); got.scope() != collideScopePackage || got.Count != 4 { + t.Errorf("lookup(pkg/one_test.go) = %+v, want package scope with count 4", got) + } + + prs := []collidePR{ + {Number: 1, Files: []collidePRFile{{Path: "pkg/one.go"}}}, + {Number: 2, Files: []collidePRFile{{Path: "pkg/one.go"}}}, + } + // --min-importers 1 used to hide this pair entirely. It is the hazard. + report := buildCollideReport(prs, lookup, fg.Coverage, "", 1) + if len(report.SharedFiles) != 1 || report.SharedFiles[0].ImporterCount == 0 { + t.Fatalf("shared files = %+v, want one file with a non-zero weight", report.SharedFiles) + } + if report.SharedFiles[0].ImporterScope != collideScopePackage { + t.Errorf("shared file scope = %q, want %q", report.SharedFiles[0].ImporterScope, collideScopePackage) + } + + var buf bytes.Buffer + renderCollideReport(&buf, report) + if output := buf.String(); !strings.Contains(output, "4 package importers") { + t.Errorf("human output does not label the granularity it measured:\n%s", output) + } +} + +// A file-resolved language keeps its file-level count and its plain label: the +// package hop exists only where the graph actually resolves at package level. +func TestCollideFileResolvedLanguagesKeepFileScope(t *testing.T) { + fg := &scanner.FileGraph{ + Root: "/repo", + Module: "example.com/app", + Imports: map[string][]string{}, + Importers: map[string][]string{"src/api.ts": {"src/a.ts", "src/b.ts"}}, + Packages: map[string][]string{"example.com/app/pkg": {"pkg/one.go"}}, + Coverage: collideCompleteCoverage(), + } + lookup := collideGraphLookup("/repo", fg, nil) + + got := lookup("src/api.ts") + if got.scope() != collideScopeFile || got.Count != 2 { + t.Errorf("lookup(src/api.ts) = %+v, want file scope with 2 importers", got) + } + if text := collideImportersText(got.Known, got.InGraph, got.Count, got.scope()); text != "2 importers" { + t.Errorf("label = %q, want %q", text, "2 importers") + } +} + +// Issue #134 measured the collision matrix among #124-#127 by merging all six +// pairs by hand, and file intersection alone got every one of them right. The +// weighting is allowed to reorder that list. It is not allowed to change it. +func TestCollideWeightingReordersPairsWithoutChangingThem(t *testing.T) { + fileScoped := collideSharedFilesOrFail(t, collideFixturePRs(), collideFixtureLookup) + // The same four PRs over the same paths, weighted at package granularity: + // the sg-rules YAML still carries no edges, but the two Go files now weigh + // by their package, and rustgraph.go outranks astgrep.go. + packageScoped := collideSharedFilesOrFail(t, collideFixturePRs(), func(path string) collideImporters { + switch path { + case "scanner/astgrep.go": + return collideImporters{Count: 11, Known: true, InGraph: true, Scope: collideScopePackage} + case "scanner/rustgraph.go": + return collideImporters{Count: 42, Known: true, InGraph: true, Scope: collideScopePackage} + default: + return collideFixtureLookup(path) + } + }) + + before := collidePairs(fileScoped) + after := collidePairs(packageScoped) + + // Same pairs, same shared-file counts. + beforeSet := pairCounts(before) + afterSet := pairCounts(after) + if len(beforeSet) != 6 { + t.Fatalf("file-level pairs = %v, want the six pairs #134 measured", beforeSet) + } + for label, count := range beforeSet { + got, ok := afterSet[label] + if !ok { + t.Errorf("weighting dropped pair %s, which #134 measured as real", label) + continue + } + if got != count { + t.Errorf("pair %s shared file count = %d under weighting, want %d", label, got, count) + } + } + for label := range afterSet { + if _, ok := beforeSet[label]; !ok { + t.Errorf("weighting invented pair %s, which is not in #134's matrix", label) + } + } + + // Order is the only thing allowed to move, and here it does: the top file + // of every pair changes from astgrep.go to the heavier rustgraph.go. + if before[0].TopFile != "scanner/astgrep.go" { + t.Fatalf("file-level top file = %q, want scanner/astgrep.go", before[0].TopFile) + } + if after[0].TopFile != "scanner/rustgraph.go" { + t.Errorf("package-level top file = %q, want scanner/rustgraph.go (42 beats 11)", after[0].TopFile) + } +} + func TestCollideHumanOutputGolden(t *testing.T) { report := buildCollideReport(collideFixturePRs(), collideFixtureLookup, collideCompleteCoverage(), "JordanCoin/codemap", 0) @@ -371,3 +538,20 @@ func mustSharedFiles(t *testing.T, prs []collidePR, lookup collideLookup, minImp func pairLabel(a, b int) string { return fmt.Sprintf("#%d+#%d", a, b) } + +func collideSharedFilesOrFail(t *testing.T, prs []collidePR, lookup collideLookup) []collideSharedFile { + t.Helper() + shared, hidden := collideSharedFiles(prs, lookup, 0) + if hidden != 0 { + t.Fatalf("hidden = %d, want 0 at --min-importers 0", hidden) + } + return shared +} + +func pairCounts(pairs []collidePair) map[string]int { + counts := make(map[string]int, len(pairs)) + for _, pair := range pairs { + counts[pairLabel(pair.A, pair.B)] = pair.SharedFileCount + } + return counts +} From 376413469b7441c86a2198afdab77eb8118df380 Mon Sep 17 00:00:00 2001 From: Jordan Coin Jackson Date: Fri, 4 Sep 2026 10:56:57 -0400 Subject: [PATCH 5/5] fix(collide): rank a pair by its heaviest shared file, not the first one seen Shared files sort by PR count first, so a pair colliding on a hub could be reported by a fixture touched by more PRs and ranked below a lesser pair. Found by independent review; the new test reproduces it and fails without the change. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01TcyheQmM3HCvxF5wRL3s5t --- collide.go | 21 ++++++++++++++------- collide_test.go | 42 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 56 insertions(+), 7 deletions(-) diff --git a/collide.go b/collide.go index fe79e4f..9b20185 100644 --- a/collide.go +++ b/collide.go @@ -131,6 +131,15 @@ type collidePair struct { TopFileInGraph bool `json:"top_file_in_graph"` } +// topWeight is the pair's severity: the importer weight of its worst shared +// file, or the unknown weight when the graph could not measure it. +func (p collidePair) topWeight() int { + if !p.TopImportersKnown { + return collideUnknownWeight + } + return p.TopImporterCount +} + // collideLanguageCoverage reports graph coverage for one language present in // the shared set, which is the only slice of the repository this verdict rests // on. @@ -534,9 +543,10 @@ func collidePairs(shared []collideSharedFile) []collidePair { pairs[key] = pair } pair.SharedFiles = append(pair.SharedFiles, file.Path) - // shared is already ordered worst-first, so the first file to - // land on a pair is its top file. - if pair.TopFile == "" { + // shared is ordered by PR count first, not by importer weight, + // so the first file to land on a pair is not necessarily its + // worst one. Keep the heaviest shared file as the top file. + if pair.TopFile == "" || file.weight() > pair.topWeight() { pair.TopFile = file.Path pair.TopImporterCount = file.ImporterCount pair.TopImporterScope = file.ImporterScope @@ -554,10 +564,7 @@ func collidePairs(shared []collideSharedFile) []collidePair { } weight := func(pair collidePair) int { - if !pair.TopImportersKnown { - return collideUnknownWeight - } - return pair.TopImporterCount + return pair.topWeight() } sort.Slice(out, func(i, j int) bool { left, right := out[i], out[j] diff --git a/collide_test.go b/collide_test.go index 81f57b3..c7319e5 100644 --- a/collide_test.go +++ b/collide_test.go @@ -157,6 +157,48 @@ func TestCollidePairsRankByImporterCountThenSharedFiles(t *testing.T) { } } +// A pair's top file must be its heaviest shared file, not the first one the +// PR-count ordering happens to hand it. Reproduction from independent review: +// #1 and #2 share a 100-importer hub and a fixture touched by four PRs; the +// fixture sorts first by PR count, so a first-file rule reported the pair by +// the fixture (0 importers) and ranked it below a lesser pair. +func TestCollidePairsTopFileIsHeaviestNotFirst(t *testing.T) { + prs := []collidePR{ + {Number: 1, Files: []collidePRFile{{Path: "hub.go"}, {Path: "fixture.go"}}}, + {Number: 2, Files: []collidePRFile{{Path: "hub.go"}, {Path: "fixture.go"}}}, + {Number: 3, Files: []collidePRFile{{Path: "fixture.go"}}}, + {Number: 4, Files: []collidePRFile{{Path: "fixture.go"}}}, + {Number: 5, Files: []collidePRFile{{Path: "mid.go"}}}, + {Number: 6, Files: []collidePRFile{{Path: "mid.go"}}}, + } + lookup := func(path string) collideImporters { + switch path { + case "hub.go": + return collideImporters{Count: 100, Known: true, InGraph: true} + case "mid.go": + return collideImporters{Count: 50, Known: true, InGraph: true} + default: + return collideImporters{Count: 0, Known: true, InGraph: true} + } + } + + pairs := collidePairs(mustSharedFiles(t, prs, lookup, 0)) + if len(pairs) == 0 { + t.Fatal("no pairs") + } + if label := pairLabel(pairs[0].A, pairs[0].B); label != "#1+#2" { + t.Errorf("top pair = %s (top file %s, %d importers), want #1+#2 on the hub", + label, pairs[0].TopFile, pairs[0].TopImporterCount) + } + if pairs[0].TopFile != "hub.go" || pairs[0].TopImporterCount != 100 { + t.Errorf("#1+#2 top file = %s (%d importers), want hub.go (100)", pairs[0].TopFile, pairs[0].TopImporterCount) + } + // An unknown weight must never displace a measured one, even a zero. + if collideUnknownWeight >= 0 { + t.Fatalf("collideUnknownWeight = %d, must sort below a measured zero", collideUnknownWeight) + } +} + // --min-importers is a severity filter, so it may only hide files whose // severity is known to be below it. func TestCollideMinImportersHidesLowCountsButNeverUnknownOnes(t *testing.T) {