feat: affinity-aware hop resolution (#482) — milestone 4#511
Conversation
Add resolveWithContext() to prefixMap for 4-tier disambiguation: 1. Affinity graph score (neighbor graph, highest priority) 2. Geographic proximity (centroid of context nodes) 3. GPS preference (existing heuristic) 4. First match (naive fallback) Enhance handleResolveHops API with: - affinityScore field on each HopCandidate - bestCandidate field when confidence threshold met - confidence field (unique_prefix/neighbor_affinity/ambiguous) - from_node and observer query params for context Existing resolve() unchanged for backward compatibility. Part of #482
…oximity gate, fix test cache invalidation - routes.go: Replace inline affinity scoring + disambiguation with resolveWithContext() call, eliminating duplicated 4-tier logic - routes.go: Only set bestCandidate for neighbor_affinity confidence (per spec: geo/GPS/first_match remain 'ambiguous' in API) - routes.go: Return 'no_match' instead of 'ambiguous' for zero candidates - routes.go: Guard against nil store in handleResolveHops - store.go: Remove unnecessary graph!=nil gate on geo_proximity tier - resolve_context_test.go: Invalidate node cache after DB inserts - resolve_context_test.go: Remove unused serveResolveHops helper - resolve_context_test.go: Remove unused mux import
Kpa-clawbot
left a comment
There was a problem hiding this comment.
Independent review
Reviewed cold against the spec at docs/specs/neighbor-affinity-graph.md (Milestone 4, "Integration with Existing Disambiguation", "Disambiguation Priority").
Must-fix
1. Missing minimum-observations guard in resolveWithContext (spec violation)
store.go line ~3369: the affinity tier auto-resolves when best.score >= 3× runner-up, but the spec requires both "score ratio ≥ 3×" and "≥ 3 observations" (affinityMinObservations). A single high-scoring observation can currently trigger neighbor_affinity confidence, which the spec explicitly prevents. The neighbor_graph.go disambiguation correctly checks e.Count < affinityMinObservations (line 360), but resolveWithContext does not.
Fix: before returning "neighbor_affinity", verify the winning edge has Count >= affinityMinObservations. You already have the edge — just check e.Count.
2. handleResolveHops discards geo/GPS confidence tiers (spec violation)
routes.go lines ~1397-1403: when resolveWithContext returns geo_proximity or gps_preference, the route handler ignores it and keeps confidence: "ambiguous". The spec's "Enhanced hop resolution" section (§c) defines the confidence field as one of unique_prefix | neighbor_affinity | geo_proximity | ambiguous. Only neighbor_affinity is currently propagated; the other resolution strategies are invisible to the API consumer.
Fix: propagate the confidence value from resolveWithContext to hr.Confidence for all tiers, not just neighbor_affinity. The bestCandidate field can remain neighbor_affinity-only (that's a strong recommendation), but confidence should reflect the actual strategy used.
3. Affinity scoring uses Score() (count×decay) instead of Jaccard similarity (spec deviation)
The spec's Disambiguation Priority table says the affinity tier wins when "Jaccard ≥ 3× runner-up". resolveWithContext compares raw Score() values (count/saturation × time_decay), which is a fundamentally different metric — it measures "how often and recently was this edge seen" rather than "how structurally similar are these nodes' neighborhoods." Two candidates could both have high raw scores (both are frequent neighbors of the context node) but very different Jaccard similarities.
If this is intentional simplification for M4 (Jaccard requires computing neighbor set overlaps), document it in the PR description and add a TODO. If not, the algorithm needs rework.
Out-of-scope (informational)
- The
Neighbors()method returns the internal slice without copying. This is safe in the current architecture (graph is replaced atomically, not mutated during serving) but fragile if someone later adds incremental updates. Consider a comment documenting this invariant. - The bubble sort in
resolveWithContext(lines ~3361-3366) works correctly but is unusual for Go —sort.Sliceis idiomatic and would be clearer. Non-blocking since the candidate list is always tiny.
… Score() vs Jaccard 1. resolveWithContext now requires affinityMinObservations (3) on the best-scoring edge before returning neighbor_affinity confidence, matching the guard in disambiguate() (neighbor_graph.go:360). 2. handleResolveHops now propagates geo_proximity and gps_preference confidence tiers from resolveWithContext instead of collapsing them to 'ambiguous'. Only truly unresolved results remain ambiguous. 3. Added comment explaining why resolveWithContext uses raw Score() (count×decay) instead of Jaccard similarity: Jaccard is used at the graph builder level to resolve ambiguous edges; here edges are already resolved and raw score correctly reflects observation frequency and recency for candidate ranking.
|
Review feedback addressed (commit
All tests pass ( |
Kpa-clawbot
left a comment
There was a problem hiding this comment.
Independent review
Reviewed cold against the spec at docs/specs/neighbor-affinity-graph.md (Milestone 4). Also verified all three fixes claimed in the follow-up comment from the prior review.
Prior review fixes — verified ✅
- Min-observations guard —
resolveWithContextnow checksbest.count >= affinityMinObservationsbefore returningneighbor_affinityconfidence. Matches the guard indisambiguate(). - Confidence tiers propagated — routes.go now propagates
geo_proximityandgps_preferencefromresolveWithContextinstead of collapsing toambiguous. - Score() vs Jaccard documented — Comment block in
resolveWithContextexplains the intentional use of rawScore()over Jaccard for candidate ranking at this level.
Spec compliance ✅
- 4-tier disambiguation priority (affinity → geo → GPS → first match): matches spec table
resolveWithContext()signature with context pubkeys + optional graph: matches spec §b- API response shape (
affinityScore,bestCandidate,confidence): matches spec §c - Confidence values include
gps_preferenceandfirst_matchbeyond the spec's listed values — reasonable extension, not a violation - Backward compatibility preserved: existing
resolve()unchanged, new params optional
Correctness ✅
- Affinity scoring in routes.go is duplicated (once for candidate scores, once inside
resolveWithContext) — redundant but not incorrect - Geo proximity uses equirectangular approximation — appropriate for relative comparison
- Edge cases handled: nil graph, no context, no match, single candidate
Test quality ✅
- 7 unit tests for
resolveWithContextcovering all 4 tiers + edge cases - 2 unit tests for
geoDistApprox - 4 API tests with solid coverage of response shape and context-driven resolution
- Backward compat test for
resolve()
Must-fix
None. All three issues from the prior review have been addressed. The implementation matches the spec.
Verdict: APPROVE (zero must-fix items)
(Cannot formally approve own repo PR due to GitHub restrictions, but this is a clean approval.)
Out-of-scope (informational)
- Bubble sort in
resolveWithContextcould besort.Slicefor idiomatic Go — non-blocking given tiny candidate lists - Duplicate affinity scoring loop (routes.go + resolveWithContext) could be refactored to avoid redundant graph traversal — no correctness impact
Summary
Milestone 4 of #482: adds affinity-aware hop resolution to improve disambiguation accuracy across all hop resolution in the app.
What changed
Backend —
prefixMap.resolveWithContext()(store.go)New method that applies a 4-tier disambiguation priority when multiple nodes match a hop prefix:
The existing
resolve()method is unchanged for backward compatibility. New callers that have context (originator, observer, adjacent hops) can useresolveWithContext()for better results.API —
handleResolveHops(routes.go)Enhanced
/api/resolve-hopsendpoint:from_node,observer— provide context for affinity scoringHopCandidate:affinityScore(float, 0.0–1.0)HopResolution:bestCandidate(pubkey when confident),confidence(one ofunique_prefix,neighbor_affinity,ambiguous)confidencefield)Types (types.go)
HopCandidate.AffinityScore *float64HopResolution.BestCandidate *stringHopResolution.Confidence stringTests
resolveWithContextcovering all 4 priority tiers + edge casesgeoDistApprox/api/resolve-hopsresponse shapeImpact
This improves ALL hop resolution across the app — analytics, route display, subpath analysis, and any future feature that resolves hop prefixes. The affinity graph (from M1/M2) now feeds directly into disambiguation decisions.
Part of #482