Skip to content

Commit 508bb1b

Browse files
authored
fix(engine): scope Ruby test globs to spec/ and test/ directories (#80)
`**/*_spec.rb` and `**/*_test.rb` sat in both config.Default().Ignore and .TestGlobs, matching on the basename alone. Any production file ending in the token `_test` or `_spec` -- an A/B-test job, a load-test harness -- was excluded from indexing AND routed to reference-only test-ref extraction, so the class it declared never became a symbol fact. The failure was silent and asymmetric: enola removed a right fact rather than adding a wrong one, so no downstream finding looked suspicious, and dead-code, impact and performance analysis were all quietly wrong for that file. A file is now a Ruby test iff its basename ends `_spec.rb`/`_test.rb` AND some directory segment is exactly `spec` or `test`. No filename-only rule works: `foo_test.rb` (a test) and `ab_test.rb` (production) both end in the token `test`. Ruby settles it by convention -- RSpec requires spec/, Minitest defaults to test/. Expressing that needed a pattern form the matcher lacked, so matchAnyGlob gained `<prefix>/**/<fileglob>` via matchDirScopedGlob. It fires only on a literal "/**/" in the pattern, which no shipped pattern contains, so existing globs are untouched. isIgnored was a token-identical copy of matchAnyGlob and now delegates to it -- one matcher is what keeps Ignore and TestGlobs from disagreeing about a file, which is the drift that caused this bug. Both lists change together: narrowing one alone leaves the file either still ignored or still misrouted. Go and TypeScript keep their filename patterns; those languages genuinely co-locate tests. The bundled mcp-arch.yaml and examples/*.yaml ignore blocks are updated in step, since a YAML `ignore:` replaces the default wholesale and none of them declares `test_globs:`. cacheVersion v96 -> v97: the file set reaching the extractor changes, so cached snapshots must re-extract.
1 parent 3cc921d commit 508bb1b

13 files changed

Lines changed: 231 additions & 65 deletions

File tree

examples/full.yaml

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -64,8 +64,8 @@ ignore:
6464
- "**/*Test.swift"
6565

6666
# Ruby / Rails
67-
- "**/*_spec.rb"
68-
- "**/*_test.rb"
67+
- "**/spec/**/*_spec.rb"
68+
- "**/test/**/*_test.rb"
6969
- "tmp/**"
7070
- "log/**"
7171
- "public/assets/**"

examples/multi-repo.yaml

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -53,8 +53,8 @@ ignore:
5353
- "**/*Tests.swift"
5454
- "**/*Test.swift"
5555
# Ruby / Rails
56-
- "**/*_spec.rb"
57-
- "**/*_test.rb"
56+
- "**/spec/**/*_spec.rb"
57+
- "**/test/**/*_test.rb"
5858
- "tmp/**"
5959
- "log/**"
6060
- "public/assets/**"

examples/ruby.yaml

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -17,8 +17,8 @@ ignore:
1717
- ".git/**"
1818
- ".enola/**"
1919
# Tests
20-
- "**/*_spec.rb"
21-
- "**/*_test.rb"
20+
- "**/spec/**/*_spec.rb"
21+
- "**/test/**/*_test.rb"
2222
- "spec/**"
2323
- "test/**"
2424
# Ruby / Rails

internal/cachecov/coverage_test.go

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -128,6 +128,7 @@ var versionCoverage = map[int][]string{
128128
94: {"TestExtractEndpointFacts_ConstantMethod"}, // Swift single-value (constant) method property
129129
95: {"TestResolveImports_CouplingKindTagged", "TestResolveImports_ReferenceBeatsAssociation"}, // Ruby synthetic-edge coupling_kind prop + framework-const ignore list
130130
96: {"TestAST_AssignedAndReturnedCallback", "TestAST_AssignedCallback_ShadowGuarded", "TestAST_ReturnedPlainVariable_NoPhantomRef", "TestAST_ReturnedForwardReference_Resolves", "TestAST_ShadowedLoopVarNotResolvedAsCall"}, // Python assignment/return value-refs + scope-wide (not just param) shadow guard
131+
97: {"TestMatchAnyGlob_MidPatternDoublestar", "TestGolden"}, // Directory-scoped Ruby test globs: production *_ab_test.rb no longer deleted from the graph
131132
}
132133

133134
func TestCacheVersionCoverage(t *testing.T) {

internal/config/config.go

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -48,8 +48,15 @@ func Default() *Config {
4848
"**/*.test.tsx",
4949
"**/*.spec.ts",
5050
"**/*.spec.tsx",
51-
"**/*_spec.rb",
52-
"**/*_test.rb",
51+
// Ruby, unlike Go and TS, has no co-located test convention: RSpec
52+
// requires spec/, Minitest defaults to test/. Demand the directory as
53+
// well as the filename — a bare "**/*_test.rb" also swallows production
54+
// code that merely ends in the token (a job named cache_warmup_ab_test.rb),
55+
// deleting it from the graph.
56+
// Keep in sync with TestGlobs below: a file that stops being a test must
57+
// stop being ignored, or it is dropped without being recovered.
58+
"**/spec/**/*_spec.rb",
59+
"**/test/**/*_test.rb",
5360
".enola/**",
5461
// Build / cache artifacts. These are generated output (often transpiled
5562
// JS, e.g. Next.js .next/) and must never be indexed as source — doing so
@@ -95,7 +102,7 @@ func Default() *Config {
95102
// include test symbols — but the engine collects them separately for
96103
// reference-only extraction so the dead-code detector can see that a
97104
// production symbol is exercised by a test and not mis-report it as dead.
98-
TestGlobs: []string{"**/*_spec.rb", "**/*_test.rb"},
105+
TestGlobs: []string{"**/spec/**/*_spec.rb", "**/test/**/*_test.rb"},
99106
Extractors: []string{"cpp", "go", "grpc", "java", "kotlin", "openapi", "php", "python", "typescript", "swift", "ruby"},
100107
Explainers: []string{"cycles", "layers", "crossrepo", "coverage", "unused-routes", "god-class", "hotspots", "dependency-depth", "exported-surface", "complexity-outliers"},
101108
Renderers: []string{"llm_context"},

internal/engine/cache.go

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -260,7 +260,13 @@ import (
260260
// assigned/iterated/aliased locals, not just parameters — so a loop var or local reusing a
261261
// same-named top-level def's name no longer fabricates a same-module edge. Cached Python
262262
// snapshots must re-extract.
263-
const cacheVersion = "v96"
263+
// v97: the Ruby ignore/test globs are directory-scoped ("**/spec/**/*_spec.rb" rather than
264+
// "**/*_spec.rb"), so a production file whose basename merely ends in the token _test/_spec
265+
// (a job named cache_warmup_ab_test.rb) is indexed as source instead of being ignored and
266+
// misrouted to reference-only test-ref extraction. The glob matcher gained the
267+
// "<prefix>/**/<fileglob>" form to express it. Cached Ruby snapshots must re-extract: the
268+
// file set reaching the extractor changes.
269+
const cacheVersion = "v97"
264270

265271
// extractorCache holds per-extractor facts keyed by a content hash of the files
266272
// the extractor depends on. It is loaded from disk at the start of a snapshot and

internal/engine/engine.go

Lines changed: 65 additions & 51 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ import (
1313
"path/filepath"
1414
"runtime"
1515
"runtime/debug"
16+
"slices"
1617
"strings"
1718
"sync"
1819
"time"
@@ -499,11 +500,33 @@ func (e *Engine) matchesTestGlob(relPath string) bool {
499500
return matchAnyGlob(filepath.ToSlash(relPath), e.cfg.TestGlobs)
500501
}
501502

502-
// matchAnyGlob reports whether a forward-slash path matches any of the patterns,
503-
// mirroring the "**/<name>/**", trailing-"/**", and "**/<glob>" handling of
504-
// isIgnored so test-glob matching stays consistent with ignore matching.
503+
// matchAnyGlob reports whether a forward-slash path matches any of the patterns.
504+
// It is the single matcher behind both the ignore list and the test globs, so a
505+
// file the two lists disagree about cannot exist: an ignored file that stops being
506+
// a test necessarily stops being ignored. Supported forms:
507+
//
508+
// vendor/** anchored directory prefix
509+
// **/build/** a directory named "build" at any depth
510+
// **/*_test.go a basename glob at any depth
511+
// **/spec/**/*_spec.rb a basename glob under a directory named "spec"
512+
//
513+
// The last form is the only one that constrains directory and filename together;
514+
// see matchDirScopedGlob for why the Ruby test globs need it.
505515
func matchAnyGlob(relPath string, patterns []string) bool {
506516
for _, pattern := range patterns {
517+
// "<prefix>/**/<fileglob>". Handled first and exclusively: the branches
518+
// below would match such a pattern only when exactly one directory sits
519+
// between prefix and file, which is an artifact of filepath.Match reading
520+
// "**" as "*", not a rule anyone intended.
521+
if i := strings.Index(pattern, "/**/"); i >= 0 {
522+
prefix, fileGlob := pattern[:i], pattern[i+len("/**/"):]
523+
if !strings.Contains(fileGlob, "/") {
524+
if matchDirScopedGlob(relPath, prefix, fileGlob) {
525+
return true
526+
}
527+
continue
528+
}
529+
}
507530
if strings.HasPrefix(pattern, "**/") && strings.HasSuffix(pattern, "/**") {
508531
seg := strings.TrimSuffix(strings.TrimPrefix(pattern, "**/"), "/**")
509532
if seg != "" && !strings.Contains(seg, "/") {
@@ -536,56 +559,47 @@ func matchAnyGlob(relPath string, patterns []string) bool {
536559
return false
537560
}
538561

539-
// isIgnored checks whether a path matches any ignore pattern.
540-
func (e *Engine) isIgnored(relPath string, isDir bool) bool {
541-
// Normalize to forward slashes for matching
542-
relPath = filepath.ToSlash(relPath)
543-
544-
for _, pattern := range e.cfg.Ignore {
545-
// "**/<seg>/**" — ignore a directory named <seg> at ANY depth (and
546-
// everything under it). The literal-prefix branch below cannot handle this
547-
// because the leading "**/" is not a real path component; match by checking
548-
// whether any path segment equals <seg>. Also covers the top-level case.
549-
if strings.HasPrefix(pattern, "**/") && strings.HasSuffix(pattern, "/**") {
550-
seg := strings.TrimSuffix(strings.TrimPrefix(pattern, "**/"), "/**")
551-
if seg != "" && !strings.Contains(seg, "/") {
552-
for _, part := range strings.Split(relPath, "/") {
553-
if part == seg {
554-
return true
555-
}
556-
}
557-
}
558-
}
559-
560-
// Handle directory-only patterns
561-
if strings.HasSuffix(pattern, "/**") {
562-
dirPrefix := strings.TrimSuffix(pattern, "/**")
563-
if relPath == dirPrefix || strings.HasPrefix(relPath, dirPrefix+"/") {
564-
return true
565-
}
566-
}
567-
568-
// Standard glob match
569-
matched, err := filepath.Match(pattern, relPath)
570-
if err == nil && matched {
571-
return true
572-
}
573-
574-
// Also try matching just the filename for patterns like **/*.go
575-
if strings.HasPrefix(pattern, "**/") {
576-
subPattern := strings.TrimPrefix(pattern, "**/")
577-
matched, err = filepath.Match(subPattern, filepath.Base(relPath))
578-
if err == nil && matched {
579-
return true
580-
}
581-
// Also try the full relative path
582-
matched, err = filepath.Match(subPattern, relPath)
583-
if err == nil && matched {
584-
return true
585-
}
562+
// matchDirScopedGlob reports whether relPath's basename matches fileGlob AND
563+
// prefix names one of its ancestor directories ("**/<seg>" for a segment at any
564+
// depth, otherwise an anchored literal path).
565+
//
566+
// A filename alone cannot classify a Ruby test. `lib/foo_test.rb` is one and
567+
// `app/jobs/cache_warmup_ab_test.rb` is a production A/B-test job, yet both end in
568+
// the token `test`; matching on the suffix deleted the latter from the graph
569+
// entirely. Ruby settles it by convention — RSpec requires spec/, Minitest defaults
570+
// to test/ — so the directory segment is the signal, and this predicate lets a
571+
// single pattern demand both halves.
572+
//
573+
// Because every element of dirSegs is by construction an ancestor of the basename,
574+
// segment equality alone places the file under the directory: no depth bookkeeping,
575+
// and "spec/user_spec.rb" (zero intervening directories) falls out for free.
576+
func matchDirScopedGlob(relPath, prefix, fileGlob string) bool {
577+
segs := strings.Split(relPath, "/")
578+
if len(segs) < 2 {
579+
return false // no directory component, so no prefix can name an ancestor
580+
}
581+
dirSegs, base := segs[:len(segs)-1], segs[len(segs)-1]
582+
583+
if m, err := filepath.Match(fileGlob, base); err != nil || !m {
584+
return false
585+
}
586+
if seg, ok := strings.CutPrefix(prefix, "**/"); ok {
587+
if seg == "" || strings.Contains(seg, "/") {
588+
return false
586589
}
590+
return slices.Contains(dirSegs, seg)
587591
}
588-
return false
592+
if prefix == "**" {
593+
return true // any directory
594+
}
595+
return strings.HasPrefix(relPath, prefix+"/")
596+
}
597+
598+
// isIgnored checks whether a path matches any ignore pattern. isDir is unused: the
599+
// patterns discriminate on shape, not on file type, and a directory that matches is
600+
// pruned by the caller.
601+
func (e *Engine) isIgnored(relPath string, isDir bool) bool {
602+
return matchAnyGlob(filepath.ToSlash(relPath), e.cfg.Ignore)
589603
}
590604

591605
// runExtractors detects applicable extractors and runs them. When cache is

internal/engine/engine_test.go

Lines changed: 115 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -166,6 +166,121 @@ func TestIsIgnored(t *testing.T) {
166166
}
167167
}
168168

169+
// TestMatchAnyGlob_MidPatternDoublestar covers the "<prefix>/**/<fileglob>" form,
170+
// which lets a pattern require BOTH a directory segment and a filename shape.
171+
//
172+
// The Ruby test globs need it. A bare "**/*_test.rb" is a filename-suffix match, so
173+
// a production ActiveJob named cache_warmup_ab_test.rb was ignored AND routed to
174+
// reference-only test-ref extraction — its class vanished from the graph. No
175+
// filename-only rule can separate that file from lib/foo_test.rb: both end in the
176+
// token "test". The directory segment is the only reliable signal, and Ruby supplies
177+
// one (RSpec requires spec/, Minitest defaults to test/).
178+
func TestMatchAnyGlob_MidPatternDoublestar(t *testing.T) {
179+
rubyTestGlobs := []string{"**/spec/**/*_spec.rb", "**/test/**/*_test.rb"}
180+
181+
tests := []struct {
182+
name string
183+
relPath string
184+
patterns []string
185+
want bool
186+
}{
187+
{
188+
// The reported bug: a production A/B-test job under app/jobs.
189+
"production job whose name ends in _ab_test",
190+
"app/jobs/reporting/cache_warmup_ab_test.rb",
191+
rubyTestGlobs,
192+
false,
193+
},
194+
{
195+
"production model named ab_test",
196+
"app/models/ab_test.rb",
197+
rubyTestGlobs,
198+
false,
199+
},
200+
{
201+
// Zero intermediate directories. filepath.Match's "*" never crosses a
202+
// separator, so the pre-existing "**/<glob>" branch could not match this.
203+
"spec directly under spec/",
204+
"spec/user_spec.rb",
205+
rubyTestGlobs,
206+
true,
207+
},
208+
{
209+
"spec one level down",
210+
"spec/services/report_worker_spec.rb",
211+
rubyTestGlobs,
212+
true,
213+
},
214+
{
215+
"spec segment at any depth, several levels down",
216+
"engines/billing/spec/models/nested/invoice_spec.rb",
217+
rubyTestGlobs,
218+
true,
219+
},
220+
{
221+
"minitest file under test/",
222+
"test/models/user_test.rb",
223+
rubyTestGlobs,
224+
true,
225+
},
226+
{
227+
// The dir segment is present but the basename shape is wrong.
228+
"support file under spec/ is not a spec",
229+
"spec/rails_helper.rb",
230+
rubyTestGlobs,
231+
false,
232+
},
233+
{
234+
// "spec" must be a DIRECTORY segment, not the basename stem.
235+
"file named spec.rb outside a spec dir",
236+
"app/models/spec.rb",
237+
rubyTestGlobs,
238+
false,
239+
},
240+
{
241+
"anchored prefix form",
242+
"spec/models/user_spec.rb",
243+
[]string{"spec/**/*_spec.rb"},
244+
true,
245+
},
246+
{
247+
"anchored prefix form does not match a nested spec dir",
248+
"engines/billing/spec/models/user_spec.rb",
249+
[]string{"spec/**/*_spec.rb"},
250+
false,
251+
},
252+
// The pre-existing pattern forms must keep their semantics — the new branch
253+
// fires only on a literal "/**/" in the pattern, which none of them contain.
254+
{
255+
"**/build/** still matches a nested build dir",
256+
"data/build/kspCaches/devDebug/Gen.kt",
257+
[]string{"**/build/**"},
258+
true,
259+
},
260+
{
261+
"**/*_test.go still matches by filename at any depth",
262+
"internal/pkg/foo_test.go",
263+
[]string{"**/*_test.go"},
264+
true,
265+
},
266+
{
267+
"vendor/** still matches an anchored prefix",
268+
"vendor/github.com/foo/bar.go",
269+
[]string{"vendor/**"},
270+
true,
271+
},
272+
}
273+
274+
for _, tt := range tests {
275+
t.Run(tt.name, func(t *testing.T) {
276+
if got := matchAnyGlob(tt.relPath, tt.patterns); got != tt.want {
277+
t.Errorf("matchAnyGlob(%q, %v) = %v, want %v",
278+
tt.relPath, tt.patterns, got, tt.want)
279+
}
280+
})
281+
}
282+
}
283+
169284
func TestResolveFactFile_SingleRepo(t *testing.T) {
170285
cfg := config.Default()
171286
eng, _ := New(cfg)

internal/engine/testdata/golden/ruby_sample.facts.jsonl

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
{"kind":"dependency","name":"config -\u003e rails/all","file":"config/application.rb","line":5,"repo":"ruby_sample","props":{"language":"ruby","source":"external"},"relations":[{"kind":"imports","target":"rails/all"}]}
44
{"kind":"file_ref","name":"app/services/report_worker.rb","file":"app/services/report_worker.rb","repo":"ruby_sample","props":{"dynamic_send_prefixes":["report_"],"language":"ruby"}}
55
{"kind":"file_ref","name":"app/views/reports/show.html.erb","file":"app/views/reports/show.html.erb","repo":"ruby_sample","props":{"language":"ruby"},"relations":[{"kind":"calls","target":"ReportPresenter"},{"kind":"calls","target":"ReportPresenter.render_summary"},{"kind":"calls","target":"can_view_reports?"},{"kind":"calls","target":"current_user"},{"kind":"calls","target":"render_summary"}]}
6+
{"kind":"module","name":"app/jobs","file":"app/jobs","repo":"ruby_sample","props":{"framework":"rails","language":"ruby","module_role":"unknown"}}
67
{"kind":"module","name":"app/models/concerns","file":"app/models/concerns","repo":"ruby_sample","props":{"framework":"rails","language":"ruby","module_role":"unknown"}}
78
{"kind":"module","name":"app/services","file":"app/services","repo":"ruby_sample","props":{"framework":"rails","language":"ruby","module_role":"unknown"}}
89
{"kind":"module","name":"config","file":"config","repo":"ruby_sample","props":{"framework":"rails","language":"ruby","module_role":"unknown"}}
@@ -16,6 +17,9 @@
1617
{"kind":"route","name":"/session","file":"config/routes.rb","line":20,"repo":"ruby_sample","props":{"action":"create","framework":"rails","language":"ruby","method":"POST","resource":"session"},"relations":[{"kind":"declares","target":"config"}]}
1718
{"kind":"route","name":"/session","file":"config/routes.rb","line":20,"repo":"ruby_sample","props":{"action":"destroy","framework":"rails","language":"ruby","method":"DELETE","resource":"session"},"relations":[{"kind":"declares","target":"config"}]}
1819
{"kind":"route","name":"/session","file":"config/routes.rb","line":20,"repo":"ruby_sample","props":{"action":"show","framework":"rails","language":"ruby","method":"GET","resource":"session"},"relations":[{"kind":"declares","target":"config"}]}
20+
{"kind":"symbol","name":"CacheWarmupABTest","file":"app/jobs/cache_warmup_ab_test.rb","line":11,"repo":"ruby_sample","props":{"exported":true,"framework":"rails","language":"ruby","superclass":"BaseWorker","symbol_kind":"class"},"relations":[{"kind":"declares","target":"app/jobs"},{"kind":"implements","target":"BaseWorker"}]}
21+
{"kind":"symbol","name":"CacheWarmupABTest#notify","file":"app/jobs/cache_warmup_ab_test.rb","line":18,"repo":"ruby_sample","props":{"cyclomatic":1,"exported":false,"framework":"rails","language":"ruby","symbol_kind":"method"},"relations":[{"kind":"declares","target":"app/jobs"}]}
22+
{"kind":"symbol","name":"CacheWarmupABTest#perform","file":"app/jobs/cache_warmup_ab_test.rb","line":12,"repo":"ruby_sample","props":{"cyclomatic":1,"exported":true,"framework":"rails","language":"ruby","symbol_kind":"method"},"relations":[{"kind":"calls","target":"notify"},{"kind":"declares","target":"app/jobs"}]}
1923
{"kind":"symbol","name":"Reporting","file":"app/services/report_worker.rb","line":6,"repo":"ruby_sample","props":{"abstract":false,"exported":true,"framework":"rails","language":"ruby","symbol_kind":"interface"},"relations":[{"kind":"declares","target":"app/services"}]}
2024
{"kind":"symbol","name":"Reporting::ReportWorker","file":"app/services/report_worker.rb","line":7,"repo":"ruby_sample","props":{"exported":true,"framework":"rails","language":"ruby","superclass":"BaseWorker","symbol_kind":"class"},"relations":[{"kind":"calls","target":"STOP_WORDS"},{"kind":"calls","target":"Trackable"},{"kind":"calls","target":"delegate"},{"kind":"calls","target":"formatted_name"},{"kind":"calls","target":"include"},{"kind":"calls","target":"subtitle"},{"kind":"calls","target":"track_metrics"},{"kind":"declares","target":"app/services"},{"kind":"implements","target":"BaseWorker"}]}
2125
{"kind":"symbol","name":"Reporting::ReportWorker#bucket","file":"app/services/report_worker.rb","line":77,"repo":"ruby_sample","props":{"cyclomatic":1,"exported":false,"framework":"rails","language":"ruby","symbol_kind":"method"},"relations":[{"kind":"declares","target":"app/services"}]}
Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,19 @@
1+
# frozen_string_literal: true
2+
3+
# v97: a PRODUCTION class whose basename ends in the token `test`. Ruby's test
4+
# globs are directory-scoped (`**/test/**/*_test.rb`), so this file — under
5+
# app/jobs, with no `test`/`spec` directory segment — is indexed as source.
6+
#
7+
# Before v97 the glob was the bare suffix `**/*_test.rb`, which both ignored this
8+
# file and routed it to reference-only test-ref extraction: the class never became
9+
# a symbol fact and vanished from the graph. Naming a job after the A/B test it
10+
# implements is ordinary; so are `*_load_test.rb`, `*_smoke_test.rb`.
11+
class CacheWarmupABTest < BaseWorker
12+
def perform(event_name, args = {})
13+
notify(event_name, args)
14+
end
15+
16+
private
17+
18+
def notify(event_name, args); end
19+
end

0 commit comments

Comments
 (0)