fix(#6006): pr_impact stops reporting zero merge conflicts when communities were never computed - #6039
Merged
Merged
Conversation
… communities were never computed
Merge risk intersects the community sets each ref touches. Both the producer
(AnalyzeMergeRisk's `c >= 0`) and the consumer (ImpactedCommunityIDs' `c < 0`)
drop the ungrouped sentinel, so a graph with no community assignments yields
empty sets, empty intersections, and an empty risky-pair list — byte-identical
to a genuine "these refs do not conflict". A false negative on a safety
question: an agent asking "do these branches conflict?" got "no", always.
The premise that this is the PERMANENT state, not an edge case, was verified
rather than inherited. cmd/grafel/index.go states it outright — the per-repo
Pass-4 algorithm pass was removed when the group-scope pass replaced it, and
the graph.fb per-entity algo fields are "left at their schema sentinels
(-2 / 0 / false) rather than recomputed per-repo"; internal/mcp/state.go
repeats it for PageRank ("a PERMANENT sentinel (0) for every entity"). Loading
every v5 graph.fb in the live store confirmed it: zero entities carry a
CommunityID. pr_impact reads graph.fb directly via LoadGraphFromDir, bypassing
applyGroupAlgoOverlay, so it saw no communities on every invocation.
Part 1 — "not computed" is now distinguishable from "no conflicts".
PRImpactResult.CommunityDataAvailable records whether the analysed graph
carried any real community id; ChangeImpact carries it per ref; MergeRiskResult
aggregates it conservatively (one uncovered ref taints the result) and names
the refs in RefsWithoutCommunityData. Conflicts mode returns an explicit MCP
error when it is false — the one shape a client cannot read as an answer —
rather than a zero-risk payload. Single mode keeps its payload (the blast
radius does not depend on communities) but labels community_data_available and
carries a reason string, following the `unavailable` idiom in tools.go.
Part 2 — the data source is the group overlay, not a graph.fb restamp.
groupCommunityStamper reads <group>-algo.json via groupalgo.ReadOverlay and
stamps CommunityID onto each ref graph before analysis, mirroring
applyGroupAlgoOverlay (same absence/staleness/version tolerance). Restamping
graph.fb was rejected: it would mean reviving a per-repo Louvain whose
partition is not the group partition every other grafel surface reports, so
merge risk would be computed against a second, disagreeing community
numbering. The overlay is the single authoritative source. No plane boundary
is crossed — internal/mcp already depends on groupalgo.
Also aligns the third filter: impacted_communities now drops c < 0 like the
other two paths, so the payload no longer reports impacted_community_count: 1
with a single community_id: -1 — the ungrouped bucket presented as a real
community. Ungrouped entities are still fully reported in changed_entities and
blast_radius with community_id: -1.
Tests are TDD-red-first and every one was mutation-verified: reverting the
c < 0 filter, the availability detection, the merge-risk aggregation, the
conflicts-mode error guard, each of the two stamp call sites, and the overlay
staleness check each makes the corresponding test fail. The MCP fixtures are
identical except for the overlay's presence, and the control arm proves the
fixture can exhibit a real conflict — so "no conflicts" cannot pass by absence.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0111KEDUVWEWm9G5RRnJqXft
…using on a stale overlay Follow-up to 4d7473d, addressing adversarial review. D1 — the previous fix reintroduced the bug it was written to remove. CommunityDataAvailable was measured over the WHOLE entity set, so any covered entity anywhere in the repo flipped it true — including when ZERO changed entities were covered. The group-algo overlay is computed from the indexed group union, so entities that exist only on a feature ref are absent from it by construction: every add-only PR hit exactly that path and got `risky_pair_count: 0` alongside `community_data_available: true`. An affirmative all-clear on a change nothing was known about, which is a worse shape than the original silent zero. The previous MCP fixture could not catch it because writeOverlay stamped svc:NewA/svc:NewB — entities that exist only on the feature refs — into the overlay, a state the group-algo pass cannot produce. The fixture is now the production shape (the overlay covers only the indexed union) and the refs that exercise the conflict MODIFY a covered entity, so the control arm still proves a real conflict is detectable. Availability now follows the changed set: unavailable when no changed entity could be placed, with the per-ref count surfaced as changed_entities_without_community so partial coverage is visible rather than silent. An empty change set stays vacuously available — that is the live default-base path (conflicts mode diffs refs[0] against itself) and hard-erroring it would break every default call. D2 — reversed. Refusing on a stale overlay was wrong for this consumer. The staleness key is the graph.fb of whichever ref is CHECKED OUT, so standing on a feature branch — the only posture from which anyone asks a merge-risk question — turned conflicts mode into a hard error, and any reindex of any repo in the group did the same for ~50 minutes (link debounce 10s, link pass ~9min, group-algo debounce 180s, sweep 10m). Merge risk is a community-level verdict and a 50-minute-old partition is overwhelmingly the same partition, so refusing bought almost no correctness and cost the tool its primary workflow. New groupalgo.ReadOverlayAnyAge applies a stale overlay and reports the staleness (community_data_stale + overlay_computed_at) instead of withholding it; absent, corrupt and version-mismatched are still hard misses. The error message now splits those two causes — telling a user to "run a group index" when the overlay exists and is fresh was actively misleading. D3 — a non-nil pointer to a negative community id counted as coverage. It is reachable (stamp copies EntityOverlay.CommunityID verbatim and the overlay uses -2 for "not assigned"; legacy graph.json carries -1), and would have produced CommunityDataAvailable=true while all three `c >= 0` filters dropped everything. Now tested for -1 and -2. D4 — the overlay was read and unmarshalled per call, both modes: 326ms and ~76MB transient heap on the live 31.9MB corpus overlay. Memoized on (path, mtime, size), one entry, with the parsed overlay treated as immutable so concurrent MCP agents share it. Staleness is deliberately outside the cache key and recomputed per call. Nit — the tool description now states that conflicts mode errors rather than returning zero pairs, within the 80-char budget budget_test.go enforces. Mutation-verified: reverting D1 reproduces the reviewer's exact payload (risky_pair_count 0 + community_data_available true); re-refusing on stale, hardcoding the stale flag, collapsing the two error causes, disabling the cache and dropping mtime from its key each fail their test. The review also found a ninth mutation that survived the previous round — ReadOverlayAnyAge's version gate was untested — so overlay_anyage_6006_test.go now covers it in both directions with a control arm. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0111KEDUVWEWm9G5RRnJqXft
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.
grafel_pr_impactanswered "do these branches conflict?" with "no", always — a false negative on a safety question, which is the worst direction to be wrong in. An agent reads absence of data as absence of risk.Mechanism
Merge risk intersects the community sets each ref touches. Both producer (
pr_impact.go:375) and consumer (:323) filter the ungrouped sentinelc < 0. With no community overlay every entity carriesCommunityID = -1, so both filters dropped everything, the intersection was empty, andAnalyzeMergeRiskreturned an empty risky-pair list — indistinguishable from a genuine no-conflict result.And this was the permanent state, not an edge case. Verified two ways:
cmd/grafel/index.go:1975-1991states the per-repo Pass-4 pass no longer runs and algo fields stay at schema sentinels;runPass4Algorithmsis retained but has no callers;carryForwardAlgoAttrsonly copies from a prior doc that also has none. Empirically, no entity in any readablegraph.fbcarries aCommunityID. Reproduced through MCP before fixing: two refs with a genuine shared community returned"risky_pair_count":0.Note the issue was already one correction deep — the original report claimed the opposite behaviour ("reports every ref pair as conflicting"), disproved by reading the filters.
Data source: the group overlay
groupCommunityStamperreads<group>-algo.jsonand stampsCommunityIDonto each ref graph before analysis, mirroringapplyGroupAlgoOverlayexactly so the two surfaces can never disagree on availability.Restamping
graph.fbwas rejected deliberately: it would revive a per-repo Louvain whose partition is not the group partition every other grafel surface reports, so merge risk would be computed against a second, disagreeing community numbering.Review confirmed no shared-state hazard —
loadRefGraphmaterialises a fresh*Documentper call, so in-place stamping cannot leak into other readers.Availability is measured over the changed set, not the entity set
The first version of this fix reintroduced the bug in a worse costume, and review caught it.
CommunityDataAvailablewas computed over the whole entity set, so any covered entity anywhere in the repo flipped it true — including when zero changed entities were covered. The test fixture concealed this: it stampedsvc:NewA/svc:NewBinto the overlay, entities that exist only on the feature refs and that the group-algo pass, computed from the indexed group union, cannot produce. Swapping in a production-shaped overlay and changing nothing else gave:The exact scenario the control arm existed to detect returned a confident all-clear — #6006 again, now wearing an availability stamp that made it look more trustworthy than before the fix.
Now: availability follows the changed set, and
changed_entities_without_communityis reported at result, per-ref and single-mode level so partial coverage is legible rather than silent. An empty change set stays vacuously available — that is the live default-base path (conflicts mode diffsrefs[0]against itself) and hard-erroring it would break every default call. Fixture rewritten to production shape: the overlay covers only pre-existing entities, conflict refs modify a covered entity, and separate add-only refs exercise the failing shape.One fixture detail worth recording: perturbing
EndLineto trigger modified-classification yieldedchanged_count: 0, becauseEndLinedoes not round-trip throughgraph.fb.Signaturedoes (load.go:601).Stale overlays are answered, not refused
The first version used
ReadOverlay, which rejects stale overlays. Review demonstrated with a real git repo that this made agit checkoutalone turn conflicts mode into a hard error: the staleness key is the graph.fb of whichever ref is currently checked out, so standing on a feature branch — the only posture from which anyone asks about conflicts — refused to answer, with the overlay fully covering both refs and the conflict entirely computable.The window is not marginal. Measured constants: link debounce 10 s, link pass ~9 min, group-algo debounce 180 s, sweep 10 m. After any reindex of any repo in the group, conflicts mode would have errored for roughly 50 minutes.
A partition 50 minutes old is overwhelmingly the same partition.
ReadOverlayAnyAgenow applies a stale overlay and reports it (community_data_stale,overlay_computed_at); absent, corrupt and version-mismatched remain hard misses. Error causes are split — the previous message said "missing or stale" for both, so a user hitting the staleness path was told to "run/await a group index" when they had just done exactly that.Also
impacted_communitiesnow dropsc < 0, so the ungrouped bucket is no longer surfaced ascommunity_id: -1.stripCommunitiessetsnil, andload.go:609maps the fb-2sentinel tonil), but it is reachable —stamp()writes the overlay value verbatim, and any legacygraph.jsoncarries it. It would have setCommunityDataAvailable: truewhile all three filters dropped everything.(path, mtime, size), parsed overlay treated as immutable so concurrent agents share it; staleness deliberately outside the key and recomputed per call; misses not cached. Previously re-read and unmarshalled per call — measured at 326 ms / 76 MB on the 31.9 MB corpus overlay, which matters on a 16 GB box running concurrent MCP agents.Verification
16 mutations across both rounds, all killed. Independently re-verified: weakening availability back to always-true fails three tests including
TestAnalyzePRImpact_AvailabilityFollowsChangedSetNotEntitySet.go build ./... && go vet ./... && gofmt -l .clean.internal/graph/...(all 8 sub-packages) andinternal/mcpgreen with raw exit codes viartk proxy.Review confirmed
grafel_impact_radius scope=changeset— the canonical advertised name, withgrafel_pr_impacta hidden alias — routes to the same handler, so this covers the surface agents actually use.Known and disclosed
available: truewithchanged_entities_without_community: 3. The verdict stands on what was placed; the count makes the gap legible. No weighting or threshold attempted.budget_test.go) caps descriptions at 80 chars, leaving room for exactly one fact. It was spent on the safety point:"PR impact + merge-risk. Conflicts mode errors (not 0 pairs) when uncomputed."Independently adversarially reviewed.
Closes #6006