Skip to content

Commit 6696fc6

Browse files
authored
fix(tsextractor): detect nested-generic and lowercase-verb HTTP client calls (#154)
The optional type-argument group in the client-call patterns was `<[^>]*>`, which cannot span a nested type argument: on `fetch<ApiResponse<Foo>>(…)` the inner class stops at the first `>`, the following `\s*\(` meets a `>`, and RE2 has no recursion, so backtracking to the empty alternative then meets `<`. One level of generics matched, two silently did not — including on the openapi-fetch shape the pattern was written for. Bind the group on `(`, which a type argument never contains. Lowercase verb calls (`axios.get('/path')`, `http.post('/path')`) matched nothing at all. They were excluded to avoid colliding with `map.get()` / `cache.delete()`; `lowerVerbCall` pays for admitting them by requiring a `/`-rooted literal argument — a condition `cleanTSPath` already enforces downstream, so no collection lookup can reach it. A lowercase call on an interpolated template stays deliberately unmatched. Both gaps erased outbound calls entirely — not counted detected, external or unresolved — so a cross-repo residual under-reported with no sign that it had. Admitting lowercase verbs immediately required a guard: a supertest call in an e2e suite is byte-identical in shape to production client traffic. Gate client-route extraction on facts.IsTestPath, and teach that predicate the two e2e conventions its .spec/.test suffixes cannot match — the hyphen means `.e2e-spec.ts` and `.e2e.ts` carry no leading dot. Ungated, one API's own test suite became 500+ client routes, promoted that service from isolated to connected, and fabricated a cross-repo dependency edge out of test traffic. Note the guard also removes pre-existing false positives: e2e HTTP calls were already being counted as production client routes before this change. cacheVersion v141. Golden fixture extended so the patterns are exercised at all — TestGolden previously passed untouched against the fixed extractor.
1 parent 627aba4 commit 6696fc6

8 files changed

Lines changed: 231 additions & 10 deletions

File tree

internal/cachecov/coverage_test.go

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -169,7 +169,8 @@ var versionCoverage = map[int][]string{
169169
135: {"TestExtract_SkipsTestdataFixtures", "TestDetect_IgnoresTestdataFixtures", "TestGolden"},
170170
136: {"TestExtractTestRefs_ResolvesAbsoluteImport", "TestExtractTestRefs_EmitsNoSymbolsModulesOrRoutes", "TestExtractTestRefs_FixtureRouterMountIsNotARoute", "TestExtractTestRefs_DropsExternalTargets", "TestExtractTestRefs_NeedsProductionFileSet", "TestGolden"},
171171
137: {"TestResolveCallTargets_PackageReexport_ResolvesToDefiningModule", "TestResolveCallTargets_PackageReexport_NameDiffersFromModule", "TestResolveCallTargets_PackageReexport_AmbiguousStaysDotted", "TestResolveCallTargets_PackageReexport_ExternalSourceIgnored", "TestResolveCallTargets_ExactModuleWinsOverReexport", "TestGolden"},
172-
140: {"TestImportableRoots_NonPackageDirStartsNewRoot", "TestResolveCallTargets_SiblingImportInNonPackageDir", "TestAST_DecoratorArgumentFunctionIsReferenced", "TestAST_DecoratorArgumentKeepsNestedCalls", "TestGolden"}, // 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
172+
140: {"TestImportableRoots_NonPackageDirStartsNewRoot", "TestResolveCallTargets_SiblingImportInNonPackageDir", "TestAST_DecoratorArgumentFunctionIsReferenced", "TestAST_DecoratorArgumentKeepsNestedCalls", "TestGolden"},
173+
141: {"TestExtractHTTPClientFacts_NestedGenericTypeArg", "TestExtractHTTPClientFacts_LowercaseVerbCalls", "TestExtractHTTPClientFacts_LowercaseInterpolatedBaseNotMatched", "TestExtractHTTPClientFacts_TestFileCallsAreNotClientRoutes", "TestExtractHTTPClientFacts_VerbNamedCalls", "TestGolden"}, // 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
173174
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
174175
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
175176
}

internal/engine/cache.go

Lines changed: 33 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -767,7 +767,39 @@ import (
767767
// @register(handler)) is a real use: the decorator stores it and the framework
768768
// invokes it later. The decorator-argument walk looked only for nested CALLS, so a
769769
// bare identifier slipped past and the referenced function had no incoming edge.
770-
const cacheVersion = "v140"
770+
// v141: two TypeScript HTTP-client detection gaps, both of which made outbound
771+
// calls vanish from the graph entirely — not counted detected, external OR
772+
// unresolved, so the cross-repo residual under-reported with no sign it had.
773+
//
774+
// The optional type-argument group in every client-call pattern was "<[^>]*>",
775+
// which cannot span a NESTED type argument: on fetch<ApiResponse<Foo>>(…) the inner
776+
// class stops at the first ">", the following "\s*\(" meets a ">" and fails, and
777+
// (RE2 having no recursion) backtracking to the empty alternative then meets "<".
778+
// One level of generics matched, two silently did not — and this hit the paths the
779+
// extractor advertises, openapi-fetch's API.GET<ApiResponse<T>>(…) among them. The
780+
// group is now bounded on "(", which a type argument never contains.
781+
//
782+
// Lowercase verb calls — axios.get('/path'), http.post('/path'), the dominant
783+
// hand-written idiom — matched nothing at all. They were excluded to avoid
784+
// colliding with map.get()/cache.delete(), which is a real hazard; the new
785+
// lowerVerbCall pays for admitting them by requiring a "/"-rooted literal argument,
786+
// a condition cleanTSPath already enforces downstream, so no collection lookup can
787+
// reach it. A lowercase call on an interpolated template (`${base}/x`) is still
788+
// deliberately not matched.
789+
//
790+
// Admitting lowercase verbs immediately required a guard it did not need before:
791+
// a supertest call in an e2e suite — request(app).get('/v2/me') — is byte-identical
792+
// in shape to production axios traffic. Client-route extraction is therefore now
793+
// gated on !facts.IsTestPath, because a test's HTTP traffic is not an architectural
794+
// dependency, and facts.IsTestPath learned the two e2e conventions its .spec/.test
795+
// suffixes could never match (".e2e-spec.ts", Nest's generated form, and ".e2e.ts",
796+
// Playwright's — the hyphen means neither carries the leading dot those entries
797+
// require). Ungated, a NestJS API's own test suite became 500+ client routes,
798+
// promoted the service from isolated to connected, and fabricated a cross-repo
799+
// dependency edge out of test traffic — the paths matched a real server because
800+
// they are the routes under test. Same principle as v-era GAP-XL-15, which keeps
801+
// test_ref facts out of the coupling graph.
802+
const cacheVersion = "v141"
771803

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

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

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -5,20 +5,22 @@
55
{"kind":"dependency","name":"ts/src -\u003e @acme/native-darwin-arm64","file":"acme-rs/ts/src/native.ts","line":4,"repo":"acme-rs","props":{"language":"typescript","source":"external"},"relations":[{"kind":"imports","target":"@acme/native-darwin-arm64"}]}
66
{"kind":"file_ref","name":"api/client.py","file":"acme/api/client.py","line":1,"repo":"acme","props":{"language":"python"},"relations":[{"kind":"calls","target":"FastAPI"},{"kind":"calls","target":"api/routers/search.get_search_router"}]}
77
{"kind":"file_ref","name":"ts/src/native.ts","file":"acme-rs/ts/src/native.ts","line":1,"repo":"acme-rs","props":{"language":"typescript"},"relations":[{"kind":"calls","target":"ts/src.load"}]}
8-
{"kind":"file_ref","name":"web/src/api.ts","file":"acme/web/src/api.ts","line":1,"repo":"acme","props":{"language":"typescript"},"relations":[{"kind":"calls","target":"web/src.fetch"}]}
8+
{"kind":"file_ref","name":"web/src/api.ts","file":"acme/web/src/api.ts","line":1,"repo":"acme","props":{"language":"typescript"},"relations":[{"kind":"calls","target":"web/src.body"},{"kind":"calls","target":"web/src.fetch"}]}
99
{"kind":"module","name":".","file":"acme-rs/.","repo":"acme-rs","props":{"language":"go","modulePath":"example.com/acme-rs","package":"main"}}
1010
{"kind":"module","name":"api","file":"acme/api","repo":"acme","props":{"language":"python"}}
1111
{"kind":"module","name":"api/routers","file":"acme/api/routers","repo":"acme","props":{"language":"python"}}
1212
{"kind":"module","name":"ts/src","file":"acme-rs/ts/src","repo":"acme-rs","props":{"language":"typescript","package_name":"@acme/sdk"}}
1313
{"kind":"module","name":"web/src","file":"acme/web/src","repo":"acme","props":{"language":"typescript","package_name":"acme-web"}}
1414
{"kind":"route","name":"/api/v1/search","file":"acme/api/routers/search.py","line":16,"repo":"acme","props":{"framework":"fastapi","language":"python","method":"POST","path":"/api/v1/search","role":"server"}}
15+
{"kind":"route","name":"/api/v1/search","file":"acme/web/src/api.ts","line":17,"repo":"acme","props":{"api":"api","framework":"axios","language":"typescript","method":"POST","role":"client","source":"ts-http-client"},"relations":[{"kind":"declares","target":"web/src"}]}
1516
{"kind":"route","name":"/api/v1/search/results","file":"acme-rs/server.go","line":9,"repo":"acme-rs","props":{"framework":"net/http","handler":"results","language":"go","method":"ALL"},"relations":[{"kind":"declares","target":"."},{"kind":"handled_by","target":"..results"}]}
1617
{"kind":"route","name":"/api/v1/search/results","file":"acme/api/routers/search.py","line":12,"repo":"acme","props":{"framework":"fastapi","language":"python","method":"GET","path":"/api/v1/search/results","role":"server"}}
1718
{"kind":"route","name":"/api/v1/search/results","file":"acme/web/src/api.ts","line":5,"repo":"acme","props":{"api":"api","framework":"fetch","language":"typescript","method":"GET","role":"client","source":"ts-http-client"},"relations":[{"kind":"declares","target":"web/src"}]}
18-
{"kind":"service","name":"acme","repo":"acme","props":{"edge_coverage":[{"detected":1,"edge_type":"http_client","external":0,"resolved":1,"unresolved":0}],"synthetic":"crossrepo"}}
19+
{"kind":"service","name":"acme","repo":"acme","props":{"edge_coverage":[{"detected":2,"edge_type":"http_client","external":0,"resolved":2,"unresolved":0}],"synthetic":"crossrepo"}}
1920
{"kind":"service","name":"acme-rs","repo":"acme-rs","props":{"synthetic":"crossrepo"}}
2021
{"kind":"symbol","name":"..main","file":"acme-rs/server.go","line":8,"repo":"acme-rs","props":{"cyclomatic":1,"exported":false,"language":"go","symbol_kind":"function"},"relations":[{"kind":"calls","target":"net/http.HandleFunc"},{"kind":"calls","target":"net/http.ListenAndServe"},{"kind":"declares","target":"."}]}
2122
{"kind":"symbol","name":"..results","file":"acme-rs/server.go","line":13,"repo":"acme-rs","props":{"cyclomatic":1,"exported":false,"http_handler":true,"language":"go","symbol_kind":"function"},"relations":[{"kind":"declares","target":"."}]}
2223
{"kind":"symbol","name":"api/routers/search.get_search_router","file":"acme/api/routers/search.py","line":9,"repo":"acme","props":{"cyclomatic":1,"exported":true,"language":"python","return_type":"APIRouter","symbol_kind":"function"},"relations":[{"kind":"declares","target":"api/routers"},{"kind":"instantiates","target":"APIRouter"}]}
2324
{"kind":"symbol","name":"ts/src.native","file":"acme-rs/ts/src/native.ts","line":6,"repo":"acme-rs","props":{"exported":true,"language":"typescript","symbol_kind":"variable"},"relations":[{"kind":"declares","target":"ts/src"}]}
2425
{"kind":"symbol","name":"web/src.fetchResults","file":"acme/web/src/api.ts","line":4,"repo":"acme","props":{"cyclomatic":1,"exported":true,"io_direct":true,"language":"typescript","performs_io":true,"symbol_kind":"function"},"relations":[{"kind":"calls","target":"web/src.fetch"},{"kind":"declares","target":"web/src"}]}
26+
{"kind":"symbol","name":"web/src.submitSearch","file":"acme/web/src/api.ts","line":16,"repo":"acme","props":{"cyclomatic":1,"exported":true,"io_direct":true,"language":"typescript","performs_io":true,"symbol_kind":"function"},"relations":[{"kind":"declares","target":"web/src"}]}

internal/engine/testdata/repos/py_fastapi_multirepo/acme/web/src/api.ts

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,3 +5,14 @@ export async function fetchResults() {
55
const res = await fetch("/api/v1/search/results");
66
return res.json();
77
}
8+
9+
// v141 regression fixture. Two things here were invisible before that version, and
10+
// each alone was enough to erase the call: the verb is LOWERCASE (only uppercase
11+
// generated-client verbs were matched, so the dominant axios idiom produced no
12+
// fact), and the type argument is NESTED (the "<[^>]*>" group stopped at the inner
13+
// ">", which also broke the uppercase and fetch paths it was written for). The
14+
// failure was silent — the call was not counted detected, external or unresolved,
15+
// so a cross-repo residual under-reported with no sign that it had.
16+
export async function submitSearch(body: Query) {
17+
return axios.post<ApiResponse<Result[]>>("/api/v1/search", body);
18+
}

internal/extractors/tsextractor/httpclient.go

Lines changed: 43 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,15 @@ import (
2121
// `window.fetch(` / `this.makeRequest(` (preceded by `.`) while rejecting calls
2222
// whose name merely ENDS in "fetch" — `router.prefetch(...)`, `query.refetch(...)`
2323
// — which are navigation/cache primitives, not outbound HTTP.
24-
var httpClientCall = regexp.MustCompile("(?:^|[^\\w])(fetch|makeRequest)\\s*(?:<[^>]*>)?\\s*\\(\\s*(?:\"([^\"]*)\"|'([^']*)'|`([^`]*)`)")
24+
// The optional type-argument group is bounded on "(" rather than ">", because a
25+
// TypeScript type argument is routinely NESTED — fetch<ApiResponse<Foo>>(…) — and
26+
// "<[^>]*>" stops at the inner ">", leaving the following "\s*\(" to meet a ">"
27+
// and fail. RE2 has no recursion, so the empty alternative is no rescue: it then
28+
// meets "<" and fails too, and the call is silently not a call. A type argument
29+
// never contains "(", so "[^()]*" runs greedily to the last ">" before the call
30+
// parenthesis and spans any nesting depth. Same reasoning at verbNamedCall and
31+
// lowerVerbCall — all three shared the defect.
32+
var httpClientCall = regexp.MustCompile("(?:^|[^\\w])(fetch|makeRequest)\\s*(?:<[^()]*>)?\\s*\\(\\s*(?:\"([^\"]*)\"|'([^']*)'|`([^`]*)`)")
2533

2634
// httpClientMethod matches a `method: 'POST'` option within a call's options
2735
// object.
@@ -34,9 +42,30 @@ var httpClientMethod = regexp.MustCompile(`method\s*:\s*['"]([A-Za-z]+)['"]`)
3442
// API.getApi().GET('/api/v3/items/{id}', { params: … })
3543
// ApiV3.getApi().DELETE('/api/v3/widgets/{id}/follow')
3644
//
37-
// Only uppercase verbs are matched: that is the generated-client convention and it
38-
// avoids colliding with ordinary lowercase methods like map.get()/cache.delete().
39-
var verbNamedCall = regexp.MustCompile("\\.(GET|POST|PUT|DELETE|PATCH)\\s*(?:<[^>]*>)?\\s*\\(\\s*(?:\"([^\"]*)\"|'([^']*)'|`([^`]*)`)")
45+
// Only uppercase verbs are matched here: that is the generated-client convention,
46+
// and it avoids colliding with ordinary lowercase methods like map.get()/
47+
// cache.delete(). The lowercase idiom is handled separately by lowerVerbCall,
48+
// which pays for admitting it with a stricter argument rule.
49+
var verbNamedCall = regexp.MustCompile("\\.(GET|POST|PUT|DELETE|PATCH)\\s*(?:<[^()]*>)?\\s*\\(\\s*(?:\"([^\"]*)\"|'([^']*)'|`([^`]*)`)")
50+
51+
// lowerVerbCall matches the hand-written client idiom that verbNamedCall's
52+
// uppercase-only rule deliberately excludes — axios.get('/path'), http.post('/path'),
53+
// apiClient.put('/path') — which is the dominant shape in TypeScript codebases and
54+
// contributed no route fact at all until now.
55+
//
56+
// The collision that motivated the uppercase-only rule (map.get("key"),
57+
// cache.delete(id), searchParams.get("q"), headers.get("content-type")) is answered
58+
// here by requiring the first argument to be a "/"-ROOTED literal. That is not a new
59+
// heuristic: cleanTSPath already rejects every non-"/"-rooted path downstream, so
60+
// admitting one here that it would drop anyway is the only case this widening adds.
61+
// A collection key beginning with "/" is vanishingly rare; a request path not
62+
// beginning with one is not a request path.
63+
//
64+
// Deliberately NOT matched: a lowercase call whose argument is a template starting
65+
// with an interpolation (axios.get(`${base}/x`)). Recovering those needs the base
66+
// resolution of cleanTSPath, and admitting them here would re-open the collision
67+
// this rule closes. They stay missed — see GAP-TS-06 for the base-URL half.
68+
var lowerVerbCall = regexp.MustCompile("\\.(get|post|put|delete|patch)\\s*(?:<[^()]*>)?\\s*\\(\\s*(?:\"(/[^\"]*)\"|'(/[^']*)'|`(/[^`]*)`)")
4069

4170
// urlProperty matches a `url:` object property whose value is a string/template
4271
// literal — the options-object client idiom, e.g.
@@ -207,6 +236,16 @@ func extractHTTPClientFacts(src []byte, relFile string) []facts.Fact {
207236
add(raw, method, "openapi-fetch", m[0])
208237
}
209238

239+
// Pass 2b — lowercase verb-named calls (axios.get('/x'), http.post('/x')). The
240+
// method is the call name, as in pass 2; the "/"-rooted argument requirement
241+
// lives in the pattern (see lowerVerbCall) rather than here, so a collection
242+
// lookup never reaches add() in the first place.
243+
for _, m := range lowerVerbCall.FindAllSubmatchIndex(src, -1) {
244+
method := strings.ToUpper(string(src[m[2]:m[3]]))
245+
raw := firstNonEmptyGroup(src, m, 2, 3, 4)
246+
add(raw, method, "axios", m[0])
247+
}
248+
210249
// Pass 3 — options-object clients: a `url:` property inside an object literal
211250
// that also carries a request-descriptor key, with the verb from a sibling
212251
// `type:`/`method:` (default GET). The scan is scoped to the enclosing object so

0 commit comments

Comments
 (0)