Suppress cluster-traversing deep-dependency chains

The depth explainer condenses SCCs and takes the longest path. After the
earlier tuning an oversized autoload cluster counted as one node, so chains
were honest but low-value: their length came mostly from threading through
the one giant cluster (already reported by the cycles explainer), and they
were near-duplicates sharing that cluster spine.

Make an oversized cluster a weight-0 sink: drop its outgoing edges in the
condensation and count it as 0 depth, so a chain earns depth only from
genuine, distinct-module layering above/outside the cluster. Real deep
layering still reports; cluster-manufactured depth no longer does.

Author: dejo1307

internal/explainers/depth/depth.go

@@ -43,13 +43,15 @@ func (e *DepthExplainer) Name() string {
 // chain passing through it (a genuine tangle deepens the chain). But an *oversized*
 // component (> common.OversizedClusterModules) is an autoload coupling cluster, not
 // deep layering — in Ruby/Rails mutual constant references collapse most of the app
-// into one giant SCC, and counting its full size would report every chain through it
-// as "depth ~100" (the cycle false-positive leaking into depth). Such a cluster is
-// therefore weighted as a single logical layer (componentWeight), so depth measures
-// real layering rather than cluster size; the cluster itself is already reported by
-// the cycles explainer. A module's reported depth is its component's depth; one
-// insight is emitted per component (keyed by its smallest member), so a cycle yields
-// a single finding rather than one per entangled module.
+// into one giant SCC. Such a cluster is therefore (a) weighted 0 (componentWeight)
+// and (b) made a sink in the condensation (its outgoing edges are dropped), so a
+// chain earns depth only from genuine, distinct-module layering above/outside the
+// cluster and never by threading through it. Otherwise every chain reaching the
+// cluster would report an inflated depth that merely restates the coupling the
+// cycles explainer already reports as a "Highly coupled module cluster". A module's
+// reported depth is its component's depth; one insight is emitted per component
+// (keyed by its smallest member), so a small cycle yields a single finding rather
+// than one per entangled module.
 func (e *DepthExplainer) Explain(ctx context.Context, store *facts.Store) ([]facts.Insight, error) {
 	graph := common.BuildModuleGraph(store)
 	if len(graph) == 0 {
@@ -69,14 +71,19 @@ func (e *DepthExplainer) Explain(ctx context.Context, store *facts.Store) ([]fac
 
 	// Build the condensed adjacency (successor component indices), deduped and
 	// sorted. Self-edges and intra-component edges are dropped — the condensation
-	// is acyclic by construction.
+	// is acyclic by construction. An oversized coupling cluster is made a sink (its
+	// outgoing edges are dropped) so a chain cannot earn depth by threading through
+	// it — see the Explain doc comment.
 	succ := make([][]int, len(sccs))
 	seen := make([]map[int]bool, len(sccs))
 	for i := range seen {
 		seen[i] = map[int]bool{}
 	}
 	for mod, neighbors := range graph {
 		si := sccOf[mod]
+		if len(sccs[si]) > common.OversizedClusterModules {
+			continue // oversized cluster is a depth sink
+		}
 		for _, n := range neighbors {
 			sj, ok := sccOf[n]
 			if !ok || sj == si {
@@ -175,29 +182,30 @@ func (e *DepthExplainer) Explain(ctx context.Context, store *facts.Store) ([]fac
 }
 
 // componentWeight is how much a strongly-connected component contributes to a
-// dependency-chain's depth: its full member count for a small tangle, but just 1
-// for an oversized autoload cluster (which is one logical layer, not deep layering
-// — see the Explain doc comment).
+// dependency-chain's depth: its full member count for a small tangle, but 0 for an
+// oversized autoload cluster. Combined with making the cluster a sink (its outgoing
+// edges are dropped), this means a chain earns depth only from genuine layering
+// above/outside the cluster, never from threading through it — see the Explain doc
+// comment.
 func componentWeight(scc []string) int {
 	if len(scc) > common.OversizedClusterModules {
-		return 1
+		return 0
 	}
 	return len(scc)
 }
 
 // chainFor reconstructs the deepest chain of distinct modules starting at
-// component i: for a small component all of its (sorted) members, but for an
-// oversized cluster only a single representative (so the evidence chain length
-// stays consistent with the reported depth instead of dumping ~100 modules), then
-// its best successor component, and so on down the DAG.
+// component i: all of each small component's (sorted) members, then its best
+// successor, and so on down the DAG. An oversized cluster (weight 0, and a sink so
+// it is always terminal) is omitted, keeping the reported chain length equal to the
+// reported depth instead of dumping ~100 cluster members.
 func chainFor(i int, sccs [][]string, bestSucc []int) []string {
 	var out []string
 	for i != -1 {
 		if len(sccs[i]) > common.OversizedClusterModules {
-			out = append(out, sccs[i][0])
-		} else {
-			out = append(out, sccs[i]...)
+			break // weight-0 sink; not part of the reported chain
 		}
+		out = append(out, sccs[i]...)
 		i = bestSucc[i]
 	}
 	return out

internal/explainers/depth/depth_test.go

@@ -224,39 +224,93 @@ func TestExplain_SelfImportDoesNotAddDepth(t *testing.T) {
 	}
 }
 
-// TestExplain_OversizedClusterNotDeep: a large autoload cluster (a ring of
-// OversizedClusterModules+3 modules) must count as ONE logical layer, not its full
-// size, so it does not masquerade as a deep dependency chain. With a short tail
-// below it the whole graph's real layering stays under minDepth and nothing is
-// reported — matching the cycles explainer already covering the cluster.
-func TestExplain_OversizedClusterNotDeep(t *testing.T) {
+// oversizedRing builds one big SCC of OversizedClusterModules+3 modules named
+// prefix+NN, returning the module list, the dep map, and the ring members.
+func oversizedRing(prefix string) ([]string, map[string][]string, []string) {
 	n := common.OversizedClusterModules + 3
-	mods := make([]string, 0, n+1)
-	deps := map[string][]string{}
 	ring := make([]string, n)
+	deps := map[string][]string{}
 	for i := 0; i < n; i++ {
-		ring[i] = fmt.Sprintf("c/m%02d", i)
-		mods = append(mods, ring[i])
+		ring[i] = fmt.Sprintf("%s%02d", prefix, i)
 	}
 	for i := 0; i < n; i++ {
-		deps[ring[i]] = []string{ring[(i+1)%n]} // one big SCC
+		deps[ring[i]] = []string{ring[(i+1)%n]}
 	}
+	return append([]string(nil), ring...), deps, ring
+}
+
+// TestExplain_OversizedClusterNotDeep: a large autoload cluster with a short tail
+// hanging off it produces no deep-chain finding — the cluster is a weight-0 sink, so
+// it neither counts toward depth nor lets a chain thread through it.
+func TestExplain_OversizedClusterNotDeep(t *testing.T) {
+	mods, deps, ring := oversizedRing("c/m")
 	// A short 2-module tail hanging off the cluster: c/m00 -> t/t0 -> t/t1.
 	mods = append(mods, "t/t0", "t/t1")
-	deps["c/m00"] = append(deps["c/m00"], "t/t0")
+	deps[ring[0]] = append(deps[ring[0]], "t/t0")
 	deps["t/t0"] = []string{"t/t1"}
 
 	insights, err := New().Explain(context.Background(), makeStore(mods, deps))
 	if err != nil {
 		t.Fatalf("Explain: %v", err)
 	}
-	// Cluster weighted as 1 + tail(2) = depth 3 < minDepth(5) -> no findings, and
-	// crucially not a "depth ~N" report of the whole cluster.
 	if len(insights) != 0 {
 		t.Fatalf("oversized cluster should not produce a deep-chain finding, got %d: %v", len(insights), titles(insights))
 	}
 }
 
+// TestExplain_ClusterIsDepthSink: genuine layering ABOVE a cluster still fires (the
+// cluster contributes nothing but the real layers count), while shallow layering
+// into a cluster does not — proving depth is earned only outside the cluster.
+func TestExplain_ClusterIsDepthSink(t *testing.T) {
+	mods, deps, ring := oversizedRing("k/m")
+	// 5 genuine layers a0->a1->a2->a3->a4, then a4 imports into the cluster.
+	for i := 0; i < 5; i++ {
+		mods = append(mods, fmt.Sprintf("a/l%d", i))
+	}
+	for i := 0; i < 4; i++ {
+		deps[fmt.Sprintf("a/l%d", i)] = []string{fmt.Sprintf("a/l%d", i+1)}
+	}
+	deps["a/l4"] = []string{ring[0]}
+	// 3 shallow layers b0->b1->b2, then b2 imports into the cluster.
+	for i := 0; i < 3; i++ {
+		mods = append(mods, fmt.Sprintf("b/l%d", i))
+	}
+	deps["b/l0"] = []string{"b/l1"}
+	deps["b/l1"] = []string{"b/l2"}
+	deps["b/l2"] = []string{ring[0]}
+
+	insights, err := New().Explain(context.Background(), makeStore(mods, deps))
+	if err != nil {
+		t.Fatalf("Explain: %v", err)
+	}
+	byMod := map[string]facts.Insight{}
+	for _, in := range insights {
+		for _, ev := range in.Evidence {
+			// first evidence fact is the chain head
+			byMod[ev.Fact] = in
+			break
+		}
+	}
+	// a/l0 has 5 real layers into the cluster -> reported at depth 5, and the chain
+	// must NOT include any cluster member.
+	aIn, ok := byMod["a/l0"]
+	if !ok {
+		t.Fatalf("5-layer chain above the cluster should be reported; got %v", titles(insights))
+	}
+	if !strings.Contains(aIn.Title, "depth 5") {
+		t.Errorf("a/l0 should be depth 5 (cluster contributes 0), got %q", aIn.Title)
+	}
+	for _, ev := range aIn.Evidence {
+		if strings.HasPrefix(ev.Fact, "k/m") {
+			t.Errorf("cluster member %q must not appear in the chain", ev.Fact)
+		}
+	}
+	// b/l0 has only 3 layers into the cluster -> below minDepth -> not reported.
+	if _, ok := byMod["b/l0"]; ok {
+		t.Errorf("3-layer chain into the cluster should be suppressed; got %v", titles(insights))
+	}
+}
+
 func TestExplain_EmptyGraph(t *testing.T) {
 	insights, err := New().Explain(context.Background(), facts.NewStore())
 	if err != nil {