Skip to content

Commit fa799ba

Browse files
committed
Make density-based explainers Rails-aware (depth, god-class, surface, hotspots)
Follow-up to the layers/cycles tuning: three explainers still misfired on autoloaded, framework-heavy, public-by-default codebases. - dependency-depth: count an oversized coupling cluster (SCC) as one logical layer instead of its full size, so a giant autoload cluster no longer reports every chain through it as an extreme depth. - god-class, hotspots: exclude framework/base scaffolding classes (Application*, *BaseController, *::Base) via a Ruby-gated helper -- high fan-in through inheritance isn't a god class. - exported-surface: skip Ruby symbols; public-by-default makes the exported/total ratio ~100% and uninformative. - explain hotspots: compute module blast radius over the module graph instead of an all-node reverse count that saturates in a densely-coupled graph. Explainer-only, no extractor change, so no cacheVersion bump.
1 parent 0272d63 commit fa799ba

12 files changed

Lines changed: 344 additions & 19 deletions

File tree

internal/explainers/common/common.go

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,15 @@ import (
1212
"github.com/enola-labs/enola/internal/facts"
1313
)
1414

15+
// OversizedClusterModules is the module-count above which a strongly-connected
16+
// component is treated as a coupling *cluster* rather than a discrete, actionable
17+
// tangle. In an autoloaded language (Ruby/Rails) mutual constant references across
18+
// many directories are the expected topology, so a large SCC is a coupling-density
19+
// signal, not a fixable cycle or a genuine deep layering. Shared by the cycles
20+
// explainer (softens such SCCs into a cluster note) and the depth explainer (counts
21+
// such an SCC as one logical layer instead of its full size).
22+
const OversizedClusterModules = 8
23+
1524
// FileDir returns the directory portion of a file path, which enola uses as the
1625
// canonical module name. A path with no separator maps to ".".
1726
func FileDir(file string) string {
@@ -147,6 +156,48 @@ func BuildModuleGraphExcluding(store *facts.Store, excludeKinds ...string) map[s
147156
return graph
148157
}
149158

159+
// rubyFrameworkBaseClasses are exact Rails/framework base-class names whose high
160+
// fan-in comes from being inherited, not from being a god class.
161+
var rubyFrameworkBaseClasses = map[string]bool{
162+
"ApplicationRecord": true, "ApplicationController": true, "ApplicationJob": true,
163+
"ApplicationMailer": true, "ApplicationService": true, "ApplicationSerializer": true,
164+
"ApplicationCable": true, "ApplicationPolicy": true, "ApplicationInteractor": true,
165+
"ApplicationPresenter": true,
166+
}
167+
168+
// rubyBaseClassSuffixes are naming conventions for user-defined base classes
169+
// (NotifierBase, ApiBaseController, Pusher::Base, ...). A class named like this is
170+
// a base others inherit from, so its inbound degree is inheritance, not coupling.
171+
var rubyBaseClassSuffixes = []string{
172+
"BaseController", "BaseJob", "BaseService", "BaseMailer", "BaseSerializer",
173+
"BasePolicy", "BasePresenter", "BaseInteractor", "Base",
174+
}
175+
176+
// IsRubyFrameworkBaseSymbol reports whether a symbol is a Rails/framework base
177+
// class — one whose high fan-in is a product of inheritance (every subclass
178+
// "depends on" it) rather than a design smell. Gated on a .rb file so non-Ruby
179+
// symbols are never affected. Used by the god-class and hotspots explainers to keep
180+
// framework scaffolding (ApplicationRecord/Controller/Job, *BaseController, *::Base)
181+
// out of their findings while still surfacing genuine central domain types.
182+
func IsRubyFrameworkBaseSymbol(name, file string) bool {
183+
if !strings.HasSuffix(file, ".rb") {
184+
return false
185+
}
186+
seg := name
187+
if i := strings.LastIndex(seg, "::"); i >= 0 {
188+
seg = seg[i+2:]
189+
}
190+
if rubyFrameworkBaseClasses[seg] {
191+
return true
192+
}
193+
for _, suffix := range rubyBaseClassSuffixes {
194+
if strings.HasSuffix(seg, suffix) {
195+
return true
196+
}
197+
}
198+
return false
199+
}
200+
150201
// SymbolModule returns the module a symbol belongs to. Symbol names encode the
151202
// module as the prefix before the first ".", e.g. "internal/auth.Login.Verify"
152203
// -> "internal/auth". Names without a "." are returned unchanged.

internal/explainers/cycles/cycles.go

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -16,8 +16,9 @@ import (
1616
// signal, not a defect. Such components are reported once as a softer,
1717
// lower-confidence "Highly coupled module cluster" note instead of an alarming
1818
// confidence-1.0 cycle with advice ("introduce an interface") that cannot
19-
// meaningfully be applied to a 99-node cluster.
20-
const maxCycleModules = 8
19+
// meaningfully be applied to a 99-node cluster. Shared with the depth explainer
20+
// via common.OversizedClusterModules.
21+
const maxCycleModules = common.OversizedClusterModules
2122

2223
// maxClusterMembers caps how many representative members are listed as evidence
2324
// for an oversized coupling cluster.

internal/explainers/depth/depth.go

Lines changed: 32 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -39,13 +39,17 @@ func (e *DepthExplainer) Name() string {
3939
//
4040
// Cycles are handled by collapsing each strongly-connected component to a single
4141
// super-node (the condensation), which is a DAG, then taking the longest path by
42-
// module count over that DAG. A component contributes its full size to any chain
43-
// passing through it. This is an intentional over-approximation: the exact
44-
// longest *simple* path is NP-hard, and inside a cycle every member is mutually
45-
// reachable, so counting the whole component is a safe cycle-safe upper bound
46-
// that never double-counts a module. A module's reported depth is its component's
47-
// depth; one insight is emitted per component (keyed by its smallest member),
48-
// so a cycle yields a single finding rather than one per entangled module.
42+
// module count over that DAG. A small component contributes its full size to any
43+
// chain passing through it (a genuine tangle deepens the chain). But an *oversized*
44+
// component (> common.OversizedClusterModules) is an autoload coupling cluster, not
45+
// deep layering — in Ruby/Rails mutual constant references collapse most of the app
46+
// into one giant SCC, and counting its full size would report every chain through it
47+
// as "depth ~100" (the cycle false-positive leaking into depth). Such a cluster is
48+
// therefore weighted as a single logical layer (componentWeight), so depth measures
49+
// real layering rather than cluster size; the cluster itself is already reported by
50+
// the cycles explainer. A module's reported depth is its component's depth; one
51+
// insight is emitted per component (keyed by its smallest member), so a cycle yields
52+
// a single finding rather than one per entangled module.
4953
func (e *DepthExplainer) Explain(ctx context.Context, store *facts.Store) ([]facts.Insight, error) {
5054
graph := common.BuildModuleGraph(store)
5155
if len(graph) == 0 {
@@ -109,7 +113,7 @@ func (e *DepthExplainer) Explain(ctx context.Context, store *facts.Store) ([]fac
109113
best, bi = d, j
110114
}
111115
}
112-
depth[i] = len(sccs[i]) + best
116+
depth[i] = componentWeight(sccs[i]) + best
113117
bestSucc[i] = bi
114118
return depth[i]
115119
}
@@ -170,13 +174,30 @@ func (e *DepthExplainer) Explain(ctx context.Context, store *facts.Store) ([]fac
170174
return insights, nil
171175
}
172176

177+
// componentWeight is how much a strongly-connected component contributes to a
178+
// dependency-chain's depth: its full member count for a small tangle, but just 1
179+
// for an oversized autoload cluster (which is one logical layer, not deep layering
180+
// — see the Explain doc comment).
181+
func componentWeight(scc []string) int {
182+
if len(scc) > common.OversizedClusterModules {
183+
return 1
184+
}
185+
return len(scc)
186+
}
187+
173188
// chainFor reconstructs the deepest chain of distinct modules starting at
174-
// component i: all of i's (sorted) members, then the members of its best
175-
// successor component, and so on down the DAG.
189+
// component i: for a small component all of its (sorted) members, but for an
190+
// oversized cluster only a single representative (so the evidence chain length
191+
// stays consistent with the reported depth instead of dumping ~100 modules), then
192+
// its best successor component, and so on down the DAG.
176193
func chainFor(i int, sccs [][]string, bestSucc []int) []string {
177194
var out []string
178195
for i != -1 {
179-
out = append(out, sccs[i]...)
196+
if len(sccs[i]) > common.OversizedClusterModules {
197+
out = append(out, sccs[i][0])
198+
} else {
199+
out = append(out, sccs[i]...)
200+
}
180201
i = bestSucc[i]
181202
}
182203
return out

internal/explainers/depth/depth_test.go

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import (
66
"strings"
77
"testing"
88

9+
"github.com/enola-labs/enola/internal/explainers/common"
910
"github.com/enola-labs/enola/internal/facts"
1011
)
1112

@@ -223,6 +224,39 @@ func TestExplain_SelfImportDoesNotAddDepth(t *testing.T) {
223224
}
224225
}
225226

227+
// TestExplain_OversizedClusterNotDeep: a large autoload cluster (a ring of
228+
// OversizedClusterModules+3 modules) must count as ONE logical layer, not its full
229+
// size, so it does not masquerade as a deep dependency chain. With a short tail
230+
// below it the whole graph's real layering stays under minDepth and nothing is
231+
// reported — matching the cycles explainer already covering the cluster.
232+
func TestExplain_OversizedClusterNotDeep(t *testing.T) {
233+
n := common.OversizedClusterModules + 3
234+
mods := make([]string, 0, n+1)
235+
deps := map[string][]string{}
236+
ring := make([]string, n)
237+
for i := 0; i < n; i++ {
238+
ring[i] = fmt.Sprintf("c/m%02d", i)
239+
mods = append(mods, ring[i])
240+
}
241+
for i := 0; i < n; i++ {
242+
deps[ring[i]] = []string{ring[(i+1)%n]} // one big SCC
243+
}
244+
// A short 2-module tail hanging off the cluster: c/m00 -> t/t0 -> t/t1.
245+
mods = append(mods, "t/t0", "t/t1")
246+
deps["c/m00"] = append(deps["c/m00"], "t/t0")
247+
deps["t/t0"] = []string{"t/t1"}
248+
249+
insights, err := New().Explain(context.Background(), makeStore(mods, deps))
250+
if err != nil {
251+
t.Fatalf("Explain: %v", err)
252+
}
253+
// Cluster weighted as 1 + tail(2) = depth 3 < minDepth(5) -> no findings, and
254+
// crucially not a "depth ~N" report of the whole cluster.
255+
if len(insights) != 0 {
256+
t.Fatalf("oversized cluster should not produce a deep-chain finding, got %d: %v", len(insights), titles(insights))
257+
}
258+
}
259+
226260
func TestExplain_EmptyGraph(t *testing.T) {
227261
insights, err := New().Explain(context.Background(), facts.NewStore())
228262
if err != nil {

internal/explainers/godclass/godclass.go

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -90,6 +90,12 @@ func (e *GodClassExplainer) Explain(ctx context.Context, store *facts.Store) ([]
9090
}
9191
var candidates []candidate
9292
for _, s := range distinct {
93+
// A Rails/framework base class (ApplicationRecord, *BaseController, *::Base)
94+
// has high fan-in purely because every subclass inherits from it — that is
95+
// not a god class. Skip it (non-Ruby symbols are unaffected).
96+
if common.IsRubyFrameworkBaseSymbol(s.Name, s.File) {
97+
continue
98+
}
9399
n := fanIn[s.Name]
94100
if n < minFanIn || float64(n) <= threshold {
95101
continue

internal/explainers/godclass/godclass_test.go

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -98,6 +98,49 @@ func TestExplain_DedupReopenedSymbol(t *testing.T) {
9898
}
9999
}
100100

101+
// TestExplain_RubyBaseClassExcluded: a Rails framework base class (.rb) with high
102+
// fan-in-via-inheritance is not reported as a god class, while a same-fan-in domain
103+
// class is. Guards the base-class exclusion.
104+
func TestExplain_RubyBaseClassExcluded(t *testing.T) {
105+
s := facts.NewStore()
106+
s.Add(facts.Fact{Kind: facts.KindSymbol, Name: "ApplicationRecord", File: "app/models/application_record.rb"})
107+
s.Add(facts.Fact{Kind: facts.KindSymbol, Name: "User", File: "app/models/user.rb"})
108+
for i := 0; i < 12; i++ {
109+
// Each caller subclasses ApplicationRecord and also references User.
110+
s.Add(facts.Fact{
111+
Kind: facts.KindSymbol, Name: fmt.Sprintf("app/models/m%d.Model", i),
112+
File: fmt.Sprintf("app/models/m%d.rb", i),
113+
Relations: []facts.Relation{{Kind: facts.RelImplements, Target: "ApplicationRecord"}, {Kind: facts.RelCalls, Target: "User"}},
114+
})
115+
}
116+
s.BuildGraph()
117+
118+
insights, err := New().Explain(context.Background(), s)
119+
if err != nil {
120+
t.Fatalf("Explain: %v", err)
121+
}
122+
for _, in := range insights {
123+
if strings.Contains(in.Title, "ApplicationRecord") {
124+
t.Errorf("framework base class should be excluded from god-class: %q", in.Title)
125+
}
126+
}
127+
foundUser := false
128+
for _, in := range insights {
129+
if strings.Contains(in.Title, "User") {
130+
foundUser = true
131+
}
132+
}
133+
if !foundUser {
134+
t.Errorf("real domain hotspot User should still be reported; got %v", func() []string {
135+
out := make([]string, len(insights))
136+
for i, in := range insights {
137+
out[i] = in.Title
138+
}
139+
return out
140+
}())
141+
}
142+
}
143+
101144
// TestExplain_CapsInsightCount: no more than maxInsights findings are emitted even
102145
// when many distinct symbols exceed the outlier threshold.
103146
func TestExplain_CapsInsightCount(t *testing.T) {

internal/explainers/hotspots/hotspots.go

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -89,6 +89,11 @@ func (e *HotspotExplainer) Explain(ctx context.Context, store *facts.Store) ([]f
8989
}
9090
var candidates []candidate
9191
for _, s := range distinct {
92+
// A Rails/framework base class has high fan-in via inheritance, not because
93+
// it is a genuine pinch point — skip it (non-Ruby symbols are unaffected).
94+
if common.IsRubyFrameworkBaseSymbol(s.Name, s.File) {
95+
continue
96+
}
9297
in := len(reverse[s.Name])
9398
out := len(forward[s.Name])
9499
if in < minDegree || out < minDegree {

internal/explainers/hotspots/hotspots_test.go

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -87,6 +87,49 @@ func TestExplain_DedupReopenedSymbol(t *testing.T) {
8787
}
8888
}
8989

90+
// TestExplain_RubyBaseClassExcluded: a Rails base class (.rb) with high fan-in AND
91+
// fan-out is still excluded from hotspots (its inbound degree is inheritance), while
92+
// a same-degree domain type is reported.
93+
func TestExplain_RubyBaseClassExcluded(t *testing.T) {
94+
s := facts.NewStore()
95+
addHub := func(name, file string) {
96+
calls := make([]facts.Relation, 0, 4)
97+
for i := 0; i < 4; i++ {
98+
tgt := fmt.Sprintf("%s_dep%d.Fn", name, i)
99+
calls = append(calls, facts.Relation{Kind: facts.RelCalls, Target: tgt})
100+
s.Add(facts.Fact{Kind: facts.KindSymbol, Name: tgt, File: "dep.rb"})
101+
}
102+
s.Add(facts.Fact{Kind: facts.KindSymbol, Name: name, File: file, Relations: calls})
103+
for i := 0; i < 4; i++ {
104+
s.Add(facts.Fact{Kind: facts.KindSymbol, Name: fmt.Sprintf("%s_caller%d.Fn", name, i),
105+
File: "caller.rb", Relations: []facts.Relation{{Kind: facts.RelCalls, Target: name}}})
106+
}
107+
}
108+
addHub("NotifierBase", "app/notifiers/notifier_base.rb")
109+
addHub("User", "app/models/user.rb")
110+
s.BuildGraph()
111+
112+
insights, err := New().Explain(context.Background(), s)
113+
if err != nil {
114+
t.Fatalf("Explain: %v", err)
115+
}
116+
var sawBase, sawUser bool
117+
for _, in := range insights {
118+
if strings.Contains(in.Title, "NotifierBase") {
119+
sawBase = true
120+
}
121+
if strings.Contains(in.Title, "User") {
122+
sawUser = true
123+
}
124+
}
125+
if sawBase {
126+
t.Errorf("framework base class should be excluded from hotspots")
127+
}
128+
if !sawUser {
129+
t.Errorf("real domain hotspot User should still be reported")
130+
}
131+
}
132+
90133
func TestExplain_BelowDegreeFloor(t *testing.T) {
91134
// High fan-out but fan-in below minDegree -> not a pinch point.
92135
store := makeStore("core.Hub", 1, 10)

internal/explainers/surface/surface.go

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -77,6 +77,15 @@ func (e *SurfaceExplainer) Explain(ctx context.Context, store *facts.Store) ([]f
7777

7878
mods := make(map[string]*moduleSurface)
7979
for _, s := range symbols {
80+
// Ruby is public-by-default: every class/method is "exported", so the
81+
// exported/total ratio is ~100% for every module and carries no signal (it
82+
// floods with namespace modules like Core::V3, RailsAdmin, V2). Skip Ruby
83+
// symbols so Ruby modules never become candidates; other languages in a
84+
// multi-repo snapshot are unaffected. "Too big/central" is already covered by
85+
// the god-class and complexity explainers.
86+
if lang, _ := s.Props["language"].(string); lang == "ruby" {
87+
continue
88+
}
8089
exported, ok := s.Props["exported"].(bool)
8190
if !ok {
8291
// Extractor didn't record visibility; ignore so it doesn't distort the ratio.

internal/explainers/surface/surface_test.go

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,42 @@ func TestExplain_LargePublicSurface(t *testing.T) {
4242
}
4343
}
4444

45+
// TestExplain_RubySkipped: Ruby symbols are public-by-default, so the exported
46+
// ratio is uninformative and Ruby modules must never be flagged; other languages in
47+
// the same store still are.
48+
func TestExplain_RubySkipped(t *testing.T) {
49+
s := facts.NewStore()
50+
// A large, fully-"exported" Ruby namespace — must be skipped.
51+
for i := 0; i < 30; i++ {
52+
s.Add(facts.Fact{
53+
Kind: facts.KindSymbol, Name: fmt.Sprintf("Core::V3.Sym%d", i),
54+
File: "app/controllers/core/v3/file.rb",
55+
Props: map[string]any{"exported": true, "language": "ruby"},
56+
})
57+
}
58+
// A non-Ruby over-exposed module — must still be reported.
59+
addModuleSymbols(s, "pkg/leaky", 20, 19)
60+
61+
insights, err := New().Explain(context.Background(), s)
62+
if err != nil {
63+
t.Fatalf("Explain: %v", err)
64+
}
65+
for _, in := range insights {
66+
if strings.Contains(in.Title, "Core::V3") {
67+
t.Errorf("Ruby module should be skipped by exported-surface: %q", in.Title)
68+
}
69+
}
70+
if len(insights) != 1 || !strings.Contains(insights[0].Title, "pkg/leaky") {
71+
t.Errorf("non-Ruby module should still be flagged; got %v", func() []string {
72+
out := make([]string, len(insights))
73+
for i, in := range insights {
74+
out[i] = in.Title
75+
}
76+
return out
77+
}())
78+
}
79+
}
80+
4581
func TestExplain_BelowRatio(t *testing.T) {
4682
s := facts.NewStore()
4783
addModuleSymbols(s, "pkg/mid", 20, 15) // 75% exported, below minExportedRatio

0 commit comments

Comments
 (0)