diff --git a/internal/graph/pr_impact.go b/internal/graph/pr_impact.go index 5cadcecdf..3df586408 100644 --- a/internal/graph/pr_impact.go +++ b/internal/graph/pr_impact.go @@ -41,6 +41,21 @@ 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"` + // 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 @@ -49,6 +64,14 @@ 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). + 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 @@ -102,10 +125,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 +224,51 @@ 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 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 && overlayAbsent(e) { + inferCandidates[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 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(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 { continue @@ -197,35 +281,52 @@ 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(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 := inferCandidates[r.ToID]; ok { + inferParents[r.ToID] = append(inferParents[r.ToID], r.FromID) + } + } + } } + inferrer := newCommunityInferrer(entities, byID, inferCandidates, 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 detail *CommunityInference var name, kind, src string if e, ok := byID[id]; ok { comm = communityOf(e) + if comm >= 0 { + source = CommunitySourceOverlay + } 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 { // Removed entity (gone from HEAD) — fall back to the diff record. @@ -238,21 +339,33 @@ 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, + CommunityInference: detail, }) 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. @@ -339,10 +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], + CommunityID: c, + ChangedCount: communityChanged[c], + BlastRadiusHit: communityBlast[c], + InferredChangedCount: communityInferred[c], + InferredOnly: inferredOnlyCommunity, }) } // Rank by total touch (changed+blast) desc, then community id asc. @@ -366,6 +487,10 @@ func AnalyzePRImpact(entities []Entity, rels []Relationship, change ChangeSet, o CommunityDataAvailable: communityDataAvailable, ChangedWithoutCommunity: changedWithoutCommunity, + + ChangedWithOverlayCommunity: changedFromOverlay, + ChangedWithInferredCommunity: changedFromInference, + CommunityDataInferredOnly: inferredOnly, } } @@ -385,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 // --------------------------------------------------------------------------- @@ -402,6 +547,16 @@ 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 + // 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. @@ -410,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. @@ -428,6 +598,27 @@ 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"` + // 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 @@ -442,13 +633,20 @@ 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)) + 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 { @@ -457,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 @@ -467,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), }) } } @@ -485,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), @@ -492,6 +717,12 @@ 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, + 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 new file mode 100644 index 000000000..a078c441e --- /dev/null +++ b/internal/graph/pr_impact_6042_test.go @@ -0,0 +1,894 @@ +// 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 failure modes pinned here are symmetric: +// +// - 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 +// 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 ( + "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. +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 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 != "" { + 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, db:f 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), + 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"}, + } +} + +// 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 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() + 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 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) + } + // 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") + } + 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, 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, 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")) + + 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) + } + 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 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, + 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) + } + 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 ────────────────────────────────────── + +// 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 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") + } + if res.CommunityDataInferredOnly { + t.Errorf("no inference happened; CommunityDataInferredOnly must be false") + } +} + +// 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", ""), // 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 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") + } +} + +// 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. 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. 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()) + + 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) + } +} + +// ── 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. +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 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", ""), + 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") + } + // 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) + } +} + +// 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() + // 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"}}} + + 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) + } +} + +// 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) + } +} + +// 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 +// 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, + 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) + } + 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 || 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. + 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) + } +} + +// 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) { + // 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. +// +// 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+2) + for i := 0; i < n; i++ { + // 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"}) + } + } + // 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()) + } +} + +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..cb02651b9 --- /dev/null +++ b/internal/graph/pr_impact_infer.go @@ -0,0 +1,414 @@ +// pr_impact_infer.go — issue #6042: infer a community for a changed entity the +// 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 +// 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 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 ──────────────────────────────────────── +// +// 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. +// +// 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. +// +// 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. +// +// ── The decision rule: unanimity ──────────────────────────────────────────── +// +// 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 ─────────────────────────────────────── +// +// - 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; 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 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 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 — 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 + // what #6006's decline path exists for. + CommunitySourceNone = "none" +) + +// Signal names as they appear in CommunityInference.Signals. +const ( + inferSignalContainer = "container" + inferSignalModule = "module" + inferSignalTargets = "call_targets" +) + +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 +) + +// 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 candidate entities occupy. + byFile map[string]map[int]int + byModule map[string]map[int]int + // 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 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 + } + 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{}{} + } + } + 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 { + 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 + } + 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]++ +} + +// 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, nil, false + } + e, ok := ci.byID[id] + if !ok { + return -1, nil, false // removed entity: no head-graph neighbours to read + } + + // 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) + + 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 + } + return -1, nil, false +} + +// 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) + 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]++ + } + } + c, support, sample, ok := plurality(hist, 1) + return signalVote{inferSignalContainer, c, support, sample}, ok, len(hist) > 0 +} + +// 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 signalVote{}, false + } + hist := map[int]int{} + for _, tid := range tgts { + t, ok := ci.byID[tid] + if !ok { + continue + } + if c := communityOf(t); c >= 0 { + hist[c]++ + } + } + c, support, sample, ok := plurality(hist, minPlacedTargets) + return signalVote{inferSignalTargets, c, support, sample}, ok +} + +// 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 + case n == bestN: + tied = true + } + } + if best < 0 || total < minSample || tied { + return -1, 0, 0, false + } + return best, bestN, total, 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..55965fa62 --- /dev/null +++ b/internal/mcp/pr_impact_6042_test.go @@ -0,0 +1,533 @@ +// 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" + "reflect" + "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"), + prImpact6042Ent("svc:D", "extra/d.go", "extra"), + } + } + // 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 != "" { + 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 + } + // 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 + } + + 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 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) + 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}, + "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"}, + }, + } + 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"` + 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"` + InferredSharedCommunities []int `json:"inferred_shared_communities"` + InferredOnly bool `json:"inferred_only"` + } `json:"risk_pairs"` + PerRef []struct { + 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"` + 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 { + 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) + } + // 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]) + } +} + +// 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) + } + // 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) + } + 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..c8a3e0fc3 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,35 @@ 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." + +// 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 @@ -294,10 +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, + Ref: ref, + Communities: comms, + CommunityDataAvailable: res.CommunityDataAvailable, + OverlayEntityCount: res.ChangedWithOverlayCommunity, + InferredEntityCount: res.ChangedWithInferredCommunity, + InferredOnlyCommunities: inferredComms, }) perRef = append(perRef, map[string]any{ "ref": ref, @@ -308,6 +344,15 @@ 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, + // #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, }) } @@ -335,6 +380,26 @@ 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, + // #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 + } + 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