Skip to content

Commit f70ad64

Browse files
authored
Fix three extraction gaps found by widening the benchmark corpus (#171)
All three were invisible to the fixtures, because a fixture only encodes the forms its author already knew about. Adding public repositories for languages the corpus had no coverage for surfaced them within minutes. - Retrofit `@GET(value = "…")`. Kotlin allows any single-argument annotation to be written with its argument named, and real Android code does. Matching only the positional form yielded no client routes at all — not wrong routes, none — so no mobile-to-backend edges existed, silently. - Axum `.route(p, get(h).layer(mw))`. A non-verb method in the MethodRouter chain terminated the walk, dropping the route entirely with the verb in plain sight. Non-verb methods are now transparent decorators. Unchanged and still deliberate: `.route(p, handler_var)` emits nothing, because there is no verb to infer and a guessed one could false-match another repo's endpoint. - TypeScript path aliases. Overlapping prefixes were resolved by map iteration, taking the first match rather than the longest — wrong by tsconfig semantics, and different between runs. It was the only reproducibility failure in the corpus, and it made the gate report edge churn on a tree nobody had touched. Longest prefix wins, ties broken on the prefix string. cacheVersion v144 and v145, each with its cachecov entry. Five tests; the alias test repeats every resolution 50 times so it is a regression test rather than a coin flip that happened to land.
1 parent f593417 commit f70ad64

9 files changed

Lines changed: 256 additions & 8 deletions

File tree

README.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -119,7 +119,7 @@ enola check --warn-only # report everything, fail nothing
119119

120120
Not a language model, and not embeddings. enola parses your source with tree-sitter and language-specific extractors, normalizes it into a typed fact model, links it into a directed graph, and runs real graph algorithms over it — Tarjan's SCC for cycles, cycle-safe longest-path for dependency depth, mean+2σ outlier tests for the statistical findings.
121121

122-
That means the same commit yields the same answer, every time — measured, not asserted: across 30 open-source repositories indexed three times each, all 30 produced a byte-identical snapshot ID and a byte-identical fact file, over 3.9 million facts with zero parse errors ([BENCHMARKS.md](docs/BENCHMARKS.md)). Every snapshot carries a **receipt**: enola's version, the git ref and whether the tree was dirty, the extractors used, and a snapshot ID that's a `sha256` fingerprint of the facts rather than a random UUID. Before trusting a comparison, enola checks the two snapshots were even built the same way — a different extractor set or changed ignore rules makes a diff meaningless, and it says so instead of reporting churn as if it were your change.
122+
That means the same commit yields the same answer, every time — measured, not asserted: across 38 open-source repositories indexed three times each, all 38 produced a byte-identical snapshot ID and a byte-identical fact file, over 4.2 million facts with zero parse errors ([BENCHMARKS.md](docs/BENCHMARKS.md)). Every snapshot carries a **receipt**: enola's version, the git ref and whether the tree was dirty, the extractors used, and a snapshot ID that's a `sha256` fingerprint of the facts rather than a random UUID. Before trusting a comparison, enola checks the two snapshots were even built the same way — a different extractor set or changed ignore rules makes a diff meaningless, and it says so instead of reporting churn as if it were your change.
123123

124124
Nothing leaves your machine. It's a local binary reading local files.
125125

internal/cachecov/coverage_test.go

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -173,6 +173,8 @@ var versionCoverage = map[int][]string{
173173
141: {"TestExtractHTTPClientFacts_NestedGenericTypeArg", "TestExtractHTTPClientFacts_LowercaseVerbCalls", "TestExtractHTTPClientFacts_LowercaseInterpolatedBaseNotMatched", "TestExtractHTTPClientFacts_TestFileCallsAreNotClientRoutes", "TestExtractHTTPClientFacts_VerbNamedCalls", "TestGolden"},
174174
142: {"TestDecoratorRoutes_NestObjectForm", "TestDecoratorRoutes_NestStringFormAndBareVerb", "TestDecoratorRoutes_Inversify", "TestDecoratorRoutes_RequiresControllerDecorator", "TestDecoratorRoutes_VocabulariesDoNotMix", "TestDecoratorRoutes_DecoratorsDoNotCarryAcrossMembers", "TestDecoratorRoutes_TestFileEmitsNothing", "TestDecoratorRoutes_CommentBetweenDecoratorAndMethod", "TestGolden"}, // TypeScript's first server-side route DSL: @Controller/@controller classes emit one server route per verb-decorated method, both argument forms, gated on the controller decorator and on IsTestPath // a nested type argument (fetch<ApiResponse<Foo>>) no longer defeats client-call detection, and lowercase verb calls (axios.get('/x')) are detected when the argument is a "/"-rooted literal — the condition that keeps map.get('key')/cache.delete(id) out // a directory without __init__.py starts a new source root, so bare-name sibling imports inside it resolve (while a like-named third-party dir whose parent IS a package stays excluded); and a function passed to a decorator as a value is recorded as a reference instead of reading as dead
175175
143: {"TestServerRoutes_ExpressApp", "TestServerRoutes_UnmountedRouterEmitsNothing", "TestServerRoutes_SameFileMountComposes", "TestServerRoutes_DoNotStealClientCalls", "TestServerRoutes_NoDoubleEmission", "TestServerRoutes_OtherFrameworks", "TestServerRoutes_TestFileEmitsNothing", "TestGolden"}, // call-registered server routes (Express/Fastify/Hono/Koa), separated from v141 client calls by receiver binding; an unmounted sub-router and a bare catch-all emit nothing
176+
144: {"TestRetrofit_NamedValueArgument", "TestAxum_PerRouteLayerDoesNotSwallowTheVerb", "TestAxum_HandlerValueWithoutVerbStillEmitsNothing"}, // Retrofit @GET(value=) + Axum per-route .layer()
177+
145: {"TestResolveImportPath_LongestAliasWinsDeterministically", "TestResolveImportPath_NonAliasPathsUnchanged"}, // TS overlapping-alias resolution: specificity + determinism
176178
139: {"TestLooksGenerated", "TestApplyDecoratorProps_FrameworkRegistered", "TestApplyDecoratorProps_ModalNeedsImportGuard", "TestGolden"}, // facts from files carrying a codegen banner gain generated=true (language-agnostic, matched against the file head), and Python decorator-registered handlers (FastAPI exception_handler/middleware/on_event/websocket, Modal local_entrypoint and — gated on a modal import — function/cls) gain framework_registered=true, so the dead-code detector can drop findings it can never act on
177179
138: {"TestResolveImports_NestedLookalikeDoesNotCaptureThirdParty", "TestResolveImports_MultiSourceRootSurvivesPackageBoundaryRule", "TestResolveImports_SubpackageNotReachableByBareName", "TestBuildSuffixIndex_NoPackageDirsStaysPermissive", "TestResolveCallTargets_NestedLookalikeThirdPartyDropped", "TestResolveCallTargets_ClassQualifiedChainResolves", "TestResolveCallTargets_ClassQualifiedUnconfirmedStaysDotted", "TestResolveCallTargets_ClassQualifiedThroughReexport", "TestResolveCallTargets_SingleSegmentNeedsNoConfirmation", "TestGolden"}, // Python: (a) a directory is only a top-level package if its parent is not one, so an internal dir sharing a third-party name no longer captures its imports; (b) call-target resolution walks the module/symbol split leftwards with an exact module lookup, so a class-qualified chain binds to module.Class.method instead of being silently rewritten to module.method — multi-segment symbols must be confirmed against real symbol names // Python: a call target imported through a package __init__.py re-export resolves to the module that defines the symbol instead of dangling as a dotted string that matches no node; exact module resolution still wins and an ambiguous re-export stays dotted rather than binding arbitrarily // Python implements plugin.TestRefExtractor (which now also receives the production file list, needed to resolve dotted absolute-import targets); a symbol exercised only by a pytest file stops reading as dead, while the pass emits ONLY KindTestRef facts so a fixture's include_router cannot re-enter the production route graph // "**/testdata/**" joins the default ignore globs, and the two extractors that walk the repo themselves — OpenAPI (Extract) and gRPC (Detect) — repeat it in skipDir, which the globs cannot reach; Go fixture repos are miniature codebases whose routes and client call sites were being attributed to the host repo's service, manufacturing a cross-repo coverage gap
178180
}

internal/engine/cache.go

Lines changed: 41 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -844,7 +844,47 @@ import (
844844
// the same file are composed; cross-file mount resolution needs a repo-wide pass and
845845
// is deliberately not attempted. Bare catch-alls (app.get('*')) are skipped for the
846846
// same reason: a SPA fallback is not an endpoint and would match any client path.
847-
const cacheVersion = "v143"
847+
// v144: two annotation/chain forms that real code uses and the extractors did not
848+
// read, each found by adding a production repository to the benchmark corpus rather
849+
// than by inspection — which is the point of having one.
850+
//
851+
// Kotlin/Retrofit: `@GET(value = "topics")`. Kotlin permits any single-argument
852+
// annotation to be written with its argument named, and Google's reference Android
853+
// app writes EVERY endpoint that way. Matching only the positional form yielded zero
854+
// client routes there, and a Retrofit interface with no routes contributes no
855+
// mobile-to-backend edges at all — so "which screens break if I change this
856+
// endpoint" answered nothing, silently, on an entire class of Android codebase.
857+
//
858+
// Rust/Axum: `.route("/x", get(handler).layer(mw))`. A non-verb method in the
859+
// MethodRouter chain was treated as a terminator, so per-route middleware — which is
860+
// idiomatic Axum — discarded the verbs beneath it and dropped the route entirely,
861+
// with `get` sitting in plain sight. Non-verb methods are now transparent: the walk
862+
// recurses past them and keeps the chain below. Only ever applied to the second
863+
// argument of `.route(path, …)`, which is a MethodRouter by construction, so any
864+
// method on it is a wrapper around one.
865+
//
866+
// Unchanged, and still deliberate: `.route(path, handler_var)` emits nothing. There
867+
// is no verb to infer, and inventing one would produce a route that could false-match
868+
// another repository's endpoint — worse than the visible gap.
869+
// v145: TypeScript alias resolution picked among OVERLAPPING aliases by Go map
870+
// iteration order, taking the first match rather than the most specific one.
871+
//
872+
// Wrong twice over. tsconfig `paths` resolution is most-specific-first, so a project
873+
// mapping both "@acme/schema" and "@acme/" means the former for "@acme/schema/x" —
874+
// the old code could resolve it to either. And because map iteration is randomized,
875+
// "either" meant a DIFFERENT answer on different runs of the same unchanged tree.
876+
//
877+
// Measured on a 15k-file monorepo that maps one package both to its source and to its
878+
// built output: 2 facts of 163,582 flipped between runs — enough that three
879+
// consecutive `enola check` runs on an untouched tree reported `edges +1/-1`, then
880+
// `+6/-6`, then clean. It was the only repository of 38 that failed byte-level
881+
// reproducibility, and the failure is the expensive kind: a delta tool that invents
882+
// churn is worse than one that is merely incomplete, because invented churn is
883+
// indistinguishable from a real change.
884+
//
885+
// Longest matching prefix now wins, ties broken on the prefix string so the result is
886+
// a total order rather than a less-arbitrary one.
887+
const cacheVersion = "v145"
848888

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

internal/extractors/kotlinextractor/retrofit.go

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,17 @@ import (
1313
//
1414
// @GET("/api/settings/entitlements/users/{userID}/active")
1515
// @POST("auth/login")
16-
var retrofitAnnotation = regexp.MustCompile(`@(GET|POST|PUT|DELETE|PATCH|HEAD|OPTIONS)\s*\(\s*"([^"]*)"`)
16+
// @GET(value = "topics")
17+
//
18+
// The `value =` form is the same annotation written with its argument named, which
19+
// Kotlin allows for any single-argument annotation and which real Android codebases
20+
// use — Google's own reference app writes every endpoint that way. Matching only the
21+
// positional form yielded ZERO client routes for such a repository, and a Retrofit
22+
// interface with no routes produces no mobile-to-backend edges at all, silently: the
23+
// "which screens break if I change this endpoint" question just returns nothing.
24+
// Found by adding a real Android application to the benchmark corpus; see
25+
// DEFECTS_FOUND.md.
26+
var retrofitAnnotation = regexp.MustCompile(`@(GET|POST|PUT|DELETE|PATCH|HEAD|OPTIONS)\s*\(\s*(?:value\s*=\s*)?"([^"]*)"`)
1727

1828
// absoluteClientURL matches an absolute http(s) URL in a Retrofit annotation,
1929
// capturing the host and the remaining path. A full URL targets a fixed external

internal/extractors/kotlinextractor/retrofit_test.go

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -86,3 +86,45 @@ interface AdService {
8686
t.Errorf("relative annotation must not be external; got %+v", rel)
8787
}
8888
}
89+
90+
// TestRetrofit_NamedValueArgument covers the `@GET(value = "…")` form.
91+
//
92+
// Kotlin allows any single-argument annotation to be written with its argument
93+
// named, and real Android codebases do — Google's reference app writes every
94+
// endpoint that way. Matching only the positional form yielded ZERO client routes
95+
// for such a repository: not a wrong route, no routes at all, and therefore no
96+
// mobile-to-backend edges and no answer to "which screens break if I change this
97+
// endpoint". Found by adding a production Android app to the benchmark corpus.
98+
func TestRetrofit_NamedValueArgument(t *testing.T) {
99+
src := `package com.example.core.network.retrofit
100+
101+
interface NetworkApi {
102+
@GET(value = "topics")
103+
suspend fun getTopics(@Query("ids") ids: List<String>?): NetworkResponse<List<NetworkTopic>>
104+
105+
@POST(value = "/api/v1/sync")
106+
suspend fun sync(@Body body: SyncRequest): NetworkResponse<Unit>
107+
108+
@GET("positional/still/works")
109+
suspend fun positional(): NetworkResponse<Unit>
110+
}
111+
`
112+
ff := extractRetrofitFacts([]byte(src), "core/network/retrofit/NetworkApi.kt")
113+
if len(ff) != 3 {
114+
t.Fatalf("expected 3 client routes, got %d: %+v", len(ff), ff)
115+
}
116+
117+
byName := map[string]string{}
118+
for _, f := range ff {
119+
byName[f.Name], _ = f.Props["method"].(string)
120+
}
121+
for path, want := range map[string]string{
122+
"topics": "GET",
123+
"/api/v1/sync": "POST",
124+
"positional/still/works": "GET",
125+
} {
126+
if got := byName[path]; got != want {
127+
t.Errorf("route %q method = %q, want %q (all of: %+v)", path, got, want, byName)
128+
}
129+
}
130+
}

internal/extractors/rustextractor/axum.go

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -139,7 +139,21 @@ func collectMethodRouterChain(node *sitter.Node, src []byte) []axumRouteEntry {
139139
}
140140
name := nodeText(field, src)
141141
if !axumHTTPMethods[name] {
142-
return nil
142+
// A non-verb method in the chain is a DECORATOR, not a terminator:
143+
// `.route("/x", get(h).layer(mw))` attaches per-route middleware and is
144+
// idiomatic Axum, as are `.route_layer(…)` and `.with_state(…)`. Bailing
145+
// out here discarded the verbs underneath it, so the whole route vanished
146+
// even though `get` was sitting right there — a route silently absent from
147+
// the graph, which is the worst way to be wrong. Recurse past it instead
148+
// and keep whatever the chain below yields.
149+
//
150+
// Safe to be permissive: this only ever runs on the SECOND argument of a
151+
// `.route(path, …)` call, which is a MethodRouter by construction, so any
152+
// method on it is a wrapper around one.
153+
//
154+
// Found on a production Axum service added to the benchmark corpus; see
155+
// DEFECTS_FOUND.md.
156+
return collectMethodRouterChain(fn.ChildByFieldName("value"), src)
143157
}
144158
entries := collectMethodRouterChain(fn.ChildByFieldName("value"), src)
145159
return append(entries, axumRouteEntry{method: strings.ToUpper(name), handler: axumFirstArgName(node, src)})

internal/extractors/rustextractor/axum_test.go

Lines changed: 69 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -258,3 +258,72 @@ fn app() -> Router {
258258
t.Errorf("expected no handler prop for a closure handler, got %v", routes[0].Props["handler"])
259259
}
260260
}
261+
262+
// TestAxum_PerRouteLayerDoesNotSwallowTheVerb covers `.route(p, get(h).layer(mw))`.
263+
//
264+
// A non-verb method in the MethodRouter chain used to terminate the walk, so
265+
// per-route middleware — idiomatic Axum — discarded the verbs beneath it and the
266+
// route vanished entirely, with `get` sitting in plain sight. A route silently absent
267+
// from the graph is the worst way to be wrong: nothing reports it, and a client
268+
// calling that path resolves to nothing. Found on a production Axum service added to
269+
// the benchmark corpus.
270+
func TestAxum_PerRouteLayerDoesNotSwallowTheVerb(t *testing.T) {
271+
ff := extractComposed(t, map[string]string{
272+
"src/router.rs": `
273+
fn build() -> Router {
274+
Router::new()
275+
.route("/plain", get(root))
276+
.route("/layered", get(openapi::handler).layer(Extension(Arc::new(openapi))))
277+
.route("/stacked", post(create).layer(a).route_layer(b))
278+
.route("/both_verbs_then_layer", get(list).post(create).layer(mw))
279+
}`,
280+
})
281+
282+
got := routePaths(ff)
283+
for _, want := range []string{"/plain", "/layered", "/stacked", "/both_verbs_then_layer"} {
284+
if !got[want] {
285+
t.Errorf("missing route %q — a decorator in the chain dropped it; got %v", want, got)
286+
}
287+
}
288+
289+
// The verbs under the decorator must survive, not just the path.
290+
methods := map[string]map[string]bool{}
291+
for _, f := range ff {
292+
if f.Kind != facts.KindRoute {
293+
continue
294+
}
295+
m, _ := f.Props["method"].(string)
296+
if methods[f.Name] == nil {
297+
methods[f.Name] = map[string]bool{}
298+
}
299+
methods[f.Name][m] = true
300+
}
301+
if !methods["/layered"]["GET"] {
302+
t.Errorf("/layered lost its GET verb: %v", methods["/layered"])
303+
}
304+
if !methods["/both_verbs_then_layer"]["GET"] || !methods["/both_verbs_then_layer"]["POST"] {
305+
t.Errorf("/both_verbs_then_layer should keep both verbs, got %v", methods["/both_verbs_then_layer"])
306+
}
307+
}
308+
309+
// TestAxum_HandlerValueWithoutVerbStillEmitsNothing pins the deliberate half of the
310+
// same walk: `.route(path, handler_var)` has no verb to infer, and inventing one
311+
// would produce a route that could false-match another repository's endpoint. The
312+
// permissive recursion above must not turn this into a guess.
313+
func TestAxum_HandlerValueWithoutVerbStillEmitsNothing(t *testing.T) {
314+
ff := extractComposed(t, map[string]string{
315+
"src/router.rs": `
316+
fn build() -> Router {
317+
Router::new()
318+
.route("/from_var", okay.clone())
319+
.route("/real", get(handler))
320+
}`,
321+
})
322+
got := routePaths(ff)
323+
if got["/from_var"] {
324+
t.Error("/from_var has no HTTP verb and must not be emitted")
325+
}
326+
if !got["/real"] {
327+
t.Error("/real should still be extracted")
328+
}
329+
}

internal/extractors/tsextractor/ts.go

Lines changed: 27 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1415,14 +1415,37 @@ func tryParseTSConfigAliases(tsconfigPath string) (map[string]string, bool) {
14151415

14161416
// resolveImportPath normalizes a TypeScript import path to a filesystem-relative path.
14171417
// It handles path aliases (@/), relative imports (./), and identifies external packages.
1418+
//
1419+
// When several aliases match, the LONGEST prefix wins. Two rules in one:
1420+
//
1421+
// - Correctness. tsconfig `paths` resolution is most-specific-first, so a project
1422+
// mapping both "@acme/schema" and "@acme/" means the former for "@acme/schema/x".
1423+
// Taking any match resolved such imports to the wrong module.
1424+
// - Determinism. This used to `range` the alias map and return on first match. Go
1425+
// randomizes map iteration, so on a monorepo that maps a package BOTH to its source
1426+
// and to its built output, the same import resolved to a different module on
1427+
// different runs — and the snapshot stopped being reproducible. Measured on a
1428+
// 15k-file monorepo: 2 facts of 163,582 flipped between runs, which was enough to
1429+
// make `enola check` report `edges +6/-6` on a tree nobody had touched. A delta
1430+
// tool that invents churn is worse than one that is merely incomplete, because the
1431+
// churn is indistinguishable from a real change.
1432+
//
1433+
// Ties are broken on the prefix string so the result is a total order, not merely a
1434+
// less-arbitrary one. See DEFECTS_FOUND.md.
14181435
func resolveImportPath(importPath, fileDir string, aliases map[string]string) (string, bool) {
1419-
// Try alias resolution first
1436+
bestPrefix, bestReplacement := "", ""
14201437
for prefix, replacement := range aliases {
1421-
if strings.HasPrefix(importPath, prefix) {
1422-
rest := strings.TrimPrefix(importPath, prefix)
1423-
return filepath.ToSlash(filepath.Clean(replacement + rest)), false
1438+
if !strings.HasPrefix(importPath, prefix) {
1439+
continue
1440+
}
1441+
if len(prefix) > len(bestPrefix) || (len(prefix) == len(bestPrefix) && prefix < bestPrefix) {
1442+
bestPrefix, bestReplacement = prefix, replacement
14241443
}
14251444
}
1445+
if bestPrefix != "" {
1446+
rest := strings.TrimPrefix(importPath, bestPrefix)
1447+
return filepath.ToSlash(filepath.Clean(bestReplacement + rest)), false
1448+
}
14261449

14271450
// Relative imports
14281451
if strings.HasPrefix(importPath, ".") {

0 commit comments

Comments
 (0)