Improving snapshot comparison

Author: dejo1307

ARCHITECTURE.md

@@ -358,7 +358,12 @@ It is a **delta, not a linter**: it judges the current snapshot against the code
 - **new and removed coupling edges** (the architecturally interesting structural change);
 - **added / removed** modules, symbols, routes, storage; and props-level **changed** facts (line-only shifts are ignored, so an edit above a symbol doesn't churn the diff).
 
-Because the baseline is the project's own prior snapshot, a pattern that was present *before and after* (e.g. an API-first route with no loaded consumer) produces no delta — the diff is structurally immune to that false-signal class. Findings are identified by explainer + cited entities (not by their volatile title/metric text), so a god-class whose fan-in merely ticked up is not reported as resolve+new. `Compute` is pure and deterministic: identical inputs render byte-identically.
+Because the baseline is the project's own prior snapshot, a pattern that was present *before and after* (e.g. an API-first route with no loaded consumer) produces no delta — the diff is structurally immune to that false-signal class. `Compute` is pure and deterministic: identical inputs render byte-identically.
+
+Two refinements keep the finding delta honest, both learned from dogfooding on a real backend:
+
+- **Stable finding identity.** A finding is keyed by its explainer plus its number-normalized title (the stable subject), so a god-class whose fan-in merely ticked up — or a whole-codebase summary finding like the `layers` pattern, whose evidence enumerates every module — does not churn as resolve+introduce on an unrelated edit. Cycles are the exception: their title carries only a member count, so they are keyed on their sorted member modules.
+- **Structural-cause classification.** A new/resolved finding is reported as a real **regression introduced** / **improvement** only when the change actually touched one of the entities it cites (a fact added/removed/changed, or an edge endpoint — so a finding that flips because a *new caller* changed a symbol's fan-in still counts). Findings that appear or clear with no structural cause — a moving `mean+2σ` threshold, or a top-N list re-ranking after a worse offender left the window — are routed to a separate **incidental finding shifts** section so they never masquerade as something the change caused.
 
 The typical loop is `generate_snapshot → set_baseline → edit → generate_snapshot → diff_snapshot`.
 

internal/diff/diff.go

@@ -16,6 +16,7 @@ package diff
 import (
 	"encoding/json"
 	"fmt"
+	"regexp"
 	"sort"
 	"strings"
 
@@ -58,9 +59,18 @@ type SnapshotDiff struct {
 	// Findings delta (the ratchet core). FindingsNew are regressions introduced
 	// by the change; FindingsResolved are issues the change cleared. Each carries
 	// through its original Confidence and Description (caveats intact) untouched —
-	// the diff manufactures no verdicts.
+	// the diff manufactures no verdicts. Only findings with a STRUCTURAL CAUSE in
+	// this change (an evidence entity that was added/removed/changed) land here.
 	FindingsNew      []facts.Insight `json:"findings_new,omitempty"`
 	FindingsResolved []facts.Insight `json:"findings_resolved,omitempty"`
+
+	// Incidental finding shifts: findings that appeared or cleared with NO
+	// structural cause in this change — a moving statistical threshold (mean+2σ) or
+	// a re-ranked top-N list whose membership shifted because some OTHER finding
+	// left the window. These are surfaced separately so they don't masquerade as
+	// regressions/improvements the change actually caused.
+	FindingsNewIncidental      []facts.Insight `json:"findings_new_incidental,omitempty"`
+	FindingsResolvedIncidental []facts.Insight `json:"findings_resolved_incidental,omitempty"`
 }
 
 // Compute returns the delta from baseline to current. A nil snapshot is treated
@@ -125,26 +135,96 @@ func Compute(baseline, current *facts.Snapshot) *SnapshotDiff {
 	for _, in := range snapInsights(current) {
 		curFind[findingKey(in)] = in
 	}
+
+	// A finding only counts as a real regression/improvement if this change
+	// structurally touched something it cites — otherwise its appearance/clearance
+	// is incidental (a moving mean+2σ threshold, or a top-N list re-ranking after
+	// some other finding left the window). touched is the set of names the change
+	// added/removed/altered, including edge endpoints (so a finding that flips
+	// because a NEW caller changed a symbol's fan-in is still counted as real).
+	touched := d.touchedNames()
 	for k, in := range curFind {
-		if _, ok := baseFind[k]; !ok {
+		if _, ok := baseFind[k]; ok {
+			continue
+		}
+		if findingHasStructuralCause(in, touched) {
 			d.FindingsNew = append(d.FindingsNew, in)
+		} else {
+			d.FindingsNewIncidental = append(d.FindingsNewIncidental, in)
 		}
 	}
 	for k, in := range baseFind {
-		if _, ok := curFind[k]; !ok {
+		if _, ok := curFind[k]; ok {
+			continue
+		}
+		if findingHasStructuralCause(in, touched) {
 			d.FindingsResolved = append(d.FindingsResolved, in)
+		} else {
+			d.FindingsResolvedIncidental = append(d.FindingsResolvedIncidental, in)
 		}
 	}
 
 	d.sortAll()
 	return d
 }
 
+// touchedNames is the set of entity names this change structurally affected:
+// added/removed/changed facts plus the endpoints of added/removed edges. A
+// finding is attributed to the change when one of its evidence entities is in
+// this set.
+func (d *SnapshotDiff) touchedNames() map[string]struct{} {
+	m := make(map[string]struct{})
+	add := func(n string) {
+		if n != "" {
+			m[n] = struct{}{}
+		}
+	}
+	for _, f := range d.FactsAdded {
+		add(f.Name)
+	}
+	for _, f := range d.FactsRemoved {
+		add(f.Name)
+	}
+	for _, c := range d.FactsChanged {
+		add(c.After.Name)
+	}
+	for _, e := range d.EdgesAdded {
+		add(e.Source)
+		add(e.Target)
+	}
+	for _, e := range d.EdgesRemoved {
+		add(e.Source)
+		add(e.Target)
+	}
+	return m
+}
+
+// findingHasStructuralCause reports whether any entity the finding cites was
+// structurally touched by this change. Evidence-less findings can't be attributed,
+// so they default to real (never silently hidden).
+func findingHasStructuralCause(in facts.Insight, touched map[string]struct{}) bool {
+	if len(in.Evidence) == 0 {
+		return true
+	}
+	for _, ev := range in.Evidence {
+		for _, e := range []string{ev.Fact, ev.Symbol, ev.File} {
+			if e == "" {
+				continue
+			}
+			if _, ok := touched[e]; ok {
+				return true
+			}
+		}
+	}
+	return false
+}
+
 // Empty reports whether the diff contains no changes of any kind.
 func (d *SnapshotDiff) Empty() bool {
 	return len(d.FactsAdded) == 0 && len(d.FactsRemoved) == 0 && len(d.FactsChanged) == 0 &&
 		len(d.EdgesAdded) == 0 && len(d.EdgesRemoved) == 0 &&
-		len(d.FindingsNew) == 0 && len(d.FindingsResolved) == 0
+		len(d.FindingsNew) == 0 && len(d.FindingsResolved) == 0 &&
+		len(d.FindingsNewIncidental) == 0 && len(d.FindingsResolvedIncidental) == 0
 }
 
 // Focused returns a copy of the diff narrowed to entries that reference focus
@@ -197,6 +277,16 @@ func (d *SnapshotDiff) Focused(focus string) *SnapshotDiff {
 			out.FindingsResolved = append(out.FindingsResolved, in)
 		}
 	}
+	for _, in := range d.FindingsNewIncidental {
+		if insightMatches(in, focus) {
+			out.FindingsNewIncidental = append(out.FindingsNewIncidental, in)
+		}
+	}
+	for _, in := range d.FindingsResolvedIncidental {
+		if insightMatches(in, focus) {
+			out.FindingsResolvedIncidental = append(out.FindingsResolvedIncidental, in)
+		}
+	}
 	return out
 }
 
@@ -249,23 +339,46 @@ func edgeKey(e Edge) string {
 	return e.Repo + "\x00" + e.Source + "\x00" + e.Kind + "\x00" + e.Target
 }
 
-// findingKey identifies an insight by its explainer plus the sorted set of
-// entities it cites (evidence Fact/Symbol/File). Title and Detail are excluded
-// because they often embed volatile metrics (e.g. "fan-in: 13"); keying on the
-// entities keeps a finding stable across runs so only a finding about a NEW entity
-// counts as new. Evidence-less insights fall back to their title.
+// titleNumber matches the volatile metrics embedded in finding titles (counts,
+// ratios, percentages) so they can be stripped for a stable identity.
+var titleNumber = regexp.MustCompile(`[0-9]+(\.[0-9]+)?`)
+
+// normalizeTitle removes the volatile numbers from a finding title, leaving the
+// stable subject. "Large public surface: x/y exports 67 of 67 symbols (100%)"
+// and the same line with different counts collapse to one identity.
+func normalizeTitle(s string) string {
+	return titleNumber.ReplaceAllString(s, "#")
+}
+
+// findingKey identifies an insight so a finding stays the SAME finding across
+// snapshots even as its metrics drift or a ranked list re-orders.
+//
+// Most explainers name their subject (module/symbol/repo/pattern) in the title
+// and vary only by counts, so the number-normalized title is the stable identity.
+// This is what stops whole-codebase "summary" findings (e.g. the layers pattern,
+// whose evidence enumerates every module) from churning resolve+introduce on any
+// edit. Cycles are the exception: their title carries only a member count, so two
+// distinct cycles would collide — they are keyed on their sorted member modules
+// (the evidence), which is also what makes a cycle stay identified as long as its
+// membership holds.
 func findingKey(in facts.Insight) string {
+	if in.Source == "cycles" {
+		return in.Source + "\x00" + sortedEvidenceEntities(in)
+	}
+	return in.Source + "\x00" + normalizeTitle(in.Title)
+}
+
+// sortedEvidenceEntities joins a finding's cited entities (Fact/Symbol/File) in
+// sorted order — a stable identity for set-defined findings like cycles.
+func sortedEvidenceEntities(in facts.Insight) string {
 	var ents []string
 	for _, ev := range in.Evidence {
 		if e := firstNonEmpty(ev.Fact, ev.Symbol, ev.File); e != "" {
 			ents = append(ents, e)
 		}
 	}
-	if len(ents) == 0 {
-		return in.Source + "\x00" + in.Title
-	}
 	sort.Strings(ents)
-	return in.Source + "\x00" + strings.Join(ents, "\x1f")
+	return strings.Join(ents, "\x1f")
 }
 
 // --- helpers ---
@@ -362,6 +475,12 @@ func (d *SnapshotDiff) sortAll() {
 	sort.Slice(d.FindingsResolved, func(i, j int) bool {
 		return findingKey(d.FindingsResolved[i]) < findingKey(d.FindingsResolved[j])
 	})
+	sort.Slice(d.FindingsNewIncidental, func(i, j int) bool {
+		return findingKey(d.FindingsNewIncidental[i]) < findingKey(d.FindingsNewIncidental[j])
+	})
+	sort.Slice(d.FindingsResolvedIncidental, func(i, j int) bool {
+		return findingKey(d.FindingsResolvedIncidental[i]) < findingKey(d.FindingsResolvedIncidental[j])
+	})
 }
 
 // KindCounts returns counts of facts by kind for the given slice, used by the

internal/diff/diff_test.go

@@ -1,6 +1,7 @@
 package diff
 
 import (
+	"fmt"
 	"strings"
 	"testing"
 
@@ -84,8 +85,10 @@ func TestCompute_PropsChangeIsAChange(t *testing.T) {
 }
 
 func TestCompute_NewFinding(t *testing.T) {
-	base := snap(nil, nil)
-	cur := snap(nil, []facts.Insight{cycleInsight("a", "b")})
+	// A new cycle comes with the structural edges that created it, so its members
+	// are in the change's touched set and it is a real regression.
+	base := snap([]facts.Fact{mod("a", "a.go"), mod("b", "b.go")}, nil)
+	cur := snap([]facts.Fact{mod("a", "a.go", "b"), mod("b", "b.go", "a")}, []facts.Insight{cycleInsight("a", "b")})
 
 	d := Compute(base, cur)
 	if len(d.FindingsNew) != 1 {
@@ -129,15 +132,70 @@ func TestCompute_FindingStableAcrossVolatileTitle(t *testing.T) {
 }
 
 func TestCompute_ResolvedFinding(t *testing.T) {
-	base := snap(nil, []facts.Insight{cycleInsight("a", "b")})
-	cur := snap(nil, nil)
+	// Breaking the cycle removes the b→a edge, so the cycle members are touched and
+	// the cleared finding is a real improvement.
+	base := snap([]facts.Fact{mod("a", "a.go", "b"), mod("b", "b.go", "a")}, []facts.Insight{cycleInsight("a", "b")})
+	cur := snap([]facts.Fact{mod("a", "a.go", "b"), mod("b", "b.go")}, nil)
 
 	d := Compute(base, cur)
 	if len(d.FindingsResolved) != 1 {
 		t.Fatalf("expected 1 resolved finding, got %+v", d.FindingsResolved)
 	}
 }
 
+// TestCompute_RankWindowFindingIsIncidental is the regression guard for the false
+// positives found dogfooding: a finding about an UNCHANGED subject — e.g. a module
+// that rose into a top-N list only because a worse one was removed — must be
+// classified incidental, not a real regression. The genuinely-removed subject's
+// finding still resolves as a real improvement.
+func TestCompute_RankWindowFindingIsIncidental(t *testing.T) {
+	surf := func(module, sym string) facts.Insight {
+		return facts.Insight{Source: "exported-surface", Confidence: 0.6,
+			Title:    "Large public surface: " + module + " exports 67 of 67 symbols (100%)",
+			Evidence: []facts.Evidence{{Symbol: sym, Detail: "exported"}}}
+	}
+	base := snap(
+		[]facts.Fact{sym("app/golf_rule.Svc", "app/golf_rule/svc.go", 1), sym("db/messaging.Repo", "db/messaging/repo.go", 1)},
+		[]facts.Insight{surf("app/golf_rule", "app/golf_rule.Svc")}, // only golf_rule reported (messaging below the line)
+	)
+	cur := snap(
+		[]facts.Fact{sym("db/messaging.Repo", "db/messaging/repo.go", 1)}, // golf_rule removed; messaging unchanged
+		[]facts.Insight{surf("db/messaging", "db/messaging.Repo")},        // messaging rose into the window
+	)
+
+	d := Compute(base, cur)
+	if len(d.FindingsNew) != 0 {
+		t.Fatalf("unchanged module rising into top-N must be incidental, got real: %+v", d.FindingsNew)
+	}
+	if len(d.FindingsNewIncidental) != 1 {
+		t.Fatalf("expected 1 incidental new finding (messaging), got %+v", d.FindingsNewIncidental)
+	}
+	if len(d.FindingsResolved) != 1 {
+		t.Fatalf("expected golf_rule (actually removed) as 1 real improvement, got %+v", d.FindingsResolved)
+	}
+}
+
+// TestCompute_SummaryFindingDoesNotChurn guards the layers/summary case: a finding
+// whose evidence enumerates the whole codebase keeps its identity when modules
+// change, so it never churns resolve+introduce.
+func TestCompute_SummaryFindingDoesNotChurn(t *testing.T) {
+	layers := func(mods ...string) facts.Insight {
+		in := facts.Insight{Source: "layers", Confidence: 0.89,
+			Title: fmt.Sprintf("Architecture pattern: go-standard (%d modules)", len(mods))}
+		for _, m := range mods {
+			in.Evidence = append(in.Evidence, facts.Evidence{Fact: m})
+		}
+		return in
+	}
+	base := snap(nil, []facts.Insight{layers("a", "b", "c", "d")})
+	cur := snap(nil, []facts.Insight{layers("a", "b", "c")})
+
+	d := Compute(base, cur)
+	if !d.Empty() {
+		t.Fatalf("summary finding churned on a module-count change: %+v", d)
+	}
+}
+
 // TestCompute_Deterministic guards the brand promise: identical inputs render
 // byte-identically regardless of input ordering or map iteration.
 func TestCompute_Deterministic(t *testing.T) {

internal/diff/render.go

@@ -40,6 +40,7 @@ func (d *SnapshotDiff) RenderSummary() string {
 		sb.WriteString("\n")
 	}
 
+	d.writeIncidentalShifts(&sb)
 	d.writeStructuralSummary(&sb)
 	return sb.String()
 }
@@ -88,6 +89,7 @@ func (d *SnapshotDiff) RenderCompact() string {
 		sb.WriteString("\n")
 	}
 
+	d.writeIncidentalShifts(&sb)
 	d.writeStructuralSummary(&sb)
 
 	// New coupling is the architecturally interesting structural change, so list
@@ -115,6 +117,27 @@ func (d *SnapshotDiff) RenderCompact() string {
 	return sb.String()
 }
 
+// writeIncidentalShifts lists findings that appeared or cleared without a
+// structural cause in this change (a moving statistical threshold, or a top-N list
+// re-ranking after some other finding left the window). They are surfaced so
+// nothing is hidden, but kept out of the regression/improvement headline so they
+// don't read as something the change caused.
+func (d *SnapshotDiff) writeIncidentalShifts(sb *strings.Builder) {
+	total := len(d.FindingsNewIncidental) + len(d.FindingsResolvedIncidental)
+	if total == 0 {
+		return
+	}
+	fmt.Fprintf(sb, "## Incidental finding shifts (%d)\n\n", total)
+	sb.WriteString("_Appeared or cleared with no structural cause in this change — a moving statistical threshold or a re-ranked top-N list. Likely NOT caused by this change; verify only if relevant._\n\n")
+	for _, in := range d.FindingsNewIncidental {
+		fmt.Fprintf(sb, "- appeared · [%s] %s\n", insightSource(in), oneLine(in.Title))
+	}
+	for _, in := range d.FindingsResolvedIncidental {
+		fmt.Fprintf(sb, "- cleared · [%s] %s\n", insightSource(in), oneLine(in.Title))
+	}
+	sb.WriteString("\n")
+}
+
 func (d *SnapshotDiff) writeProvenance(sb *strings.Builder) {
 	if d.BaselineGeneratedAt != "" || d.CurrentGeneratedAt != "" {
 		fmt.Fprintf(sb, "_Baseline %s → current %s._\n\n", orDash(d.BaselineGeneratedAt), orDash(d.CurrentGeneratedAt))