From 813eac693170c11c2488ac6293f8dc98d00a9603 Mon Sep 17 00:00:00 2001 From: Jorge Cajas Date: Thu, 30 Jul 2026 01:31:34 +0800 Subject: [PATCH 1/2] feat(#6042): infer a community for unplaced changed entities, labelled as inferred MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #6006 made grafel_pr_impact decline when NO changed entity could be placed in a community. Correct — with zero community signal any verdict is invented — but it makes the tool useless for the most ordinary PR shape there is: one that only adds code. A newly added entity is never in the group-algo overlay, because the overlay is computed from the last indexed group union and the entity did not exist then. So an add-only PR had every changed entity unplaced and got an explanatory IsError instead of a verdict. The information to do better is already in the graph. This infers a community for an unplaced changed entity from its PLACED neighbours — and labels it. ## The signals, and why these three Only signals the graph demonstrably carries at this layer were used: container (weight 3) — entities sharing the new entity's SourceFile, plus any CONTAINS parent. A file is the smallest unit Louvain would essentially never split, so a new function in a placed file is in that file's community with very high confidence. module (weight 2) — Properties["module"], stamped on the full (cmd/grafel/index.go) and incremental (extractors/incremental.go) paths and round-tripped through graph.fb (load.go restores the FB scalar into props). NOTE it is module.Derive — a depth-capped path rollup — so it is a COARSER view of the same fact the container signal reads, not an independent one. Hence the lower weight: it can never outvote the file on its own. targets (weight 1) — placed entities the new entity calls. Closest to what Louvain itself would have seen. Requires >= 2 placed targets: one outbound edge is not evidence (everything calls a logger), and a single-edge rule would place half of every new package into whatever community the nearest shared utility happens to sit in. Each signal casts ONE vote for the strict plurality of its own evidence, and abstains entirely when it has no unique winner (a file split 1-1 across communities) rather than letting map order decide. A community is inferred only when its weight strictly exceeds the total weight of all dissent: container alone -> infer (3 > 0) container 7 vs module 9 -> infer 7 (3 > 2 — container weighted highest) container 7 vs module 9 + targets 9 -> DECLINE (3 is not > 3) nothing available -> DECLINE Deliberately NOT inferred: overlay-placed entities (the overlay is ground truth and wins outright), removed entities (absent from the head graph, so no neighbours — the diff record is not a community signal), blast-radius entities (only the changed set drives the verdict), and chained inference (only OVERLAY-placed entities vote; letting guesses vote would propagate one weak signal across a whole new package and make the result order-dependent). ## Inference is never presented as fact Silent inference would be the #6006 defect one layer up. So: - ChangedEntity.CommunitySource = overlay | inferred | none, per entity, no omitempty — the label is always on the wire. - changed_entities_with_overlay_community / _with_inferred_community aggregate alongside the existing changed_entities_without_community; the three always sum to changed_count. - community_data_inferred_only at the verdict level (both modes), plus inferred_entity_count and refs_with_inferred_community_data in conflicts mode, and a plain-language community_data_note when the whole verdict is deduced. - ImpactedCommunity.inferred_changed_count shows how much of a community's involvement was deduced. - CommunityDataInferredOnly is FALSE when nothing was placed at all: that is a decline, not a low-confidence answer. #6006's decline path is untouched and still binds. The >= 2 placed targets threshold is precisely what keeps its add-only fixture (an added entity calling exactly one placed entity) declining; a mutation lowering it to 1 fails TestAnalyzePRImpact_AvailabilityFollowsChangedSetNotEntitySet. ## Cost Nothing is built unless a changed entity is present in the head graph AND unplaced. Then: one pass over entities filtered to the (small) set of files and modules the unplaced changed entities occupy, and outbound/CONTAINS edges captured during the adjacency pass AnalyzePRImpact already makes, for changed ids only. No BFS, no transitive walk. Measured on a 20k-entity / 20k-edge graph with a 1-entity add-only change set: 3.86 ms -> 4.47 ms (+0.61 ms, +16%) and +1 KB / +12 allocs. Against the handler's 326 ms / 76 MB overlay parse (memoized on path+mtime+size, unchanged here) that is under 0.2% of a cold call. ## Fixtures Every fixture uses a PRODUCTION-SHAPED overlay covering only entities that existed at the last group index; newly added entities are absent from it by construction. Covered: a new entity in a placed file (infers), in a new file with two placed call targets (infers), in a new module (infers from module), with no placed neighbours (declines), with disagreeing signals (declines), with a 1-1 split container (abstains), a single call target (declines), chained inference (declines), and an overlay-placed entity (never overwritten). 19 mutations were reintroduced and all 19 were killed, including: inference disabled entirely, inference always succeeding, minPlacedTargets lowered to 1, plurality ties decided by map order, inference chaining, inference overriding the overlay, removed entities inferred from the diff record, the placed-vs- inferred label collapsed to "overlay", the aggregate counts dropped from the MCP payload, community_source dropped from the wire, and the decline path weakened to always-available. Refs #6006 --- internal/graph/pr_impact.go | 185 +++++++++-- internal/graph/pr_impact_6042_test.go | 456 ++++++++++++++++++++++++++ internal/graph/pr_impact_infer.go | 302 +++++++++++++++++ internal/mcp/pr_impact_6042_test.go | 373 +++++++++++++++++++++ internal/mcp/pr_impact_tools.go | 48 ++- 5 files changed, 1330 insertions(+), 34 deletions(-) create mode 100644 internal/graph/pr_impact_6042_test.go create mode 100644 internal/graph/pr_impact_infer.go create mode 100644 internal/mcp/pr_impact_6042_test.go diff --git a/internal/graph/pr_impact.go b/internal/graph/pr_impact.go index 5cadcecdf..02a5c881b 100644 --- a/internal/graph/pr_impact.go +++ b/internal/graph/pr_impact.go @@ -41,6 +41,16 @@ type ChangedEntity struct { SourceFile string `json:"source_file,omitempty"` Change string `json:"change"` // added | removed | modified CommunityID int `json:"community_id"` // -1 when ungrouped/unknown + + // CommunitySource says HOW CommunityID was arrived at (#6042): + // "overlay" (measured — the group-algo partition placed it), "inferred" + // (deduced from placed neighbours, see pr_impact_infer.go), or "none" (not + // placed at all, CommunityID is -1). + // + // A caller that ignores this field is reading a guess as a measurement, which + // is the same defect class as #6006 — hence no omitempty: the label is always + // on the wire. + CommunitySource string `json:"community_source"` } // ImpactedCommunity is a community touched by the change, with how many changed @@ -49,6 +59,10 @@ type ImpactedCommunity struct { CommunityID int `json:"community_id"` ChangedCount int `json:"changed_count"` // changed entities in this community BlastRadiusHit int `json:"blast_radius_hit"` // downstream entities in this community + // InferredChangedCount is how many of ChangedCount were placed here by + // INFERENCE rather than by the overlay (#6042). Equal to ChangedCount means + // this community's involvement is entirely deduced. + InferredChangedCount int `json:"inferred_changed_count,omitempty"` } // BlastEntity is a downstream entity that transitively depends on a changed @@ -102,10 +116,30 @@ type PRImpactResult struct { // live default-base case — conflicts mode diffs refs[0] against itself.) CommunityDataAvailable bool `json:"community_data_available"` // ChangedWithoutCommunity counts changed entities carrying no community, i.e. - // the entities this analysis could not place. Non-zero with - // CommunityDataAvailable=true is PARTIAL coverage: the verdict stands on the - // entities that were placed, but this many were invisible to it. + // the entities this analysis could not place — neither by the overlay nor by + // inference. Non-zero with CommunityDataAvailable=true is PARTIAL coverage: + // the verdict stands on the entities that were placed, but this many were + // invisible to it. ChangedWithoutCommunity int `json:"changed_entities_without_community"` + + // ── #6042: measured vs deduced ────────────────────────────────────────── + // + // ChangedWithOverlayCommunity + ChangedWithInferredCommunity + + // ChangedWithoutCommunity == ChangedCount, always. + + // ChangedWithOverlayCommunity counts changed entities the group-algo overlay + // placed directly. These are MEASURED. + ChangedWithOverlayCommunity int `json:"changed_entities_with_overlay_community"` + // ChangedWithInferredCommunity counts changed entities placed by inference + // from their placed neighbours (pr_impact_infer.go). These are DEDUCED — good + // enough to triage with, not good enough to present as measured. + ChangedWithInferredCommunity int `json:"changed_entities_with_inferred_community"` + // CommunityDataInferredOnly is the verdict-level confidence marker: the + // analysis ran, but EVERY placement behind it was inferred. An agent should + // weigh such a verdict differently from one the overlay measured. False when + // nothing was placed at all — that case is a decline + // (CommunityDataAvailable=false), not a low-confidence answer. + CommunityDataInferredOnly bool `json:"community_data_inferred_only"` } // PRImpactOptions bounds the analysis. @@ -181,10 +215,45 @@ func AnalyzePRImpact(entities []Entity, rels []Relationship, change ChangeSet, o byID[entities[i].ID] = entities[i] } + // ── Part 1: changed entities + their communities ───────────────────────── + classOf := map[string]string{} + for _, e := range change.Removed { + classOf[e.ID] = "removed" + } + for _, e := range change.Added { + classOf[e.ID] = "added" + } + for _, e := range change.Modified { + classOf[e.ID] = "modified" + } + + changedIDs := change.ChangedIDs() + seedSet := make(map[string]struct{}, len(changedIDs)) + // #6042: the changed entities the overlay could NOT place, and which are + // present in the head graph so they have neighbours to read. Computed before + // the edge pass below so that pass can capture their edges in one sweep + // instead of building a whole outbound adjacency for the graph. + unplacedChanged := make(map[string]struct{}) + for _, id := range changedIDs { + seedSet[id] = struct{}{} + if e, ok := byID[id]; ok && communityOf(e) < 0 { + unplacedChanged[id] = struct{}{} + } + } + // Inbound adjacency: in[X] = entities that depend on X (callers). Restricted // to edges whose both endpoints are present in the entity set, matching the // edge-filtering contract used elsewhere. + // + // The same sweep collects the #6042 inference inputs — outbound targets and + // CONTAINS parents — but ONLY for unplaced changed entities, so the extra + // memory is O(deg(changed)) rather than O(E). in := make(map[string][]string, len(entities)) + var inferParents, inferTargets map[string][]string + if len(unplacedChanged) > 0 { + inferParents = make(map[string][]string, len(unplacedChanged)) + inferTargets = make(map[string][]string, len(unplacedChanged)) + } for _, r := range rels { if r.FromID == "" || r.ToID == "" || r.FromID == r.ToID { continue @@ -197,35 +266,43 @@ func AnalyzePRImpact(entities []Entity, rels []Relationship, change ChangeSet, o } // r.FromID depends on r.ToID, so FromID is a downstream dependent of ToID. in[r.ToID] = append(in[r.ToID], r.FromID) - } - // ── Part 1: changed entities + their communities ───────────────────────── - classOf := map[string]string{} - for _, e := range change.Removed { - classOf[e.ID] = "removed" - } - for _, e := range change.Added { - classOf[e.ID] = "added" - } - for _, e := range change.Modified { - classOf[e.ID] = "modified" + if len(unplacedChanged) > 0 { + if _, ok := unplacedChanged[r.FromID]; ok { + inferTargets[r.FromID] = append(inferTargets[r.FromID], r.ToID) + } + if r.Kind == "CONTAINS" { + if _, ok := unplacedChanged[r.ToID]; ok { + inferParents[r.ToID] = append(inferParents[r.ToID], r.FromID) + } + } + } } + inferrer := newCommunityInferrer(entities, byID, unplacedChanged, inferParents, inferTargets) - changedIDs := change.ChangedIDs() changed := make([]ChangedEntity, 0, len(changedIDs)) - // communityChanged[community] = #changed entities in it. + // communityChanged[community] = #changed entities in it; communityInferred is + // the inferred subset of that, so a caller can see how much of a community's + // involvement was deduced rather than measured. communityChanged := map[int]int{} + communityInferred := map[int]int{} // #6006: how many changed entities we could NOT place in a community. This, // not the graph-wide entity set, decides whether the community-derived output // below means anything — see PRImpactResult.CommunityDataAvailable. changedWithoutCommunity := 0 - seedSet := make(map[string]struct{}, len(changedIDs)) + // #6042: the measured/deduced split behind the verdict. + changedFromOverlay, changedFromInference := 0, 0 for _, id := range changedIDs { - seedSet[id] = struct{}{} comm := -1 + source := CommunitySourceNone var name, kind, src string if e, ok := byID[id]; ok { comm = communityOf(e) + if comm >= 0 { + source = CommunitySourceOverlay + } else if c, inferred := inferrer.infer(id); inferred { + comm, source = c, CommunitySourceInferred + } name, kind, src = e.Name, e.Kind, e.SourceFile } else { // Removed entity (gone from HEAD) — fall back to the diff record. @@ -238,21 +315,32 @@ func AnalyzePRImpact(entities []Entity, rels []Relationship, change ChangeSet, o } } changed = append(changed, ChangedEntity{ - ID: id, - Name: name, - Kind: kind, - SourceFile: src, - Change: classOf[id], - CommunityID: comm, + ID: id, + Name: name, + Kind: kind, + SourceFile: src, + Change: classOf[id], + CommunityID: comm, + CommunitySource: source, }) communityChanged[comm]++ - if comm < 0 { + switch source { + case CommunitySourceOverlay: + changedFromOverlay++ + case CommunitySourceInferred: + changedFromInference++ + communityInferred[comm]++ + default: changedWithoutCommunity++ } } // Vacuously available when nothing changed; otherwise at least one changed - // entity must have been placed for the community verdict to mean anything. + // entity must have been placed — by the overlay or, since #6042, by inference + // — for the community verdict to mean anything. communityDataAvailable := len(changedIDs) == 0 || changedWithoutCommunity < len(changedIDs) + // #6042: the analysis ran, but every placement behind it is a deduction. Not + // set when nothing was placed at all — that is a decline, not a soft answer. + inferredOnly := communityDataAvailable && changedFromInference > 0 && changedFromOverlay == 0 // ── Part 2: downstream blast radius (inbound BFS from all seeds) ────────── // Multi-source BFS: distance is hops from the nearest changed seed. @@ -340,9 +428,10 @@ func AnalyzePRImpact(entities []Entity, rels []Relationship, change ChangeSet, o continue } impacted = append(impacted, ImpactedCommunity{ - CommunityID: c, - ChangedCount: communityChanged[c], - BlastRadiusHit: communityBlast[c], + CommunityID: c, + ChangedCount: communityChanged[c], + BlastRadiusHit: communityBlast[c], + InferredChangedCount: communityInferred[c], }) } // Rank by total touch (changed+blast) desc, then community id asc. @@ -366,6 +455,10 @@ func AnalyzePRImpact(entities []Entity, rels []Relationship, change ChangeSet, o CommunityDataAvailable: communityDataAvailable, ChangedWithoutCommunity: changedWithoutCommunity, + + ChangedWithOverlayCommunity: changedFromOverlay, + ChangedWithInferredCommunity: changedFromInference, + CommunityDataInferredOnly: inferredOnly, } } @@ -402,6 +495,12 @@ type ChangeImpact struct { // because nothing was computed, and no conclusion about merge safety can be // drawn from this ref at all. CommunityDataAvailable bool + + // OverlayEntityCount / InferredEntityCount carry this ref's measured/deduced + // split (#6042), so the merge-risk verdict can say whether it rests on the + // group-algo partition or on inference from placed neighbours. + OverlayEntityCount int + InferredEntityCount int } // MergeRiskPair is two refs whose impacted-community sets overlap. @@ -428,6 +527,21 @@ type MergeRiskResult struct { CommunityDataAvailable bool `json:"community_data_available"` // RefsWithoutCommunityData names the refs that had no community data, sorted. RefsWithoutCommunityData []string `json:"refs_without_community_data,omitempty"` + + // ── #6042: how much of this verdict is deduced ────────────────────────── + + // InferredEntityCount totals the changed entities across all refs that were + // placed by INFERENCE rather than by the group-algo overlay. + InferredEntityCount int `json:"inferred_entity_count"` + // CommunityDataInferredOnly is true when the analysis ran but NO ref + // contributed a single overlay-measured entity — the add-only PR shape #6042 + // exists for. The pairs below are then a reasoned guess at what Louvain would + // have said, not a reading of what it did say. False when the data was + // unavailable altogether: that is a decline, not a low-confidence answer. + CommunityDataInferredOnly bool `json:"community_data_inferred_only"` + // RefsWithInferredCommunityData names the refs that contributed at least one + // inferred placement, sorted. + RefsWithInferredCommunityData []string `json:"refs_with_inferred_community_data,omitempty"` } // AnalyzeMergeRisk intersects every change's impacted-community set pairwise and @@ -442,11 +556,17 @@ func AnalyzeMergeRisk(impacts []ChangeImpact) MergeRiskResult { norm := make([]ChangeImpact, len(impacts)) copy(norm, impacts) sort.SliceStable(norm, func(i, j int) bool { return norm[i].Ref < norm[j].Ref }) - var missing []string + var missing, inferredRefs []string + totalInferred, totalOverlay := 0, 0 for _, ci := range norm { if !ci.CommunityDataAvailable { missing = append(missing, ci.Ref) } + totalInferred += ci.InferredEntityCount + totalOverlay += ci.OverlayEntityCount + if ci.InferredEntityCount > 0 { + inferredRefs = append(inferredRefs, ci.Ref) + } } sets := make([]map[int]struct{}, len(norm)) for i, ci := range norm { @@ -492,6 +612,11 @@ func AnalyzeMergeRisk(impacts []ChangeImpact) MergeRiskResult { CommunityDataAvailable: len(missing) == 0, RefsWithoutCommunityData: missing, + + InferredEntityCount: totalInferred, + // Available, something was inferred, and nothing at all was measured. + CommunityDataInferredOnly: len(missing) == 0 && totalInferred > 0 && totalOverlay == 0, + RefsWithInferredCommunityData: inferredRefs, } } diff --git a/internal/graph/pr_impact_6042_test.go b/internal/graph/pr_impact_6042_test.go new file mode 100644 index 000000000..e0a70accf --- /dev/null +++ b/internal/graph/pr_impact_6042_test.go @@ -0,0 +1,456 @@ +// pr_impact_6042_test.go — issue #6042: an add-only PR must still get a +// merge-risk verdict, WITHOUT the verdict pretending to be measured. +// +// #6006 made the tool decline when no changed entity could be placed in a +// community. That is correct but useless for the most ordinary PR shape there +// is: one that only adds code. A newly added entity is never in the group-algo +// overlay, because the overlay is computed from the last indexed group union +// and the entity did not exist then. +// +// So we INFER a community from the entity's PLACED neighbours — and label it. +// The two failure modes this file pins are symmetric: +// +// - inference that never fires (the tool stays useless), and +// - inference presented as fact (the #6006 defect class, reintroduced one +// layer up: a confident answer the caller cannot distinguish from a +// measured one). +// +// FIXTURE SHAPE IS LOAD-BEARING. Every fixture below is production-shaped: only +// entities that existed at the last group index carry a CommunityID. Newly +// added entities carry NONE — stamping them would fabricate a state the +// group-algo pass cannot produce, and would make every test here vacuous. +package graph + +import "testing" + +// placedEnt is an entity as the overlay stamper leaves it: covered by the last +// group index, so it carries a real community id. +func placedEnt(id, file, module string, community int) Entity { + e := Entity{ID: id, Name: id, Kind: "function", SourceFile: file, CommunityID: ci(community)} + if module != "" { + e = e.WithProperties(map[string]string{"module": module}) + } + return e +} + +// newEnt is an entity that exists only on the feature ref: no community, by +// construction, because the overlay predates it. +func newEnt(id, file, module string) Entity { + e := Entity{ID: id, Name: id, Kind: "function", SourceFile: file} + if module != "" { + e = e.WithProperties(map[string]string{"module": module}) + } + return e +} + +// prImpact6042Fixture is the "last indexed group union": two communities, each +// with its own directory/module. Nothing here is new. +// +// community 7 (core): core:a, core:b, core:c in core/*.go, module "core" +// community 9 (db): db:d, db:e in db/*.go, module "db" +func prImpact6042Fixture() ([]Entity, []Relationship) { + return []Entity{ + placedEnt("core:a", "core/a.go", "core", 7), + placedEnt("core:b", "core/b.go", "core", 7), + placedEnt("core:c", "core/c.go", "core", 7), + placedEnt("db:d", "db/d.go", "db", 9), + placedEnt("db:e", "db/e.go", "db", 9), + }, []Relationship{ + {FromID: "core:a", ToID: "core:b", Kind: "CALLS"}, + {FromID: "db:d", ToID: "db:e", Kind: "CALLS"}, + } +} + +// addOne appends one added entity (+ its outbound edges) and returns the change +// set that adds exactly it — the add-only PR shape. +func addOne(ents []Entity, rels []Relationship, e Entity, calls ...string) ([]Entity, []Relationship, ChangeSet) { + ents = append(ents, e) + for _, target := range calls { + rels = append(rels, Relationship{FromID: e.ID, ToID: target, Kind: "CALLS"}) + } + return ents, rels, ChangeSet{Added: []DiffEntityEntry{{ID: e.ID, Name: e.Name, Kind: e.Kind}}} +} + +// changedByID finds the changed-entity record, failing loudly when absent. +func changedByID(t *testing.T, res PRImpactResult, id string) ChangedEntity { + t.Helper() + for _, c := range res.ChangedEntities { + if c.ID == id { + return c + } + } + t.Fatalf("changed entity %q missing from result: %+v", id, res.ChangedEntities) + return ChangedEntity{} +} + +// ── The signals ────────────────────────────────────────────────────────────── + +// Signal 1 — containing component. A new function added to an ALREADY-PLACED +// file belongs to that file's community with very high confidence. Module is +// deliberately unset here so the file is the only signal in play. +func TestAnalyzePRImpact_InfersFromContainingFile(t *testing.T) { + ents, rels := prImpact6042Fixture() + ents, rels, change := addOne(ents, rels, newEnt("core:new", "core/a.go", "")) + + res := AnalyzePRImpact(ents, rels, change, DefaultPRImpactOptions()) + + got := changedByID(t, res, "core:new") + if got.CommunityID != 7 { + t.Errorf("new entity in core/a.go (community 7) got community %d, want 7", got.CommunityID) + } + if got.CommunitySource != CommunitySourceInferred { + t.Errorf("community_source = %q, want %q — an inferred placement presented as "+ + "measured is the #6006 defect one layer up", got.CommunitySource, CommunitySourceInferred) + } + if !res.CommunityDataAvailable { + t.Errorf("an inferred placement is still a placement; CommunityDataAvailable = false") + } + if res.ChangedWithInferredCommunity != 1 || res.ChangedWithOverlayCommunity != 0 || + res.ChangedWithoutCommunity != 0 { + t.Errorf("counts = overlay %d / inferred %d / none %d, want 0/1/0", + res.ChangedWithOverlayCommunity, res.ChangedWithInferredCommunity, res.ChangedWithoutCommunity) + } + if !res.CommunityDataInferredOnly { + t.Errorf("every placement in this verdict is inferred; CommunityDataInferredOnly = false — " + + "the caller cannot tell a fully-inferred verdict from a measured one") + } + if ids := res.ImpactedCommunityIDs(); len(ids) != 1 || ids[0] != 7 { + t.Errorf("inferred community must reach the merge-risk key; got %v, want [7]", ids) + } + // The rollup says how much of the community's touch was inferred. + if len(res.ImpactedCommunities) != 1 || res.ImpactedCommunities[0].InferredChangedCount != 1 { + t.Errorf("impacted community rollup must count the inferred entity: %+v", res.ImpactedCommunities) + } +} + +// Signal 2 — module. A brand-new FILE inside an existing module: no same-file +// sibling exists, so only the module prior can place it. +func TestAnalyzePRImpact_InfersFromModuleWhenFileIsNew(t *testing.T) { + ents, rels := prImpact6042Fixture() + ents, rels, change := addOne(ents, rels, newEnt("core:new", "core/brand_new.go", "core")) + + res := AnalyzePRImpact(ents, rels, change, DefaultPRImpactOptions()) + + got := changedByID(t, res, "core:new") + if got.CommunityID != 7 || got.CommunitySource != CommunitySourceInferred { + t.Errorf("new file in module \"core\" got community %d source %q, want 7/inferred", + got.CommunityID, got.CommunitySource) + } +} + +// Signal 3 — outbound call targets. A new entity in a new file in a new module, +// calling two placed entities that agree. This is close to what Louvain would +// have decided had it seen the entity. +func TestAnalyzePRImpact_InfersFromPlacedCallTargets(t *testing.T) { + ents, rels := prImpact6042Fixture() + ents, rels, change := addOne(ents, rels, + newEnt("new:x", "newpkg/x.go", "newpkg"), "core:a", "core:b") + + res := AnalyzePRImpact(ents, rels, change, DefaultPRImpactOptions()) + + got := changedByID(t, res, "new:x") + if got.CommunityID != 7 || got.CommunitySource != CommunitySourceInferred { + t.Errorf("new entity calling two community-7 entities got community %d source %q, want 7/inferred", + got.CommunityID, got.CommunitySource) + } +} + +// ── The decline paths that MUST survive ────────────────────────────────────── + +// A SINGLE placed call target is not evidence. Everything calls a logger; one +// edge would place half of every new package in whatever community the shared +// utility happens to sit in. This threshold is also exactly what keeps +// #6006's TestAnalyzePRImpact_AvailabilityFollowsChangedSetNotEntitySet binding. +func TestAnalyzePRImpact_SingleCallTargetIsNotEnoughToInfer(t *testing.T) { + ents, rels := prImpact6042Fixture() + ents, rels, change := addOne(ents, rels, newEnt("new:x", "newpkg/x.go", "newpkg"), "core:a") + + res := AnalyzePRImpact(ents, rels, change, DefaultPRImpactOptions()) + + got := changedByID(t, res, "new:x") + if got.CommunitySource != CommunitySourceNone || got.CommunityID != -1 { + t.Errorf("one outbound edge is not a community signal; got community %d source %q, want -1/none", + got.CommunityID, got.CommunitySource) + } + if res.CommunityDataAvailable { + t.Errorf("nothing could be placed, so the #6006 decline path must stand") + } + if res.ChangedWithoutCommunity != 1 { + t.Errorf("ChangedWithoutCommunity = %d, want 1", res.ChangedWithoutCommunity) + } +} + +// No placed neighbours at all — a whole new subsystem. Nothing to infer from, +// so the tool must still decline rather than invent a community. +func TestAnalyzePRImpact_NoPlacedNeighboursStillDeclines(t *testing.T) { + ents, rels := prImpact6042Fixture() + ents, rels, change := addOne(ents, rels, newEnt("new:lonely", "brandnew/l.go", "brandnew")) + + res := AnalyzePRImpact(ents, rels, change, DefaultPRImpactOptions()) + + got := changedByID(t, res, "new:lonely") + if got.CommunitySource != CommunitySourceNone { + t.Errorf("no placed neighbour exists; community_source = %q, want none", got.CommunitySource) + } + if res.CommunityDataAvailable { + t.Errorf("CommunityDataAvailable = true with nothing placed and nothing inferrable — " + + "#6006's decline path was weakened into meaninglessness") + } + if res.CommunityDataInferredOnly { + t.Errorf("no inference happened; CommunityDataInferredOnly must be false") + } +} + +// Signals that DISAGREE must decline, not guess. Here the containing file says +// community 7 while the module AND the call targets say 9 — the containing +// component is weighted highest, but not highly enough to outweigh both of the +// other signals together. +func TestAnalyzePRImpact_ConflictingSignalsDecline(t *testing.T) { + ents, rels := prImpact6042Fixture() + ents, rels, change := addOne(ents, rels, + newEnt("core:new", "core/a.go", "db"), // file → 7, module → 9 + "db:d", "db:e") // targets → 9 + + res := AnalyzePRImpact(ents, rels, change, DefaultPRImpactOptions()) + + got := changedByID(t, res, "core:new") + if got.CommunitySource != CommunitySourceNone { + t.Errorf("file says 7, module and call targets say 9 — inference must abstain; got %d/%q", + got.CommunityID, got.CommunitySource) + } + if res.CommunityDataAvailable { + t.Errorf("ambiguous signals are not a placement") + } +} + +// A file whose placed entities are SPLIT between communities gives no plurality, +// so the container signal abstains — and with no other signal, the entity is +// unplaced. Without this, a 50/50 file would be decided by map iteration order. +func TestAnalyzePRImpact_AmbiguousContainerAbstains(t *testing.T) { + ents, rels := prImpact6042Fixture() + // A second placed entity in core/a.go, in the OTHER community. + ents = append(ents, placedEnt("core:split", "core/a.go", "", 9)) + ents, rels, change := addOne(ents, rels, newEnt("core:new", "core/a.go", "")) + + res := AnalyzePRImpact(ents, rels, change, DefaultPRImpactOptions()) + + got := changedByID(t, res, "core:new") + if got.CommunitySource != CommunitySourceNone { + t.Errorf("core/a.go is split 1-1 between communities 7 and 9; inference must abstain, got %d/%q", + got.CommunityID, got.CommunitySource) + } +} + +// Inference must not CHAIN. Only overlay-placed entities may vote; an entity +// that was itself inferred is a guess, and letting guesses vote would propagate +// one weak signal across a whole new package. +func TestAnalyzePRImpact_InferenceDoesNotChain(t *testing.T) { + ents, rels := prImpact6042Fixture() + // first and second each infer 7 from their own placed call targets. third's + // only neighbours are first and second — enough targets to vote, but neither + // is OVERLAY-placed, so third must stay unplaced. Distinct new files keep the + // container signal out of it. + ents = append(ents, + newEnt("new:first", "newpkg/x.go", ""), + newEnt("new:second", "newpkg/y.go", ""), + newEnt("new:third", "newpkg/z.go", "")) + rels = append(rels, + Relationship{FromID: "new:first", ToID: "core:a", Kind: "CALLS"}, + Relationship{FromID: "new:first", ToID: "core:b", Kind: "CALLS"}, + Relationship{FromID: "new:second", ToID: "core:a", Kind: "CALLS"}, + Relationship{FromID: "new:second", ToID: "core:c", Kind: "CALLS"}, + Relationship{FromID: "new:third", ToID: "new:first", Kind: "CALLS"}, + Relationship{FromID: "new:third", ToID: "new:second", Kind: "CALLS"}, + ) + change := ChangeSet{Added: []DiffEntityEntry{ + {ID: "new:first"}, {ID: "new:second"}, {ID: "new:third"}, + }} + + res := AnalyzePRImpact(ents, rels, change, DefaultPRImpactOptions()) + + for _, id := range []string{"new:first", "new:second"} { + if got := changedByID(t, res, id); got.CommunitySource != CommunitySourceInferred { + t.Errorf("%s has two placed call targets; want inferred, got %q", id, got.CommunitySource) + } + } + got := changedByID(t, res, "new:third") + if got.CommunitySource != CommunitySourceNone { + t.Errorf("new:third's only neighbours were THEMSELVES inferred; want none, got %d/%q — "+ + "inference chained, so one weak signal propagated across a whole new package "+ + "and the result now depends on processing order", got.CommunityID, got.CommunitySource) + } +} + +// ── Labelling: measured and inferred must never blur ───────────────────────── + +// A mixed change — one modified entity the overlay covers, one added entity we +// infer — must report both counts separately, and must NOT claim the verdict +// rests entirely on inference. +func TestAnalyzePRImpact_OverlayAndInferredAreCountedSeparately(t *testing.T) { + ents, rels := prImpact6042Fixture() + ents = append(ents, newEnt("core:new", "core/a.go", "core")) + change := ChangeSet{ + Modified: []DiffEntityEntry{{ID: "core:c"}}, + Added: []DiffEntityEntry{{ID: "core:new"}}, + } + + res := AnalyzePRImpact(ents, rels, change, DefaultPRImpactOptions()) + + if got := changedByID(t, res, "core:c"); got.CommunitySource != CommunitySourceOverlay { + t.Errorf("core:c is covered by the overlay; community_source = %q, want overlay", got.CommunitySource) + } + if got := changedByID(t, res, "core:new"); got.CommunitySource != CommunitySourceInferred { + t.Errorf("core:new is new; community_source = %q, want inferred", got.CommunitySource) + } + if res.ChangedWithOverlayCommunity != 1 || res.ChangedWithInferredCommunity != 1 { + t.Errorf("counts = overlay %d / inferred %d, want 1/1", + res.ChangedWithOverlayCommunity, res.ChangedWithInferredCommunity) + } + if res.CommunityDataInferredOnly { + t.Errorf("one entity was really measured; CommunityDataInferredOnly must be false") + } +} + +// Nothing may be inferred when the entity was already placed by the overlay — +// the overlay is ground truth and must win outright. +func TestAnalyzePRImpact_OverlayPlacementIsNeverOverwritten(t *testing.T) { + ents, rels := prImpact6042Fixture() + // db:d sits in db/d.go but we pretend a stray placed sibling suggests 7. + ents = append(ents, placedEnt("db:stray", "db/d.go", "db", 7)) + res := AnalyzePRImpact(ents, rels, ChangeSet{ + Modified: []DiffEntityEntry{{ID: "db:d"}}, + }, DefaultPRImpactOptions()) + + got := changedByID(t, res, "db:d") + if got.CommunityID != 9 || got.CommunitySource != CommunitySourceOverlay { + t.Errorf("overlay-placed entity got %d/%q, want 9/overlay", got.CommunityID, got.CommunitySource) + } +} + +// An entity that is REMOVED on the head ref is absent from the head graph, so it +// has no neighbours to infer from. It must not be silently inferred from the +// diff record either. +func TestAnalyzePRImpact_RemovedEntityIsNotInferred(t *testing.T) { + ents, rels := prImpact6042Fixture() + change := ChangeSet{Removed: []DiffEntityEntry{ + {ID: "gone:z", Name: "Z", Kind: "function", SourceFile: "core/a.go"}, + }} + + res := AnalyzePRImpact(ents, rels, change, DefaultPRImpactOptions()) + + got := changedByID(t, res, "gone:z") + if got.CommunitySource != CommunitySourceNone { + t.Errorf("a removed entity has no head-graph neighbours; got %d/%q, want -1/none", + got.CommunityID, got.CommunitySource) + } +} + +// ── Merge risk ─────────────────────────────────────────────────────────────── + +// Two add-only refs whose entities were both INFERRED into the same community do +// produce a risky pair — that is the whole point of #6042 — but the result must +// say the verdict rests entirely on inference. +func TestAnalyzeMergeRisk_InferredOnlyVerdictIsFlagged(t *testing.T) { + inferred := AnalyzeMergeRisk([]ChangeImpact{ + {Ref: "pr-a", Communities: []int{7}, CommunityDataAvailable: true, InferredEntityCount: 1}, + {Ref: "pr-b", Communities: []int{7}, CommunityDataAvailable: true, InferredEntityCount: 2}, + }) + if inferred.RiskyPairs != 1 { + t.Fatalf("both refs land in community 7; want 1 risky pair, got %d", inferred.RiskyPairs) + } + if !inferred.CommunityDataInferredOnly { + t.Errorf("no ref contributed an overlay-measured entity; CommunityDataInferredOnly = false — " + + "an inferred verdict is indistinguishable from a measured one") + } + if inferred.InferredEntityCount != 3 { + t.Errorf("InferredEntityCount = %d, want 3", inferred.InferredEntityCount) + } + + // A measured verdict must NOT carry the marker. + measured := AnalyzeMergeRisk([]ChangeImpact{ + {Ref: "pr-a", Communities: []int{7}, CommunityDataAvailable: true, OverlayEntityCount: 1}, + {Ref: "pr-b", Communities: []int{7}, CommunityDataAvailable: true, OverlayEntityCount: 1}, + }) + if measured.CommunityDataInferredOnly { + t.Errorf("both refs were measured; CommunityDataInferredOnly must be false") + } + if measured.InferredEntityCount != 0 { + t.Errorf("InferredEntityCount = %d, want 0", measured.InferredEntityCount) + } + + // Partly measured is not "inferred only" — but the inferred count still shows. + mixed := AnalyzeMergeRisk([]ChangeImpact{ + {Ref: "pr-a", Communities: []int{7}, CommunityDataAvailable: true, OverlayEntityCount: 2}, + {Ref: "pr-b", Communities: []int{7}, CommunityDataAvailable: true, InferredEntityCount: 1}, + }) + if mixed.CommunityDataInferredOnly { + t.Errorf("pr-a contributed measured entities; CommunityDataInferredOnly must be false") + } + if mixed.InferredEntityCount != 1 { + t.Errorf("InferredEntityCount = %d, want 1", mixed.InferredEntityCount) + } +} + +// An UNAVAILABLE verdict is not an inferred one. #6006's decline must not start +// wearing #6042's confidence marker. +func TestAnalyzeMergeRisk_UnavailableIsNotInferredOnly(t *testing.T) { + // pr-a inferred one entity; pr-b could not be placed at all. The verdict is a + // DECLINE, and a decline must not wear the low-confidence marker — that would + // tell a caller a verdict exists when none does. + res := AnalyzeMergeRisk([]ChangeImpact{ + {Ref: "pr-a", Communities: []int{7}, CommunityDataAvailable: true, InferredEntityCount: 1}, + {Ref: "pr-b", CommunityDataAvailable: false}, + }) + if res.CommunityDataAvailable { + t.Fatalf("precondition: one uncovered ref must make the whole result unavailable") + } + if res.CommunityDataInferredOnly { + t.Errorf("this is a DECLINE, not a low-confidence answer; CommunityDataInferredOnly must be false") + } + + // And with nothing inferred anywhere either. + none := AnalyzeMergeRisk([]ChangeImpact{ + {Ref: "pr-a", CommunityDataAvailable: false}, + {Ref: "pr-b", CommunityDataAvailable: false}, + }) + if none.CommunityDataInferredOnly || none.InferredEntityCount != 0 { + t.Errorf("nothing was inferred; got inferred_only=%v count=%d", + none.CommunityDataInferredOnly, none.InferredEntityCount) + } +} + +// Cost guard: inference is a bounded add-on, not a graph walk. It touches only +// the changed entities' own files/modules/outbound edges, so a large graph with +// a small change set must not pay for it. +func BenchmarkAnalyzePRImpact_Inference(b *testing.B) { + const n = 20000 + ents := make([]Entity, 0, n+1) + rels := make([]Relationship, 0, n) + for i := 0; i < n; i++ { + f := "pkg" + string(rune('a'+i%20)) + "/f.go" + ents = append(ents, placedEnt("e"+itoaBench(i), f, "pkg"+string(rune('a'+i%20)), i%50)) + if i > 0 { + rels = append(rels, Relationship{FromID: "e" + itoaBench(i), ToID: "e" + itoaBench(i-1), Kind: "CALLS"}) + } + } + ents = append(ents, newEnt("new:x", "pkga/f.go", "pkga")) + change := ChangeSet{Added: []DiffEntityEntry{{ID: "new:x"}}} + b.ResetTimer() + for i := 0; i < b.N; i++ { + _ = AnalyzePRImpact(ents, rels, change, DefaultPRImpactOptions()) + } +} + +func itoaBench(i int) string { + if i == 0 { + return "0" + } + var buf [12]byte + p := len(buf) + for i > 0 { + p-- + buf[p] = byte('0' + i%10) + i /= 10 + } + return string(buf[p:]) +} diff --git a/internal/graph/pr_impact_infer.go b/internal/graph/pr_impact_infer.go new file mode 100644 index 000000000..8bc8d54f7 --- /dev/null +++ b/internal/graph/pr_impact_infer.go @@ -0,0 +1,302 @@ +// pr_impact_infer.go — issue #6042: infer a community for a changed entity the +// group-algo overlay cannot place, from the entities around it that it CAN. +// +// WHY THIS EXISTS. The overlay is computed from the last indexed group union, so +// an entity that a PR ADDS is absent from it by construction. #6006 made that +// state an explicit decline rather than a silent "no conflicts" — correct, but +// it turns the single most ordinary PR shape there is (one that only adds code) +// into a tool that always refuses to answer. +// +// WHAT IT MUST NOT BECOME. Inference presented as fact is the #6006 defect one +// layer up: a confident answer the caller cannot tell from a measured one. So +// every inferred placement is labelled at the entity level +// (ChangedEntity.CommunitySource) and aggregated at the verdict level +// (CommunityDataInferredOnly / InferredEntityCount), and inference that fails +// falls back to the existing decline path unchanged. +// +// ── The signals, and why these three ──────────────────────────────────────── +// +// Only signals the graph actually carries at this layer were used: +// +// container (weight 3) — the entities sharing the new entity's SourceFile, plus +// any CONTAINS parent. A new function in an already-placed file is in that +// file's community with very high confidence; a file is the smallest unit +// Louvain would essentially never split. +// module (weight 2) — Properties["module"], stamped on both the full +// (cmd/grafel/index.go) and incremental (internal/extractors/incremental.go) +// paths and round-tripped through graph.fb (load.go restores the FB scalar +// into props). It is a depth-capped path rollup, so it is a COARSER version +// of the container signal rather than an independent one — hence a lower +// weight, and hence it can never outvote the file on its own. +// targets (weight 1) — placed entities the new entity calls/references. This is +// closest to what Louvain itself would have seen. It requires at least +// minPlacedTargets placed targets: one outbound edge is not evidence +// (everything calls a logger), and a single-edge rule would place half of +// every new package into whatever community the nearest shared utility +// happens to sit in. +// +// ── The decision rule ─────────────────────────────────────────────────────── +// +// Each signal casts ONE vote, for the strict plurality of its own evidence; a +// signal with no unique winner (a file split 1-1 across communities) abstains +// entirely rather than letting map order decide. A community is then inferred +// only when its weight is STRICTLY GREATER than the total weight of every +// dissenting vote. So: +// +// container alone -> infer (3 > 0) +// container 7 vs module 9 -> infer 7 (3 > 2; container is weighted highest) +// container 7 vs module 9 + targets 9 -> DECLINE (3 is not > 3) +// module alone / targets alone -> infer (2 > 0 / 1 > 0) +// nothing available -> DECLINE +// +// ── What is deliberately NOT inferred ─────────────────────────────────────── +// +// - Overlay-placed entities. The overlay is ground truth and always wins. +// - Removed entities. They are absent from the head graph, so they have no +// neighbours; the diff record is not a community signal. +// - Blast-radius entities. Only the CHANGED set drives the verdict, and +// inferring for a potentially unbounded downstream set would cost far more +// than it is worth. +// - Chained inference. Only OVERLAY-placed entities vote. An inferred +// placement is a guess, and letting guesses vote would propagate one weak +// signal across an entire new package — and would make the result depend on +// the order entities were processed in. +// +// ── Cost ──────────────────────────────────────────────────────────────────── +// +// Nothing is built unless at least one changed entity is present in the head +// graph AND unplaced. When that holds, the added work is: +// +// one pass over `entities`, filtered to the (small) set of files and modules +// the unplaced changed entities live in — O(N) comparisons, allocations bounded +// by the changed set, not by the graph; and +// outbound/CONTAINS edges captured during the adjacency pass AnalyzePRImpact +// already makes, and only for changed ids — O(E) comparisons, O(deg(changed)) +// memory. +// +// No BFS, no transitive walk. Next to the 31.9 MB overlay parse the handler +// already performs (memoized on path+mtime+size), this is noise. +package graph + +// CommunitySource labels HOW a changed entity's community was determined. +// It is the whole point of #6042: an inferred placement that a caller cannot +// distinguish from a measured one is worse than no placement at all. +const ( + // CommunitySourceOverlay — measured. The group-algo overlay placed this + // entity directly; it existed at the last group index. + CommunitySourceOverlay = "overlay" + // CommunitySourceInferred — a guess, from placed neighbours. Good enough to + // triage merge risk with, not good enough to present as measured. + CommunitySourceInferred = "inferred" + // CommunitySourceNone — not placed and not inferrable. These entities are the + // ones #6006's decline path exists for. + CommunitySourceNone = "none" +) + +const ( + inferWeightContainer = 3 + inferWeightModule = 2 + inferWeightTargets = 1 + + // minPlacedTargets is how many placed outbound targets the weakest signal + // needs before it votes at all. See the package comment: one edge is not + // evidence, and this threshold is also what keeps #6006's add-only decline + // (a new entity calling exactly one placed entity) binding. + minPlacedTargets = 2 +) + +// communityInferrer holds the (small) indexes needed to place unplaced changed +// entities. Zero value infers nothing, which is what callers get when every +// changed entity was already placed. +type communityInferrer struct { + byID map[string]Entity + // byFile/byModule are community histograms over OVERLAY-PLACED entities, + // restricted to the files/modules the unplaced changed entities occupy. + byFile map[string]map[int]int + byModule map[string]map[int]int + // parents[id] = CONTAINS parents of id; targets[id] = outbound edge targets of + // id. Populated only for unplaced changed ids. + parents map[string][]string + targets map[string][]string +} + +// newCommunityInferrer builds the indexes for `want` — the unplaced changed +// entity ids that are present in the head graph. Returns the zero inferrer when +// there is nothing to infer, so the O(N) entity pass is skipped entirely. +func newCommunityInferrer(entities []Entity, byID map[string]Entity, want map[string]struct{}, + parents, targets map[string][]string) *communityInferrer { + if len(want) == 0 { + return nil + } + // Which files/modules do we actually care about? Usually a handful. + wantFiles := make(map[string]struct{}, len(want)) + wantModules := make(map[string]struct{}, len(want)) + for id := range want { + e := byID[id] + if e.SourceFile != "" { + wantFiles[e.SourceFile] = struct{}{} + } + if m := e.PropGet("module"); m != "" { + wantModules[m] = struct{}{} + } + } + // Also index the files/modules of the CONTAINS parents, so a parent that + // lives elsewhere still contributes through its own community (below we read + // the parent's community directly, so no extra file bookkeeping is needed). + ci := &communityInferrer{ + byID: byID, + byFile: make(map[string]map[int]int, len(wantFiles)), + byModule: make(map[string]map[int]int, len(wantModules)), + parents: parents, + targets: targets, + } + if len(wantFiles) == 0 && len(wantModules) == 0 && len(parents) == 0 && len(targets) == 0 { + return ci // nothing to histogram; target/parent votes still work + } + for i := range entities { + c := communityOf(entities[i]) + if c < 0 { + continue // only OVERLAY-placed entities vote — no chained inference + } + if f := entities[i].SourceFile; f != "" { + if _, ok := wantFiles[f]; ok { + addVote(ci.byFile, f, c) + } + } + if len(wantModules) > 0 { + if m := entities[i].PropGet("module"); m != "" { + if _, ok := wantModules[m]; ok { + addVote(ci.byModule, m, c) + } + } + } + } + return ci +} + +func addVote(dst map[string]map[int]int, key string, community int) { + h := dst[key] + if h == nil { + h = map[int]int{} + dst[key] = h + } + h[community]++ +} + +// infer returns the community inferred for id, and whether inference succeeded. +// See the package comment for the rule; the short version is that each signal +// casts one weighted vote and the winner must strictly outweigh all dissent. +func (ci *communityInferrer) infer(id string) (int, bool) { + if ci == nil { + return -1, false + } + e, ok := ci.byID[id] + if !ok { + return -1, false // removed entity: no head-graph neighbours to read + } + + votes := map[int]int{} + if c, ok := ci.containerVote(id, e); ok { + votes[c] += inferWeightContainer + } + if m := e.PropGet("module"); m != "" { + if c, ok := plurality(ci.byModule[m], 1); ok { + votes[c] += inferWeightModule + } + } + if c, ok := ci.targetVote(id); ok { + votes[c] += inferWeightTargets + } + if len(votes) == 0 { + return -1, false + } + + best, bestW, total := -1, 0, 0 + tied := false + for c, w := range votes { + total += w + switch { + case w > bestW: + best, bestW, tied = c, w, false + case w == bestW: + tied = true + } + } + // Strictly greater than the sum of all dissent: 2*bestW > total. + if tied || 2*bestW <= total { + return -1, false + } + return best, true +} + +// containerVote combines the entity's own file with any CONTAINS parents — the +// "containing component" signal. Both describe the same thing (what physically +// encloses the entity) so they share one vote rather than double-counting. +func (ci *communityInferrer) containerVote(id string, e Entity) (int, bool) { + var hist map[int]int + if base := ci.byFile[e.SourceFile]; e.SourceFile != "" && len(base) > 0 { + hist = make(map[int]int, len(base)+1) + for c, n := range base { + hist[c] = n + } + } + for _, pid := range ci.parents[id] { + p, ok := ci.byID[pid] + if !ok { + continue + } + if c := communityOf(p); c >= 0 { + if hist == nil { + hist = map[int]int{} + } + hist[c]++ + } + } + return plurality(hist, 1) +} + +// targetVote is the plurality community of the entity's placed outbound targets, +// requiring at least minPlacedTargets of them. +func (ci *communityInferrer) targetVote(id string) (int, bool) { + tgts := ci.targets[id] + if len(tgts) < minPlacedTargets { + return -1, false + } + hist := map[int]int{} + placed := 0 + for _, tid := range tgts { + t, ok := ci.byID[tid] + if !ok { + continue + } + if c := communityOf(t); c >= 0 { + hist[c]++ + placed++ + } + } + if placed < minPlacedTargets { + return -1, false + } + return plurality(hist, minPlacedTargets) +} + +// plurality returns the uniquely most-common community in hist, provided it has +// at least `min` votes. A tie ABSTAINS: with no unique winner the answer would +// otherwise be decided by map iteration order, which is both non-deterministic +// and, worse, an invented placement. +func plurality(hist map[int]int, min int) (int, bool) { + best, bestN := -1, 0 + tied := false + for c, n := range hist { + switch { + case n > bestN: + best, bestN, tied = c, n, false + case n == bestN: + tied = true + } + } + if best < 0 || bestN < min || tied { + return -1, false + } + return best, true +} diff --git a/internal/mcp/pr_impact_6042_test.go b/internal/mcp/pr_impact_6042_test.go new file mode 100644 index 000000000..8ab454499 --- /dev/null +++ b/internal/mcp/pr_impact_6042_test.go @@ -0,0 +1,373 @@ +// pr_impact_6042_test.go — issue #6042: an add-only PR must get a merge-risk +// verdict instead of a decline, and the payload must make an INFERRED verdict +// impossible to mistake for a measured one. +// +// THE OVERLAY IN THESE FIXTURES IS PRODUCTION-SHAPED. It covers exactly the +// entities that existed at the last group index (svc:A, svc:B, svc:C) and +// nothing else. Entities that live only on a feature ref are absent from it by +// construction — that is the entire premise of this issue, and a fixture that +// stamped them in would certify a behaviour the code does not have. +package mcp + +import ( + "encoding/json" + "os" + "path/filepath" + "strings" + "testing" + "time" + + "github.com/cajasmota/grafel/internal/daemon" + "github.com/cajasmota/grafel/internal/graph" + "github.com/cajasmota/grafel/internal/graph/fbwriter" + "github.com/cajasmota/grafel/internal/graph/groupalgo" + "github.com/cajasmota/grafel/internal/registry" + "github.com/cajasmota/grafel/internal/testsupport" + mcpapi "github.com/mark3labs/mcp-go/mcp" +) + +type prImpact6042Env struct { + srv *Server + overlayPath string + curMtimes map[string]int64 +} + +func prImpact6042Ent(id, file, module string) graph.Entity { + e := graph.Entity{ID: id, Name: id, Kind: "function", SourceFile: file, Language: "go"} + if module != "" { + e = e.WithProperties(map[string]string{"module": module}) + } + return e +} + +// setupPRImpact6042 writes the ref graphs for one repo. The indexed group union +// (what the overlay knows about) is main: +// +// svc:A, svc:B — core/*.go, module "core", community 7 +// svc:C — util/c.go, module "util", community 9 +// +// Every other ref ADDS exactly one entity that the overlay cannot possibly know +// about, differing only in which inference signal is available to it. +func setupPRImpact6042(t *testing.T) prImpact6042Env { + t.Helper() + testsupport.IsolateHome(t) + root := t.TempDir() + t.Setenv("GRAFEL_HOME", filepath.Join(root, "home")) + t.Setenv("GRAFEL_DAEMON_ROOT", filepath.Join(root, "daemon")) + + repoPath := filepath.Join(root, "svc") + if err := os.MkdirAll(repoPath, 0o755); err != nil { + t.Fatalf("mkdir repo: %v", err) + } + + base := func() []graph.Entity { + return []graph.Entity{ + prImpact6042Ent("svc:A", "core/a.go", "core"), + prImpact6042Ent("svc:B", "core/b.go", "core"), + prImpact6042Ent("svc:C", "util/c.go", "util"), + } + } + // mk builds a ref graph: the base three entities plus zero or one added + // entity, with the given outbound CALLS edges. + mk := func(added graph.Entity, calls ...string) *graph.Document { + d := &graph.Document{Version: 1, Repo: "svc", Entities: base()} + if added.ID != "" { + d.Entities = append(d.Entities, added) + for _, target := range calls { + d.Relationships = append(d.Relationships, + graph.Relationship{FromID: added.ID, ToID: target, Kind: "CALLS"}) + } + } + return d + } + // modA perturbs svc:A's signature so DiffDocs classifies it as MODIFIED — + // the measured control arm. + modA := func() *graph.Document { + d := mk(graph.Entity{}) + d.Entities[0].Signature = "func A(x int)" + return d + } + + refs := map[string]*graph.Document{ + "main": mk(graph.Entity{}), + // Signal 1: the added entity lives in an ALREADY-PLACED file. + "inFile": mk(prImpact6042Ent("svc:NewInA", "core/a.go", "core")), + // Signal 3: brand-new file, brand-new module, but it calls two placed + // entities that agree on community 7. + "viaTargets": mk(prImpact6042Ent("svc:NewT", "newpkg/t.go", "newpkg"), "svc:A", "svc:B"), + // Nothing to infer from: new file, new module, no edges. + "isolated": mk(prImpact6042Ent("svc:NewI", "lonely/i.go", "lonely")), + // Measured control arm. + "modA": modA(), + } + for ref, doc := range refs { + dir := daemon.StateDirForRepoRef(repoPath, ref) + if err := fbwriter.WriteAtomic(filepath.Join(dir, "graph.fb"), doc); err != nil { + t.Fatalf("write graph.fb for %s: %v", ref, err) + } + } + if err := fbwriter.WriteAtomic( + filepath.Join(daemon.StateDirForRepo(repoPath), "graph.fb"), mk(graph.Entity{})); err != nil { + t.Fatalf("write HEAD graph.fb: %v", err) + } + + cfgPath, err := registry.ConfigPathFor("acme") + if err != nil { + t.Fatalf("config path: %v", err) + } + cfg := ®istry.GroupConfig{Name: "acme", Repos: []registry.Repo{{Slug: "svc", Path: repoPath}}} + if err := registry.SaveGroupConfig(cfgPath, cfg); err != nil { + t.Fatalf("save group config: %v", err) + } + if err := registry.AddGroup("acme", cfgPath); err != nil { + t.Fatalf("add group: %v", err) + } + overlayPath, err := groupalgo.OverlayPath("acme") + if err != nil { + t.Fatalf("overlay path: %v", err) + } + cur, err := groupalgo.CurrentSourceMtimes("acme") + if err != nil { + t.Fatalf("current mtimes: %v", err) + } + env := prImpact6042Env{ + srv: newTestServer(t, &graph.Document{Repo: "svc"}), + overlayPath: overlayPath, + curMtimes: cur, + } + env.writeOverlay(t) + return env +} + +// writeOverlay covers ONLY the last-indexed group union. svc:NewInA, svc:NewT +// and svc:NewI are deliberately absent — production cannot place them, and this +// issue is entirely about that state. +func (e prImpact6042Env) writeOverlay(t *testing.T) { + t.Helper() + ov := &groupalgo.Overlay{ + AlgoVersion: groupalgo.OverlayAlgoVersion, + Group: "acme", + ComputedAt: time.Now().UTC().Add(-time.Hour), + SourceMtimes: e.curMtimes, + Results: map[string]groupalgo.EntityOverlay{ + "svc:A": {CommunityID: 7, PageRank: 0.1}, + "svc:B": {CommunityID: 7, PageRank: 0.1}, + "svc:C": {CommunityID: 9, PageRank: 0.1}, + }, + Communities: []graph.CommunityResult{ + {ID: 7, Size: 2, AutoName: "core"}, + {ID: 9, Size: 1, AutoName: "util"}, + }, + } + if err := groupalgo.WriteOverlayTo(e.overlayPath, ov); err != nil { + t.Fatalf("write overlay: %v", err) + } +} + +func (e prImpact6042Env) conflicts(t *testing.T, refs ...string) *mcpapi.CallToolResult { + t.Helper() + rs := make([]any, len(refs)) + for i, r := range refs { + rs[i] = r + } + return callHandlerResult(t, e.srv.handlePRImpact, map[string]any{ + "group": "acme", "repo": "svc", "base": "main", "refs": rs, + }) +} + +func (e prImpact6042Env) single(t *testing.T, head string) *mcpapi.CallToolResult { + t.Helper() + return callHandlerResult(t, e.srv.handlePRImpact, map[string]any{ + "group": "acme", "repo": "svc", "base": "main", "head": head, + }) +} + +type prImpact6042Payload struct { + RiskyPairCount int `json:"risky_pair_count"` + CommunityDataAvailable bool `json:"community_data_available"` + CommunityDataInferredOnly bool `json:"community_data_inferred_only"` + InferredEntityCount int `json:"inferred_entity_count"` + ChangedCount int `json:"changed_count"` + ChangedUncovered int `json:"changed_entities_without_community"` + ChangedOverlay int `json:"changed_entities_with_overlay_community"` + ChangedInferred int `json:"changed_entities_with_inferred_community"` + RiskPairs []struct { + SharedCommunities []int `json:"shared_communities"` + } `json:"risk_pairs"` + PerRef []struct { + Ref string `json:"ref"` + ChangedOverlay int `json:"changed_entities_with_overlay_community"` + ChangedInferred int `json:"changed_entities_with_inferred_community"` + Uncovered int `json:"changed_entities_without_community"` + } `json:"per_ref"` + ChangedEntities []struct { + ID string `json:"id"` + CommunityID int `json:"community_id"` + CommunitySource string `json:"community_source"` + } `json:"changed_entities"` +} + +func must6042Payload(t *testing.T, res *mcpapi.CallToolResult) prImpact6042Payload { + t.Helper() + if res == nil || res.IsError { + t.Fatalf("expected a successful result, got: %s", resultText(res)) + } + var p prImpact6042Payload + if err := json.Unmarshal([]byte(resultText(res)), &p); err != nil { + t.Fatalf("unmarshal payload: %v\n%s", err, resultText(res)) + } + return p +} + +// THE #6042 test. Two refs that only ADD entities — the shape #6006 declined — +// now produce a verdict, because both added entities can be inferred into +// community 7 from their placed neighbours. The verdict must be flagged as +// resting entirely on inference. +func TestPRImpact6042_AddOnlyRefsGetAnInferredVerdict(t *testing.T) { + env := setupPRImpact6042(t) + + res := env.conflicts(t, "inFile", "viaTargets") + p := must6042Payload(t, res) + + if p.RiskyPairCount != 1 || len(p.RiskPairs) != 1 || + len(p.RiskPairs[0].SharedCommunities) != 1 || p.RiskPairs[0].SharedCommunities[0] != 7 { + t.Fatalf("both refs infer into community 7; want 1 risky pair sharing [7], got %+v", p) + } + if !p.CommunityDataAvailable { + t.Errorf("an inferred placement is still a placement; community_data_available = false") + } + if !p.CommunityDataInferredOnly { + t.Errorf("NOTHING in this verdict was measured; community_data_inferred_only = false — " + + "the caller cannot tell this from a verdict the group partition actually produced") + } + if p.InferredEntityCount != 2 { + t.Errorf("inferred_entity_count = %d, want 2", p.InferredEntityCount) + } + if len(p.PerRef) != 2 { + t.Fatalf("want 2 per-ref entries, got %+v", p.PerRef) + } + for _, r := range p.PerRef { + if r.ChangedInferred != 1 || r.ChangedOverlay != 0 || r.Uncovered != 0 { + t.Errorf("per_ref %s = overlay %d / inferred %d / none %d, want 0/1/0", + r.Ref, r.ChangedOverlay, r.ChangedInferred, r.Uncovered) + } + } +} + +// The control arm that proves the marker discriminates: a ref that MODIFIES an +// overlay-covered entity is measured, and must NOT be flagged as inferred. +func TestPRImpact6042_MeasuredVerdictIsNotFlaggedAsInferred(t *testing.T) { + env := setupPRImpact6042(t) + + p := must6042Payload(t, env.conflicts(t, "modA", "inFile")) + if !p.CommunityDataAvailable { + t.Fatalf("modA touches an overlay-covered entity; community data must be available") + } + if p.CommunityDataInferredOnly { + t.Errorf("modA's placement was MEASURED; community_data_inferred_only must be false") + } + if p.InferredEntityCount != 1 { + t.Errorf("inferred_entity_count = %d, want 1 (inFile's added entity)", p.InferredEntityCount) + } +} + +// #6006's decline path must survive. A ref whose added entity has no placed +// neighbours at all cannot be inferred, and one uninferrable ref still taints +// the whole merge-risk verdict. +func TestPRImpact6042_UninferrableRefStillDeclines(t *testing.T) { + env := setupPRImpact6042(t) + + res := env.conflicts(t, "isolated", "inFile") + if res == nil { + t.Fatal("nil result") + } + text := resultText(res) + if !res.IsError { + t.Fatalf("the isolated ref cannot be placed or inferred, so merge risk was not computed; "+ + "must not return a payload: %s", text) + } + if !strings.Contains(text, "isolated") { + t.Errorf("error must name the ref that could not be placed, got: %s", text) + } + // The explanation must say inference was attempted — otherwise a caller reads + // this as "grafel never tried" and reindexes for nothing. + if !strings.Contains(strings.ToLower(text), "infer") { + t.Errorf("error must say that inference from placed neighbours was attempted and failed, got: %s", text) + } + var payload map[string]any + if json.Unmarshal([]byte(text), &payload) == nil { + if v, ok := payload["risky_pair_count"]; ok { + t.Errorf("uncomputed merge risk still emitted risky_pair_count=%v", v) + } + } +} + +// Single mode must label EVERY changed entity with how it was placed, and carry +// the same verdict-level marker. +func TestPRImpact6042_SingleModeLabelsInferencePerEntity(t *testing.T) { + env := setupPRImpact6042(t) + + // Inferred: the added entity sits in an already-placed file. + p := must6042Payload(t, env.single(t, "inFile")) + if p.ChangedCount != 1 { + t.Fatalf("fixture must change exactly one entity, got %+v", p.ChangedEntities) + } + got := p.ChangedEntities[0] + if got.ID != "svc:NewInA" || got.CommunityID != 7 || got.CommunitySource != "inferred" { + t.Errorf("changed entity = %+v, want svc:NewInA / community 7 / source inferred", got) + } + if !p.CommunityDataAvailable || !p.CommunityDataInferredOnly { + t.Errorf("want available=true inferred_only=true, got %v/%v", + p.CommunityDataAvailable, p.CommunityDataInferredOnly) + } + if p.ChangedInferred != 1 || p.ChangedOverlay != 0 || p.ChangedUncovered != 0 { + t.Errorf("counts = overlay %d / inferred %d / none %d, want 0/1/0", + p.ChangedOverlay, p.ChangedInferred, p.ChangedUncovered) + } + + // Measured: the modified entity is in the overlay. + m := must6042Payload(t, env.single(t, "modA")) + if len(m.ChangedEntities) == 0 || m.ChangedEntities[0].CommunitySource != "overlay" { + t.Errorf("modA's changed entity should be sourced from the overlay, got %+v", m.ChangedEntities) + } + if m.CommunityDataInferredOnly { + t.Errorf("modA is measured; community_data_inferred_only must be false") + } + + // Neither: nothing to infer from. Single mode still returns a payload (the + // blast radius is valid) but must not claim any placement. + i := must6042Payload(t, env.single(t, "isolated")) + if len(i.ChangedEntities) == 0 || i.ChangedEntities[0].CommunitySource != "none" { + t.Errorf("the isolated added entity has no placed neighbours; want source none, got %+v", + i.ChangedEntities) + } + if i.CommunityDataAvailable || i.CommunityDataInferredOnly { + t.Errorf("nothing was placed or inferred; want available=false inferred_only=false, got %v/%v", + i.CommunityDataAvailable, i.CommunityDataInferredOnly) + } + if i.ChangedUncovered != 1 { + t.Errorf("changed_entities_without_community = %d, want 1", i.ChangedUncovered) + } +} + +// Inference reads the graph the handler already loaded and the overlay it +// already parsed — it must not add an overlay read per call. +func TestPRImpact6042_InferenceDoesNotReparseTheOverlay(t *testing.T) { + env := setupPRImpact6042(t) + + _ = env.conflicts(t, "inFile", "viaTargets") // prime + overlayCacheMu.Lock() + before := overlayCacheHits + overlayCacheMu.Unlock() + + for i := 0; i < 2; i++ { + _ = must6042Payload(t, env.conflicts(t, "inFile", "viaTargets")) + } + + overlayCacheMu.Lock() + got := overlayCacheHits - before + overlayCacheMu.Unlock() + if got != 2 { + t.Errorf("expected 2 overlay cache hits across 2 repeat calls, got %d", got) + } +} diff --git a/internal/mcp/pr_impact_tools.go b/internal/mcp/pr_impact_tools.go index 03d62ad25..fe5b952ba 100644 --- a/internal/mcp/pr_impact_tools.go +++ b/internal/mcp/pr_impact_tools.go @@ -109,6 +109,16 @@ func (s *Server) handlePRImpact(_ context.Context, req mcpapi.CallToolRequest) ( // impacted_communities is indistinguishable from "nothing was computed". "community_data_available": res.CommunityDataAvailable, "changed_entities_without_community": res.ChangedWithoutCommunity, + // #6042: each changed_entities row carries community_source + // (overlay|inferred|none); these are the aggregates. An inferred placement + // is a deduction from placed neighbours, not a reading of the group + // partition, and a caller must be able to tell the two apart. + "changed_entities_with_overlay_community": res.ChangedWithOverlayCommunity, + "changed_entities_with_inferred_community": res.ChangedWithInferredCommunity, + "community_data_inferred_only": res.CommunityDataInferredOnly, + } + if res.CommunityDataInferredOnly { + out["community_data_note"] = inferredOnlyNote } stamper.describeInto(out) if !res.CommunityDataAvailable { @@ -186,13 +196,25 @@ func (s groupCommunityStamper) unavailableCause(group string) string { "(%s-algo.json is absent, corrupt, or was produced by a different algorithm "+ "version) — run or await a group index so community detection produces it", group, group) } - return "the group-algo overlay exists but does not cover the changed entities. " + - "The overlay is computed from the INDEXED group union, so entities that exist only " + - "on a feature ref are absent from it by construction — this is the expected shape for " + - "a change that only ADDS entities. Reindex the group with those refs' code present, " + + return "the group-algo overlay exists but does not cover the changed entities, and no " + + "community could be INFERRED for them either (#6042: grafel tries the containing file, " + + "the module, and the placed entities they call). The overlay is computed from the " + + "INDEXED group union, so entities that exist only on a feature ref are absent from it by " + + "construction — this is the expected shape for a change that only ADDS entities. " + + "Inference then failed because those entities have no overlay-placed neighbours at all, " + + "or because their signals disagreed. Reindex the group with those refs' code present, " + "or fall back to single mode and triage by blast radius" } +// inferredOnlyNote is attached to any payload whose every placement was +// inferred (#6042). The structured flags are the contract; this is the sentence +// that stops an agent reading the verdict as a measurement. +const inferredOnlyNote = "every community placement behind this verdict was INFERRED from " + + "placed neighbours (containing file / module / outbound call targets), not read from the " + + "group-algo partition — the changed entities are new, so the partition has never seen them. " + + "Treat this as a well-founded estimate of what community detection would say, not as a " + + "measurement. Per-entity provenance is in changed_entities[].community_source." + // ── overlay read cache (one entry) ─────────────────────────────────────────── // // The overlay is read and unmarshalled on every grafel_pr_impact call, in both @@ -298,6 +320,8 @@ func (s *Server) prImpactConflicts(groupName, repoSlug, repoPath, base string, r Ref: ref, Communities: comms, CommunityDataAvailable: res.CommunityDataAvailable, + OverlayEntityCount: res.ChangedWithOverlayCommunity, + InferredEntityCount: res.ChangedWithInferredCommunity, }) perRef = append(perRef, map[string]any{ "ref": ref, @@ -308,6 +332,10 @@ func (s *Server) prImpactConflicts(groupName, repoSlug, repoPath, base string, r // that (say) 3 of 4 changed entities were placed even when the overall // verdict stands. "changed_entities_without_community": res.ChangedWithoutCommunity, + // #6042: and how much of this ref's placement was measured vs deduced. + "changed_entities_with_overlay_community": res.ChangedWithOverlayCommunity, + "changed_entities_with_inferred_community": res.ChangedWithInferredCommunity, + "community_data_inferred_only": res.CommunityDataInferredOnly, }) } @@ -335,6 +363,18 @@ func (s *Server) prImpactConflicts(groupName, repoSlug, repoPath, base string, r "ref_count": risk.RefCount, "risky_pair_count": risk.RiskyPairs, "community_data_available": risk.CommunityDataAvailable, + // #6042 — the verdict-level confidence marker. `inferred_only` means the + // pairs above are grafel's best reconstruction of what the group partition + // WOULD have said about entities it has never seen, not a reading of what + // it did say. Zero risky pairs under that flag is a weaker all-clear. + "inferred_entity_count": risk.InferredEntityCount, + "community_data_inferred_only": risk.CommunityDataInferredOnly, + } + if len(risk.RefsWithInferredCommunityData) > 0 { + out["refs_with_inferred_community_data"] = risk.RefsWithInferredCommunityData + } + if risk.CommunityDataInferredOnly { + out["community_data_note"] = inferredOnlyNote } stamper.describeInto(out) return jsonResult(out), nil From 54d7cf5d086019060088c7585dd3b6571e8f637d Mon Sep 17 00:00:00 2001 From: Jorge Cajas Date: Thu, 30 Jul 2026 02:36:48 +0800 Subject: [PATCH 2/2] fix(#6042): per-pair inference provenance, overlay-absence candidacy, unanimity rule MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses an adversarial review of the previous commit. Three findings reproduced the exact failure mode this issue exists to avoid; two more were guards held by nothing. ## D1 — a conflict could be 100% manufactured while inferred_only was false CommunityDataInferredOnly is a WHOLE-VERDICT flag and says nothing about an individual overlap. Two refs can each carry measured communities — so every aggregate reads "measured" — and still share ONLY a community that both sides reached by inference. The reported conflict is then entirely deduced and nothing in the payload said so. Conflicts mode makes it worse: it emits no changed_entities at all, so per-entity community_source was single-mode only. Now: PRImpactResult.InferredOnlyCommunityIDs (a community is inference-only when no overlay-placed changed entity AND no blast-radius entity puts it there — the blast radius is real edges to really-placed entities, so it counts as measured); ChangeImpact.InferredOnlyCommunities; MergeRiskPair.InferredSharedCommunities + InferredOnly; MergeRiskResult.InferredOnlyPairCount; per_ref.inferred_communities in conflicts mode; and a distinct prose note that fires on a deduced PAIR even when the verdict-level flag is false. One inferred side is enough to mark the overlap — the claim "these refs collide here" was never observed on that side. ## D2 — inference was overruling the group algorithm's own decision Candidacy keyed on communityOf(e) < 0, which is true both for "absent from the overlay" and for a NON-NIL -2/-1. Those entities were IN the last group index and community detection DECLINED to place them. #6042 is about entities the partition has never seen. Candidacy is now Entity.CommunityID == nil (overlayAbsent). #6006's TestAnalyzePRImpact_NegativeCommunityIDIsNotCoverage passed only because its fixture was a lone entity with no file siblings, no module and no edges — a shape production essentially never produces, so post-#6042 it certified nothing. Re-shaped: the entity now sits in a placed file, in a concentrated module, and calls two placed entities, so every signal is available and must still be refused. ## D3 + D5 — the dissent guard was unpinned, and module double-counted The 3/2/1 weighting required the winner to strictly outweigh all dissent, but with module subordinate to the container that comparison could never block anything: mutating it away passed both full suites. Rather than pin dead arithmetic, the rule is now the issue's own — UNANIMITY. At most two votes exist (the container-or-module primary, and the call targets) and both must agree. Module is now a strict FALLBACK, not a co-voter: module is module.Derive( SourceFile), a pure function of the path, so the module histogram is a superset of the file histogram and letting both vote counted one measurement twice. It also now requires a real sample (>= 3 placed) and real concentration (>= 0.7), because module.Derive has no per-Go-package marker: a single-module Go repo falls through to DefaultDepth and everything under internal/graph/** shares one label, where a 26-of-51 plurality is noise wearing a community id. A container that abstains from CONTRADICTION (a file split 1-1) does NOT fall through to the module — the module is a superset of that same contradiction and would resolve it only by dilution. Only an absent container falls through. Every inferred placement now carries its margin (CommunityInference: signals, support, sample) so 26/51 and 40/41 are distinguishable on the wire. ## D4 — the target signal accepted every edge kind It appended any outbound edge, so IMPORTS and CONTAINS drove inference. The "everything calls a logger" rationale for minPlacedTargets=2 buys nothing under IMPORTS, where importing two placed packages is universal; CONTAINS is the container signal wearing a second name. Now allowlisted to genuine call/reference kinds. A placed CONTAINS PARENT remains legitimate container evidence. ## D6 — the benchmark never inferred Its 20k placed entities cycled over 5 communities in EQUAL counts, so both histograms tied, both signals abstained, and it timed the decline path. Fixture rebuilt (one community per module, plus two call targets) and it now ASSERTS inference fires before timing. Re-measured, 20k entities / 20k edges, 1-entity add-only change: 3.63 ms -> 4.87 ms (+1.24 ms, +34%), +859 B, +13 allocs. The earlier "+0.61 ms" measured the histogram build only and is withdrawn. Against the handler's memoized 326 ms / 76 MB overlay parse this is ~0.4% of a cold call. ## D7/D8 — shipped deliverables with no coverage community_data_note and refs_with_inferred_community_data could both be deleted with both suites green; conflicts mode exposed no per-entity provenance. All now covered, including a measured control arm for every marker. ## Verification Mutation battery REBUILT FROM SCRATCH against the final tree: 34 mutations, all 34 killed by real test failures. One (M16) initially survived and one earlier "kill" turned out to be a build failure rather than a test failure — both fixed and re-run. Covers: inference disabled; unanimity guard removed; ties by map order; minPlacedTargets=1 (which fails #6006's AvailabilityFollowsChangedSetNotEntitySet, confirming it still binds); label collapsed to "overlay"; margin dropped; chaining; overlay overridden; removed entities inferred; candidacy on communityOf<0; IMPORTS/CONTAINS admitted; module concentration and sample floors removed; module outranking the container; contradictory container falling through; per-pair marker hardwired false; pair marked only when both sides inferred; blast radius ignored; and every payload field, note and per-ref list deleted in turn. go build / go vet / gofmt clean; full internal/graph/... and internal/mcp green. Refs #6006 --- internal/graph/pr_impact.go | 170 ++++++-- internal/graph/pr_impact_6006_test.go | 32 +- internal/graph/pr_impact_6042_test.go | 550 +++++++++++++++++++++++--- internal/graph/pr_impact_infer.go | 394 +++++++++++------- internal/mcp/pr_impact_6042_test.go | 210 ++++++++-- internal/mcp/pr_impact_tools.go | 37 +- 6 files changed, 1131 insertions(+), 262 deletions(-) diff --git a/internal/graph/pr_impact.go b/internal/graph/pr_impact.go index 02a5c881b..3df586408 100644 --- a/internal/graph/pr_impact.go +++ b/internal/graph/pr_impact.go @@ -51,6 +51,11 @@ type ChangedEntity struct { // is the same defect class as #6006 — hence no omitempty: the label is always // on the wire. CommunitySource string `json:"community_source"` + // CommunityInference is the provenance of an inferred placement: which signals + // voted and how strong the deciding one was. Present only when + // CommunitySource == "inferred". A 26-of-51 plurality and a 40-of-41 consensus + // are both "inferred"; only this tells them apart. + CommunityInference *CommunityInference `json:"community_inference,omitempty"` } // ImpactedCommunity is a community touched by the change, with how many changed @@ -60,9 +65,13 @@ type ImpactedCommunity struct { ChangedCount int `json:"changed_count"` // changed entities in this community BlastRadiusHit int `json:"blast_radius_hit"` // downstream entities in this community // InferredChangedCount is how many of ChangedCount were placed here by - // INFERENCE rather than by the overlay (#6042). Equal to ChangedCount means - // this community's involvement is entirely deduced. + // INFERENCE rather than by the overlay (#6042). InferredChangedCount int `json:"inferred_changed_count,omitempty"` + // InferredOnly is true when this community appears in the impact set SOLELY + // because of inferred placements: no overlay-placed changed entity and no + // blast-radius entity puts it here. Such a community is a deduction, and any + // merge-risk overlap resting on it is a deduced conflict, not a measured one. + InferredOnly bool `json:"inferred_only,omitempty"` } // BlastEntity is a downstream entity that transitively depends on a changed @@ -229,15 +238,21 @@ func AnalyzePRImpact(entities []Entity, rels []Relationship, change ChangeSet, o changedIDs := change.ChangedIDs() seedSet := make(map[string]struct{}, len(changedIDs)) - // #6042: the changed entities the overlay could NOT place, and which are - // present in the head graph so they have neighbours to read. Computed before - // the edge pass below so that pass can capture their edges in one sweep - // instead of building a whole outbound adjacency for the graph. - unplacedChanged := make(map[string]struct{}) + // #6042: the inference CANDIDATES — changed entities present in the head graph + // that the partition has never seen (Entity.CommunityID == nil). Computed + // before the edge pass below so that pass can capture their edges in one + // sweep instead of building a whole outbound adjacency for the graph. + // + // NOT communityOf(e) < 0. An entity carrying a NON-NIL negative id was in the + // last group index and community detection declined to place it (-2 is + // groupalgo's "not assigned"; legacy graph.json can carry -1). Inferring for + // those would overrule the algorithm's own decision with a path heuristic — + // see overlayAbsent and TestAnalyzePRImpact_NegativeCommunityIDIsNotInferred. + inferCandidates := make(map[string]struct{}) for _, id := range changedIDs { seedSet[id] = struct{}{} - if e, ok := byID[id]; ok && communityOf(e) < 0 { - unplacedChanged[id] = struct{}{} + if e, ok := byID[id]; ok && overlayAbsent(e) { + inferCandidates[id] = struct{}{} } } @@ -246,13 +261,13 @@ func AnalyzePRImpact(entities []Entity, rels []Relationship, change ChangeSet, o // edge-filtering contract used elsewhere. // // The same sweep collects the #6042 inference inputs — outbound targets and - // CONTAINS parents — but ONLY for unplaced changed entities, so the extra - // memory is O(deg(changed)) rather than O(E). + // CONTAINS parents — but ONLY for inference candidates, so the extra memory is + // O(deg(changed)) rather than O(E). in := make(map[string][]string, len(entities)) var inferParents, inferTargets map[string][]string - if len(unplacedChanged) > 0 { - inferParents = make(map[string][]string, len(unplacedChanged)) - inferTargets = make(map[string][]string, len(unplacedChanged)) + if len(inferCandidates) > 0 { + inferParents = make(map[string][]string, len(inferCandidates)) + inferTargets = make(map[string][]string, len(inferCandidates)) } for _, r := range rels { if r.FromID == "" || r.ToID == "" || r.FromID == r.ToID { @@ -267,18 +282,26 @@ func AnalyzePRImpact(entities []Entity, rels []Relationship, change ChangeSet, o // r.FromID depends on r.ToID, so FromID is a downstream dependent of ToID. in[r.ToID] = append(in[r.ToID], r.FromID) - if len(unplacedChanged) > 0 { - if _, ok := unplacedChanged[r.FromID]; ok { - inferTargets[r.FromID] = append(inferTargets[r.FromID], r.ToID) + if len(inferCandidates) > 0 { + // Only edge kinds that actually carry community signal feed the target + // vote. IMPORTS in particular is excluded: every new file imports two + // placed packages, so it would clear the minPlacedTargets threshold + // universally while saying almost nothing about community membership. + // CONTAINS is excluded here because it is the CONTAINER signal, captured + // separately below — one edge must not vote twice under two names. + if isInferTargetKind(r.Kind) { + if _, ok := inferCandidates[r.FromID]; ok { + inferTargets[r.FromID] = append(inferTargets[r.FromID], r.ToID) + } } if r.Kind == "CONTAINS" { - if _, ok := unplacedChanged[r.ToID]; ok { + if _, ok := inferCandidates[r.ToID]; ok { inferParents[r.ToID] = append(inferParents[r.ToID], r.FromID) } } } } - inferrer := newCommunityInferrer(entities, byID, unplacedChanged, inferParents, inferTargets) + inferrer := newCommunityInferrer(entities, byID, inferCandidates, inferParents, inferTargets) changed := make([]ChangedEntity, 0, len(changedIDs)) // communityChanged[community] = #changed entities in it; communityInferred is @@ -295,13 +318,14 @@ func AnalyzePRImpact(entities []Entity, rels []Relationship, change ChangeSet, o for _, id := range changedIDs { comm := -1 source := CommunitySourceNone + var detail *CommunityInference var name, kind, src string if e, ok := byID[id]; ok { comm = communityOf(e) if comm >= 0 { source = CommunitySourceOverlay - } else if c, inferred := inferrer.infer(id); inferred { - comm, source = c, CommunitySourceInferred + } else if c, d, inferred := inferrer.infer(id); inferred { + comm, source, detail = c, CommunitySourceInferred, d } name, kind, src = e.Name, e.Kind, e.SourceFile } else { @@ -315,13 +339,14 @@ func AnalyzePRImpact(entities []Entity, rels []Relationship, change ChangeSet, o } } changed = append(changed, ChangedEntity{ - ID: id, - Name: name, - Kind: kind, - SourceFile: src, - Change: classOf[id], - CommunityID: comm, - CommunitySource: source, + ID: id, + Name: name, + Kind: kind, + SourceFile: src, + Change: classOf[id], + CommunityID: comm, + CommunitySource: source, + CommunityInference: detail, }) communityChanged[comm]++ switch source { @@ -427,11 +452,18 @@ func AnalyzePRImpact(entities []Entity, rels []Relationship, change ChangeSet, o if c < 0 { continue } + // #6042 D1: this community rests ENTIRELY on inference when no + // overlay-placed changed entity and no blast-radius entity put it here. + // The blast radius counts as measured: it is real edges from real seeds to + // entities the overlay did place. + inferredOnlyCommunity := communityInferred[c] > 0 && + communityChanged[c] == communityInferred[c] && communityBlast[c] == 0 impacted = append(impacted, ImpactedCommunity{ CommunityID: c, ChangedCount: communityChanged[c], BlastRadiusHit: communityBlast[c], InferredChangedCount: communityInferred[c], + InferredOnly: inferredOnlyCommunity, }) } // Rank by total touch (changed+blast) desc, then community id asc. @@ -478,6 +510,26 @@ func (r PRImpactResult) ImpactedCommunityIDs() []int { return out } +// InferredOnlyCommunityIDs returns the sorted subset of ImpactedCommunityIDs +// whose presence rests ENTIRELY on inferred placements (#6042 D1). +// +// This is what makes per-PAIR provenance possible. CommunityDataInferredOnly is +// a whole-verdict flag and says nothing about an individual overlap: a ref can +// have two measured communities and one inferred one, and if the inferred one is +// the ONLY community it shares with another ref, the reported conflict is +// entirely manufactured while the verdict-level flag reads false. +func (r PRImpactResult) InferredOnlyCommunityIDs() []int { + out := make([]int, 0, len(r.ImpactedCommunities)) + for _, c := range r.ImpactedCommunities { + if c.CommunityID < 0 || !c.InferredOnly { + continue + } + out = append(out, c.CommunityID) + } + sort.Ints(out) + return out +} + // --------------------------------------------------------------------------- // Cross-change merge-risk // --------------------------------------------------------------------------- @@ -501,6 +553,10 @@ type ChangeImpact struct { // group-algo partition or on inference from placed neighbours. OverlayEntityCount int InferredEntityCount int + // InferredOnlyCommunities are the subset of Communities that this ref touches + // ONLY through inferred placements (PRImpactResult.InferredOnlyCommunityIDs). + // A pair overlapping solely on these is a deduced conflict. + InferredOnlyCommunities []int } // MergeRiskPair is two refs whose impacted-community sets overlap. @@ -509,6 +565,21 @@ type MergeRiskPair struct { RefB string `json:"ref_b"` SharedCount int `json:"shared_community_count"` SharedCommunities []int `json:"shared_communities"` + + // InferredSharedCommunities are the shared communities that at least one side + // touches ONLY through inference (#6042 D1). The overlap on such a community + // was not observed — it was deduced on one or both sides. + InferredSharedCommunities []int `json:"inferred_shared_communities,omitempty"` + // InferredOnly is true when EVERY shared community is in the list above, i.e. + // this reported conflict is entirely a product of inference. + // + // The verdict-level CommunityDataInferredOnly cannot express this: a pair of + // refs can each have measured communities (so the verdict looks measured) and + // still overlap ONLY on an inferred one, making the reported conflict + // manufactured while every aggregate flag reads "measured". That is the #6006 + // defect class at pair granularity, which is the granularity a merge decision + // is actually made at. + InferredOnly bool `json:"inferred_only"` } // MergeRiskResult is the ranked triage output of AnalyzeMergeRisk. @@ -542,6 +613,12 @@ type MergeRiskResult struct { // RefsWithInferredCommunityData names the refs that contributed at least one // inferred placement, sorted. RefsWithInferredCommunityData []string `json:"refs_with_inferred_community_data,omitempty"` + // InferredOnlyPairCount is how many of the reported risk pairs overlap SOLELY + // on communities that at least one side reached by inference. Non-zero means + // at least one reported conflict is deduced rather than observed — even when + // CommunityDataInferredOnly is false, which it will be whenever the refs also + // touch measured communities that happen not to overlap. + InferredOnlyPairCount int `json:"inferred_only_pair_count"` } // AnalyzeMergeRisk intersects every change's impacted-community set pairwise and @@ -569,6 +646,7 @@ func AnalyzeMergeRisk(impacts []ChangeImpact) MergeRiskResult { } } sets := make([]map[int]struct{}, len(norm)) + inferredSets := make([]map[int]struct{}, len(norm)) for i, ci := range norm { s := make(map[int]struct{}, len(ci.Communities)) for _, c := range ci.Communities { @@ -577,6 +655,13 @@ func AnalyzeMergeRisk(impacts []ChangeImpact) MergeRiskResult { } } sets[i] = s + inf := make(map[int]struct{}, len(ci.InferredOnlyCommunities)) + for _, c := range ci.InferredOnlyCommunities { + if c >= 0 { + inf[c] = struct{}{} + } + } + inferredSets[i] = inf } var pairs []MergeRiskPair @@ -587,11 +672,24 @@ func AnalyzeMergeRisk(impacts []ChangeImpact) MergeRiskResult { continue } sort.Ints(shared) + // #6042 D1 — per-pair provenance. A shared community is DEDUCED when + // either side reaches it only through inference: the overlap was never + // observed on that side, so the conflict itself is a deduction. + var inferredShared []int + for _, c := range shared { + _, a := inferredSets[i][c] + _, b := inferredSets[j][c] + if a || b { + inferredShared = append(inferredShared, c) + } + } pairs = append(pairs, MergeRiskPair{ - RefA: norm[i].Ref, - RefB: norm[j].Ref, - SharedCount: len(shared), - SharedCommunities: shared, + RefA: norm[i].Ref, + RefB: norm[j].Ref, + SharedCount: len(shared), + SharedCommunities: shared, + InferredSharedCommunities: inferredShared, + InferredOnly: len(inferredShared) == len(shared), }) } } @@ -605,6 +703,13 @@ func AnalyzeMergeRisk(impacts []ChangeImpact) MergeRiskResult { return pairs[i].RefB < pairs[j].RefB }) + inferredOnlyPairs := 0 + for _, p := range pairs { + if p.InferredOnly { + inferredOnlyPairs++ + } + } + return MergeRiskResult{ Pairs: pairs, RefCount: len(norm), @@ -617,6 +722,7 @@ func AnalyzeMergeRisk(impacts []ChangeImpact) MergeRiskResult { // Available, something was inferred, and nothing at all was measured. CommunityDataInferredOnly: len(missing) == 0 && totalInferred > 0 && totalOverlay == 0, RefsWithInferredCommunityData: inferredRefs, + InferredOnlyPairCount: inferredOnlyPairs, } } diff --git a/internal/graph/pr_impact_6006_test.go b/internal/graph/pr_impact_6006_test.go index 7fa4cf2cd..c7e53adac 100644 --- a/internal/graph/pr_impact_6006_test.go +++ b/internal/graph/pr_impact_6006_test.go @@ -119,10 +119,33 @@ func TestAnalyzePRImpact_EmptyChangeSetIsAvailable(t *testing.T) { // "not assigned a community"; legacy graph.json can carry -1 directly. If such a // value counted as coverage, CommunityDataAvailable would be true while all // three `c >= 0` filters dropped everything — #6006 again, stamped available. +// +// THE FIXTURE MUST HAVE NEIGHBOURS (#6042). The first cut of this test used a +// lone entity with no file siblings, no module and no edges — a shape production +// essentially never produces. Once #6042 added inference, that fixture passed +// for the wrong reason: there was simply nothing to infer FROM, so it no longer +// tested the sentinel at all. The entity below sits in a placed file, in a +// concentrated module, and calls two placed entities, so every inference signal +// is available and must still be refused: this entity was IN the last group +// index and community detection declined to place it. Inferring here would +// overrule the algorithm, not fill a gap in it. func TestAnalyzePRImpact_NegativeCommunityIDIsNotCoverage(t *testing.T) { for _, cid := range []int{-1, -2} { - ents := []Entity{{ID: "a", Name: "A", Kind: "function", CommunityID: ci(cid)}} - res := AnalyzePRImpact(ents, nil, ChangeSet{ + ents := []Entity{ + Entity{ID: "sib1", Name: "S1", Kind: "function", SourceFile: "core/a.go", CommunityID: ci(4)}. + WithProperties(map[string]string{"module": "core"}), + Entity{ID: "sib2", Name: "S2", Kind: "function", SourceFile: "core/b.go", CommunityID: ci(4)}. + WithProperties(map[string]string{"module": "core"}), + Entity{ID: "sib3", Name: "S3", Kind: "function", SourceFile: "core/c.go", CommunityID: ci(4)}. + WithProperties(map[string]string{"module": "core"}), + Entity{ID: "a", Name: "A", Kind: "function", SourceFile: "core/a.go", CommunityID: ci(cid)}. + WithProperties(map[string]string{"module": "core"}), + } + rels := []Relationship{ + {FromID: "a", ToID: "sib2", Kind: "CALLS"}, + {FromID: "a", ToID: "sib3", Kind: "CALLS"}, + } + res := AnalyzePRImpact(ents, rels, ChangeSet{ Modified: []DiffEntityEntry{{ID: "a"}}, }, DefaultPRImpactOptions()) if res.CommunityDataAvailable { @@ -133,6 +156,11 @@ func TestAnalyzePRImpact_NegativeCommunityIDIsNotCoverage(t *testing.T) { t.Errorf("community_id=%d: ChangedWithoutCommunity = %d, want 1", cid, res.ChangedWithoutCommunity) } + if res.ChangedWithInferredCommunity != 0 { + t.Errorf("community_id=%d: the partition SAW this entity and declined to place it; "+ + "inference must not overrule that (ChangedWithInferredCommunity = %d)", + cid, res.ChangedWithInferredCommunity) + } } } diff --git a/internal/graph/pr_impact_6042_test.go b/internal/graph/pr_impact_6042_test.go index e0a70accf..a078c441e 100644 --- a/internal/graph/pr_impact_6042_test.go +++ b/internal/graph/pr_impact_6042_test.go @@ -8,12 +8,11 @@ // and the entity did not exist then. // // So we INFER a community from the entity's PLACED neighbours — and label it. -// The two failure modes this file pins are symmetric: +// The failure modes pinned here are symmetric: // -// - inference that never fires (the tool stays useless), and -// - inference presented as fact (the #6006 defect class, reintroduced one -// layer up: a confident answer the caller cannot distinguish from a -// measured one). +// - inference that never fires (the tool stays useless), +// - inference presented as fact (the #6006 defect class one layer up), and +// - inference that overrules a decision the partition actually made. // // FIXTURE SHAPE IS LOAD-BEARING. Every fixture below is production-shaped: only // entities that existed at the last group index carry a CommunityID. Newly @@ -21,7 +20,10 @@ // group-algo pass cannot produce, and would make every test here vacuous. package graph -import "testing" +import ( + "reflect" + "testing" +) // placedEnt is an entity as the overlay stamper leaves it: covered by the last // group index, so it carries a real community id. @@ -33,8 +35,9 @@ func placedEnt(id, file, module string, community int) Entity { return e } -// newEnt is an entity that exists only on the feature ref: no community, by -// construction, because the overlay predates it. +// newEnt is an entity that exists only on the feature ref: NO CommunityID +// pointer at all, by construction, because the overlay predates it. That nil — +// not a negative id — is what makes it an inference candidate. func newEnt(id, file, module string) Entity { e := Entity{ID: id, Name: id, Kind: "function", SourceFile: file} if module != "" { @@ -47,7 +50,7 @@ func newEnt(id, file, module string) Entity { // with its own directory/module. Nothing here is new. // // community 7 (core): core:a, core:b, core:c in core/*.go, module "core" -// community 9 (db): db:d, db:e in db/*.go, module "db" +// community 9 (db): db:d, db:e, db:f in db/*.go, module "db" func prImpact6042Fixture() ([]Entity, []Relationship) { return []Entity{ placedEnt("core:a", "core/a.go", "core", 7), @@ -55,22 +58,29 @@ func prImpact6042Fixture() ([]Entity, []Relationship) { placedEnt("core:c", "core/c.go", "core", 7), placedEnt("db:d", "db/d.go", "db", 9), placedEnt("db:e", "db/e.go", "db", 9), + placedEnt("db:f", "db/f.go", "db", 9), }, []Relationship{ {FromID: "core:a", ToID: "core:b", Kind: "CALLS"}, {FromID: "db:d", ToID: "db:e", Kind: "CALLS"}, } } -// addOne appends one added entity (+ its outbound edges) and returns the change -// set that adds exactly it — the add-only PR shape. -func addOne(ents []Entity, rels []Relationship, e Entity, calls ...string) ([]Entity, []Relationship, ChangeSet) { +// addOneKind appends one added entity (+ its outbound edges of the given kind) +// and returns the change set that adds exactly it — the add-only PR shape. +func addOneKind(ents []Entity, rels []Relationship, e Entity, kind string, targets ...string) ( + []Entity, []Relationship, ChangeSet) { ents = append(ents, e) - for _, target := range calls { - rels = append(rels, Relationship{FromID: e.ID, ToID: target, Kind: "CALLS"}) + for _, target := range targets { + rels = append(rels, Relationship{FromID: e.ID, ToID: target, Kind: kind}) } return ents, rels, ChangeSet{Added: []DiffEntityEntry{{ID: e.ID, Name: e.Name, Kind: e.Kind}}} } +func addOne(ents []Entity, rels []Relationship, e Entity, calls ...string) ( + []Entity, []Relationship, ChangeSet) { + return addOneKind(ents, rels, e, "CALLS", calls...) +} + // changedByID finds the changed-entity record, failing loudly when absent. func changedByID(t *testing.T, res PRImpactResult, id string) ChangedEntity { t.Helper() @@ -86,7 +96,7 @@ func changedByID(t *testing.T, res PRImpactResult, id string) ChangedEntity { // ── The signals ────────────────────────────────────────────────────────────── // Signal 1 — containing component. A new function added to an ALREADY-PLACED -// file belongs to that file's community with very high confidence. Module is +// file belongs to that file's community with high confidence. Module is // deliberately unset here so the file is the only signal in play. func TestAnalyzePRImpact_InfersFromContainingFile(t *testing.T) { ents, rels := prImpact6042Fixture() @@ -102,6 +112,18 @@ func TestAnalyzePRImpact_InfersFromContainingFile(t *testing.T) { t.Errorf("community_source = %q, want %q — an inferred placement presented as "+ "measured is the #6006 defect one layer up", got.CommunitySource, CommunitySourceInferred) } + // The margin must travel with the placement: "inferred" alone cannot + // distinguish a file consensus from a coin flip. + if got.CommunityInference == nil { + t.Fatalf("inferred placement carries no provenance; a caller cannot weigh it") + } + if !reflect.DeepEqual(got.CommunityInference.Signals, []string{inferSignalContainer}) { + t.Errorf("signals = %v, want [container]", got.CommunityInference.Signals) + } + if got.CommunityInference.Support != 1 || got.CommunityInference.Sample != 1 { + t.Errorf("support/sample = %d/%d, want 1/1", + got.CommunityInference.Support, got.CommunityInference.Sample) + } if !res.CommunityDataAvailable { t.Errorf("an inferred placement is still a placement; CommunityDataAvailable = false") } @@ -117,14 +139,20 @@ func TestAnalyzePRImpact_InfersFromContainingFile(t *testing.T) { if ids := res.ImpactedCommunityIDs(); len(ids) != 1 || ids[0] != 7 { t.Errorf("inferred community must reach the merge-risk key; got %v, want [7]", ids) } - // The rollup says how much of the community's touch was inferred. - if len(res.ImpactedCommunities) != 1 || res.ImpactedCommunities[0].InferredChangedCount != 1 { - t.Errorf("impacted community rollup must count the inferred entity: %+v", res.ImpactedCommunities) + // The rollup says how much of the community's touch was inferred, and that + // this community is in the impact set ONLY because of inference. + if len(res.ImpactedCommunities) != 1 || res.ImpactedCommunities[0].InferredChangedCount != 1 || + !res.ImpactedCommunities[0].InferredOnly { + t.Errorf("impacted community rollup must mark the community as inference-only: %+v", + res.ImpactedCommunities) + } + if ids := res.InferredOnlyCommunityIDs(); len(ids) != 1 || ids[0] != 7 { + t.Errorf("InferredOnlyCommunityIDs = %v, want [7]", ids) } } -// Signal 2 — module. A brand-new FILE inside an existing module: no same-file -// sibling exists, so only the module prior can place it. +// Signal 2 — module, the FALLBACK. A brand-new FILE inside an existing module: +// no same-file sibling exists, so only the module prior can place it. func TestAnalyzePRImpact_InfersFromModuleWhenFileIsNew(t *testing.T) { ents, rels := prImpact6042Fixture() ents, rels, change := addOne(ents, rels, newEnt("core:new", "core/brand_new.go", "core")) @@ -136,11 +164,20 @@ func TestAnalyzePRImpact_InfersFromModuleWhenFileIsNew(t *testing.T) { t.Errorf("new file in module \"core\" got community %d source %q, want 7/inferred", got.CommunityID, got.CommunitySource) } + if got.CommunityInference == nil || + !reflect.DeepEqual(got.CommunityInference.Signals, []string{inferSignalModule}) { + t.Fatalf("want the module signal recorded, got %+v", got.CommunityInference) + } + // 3 of 3 — the concentration the module fallback demands. + if got.CommunityInference.Support != 3 || got.CommunityInference.Sample != 3 { + t.Errorf("support/sample = %d/%d, want 3/3", + got.CommunityInference.Support, got.CommunityInference.Sample) + } } // Signal 3 — outbound call targets. A new entity in a new file in a new module, -// calling two placed entities that agree. This is close to what Louvain would -// have decided had it seen the entity. +// calling two placed entities that agree. This is the only signal not derived +// from the file path, and the closest to what community detection would see. func TestAnalyzePRImpact_InfersFromPlacedCallTargets(t *testing.T) { ents, rels := prImpact6042Fixture() ents, rels, change := addOne(ents, rels, @@ -153,6 +190,90 @@ func TestAnalyzePRImpact_InfersFromPlacedCallTargets(t *testing.T) { t.Errorf("new entity calling two community-7 entities got community %d source %q, want 7/inferred", got.CommunityID, got.CommunitySource) } + if got.CommunityInference == nil || + !reflect.DeepEqual(got.CommunityInference.Signals, []string{inferSignalTargets}) { + t.Fatalf("want the call_targets signal recorded, got %+v", got.CommunityInference) + } +} + +// Both signals agreeing is the strongest case, and BOTH must be recorded — a +// caller that sees one signal cannot tell it from two independent ones. +func TestAnalyzePRImpact_AgreeingSignalsAreBothRecorded(t *testing.T) { + ents, rels := prImpact6042Fixture() + ents, rels, change := addOne(ents, rels, + newEnt("core:new", "core/a.go", "core"), "core:b", "core:c") + + res := AnalyzePRImpact(ents, rels, change, DefaultPRImpactOptions()) + + got := changedByID(t, res, "core:new") + if got.CommunityID != 7 || got.CommunitySource != CommunitySourceInferred { + t.Fatalf("got %d/%q, want 7/inferred", got.CommunityID, got.CommunitySource) + } + if got.CommunityInference == nil || !reflect.DeepEqual( + got.CommunityInference.Signals, []string{inferSignalContainer, inferSignalTargets}) { + t.Errorf("signals = %+v, want [container call_targets]", got.CommunityInference) + } +} + +// ── Signal hygiene: what must NOT count as evidence ────────────────────────── + +// IMPORTS must not drive inference. Every new file imports two placed packages, +// so counting imports would clear the >= 2 threshold universally while carrying +// almost no community information — the exact failure the threshold exists to +// prevent, reintroduced by edge kind. +func TestAnalyzePRImpact_ImportsAreNotACommunitySignal(t *testing.T) { + ents, rels := prImpact6042Fixture() + ents, rels, change := addOneKind(ents, rels, + newEnt("new:x", "newpkg/x.go", "newpkg"), "IMPORTS", "core:a", "core:b") + + res := AnalyzePRImpact(ents, rels, change, DefaultPRImpactOptions()) + + got := changedByID(t, res, "new:x") + if got.CommunitySource != CommunitySourceNone { + t.Errorf("IMPORTS is universal and carries no community signal; got %d/%q, want -1/none", + got.CommunityID, got.CommunitySource) + } + if res.CommunityDataAvailable { + t.Errorf("nothing inferrable; the #6006 decline path must stand") + } +} + +// CONTAINS must not vote as a call target. It IS the container signal; letting +// it through here would give one edge two votes under two names. +func TestAnalyzePRImpact_ContainsDoesNotVoteAsCallTarget(t *testing.T) { + ents, rels := prImpact6042Fixture() + // The new entity CONTAINS two placed entities (child-ward, not parent-ward), + // so the container signal cannot use them either. + ents, rels, change := addOneKind(ents, rels, + newEnt("new:x", "newpkg/x.go", "newpkg"), "CONTAINS", "core:a", "core:b") + + res := AnalyzePRImpact(ents, rels, change, DefaultPRImpactOptions()) + + if got := changedByID(t, res, "new:x"); got.CommunitySource != CommunitySourceNone { + t.Errorf("outbound CONTAINS is not a call-target signal; got %d/%q, want -1/none", + got.CommunityID, got.CommunitySource) + } +} + +// A placed CONTAINS PARENT is legitimate container evidence, even when the new +// entity's file is new. This is the signal the issue names first. +func TestAnalyzePRImpact_ContainsParentIsContainerEvidence(t *testing.T) { + ents, rels := prImpact6042Fixture() + ents = append(ents, newEnt("new:method", "newpkg/x.go", "")) + rels = append(rels, Relationship{FromID: "core:a", ToID: "new:method", Kind: "CONTAINS"}) + change := ChangeSet{Added: []DiffEntityEntry{{ID: "new:method"}}} + + res := AnalyzePRImpact(ents, rels, change, DefaultPRImpactOptions()) + + got := changedByID(t, res, "new:method") + if got.CommunityID != 7 || got.CommunitySource != CommunitySourceInferred { + t.Errorf("a placed CONTAINS parent places its child; got %d/%q, want 7/inferred", + got.CommunityID, got.CommunitySource) + } + if got.CommunityInference == nil || + !reflect.DeepEqual(got.CommunityInference.Signals, []string{inferSignalContainer}) { + t.Errorf("want the container signal recorded, got %+v", got.CommunityInference) + } } // ── The decline paths that MUST survive ────────────────────────────────────── @@ -192,6 +313,9 @@ func TestAnalyzePRImpact_NoPlacedNeighboursStillDeclines(t *testing.T) { if got.CommunitySource != CommunitySourceNone { t.Errorf("no placed neighbour exists; community_source = %q, want none", got.CommunitySource) } + if got.CommunityInference != nil { + t.Errorf("no inference happened, so no provenance: %+v", got.CommunityInference) + } if res.CommunityDataAvailable { t.Errorf("CommunityDataAvailable = true with nothing placed and nothing inferrable — " + "#6006's decline path was weakened into meaninglessness") @@ -201,21 +325,19 @@ func TestAnalyzePRImpact_NoPlacedNeighboursStillDeclines(t *testing.T) { } } -// Signals that DISAGREE must decline, not guess. Here the containing file says -// community 7 while the module AND the call targets say 9 — the containing -// component is weighted highest, but not highly enough to outweigh both of the -// other signals together. -func TestAnalyzePRImpact_ConflictingSignalsDecline(t *testing.T) { +// Signals that DISAGREE must decline, not guess: the containing file says 7, +// the call targets say 9. +func TestAnalyzePRImpact_ContainerAndTargetsDisagreeDecline(t *testing.T) { ents, rels := prImpact6042Fixture() ents, rels, change := addOne(ents, rels, - newEnt("core:new", "core/a.go", "db"), // file → 7, module → 9 - "db:d", "db:e") // targets → 9 + newEnt("core:new", "core/a.go", ""), // container → 7 + "db:d", "db:e") // targets → 9 res := AnalyzePRImpact(ents, rels, change, DefaultPRImpactOptions()) got := changedByID(t, res, "core:new") if got.CommunitySource != CommunitySourceNone { - t.Errorf("file says 7, module and call targets say 9 — inference must abstain; got %d/%q", + t.Errorf("file says 7 and call targets say 9 — inference must abstain; got %d/%q", got.CommunityID, got.CommunitySource) } if res.CommunityDataAvailable { @@ -223,14 +345,33 @@ func TestAnalyzePRImpact_ConflictingSignalsDecline(t *testing.T) { } } +// The same rule with the module standing in as the primary: a brand-new file in +// module "core" (→ 7) whose call targets say 9. +func TestAnalyzePRImpact_ModuleAndTargetsDisagreeDecline(t *testing.T) { + ents, rels := prImpact6042Fixture() + ents, rels, change := addOne(ents, rels, + newEnt("core:new", "core/brand_new.go", "core"), // module → 7 (the file is new) + "db:d", "db:e") // targets → 9 + + res := AnalyzePRImpact(ents, rels, change, DefaultPRImpactOptions()) + + if got := changedByID(t, res, "core:new"); got.CommunitySource != CommunitySourceNone { + t.Errorf("module says 7 and call targets say 9 — inference must abstain; got %d/%q", + got.CommunityID, got.CommunitySource) + } +} + // A file whose placed entities are SPLIT between communities gives no plurality, -// so the container signal abstains — and with no other signal, the entity is -// unplaced. Without this, a 50/50 file would be decided by map iteration order. +// so the container signal abstains. Critically it must NOT then fall through to +// the module: the container did not abstain for lack of evidence, it abstained +// because its evidence was contradictory, and the module is a strictly coarser +// view of the same path that would paper over exactly that. func TestAnalyzePRImpact_AmbiguousContainerAbstains(t *testing.T) { ents, rels := prImpact6042Fixture() - // A second placed entity in core/a.go, in the OTHER community. - ents = append(ents, placedEnt("core:split", "core/a.go", "", 9)) - ents, rels, change := addOne(ents, rels, newEnt("core:new", "core/a.go", "")) + // A second placed entity in core/a.go, in the OTHER community. Module "core" + // still leans 7 overall, so a fall-through would silently place this entity. + ents = append(ents, placedEnt("core:split", "core/a.go", "core", 9)) + ents, rels, change := addOne(ents, rels, newEnt("core:new", "core/a.go", "core")) res := AnalyzePRImpact(ents, rels, change, DefaultPRImpactOptions()) @@ -241,6 +382,164 @@ func TestAnalyzePRImpact_AmbiguousContainerAbstains(t *testing.T) { } } +// ── The module fallback is bounded ─────────────────────────────────────────── + +// module.Derive is a depth-capped PATH PREFIX: with no per-Go-package marker a +// single-module repo puts everything under internal/graph/** in one bucket. A +// bare plurality over such a bucket is noise wearing a community id, so the +// module vote requires real concentration. +func TestAnalyzePRImpact_ModulePluralityWithoutConcentrationDeclines(t *testing.T) { + // A big heterogeneous module: 6 in community 7, 5 in community 9 — a genuine + // plurality (6/11 = 0.55) and nothing like a consensus. + var ents []Entity + for i := 0; i < 6; i++ { + s := itoaBench(i) + ents = append(ents, placedEnt("m:a"+s, "mixed/a"+s+".go", "mixed", 7)) + } + for i := 0; i < 5; i++ { + s := itoaBench(i) + ents = append(ents, placedEnt("m:b"+s, "mixed/b"+s+".go", "mixed", 9)) + } + ents, rels, change := addOne(ents, nil, newEnt("m:new", "mixed/brand_new.go", "mixed")) + + res := AnalyzePRImpact(ents, rels, change, DefaultPRImpactOptions()) + + got := changedByID(t, res, "m:new") + if got.CommunitySource != CommunitySourceNone { + t.Errorf("module \"mixed\" is 6/11 in community 7 — a plurality, not a signal; "+ + "got %d/%q, want -1/none", got.CommunityID, got.CommunitySource) + } + if res.CommunityDataAvailable { + t.Errorf("a 55%% path-prefix lean is not a placement") + } +} + +// A tight module still places — the floor must reject noise without rejecting +// the genuinely concentrated case. +func TestAnalyzePRImpact_ConcentratedModuleStillInfers(t *testing.T) { + var ents []Entity + for i := 0; i < 9; i++ { + s := itoaBench(i) + ents = append(ents, placedEnt("m:a"+s, "tight/a"+s+".go", "tight", 7)) + } + ents = append(ents, placedEnt("m:z", "tight/z.go", "tight", 9)) // 9/10 = 0.9 + ents, rels, change := addOne(ents, nil, newEnt("m:new", "tight/brand_new.go", "tight")) + + res := AnalyzePRImpact(ents, rels, change, DefaultPRImpactOptions()) + + got := changedByID(t, res, "m:new") + if got.CommunityID != 7 || got.CommunitySource != CommunitySourceInferred { + t.Fatalf("a 9-of-10 module is a real signal; got %d/%q, want 7/inferred", + got.CommunityID, got.CommunitySource) + } + // And the margin is on the wire, so 9/10 is distinguishable from 6/11. + if got.CommunityInference.Support != 9 || got.CommunityInference.Sample != 10 { + t.Errorf("support/sample = %d/%d, want 9/10", + got.CommunityInference.Support, got.CommunityInference.Sample) + } +} + +// A module with too FEW placed entities is not a prior at all, however unanimous. +func TestAnalyzePRImpact_TinyModuleSampleDeclines(t *testing.T) { + ents := []Entity{ + placedEnt("t:a", "tiny/a.go", "tiny", 7), + placedEnt("t:b", "tiny/b.go", "tiny", 7), + } + ents, rels, change := addOne(ents, nil, newEnt("t:new", "tiny/brand_new.go", "tiny")) + + res := AnalyzePRImpact(ents, rels, change, DefaultPRImpactOptions()) + + if got := changedByID(t, res, "t:new"); got.CommunitySource != CommunitySourceNone { + t.Errorf("2 placed entities is not a module prior; got %d/%q, want -1/none", + got.CommunityID, got.CommunitySource) + } +} + +// The module must NOT vote alongside the container. module is +// module.Derive(SourceFile) — a pure function of the path — so the module +// histogram is a strict SUPERSET of the file histogram, and letting both vote +// counts one measurement twice. Here the file (community 7) is the specific +// evidence and the wider module leans 9; the file must simply win, with the +// module recorded nowhere. +func TestAnalyzePRImpact_ModuleDoesNotVoteAlongsideContainer(t *testing.T) { + ents := []Entity{ + placedEnt("w:a", "wide/a.go", "wide", 7), // the new entity's file + placedEnt("w:b", "wide/b.go", "wide", 9), + placedEnt("w:c", "wide/c.go", "wide", 9), + placedEnt("w:d", "wide/d.go", "wide", 9), + placedEnt("w:e", "wide/e.go", "wide", 9), // module "wide" is 4/5 → 9 + } + ents, rels, change := addOne(ents, nil, newEnt("w:new", "wide/a.go", "wide")) + + res := AnalyzePRImpact(ents, rels, change, DefaultPRImpactOptions()) + + got := changedByID(t, res, "w:new") + if got.CommunityID != 7 || got.CommunitySource != CommunitySourceInferred { + t.Errorf("the containing file is the more specific evidence; got %d/%q, want 7/inferred", + got.CommunityID, got.CommunitySource) + } + if got.CommunityInference == nil || + !reflect.DeepEqual(got.CommunityInference.Signals, []string{inferSignalContainer}) { + t.Errorf("the module must not appear as a second, independent signal: %+v", + got.CommunityInference) + } +} + +// ── Inference must not overrule the partition ──────────────────────────────── + +// A NON-NIL negative community id means the partition SAW this entity and +// declined to place it (-2 is groupalgo's "not assigned"; legacy graph.json can +// carry -1). #6042 is about entities the partition has NEVER seen. Inferring +// here replaces a decision community detection actually made with a path-prefix +// heuristic. +// +// The fixture is production-shaped and deliberately RICH in signal: the entity +// sits in a placed file, in a concentrated module, and calls two placed +// entities. If candidacy keyed on communityOf(e) < 0 instead of the nil pointer, +// every one of those would fire and this would come back "inferred". +func TestAnalyzePRImpact_NegativeCommunityIDIsNotInferred(t *testing.T) { + for _, cid := range []int{-1, -2} { + ents, rels := prImpact6042Fixture() + declined := placedEnt("core:declined", "core/a.go", "core", cid) + ents, rels, change := addOne(ents, rels, declined, "core:b", "core:c") + + res := AnalyzePRImpact(ents, rels, change, DefaultPRImpactOptions()) + + got := changedByID(t, res, "core:declined") + if got.CommunitySource != CommunitySourceNone { + t.Errorf("community_id=%d: the partition saw this entity and declined to place it; "+ + "got %d/%q — inference overruled the group algorithm", + cid, got.CommunityID, got.CommunitySource) + } + if got.CommunityInference != nil { + t.Errorf("community_id=%d: provenance emitted for a non-inference: %+v", + cid, got.CommunityInference) + } + if res.CommunityDataAvailable { + t.Errorf("community_id=%d: an unplaced-by-the-algorithm entity is not coverage", cid) + } + } +} + +// Nothing may be inferred when the entity was already placed by the overlay — +// the overlay is ground truth and must win outright. +func TestAnalyzePRImpact_OverlayPlacementIsNeverOverwritten(t *testing.T) { + ents, rels := prImpact6042Fixture() + // db:d sits in db/d.go; a stray placed sibling there suggests 7. + ents = append(ents, placedEnt("db:stray", "db/d.go", "db", 7)) + res := AnalyzePRImpact(ents, rels, ChangeSet{ + Modified: []DiffEntityEntry{{ID: "db:d"}}, + }, DefaultPRImpactOptions()) + + got := changedByID(t, res, "db:d") + if got.CommunityID != 9 || got.CommunitySource != CommunitySourceOverlay { + t.Errorf("overlay-placed entity got %d/%q, want 9/overlay", got.CommunityID, got.CommunitySource) + } + if got.CommunityInference != nil { + t.Errorf("a measured placement must carry no inference provenance: %+v", got.CommunityInference) + } +} + // Inference must not CHAIN. Only overlay-placed entities may vote; an entity // that was itself inferred is a guess, and letting guesses vote would propagate // one weak signal across a whole new package. @@ -248,8 +547,8 @@ func TestAnalyzePRImpact_InferenceDoesNotChain(t *testing.T) { ents, rels := prImpact6042Fixture() // first and second each infer 7 from their own placed call targets. third's // only neighbours are first and second — enough targets to vote, but neither - // is OVERLAY-placed, so third must stay unplaced. Distinct new files keep the - // container signal out of it. + // is OVERLAY-placed, so third must stay unplaced. Distinct new files and no + // modules keep the path signals out of it. ents = append(ents, newEnt("new:first", "newpkg/x.go", ""), newEnt("new:second", "newpkg/y.go", ""), @@ -309,21 +608,35 @@ func TestAnalyzePRImpact_OverlayAndInferredAreCountedSeparately(t *testing.T) { if res.CommunityDataInferredOnly { t.Errorf("one entity was really measured; CommunityDataInferredOnly must be false") } + // Community 7 holds a MEASURED changed entity too, so it is not an + // inference-only community even though one of its entities was inferred. + if ids := res.InferredOnlyCommunityIDs(); len(ids) != 0 { + t.Errorf("community 7 also holds a measured changed entity; InferredOnlyCommunityIDs = %v, want []", ids) + } } -// Nothing may be inferred when the entity was already placed by the overlay — -// the overlay is ground truth and must win outright. -func TestAnalyzePRImpact_OverlayPlacementIsNeverOverwritten(t *testing.T) { +// A community reached by the BLAST RADIUS is measured — real edges to entities +// the overlay really placed — even when the seed was inferred. It must not be +// marked inference-only. +func TestAnalyzePRImpact_BlastRadiusMakesACommunityMeasured(t *testing.T) { ents, rels := prImpact6042Fixture() - // db:d sits in db/d.go but we pretend a stray placed sibling suggests 7. - ents = append(ents, placedEnt("db:stray", "db/d.go", "db", 7)) - res := AnalyzePRImpact(ents, rels, ChangeSet{ - Modified: []DiffEntityEntry{{ID: "db:d"}}, - }, DefaultPRImpactOptions()) + // The new entity is inferred into 7, and core:a (community 7, placed) depends + // on it, so 7 is also reached by the blast radius. + ents = append(ents, newEnt("core:new", "core/a.go", "")) + rels = append(rels, Relationship{FromID: "core:a", ToID: "core:new", Kind: "CALLS"}) + change := ChangeSet{Added: []DiffEntityEntry{{ID: "core:new"}}} - got := changedByID(t, res, "db:d") - if got.CommunityID != 9 || got.CommunitySource != CommunitySourceOverlay { - t.Errorf("overlay-placed entity got %d/%q, want 9/overlay", got.CommunityID, got.CommunitySource) + res := AnalyzePRImpact(ents, rels, change, DefaultPRImpactOptions()) + + if got := changedByID(t, res, "core:new"); got.CommunitySource != CommunitySourceInferred { + t.Fatalf("precondition: the added entity must be inferred, got %q", got.CommunitySource) + } + if res.BlastRadiusCount == 0 { + t.Fatalf("precondition: core:a must be in the blast radius") + } + if ids := res.InferredOnlyCommunityIDs(); len(ids) != 0 { + t.Errorf("community 7 is also reached by a real edge to a really-placed entity; "+ + "InferredOnlyCommunityIDs = %v, want []", ids) } } @@ -345,6 +658,36 @@ func TestAnalyzePRImpact_RemovedEntityIsNotInferred(t *testing.T) { } } +// The three labels must always partition the changed set — otherwise the +// aggregate counts silently stop adding up and a caller cannot reconcile them. +func TestAnalyzePRImpact_SourceCountsPartitionTheChangedSet(t *testing.T) { + ents, rels := prImpact6042Fixture() + ents = append(ents, + newEnt("core:new", "core/a.go", "core"), // inferred + newEnt("new:lonely", "brandnew/l.go", "")) // none + change := ChangeSet{ + Modified: []DiffEntityEntry{{ID: "core:c"}}, // overlay + Added: []DiffEntityEntry{{ID: "core:new"}, {ID: "new:lonely"}}, // inferred + none + Removed: []DiffEntityEntry{{ID: "gone:z", SourceFile: "core/a.go"}}, // none + } + + res := AnalyzePRImpact(ents, rels, change, DefaultPRImpactOptions()) + + sum := res.ChangedWithOverlayCommunity + res.ChangedWithInferredCommunity + res.ChangedWithoutCommunity + if sum != res.ChangedCount { + t.Errorf("overlay %d + inferred %d + none %d = %d, want changed_count %d", + res.ChangedWithOverlayCommunity, res.ChangedWithInferredCommunity, + res.ChangedWithoutCommunity, sum, res.ChangedCount) + } + for _, c := range res.ChangedEntities { + switch c.CommunitySource { + case CommunitySourceOverlay, CommunitySourceInferred, CommunitySourceNone: + default: + t.Errorf("entity %s carries no usable community_source (%q)", c.ID, c.CommunitySource) + } + } +} + // ── Merge risk ─────────────────────────────────────────────────────────────── // Two add-only refs whose entities were both INFERRED into the same community do @@ -352,8 +695,10 @@ func TestAnalyzePRImpact_RemovedEntityIsNotInferred(t *testing.T) { // say the verdict rests entirely on inference. func TestAnalyzeMergeRisk_InferredOnlyVerdictIsFlagged(t *testing.T) { inferred := AnalyzeMergeRisk([]ChangeImpact{ - {Ref: "pr-a", Communities: []int{7}, CommunityDataAvailable: true, InferredEntityCount: 1}, - {Ref: "pr-b", Communities: []int{7}, CommunityDataAvailable: true, InferredEntityCount: 2}, + {Ref: "pr-a", Communities: []int{7}, CommunityDataAvailable: true, InferredEntityCount: 1, + InferredOnlyCommunities: []int{7}}, + {Ref: "pr-b", Communities: []int{7}, CommunityDataAvailable: true, InferredEntityCount: 2, + InferredOnlyCommunities: []int{7}}, }) if inferred.RiskyPairs != 1 { t.Fatalf("both refs land in community 7; want 1 risky pair, got %d", inferred.RiskyPairs) @@ -374,8 +719,12 @@ func TestAnalyzeMergeRisk_InferredOnlyVerdictIsFlagged(t *testing.T) { if measured.CommunityDataInferredOnly { t.Errorf("both refs were measured; CommunityDataInferredOnly must be false") } - if measured.InferredEntityCount != 0 { - t.Errorf("InferredEntityCount = %d, want 0", measured.InferredEntityCount) + if measured.InferredEntityCount != 0 || measured.InferredOnlyPairCount != 0 { + t.Errorf("measured verdict reported inferred count %d / inferred pairs %d, want 0/0", + measured.InferredEntityCount, measured.InferredOnlyPairCount) + } + if measured.Pairs[0].InferredOnly || len(measured.Pairs[0].InferredSharedCommunities) != 0 { + t.Errorf("a measured overlap must not be marked deduced: %+v", measured.Pairs[0]) } // Partly measured is not "inferred only" — but the inferred count still shows. @@ -391,6 +740,77 @@ func TestAnalyzeMergeRisk_InferredOnlyVerdictIsFlagged(t *testing.T) { } } +// Both refs carry MEASURED communities, so every aggregate flag reads +// "measured" — and yet the only community they SHARE, the entire basis of the +// reported conflict, exists on both sides purely by inference. A whole-verdict +// flag cannot express this; the pair must carry its own provenance, because the +// pair is where a merge decision is actually made. +func TestAnalyzeMergeRisk_PairOverlappingOnlyOnInferenceIsMarked(t *testing.T) { + res := AnalyzeMergeRisk([]ChangeImpact{ + {Ref: "pr-a", Communities: []int{3, 7}, CommunityDataAvailable: true, + OverlayEntityCount: 1, InferredEntityCount: 1, InferredOnlyCommunities: []int{7}}, + {Ref: "pr-b", Communities: []int{7, 9}, CommunityDataAvailable: true, + OverlayEntityCount: 1, InferredEntityCount: 1, InferredOnlyCommunities: []int{7}}, + }) + + if res.RiskyPairs != 1 { + t.Fatalf("want 1 risky pair sharing community 7, got %+v", res.Pairs) + } + // The verdict-level flag is correctly false — measured entities exist — which + // is exactly why it cannot be the only signal. + if res.CommunityDataInferredOnly { + t.Fatalf("precondition: both refs have measured entities, so the verdict is not inferred-only") + } + p := res.Pairs[0] + if !reflect.DeepEqual(p.InferredSharedCommunities, []int{7}) { + t.Errorf("inferred_shared_communities = %v, want [7]", p.InferredSharedCommunities) + } + if !p.InferredOnly { + t.Errorf("the ONLY shared community is inferred on both sides, so this reported conflict is " + + "entirely manufactured — risk_pairs[].inferred_only = false lets an agent read it as measured") + } + if res.InferredOnlyPairCount != 1 { + t.Errorf("InferredOnlyPairCount = %d, want 1", res.InferredOnlyPairCount) + } +} + +// One inferred side is enough to make the OVERLAP deduced: the community was +// never observed on that ref, so the conflict was not observed either. +func TestAnalyzeMergeRisk_OneInferredSideMarksTheSharedCommunity(t *testing.T) { + res := AnalyzeMergeRisk([]ChangeImpact{ + {Ref: "pr-a", Communities: []int{7}, CommunityDataAvailable: true, OverlayEntityCount: 1}, + {Ref: "pr-b", Communities: []int{7}, CommunityDataAvailable: true, + InferredEntityCount: 1, InferredOnlyCommunities: []int{7}}, + }) + if len(res.Pairs) != 1 || !res.Pairs[0].InferredOnly { + t.Errorf("pr-b reaches community 7 only by inference, so the overlap is deduced: %+v", res.Pairs) + } +} + +// A pair that ALSO shares a measured community is not "inferred only" — the +// conflict stands on its own — but the deduced community is still named. +func TestAnalyzeMergeRisk_PartlyMeasuredOverlapIsNotInferredOnly(t *testing.T) { + res := AnalyzeMergeRisk([]ChangeImpact{ + {Ref: "pr-a", Communities: []int{5, 7}, CommunityDataAvailable: true, + OverlayEntityCount: 1, InferredEntityCount: 1, InferredOnlyCommunities: []int{7}}, + {Ref: "pr-b", Communities: []int{5, 7}, CommunityDataAvailable: true, OverlayEntityCount: 2}, + }) + if len(res.Pairs) != 1 { + t.Fatalf("want 1 pair, got %+v", res.Pairs) + } + p := res.Pairs[0] + if p.InferredOnly { + t.Errorf("community 5 is measured on both sides; the conflict is real: %+v", p) + } + if !reflect.DeepEqual(p.InferredSharedCommunities, []int{7}) { + t.Errorf("inferred_shared_communities = %v, want [7] — the deduced overlap must still be named", + p.InferredSharedCommunities) + } + if res.InferredOnlyPairCount != 0 { + t.Errorf("InferredOnlyPairCount = %d, want 0", res.InferredOnlyPairCount) + } +} + // An UNAVAILABLE verdict is not an inferred one. #6006's decline must not start // wearing #6042's confidence marker. func TestAnalyzeMergeRisk_UnavailableIsNotInferredOnly(t *testing.T) { @@ -422,19 +842,37 @@ func TestAnalyzeMergeRisk_UnavailableIsNotInferredOnly(t *testing.T) { // Cost guard: inference is a bounded add-on, not a graph walk. It touches only // the changed entities' own files/modules/outbound edges, so a large graph with // a small change set must not pay for it. +// +// The fixture ASSERTS that inference actually fires before timing anything. An +// earlier version of this benchmark spread every file and module evenly across +// communities, so both signals abstained and it timed the DECLINE path while +// claiming to measure inference. func BenchmarkAnalyzePRImpact_Inference(b *testing.B) { const n = 20000 ents := make([]Entity, 0, n+1) - rels := make([]Relationship, 0, n) + rels := make([]Relationship, 0, n+2) for i := 0; i < n; i++ { - f := "pkg" + string(rune('a'+i%20)) + "/f.go" - ents = append(ents, placedEnt("e"+itoaBench(i), f, "pkg"+string(rune('a'+i%20)), i%50)) + // One file per entity, one module per 1000 entities, and a module maps to + // exactly one community — so the module signal is concentrated and fires. + mod := "pkg" + itoaBench(i/1000) + ents = append(ents, placedEnt("e"+itoaBench(i), mod+"/f"+itoaBench(i)+".go", mod, i/1000)) if i > 0 { rels = append(rels, Relationship{FromID: "e" + itoaBench(i), ToID: "e" + itoaBench(i-1), Kind: "CALLS"}) } } - ents = append(ents, newEnt("new:x", "pkga/f.go", "pkga")) + // A new file in an existing module, calling two placed entities inside it. + ents = append(ents, newEnt("new:x", "pkg0/brand_new.go", "pkg0")) + rels = append(rels, + Relationship{FromID: "new:x", ToID: "e1", Kind: "CALLS"}, + Relationship{FromID: "new:x", ToID: "e2", Kind: "CALLS"}) change := ChangeSet{Added: []DiffEntityEntry{{ID: "new:x"}}} + + probe := AnalyzePRImpact(ents, rels, change, DefaultPRImpactOptions()) + if len(probe.ChangedEntities) != 1 || + probe.ChangedEntities[0].CommunitySource != CommunitySourceInferred { + b.Fatalf("benchmark fixture does not exercise inference: %+v", probe.ChangedEntities) + } + b.ResetTimer() for i := 0; i < b.N; i++ { _ = AnalyzePRImpact(ents, rels, change, DefaultPRImpactOptions()) diff --git a/internal/graph/pr_impact_infer.go b/internal/graph/pr_impact_infer.go index 8bc8d54f7..cb02651b9 100644 --- a/internal/graph/pr_impact_infer.go +++ b/internal/graph/pr_impact_infer.go @@ -1,5 +1,5 @@ // pr_impact_infer.go — issue #6042: infer a community for a changed entity the -// group-algo overlay cannot place, from the entities around it that it CAN. +// group-algo partition has NEVER SEEN, from the entities around it that it has. // // WHY THIS EXISTS. The overlay is computed from the last indexed group union, so // an entity that a PR ADDS is absent from it by construction. #6006 made that @@ -9,126 +9,205 @@ // // WHAT IT MUST NOT BECOME. Inference presented as fact is the #6006 defect one // layer up: a confident answer the caller cannot tell from a measured one. So -// every inferred placement is labelled at the entity level -// (ChangedEntity.CommunitySource) and aggregated at the verdict level -// (CommunityDataInferredOnly / InferredEntityCount), and inference that fails -// falls back to the existing decline path unchanged. +// every inferred placement is labelled per entity (CommunitySource), carries its +// own margin (CommunityInference), is tracked per community (ImpactedCommunity. +// InferredOnly) and per merge-risk pair (MergeRiskPair.InferredOnly), and +// inference that fails falls back to the existing decline path unchanged. +// +// ── Who is a candidate ────────────────────────────────────────────────────── +// +// ONLY entities ABSENT from the overlay — Entity.CommunityID == nil. This is not +// the same as communityOf(e) < 0, and the difference matters: +// +// nil the partition has never seen this entity: it did not +// exist at the last group index. THIS is #6042. +// non-nil, negative (-2) the partition DID see it and declined to place it +// (groupalgo writes -2 for "not assigned a community"; +// legacy graph.json can carry -1 directly). +// +// Inferring for the second class would override the group algorithm's own +// decision with a path-prefix heuristic — a strictly worse answer than the one +// community detection already gave, wearing a confident label. See +// TestAnalyzePRImpact_NegativeCommunityIDIsNotInferred. // // ── The signals, and why these three ──────────────────────────────────────── // -// Only signals the graph actually carries at this layer were used: +// container (primary) — the placed entities sharing the new entity's +// SourceFile, plus any placed CONTAINS parent. A file is the smallest unit +// community detection would essentially never split, so a new function in +// an already-placed file is in that file's community with high confidence. +// +// module (fallback) — Properties["module"], stamped on both the full +// (cmd/grafel/index.go) and incremental (extractors/incremental.go) paths +// and round-tripped through graph.fb (load.go restores the FB scalar into +// props). It votes ONLY when the container abstains, because it is not an +// independent signal: module is module.Derive(SourceFile), a pure function +// of the path, so the module histogram is a strict SUPERSET of the file +// histogram. Letting both vote would count one measurement twice, and a +// file-vs-module disagreement is never "two sources disagree" — it is "my +// file leans X while the wider directory leans Y", where the file is +// strictly the more specific evidence. // -// container (weight 3) — the entities sharing the new entity's SourceFile, plus -// any CONTAINS parent. A new function in an already-placed file is in that -// file's community with very high confidence; a file is the smallest unit -// Louvain would essentially never split. -// module (weight 2) — Properties["module"], stamped on both the full -// (cmd/grafel/index.go) and incremental (internal/extractors/incremental.go) -// paths and round-tripped through graph.fb (load.go restores the FB scalar -// into props). It is a depth-capped path rollup, so it is a COARSER version -// of the container signal rather than an independent one — hence a lower -// weight, and hence it can never outvote the file on its own. -// targets (weight 1) — placed entities the new entity calls/references. This is -// closest to what Louvain itself would have seen. It requires at least -// minPlacedTargets placed targets: one outbound edge is not evidence -// (everything calls a logger), and a single-edge rule would place half of -// every new package into whatever community the nearest shared utility -// happens to sit in. +// module.Derive is also weak on its own: MarkerFileNames has no per-Go- +// package marker, so a single-module Go repo falls through to DefaultDepth +// and everything under (say) internal/graph/** shares one label. A bare +// plurality over such a bucket (26 of 51) is not a community signal, so the +// module vote additionally requires a real sample AND real concentration. // -// ── The decision rule ─────────────────────────────────────────────────────── +// call targets — placed entities the new entity CALLS/USES/EXTENDS/…, over an +// ALLOWLIST of edge kinds (inferTargetKinds). This is the only signal not +// derived from the file path, and the closest to what community detection +// itself would have seen. IMPORTS is excluded: importing two placed +// packages is universal, so it would clear the >= 2 threshold for +// essentially every new file while carrying almost no community +// information. CONTAINS is excluded because it IS the container signal and +// must not vote twice under a second name. // -// Each signal casts ONE vote, for the strict plurality of its own evidence; a -// signal with no unique winner (a file split 1-1 across communities) abstains -// entirely rather than letting map order decide. A community is then inferred -// only when its weight is STRICTLY GREATER than the total weight of every -// dissenting vote. So: +// ── The decision rule: unanimity ──────────────────────────────────────────── // -// container alone -> infer (3 > 0) -// container 7 vs module 9 -> infer 7 (3 > 2; container is weighted highest) -// container 7 vs module 9 + targets 9 -> DECLINE (3 is not > 3) -// module alone / targets alone -> infer (2 > 0 / 1 > 0) -// nothing available -> DECLINE +// At most two votes are ever cast — the container-or-module primary, and the +// call targets — and BOTH must agree: +// +// primary only -> infer +// targets only -> infer +// primary + targets agree -> infer (the strongest case) +// primary + targets differ -> DECLINE +// nothing votes -> DECLINE +// +// This is the issue's own rule ("no inference at all when signals disagree"), +// and it is deliberately arithmetic-free. An earlier cut scored the signals +// 3/2/1 and required the winner to outweigh all dissent; once module is +// subordinate to the container that comparison can never actually block +// anything, so it was a guard held by nothing — exactly the kind of untested +// branch this feature must not ship. Each signal also abstains internally when +// its own evidence has no unique winner (a file split 1-1), rather than letting +// map iteration order decide. // // ── What is deliberately NOT inferred ─────────────────────────────────────── // -// - Overlay-placed entities. The overlay is ground truth and always wins. -// - Removed entities. They are absent from the head graph, so they have no -// neighbours; the diff record is not a community signal. -// - Blast-radius entities. Only the CHANGED set drives the verdict, and -// inferring for a potentially unbounded downstream set would cost far more -// than it is worth. +// - Entities the overlay placed (it is ground truth) or explicitly declined to +// place (see "Who is a candidate"). +// - Removed entities: absent from the head graph, so they have no neighbours; +// the diff record is not a community signal. +// - Blast-radius entities: only the CHANGED set drives the verdict, and +// inferring for an unbounded downstream set would cost far more than it is +// worth. // - Chained inference. Only OVERLAY-placed entities vote. An inferred -// placement is a guess, and letting guesses vote would propagate one weak -// signal across an entire new package — and would make the result depend on -// the order entities were processed in. +// placement is a guess; letting guesses vote would propagate one weak signal +// across an entire new package and make the result depend on processing +// order. // // ── Cost ──────────────────────────────────────────────────────────────────── // -// Nothing is built unless at least one changed entity is present in the head -// graph AND unplaced. When that holds, the added work is: -// -// one pass over `entities`, filtered to the (small) set of files and modules -// the unplaced changed entities live in — O(N) comparisons, allocations bounded -// by the changed set, not by the graph; and -// outbound/CONTAINS edges captured during the adjacency pass AnalyzePRImpact -// already makes, and only for changed ids — O(E) comparisons, O(deg(changed)) -// memory. -// -// No BFS, no transitive walk. Next to the 31.9 MB overlay parse the handler -// already performs (memoized on path+mtime+size), this is noise. +// Nothing is built unless a changed entity is present in the head graph AND +// absent from the overlay. Then: one pass over `entities` filtered to the small +// set of files and modules those entities occupy, plus outbound/CONTAINS edges +// captured during the adjacency pass AnalyzePRImpact already makes, for changed +// ids only. No BFS, no transitive walk. See BenchmarkAnalyzePRImpact_Inference, +// whose fixture ASSERTS that inference actually fires before timing it. package graph -// CommunitySource labels HOW a changed entity's community was determined. -// It is the whole point of #6042: an inferred placement that a caller cannot -// distinguish from a measured one is worse than no placement at all. +// CommunitySource labels HOW a changed entity's community was determined. It is +// the whole point of #6042: an inferred placement a caller cannot distinguish +// from a measured one is worse than no placement at all. const ( // CommunitySourceOverlay — measured. The group-algo overlay placed this // entity directly; it existed at the last group index. CommunitySourceOverlay = "overlay" - // CommunitySourceInferred — a guess, from placed neighbours. Good enough to + // CommunitySourceInferred — deduced from placed neighbours. Good enough to // triage merge risk with, not good enough to present as measured. CommunitySourceInferred = "inferred" - // CommunitySourceNone — not placed and not inferrable. These entities are the - // ones #6006's decline path exists for. + // CommunitySourceNone — not placed and not inferrable. These entities are + // what #6006's decline path exists for. CommunitySourceNone = "none" ) +// Signal names as they appear in CommunityInference.Signals. const ( - inferWeightContainer = 3 - inferWeightModule = 2 - inferWeightTargets = 1 + inferSignalContainer = "container" + inferSignalModule = "module" + inferSignalTargets = "call_targets" +) - // minPlacedTargets is how many placed outbound targets the weakest signal - // needs before it votes at all. See the package comment: one edge is not - // evidence, and this threshold is also what keeps #6006's add-only decline - // (a new entity calling exactly one placed entity) binding. +const ( + // minPlacedTargets is how many placed outbound targets the call-target signal + // needs before it votes. One edge is not evidence, and this threshold is also + // what keeps #6006's add-only decline (a new entity calling exactly one placed + // entity) binding. minPlacedTargets = 2 + // minModuleSample / minModuleConcentration bound the module fallback: at + // least this many placed entities in the module, and at least this fraction + // of them in the winning community. A 26/51 plurality over a depth-capped + // path bucket is not a community signal; 40/41 is. + minModuleSample = 3 + minModuleConcentration = 0.7 ) -// communityInferrer holds the (small) indexes needed to place unplaced changed -// entities. Zero value infers nothing, which is what callers get when every -// changed entity was already placed. +// inferTargetKinds is the allowlist of edge kinds the call-target signal reads. +// Containment edges (CONTAINS — that is the container signal) and package-level +// edges (IMPORTS, DEPENDS_ON — universal, and already what the module signal +// measures) are excluded on purpose; see the package comment. +var inferTargetKinds = map[string]struct{}{ + "CALLS": {}, + "REFERENCES": {}, + "USES": {}, + "USES_HOOK": {}, + "EXTENDS": {}, + "IMPLEMENTS": {}, + "INJECTED_INTO": {}, + "RETURNS": {}, + "ACCEPTS_INPUT": {}, +} + +// isInferTargetKind reports whether an edge kind carries community signal for +// the call-target vote. +func isInferTargetKind(kind string) bool { + _, ok := inferTargetKinds[kind] + return ok +} + +// overlayAbsent reports whether the group-algo partition has never seen this +// entity — the only class #6042 infers for. See the package comment. +func overlayAbsent(e Entity) bool { return e.CommunityID == nil } + +// CommunityInference is the provenance of ONE inferred placement: which signals +// voted, and how strong the deciding signal's evidence was. Without the margin a +// 26-of-51 plurality and a 40-of-41 consensus are indistinguishable on the wire, +// and an agent cannot weigh them differently. +type CommunityInference struct { + // Signals that voted, primary first. Two entries means the container-or-module + // primary and the call targets independently agreed — the strongest case. + Signals []string `json:"signals"` + // Support / Sample are the deciding signal's placed neighbours backing the + // chosen community, out of those it considered. + Support int `json:"support"` + Sample int `json:"sample"` +} + +// communityInferrer holds the (small) indexes needed to place overlay-absent +// changed entities. A nil inferrer infers nothing, which is what callers get +// when every changed entity was already placed — or explicitly declined — by the +// partition. type communityInferrer struct { byID map[string]Entity // byFile/byModule are community histograms over OVERLAY-PLACED entities, - // restricted to the files/modules the unplaced changed entities occupy. + // restricted to the files/modules the candidate entities occupy. byFile map[string]map[int]int byModule map[string]map[int]int - // parents[id] = CONTAINS parents of id; targets[id] = outbound edge targets of - // id. Populated only for unplaced changed ids. + // parents[id] = CONTAINS parents of id; targets[id] = allowlisted outbound + // edge targets of id. Populated only for candidate ids. parents map[string][]string targets map[string][]string } -// newCommunityInferrer builds the indexes for `want` — the unplaced changed -// entity ids that are present in the head graph. Returns the zero inferrer when -// there is nothing to infer, so the O(N) entity pass is skipped entirely. +// newCommunityInferrer builds the indexes for `want` — the overlay-absent +// changed entity ids present in the head graph. Returns nil when there is +// nothing to infer, so the O(N) entity pass is skipped entirely. func newCommunityInferrer(entities []Entity, byID map[string]Entity, want map[string]struct{}, parents, targets map[string][]string) *communityInferrer { if len(want) == 0 { return nil } - // Which files/modules do we actually care about? Usually a handful. wantFiles := make(map[string]struct{}, len(want)) wantModules := make(map[string]struct{}, len(want)) for id := range want { @@ -140,9 +219,6 @@ func newCommunityInferrer(entities []Entity, byID map[string]Entity, want map[st wantModules[m] = struct{}{} } } - // Also index the files/modules of the CONTAINS parents, so a parent that - // lives elsewhere still contributes through its own community (below we read - // the parent's community directly, so no extra file bookkeeping is needed). ci := &communityInferrer{ byID: byID, byFile: make(map[string]map[int]int, len(wantFiles)), @@ -150,13 +226,13 @@ func newCommunityInferrer(entities []Entity, byID map[string]Entity, want map[st parents: parents, targets: targets, } - if len(wantFiles) == 0 && len(wantModules) == 0 && len(parents) == 0 && len(targets) == 0 { - return ci // nothing to histogram; target/parent votes still work + if len(wantFiles) == 0 && len(wantModules) == 0 { + return ci // no path signals possible; target/parent votes still work } for i := range entities { c := communityOf(entities[i]) if c < 0 { - continue // only OVERLAY-placed entities vote — no chained inference + continue // only OVERLAY-PLACED entities vote — no chained inference } if f := entities[i].SourceFile; f != "" { if _, ok := wantFiles[f]; ok { @@ -183,56 +259,75 @@ func addVote(dst map[string]map[int]int, key string, community int) { h[community]++ } -// infer returns the community inferred for id, and whether inference succeeded. -// See the package comment for the rule; the short version is that each signal -// casts one weighted vote and the winner must strictly outweigh all dissent. -func (ci *communityInferrer) infer(id string) (int, bool) { +// signalVote is one signal's opinion, with the evidence behind it. +type signalVote struct { + name string + community int + support int + sample int +} + +// infer returns the inferred community for id and its provenance. Unanimity: the +// container-or-module primary and the call targets must not contradict each +// other, and at least one must vote. See the package comment. +func (ci *communityInferrer) infer(id string) (int, *CommunityInference, bool) { if ci == nil { - return -1, false + return -1, nil, false } e, ok := ci.byID[id] if !ok { - return -1, false // removed entity: no head-graph neighbours to read + return -1, nil, false // removed entity: no head-graph neighbours to read } - votes := map[int]int{} - if c, ok := ci.containerVote(id, e); ok { - votes[c] += inferWeightContainer - } - if m := e.PropGet("module"); m != "" { - if c, ok := plurality(ci.byModule[m], 1); ok { - votes[c] += inferWeightModule - } - } - if c, ok := ci.targetVote(id); ok { - votes[c] += inferWeightTargets - } - if len(votes) == 0 { - return -1, false + // Primary: the containing component, falling back to the module ONLY when the + // container has NO EVIDENCE — module is a coarser view of the same path, so + // the two must never both vote. + // + // The distinction between "no evidence" and "contradictory evidence" is + // load-bearing. A file split 1-1 across two communities has spoken: this + // location is genuinely ambiguous. Falling through to the module then asks a + // SUPERSET of that same evidence — the file's entities are inside the + // module's sample — and gets a confident answer purely because the wider + // bucket dilutes the contradiction. That is manufacturing agreement. + primary, hasPrimary, containerHadEvidence := ci.containerVote(id, e) + switch { + case hasPrimary: + case containerHadEvidence: + return -1, nil, false // the container looked and found a contradiction + default: + primary, hasPrimary = ci.moduleVote(e) } + targets, hasTargets := ci.targetVote(id) - best, bestW, total := -1, 0, 0 - tied := false - for c, w := range votes { - total += w - switch { - case w > bestW: - best, bestW, tied = c, w, false - case w == bestW: - tied = true + switch { + case hasPrimary && hasTargets: + if primary.community != targets.community { + return -1, nil, false // signals disagree — decline rather than guess } + return primary.community, &CommunityInference{ + Signals: []string{primary.name, targets.name}, + Support: primary.support, + Sample: primary.sample, + }, true + case hasPrimary: + return primary.community, &CommunityInference{ + Signals: []string{primary.name}, Support: primary.support, Sample: primary.sample, + }, true + case hasTargets: + return targets.community, &CommunityInference{ + Signals: []string{targets.name}, Support: targets.support, Sample: targets.sample, + }, true } - // Strictly greater than the sum of all dissent: 2*bestW > total. - if tied || 2*bestW <= total { - return -1, false - } - return best, true + return -1, nil, false } -// containerVote combines the entity's own file with any CONTAINS parents — the -// "containing component" signal. Both describe the same thing (what physically -// encloses the entity) so they share one vote rather than double-counting. -func (ci *communityInferrer) containerVote(id string, e Entity) (int, bool) { +// containerVote combines the entity's own file with any CONTAINS parents — both +// describe what physically encloses the entity, so they share one vote. +// +// The third return says whether the container had ANY placed evidence to look +// at, which is what lets infer() tell "the file is new" (fall through to the +// module) from "the file is contradictory" (decline outright). +func (ci *communityInferrer) containerVote(id string, e Entity) (signalVote, bool, bool) { var hist map[int]int if base := ci.byFile[e.SourceFile]; e.SourceFile != "" && len(base) > 0 { hist = make(map[int]int, len(base)+1) @@ -252,18 +347,37 @@ func (ci *communityInferrer) containerVote(id string, e Entity) (int, bool) { hist[c]++ } } - return plurality(hist, 1) + c, support, sample, ok := plurality(hist, 1) + return signalVote{inferSignalContainer, c, support, sample}, ok, len(hist) > 0 } -// targetVote is the plurality community of the entity's placed outbound targets, -// requiring at least minPlacedTargets of them. -func (ci *communityInferrer) targetVote(id string) (int, bool) { +// moduleVote is the fallback prior. It demands a real sample AND real +// concentration, because module is a depth-capped path rollup that can cover a +// large, heterogeneous slice of a repo — a bare plurality over such a bucket is +// noise wearing a community id. +func (ci *communityInferrer) moduleVote(e Entity) (signalVote, bool) { + m := e.PropGet("module") + if m == "" { + return signalVote{}, false + } + c, support, sample, ok := plurality(ci.byModule[m], minModuleSample) + if !ok { + return signalVote{}, false + } + if float64(support)/float64(sample) < minModuleConcentration { + return signalVote{}, false + } + return signalVote{inferSignalModule, c, support, sample}, true +} + +// targetVote is the plurality community of the entity's placed outbound targets +// over the allowlisted edge kinds, requiring at least minPlacedTargets of them. +func (ci *communityInferrer) targetVote(id string) (signalVote, bool) { tgts := ci.targets[id] if len(tgts) < minPlacedTargets { - return -1, false + return signalVote{}, false } hist := map[int]int{} - placed := 0 for _, tid := range tgts { t, ok := ci.byID[tid] if !ok { @@ -271,23 +385,21 @@ func (ci *communityInferrer) targetVote(id string) (int, bool) { } if c := communityOf(t); c >= 0 { hist[c]++ - placed++ } } - if placed < minPlacedTargets { - return -1, false - } - return plurality(hist, minPlacedTargets) + c, support, sample, ok := plurality(hist, minPlacedTargets) + return signalVote{inferSignalTargets, c, support, sample}, ok } -// plurality returns the uniquely most-common community in hist, provided it has -// at least `min` votes. A tie ABSTAINS: with no unique winner the answer would -// otherwise be decided by map iteration order, which is both non-deterministic -// and, worse, an invented placement. -func plurality(hist map[int]int, min int) (int, bool) { - best, bestN := -1, 0 +// plurality returns the uniquely most-common community in hist, with its support +// and the total sample, provided the sample is at least minSample. A tie +// ABSTAINS: with no unique winner the answer would be decided by map iteration +// order — non-deterministic, and an invented placement. +func plurality(hist map[int]int, minSample int) (community, support, sample int, ok bool) { + best, bestN, total := -1, 0, 0 tied := false for c, n := range hist { + total += n switch { case n > bestN: best, bestN, tied = c, n, false @@ -295,8 +407,8 @@ func plurality(hist map[int]int, min int) (int, bool) { tied = true } } - if best < 0 || bestN < min || tied { - return -1, false + if best < 0 || total < minSample || tied { + return -1, 0, 0, false } - return best, true + return best, bestN, total, true } diff --git a/internal/mcp/pr_impact_6042_test.go b/internal/mcp/pr_impact_6042_test.go index 8ab454499..55965fa62 100644 --- a/internal/mcp/pr_impact_6042_test.go +++ b/internal/mcp/pr_impact_6042_test.go @@ -13,6 +13,7 @@ import ( "encoding/json" "os" "path/filepath" + "reflect" "strings" "testing" "time" @@ -65,10 +66,11 @@ func setupPRImpact6042(t *testing.T) prImpact6042Env { prImpact6042Ent("svc:A", "core/a.go", "core"), prImpact6042Ent("svc:B", "core/b.go", "core"), prImpact6042Ent("svc:C", "util/c.go", "util"), + prImpact6042Ent("svc:D", "extra/d.go", "extra"), } } - // mk builds a ref graph: the base three entities plus zero or one added - // entity, with the given outbound CALLS edges. + // mk builds a ref graph: the base entities plus zero or one added entity, + // with the given outbound CALLS edges. mk := func(added graph.Entity, calls ...string) *graph.Document { d := &graph.Document{Version: 1, Repo: "svc", Entities: base()} if added.ID != "" { @@ -80,11 +82,10 @@ func setupPRImpact6042(t *testing.T) prImpact6042Env { } return d } - // modA perturbs svc:A's signature so DiffDocs classifies it as MODIFIED — - // the measured control arm. - modA := func() *graph.Document { - d := mk(graph.Entity{}) - d.Entities[0].Signature = "func A(x int)" + // modify perturbs one base entity's signature so DiffDocs classifies it as + // MODIFIED — a MEASURED changed entity, since the overlay covers it. + modify := func(d *graph.Document, idx int, sig string) *graph.Document { + d.Entities[idx].Signature = sig return d } @@ -97,8 +98,16 @@ func setupPRImpact6042(t *testing.T) prImpact6042Env { "viaTargets": mk(prImpact6042Ent("svc:NewT", "newpkg/t.go", "newpkg"), "svc:A", "svc:B"), // Nothing to infer from: new file, new module, no edges. "isolated": mk(prImpact6042Ent("svc:NewI", "lonely/i.go", "lonely")), - // Measured control arm. - "modA": modA(), + // Measured control arms: both modify an overlay-covered entity in + // community 7, so their overlap is observed on BOTH sides. + "modA": modify(mk(graph.Entity{}), 0, "func A(x int)"), + "modB": modify(mk(graph.Entity{}), 1, "func B(x int)"), + // The D1 shape: each ref has a MEASURED community of its own (9 / 5) plus + // an added entity inferred into 7. Their ONLY overlap is 7 — the inferred + // one — so the conflict is manufactured while every aggregate flag reads + // "measured". + "mixedA": modify(mk(prImpact6042Ent("svc:NewMA", "core/a.go", "core")), 2, "func C(x int)"), + "mixedB": modify(mk(prImpact6042Ent("svc:NewMB", "core/b.go", "core")), 3, "func D(x int)"), } for ref, doc := range refs { dir := daemon.StateDirForRepoRef(repoPath, ref) @@ -153,8 +162,10 @@ func (e prImpact6042Env) writeOverlay(t *testing.T) { "svc:A": {CommunityID: 7, PageRank: 0.1}, "svc:B": {CommunityID: 7, PageRank: 0.1}, "svc:C": {CommunityID: 9, PageRank: 0.1}, + "svc:D": {CommunityID: 5, PageRank: 0.1}, }, Communities: []graph.CommunityResult{ + {ID: 5, Size: 1, AutoName: "extra"}, {ID: 7, Size: 2, AutoName: "core"}, {ID: 9, Size: 1, AutoName: "util"}, }, @@ -183,28 +194,44 @@ func (e prImpact6042Env) single(t *testing.T, head string) *mcpapi.CallToolResul } type prImpact6042Payload struct { - RiskyPairCount int `json:"risky_pair_count"` - CommunityDataAvailable bool `json:"community_data_available"` - CommunityDataInferredOnly bool `json:"community_data_inferred_only"` - InferredEntityCount int `json:"inferred_entity_count"` - ChangedCount int `json:"changed_count"` - ChangedUncovered int `json:"changed_entities_without_community"` - ChangedOverlay int `json:"changed_entities_with_overlay_community"` - ChangedInferred int `json:"changed_entities_with_inferred_community"` + RiskyPairCount int `json:"risky_pair_count"` + CommunityDataAvailable bool `json:"community_data_available"` + CommunityDataInferredOnly bool `json:"community_data_inferred_only"` + InferredEntityCount int `json:"inferred_entity_count"` + InferredOnlyPairCount int `json:"inferred_only_pair_count"` + CommunityDataNote string `json:"community_data_note"` + RefsWithInferred []string `json:"refs_with_inferred_community_data"` + ChangedCount int `json:"changed_count"` + ChangedUncovered int `json:"changed_entities_without_community"` + ChangedOverlay int `json:"changed_entities_with_overlay_community"` + ChangedInferred int `json:"changed_entities_with_inferred_community"` RiskPairs []struct { - SharedCommunities []int `json:"shared_communities"` + SharedCommunities []int `json:"shared_communities"` + InferredSharedCommunities []int `json:"inferred_shared_communities"` + InferredOnly bool `json:"inferred_only"` } `json:"risk_pairs"` PerRef []struct { - Ref string `json:"ref"` - ChangedOverlay int `json:"changed_entities_with_overlay_community"` - ChangedInferred int `json:"changed_entities_with_inferred_community"` - Uncovered int `json:"changed_entities_without_community"` + Ref string `json:"ref"` + ImpactedCommunities []int `json:"impacted_communities"` + InferredCommunities []int `json:"inferred_communities"` + ChangedOverlay int `json:"changed_entities_with_overlay_community"` + ChangedInferred int `json:"changed_entities_with_inferred_community"` + Uncovered int `json:"changed_entities_without_community"` } `json:"per_ref"` ChangedEntities []struct { - ID string `json:"id"` - CommunityID int `json:"community_id"` - CommunitySource string `json:"community_source"` + ID string `json:"id"` + CommunityID int `json:"community_id"` + CommunitySource string `json:"community_source"` + CommunityInference *struct { + Signals []string `json:"signals"` + Support int `json:"support"` + Sample int `json:"sample"` + } `json:"community_inference"` } `json:"changed_entities"` + ImpactedCommunities []struct { + CommunityID int `json:"community_id"` + InferredOnly bool `json:"inferred_only"` + } `json:"impacted_communities"` } func must6042Payload(t *testing.T, res *mcpapi.CallToolResult) prImpact6042Payload { @@ -251,6 +278,124 @@ func TestPRImpact6042_AddOnlyRefsGetAnInferredVerdict(t *testing.T) { t.Errorf("per_ref %s = overlay %d / inferred %d / none %d, want 0/1/0", r.Ref, r.ChangedOverlay, r.ChangedInferred, r.Uncovered) } + // Conflicts mode emits no changed_entities, so this is the ONLY place a + // caller can see which communities this ref reached by inference. + if len(r.InferredCommunities) != 1 || r.InferredCommunities[0] != 7 { + t.Errorf("per_ref %s inferred_communities = %v, want [7]", r.Ref, r.InferredCommunities) + } + } + // The pair itself must be marked, not just the verdict. + if !p.RiskPairs[0].InferredOnly || len(p.RiskPairs[0].InferredSharedCommunities) != 1 { + t.Errorf("the shared community is inferred on both sides; risk pair must say so: %+v", + p.RiskPairs[0]) + } + if p.InferredOnlyPairCount != 1 { + t.Errorf("inferred_only_pair_count = %d, want 1", p.InferredOnlyPairCount) + } + // Structured flags are the contract, but the prose note is what stops an + // agent reading the verdict as a measurement. + if !strings.Contains(strings.ToLower(p.CommunityDataNote), "inferred") { + t.Errorf("community_data_note missing or unhelpful: %q", p.CommunityDataNote) + } + if !reflect.DeepEqual(p.RefsWithInferred, []string{"inFile", "viaTargets"}) { + t.Errorf("refs_with_inferred_community_data = %v, want [inFile viaTargets]", p.RefsWithInferred) + } +} + +// THE D1 REGRESSION, end to end. Both refs modify an overlay-covered entity, so +// both carry MEASURED communities and every aggregate flag reads "measured" — +// community_data_inferred_only is legitimately false. But their measured +// communities (9 and 5) do not overlap: the ONLY shared community is 7, which +// each ref reaches solely through an entity the partition has never seen. +// +// The reported conflict is therefore entirely manufactured, and a whole-verdict +// flag cannot say so. Without per-pair provenance an agent reads a fabricated +// conflict as measured — the #6006 defect class at the granularity a merge +// decision is actually made at. +func TestPRImpact6042_ConflictRestingOnlyOnInferenceIsMarkedPerPair(t *testing.T) { + env := setupPRImpact6042(t) + + p := must6042Payload(t, env.conflicts(t, "mixedA", "mixedB")) + + if p.RiskyPairCount != 1 || len(p.RiskPairs) != 1 { + t.Fatalf("want exactly 1 risky pair, got %+v", p) + } + pair := p.RiskPairs[0] + if !reflect.DeepEqual(pair.SharedCommunities, []int{7}) { + t.Fatalf("fixture must overlap on community 7 only, got %v — the test proves nothing otherwise", + pair.SharedCommunities) + } + // The precondition that makes this dangerous: the verdict looks measured. + if p.CommunityDataInferredOnly { + t.Fatalf("precondition: both refs modify overlay-covered entities, so the verdict "+ + "is not inferred-only; got %+v", p) + } + for _, r := range p.PerRef { + if r.ChangedOverlay != 1 || r.ChangedInferred != 1 { + t.Fatalf("precondition: per_ref %s must have 1 measured + 1 inferred entity, got %+v", r.Ref, r) + } + } + + if !pair.InferredOnly { + t.Errorf("every shared community is reached only by inference, so this reported conflict " + + "is DEDUCED — risk_pairs[].inferred_only = false lets an agent read it as measured") + } + if !reflect.DeepEqual(pair.InferredSharedCommunities, []int{7}) { + t.Errorf("inferred_shared_communities = %v, want [7]", pair.InferredSharedCommunities) + } + if p.InferredOnlyPairCount != 1 { + t.Errorf("inferred_only_pair_count = %d, want 1", p.InferredOnlyPairCount) + } + // And the note must fire even though the verdict-level flag is false. + if !strings.Contains(strings.ToLower(p.CommunityDataNote), "deduced") { + t.Errorf("a manufactured conflict must be called out in prose too; note = %q", p.CommunityDataNote) + } +} + +// The measured control arm for the pair marker: two refs that overlap on a +// community they BOTH really touch must not be marked deduced. Without this, +// "inferred_only: true" everywhere would satisfy the test above while carrying +// no information at all. +func TestPRImpact6042_MeasuredConflictIsNotMarkedInferred(t *testing.T) { + env := setupPRImpact6042(t) + + // modA modifies svc:A and modB modifies svc:B — both overlay-covered, both in + // community 7. Nothing here is inferred on either side. + p := must6042Payload(t, env.conflicts(t, "modA", "modB")) + if p.RiskyPairCount != 1 { + t.Fatalf("want 1 risky pair on community 7, got %+v", p) + } + if p.RiskPairs[0].InferredOnly || len(p.RiskPairs[0].InferredSharedCommunities) != 0 { + t.Errorf("both refs reach community 7 by MEASUREMENT; the overlap is real: %+v", p.RiskPairs[0]) + } + if p.InferredOnlyPairCount != 0 || p.InferredEntityCount != 0 { + t.Errorf("inferred_only_pair_count = %d / inferred_entity_count = %d, want 0/0", + p.InferredOnlyPairCount, p.InferredEntityCount) + } + if p.CommunityDataNote != "" { + t.Errorf("a fully measured conflict must carry no inference note: %q", p.CommunityDataNote) + } + for _, r := range p.PerRef { + if len(r.InferredCommunities) != 0 { + t.Errorf("per_ref %s inferred_communities = %v, want []", r.Ref, r.InferredCommunities) + } + } +} + +// One inferred SIDE is enough to make the overlap deduced: modA really touches +// community 7, but mixedA only gets there through an entity the partition has +// never seen, so the claim "these two refs collide in community 7" is half +// guess. It must be marked even though one side is solid. +func TestPRImpact6042_OneInferredSideStillMarksTheConflict(t *testing.T) { + env := setupPRImpact6042(t) + + p := must6042Payload(t, env.conflicts(t, "modA", "mixedA")) + if p.RiskyPairCount != 1 || !reflect.DeepEqual(p.RiskPairs[0].SharedCommunities, []int{7}) { + t.Fatalf("fixture must produce exactly one overlap, on community 7: %+v", p.RiskPairs) + } + if !p.RiskPairs[0].InferredOnly { + t.Errorf("mixedA reaches community 7 only by inference, so the OVERLAP was never observed: %+v", + p.RiskPairs[0]) } } @@ -316,6 +461,21 @@ func TestPRImpact6042_SingleModeLabelsInferencePerEntity(t *testing.T) { if got.ID != "svc:NewInA" || got.CommunityID != 7 || got.CommunitySource != "inferred" { t.Errorf("changed entity = %+v, want svc:NewInA / community 7 / source inferred", got) } + // The margin must survive to the wire: "inferred" alone cannot tell a file + // consensus from a coin flip. + if got.CommunityInference == nil || len(got.CommunityInference.Signals) == 0 || + got.CommunityInference.Sample == 0 { + t.Errorf("community_inference missing from the payload: %+v", got.CommunityInference) + } + if !strings.Contains(strings.ToLower(p.CommunityDataNote), "inferred") { + t.Errorf("single mode must carry the prose note when the whole verdict is inferred; got %q", + p.CommunityDataNote) + } + // The impacted community carries its own inference marker too. + if len(p.ImpactedCommunities) != 1 || !p.ImpactedCommunities[0].InferredOnly { + t.Errorf("impacted_communities must mark community 7 as inference-only: %+v", + p.ImpactedCommunities) + } if !p.CommunityDataAvailable || !p.CommunityDataInferredOnly { t.Errorf("want available=true inferred_only=true, got %v/%v", p.CommunityDataAvailable, p.CommunityDataInferredOnly) diff --git a/internal/mcp/pr_impact_tools.go b/internal/mcp/pr_impact_tools.go index fe5b952ba..c8a3e0fc3 100644 --- a/internal/mcp/pr_impact_tools.go +++ b/internal/mcp/pr_impact_tools.go @@ -215,6 +215,16 @@ const inferredOnlyNote = "every community placement behind this verdict was INFE "Treat this as a well-founded estimate of what community detection would say, not as a " + "measurement. Per-entity provenance is in changed_entities[].community_source." +// inferredPairsNote covers the narrower — and more dangerous — case (#6042 D1): +// the verdict as a whole rests partly on measured data, so every aggregate flag +// reads "measured", yet some reported CONFLICT exists only because one side was +// inferred into the shared community. The pair is where the merge decision is +// actually made, so this must be said even when the verdict-level flag is false. +const inferredPairsNote = "at least one reported conflict is DEDUCED, not observed: the refs " + + "overlap only on communities that one or both sides reached by inference from placed " + + "neighbours. See risk_pairs[].inferred_only and per_ref[].inferred_communities — the " + + "entities behind those communities are new, so the group-algo partition has never seen them." + // ── overlay read cache (one entry) ─────────────────────────────────────────── // // The overlay is read and unmarshalled on every grafel_pr_impact call, in both @@ -316,12 +326,14 @@ func (s *Server) prImpactConflicts(groupName, repoSlug, repoPath, base string, r } res := graph.AnalyzePRImpact(headDoc.Entities, headDoc.Relationships, change, opts) comms := res.ImpactedCommunityIDs() + inferredComms := res.InferredOnlyCommunityIDs() impacts = append(impacts, graph.ChangeImpact{ - Ref: ref, - Communities: comms, - CommunityDataAvailable: res.CommunityDataAvailable, - OverlayEntityCount: res.ChangedWithOverlayCommunity, - InferredEntityCount: res.ChangedWithInferredCommunity, + Ref: ref, + Communities: comms, + CommunityDataAvailable: res.CommunityDataAvailable, + OverlayEntityCount: res.ChangedWithOverlayCommunity, + InferredEntityCount: res.ChangedWithInferredCommunity, + InferredOnlyCommunities: inferredComms, }) perRef = append(perRef, map[string]any{ "ref": ref, @@ -336,6 +348,11 @@ func (s *Server) prImpactConflicts(groupName, repoSlug, repoPath, base string, r "changed_entities_with_overlay_community": res.ChangedWithOverlayCommunity, "changed_entities_with_inferred_community": res.ChangedWithInferredCommunity, "community_data_inferred_only": res.CommunityDataInferredOnly, + // #6042 D1 — WHICH of impacted_communities this ref reaches only by + // inference. Conflicts mode emits no changed_entities, so without this + // the per-entity community_source is invisible here and a caller cannot + // tell which side of an overlap was deduced. + "inferred_communities": inferredComms, }) } @@ -369,12 +386,20 @@ func (s *Server) prImpactConflicts(groupName, repoSlug, repoPath, base string, r // it did say. Zero risky pairs under that flag is a weaker all-clear. "inferred_entity_count": risk.InferredEntityCount, "community_data_inferred_only": risk.CommunityDataInferredOnly, + // #6042 D1 — how many of the reported conflicts are deduced rather than + // observed. This can be non-zero while community_data_inferred_only is + // false: both refs can carry measured communities and still overlap ONLY on + // an inferred one, which makes the reported conflict manufactured. + "inferred_only_pair_count": risk.InferredOnlyPairCount, } if len(risk.RefsWithInferredCommunityData) > 0 { out["refs_with_inferred_community_data"] = risk.RefsWithInferredCommunityData } - if risk.CommunityDataInferredOnly { + switch { + case risk.CommunityDataInferredOnly: out["community_data_note"] = inferredOnlyNote + case risk.InferredOnlyPairCount > 0: + out["community_data_note"] = inferredPairsNote } stamper.describeInto(out) return jsonResult(out), nil