feat(#6042): infer a community for unplaced changed entities, labelled as inferred - #6043
Merged
Conversation
…d as inferred #6006 made grafel_pr_impact decline when NO changed entity could be placed in a community. Correct — with zero community signal any verdict is invented — but it makes the tool useless for the most ordinary PR shape there is: one that only adds code. A newly added entity is never in the group-algo overlay, because the overlay is computed from the last indexed group union and the entity did not exist then. So an add-only PR had every changed entity unplaced and got an explanatory IsError instead of a verdict. The information to do better is already in the graph. This infers a community for an unplaced changed entity from its PLACED neighbours — and labels it. ## The signals, and why these three Only signals the graph demonstrably carries at this layer were used: container (weight 3) — entities sharing the new entity's SourceFile, plus any CONTAINS parent. A file is the smallest unit Louvain would essentially never split, so a new function in a placed file is in that file's community with very high confidence. module (weight 2) — Properties["module"], stamped on the full (cmd/grafel/index.go) and incremental (extractors/incremental.go) paths and round-tripped through graph.fb (load.go restores the FB scalar into props). NOTE it is module.Derive — a depth-capped path rollup — so it is a COARSER view of the same fact the container signal reads, not an independent one. Hence the lower weight: it can never outvote the file on its own. targets (weight 1) — placed entities the new entity calls. Closest to what Louvain itself would have seen. Requires >= 2 placed targets: one outbound edge is not evidence (everything calls a logger), and a single-edge rule would place half of every new package into whatever community the nearest shared utility happens to sit in. Each signal casts ONE vote for the strict plurality of its own evidence, and abstains entirely when it has no unique winner (a file split 1-1 across communities) rather than letting map order decide. A community is inferred only when its weight strictly exceeds the total weight of all dissent: container alone -> infer (3 > 0) container 7 vs module 9 -> infer 7 (3 > 2 — container weighted highest) container 7 vs module 9 + targets 9 -> DECLINE (3 is not > 3) nothing available -> DECLINE Deliberately NOT inferred: overlay-placed entities (the overlay is ground truth and wins outright), removed entities (absent from the head graph, so no neighbours — the diff record is not a community signal), blast-radius entities (only the changed set drives the verdict), and chained inference (only OVERLAY-placed entities vote; letting guesses vote would propagate one weak signal across a whole new package and make the result order-dependent). ## Inference is never presented as fact Silent inference would be the #6006 defect one layer up. So: - ChangedEntity.CommunitySource = overlay | inferred | none, per entity, no omitempty — the label is always on the wire. - changed_entities_with_overlay_community / _with_inferred_community aggregate alongside the existing changed_entities_without_community; the three always sum to changed_count. - community_data_inferred_only at the verdict level (both modes), plus inferred_entity_count and refs_with_inferred_community_data in conflicts mode, and a plain-language community_data_note when the whole verdict is deduced. - ImpactedCommunity.inferred_changed_count shows how much of a community's involvement was deduced. - CommunityDataInferredOnly is FALSE when nothing was placed at all: that is a decline, not a low-confidence answer. #6006's decline path is untouched and still binds. The >= 2 placed targets threshold is precisely what keeps its add-only fixture (an added entity calling exactly one placed entity) declining; a mutation lowering it to 1 fails TestAnalyzePRImpact_AvailabilityFollowsChangedSetNotEntitySet. ## Cost Nothing is built unless a changed entity is present in the head graph AND unplaced. Then: one pass over entities filtered to the (small) set of files and modules the unplaced changed entities occupy, and outbound/CONTAINS edges captured during the adjacency pass AnalyzePRImpact already makes, for changed ids only. No BFS, no transitive walk. Measured on a 20k-entity / 20k-edge graph with a 1-entity add-only change set: 3.86 ms -> 4.47 ms (+0.61 ms, +16%) and +1 KB / +12 allocs. Against the handler's 326 ms / 76 MB overlay parse (memoized on path+mtime+size, unchanged here) that is under 0.2% of a cold call. ## Fixtures Every fixture uses a PRODUCTION-SHAPED overlay covering only entities that existed at the last group index; newly added entities are absent from it by construction. Covered: a new entity in a placed file (infers), in a new file with two placed call targets (infers), in a new module (infers from module), with no placed neighbours (declines), with disagreeing signals (declines), with a 1-1 split container (abstains), a single call target (declines), chained inference (declines), and an overlay-placed entity (never overwritten). 19 mutations were reintroduced and all 19 were killed, including: inference disabled entirely, inference always succeeding, minPlacedTargets lowered to 1, plurality ties decided by map order, inference chaining, inference overriding the overlay, removed entities inferred from the diff record, the placed-vs- inferred label collapsed to "overlay", the aggregate counts dropped from the MCP payload, community_source dropped from the wire, and the decline path weakened to always-available. Refs #6006
… unanimity rule Addresses an adversarial review of the previous commit. Three findings reproduced the exact failure mode this issue exists to avoid; two more were guards held by nothing. ## D1 — a conflict could be 100% manufactured while inferred_only was false CommunityDataInferredOnly is a WHOLE-VERDICT flag and says nothing about an individual overlap. Two refs can each carry measured communities — so every aggregate reads "measured" — and still share ONLY a community that both sides reached by inference. The reported conflict is then entirely deduced and nothing in the payload said so. Conflicts mode makes it worse: it emits no changed_entities at all, so per-entity community_source was single-mode only. Now: PRImpactResult.InferredOnlyCommunityIDs (a community is inference-only when no overlay-placed changed entity AND no blast-radius entity puts it there — the blast radius is real edges to really-placed entities, so it counts as measured); ChangeImpact.InferredOnlyCommunities; MergeRiskPair.InferredSharedCommunities + InferredOnly; MergeRiskResult.InferredOnlyPairCount; per_ref.inferred_communities in conflicts mode; and a distinct prose note that fires on a deduced PAIR even when the verdict-level flag is false. One inferred side is enough to mark the overlap — the claim "these refs collide here" was never observed on that side. ## D2 — inference was overruling the group algorithm's own decision Candidacy keyed on communityOf(e) < 0, which is true both for "absent from the overlay" and for a NON-NIL -2/-1. Those entities were IN the last group index and community detection DECLINED to place them. #6042 is about entities the partition has never seen. Candidacy is now Entity.CommunityID == nil (overlayAbsent). #6006's TestAnalyzePRImpact_NegativeCommunityIDIsNotCoverage passed only because its fixture was a lone entity with no file siblings, no module and no edges — a shape production essentially never produces, so post-#6042 it certified nothing. Re-shaped: the entity now sits in a placed file, in a concentrated module, and calls two placed entities, so every signal is available and must still be refused. ## D3 + D5 — the dissent guard was unpinned, and module double-counted The 3/2/1 weighting required the winner to strictly outweigh all dissent, but with module subordinate to the container that comparison could never block anything: mutating it away passed both full suites. Rather than pin dead arithmetic, the rule is now the issue's own — UNANIMITY. At most two votes exist (the container-or-module primary, and the call targets) and both must agree. Module is now a strict FALLBACK, not a co-voter: module is module.Derive( SourceFile), a pure function of the path, so the module histogram is a superset of the file histogram and letting both vote counted one measurement twice. It also now requires a real sample (>= 3 placed) and real concentration (>= 0.7), because module.Derive has no per-Go-package marker: a single-module Go repo falls through to DefaultDepth and everything under internal/graph/** shares one label, where a 26-of-51 plurality is noise wearing a community id. A container that abstains from CONTRADICTION (a file split 1-1) does NOT fall through to the module — the module is a superset of that same contradiction and would resolve it only by dilution. Only an absent container falls through. Every inferred placement now carries its margin (CommunityInference: signals, support, sample) so 26/51 and 40/41 are distinguishable on the wire. ## D4 — the target signal accepted every edge kind It appended any outbound edge, so IMPORTS and CONTAINS drove inference. The "everything calls a logger" rationale for minPlacedTargets=2 buys nothing under IMPORTS, where importing two placed packages is universal; CONTAINS is the container signal wearing a second name. Now allowlisted to genuine call/reference kinds. A placed CONTAINS PARENT remains legitimate container evidence. ## D6 — the benchmark never inferred Its 20k placed entities cycled over 5 communities in EQUAL counts, so both histograms tied, both signals abstained, and it timed the decline path. Fixture rebuilt (one community per module, plus two call targets) and it now ASSERTS inference fires before timing. Re-measured, 20k entities / 20k edges, 1-entity add-only change: 3.63 ms -> 4.87 ms (+1.24 ms, +34%), +859 B, +13 allocs. The earlier "+0.61 ms" measured the histogram build only and is withdrawn. Against the handler's memoized 326 ms / 76 MB overlay parse this is ~0.4% of a cold call. ## D7/D8 — shipped deliverables with no coverage community_data_note and refs_with_inferred_community_data could both be deleted with both suites green; conflicts mode exposed no per-entity provenance. All now covered, including a measured control arm for every marker. ## Verification Mutation battery REBUILT FROM SCRATCH against the final tree: 34 mutations, all 34 killed by real test failures. One (M16) initially survived and one earlier "kill" turned out to be a build failure rather than a test failure — both fixed and re-run. Covers: inference disabled; unanimity guard removed; ties by map order; minPlacedTargets=1 (which fails #6006's AvailabilityFollowsChangedSetNotEntitySet, confirming it still binds); label collapsed to "overlay"; margin dropped; chaining; overlay overridden; removed entities inferred; candidacy on communityOf<0; IMPORTS/CONTAINS admitted; module concentration and sample floors removed; module outranking the container; contradictory container falling through; per-pair marker hardwired false; pair marked only when both sides inferred; blast radius ignored; and every payload field, note and per-ref list deleted in turn. go build / go vet / gofmt clean; full internal/graph/... and internal/mcp green. Refs #6006
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
After #6006, conflicts mode declines when no changed entity can be placed in a community. Correct — with zero community signal any verdict would be invented — but useless for a completely ordinary PR shape: one that only adds code. A newly added entity is never in the group overlay, because the overlay comes from the last indexed group union and the entity did not exist then.
This infers a community for such entities from their overlay-placed neighbours, and labels every inference as an inference.
The rule: unanimity over at most two votes
SourceFile, plus any placedCONTAINSparent.CONTAINSwas checked before being relied on: it is not a universal file→function edge (class→method, module→entity, produced ad hoc per language), so same-SourceFileis the load-bearing half.Properties["module"]) — a strict fallback, voting only when the container abstains, with a sample floor (≥3) and a concentration floor (≥0.7).A community is inferred only when the votes are unanimous. Not inferred, deliberately: overlay-placed entities, removed entities, blast-radius entities, and chained inference — only overlay-placed entities vote, so inference cannot propagate across a new package or become order-dependent.
Review drove three structural corrections here, and each removed a class of wrongness rather than patching one:
Module is not independent evidence. The module histogram is a strict superset of the file histogram, because
module.Deriveis a pure function ofSourceFile. Under the original 3/2/1 weighting, agreement counted one measurement twice at weight 5. Worse, module alone inferred on a bare plurality: a module with 51 placed entities split 26/25 produced a confident labelled placement.MarkerFileNameshas no per-Go-package marker, so in a single-module Go repo everything under a top-level directory falls through toDefaultDepth = 2and shares one label — meaning the most common add-only shape resolved against a large heterogeneous bucket. Now 26/51 declines, 9/10 infers, and both margins ride on the wire viaCommunityInference{signals, support, sample}.The strict-majority arithmetic was removed, not pinned. Once module became subordinate to the container, the
2*best > totalcomparison could never block anything — dead arithmetic that a surviving mutation had already shown nothing held. Unanimity over at most two votes makes the three-way split impossible rather than untested.A container that abstains from contradiction must not fall through to module. Found while implementing the above: if a file's placed entities split 1-1, the module is a superset containing that same contradiction and resolves it only by dilution.
containerVotenow reports whether it had evidence at all, distinct from having no winner.Provenance at every granularity below the verdict
The first version could ship a 100% manufactured conflict with
community_data_inferred_only: false. Two refs with communities[3,7]and[9,7]: the 3 and 9 are measured, but 7 — the sole basis of the reported conflict — existed on both sides purely by inference.CommunityDataInferredOnlyis a whole-verdict flag and says nothing about any individual pair. That is #6006's defect class one level down.Now a community is inference-only when neither an overlay-placed changed entity nor a blast-radius entity puts it there — the blast radius counts as measured, since it is real edges to really-placed entities, pinned by its own test. That flows through
ChangeImpact.InferredOnlyCommunities→MergeRiskPair.InferredSharedCommunities/InferredOnly→inferred_only_pair_count, plusper_ref[].inferred_communities(conflicts mode emits nochanged_entities, so per-entitycommunity_sourcewas single-mode only) and a separate prose note that fires on a deduced pair even when the verdict flag is correctly false.One inferred side marks the overlap. The first control-arm fixture asserted the opposite; the test was fixed rather than the code, because "these refs collide here" was never observed on that side.
Candidacy is overlay-absence, not a negative id
Inference originally triggered on
communityID < 0, which includes the-2sentinel written for entities that are in the overlay and which Louvain explicitly declined to place. This issue is about entities the partition has never seen.-2is only ever written for present entities (overlay.go:165), soCommunityID == nilis exactly "never seen"; candidacy is nowoverlayAbsent(e).#6006's negative-id test previously passed only because its fixture was a single entity with no file siblings, no module and no edges — a shape production essentially never produces, so it had quietly stopped covering production. Re-shaped: the entity now sits in a placed file, in a concentrated module, and calls two placed entities, so every signal fires and it must still be refused. Independently verified — widening candidacy to
< 0fails both that test and the new one.Cost — the earlier figure is withdrawn
The first benchmark reported +0.61 ms. That fixture never inferred: its 1000 placed entities cycled over 5 communities in equal counts, so both histograms tied and both signals abstained. It timed the histogram build against a decline.
Rebuilt to assert inference fires before timing: 3.63 ms → 4.87 ms (+1.24 ms, +34%), +859 B, +13 allocs on 20k entities / 20k edges with a one-entity add-only change. Still ~0.4% of a cold call against the memoized 326 ms / 76 MB overlay parse, and nothing is built when every changed entity is already placed.
Verification
34 mutations, all killed by real test failures — the battery was rebuilt from scratch against the final tree rather than carried forward, and that mattered: one mutation initially survived (the decline message losing its inference sentence, where the first attempt removed only one of two clauses) and one earlier "kill" turned out to be a build failure rather than a test failure. Both corrected and re-run.
The battery permanently includes the four that survived review: the
tied-only guard, the missing edge-kind filter, and deletingcommunity_data_note/refs_with_inferred_community_data.minPlacedTargets=1still failsTestAnalyzePRImpact_AvailabilityFollowsChangedSetNotEntitySet, confirming #6006 binds — and since it binds through that constant, the mutation is now permanent in the battery.go build ./... && go vet ./... && gofmt -l .clean.internal/graph/...andinternal/mcpgreen post-rebase, raw exit 0 viartk proxy.Known and disclosed
Unanimity makes container-vs-targets disagreement decline where the old weighting inferred the container, so the inference rate is lower than the first version implied. Most add-only PRs into existing directories are still expected to infer, but that is not measured on a live corpus and no number is claimed.
Blast-radius entities are not inferred, so
blast_radius_hitslightly understates communities on add-heavy PRs. The tool description is capped at 80 chars bybudget_test.go, so the inference contract lives in the payload only.Independently adversarially reviewed.
Closes #6042