diff --git a/gitnexus/src/cli/analyze.ts b/gitnexus/src/cli/analyze.ts index 0cf17ab13..d399c5082 100644 --- a/gitnexus/src/cli/analyze.ts +++ b/gitnexus/src/cli/analyze.ts @@ -13,7 +13,8 @@ import os from 'os'; import { spawn } from 'child_process'; import v8 from 'v8'; import cliProgress from 'cli-progress'; -import { closeLbug } from '../core/lbug/lbug-adapter.js'; +import { isLbugReady } from '../core/lbug/lbug-adapter.js'; +import { boundedCheckpointBeforeExit } from '../core/lbug/shutdown-helpers.js'; import { isLbugCheckpointIoError, isWalCorruptionError, @@ -738,6 +739,17 @@ export const analyzeCommand = async (inputPath?: string, options?: AnalyzeOption } finally { restoreAnalyzeEnv(envSnap); } + // If analyzeCommandImpl returned via a soft `process.exitCode = 1` error path + // while LadybugDB native handles are still open, the event loop won't drain and + // the process would HANG (#2264 review P1). The full analyze paths skip-close the + // DB — handles are left open and reclaimed by process.exit — so a soft return + // after a real analyze must force the exit. The success path never reaches here + // (analyzeCommandImpl calls process.exit(0) itself); early-validation errors and + // unit tests that mock runFullAnalysis never open the DB, so isLbugReady() is + // false and the soft return is preserved. + if (isLbugReady()) { + process.exit(typeof process.exitCode === 'number' ? process.exitCode : 1); + } }; const analyzeCommandImpl = async ( @@ -1168,13 +1180,18 @@ const analyzeCommandImpl = async ( aborted = true; bar.stop(); console.log('\n Interrupted — cleaning up...'); - closeLbug() - .catch(() => {}) - .finally(async () => { + // Bounded CHECKPOINT-then-exit (#2264 review P3): skip the native close (the + // LadybugDB destructor can double-free after --pdg writes), but don't hang + // behind a long --pdg COPY holding the connection lock — bound it so a single + // Ctrl-C stays responsive; the WAL replays on the next analyze. A second + // Ctrl-C (`if (aborted) process.exit(1)` above) remains the escape hatch. + void boundedCheckpointBeforeExit({ + exitCode: 130, + beforeExit: async () => { const { flushLoggerSync } = await import('../core/logger.js'); flushLoggerSync(); - process.exit(130); - }); + }, + }); }; process.on('SIGINT', sigintHandler); @@ -1273,6 +1290,12 @@ const analyzeCommandImpl = async ( // Extra fetch-wrapper names from `.gitnexusrc` (#1589/#1852 residual); // forwarded to the routes phase consumer scan. fetchWrappers: options.fetchWrappers, + // The CLI always process.exit()s after this returns (success path at the + // end of analyzeCommandImpl, error/interrupt paths via process.exit too), + // so the finalize close skips the native conn/db close — it can double-free + // in LadybugDB's ClientContext destructor after --pdg writes (#2264). The + // CHECKPOINT keeps the index durable; process exit reclaims the handles. + skipNativeCloseOnExit: true, }, { onProgress: (_phase, percent, message) => { diff --git a/gitnexus/src/core/group/extractors/http-patterns/java.ts b/gitnexus/src/core/group/extractors/http-patterns/java.ts index 3ad66543e..5afebbace 100644 --- a/gitnexus/src/core/group/extractors/http-patterns/java.ts +++ b/gitnexus/src/core/group/extractors/http-patterns/java.ts @@ -11,6 +11,22 @@ import { isRouteMemberKey, findEnclosingClass, } from '../../../ingestion/route-extractors/spring-shared.js'; +import { + REST_TEMPLATE_TO_HTTP, + WEB_CLIENT_SHORT_TO_HTTP, + WEB_CLIENT_LONG_VERB_RE, + EXCHANGE_ANNOTATION_TO_HTTP, + parseRequestLine, + pushPrefix, + joinPath, + scanSpringInheritanceProject, + type SharedSpringType, + OPENFEIGN_FRAMEWORK, + HTTP_INTERFACE_FRAMEWORK, + FEIGN_CONFIDENCE, + REQUEST_LINE_CONFIDENCE, + EXCHANGE_CONFIDENCE, +} from './spring-consumer-shared.js'; import type { HttpDetection, HttpFileDetections, @@ -49,38 +65,23 @@ import type { // corrupt every route). That key filtering is done in `isRouteMemberKey`, and // all of these annotations are matched by the one `JAVA_ROUTE_ANNOTATION_PATTERNS` // query below (see its header for why the filtering lives in JS, not the query). -interface SpringRouteBinding { - method: string; - path: string; - ownerPrefix?: string; -} - -interface SpringMethodInfo { - name: string; - routes: SpringRouteBinding[]; -} - -interface SpringTypeInfo { - filePath: string; - kind: 'class' | 'interface'; - name: string; - classPrefix: string; - implementedInterfaces: string[]; - isController: boolean; - methods: SpringMethodInfo[]; -} +// The Spring class/interface view (`SharedSpringType`) and the interface-based +// controller inheritance algorithm (`scanSpringInheritanceProject`) are shared +// with kotlin.ts via spring-consumer-shared.ts so both plugins emit identical +// provider contracts. `collectSpringTypes` below produces that shared shape. // ─── Route-defining annotations (one generic query, one pass) ───────── // Every Java route-mapper annotation shares one shape: an annotation carrying a -// single string argument — positional `"..."` or named `key = "..."` — on a -// class, interface, or method. This SINGLE query matches that shape generically; -// `scanRouteAnnotations` then reads the annotation NAME (`@ann`) and declaration -// kind (`@node.type`) in its for-loop to decide what each match means. Adding a -// new framework annotation that follows this single-string-argument shape is a -// change to that loop (and the lookup maps), not to this query. Annotations with -// a different argument shape — e.g. an array value `@RequestMapping({"/a","/b"})` -// — are out of scope here (as they were for the prior queries) and would need a -// new branch. +// string argument — positional `"..."` or named `key = "..."`, each also in its +// array form `{"..."}` / `key = {"..."}` (Spring's `path`/`value` are +// `String[]`) — on a class, interface, or method. This SINGLE query matches that +// shape generically; the `@value` capture is an alternation over a bare +// `string_literal` and one nested in an `element_value_array_initializer`, so a +// single-element array yields the same `@value` (a multi-element array yields one +// match per element). `scanRouteAnnotations` then reads the annotation NAME +// (`@ann`) and declaration kind (`@node.type`) in its for-loop to decide what +// each match means. Adding a new framework annotation that follows this shape is +// a change to that loop (and the lookup maps), not to this query. // // Captures (shared across all branches; intentionally framework-agnostic): // @ann → the annotation name identifier (RequestMapping, GetMapping, RequestLine, …) @@ -96,6 +97,18 @@ interface SpringTypeInfo { // silently dropping sibling-branch matches. Keeping the query predicate-free // sidesteps that hazard entirely; all name/key discrimination lives in the // for-loop, where it reads as straight-line code. +// +// KNOWN LIMITATION — fully-qualified route annotations are not matched. `@ann` +// binds `name: (identifier)`, but a FQN annotation (`@org.springframework… +// GetMapping("/x")`) parses its name as a `scoped_identifier`, which this query +// does not match, so its route is not extracted. (The class is still recognized +// as a controller — `hasAnnotation` trailing-segment-matches the FQN — only the +// route-string extraction is missed.) In practice annotations are imported and +// written by simple name, so this is rare. It is a minor asymmetry with the +// Kotlin plugin, whose grammar models a FQN as separate `type_identifier` +// segments that its route queries DO match. Aligning Java would mean matching +// `scoped_identifier` too; that is deferred to avoid re-keying existing Java +// contracts via the predicate hazard above. Pinned by an anti-overreach test. const JAVA_ROUTE_ANNOTATION_PATTERNS = compilePatterns({ name: 'java-route-annotation', language: Java, @@ -108,12 +121,12 @@ const JAVA_ROUTE_ANNOTATION_PATTERNS = compilePatterns({ (modifiers (annotation name: (identifier) @ann - arguments: (annotation_argument_list (string_literal) @value)))) @node + arguments: (annotation_argument_list [(string_literal) @value (element_value_array_initializer (string_literal) @value)])))) @node (interface_declaration (modifiers (annotation name: (identifier) @ann - arguments: (annotation_argument_list (string_literal) @value)))) @node + arguments: (annotation_argument_list [(string_literal) @value (element_value_array_initializer (string_literal) @value)])))) @node (class_declaration (modifiers (annotation @@ -121,7 +134,7 @@ const JAVA_ROUTE_ANNOTATION_PATTERNS = compilePatterns({ arguments: (annotation_argument_list (element_value_pair key: (identifier) @key - value: (string_literal) @value))))) @node + value: [(string_literal) @value (element_value_array_initializer (string_literal) @value)]))))) @node (interface_declaration (modifiers (annotation @@ -129,12 +142,12 @@ const JAVA_ROUTE_ANNOTATION_PATTERNS = compilePatterns({ arguments: (annotation_argument_list (element_value_pair key: (identifier) @key - value: (string_literal) @value))))) @node + value: [(string_literal) @value (element_value_array_initializer (string_literal) @value)]))))) @node (method_declaration (modifiers (annotation name: (identifier) @ann - arguments: (annotation_argument_list (string_literal) @value))) + arguments: (annotation_argument_list [(string_literal) @value (element_value_array_initializer (string_literal) @value)]))) name: (identifier) @member) @node (method_declaration (modifiers @@ -143,7 +156,7 @@ const JAVA_ROUTE_ANNOTATION_PATTERNS = compilePatterns({ arguments: (annotation_argument_list (element_value_pair key: (identifier) @key - value: (string_literal) @value)))) + value: [(string_literal) @value (element_value_array_initializer (string_literal) @value)])))) name: (identifier) @member) @node ] `, @@ -167,59 +180,10 @@ const SPRING_TYPE_DECLARATION_PATTERNS = compilePatterns({ ], } satisfies LanguagePatterns>); -// ─── Consumer: OpenFeign `@RequestLine("METHOD /path")` parsing ─────── -// OpenFeign's native annotation pairs an HTTP method and path in a single -// string literal — see https://github.com/OpenFeign/feign#interface-annotations. -// It is method-level only and is mutually exclusive with Spring MVC -// `@GetMapping` / `@PostMapping` etc. on the same method (mixing them -// requires a different Feign Contract — they are not combined). The match -// itself comes from `JAVA_ROUTE_ANNOTATION_PATTERNS`; this regex splits the -// verb from the path of the captured literal. -// -// Examples: -// @RequestLine("GET /users/{id}") -// @RequestLine("POST /users?status=active") -const REQUEST_LINE_VERB_RE = /^\s*(GET|POST|PUT|DELETE|PATCH|HEAD|OPTIONS)\s+(\S.*?)\s*$/i; - -/** - * Parse a Feign `@RequestLine` value into a method + path pair. - * - * `@RequestLine("METHOD /path[?query]")` packs both fields in one string; - * the query portion is dropped because contract IDs are method+path only - * (consistent with how other consumers like RestTemplate/WebClient drop - * query strings when their values are inline literals). - * - * Returns null if the value is not a recognized HTTP verb followed by a - * path beginning with `/`. - */ -function parseRequestLine(raw: string): { method: string; path: string } | null { - const match = REQUEST_LINE_VERB_RE.exec(raw); - if (!match) return null; - const [, verb, rest] = match; - if (typeof verb !== 'string' || typeof rest !== 'string') return null; - const queryIdx = rest.indexOf('?'); - const pathOnly = (queryIdx >= 0 ? rest.slice(0, queryIdx) : rest).trim(); - if (!pathOnly.startsWith('/')) return null; - return { method: verb.toUpperCase(), path: pathOnly }; -} - -// ─── Consumer: Spring RestTemplate (object-named + method-named) ────── -// RestTemplate.getForObject / getForEntity → GET -// RestTemplate.postForObject / postForEntity → POST -// RestTemplate.put → PUT -// RestTemplate.delete → DELETE -// RestTemplate.patchForObject → PATCH -// Source-scan only: receiver must be named exactly `restTemplate`. -// Fields, `this.restTemplate`, aliases, and other injection names are deferred. -const REST_TEMPLATE_TO_HTTP: Record = { - getForObject: 'GET', - getForEntity: 'GET', - postForObject: 'POST', - postForEntity: 'POST', - put: 'PUT', - delete: 'DELETE', - patchForObject: 'PATCH', -}; +// OpenFeign `@RequestLine` parsing (`parseRequestLine`), the RestTemplate and +// WebClient short-form verb maps, the `@*Exchange` verb map, `joinPath`, and the +// shared confidence/framework constants live in `spring-consumer-shared.ts` so +// the Java and Kotlin plugins emit identical contract IDs. interface RestTemplateMeta { framework: 'spring-rest-template'; @@ -261,14 +225,6 @@ const REST_TEMPLATE_EXCHANGE_PATTERNS = compilePatterns({ ], } satisfies LanguagePatterns); -const WEB_CLIENT_SHORT_TO_HTTP: Record = { - get: 'GET', - post: 'POST', - put: 'PUT', - delete: 'DELETE', - patch: 'PATCH', -}; - const WEB_CLIENT_SHORT_FORM_PATTERNS = compilePatterns({ name: 'java-web-client-short-form', language: Java, @@ -288,6 +244,37 @@ const WEB_CLIENT_SHORT_FORM_PATTERNS = compilePatterns({ ], } satisfies LanguagePatterns>); +// ─── Consumer: WebClient long form `webClient.method(HttpMethod.X).uri("/y")` ─ +// The fluent long form carries the verb as a `HttpMethod.X` field access through +// `.method(...)` and the path on a separate `.uri(...)` hop. A single structural +// query matches the whole chain (the same field-access shape used by +// REST_TEMPLATE_EXCHANGE_PATTERNS) — the earlier "intentionally deferred" note +// predated the Kotlin plugin proving the structural query is enough. Variable- +// bound verbs (`webClient.method(verb).uri(...)`) do NOT match: the value carries +// a bare `identifier`, not a `HttpMethod.X` field access — source-scan can't +// follow the binding (anti-overreach test pins this, parity with Kotlin). +const WEB_CLIENT_LONG_FORM_PATTERNS = compilePatterns({ + name: 'java-web-client-long-form', + language: Java, + patterns: [ + { + meta: {}, + query: ` + (method_invocation + object: (method_invocation + object: (identifier) @obj (#eq? @obj "webClient") + name: (identifier) @method_call (#eq? @method_call "method") + arguments: (argument_list + (field_access + object: (identifier) @httpMethodCls (#eq? @httpMethodCls "HttpMethod") + field: (identifier) @verb))) + name: (identifier) @uri_method (#eq? @uri_method "uri") + arguments: (argument_list . (string_literal) @path)) + `, + }, + ], +} satisfies LanguagePatterns>); + // ─── Consumer: OkHttp `new Request.Builder().url("path")` ───────────── // Note: `Request.Builder` is a `scoped_type_identifier` whose text includes // the dot, so `#eq?` against the literal string matches cleanly (no need @@ -370,38 +357,6 @@ function findEnclosingInterface(node: Parser.SyntaxNode): Parser.SyntaxNode | nu return null; } -/** - * Join a class-level prefix and a method-level path into a single URL - * path. Mirrors the semantics of the original regex implementation: - * strip trailing slashes on the prefix, then ensure a single slash - * between prefix and method path. - */ -function joinPath(prefix: string, methodPath: string): string { - const cleanPrefix = prefix.replace(/^\/+/, '').replace(/\/+$/, ''); - const cleanSub = methodPath.replace(/^\/+/, ''); - if (!cleanPrefix) return `/${cleanSub}`; - return `/${cleanPrefix}/${cleanSub}`; -} - -function joinInheritedSpringPath( - controllerPrefix: string, - inheritedPath: string, - inheritedOwnerPrefix = '', -): string { - const joined = joinPath(controllerPrefix, inheritedPath); - const cleanPrefix = controllerPrefix.replace(/^\/+/, '').replace(/\/+$/, ''); - const cleanOwnerPrefix = inheritedOwnerPrefix.replace(/^\/+/, '').replace(/\/+$/, ''); - const cleanInherited = inheritedPath.replace(/^\/+/, ''); - if (!cleanPrefix) return joined; - if ( - cleanPrefix === cleanOwnerPrefix && - (cleanInherited === cleanPrefix || cleanInherited.startsWith(`${cleanPrefix}/`)) - ) { - return `/${cleanInherited}`; - } - return joined; -} - function getNodeName(node: Parser.SyntaxNode): string | null { return node.childForFieldName('name')?.text ?? null; } @@ -440,14 +395,18 @@ interface RequestLineAnnotation { } interface RouteAnnotationScan { - /** Spring `@RequestMapping` URL prefix per class/interface node id (last write wins). */ - prefixByTypeId: Map; - /** OpenFeign interface prefix per interface node id; `@FeignClient(path)` wins over `@RequestMapping`. */ - feignPrefixByInterfaceId: Map; + /** Spring `@RequestMapping` URL prefixes per class/interface node id (one per array element). */ + prefixByTypeId: Map; + /** OpenFeign interface prefixes per interface node id; `@FeignClient(path)` wins over `@RequestMapping`. */ + feignPrefixByInterfaceId: Map; + /** Spring HTTP Interface `@HttpExchange(url|value)` type-level prefixes per class/interface node id. */ + httpExchangePrefixByTypeId: Map; /** One entry per resolved Spring `@(Get|...)Mapping` route — a method with N mappings yields N entries. */ methodRoutes: MethodRouteAnnotation[]; /** One entry per OpenFeign `@RequestLine` whose value parses to a verb + path. */ requestLines: RequestLineAnnotation[]; + /** One entry per Spring HTTP Interface `@(Get|...)Exchange` method — always a consumer. */ + exchangeRoutes: MethodRouteAnnotation[]; } /** @@ -468,13 +427,17 @@ function scanRouteAnnotations(tree: Parser.Tree): RouteAnnotationScan { // collectSpringTypes cross-file inheritance), while `feignPrefixByInterfaceId` // feeds the OpenFeign *consumer* path in scan(). An interface carrying both // `@RequestMapping` and `@FeignClient(path)` lands a different value in each. - const prefixByTypeId = new Map(); - const feignPrefixByInterfaceId = new Map(); + const prefixByTypeId = new Map(); + const feignPrefixByInterfaceId = new Map(); + const httpExchangePrefixByTypeId = new Map(); const methodRoutes: MethodRouteAnnotation[] = []; const requestLines: RequestLineAnnotation[] = []; + const exchangeRoutes: MethodRouteAnnotation[] = []; // Interface `@RequestMapping` prefixes rank below `@FeignClient(path)`; // collect them and apply only after the FeignClient pass below. const interfaceRequestMappingPrefixes: Array<{ id: number; prefix: string }> = []; + // `pushPrefix` (the de-duping accumulator) is shared from + // spring-consumer-shared.ts so Java and Kotlin build prefix maps identically. for (const { captures } of matches) { const annNode = captures.ann; @@ -510,6 +473,20 @@ function scanRouteAnnotations(tree: Parser.Tree): RouteAnnotationScan { parsed, }); } + } else if (EXCHANGE_ANNOTATION_TO_HTTP[ann]) { + // Spring 6 HTTP Interface `@(Get|...)Exchange` — the path lives in the + // `url` or `value` attribute (or positionally); other attributes + // (`accept`, `contentType`, …) are not routes. + if (keyNode && keyNode.text !== 'url' && keyNode.text !== 'value') continue; + const rawPath = unquoteLiteral(valueNode.text); + if (rawPath !== null) { + exchangeRoutes.push({ + methodNode: node, + methodName: captures.member?.text ?? null, + httpMethod: EXCHANGE_ANNOTATION_TO_HTTP[ann], + rawPath, + }); + } } continue; } @@ -520,7 +497,7 @@ function scanRouteAnnotations(tree: Parser.Tree): RouteAnnotationScan { if (!isRouteMemberKey(keyNode)) continue; const prefix = unquoteLiteral(valueNode.text); if (prefix !== null) { - prefixByTypeId.set(node.id, prefix); + pushPrefix(prefixByTypeId, node.id, prefix); if (node.type === 'interface_declaration') { interfaceRequestMappingPrefixes.push({ id: node.id, prefix }); } @@ -529,17 +506,30 @@ function scanRouteAnnotations(tree: Parser.Tree): RouteAnnotationScan { // Feign's `name`/`value` identify a service, not a path — only `path` is a prefix. if (!keyNode || keyNode.text !== 'path') continue; const prefix = unquoteLiteral(valueNode.text); - if (prefix !== null && !feignPrefixByInterfaceId.has(node.id)) { - feignPrefixByInterfaceId.set(node.id, prefix); - } + if (prefix !== null) pushPrefix(feignPrefixByInterfaceId, node.id, prefix); + } else if (ann === 'HttpExchange') { + // Spring HTTP Interface type-level prefix: the path lives in `url`/`value` + // (or positionally). Applies to its `@(Get|...)Exchange` consumer methods. + if (keyNode && keyNode.text !== 'url' && keyNode.text !== 'value') continue; + const prefix = unquoteLiteral(valueNode.text); + if (prefix !== null) pushPrefix(httpExchangePrefixByTypeId, node.id, prefix); } } + // `@RequestMapping` on a Feign interface is the fallback prefix, but only when + // the interface has no `@FeignClient(path)` of its own (path wins). for (const { id, prefix } of interfaceRequestMappingPrefixes) { - if (!feignPrefixByInterfaceId.has(id)) feignPrefixByInterfaceId.set(id, prefix); + if (!feignPrefixByInterfaceId.has(id)) pushPrefix(feignPrefixByInterfaceId, id, prefix); } - return { prefixByTypeId, feignPrefixByInterfaceId, methodRoutes, requestLines }; + return { + prefixByTypeId, + feignPrefixByInterfaceId, + httpExchangePrefixByTypeId, + methodRoutes, + requestLines, + exchangeRoutes, + }; } function collectDirectMethods(typeNode: Parser.SyntaxNode): Parser.SyntaxNode[] { @@ -578,15 +568,15 @@ function collectImplementedInterfaces(typeNode: Parser.SyntaxNode): string[] { return out; } -function collectSpringTypes(filePath: string, tree: Parser.Tree): SpringTypeInfo[] { +function collectSpringTypes(filePath: string, tree: Parser.Tree): SharedSpringType[] { const { prefixByTypeId, methodRoutes } = scanRouteAnnotations(tree); - const routesByMethodId = new Map(); + const routesByMethodId = new Map>(); for (const route of methodRoutes) { const routes = routesByMethodId.get(route.methodNode.id) ?? []; routes.push({ method: route.httpMethod, path: route.rawPath }); routesByMethodId.set(route.methodNode.id, routes); } - const out: SpringTypeInfo[] = []; + const out: SharedSpringType[] = []; for (const match of runCompiledPatterns(SPRING_TYPE_DECLARATION_PATTERNS, tree)) { const typeNode = match.captures.type; @@ -598,13 +588,16 @@ function collectSpringTypes(filePath: string, tree: Parser.Tree): SpringTypeInfo name: getNodeName(methodNode), routes: routesByMethodId.get(methodNode.id) ?? [], })) - .filter((method): method is SpringMethodInfo => method.name !== null); + .filter( + (method): method is { name: string; routes: Array<{ method: string; path: string }> } => + method.name !== null, + ); out.push({ filePath, kind, name: typeNameNode.text, - classPrefix: prefixByTypeId.get(typeNode.id) ?? '', + classPrefixes: prefixByTypeId.get(typeNode.id) ?? [], implementedInterfaces: kind === 'class' ? collectImplementedInterfaces(typeNode) : [], isController: kind === 'class' && hasAnnotation(typeNode, ['RestController', 'Controller']), methods, @@ -614,62 +607,13 @@ function collectSpringTypes(filePath: string, tree: Parser.Tree): SpringTypeInfo return out; } +// The interface-based-controller inheritance algorithm is shared with kotlin.ts +// (`scanSpringInheritanceProject`); this collects the `SharedSpringType` view and +// delegates so Java and Kotlin emit byte-identical provider contracts. function scanSpringProject(files: readonly HttpScanInput[]): HttpFileDetections[] { - const types = files.flatMap((file) => collectSpringTypes(file.filePath, file.tree)); - const interfaceRoutes = new Map | null>(); - - for (const type of types) { - if (type.kind !== 'interface') continue; - if (interfaceRoutes.has(type.name)) { - interfaceRoutes.set(type.name, null); - continue; - } - const methodMap = new Map(); - for (const method of type.methods) { - const routes = method.routes.map((route) => ({ - method: route.method, - path: type.classPrefix ? joinPath(type.classPrefix, route.path) : route.path, - ownerPrefix: type.classPrefix, - })); - if (routes.length > 0) methodMap.set(method.name, routes); - } - interfaceRoutes.set(type.name, methodMap); - } - - const detectionsByFile = new Map(); - for (const type of types) { - if (type.kind !== 'class' || !type.isController) continue; - for (const method of type.methods) { - if (method.routes.length > 0) continue; - const inheritedRoutes = type.implementedInterfaces.flatMap((interfaceName) => { - const routeMap = interfaceRoutes.get(interfaceName); - if (!routeMap) return []; - const routes = routeMap.get(method.name) ?? []; - return routes.map((route) => ({ - method: route.method, - path: joinInheritedSpringPath(type.classPrefix, route.path, route.ownerPrefix), - })); - }); - - for (const route of inheritedRoutes) { - const detections = detectionsByFile.get(type.filePath) ?? []; - detections.push({ - role: 'provider', - framework: 'spring', - method: route.method, - path: route.path, - name: method.name, - confidence: 0.8, - }); - detectionsByFile.set(type.filePath, detections); - } - } - } - - return [...detectionsByFile.entries()].map(([filePath, detections]) => ({ - filePath, - detections, - })); + return scanSpringInheritanceProject( + files.flatMap((file) => collectSpringTypes(file.filePath, file.tree)), + ); } export const JAVA_HTTP_PLUGIN: HttpLanguagePlugin = { @@ -682,8 +626,14 @@ export const JAVA_HTTP_PLUGIN: HttpLanguagePlugin = { // `scanRouteAnnotations` resolves every route-defining annotation — // class/interface prefixes, method `@(Get|...)Mapping`s and native // `@RequestLine`s — from a single `matches()` pass over the tree. - const { prefixByTypeId, feignPrefixByInterfaceId, methodRoutes, requestLines } = - scanRouteAnnotations(tree); + const { + prefixByTypeId, + feignPrefixByInterfaceId, + httpExchangePrefixByTypeId, + methodRoutes, + requestLines, + exchangeRoutes, + } = scanRouteAnnotations(tree); // A `@(Get|...)Mapping` inside a `@FeignClient` interface is an OpenFeign // *consumer* (it describes a remote call); the same annotation inside a @@ -693,28 +643,34 @@ export const JAVA_HTTP_PLUGIN: HttpLanguagePlugin = { for (const route of methodRoutes) { const enclosingInterface = findEnclosingInterface(route.methodNode); if (enclosingInterface && hasAnnotation(enclosingInterface, 'FeignClient')) { - const prefix = feignPrefixByInterfaceId.get(enclosingInterface.id) ?? ''; - out.push({ - role: 'consumer', - framework: 'openfeign', - method: route.httpMethod, - path: joinPath(prefix, route.rawPath), - name: route.methodName, - confidence: 0.7, - }); + const prefixes = feignPrefixByInterfaceId.get(enclosingInterface.id) ?? ['']; + for (const prefix of prefixes) { + out.push({ + role: 'consumer', + framework: OPENFEIGN_FRAMEWORK, + method: route.httpMethod, + path: joinPath(prefix, route.rawPath), + name: route.methodName, + confidence: FEIGN_CONFIDENCE, + }); + } continue; } const enclosingClass = findEnclosingClass(route.methodNode); if (!enclosingClass) continue; - const prefix = prefixByTypeId.get(enclosingClass.id) ?? ''; - out.push({ - role: 'provider', - framework: 'spring', - method: route.httpMethod, - path: joinPath(prefix, route.rawPath), - name: route.methodName, - confidence: 0.8, - }); + // A multi-element class `@RequestMapping({"/a","/b"})` registers the method + // under each prefix — emit one provider per (prefix × this route). + const prefixes = prefixByTypeId.get(enclosingClass.id) ?? ['']; + for (const prefix of prefixes) { + out.push({ + role: 'provider', + framework: 'spring', + method: route.httpMethod, + path: joinPath(prefix, route.rawPath), + name: route.methodName, + confidence: 0.8, + }); + } } // Native OpenFeign `@RequestLine("METHOD /path")`. Method-level only and @@ -730,15 +686,38 @@ export const JAVA_HTTP_PLUGIN: HttpLanguagePlugin = { for (const requestLine of requestLines) { const enclosingInterface = findEnclosingInterface(requestLine.methodNode); if (!enclosingInterface) continue; - const prefix = feignPrefixByInterfaceId.get(enclosingInterface.id) ?? ''; - out.push({ - role: 'consumer', - framework: 'openfeign', - method: requestLine.parsed.method, - path: joinPath(prefix, requestLine.parsed.path), - name: requestLine.methodName, - confidence: 0.75, - }); + const prefixes = feignPrefixByInterfaceId.get(enclosingInterface.id) ?? ['']; + for (const prefix of prefixes) { + out.push({ + role: 'consumer', + framework: OPENFEIGN_FRAMEWORK, + method: requestLine.parsed.method, + path: joinPath(prefix, requestLine.parsed.path), + name: requestLine.methodName, + confidence: REQUEST_LINE_CONFIDENCE, + }); + } + } + + // ─── Consumers: Spring HTTP Interface @(Get|...)Exchange ──────── + // Declarative client interfaces proxied by `HttpServiceProxyFactory` + // (over RestClient / WebClient / RestTemplate). Always a consumer — no + // provider ambiguity — with an optional type-level `@HttpExchange(url)` + // prefix. The verb comes from the annotation name (`@GetExchange` → GET). + for (const route of exchangeRoutes) { + const enclosing = + findEnclosingInterface(route.methodNode) ?? findEnclosingClass(route.methodNode); + const prefixes = enclosing ? (httpExchangePrefixByTypeId.get(enclosing.id) ?? ['']) : ['']; + for (const prefix of prefixes) { + out.push({ + role: 'consumer', + framework: HTTP_INTERFACE_FRAMEWORK, + method: route.httpMethod, + path: joinPath(prefix, route.rawPath), + name: route.methodName, + confidence: EXCHANGE_CONFIDENCE, + }); + } } // ─── Consumers: RestTemplate ──────────────────────────────────── @@ -777,9 +756,9 @@ export const JAVA_HTTP_PLUGIN: HttpLanguagePlugin = { } // ─── Consumers: WebClient.get().uri("path") short form ───────── - // Source-scan only: receiver must be named exactly `webClient`. - // The real long-form chain `webClient.method(HttpMethod.X).uri("/x")` - // needs multi-hop chain analysis and is intentionally deferred. + // Source-scan only: receiver must be named exactly `webClient`. The + // long-form chain `webClient.method(HttpMethod.X).uri("/x")` is handled + // separately below by WEB_CLIENT_LONG_FORM_PATTERNS. for (const match of runCompiledPatterns(WEB_CLIENT_SHORT_FORM_PATTERNS, tree)) { const verbNode = match.captures.verb; const pathNode = match.captures.path; @@ -798,6 +777,29 @@ export const JAVA_HTTP_PLUGIN: HttpLanguagePlugin = { }); } + // ─── Consumers: WebClient.method(HttpMethod.X).uri("path") long form ─ + // The verb is captured as the literal `HttpMethod.X` field name; gate it on + // the shared verb regex (HEAD/OPTIONS/TRACE excluded, matching the short + // form). The short-form query requires an empty inner argument list, so it + // cannot also fire on this chain — no double-emit. + for (const match of runCompiledPatterns(WEB_CLIENT_LONG_FORM_PATTERNS, tree)) { + const verbNode = match.captures.verb; + const pathNode = match.captures.path; + if (!verbNode || !pathNode) continue; + const verbText = verbNode.text; + if (!WEB_CLIENT_LONG_VERB_RE.test(verbText)) continue; + const path = unquoteLiteral(pathNode.text); + if (path === null) continue; + out.push({ + role: 'consumer', + framework: 'spring-web-client', + method: verbText, + path, + name: null, + confidence: 0.7, + }); + } + // ─── Consumers: OkHttp Request.Builder().url("path") ──────────── for (const match of runCompiledPatterns(OK_HTTP_PATTERNS, tree)) { const pathNode = match.captures.path; diff --git a/gitnexus/src/core/group/extractors/http-patterns/kotlin.ts b/gitnexus/src/core/group/extractors/http-patterns/kotlin.ts index 14dce0ae1..4286ba758 100644 --- a/gitnexus/src/core/group/extractors/http-patterns/kotlin.ts +++ b/gitnexus/src/core/group/extractors/http-patterns/kotlin.ts @@ -1,4 +1,4 @@ -import Parser from 'tree-sitter'; +import type Parser from 'tree-sitter'; import { requireVendoredGrammar } from '../../../tree-sitter/vendored-grammars.js'; import { compilePatterns, @@ -6,7 +6,32 @@ import { unquoteLiteral, type LanguagePatterns, } from '../tree-sitter-scanner.js'; -import type { HttpDetection, HttpLanguagePlugin } from './types.js'; +import type { + HttpDetection, + HttpFileDetections, + HttpLanguagePlugin, + HttpScanInput, +} from './types.js'; +import { + METHOD_ANNOTATION_TO_HTTP, + findEnclosingClass, +} from '../../../ingestion/route-extractors/spring-shared.js'; +import { + REST_TEMPLATE_TO_HTTP, + WEB_CLIENT_SHORT_TO_HTTP, + WEB_CLIENT_LONG_VERB_RE, + EXCHANGE_ANNOTATION_TO_HTTP, + parseRequestLine, + pushPrefix, + joinPath, + scanSpringInheritanceProject, + type SharedSpringType, + OPENFEIGN_FRAMEWORK, + HTTP_INTERFACE_FRAMEWORK, + FEIGN_CONFIDENCE, + REQUEST_LINE_CONFIDENCE, + EXCHANGE_CONFIDENCE, +} from './spring-consumer-shared.js'; /** * Kotlin HTTP plugin (Spring providers + consumers). @@ -74,53 +99,37 @@ try { Kotlin = null; } -const METHOD_ANNOTATION_TO_HTTP: Record = { - GetMapping: 'GET', - PostMapping: 'POST', - PutMapping: 'PUT', - DeleteMapping: 'DELETE', - PatchMapping: 'PATCH', -}; +// The Spring `@(Get|...)Mapping` verb map, RestTemplate / WebClient short-form +// verb maps, the `@(Get|...)Exchange` verb map, `joinPath`, `parseRequestLine`, +// and the shared confidence/framework constants are imported from +// `spring-consumer-shared.ts` / `spring-shared.ts` so the Kotlin and Java +// plugins emit identical contract IDs. + +// The WebClient long-form verb gate (`WEB_CLIENT_LONG_VERB_RE`) is imported from +// `spring-consumer-shared.ts` so the Java and Kotlin long-form scans accept the +// same verb set (HEAD/OPTIONS/TRACE excluded, matching the short form). + +// The de-duping prefix accumulator (`pushPrefix`) is imported from +// `spring-consumer-shared.ts` so the Java and Kotlin plugins build their +// per-declaration prefix maps identically. /** - * RestTemplate method-name → HTTP verb. Mirrors the Java plugin's - * `REST_TEMPLATE_TO_HTTP` (java.ts) so a polyglot repo emits the - * same contract IDs from .java and .kt sources. + * Tree-sitter sub-pattern for the Kotlin `arrayOf("/a", "/b")` annotation-array + * form, capturing each element string under `cap` (`@prefix` or `@path`). + * + * Kept as a DEDICATED query fragment embedded in its own pattern — NEVER as an + * arm of the `[(string_literal) (collection_literal …)]` alternation. The + * `#eq? @arrayOf "arrayOf"` predicate, sharing a single alternation bucket with + * the string/collection arms, would evaluate FALSE for those arms (where + * `@arrayOf` is absent) and silently drop them — the tree-sitter 0.21.x hazard + * documented in `java.ts`. tree-sitter yields one match per `arrayOf` element, + * so multi-element arrays accumulate through the same loops as `collection_literal` + * (verified by AST probe). The `arrayOf` callee constraint keeps unrelated calls + * (`buildPath("/x")`) from matching. */ -const REST_TEMPLATE_TO_HTTP: Record = { - getForObject: 'GET', - getForEntity: 'GET', - postForObject: 'POST', - postForEntity: 'POST', - put: 'PUT', - delete: 'DELETE', - patchForObject: 'PATCH', -}; - -/** - * WebClient short-form verb → HTTP verb. The reactive WebClient API - * exposes `.get()`, `.post()`, `.put()`, `.delete()`, `.patch()` as - * one-liners that return a `RequestHeadersUriSpec` whose `.uri(...)` - * carries the path. We capture both pieces in a single query (see - * `WEB_CLIENT_SHORT_PATTERNS` below) and translate the verb here. - */ -const WEB_CLIENT_SHORT_TO_HTTP: Record = { - get: 'GET', - post: 'POST', - put: 'PUT', - delete: 'DELETE', - patch: 'PATCH', -}; - -/** - * Allowed HTTP verbs for the WebClient long-form path - * `webClient.method(HttpMethod.X).uri("/y")`. Compiled once at module - * load (instead of inside the scan loop) per maintainer feedback on - * PR #1884. Mirrors the keys of `WEB_CLIENT_SHORT_TO_HTTP` above — - * keeping HEAD/OPTIONS/TRACE intentionally excluded for symmetry - * with the short form and the Java plugin. - */ -const WEB_CLIENT_LONG_VERB_RE = /^(GET|POST|PUT|DELETE|PATCH)$/; +const arrayOfArg = (cap: string): string => `(call_expression + (simple_identifier) @arrayOf (#eq? @arrayOf "arrayOf") + (call_suffix (value_arguments (value_argument (string_literal) ${cap}))))`; /** * Build the plugin only if the Kotlin grammar is available. Compiling @@ -161,7 +170,7 @@ function buildKotlinPlugin(language: unknown): HttpLanguagePlugin { (constructor_invocation (user_type (type_identifier) @ann (#eq? @ann "RequestMapping")) (value_arguments - (value_argument . (string_literal) @prefix))))) + (value_argument . [(string_literal) @prefix (collection_literal (string_literal) @prefix)]))))) (type_identifier) @cls) @class `, }, @@ -176,7 +185,35 @@ function buildKotlinPlugin(language: unknown): HttpLanguagePlugin { (value_arguments (value_argument (simple_identifier) @key (#match? @key "^(path|value)$") - (string_literal) @prefix))))) + [(string_literal) @prefix (collection_literal (string_literal) @prefix)]))))) + (type_identifier) @cls) @class + `, + }, + { + meta: {}, + query: ` + (class_declaration + (modifiers + (annotation + (constructor_invocation + (user_type (type_identifier) @ann (#eq? @ann "RequestMapping")) + (value_arguments + (value_argument . ${arrayOfArg('@prefix')}))))) + (type_identifier) @cls) @class + `, + }, + { + meta: {}, + query: ` + (class_declaration + (modifiers + (annotation + (constructor_invocation + (user_type (type_identifier) @ann (#eq? @ann "RequestMapping")) + (value_arguments + (value_argument + (simple_identifier) @key (#match? @key "^(path|value)$") + ${arrayOfArg('@prefix')}))))) (type_identifier) @cls) @class `, }, @@ -200,7 +237,7 @@ function buildKotlinPlugin(language: unknown): HttpLanguagePlugin { (constructor_invocation (user_type (type_identifier) @ann (#match? @ann "^(Get|Post|Put|Delete|Patch)Mapping$")) (value_arguments - (value_argument . (string_literal) @path))))) + (value_argument . [(string_literal) @path (collection_literal (string_literal) @path)]))))) (simple_identifier) @method_name) @method `, }, @@ -215,7 +252,35 @@ function buildKotlinPlugin(language: unknown): HttpLanguagePlugin { (value_arguments (value_argument (simple_identifier) @key (#match? @key "^(path|value)$") - (string_literal) @path))))) + [(string_literal) @path (collection_literal (string_literal) @path)]))))) + (simple_identifier) @method_name) @method + `, + }, + { + meta: {}, + query: ` + (function_declaration + (modifiers + (annotation + (constructor_invocation + (user_type (type_identifier) @ann (#match? @ann "^(Get|Post|Put|Delete|Patch)Mapping$")) + (value_arguments + (value_argument . ${arrayOfArg('@path')}))))) + (simple_identifier) @method_name) @method + `, + }, + { + meta: {}, + query: ` + (function_declaration + (modifiers + (annotation + (constructor_invocation + (user_type (type_identifier) @ann (#match? @ann "^(Get|Post|Put|Delete|Patch)Mapping$")) + (value_arguments + (value_argument + (simple_identifier) @key (#match? @key "^(path|value)$") + ${arrayOfArg('@path')}))))) (simple_identifier) @method_name) @method `, }, @@ -400,31 +465,362 @@ function buildKotlinPlugin(language: unknown): HttpLanguagePlugin { ], } satisfies LanguagePatterns>); - /** - * Find the nearest enclosing class_declaration ancestor for a node, or - * null if the node is top-level. Mirrors the Java plugin's helper. - */ - function findEnclosingClass(node: Parser.SyntaxNode): Parser.SyntaxNode | null { - let cur: Parser.SyntaxNode | null = node.parent; - while (cur) { - if (cur.type === 'class_declaration') return cur; - cur = cur.parent; - } - return null; - } + // ─── Consumer (OpenFeign): @FeignClient interface marker + path prefix ─ + // A `@FeignClient` interface's `@(Get|...)Mapping` methods describe OUTBOUND + // calls (consumers), not routes the service serves. Pattern 1 marks the + // interface; pattern 2 captures its optional `path = "/prefix"` (the + // `name`/`value`/`url` attributes identify the remote service, not a path). + // In tree-sitter-kotlin an `interface` is a `class_declaration`, so the + // method-route loop reclassifies @*Mapping methods whose enclosing + // class_declaration is in `feignClassIds` (see scan()). + const SPRING_FEIGN_CLIENT_PATTERNS = compilePatterns({ + name: 'kotlin-spring-feign-client', + language, + patterns: [ + { + meta: {}, + query: ` + (class_declaration + (modifiers + (annotation + (constructor_invocation + (user_type (type_identifier) @ann (#eq? @ann "FeignClient")))))) @class + `, + }, + { + meta: {}, + query: ` + (class_declaration + (modifiers + (annotation + (constructor_invocation + (user_type (type_identifier) @ann (#eq? @ann "FeignClient")) + (value_arguments + (value_argument + (simple_identifier) @key (#eq? @key "path") + [(string_literal) @prefix (collection_literal (string_literal) @prefix)])))))) @class + `, + }, + { + meta: {}, + query: ` + (class_declaration + (modifiers + (annotation + (constructor_invocation + (user_type (type_identifier) @ann (#eq? @ann "FeignClient")) + (value_arguments + (value_argument + (simple_identifier) @key (#eq? @key "path") + ${arrayOfArg('@prefix')})))))) @class + `, + }, + ], + } satisfies LanguagePatterns>); + + // ─── Consumer: Spring 6 HTTP Interface @(Get|...)Exchange ───────────── + // Declarative client interfaces proxied by HttpServiceProxyFactory (over + // RestClient / WebClient / RestTemplate). The path lives in `url`/`value` + // (named) or positionally. Always a consumer — no provider ambiguity. + const SPRING_EXCHANGE_PATTERNS = compilePatterns({ + name: 'kotlin-spring-http-exchange', + language, + patterns: [ + { + meta: {}, + query: ` + (function_declaration + (modifiers + (annotation + (constructor_invocation + (user_type (type_identifier) @ann (#match? @ann "^(Get|Post|Put|Delete|Patch)Exchange$")) + (value_arguments + (value_argument . [(string_literal) @path (collection_literal (string_literal) @path)]))))) + (simple_identifier) @method_name) @method + `, + }, + { + meta: {}, + query: ` + (function_declaration + (modifiers + (annotation + (constructor_invocation + (user_type (type_identifier) @ann (#match? @ann "^(Get|Post|Put|Delete|Patch)Exchange$")) + (value_arguments + (value_argument + (simple_identifier) @key (#match? @key "^(url|value)$") + [(string_literal) @path (collection_literal (string_literal) @path)]))))) + (simple_identifier) @method_name) @method + `, + }, + { + meta: {}, + query: ` + (function_declaration + (modifiers + (annotation + (constructor_invocation + (user_type (type_identifier) @ann (#match? @ann "^(Get|Post|Put|Delete|Patch)Exchange$")) + (value_arguments + (value_argument . ${arrayOfArg('@path')}))))) + (simple_identifier) @method_name) @method + `, + }, + { + meta: {}, + query: ` + (function_declaration + (modifiers + (annotation + (constructor_invocation + (user_type (type_identifier) @ann (#match? @ann "^(Get|Post|Put|Delete|Patch)Exchange$")) + (value_arguments + (value_argument + (simple_identifier) @key (#match? @key "^(url|value)$") + ${arrayOfArg('@path')}))))) + (simple_identifier) @method_name) @method + `, + }, + ], + } satisfies LanguagePatterns>); + + // ─── Consumer: HTTP Interface type-level @HttpExchange(url) prefix ───── + const SPRING_HTTP_EXCHANGE_CLASS_PATTERNS = compilePatterns({ + name: 'kotlin-spring-http-exchange-class', + language, + patterns: [ + { + meta: {}, + query: ` + (class_declaration + (modifiers + (annotation + (constructor_invocation + (user_type (type_identifier) @ann (#eq? @ann "HttpExchange")) + (value_arguments + (value_argument . [(string_literal) @prefix (collection_literal (string_literal) @prefix)])))))) @class + `, + }, + { + meta: {}, + query: ` + (class_declaration + (modifiers + (annotation + (constructor_invocation + (user_type (type_identifier) @ann (#eq? @ann "HttpExchange")) + (value_arguments + (value_argument + (simple_identifier) @key (#match? @key "^(url|value)$") + [(string_literal) @prefix (collection_literal (string_literal) @prefix)])))))) @class + `, + }, + { + meta: {}, + query: ` + (class_declaration + (modifiers + (annotation + (constructor_invocation + (user_type (type_identifier) @ann (#eq? @ann "HttpExchange")) + (value_arguments + (value_argument . ${arrayOfArg('@prefix')})))))) @class + `, + }, + { + meta: {}, + query: ` + (class_declaration + (modifiers + (annotation + (constructor_invocation + (user_type (type_identifier) @ann (#eq? @ann "HttpExchange")) + (value_arguments + (value_argument + (simple_identifier) @key (#match? @key "^(url|value)$") + ${arrayOfArg('@prefix')})))))) @class + `, + }, + ], + } satisfies LanguagePatterns>); + + // ─── Consumer: OpenFeign native @RequestLine("VERB /path") ──────────── + // Two patterns mirror the positional vs named split. java.ts accepts the + // named `value` argument (java.ts:442 drops any non-`value` key); the + // positional pattern's `.` anchor only matches when the string literal is the + // first argument, so the named form needs its own pattern. Constraining + // `#eq? @key "value"` keeps non-`value` keys (`name`, etc.) dropped — Java + // parity, just enforced in the query rather than the JS loop. + const SPRING_REQUEST_LINE_PATTERNS = compilePatterns({ + name: 'kotlin-spring-request-line', + language, + patterns: [ + { + meta: {}, + query: ` + (function_declaration + (modifiers + (annotation + (constructor_invocation + (user_type (type_identifier) @ann (#eq? @ann "RequestLine")) + (value_arguments + (value_argument . (string_literal) @value))))) + (simple_identifier) @method_name) @method + `, + }, + { + meta: {}, + query: ` + (function_declaration + (modifiers + (annotation + (constructor_invocation + (user_type (type_identifier) @ann (#eq? @ann "RequestLine")) + (value_arguments + (value_argument + (simple_identifier) @key (#eq? @key "value") + (string_literal) @value))))) + (simple_identifier) @method_name) @method + `, + }, + ], + } satisfies LanguagePatterns>); + + // ─── Provider via interface inheritance (Spring interface-based controller) ─ + // Pattern: `@RestController class X(...) : XApi` where the route annotations + // live on the `XApi` interface and the controller's `override fun` carries + // none. Java resolves this in `scanProject` (scanSpringProject); this is the + // Kotlin port. tree-sitter-kotlin models BOTH class and interface as + // `class_declaration`; the `interface` keyword token distinguishes them. + const KOTLIN_TYPE_DECLARATION_PATTERNS = compilePatterns({ + name: 'kotlin-type-declaration', + language, + patterns: [{ meta: {}, query: `(class_declaration (type_identifier) @name) @type` }], + } satisfies LanguagePatterns>); + + /** A `class_declaration` is an interface when it carries the `interface` keyword token. */ + const isKotlinInterface = (node: Parser.SyntaxNode): boolean => + node.children.some((c) => c.type === 'interface'); + + /** Resolve an `annotation` node's simple name: `@Foo` / `@Foo(...)` / `@a.b.Foo` → "Foo". */ + const kotlinAnnotationName = (annotation: Parser.SyntaxNode): string | null => { + const direct = annotation.namedChildren.find((c) => c.type === 'user_type'); + const ctor = annotation.namedChildren.find((c) => c.type === 'constructor_invocation'); + const userType = direct ?? ctor?.namedChildren.find((c) => c.type === 'user_type'); + // A fully-qualified annotation (`@a.b.Foo`) parses to a `user_type` carrying + // one `type_identifier` per dotted segment (`a`, `b`, `Foo`); the trailing + // one is the simple name. Taking the FIRST would resolve `@org…RestController` + // to "org" and miss the controller. + const idents = userType?.namedChildren.filter((c) => c.type === 'type_identifier') ?? []; + const ident = idents.at(-1); + return ident ? ident.text : null; + }; /** - * Join a class-level prefix and a method-level path. Identical - * semantics to the Java plugin: strip leading/trailing slashes on - * the prefix, strip leading slashes on the method path, ensure a - * single slash between them. + * Whether a `class_declaration` is a Spring `@RestController` / `@Controller`. + * All forms attach under `modifiers` as an `annotation` (confirmed against + * tree-sitter-kotlin fwcd): the bare `@RestController`, the common + * `@RestController @RequestMapping("/x")` pair, AND the arg-form + * `@RestController("beanName")` — the last parses to an `annotation` whose + * child is a `constructor_invocation` (NOT a detached sibling), which + * `kotlinAnnotationName` reads. A single pass over `modifiers` covers them all. */ - function joinPath(prefix: string, methodPath: string): string { - const cleanPrefix = prefix.replace(/^\/+/, '').replace(/\/+$/, ''); - const cleanSub = methodPath.replace(/^\/+/, ''); - if (!cleanPrefix) return `/${cleanSub}`; - return `/${cleanPrefix}/${cleanSub}`; - } + const CONTROLLER_ANNOTATIONS = new Set(['RestController', 'Controller']); + const kotlinClassIsController = (typeNode: Parser.SyntaxNode): boolean => { + const modifiers = typeNode.namedChildren.find((c) => c.type === 'modifiers'); + for (const ann of modifiers?.namedChildren ?? []) { + if (ann.type !== 'annotation') continue; + const name = kotlinAnnotationName(ann); + if (name && CONTROLLER_ANNOTATIONS.has(name)) return true; + } + return false; + }; + + /** Supertype names from `: A, B` (`delegation_specifier` → `user_type` → `type_identifier`). */ + const collectKotlinSupertypes = (node: Parser.SyntaxNode): string[] => { + const out: string[] = []; + for (const child of node.namedChildren) { + if (child.type !== 'delegation_specifier') continue; + const userType = child.namedChildren.find((c) => c.type === 'user_type'); + // FQN supertype (`: a.b.Api`) → one `type_identifier` per segment; the + // trailing one is the simple name (taking the first would yield "a"). + const idents = userType?.namedChildren.filter((c) => c.type === 'type_identifier') ?? []; + const ident = idents.at(-1); + if (ident) out.push(ident.text); + } + return out; + }; + + /** Direct `function_declaration` members of a type (no descent into nested types). */ + const collectKotlinDirectMethods = (typeNode: Parser.SyntaxNode): Parser.SyntaxNode[] => { + const body = typeNode.namedChildren.find((c) => c.type === 'class_body'); + if (!body) return []; + return body.namedChildren.filter((c) => c.type === 'function_declaration'); + }; + + const kotlinFunctionName = (fn: Parser.SyntaxNode): string | null => + fn.namedChildren.find((c) => c.type === 'simple_identifier')?.text ?? null; + + const collectKotlinSpringTypes = (filePath: string, tree: Parser.Tree): SharedSpringType[] => { + // Class-level @RequestMapping prefixes (reuse the provider class-prefix query). + const prefixByClassId = new Map(); + for (const match of runCompiledPatterns(SPRING_CLASS_PREFIX_PATTERNS, tree)) { + const prefixNode = match.captures.prefix; + const classNode = match.captures.class; + if (!prefixNode || !classNode) continue; + const prefix = unquoteLiteral(prefixNode.text); + if (prefix !== null) pushPrefix(prefixByClassId, classNode.id, prefix); + } + // Method @(Get|...)Mapping routes keyed by the function_declaration node id. + const routesByMethodId = new Map>(); + for (const match of runCompiledPatterns(SPRING_METHOD_ROUTE_PATTERNS, tree)) { + const annNode = match.captures.ann; + const pathNode = match.captures.path; + const methodNode = match.captures.method; + if (!annNode || !pathNode || !methodNode) continue; + const httpMethod = METHOD_ANNOTATION_TO_HTTP[annNode.text]; + if (!httpMethod) continue; + const rawPath = unquoteLiteral(pathNode.text); + if (rawPath === null) continue; + const arr = routesByMethodId.get(methodNode.id) ?? []; + arr.push({ method: httpMethod, path: rawPath }); + routesByMethodId.set(methodNode.id, arr); + } + + const out: SharedSpringType[] = []; + for (const match of runCompiledPatterns(KOTLIN_TYPE_DECLARATION_PATTERNS, tree)) { + const typeNode = match.captures.type; + const nameNode = match.captures.name; + if (!typeNode || !nameNode) continue; + const kind = isKotlinInterface(typeNode) ? 'interface' : 'class'; + const methods = collectKotlinDirectMethods(typeNode) + .map((fn) => ({ name: kotlinFunctionName(fn), routes: routesByMethodId.get(fn.id) ?? [] })) + .filter((m): m is { name: string; routes: Array<{ method: string; path: string }> } => { + return m.name !== null; + }); + out.push({ + filePath, + kind, + name: nameNode.text, + isController: kind === 'class' ? kotlinClassIsController(typeNode) : false, + classPrefixes: prefixByClassId.get(typeNode.id) ?? [], + implementedInterfaces: kind === 'class' ? collectKotlinSupertypes(typeNode) : [], + methods, + }); + } + return out; + }; + + // The interface-based-controller inheritance algorithm is shared with java.ts + // (`scanSpringInheritanceProject`); this collects the language-specific + // `SharedSpringType` view and delegates. kotlinClassIsController handles every + // controller form (bare, paired, and the arg-form `@RestController("bean")`) + // via the `modifiers` `annotation`/`constructor_invocation` shape. + const scanKotlinProject = (files: readonly HttpScanInput[]): HttpFileDetections[] => + scanSpringInheritanceProject( + files.flatMap((f) => collectKotlinSpringTypes(f.filePath, f.tree)), + ); return { name: 'kotlin-http', @@ -433,16 +829,42 @@ function buildKotlinPlugin(language: unknown): HttpLanguagePlugin { const out: HttpDetection[] = []; // ─── Class prefixes ───────────────────────────────────────────── - const prefixByClassId = new Map(); + const prefixByClassId = new Map(); for (const match of runCompiledPatterns(SPRING_CLASS_PREFIX_PATTERNS, tree)) { const prefixNode = match.captures.prefix; const classNode = match.captures.class; if (!prefixNode || !classNode) continue; const prefix = unquoteLiteral(prefixNode.text); - if (prefix !== null) prefixByClassId.set(classNode.id, prefix); + if (prefix !== null) pushPrefix(prefixByClassId, classNode.id, prefix); } - // ─── Method routes ────────────────────────────────────────────── + // ─── OpenFeign client interfaces + HTTP Interface type prefixes ── + // In tree-sitter-kotlin an `interface` is a `class_declaration`, so a + // `@FeignClient` interface's @(Get|...)Mapping methods would otherwise be + // mis-emitted as providers. Collect the FeignClient class ids (and their + // optional `path` prefix) so the method-route loop can reclassify them. + const feignClassIds = new Set(); + const feignPrefixByClassId = new Map(); + for (const match of runCompiledPatterns(SPRING_FEIGN_CLIENT_PATTERNS, tree)) { + const classNode = match.captures.class; + if (!classNode) continue; + feignClassIds.add(classNode.id); + const prefixNode = match.captures.prefix; + if (prefixNode) { + const prefix = unquoteLiteral(prefixNode.text); + if (prefix !== null) pushPrefix(feignPrefixByClassId, classNode.id, prefix); + } + } + const httpExchangePrefixByClassId = new Map(); + for (const match of runCompiledPatterns(SPRING_HTTP_EXCHANGE_CLASS_PATTERNS, tree)) { + const classNode = match.captures.class; + const prefixNode = match.captures.prefix; + if (!classNode || !prefixNode) continue; + const prefix = unquoteLiteral(prefixNode.text); + if (prefix !== null) pushPrefix(httpExchangePrefixByClassId, classNode.id, prefix); + } + + // ─── Method routes (Spring providers) + OpenFeign consumers ───── for (const match of runCompiledPatterns(SPRING_METHOD_ROUTE_PATTERNS, tree)) { const annNode = match.captures.ann; const pathNode = match.captures.path; @@ -454,16 +876,45 @@ function buildKotlinPlugin(language: unknown): HttpLanguagePlugin { const rawPath = unquoteLiteral(pathNode.text); if (rawPath === null) continue; const enclosingClass = findEnclosingClass(methodNode); - const prefix = enclosingClass ? (prefixByClassId.get(enclosingClass.id) ?? '') : ''; - const fullPath = joinPath(prefix, rawPath); - out.push({ - role: 'provider', - framework: 'spring', - method: httpMethod, - path: fullPath, - name: nameNode?.text ?? null, - confidence: 0.8, - }); + // A @(Get|...)Mapping inside a @FeignClient interface is an OpenFeign + // consumer (a remote call), not a route this service serves. + if (enclosingClass && feignClassIds.has(enclosingClass.id)) { + // @FeignClient(path) wins over @RequestMapping; a multi-element prefix + // yields one consumer per (prefix × this route). + const prefixes = feignPrefixByClassId.get(enclosingClass.id) ?? + prefixByClassId.get(enclosingClass.id) ?? ['']; + for (const prefix of prefixes) { + out.push({ + role: 'consumer', + framework: OPENFEIGN_FRAMEWORK, + method: httpMethod, + path: joinPath(prefix, rawPath), + name: nameNode?.text ?? null, + confidence: FEIGN_CONFIDENCE, + }); + } + continue; + } + // A @(Get|...)Mapping on a (non-Feign) interface declares a route + // *contract*, not a route this service serves — the implementing + // @RestController is the provider, emitted via scanProject's interface + // inheritance. Java drops these implicitly (findEnclosingClass returns + // null for an interface_declaration); tree-sitter-kotlin models an + // interface as a class_declaration, so skip it explicitly here. + if (enclosingClass && isKotlinInterface(enclosingClass)) continue; + // A multi-element class `@RequestMapping(["/a","/b"])` registers the method + // under each prefix — emit one provider per (prefix × this route). + const prefixes = enclosingClass ? (prefixByClassId.get(enclosingClass.id) ?? ['']) : ['']; + for (const prefix of prefixes) { + out.push({ + role: 'provider', + framework: 'spring', + method: httpMethod, + path: joinPath(prefix, rawPath), + name: nameNode?.text ?? null, + confidence: 0.8, + }); + } } // ─── Consumers: RestTemplate ──────────────────────────────────── @@ -547,8 +998,74 @@ function buildKotlinPlugin(language: unknown): HttpLanguagePlugin { }); } + // ─── Consumers: Spring HTTP Interface @(Get|...)Exchange ──────── + for (const match of runCompiledPatterns(SPRING_EXCHANGE_PATTERNS, tree)) { + const annNode = match.captures.ann; + const pathNode = match.captures.path; + const nameNode = match.captures.method_name; + const methodNode = match.captures.method; + if (!annNode || !pathNode || !methodNode) continue; + const httpMethod = EXCHANGE_ANNOTATION_TO_HTTP[annNode.text]; + if (!httpMethod) continue; + const rawPath = unquoteLiteral(pathNode.text); + if (rawPath === null) continue; + const enclosingClass = findEnclosingClass(methodNode); + const prefixes = enclosingClass + ? (httpExchangePrefixByClassId.get(enclosingClass.id) ?? ['']) + : ['']; + for (const prefix of prefixes) { + out.push({ + role: 'consumer', + framework: HTTP_INTERFACE_FRAMEWORK, + method: httpMethod, + path: joinPath(prefix, rawPath), + name: nameNode?.text ?? null, + confidence: EXCHANGE_CONFIDENCE, + }); + } + } + + // ─── Consumers: OpenFeign native @RequestLine("VERB /path") ───── + // Method-level only and always declared on an interface — Feign builds its + // proxy from the interface, so a `@RequestLine` on a concrete class is not + // a client call. We do NOT require an enclosing `@FeignClient` (core Feign + // uses `@RequestLine` with `Feign.builder()`, not Spring Cloud's + // `@FeignClient`); the `RequestLine` name plus the structural interface + // check keep false positives away. Mirrors java.ts's `findEnclosingInterface` + // gate — in tree-sitter-kotlin an interface is a `class_declaration`, so we + // test the `interface` keyword via isKotlinInterface. + for (const match of runCompiledPatterns(SPRING_REQUEST_LINE_PATTERNS, tree)) { + const valueNode = match.captures.value; + const nameNode = match.captures.method_name; + const methodNode = match.captures.method; + if (!valueNode || !methodNode) continue; + const raw = unquoteLiteral(valueNode.text); + const parsed = raw !== null ? parseRequestLine(raw) : null; + if (!parsed) continue; + const enclosingClass = findEnclosingClass(methodNode); + if (!enclosingClass || !isKotlinInterface(enclosingClass)) continue; + // Mirror java.ts (which pre-merges the @RequestMapping fallback into + // feignPrefixByInterfaceId, "path wins"): @FeignClient(path) wins, else + // the interface's class-level @RequestMapping prefix, else none. Without + // the prefixByClassId fallback Kotlin dropped the class prefix that Java + // applies — the same fallback chain the @GetMapping-in-Feign path uses above. + const prefixes = feignPrefixByClassId.get(enclosingClass.id) ?? + prefixByClassId.get(enclosingClass.id) ?? ['']; + for (const prefix of prefixes) { + out.push({ + role: 'consumer', + framework: OPENFEIGN_FRAMEWORK, + method: parsed.method, + path: joinPath(prefix, parsed.path), + name: nameNode?.text ?? null, + confidence: REQUEST_LINE_CONFIDENCE, + }); + } + } + return out; }, + scanProject: scanKotlinProject, }; } diff --git a/gitnexus/src/core/group/extractors/http-patterns/spring-consumer-shared.ts b/gitnexus/src/core/group/extractors/http-patterns/spring-consumer-shared.ts new file mode 100644 index 000000000..f42894da6 --- /dev/null +++ b/gitnexus/src/core/group/extractors/http-patterns/spring-consumer-shared.ts @@ -0,0 +1,269 @@ +/** + * Shared, language-agnostic primitives for Spring / OpenFeign / Spring-HTTP-Interface + * HTTP *consumer* extraction, used by BOTH the Java (`java.ts`) and Kotlin + * (`kotlin.ts`) group-layer HTTP plugins so the two cannot drift apart. + * + * These are pure value maps + string helpers — no tree-sitter AST knowledge. + * Each language plugin keeps its own grammar-specific queries and walkers and + * funnels the extracted (verb, path, prefix) facts through these helpers, so a + * polyglot repo emits byte-identical contract IDs from `.java` and `.kt`. + * + * The provider-side annotation→verb map (`METHOD_ANNOTATION_TO_HTTP`), + * `isRouteMemberKey`, and `findEnclosingClass` live in the lower + * `ingestion/route-extractors/spring-shared.ts` (shared with the ingestion + * route extractor). This module is the consumer-side counterpart and lives in + * the group layer beside the plugins that use it. + */ + +import type { HttpDetection, HttpFileDetections } from './types.js'; + +/** + * RestTemplate method-name → HTTP verb. Source-scan only: the receiver must be + * named exactly `restTemplate` (the per-language query enforces that). + */ +export const REST_TEMPLATE_TO_HTTP: Record = { + getForObject: 'GET', + getForEntity: 'GET', + postForObject: 'POST', + postForEntity: 'POST', + put: 'PUT', + delete: 'DELETE', + patchForObject: 'PATCH', +}; + +/** + * Reactive WebClient short-form verb helper → HTTP verb + * (`webClient.get().uri("/x")`, `.post()`, ...). HEAD/OPTIONS/TRACE are + * intentionally excluded for symmetry across the plugins. + */ +export const WEB_CLIENT_SHORT_TO_HTTP: Record = { + get: 'GET', + post: 'POST', + put: 'PUT', + delete: 'DELETE', + patch: 'PATCH', +}; + +/** + * Accepted HTTP verbs for the WebClient long form + * `webClient.method(HttpMethod.X).uri("/y")`. The verb is captured as the + * literal `HttpMethod.X` field name (`GET`, `POST`, …); HEAD/OPTIONS/TRACE are + * intentionally excluded for symmetry with the short form + * (`WEB_CLIENT_SHORT_TO_HTTP`). Shared so the Java and Kotlin long-form scans + * gate verbs identically. + */ +export const WEB_CLIENT_LONG_VERB_RE = /^(GET|POST|PUT|DELETE|PATCH)$/; + +/** + * Spring 6 declarative HTTP Interface shortcut annotation → HTTP verb. + * + * `@GetExchange`/`@PostExchange`/… on a service interface proxied by + * `HttpServiceProxyFactory` (over RestClient / WebClient / RestTemplate) + * describe an OUTBOUND call — i.e. a CONSUMER, the modern analogue of an + * OpenFeign `@(Get|Post|...)Mapping` interface method. The path lives in the + * annotation's `url` (or `value`) attribute, or positionally. + * + * The base `@HttpExchange(method = "GET", url = "...")` form carries its verb + * in an attribute rather than the annotation name; the shortcut annotations + * above are the overwhelmingly common case and the only ones mapped here. + */ +export const EXCHANGE_ANNOTATION_TO_HTTP: Record = { + GetExchange: 'GET', + PostExchange: 'POST', + PutExchange: 'PUT', + DeleteExchange: 'DELETE', + PatchExchange: 'PATCH', +}; + +/** + * Accumulate a route prefix (de-duped) under a class/interface declaration node + * id. A Spring route attribute is `String[]`; a multi-element array (`["/a","/b"]`, + * `arrayOf("/a","/b")`, `{"/a","/b"}`) yields one query match per element, so + * prefixes accumulate rather than overwrite. Shared by the Java and Kotlin plugins + * so both build their prefix maps identically. + */ +export const pushPrefix = (map: Map, id: number, prefix: string): void => { + const arr = map.get(id) ?? []; + if (!arr.includes(prefix)) arr.push(prefix); + map.set(id, arr); +}; + +/** Framework tags emitted on consumer detections (stable contract metadata). */ +export const OPENFEIGN_FRAMEWORK = 'openfeign'; +export const HTTP_INTERFACE_FRAMEWORK = 'spring-http-interface'; + +/** Consumer-detection confidences, shared so `.java` and `.kt` agree. */ +export const FEIGN_CONFIDENCE = 0.7; +export const REQUEST_LINE_CONFIDENCE = 0.75; +export const EXCHANGE_CONFIDENCE = 0.75; + +/** + * OpenFeign's native `@RequestLine("METHOD /path[?query]")` packs an HTTP + * method and path in a single string literal — see + * https://github.com/OpenFeign/feign#interface-annotations. This regex splits + * the verb from the path of that literal. + */ +export const REQUEST_LINE_VERB_RE = /^\s*(GET|POST|PUT|DELETE|PATCH|HEAD|OPTIONS)\s+(\S.*?)\s*$/i; + +/** + * Parse a Feign `@RequestLine` value into a method + path pair. The query + * portion is dropped because contract IDs are method+path only (consistent + * with how RestTemplate/WebClient consumers drop inline query strings). + * + * Returns null if the value is not a recognized HTTP verb followed by a path + * beginning with `/`. + */ +export function parseRequestLine(raw: string): { method: string; path: string } | null { + const match = REQUEST_LINE_VERB_RE.exec(raw); + if (!match) return null; + const [, verb, rest] = match; + if (typeof verb !== 'string' || typeof rest !== 'string') return null; + const queryIdx = rest.indexOf('?'); + const pathOnly = (queryIdx >= 0 ? rest.slice(0, queryIdx) : rest).trim(); + if (!pathOnly.startsWith('/')) return null; + return { method: verb.toUpperCase(), path: pathOnly }; +} + +/** + * Join a class/interface-level prefix and a method-level path into a single + * URL path: strip leading/trailing slashes on the prefix and leading slashes + * on the method path, then ensure exactly one slash between them. + */ +export function joinPath(prefix: string, methodPath: string): string { + const cleanPrefix = prefix.replace(/^\/+/, '').replace(/\/+$/, ''); + const cleanSub = methodPath.replace(/^\/+/, ''); + if (!cleanPrefix) return `/${cleanSub}`; + return `/${cleanPrefix}/${cleanSub}`; +} + +/** + * Join a controller's own class prefix with a route inherited from an interface + * (interface-based controllers, #1743). The inherited path already has the + * interface's own class prefix (`inheritedOwnerPrefix`) baked in; when the + * controller repeats that same prefix we must NOT prepend it twice (#2057). + * Shared by both plugins' `scanProject` so Java and Kotlin agree. + */ +export function joinInheritedSpringPath( + controllerPrefix: string, + inheritedPath: string, + inheritedOwnerPrefix = '', +): string { + const joined = joinPath(controllerPrefix, inheritedPath); + const cleanPrefix = controllerPrefix.replace(/^\/+/, '').replace(/\/+$/, ''); + const cleanOwnerPrefix = inheritedOwnerPrefix.replace(/^\/+/, '').replace(/\/+$/, ''); + const cleanInherited = inheritedPath.replace(/^\/+/, ''); + if (!cleanPrefix) return joined; + if ( + cleanPrefix === cleanOwnerPrefix && + (cleanInherited === cleanPrefix || cleanInherited.startsWith(`${cleanPrefix}/`)) + ) { + return `/${cleanInherited}`; + } + return joined; +} + +/** + * Language-agnostic view of a Spring class/interface that each plugin's + * grammar-specific collector produces. The interface-based-controller + * inheritance algorithm (`scanSpringInheritanceProject`) operates only on this + * shape, so the Java and Kotlin plugins share one algorithm and cannot drift. + * + * `methods[].routes` carry only `{ method, path }` — the interface's own class + * prefix is applied *inside* `scanSpringInheritanceProject` (it is not part of + * the collector's output). + */ +export interface SharedSpringType { + filePath: string; + kind: 'class' | 'interface'; + name: string; + /** Class-level `@RequestMapping` prefixes — one per array element. */ + classPrefixes: string[]; + implementedInterfaces: string[]; + isController: boolean; + methods: Array<{ name: string; routes: Array<{ method: string; path: string }> }>; +} + +/** + * Resolve interface-based-controller provider routes (#1743): a concrete + * `@RestController`/`@Controller` class inherits the `@(Get|...)Mapping` routes + * declared on the interface it implements. Shared by the Java and Kotlin plugins + * so both emit byte-identical provider contracts. + * + * An interface name that resolves to two distinct interfaces is ambiguous and + * its routes are dropped (the `null` marker). The controller's own class + * prefix(es) cross-product the inherited routes; `joinInheritedSpringPath` + * avoids doubling a prefix the interface already baked in (#2057). + */ +export function scanSpringInheritanceProject(types: SharedSpringType[]): HttpFileDetections[] { + // interface name → (method name → routes). `ownerPrefix` records the + // interface's own class prefix so the controller side avoids doubling it + // (#2057). `null` marks an ambiguous (duplicated) interface name. + type InheritedRoute = { method: string; path: string; ownerPrefix: string }; + const interfaceRoutes = new Map | null>(); + for (const type of types) { + if (type.kind !== 'interface') continue; + if (interfaceRoutes.has(type.name)) { + interfaceRoutes.set(type.name, null); + continue; + } + const prefixes = type.classPrefixes.length ? type.classPrefixes : ['']; + const methodMap = new Map(); + for (const method of type.methods) { + // Cross-product the interface's class prefixes with each method route, so a + // multi-element `@RequestMapping(["/a","/b"])` interface yields N bindings. + const routes = method.routes.flatMap((route) => + prefixes.map((prefix) => ({ + method: route.method, + path: prefix ? joinPath(prefix, route.path) : route.path, + ownerPrefix: prefix, + })), + ); + if (routes.length > 0) methodMap.set(method.name, routes); + } + interfaceRoutes.set(type.name, methodMap); + } + + const detectionsByFile = new Map(); + for (const type of types) { + if (type.kind !== 'class' || !type.isController) continue; + // Cross-product the controller's own class prefixes with each inherited + // route; `['']` keeps the common no-prefix controller emitting the + // interface path unchanged. + const controllerPrefixes = type.classPrefixes.length ? type.classPrefixes : ['']; + for (const method of type.methods) { + if (method.routes.length > 0) continue; // own @*Mapping → already a provider via scan() + const inherited = type.implementedInterfaces.flatMap((iface) => { + const routeMap = interfaceRoutes.get(iface); + if (!routeMap) return []; + const routes = routeMap.get(method.name) ?? []; + return routes.flatMap((route) => + controllerPrefixes.map((controllerPrefix) => ({ + method: route.method, + path: joinInheritedSpringPath(controllerPrefix, route.path, route.ownerPrefix), + })), + ); + }); + const seen = new Set(); + for (const route of inherited) { + const key = `${route.method} ${route.path}`; + if (seen.has(key)) continue; + seen.add(key); + const detections = detectionsByFile.get(type.filePath) ?? []; + detections.push({ + role: 'provider', + framework: 'spring', + method: route.method, + path: route.path, + name: method.name, + confidence: 0.8, + }); + detectionsByFile.set(type.filePath, detections); + } + } + } + + return [...detectionsByFile.entries()].map(([filePath, detections]) => ({ + filePath, + detections, + })); +} diff --git a/gitnexus/src/core/ingestion/workers/parse-worker.ts b/gitnexus/src/core/ingestion/workers/parse-worker.ts index 6c603984f..436833a5a 100644 --- a/gitnexus/src/core/ingestion/workers/parse-worker.ts +++ b/gitnexus/src/core/ingestion/workers/parse-worker.ts @@ -966,8 +966,13 @@ const processBatch = ( try { setLanguage(language, regularFiles[0].path); processFileGroup(regularFiles, language, queryString, result, onFileProcessed); - } catch { - // parser unavailable — skip this language group + } catch (err) { + // A throw here drops the whole language group — surface it to the pool + // (#2264) instead of silently skipping. The old empty catch hid real + // extractor/parser failures, not just an unavailable grammar. + reportWarning( + `Skipped ${regularFiles.length} ${language} file(s) after a processing error: ${err instanceof Error ? err.message : String(err)}`, + ); } } else { result.skippedLanguages[language] = @@ -981,8 +986,12 @@ const processBatch = ( try { setLanguage(language, tsxFiles[0].path); processFileGroup(tsxFiles, language, queryString, result, onFileProcessed); - } catch { - // parser unavailable — skip this language group + } catch (err) { + // See above — surface a tsx-group processing failure rather than + // silently dropping every file in it (#2264). + reportWarning( + `Skipped ${tsxFiles.length} ${language} (tsx) file(s) after a processing error: ${err instanceof Error ? err.message : String(err)}`, + ); } } else { result.skippedLanguages[language] = @@ -1142,6 +1151,23 @@ export function extractORMQueries( import { extractFastAPIRouterBindings } from '../route-extractors/fastapi-router-bindings.js'; +/** + * Report a non-fatal worker issue to the pool over IPC so a caught error is not + * invisible to the operator (#2264). The pool logs it on the main thread AND + * resets the worker idle timer (so a worker grinding through failing files isn't + * falsely idle-evicted). Falls back to the local logger when there's no parent — + * this code also runs on the main thread in tests / the non-worker path. Fatal, + * group-aborting errors go through the message handler's + * `{ type: 'error', errorStack }` channel instead. + */ +function reportWarning(message: string): void { + if (parentPort) { + parentPort.postMessage({ type: 'warning', message }); + } else { + logger.warn(message); + } +} + const processFileGroup = ( files: ParseWorkerInput[], language: SupportedLanguages, @@ -1154,12 +1180,9 @@ const processFileGroup = ( const lang = parser.getLanguage(); query = new Parser.Query(lang, queryString); } catch (err) { - const message = `Query compilation failed for ${language}: ${err instanceof Error ? err.message : String(err)}`; - if (parentPort) { - parentPort.postMessage({ type: 'warning', message }); - } else { - logger.warn(message); - } + reportWarning( + `Query compilation failed for ${language}: ${err instanceof Error ? err.message : String(err)}`, + ); return; } @@ -1203,7 +1226,7 @@ const processFileGroup = ( bufferSize: getTreeSitterBufferSize(parseContent), }); } catch (err) { - logger.warn( + reportWarning( `Failed to parse file ${file.path}: ${err instanceof Error ? err.message : String(err)}`, ); continue; @@ -1216,7 +1239,7 @@ const processFileGroup = ( try { matches = query.matches(tree.rootNode); } catch (err) { - logger.warn( + reportWarning( `Query execution failed for ${file.path}: ${err instanceof Error ? err.message : String(err)}`, ); continue; @@ -1237,13 +1260,7 @@ const processFileGroup = ( provider, parseContent, file.path, - (message) => { - if (parentPort) { - parentPort.postMessage({ type: 'warning', message }); - } else { - logger.warn(message); - } - }, + reportWarning, tree, scopeSourceKind, ); @@ -1306,9 +1323,9 @@ const processFileGroup = ( }; } } catch (err) { - const message = `CFG build failed for ${file.path}: ${err instanceof Error ? err.message : String(err)}`; - if (parentPort) parentPort.postMessage({ type: 'warning', message }); - else logger.warn(message); + reportWarning( + `CFG build failed for ${file.path}: ${err instanceof Error ? err.message : String(err)}`, + ); } } @@ -2109,7 +2126,12 @@ const processFileGroup = ( if (parsedTemplateConstraints !== undefined) { constraintsTag = templateConstraintsIdTag(parsedTemplateConstraints); } - } catch { + } catch (err) { + // Optional C++ template-constraint enrichment: fall back to no tag, but + // surface the failure (#2264) — matches the CFG-build warning above. + reportWarning( + `Template-constraint extraction failed for ${file.path}: ${err instanceof Error ? err.message : String(err)}`, + ); parsedTemplateConstraints = undefined; constraintsTag = ''; } diff --git a/gitnexus/src/core/lbug/conn-lock.ts b/gitnexus/src/core/lbug/conn-lock.ts new file mode 100644 index 000000000..7948371bc --- /dev/null +++ b/gitnexus/src/core/lbug/conn-lock.ts @@ -0,0 +1,66 @@ +/** + * Serialize every operation on the shared singleton LadybugDB connection. + * + * LadybugDB is single-writer and its `Connection` is NOT safe for concurrent + * query execution: dispatching two queries on one connection at the same time + * lets two libuv workers mutate shared native engine state at once, corrupting + * the heap. This surfaced as `double free or corruption (out)` / SIGSEGV at the + * end of `analyze --pdg`, where the periodic WAL-checkpoint driver + * (`wal-checkpoint-driver.ts`) fired `CHECKPOINT` on the same connection a + * long-running PDG-table COPY was still using. `--pdg` makes those COPYs outlast + * the driver's 5 s tick, so the overlap (rare without `--pdg`) becomes reliable. + * + * Every singleton-`conn` helper in `lbug-adapter.ts` runs its full query + + * result-drain inside this lock, so the checkpoint driver, the bulk COPY, the + * embedding writeback, and the PDG edge deletes are mutually exclusive — the + * property that makes a strictly-serial workload stable. + * + * Implementation: a promise chain. Each caller installs a fresh unresolved tail, + * awaits the previous holder's tail, runs, then releases its own in `finally` + * (so a thrown op never wedges the connection). FIFO and non-reentrant: a wrapped + * helper MUST NOT call another wrapped helper — the inner call would await its own + * holder's tail and deadlock. The re-entry guard below catches this and throws + * instead of hanging. A boolean flag can't do this: a legitimately-queued + * top-level caller also runs while the lock is held, so only AsyncLocalStorage — + * which marks the *async context* of the running `fn` — distinguishes a true + * nested call from normal contention. + */ +import { AsyncLocalStorage } from 'node:async_hooks'; + +let tail: Promise = Promise.resolve(); + +// Set (to `true`) only inside a holding `fn`'s async context. A withConnLock call +// that observes it set is a nested/re-entrant call from within a critical section. +const inCriticalSection = new AsyncLocalStorage(); + +export const withConnLock = async (fn: () => Promise): Promise => { + if (inCriticalSection.getStore()) { + throw new Error( + 'conn-lock re-entry: a withConnLock-wrapped helper called another wrapped ' + + 'helper, which would deadlock the single LadybugDB connection. Run the inner ' + + 'work outside the lock, or inline it. See src/core/lbug/conn-lock.ts.', + ); + } + const prior = tail; + let release!: () => void; + tail = new Promise((resolve) => { + release = resolve; + }); + await prior; + try { + return await inCriticalSection.run(true, fn); + } finally { + release(); + } +}; + +/** + * Test-only: reset the lock chain to a fresh resolved tail. Production code has + * no reason to call this — a leaked-but-resolved tail is harmless — but unit + * tests want a clean chain per case. + * + * @internal + */ +export const _resetConnLockForTests = (): void => { + tail = Promise.resolve(); +}; diff --git a/gitnexus/src/core/lbug/lbug-adapter.ts b/gitnexus/src/core/lbug/lbug-adapter.ts index eb04ec5a4..903493f8f 100644 --- a/gitnexus/src/core/lbug/lbug-adapter.ts +++ b/gitnexus/src/core/lbug/lbug-adapter.ts @@ -6,6 +6,8 @@ import { finished } from 'stream/promises'; import path from 'path'; import lbug from '@ladybugdb/core'; import { closeQueryResults } from './query-result-utils.js'; +import { withConnLock } from './conn-lock.js'; +import { isWalDriverActive } from './wal-driver-state.js'; import { KnowledgeGraph } from '../graph/types.js'; import { NODE_TABLES, @@ -178,6 +180,25 @@ export const splitRelCsvByLabelPair = async ( let db: lbug.Database | null = null; let conn: lbug.Connection | null = null; + +// Serialize every operation on the shared singleton `conn`. LadybugDB's +// Connection is single-writer and is NOT safe for concurrent query execution; +// the periodic WAL-checkpoint driver overlapping a long `--pdg` COPY on this +// connection corrupted native state (`double free or corruption`). Each +// singleton-`conn` helper below runs its full query + drain inside withConnLock. +// Invariant: a wrapped helper MUST NOT call another wrapped helper (re-entry +// self-deadlocks); all current holders are leaf-level. `streamQuery` is +// deliberately NOT wrapped — its per-row callback can re-enter the adapter and +// it only runs on the read path where the checkpoint driver is inactive. +// See conn-lock.ts for the full rationale. +// +// The gate that decides whether an op must take withConnLock: only operations on +// the shared singleton `conn` serialize. Per-file / temp connections (distinct +// native objects with no shared engine state) must NOT block on — or be blocked +// by — the singleton's lock. Reads the live `conn` binding at call time (it's +// reassigned only at open/close, never mid-load). +const isSharedSingletonConn = (c: lbug.Connection): boolean => c === conn; + let currentDbPath: string | null = null; let currentDbReadOnly = false; let ftsLoaded = false; @@ -472,8 +493,14 @@ const readQueryRows = async ( }; const queryAndDrain = async (targetConn: lbug.Connection, cypher: string): Promise => { - const queryResult = await targetConn.query(cypher); - await drainQueryResult(queryResult); + const run = async (): Promise => { + const queryResult = await targetConn.query(cypher); + await drainQueryResult(queryResult); + }; + // Serialize only when this runs on the shared singleton connection (the bulk + // node/relationship COPY captures `writeConn = conn`); per-file / temp + // connections skip the lock — see isSharedSingletonConn. + return isSharedSingletonConn(targetConn) ? withConnLock(run) : run(); }; const READ_ONLY_SHADOW_REPLAY_PROBE = 'MATCH (n) RETURN n LIMIT 1'; @@ -1116,7 +1143,12 @@ export const loadGraphToLbug = async ( log(`Loading edges: ${pairIdx}/${relsByPair.size} types (${fromLabel} -> ${toLabel})`); } - await copyCsvWithRetry(conn, copyQuery, (retryErr) => { + // Use the captured `writeConn` (not the module-level `conn`) for the rel + // COPY, matching the node COPY above — one captured reference for the whole + // bulk load (#2264 review P3). Same object during analyze (`conn` is only + // reassigned at open/close under the session lock, never mid-load), so the + // queryAndDrain `targetConn === conn` lock gate still engages. + await copyCsvWithRetry(writeConn, copyQuery, (retryErr) => { const retryMsg = retryErr instanceof Error ? retryErr.message : String(retryErr); warnings.push(`${fromLabel}->${toLabel} (${rows} edges): ${retryMsg.slice(0, 80)}`); failedPairEdges += rows; @@ -1498,6 +1530,18 @@ export const streamQuery = async ( cypher: string, onRow: (row: any) => void | Promise, ): Promise => { + if (isWalDriverActive()) { + // streamQuery reads rows on the singleton connection WITHOUT withConnLock; if + // the WAL-checkpoint driver is live, those reads could race a CHECKPOINT — the + // #2264 corruption window. Today the serve/read path never runs the driver + // (analyze runs in a forked worker), so this fails loud only if a future + // in-process analyze overlaps a stream. Run analysis in a worker, or stop the + // driver before streaming. See conn-lock.ts. + throw new Error( + 'streamQuery cannot run while the WAL-checkpoint driver is active (it would ' + + 'race a CHECKPOINT on the unlocked read connection — #2264).', + ); + } if (!conn) { throw new Error('LadybugDB not initialized. Call initLbug first.'); } @@ -1535,23 +1579,27 @@ export const executePrepared = async ( cypher: string, params: Record, ): Promise => { - if (!conn) { + const c = conn; + if (!c) { throw new Error('LadybugDB not initialized. Call initLbug first.'); } - const stmt = await conn.prepare(cypher); - if (!stmt.isSuccess()) { - const errMsg = await stmt.getErrorMessage(); - throw new Error(`Prepare failed: ${errMsg}`); - } - const queryResult = await conn.execute(stmt, params); - return await readQueryRows(queryResult); + return withConnLock(async () => { + const stmt = await c.prepare(cypher); + if (!stmt.isSuccess()) { + const errMsg = await stmt.getErrorMessage(); + throw new Error(`Prepare failed: ${errMsg}`); + } + const queryResult = await c.execute(stmt, params); + return await readQueryRows(queryResult); + }); }; export const executeWithReusedStatement = async ( cypher: string, paramsList: Array>, ): Promise => { - if (!conn) { + const c = conn; + if (!c) { throw new Error('LadybugDB not initialized. Call initLbug first.'); } if (paramsList.length === 0) return; @@ -1559,39 +1607,50 @@ export const executeWithReusedStatement = async ( const SUB_BATCH_SIZE = 4; for (let i = 0; i < paramsList.length; i += SUB_BATCH_SIZE) { const subBatch = paramsList.slice(i, i + SUB_BATCH_SIZE); - const stmt = await conn.prepare(cypher); - if (!stmt.isSuccess()) { - const errMsg = await stmt.getErrorMessage(); - throw new Error(`Prepare failed: ${errMsg}`); - } - try { - for (const params of subBatch) { - await drainQueryResult(await conn.execute(stmt, params)); + // One critical section per sub-batch: the prepare + its executes run with + // exclusive access to the connection (so the WAL checkpoint driver cannot + // interleave a CHECKPOINT mid-batch), while the lock is released between + // sub-batches to let the driver checkpoint during a long writeback. + await withConnLock(async () => { + const stmt = await c.prepare(cypher); + if (!stmt.isSuccess()) { + const errMsg = await stmt.getErrorMessage(); + throw new Error(`Prepare failed: ${errMsg}`); } - } catch (e) { - const msg = e instanceof Error ? e.message : String(e); - const queryPreview = cypher.replace(/\s+/g, ' ').slice(0, 120); - throw new Error( - `Batch execution failed for rows ${i + 1}-${i + subBatch.length}: ${msg} (${queryPreview})`, - ); - } - // Note: LadybugDB PreparedStatement doesn't require explicit close() + try { + for (const params of subBatch) { + await drainQueryResult(await c.execute(stmt, params)); + } + } catch (e) { + const msg = e instanceof Error ? e.message : String(e); + const queryPreview = cypher.replace(/\s+/g, ' ').slice(0, 120); + throw new Error( + `Batch execution failed for rows ${i + 1}-${i + subBatch.length}: ${msg} (${queryPreview})`, + ); + } + // Note: LadybugDB PreparedStatement doesn't require explicit close() + }); } }; export const getLbugStats = async (): Promise<{ nodes: number; edges: number }> => { - if (!conn) return { nodes: 0, edges: 0 }; + const c = conn; + if (!c) return { nodes: 0, edges: 0 }; + // Called during analyze finalize while the WAL-checkpoint driver is still + // running; each count read takes the connection lock so it cannot execute + // concurrently with a driver CHECKPOINT. Per-query locking lets the driver + // checkpoint between table counts rather than waiting for the whole sweep. let totalNodes = 0; for (const tableName of NODE_TABLES) { try { - const queryResult = await conn.query( - `MATCH (n:${escapeTableName(tableName)}) RETURN count(n) AS cnt`, - ); - const nodeRows = await readQueryRows(queryResult); - if (nodeRows.length > 0) { - totalNodes += Number(nodeRows[0]?.cnt ?? nodeRows[0]?.[0] ?? 0); - } + totalNodes += await withConnLock(async () => { + const queryResult = await c.query( + `MATCH (n:${escapeTableName(tableName)}) RETURN count(n) AS cnt`, + ); + const nodeRows = await readQueryRows(queryResult); + return nodeRows.length > 0 ? Number(nodeRows[0]?.cnt ?? nodeRows[0]?.[0] ?? 0) : 0; + }); } catch { // ignore } @@ -1599,13 +1658,13 @@ export const getLbugStats = async (): Promise<{ nodes: number; edges: number }> let totalEdges = 0; try { - const queryResult = await conn.query( - `MATCH ()-[r:${REL_TABLE_NAME}]->() RETURN count(r) AS cnt`, - ); - const edgeRows = await readQueryRows(queryResult); - if (edgeRows.length > 0) { - totalEdges = Number(edgeRows[0]?.cnt ?? edgeRows[0]?.[0] ?? 0); - } + totalEdges = await withConnLock(async () => { + const queryResult = await c.query( + `MATCH ()-[r:${REL_TABLE_NAME}]->() RETURN count(r) AS cnt`, + ); + const edgeRows = await readQueryRows(queryResult); + return edgeRows.length > 0 ? Number(edgeRows[0]?.cnt ?? edgeRows[0]?.[0] ?? 0) : 0; + }); } catch { // ignore } @@ -1624,67 +1683,75 @@ export const loadCachedEmbeddings = async (): Promise<{ embeddingNodeIds: Set; embeddings: CachedEmbedding[]; }> => { - if (!conn) { + const c = conn; + if (!c) { return { embeddingNodeIds: new Set(), embeddings: [] }; } - const embeddingNodeIds = new Set(); - const embeddings: CachedEmbedding[] = []; - try { - // Schema migration detection: query with new columns to verify schema version. - // Old schema only had (nodeId, embedding); new schema adds (id, chunkIndex, startLine, endLine, contentHash). - // If the query fails (column missing), we return empty cache to force a full rebuild. + // The whole read runs inside the connection lock (#2264 review P2). It's safe + // today only by call-ordering (loadCachedEmbeddings runs before the WAL driver + // starts), but the lock makes it robust to future reordering — a concurrent + // CHECKPOINT on the singleton connection is the documented corruption trigger. + // Leaf read: no nested withConnLock-wrapped helpers inside. + return withConnLock(async () => { + const embeddingNodeIds = new Set(); + const embeddings: CachedEmbedding[] = []; try { - const check = await conn.query( - `MATCH (e:${EMBEDDING_TABLE_NAME}) RETURN e.nodeId AS nodeId, e.chunkIndex AS chunkIndex LIMIT 1`, - ); - await readQueryRows(check); - } catch { - return { embeddingNodeIds: new Set(), embeddings: [] }; - } - - // Try to read contentHash alongside chunk columns - let rows: any; - let hasContentHash = true; - try { - rows = await conn.query( - `MATCH (e:${EMBEDDING_TABLE_NAME}) RETURN e.nodeId AS nodeId, e.chunkIndex AS chunkIndex, e.startLine AS startLine, e.endLine AS endLine, e.embedding AS embedding, e.contentHash AS contentHash`, - ); - } catch (err: any) { - // Fallback for legacy DBs without contentHash column - const msg = err?.message ?? ''; - if (isMissingColumnOrTableError(msg)) { - hasContentHash = false; - rows = await conn.query( - `MATCH (e:${EMBEDDING_TABLE_NAME}) RETURN e.nodeId AS nodeId, e.chunkIndex AS chunkIndex, e.startLine AS startLine, e.endLine AS endLine, e.embedding AS embedding`, + // Schema migration detection: query with new columns to verify schema version. + // Old schema only had (nodeId, embedding); new schema adds (id, chunkIndex, startLine, endLine, contentHash). + // If the query fails (column missing), we return empty cache to force a full rebuild. + try { + const check = await c.query( + `MATCH (e:${EMBEDDING_TABLE_NAME}) RETURN e.nodeId AS nodeId, e.chunkIndex AS chunkIndex LIMIT 1`, ); - } else { - throw err; + await readQueryRows(check); + } catch { + return { embeddingNodeIds: new Set(), embeddings: [] }; } - } - for (const row of await readQueryRows(rows)) { - const nodeId = String(row.nodeId ?? row[0] ?? ''); - if (!nodeId) continue; - embeddingNodeIds.add(nodeId); - const embedding = row.embedding ?? row[4]; - if (embedding) { - embeddings.push({ - nodeId, - chunkIndex: Number(row.chunkIndex ?? row[1] ?? 0), - startLine: Number(row.startLine ?? row[2] ?? 0), - endLine: Number(row.endLine ?? row[3] ?? 0), - embedding: Array.isArray(embedding) - ? embedding.map(Number) - : Array.from(embedding as any).map(Number), - contentHash: hasContentHash ? (row.contentHash ?? row[5] ?? undefined) : undefined, - }); - } - } - } catch { - /* embedding table may not exist */ - } - return { embeddingNodeIds, embeddings }; + // Try to read contentHash alongside chunk columns + let rows: any; + let hasContentHash = true; + try { + rows = await c.query( + `MATCH (e:${EMBEDDING_TABLE_NAME}) RETURN e.nodeId AS nodeId, e.chunkIndex AS chunkIndex, e.startLine AS startLine, e.endLine AS endLine, e.embedding AS embedding, e.contentHash AS contentHash`, + ); + } catch (err: any) { + // Fallback for legacy DBs without contentHash column + const msg = err?.message ?? ''; + if (isMissingColumnOrTableError(msg)) { + hasContentHash = false; + rows = await c.query( + `MATCH (e:${EMBEDDING_TABLE_NAME}) RETURN e.nodeId AS nodeId, e.chunkIndex AS chunkIndex, e.startLine AS startLine, e.endLine AS endLine, e.embedding AS embedding`, + ); + } else { + throw err; + } + } + for (const row of await readQueryRows(rows)) { + const nodeId = String(row.nodeId ?? row[0] ?? ''); + if (!nodeId) continue; + embeddingNodeIds.add(nodeId); + const embedding = row.embedding ?? row[4]; + if (embedding) { + embeddings.push({ + nodeId, + chunkIndex: Number(row.chunkIndex ?? row[1] ?? 0), + startLine: Number(row.startLine ?? row[2] ?? 0), + endLine: Number(row.endLine ?? row[3] ?? 0), + embedding: Array.isArray(embedding) + ? embedding.map(Number) + : Array.from(embedding as any).map(Number), + contentHash: hasContentHash ? (row.contentHash ?? row[5] ?? undefined) : undefined, + }); + } + } + } catch { + /* embedding table may not exist */ + } + + return { embeddingNodeIds, embeddings }; + }); }; /** @@ -1768,10 +1835,13 @@ export const fetchExistingEmbeddingHashes = async ( * @see safeClose — CHECKPOINT + connection/database close */ export const flushWAL = async (): Promise => { - if (!conn) return; + const c = conn; + if (!c) return; try { - const checkpointResult = await conn.query('CHECKPOINT'); - await drainQueryResult(checkpointResult); + await withConnLock(async () => { + const checkpointResult = await c.query('CHECKPOINT'); + await drainQueryResult(checkpointResult); + }); } catch (err) { logger.debug( `GitNexus: LadybugDB CHECKPOINT skipped/failed during WAL flush: ${summarizeError(err)}`, @@ -1794,9 +1864,15 @@ export const flushWAL = async (): Promise => { * whether to retry. */ export const tryFlushWAL = async (): Promise => { - if (!conn) return false; - const checkpointResult = await conn.query('CHECKPOINT'); - await drainQueryResult(checkpointResult); + const c = conn; + if (!c) return false; + // Runs on the periodic WAL-checkpoint driver. The lock makes this CHECKPOINT + // wait for any in-flight COPY / writeback on the singleton connection instead + // of executing concurrently with it (the `analyze --pdg` heap-corruption bug). + await withConnLock(async () => { + const checkpointResult = await c.query('CHECKPOINT'); + await drainQueryResult(checkpointResult); + }); return true; }; @@ -1856,6 +1932,39 @@ export const safeClose = async (): Promise => { } }; +/** + * CHECKPOINT for durability, then DELIBERATELY skip the native connection/database + * teardown. The name encodes the contract — there is no boolean flag to misuse: + * call this ONLY from a path that guarantees a `process.exit` immediately after + * (the CLI analyze success/SIGINT paths and the forked worker). + * + * LadybugDB's ClientContext/Connection destructor can double-free after large + * --pdg writes (gdb: `double free or corruption` in ClientContext::~ClientContext + * via NodeConnection::Close), aborting the process AFTER a fully-written, + * checkpointed index. flushWAL already persisted the data; process exit reclaims + * the native handles. We leave the handles referenced and module state intact so a + * GC finalizer cannot run the same destructor before exit, and any post-analyze + * read reuses the live connection. Mirrors the pool adapter's fire-and-forget + * native teardown (pool-adapter.ts) and the ONNX native-cleanup philosophy. + * Workaround for a LadybugDB engine bug (to be reported upstream). + * + * SAFETY: only valid when a process.exit is guaranteed to follow. Long-lived + * callers (MCP server, tests) leave `skipNativeCloseOnExit` unset, so + * runFullAnalysis closes for real via {@link closeLbug} — never this. + */ +export const closeLbugBeforeExit = async (): Promise => { + await flushWAL(); + // NOTE (#2264): unlike safeClose, this deliberately does NOT run + // finalizeLbugSidecarsAfterClose. That step inspects/quarantines orphan WAL + + // sidecar files and is designed to run AFTER the native close has released the + // WAL handle; running it here — with the connection still open — would risk a + // Windows file-lock on the in-use WAL for no benefit. The CHECKPOINT above + // already made the index durable, and the next run's preflightLbugSidecars + // reconciles any residual WAL on open. The deferred sidecar housekeeping is the + // accepted trade-off of skipping the native close to dodge the destructor + // double-free. +}; + export const closeLbug = async (): Promise => { await safeClose(); currentDbPath = null; @@ -1903,12 +2012,17 @@ export const deleteNodesForFile = async ( if (tableName === 'Community' || tableName === 'Process') continue; try { - // First count how many we'll delete + // First count how many we'll delete. On the singleton connection this + // count runs inside withConnLock (incremental --pdg writeback executes + // while the WAL driver is live); per-query/temp connections skip the + // lock, matching queryAndDrain's `targetConn === conn` gate — the sibling + // DETACH DELETE below already routes through it. (#2264) const tn = escapeTableName(tableName); - const countResult = await targetConn!.query( - `MATCH (n:${tn}) WHERE n.filePath = '${escapedPath}' RETURN count(n) AS cnt`, - ); - const rows = await readQueryRows(countResult); + const countCypher = `MATCH (n:${tn}) WHERE n.filePath = '${escapedPath}' RETURN count(n) AS cnt`; + const runCount = async () => readQueryRows(await targetConn!.query(countCypher)); + const rows = isSharedSingletonConn(targetConn!) + ? await withConnLock(runCount) + : await runCount(); const count = Number(rows[0]?.cnt ?? rows[0]?.[0] ?? 0); if (count > 0) { @@ -1959,7 +2073,8 @@ export const getEmbeddingTableName = (): string => EMBEDDING_TABLE_NAME; * exports. */ export const queryImporters = async (targetFilePath: string): Promise => { - if (!conn) { + const c = conn; + if (!c) { throw new Error('LadybugDB not initialized. Call initLbug first.'); } const escaped = targetFilePath.replace(/'/g, "''"); @@ -1968,22 +2083,27 @@ export const queryImporters = async (targetFilePath: string): Promise WHERE r.type = 'IMPORTS' AND b.filePath = '${escaped}' RETURN DISTINCT a.filePath AS importer `; - let queryResult: lbug.QueryResult | lbug.QueryResult[] | undefined; - try { - queryResult = await conn.query(cypher); - const result = Array.isArray(queryResult) ? queryResult[0] : queryResult; - const rows = await result.getAll(); - const out: string[] = []; - for (const row of rows) { - const v = (row as { importer?: unknown }).importer; - if (typeof v === 'string' && v.length > 0) out.push(v); + // Runs inside the connection lock: queryImporters is called in the importer-BFS + // loop during incremental --pdg writeback while the WAL driver is live, so an + // unlocked conn.query here could race a concurrent CHECKPOINT on the singleton. + return withConnLock(async () => { + let queryResult: lbug.QueryResult | lbug.QueryResult[] | undefined; + try { + queryResult = await c.query(cypher); + const result = Array.isArray(queryResult) ? queryResult[0] : queryResult; + const rows = await result.getAll(); + const out: string[] = []; + for (const row of rows) { + const v = (row as { importer?: unknown }).importer; + if (typeof v === 'string' && v.length > 0) out.push(v); + } + return out; + } catch { + return []; + } finally { + if (queryResult) await closeQueryResults(queryResult); } - return out; - } catch { - return []; - } finally { - if (queryResult) await closeQueryResults(queryResult); - } + }); }; /** @@ -1996,28 +2116,35 @@ export const queryImporters = async (targetFilePath: string): Promise export const deleteAllCommunitiesAndProcesses = async (): Promise<{ nodesDeleted: number; }> => { - if (!conn) { + const c = conn; + if (!c) { throw new Error('LadybugDB not initialized. Call initLbug first.'); } - let nodesDeleted = 0; - for (const label of ['Community', 'Process']) { - let countResult: lbug.QueryResult | lbug.QueryResult[] | undefined; - try { - countResult = await conn.query(`MATCH (n:${label}) RETURN count(n) AS cnt`); - const result = Array.isArray(countResult) ? countResult[0] : countResult; - const rows = await result.getAll(); - const count = Number(rows[0]?.cnt ?? rows[0]?.[0] ?? 0); - if (count > 0) { - await conn.query(`MATCH (n:${label}) DETACH DELETE n`); - nodesDeleted += count; + // count + DETACH DELETE run inside the connection lock so they cannot execute + // concurrently with the WAL-checkpoint driver's CHECKPOINT on the singleton + // connection. This runs during incremental --pdg writeback while the driver is + // live; mirrors the wrapped deleteAllInterprocTaintPaths / deleteAllCallSummaries. + return withConnLock(async () => { + let nodesDeleted = 0; + for (const label of ['Community', 'Process']) { + let countResult: lbug.QueryResult | lbug.QueryResult[] | undefined; + try { + countResult = await c.query(`MATCH (n:${label}) RETURN count(n) AS cnt`); + const result = Array.isArray(countResult) ? countResult[0] : countResult; + const rows = await result.getAll(); + const count = Number(rows[0]?.cnt ?? rows[0]?.[0] ?? 0); + if (count > 0) { + await closeQueryResults(await c.query(`MATCH (n:${label}) DETACH DELETE n`)); + nodesDeleted += count; + } + } catch { + // Table may not exist yet on a freshly-initialized DB — fine. + } finally { + if (countResult) await closeQueryResults(countResult); } - } catch { - // Table may not exist yet on a freshly-initialized DB — fine. - } finally { - if (countResult) await closeQueryResults(countResult); } - } - return { nodesDeleted }; + return { nodesDeleted }; + }); }; /** @@ -2036,44 +2163,51 @@ export const deleteAllCommunitiesAndProcesses = async (): Promise<{ * plain DELETE on the typed CodeRelation rows — endpoints are untouched. */ export const deleteAllInterprocTaintPaths = async (): Promise<{ edgesDeleted: number }> => { - if (!conn) { + const c = conn; + if (!c) { throw new Error('LadybugDB not initialized. Call initLbug first.'); } - let edgesDeleted = 0; - let countResult: lbug.QueryResult | lbug.QueryResult[] | undefined; - try { - countResult = await conn.query( - `MATCH ()-[r:CodeRelation]->() WHERE r.type = 'TAINT_PATH' RETURN count(r) AS cnt`, - ); - const result = Array.isArray(countResult) ? countResult[0] : countResult; - const rows = await result.getAll(); - const count = Number(rows[0]?.cnt ?? rows[0]?.[0] ?? 0); - if (count > 0) { - await conn.query(`MATCH ()-[r:CodeRelation]->() WHERE r.type = 'TAINT_PATH' DELETE r`); - edgesDeleted = count; - } - } catch (err) { - // A missing table on a freshly-initialized DB is the benign, expected case - // (the count query above is what throws) — stay silent. Any OTHER failure - // (lock, disk, native error) would leave stale TAINT_PATH rows that the - // subsequent re-extract then DUPLICATES (CodeRelation has no PK), so it - // must ABORT the writeback (#2084 review P2-5): re-throw so the caller's - // crash-recovery dirty flag forces a clean full rebuild on the next run, - // rather than silently writing duplicate cross-function findings. - const msg = err instanceof Error ? err.message : String(err); - if (/no table|not exist|not found|does not exist|Table .* does not exist/i.test(msg)) { + // count + DELETE run as one critical section on the singleton connection so a + // concurrent WAL-checkpoint cannot corrupt native state mid-delete (#pdg). + return withConnLock(async () => { + let edgesDeleted = 0; + let countResult: lbug.QueryResult | lbug.QueryResult[] | undefined; + try { + countResult = await c.query( + `MATCH ()-[r:CodeRelation]->() WHERE r.type = 'TAINT_PATH' RETURN count(r) AS cnt`, + ); + const result = Array.isArray(countResult) ? countResult[0] : countResult; + const rows = await result.getAll(); + const count = Number(rows[0]?.cnt ?? rows[0]?.[0] ?? 0); + if (count > 0) { + await closeQueryResults( + await c.query(`MATCH ()-[r:CodeRelation]->() WHERE r.type = 'TAINT_PATH' DELETE r`), + ); + edgesDeleted = count; + } + } catch (err) { + // A missing table on a freshly-initialized DB is the benign, expected case + // (the count query above is what throws) — stay silent. Any OTHER failure + // (lock, disk, native error) would leave stale TAINT_PATH rows that the + // subsequent re-extract then DUPLICATES (CodeRelation has no PK), so it + // must ABORT the writeback (#2084 review P2-5): re-throw so the caller's + // crash-recovery dirty flag forces a clean full rebuild on the next run, + // rather than silently writing duplicate cross-function findings. + const msg = err instanceof Error ? err.message : String(err); + if (/no table|not exist|not found|does not exist|Table .* does not exist/i.test(msg)) { + if (countResult) await closeQueryResults(countResult); + return { edgesDeleted }; + } if (countResult) await closeQueryResults(countResult); - return { edgesDeleted }; + throw new Error( + `[taint-interproc] failed to clear existing TAINT_PATH edges before incremental ` + + `re-write (${msg}) — aborting to avoid duplicate cross-function findings; ` + + `the next run will full-rebuild`, + ); } if (countResult) await closeQueryResults(countResult); - throw new Error( - `[taint-interproc] failed to clear existing TAINT_PATH edges before incremental ` + - `re-write (${msg}) — aborting to avoid duplicate cross-function findings; ` + - `the next run will full-rebuild`, - ); - } - if (countResult) await closeQueryResults(countResult); - return { edgesDeleted }; + return { edgesDeleted }; + }); }; /** @@ -2088,42 +2222,49 @@ export const deleteAllInterprocTaintPaths = async (): Promise<{ edgesDeleted: nu * an unchanged function's summary from being lost. */ export const deleteAllCallSummaries = async (): Promise<{ edgesDeleted: number }> => { - if (!conn) { + const c = conn; + if (!c) { throw new Error('LadybugDB not initialized. Call initLbug first.'); } - let edgesDeleted = 0; - let countResult: lbug.QueryResult | lbug.QueryResult[] | undefined; - try { - countResult = await conn.query( - `MATCH ()-[r:CodeRelation]->() WHERE r.type = 'CALL_SUMMARY' RETURN count(r) AS cnt`, - ); - const result = Array.isArray(countResult) ? countResult[0] : countResult; - const rows = await result.getAll(); - const count = Number(rows[0]?.cnt ?? rows[0]?.[0] ?? 0); - if (count > 0) { - await conn.query(`MATCH ()-[r:CodeRelation]->() WHERE r.type = 'CALL_SUMMARY' DELETE r`); - edgesDeleted = count; - } - } catch (err) { - // A missing table on a freshly-initialized DB is the benign, expected case - // (the count query is what throws) — stay silent. Any OTHER failure would - // leave stale rows that the re-extract then DUPLICATES (CodeRelation has no - // PK), so it must ABORT the writeback: re-throw so the caller's crash- - // recovery dirty flag forces a clean full rebuild on the next run. - const msg = err instanceof Error ? err.message : String(err); - if (/no table|not exist|not found|does not exist|Table .* does not exist/i.test(msg)) { + // count + DELETE run as one critical section on the singleton connection so a + // concurrent WAL-checkpoint cannot corrupt native state mid-delete (#pdg). + return withConnLock(async () => { + let edgesDeleted = 0; + let countResult: lbug.QueryResult | lbug.QueryResult[] | undefined; + try { + countResult = await c.query( + `MATCH ()-[r:CodeRelation]->() WHERE r.type = 'CALL_SUMMARY' RETURN count(r) AS cnt`, + ); + const result = Array.isArray(countResult) ? countResult[0] : countResult; + const rows = await result.getAll(); + const count = Number(rows[0]?.cnt ?? rows[0]?.[0] ?? 0); + if (count > 0) { + await closeQueryResults( + await c.query(`MATCH ()-[r:CodeRelation]->() WHERE r.type = 'CALL_SUMMARY' DELETE r`), + ); + edgesDeleted = count; + } + } catch (err) { + // A missing table on a freshly-initialized DB is the benign, expected case + // (the count query is what throws) — stay silent. Any OTHER failure would + // leave stale rows that the re-extract then DUPLICATES (CodeRelation has no + // PK), so it must ABORT the writeback: re-throw so the caller's crash- + // recovery dirty flag forces a clean full rebuild on the next run. + const msg = err instanceof Error ? err.message : String(err); + if (/no table|not exist|not found|does not exist|Table .* does not exist/i.test(msg)) { + if (countResult) await closeQueryResults(countResult); + return { edgesDeleted }; + } if (countResult) await closeQueryResults(countResult); - return { edgesDeleted }; + throw new Error( + `[call-summary] failed to clear existing CALL_SUMMARY edges before incremental ` + + `re-write (${msg}) — aborting to avoid duplicate summaries; ` + + `the next run will full-rebuild`, + ); } if (countResult) await closeQueryResults(countResult); - throw new Error( - `[call-summary] failed to clear existing CALL_SUMMARY edges before incremental ` + - `re-write (${msg}) — aborting to avoid duplicate summaries; ` + - `the next run will full-rebuild`, - ); - } - if (countResult) await closeQueryResults(countResult); - return { edgesDeleted }; + return { edgesDeleted }; + }); }; // ============================================================================ diff --git a/gitnexus/src/core/lbug/shutdown-helpers.ts b/gitnexus/src/core/lbug/shutdown-helpers.ts new file mode 100644 index 000000000..542709d72 --- /dev/null +++ b/gitnexus/src/core/lbug/shutdown-helpers.ts @@ -0,0 +1,53 @@ +/** + * Shared bounded "checkpoint, then exit" cleanup for interrupt/cancel signals + * (#2264). The CLI SIGINT handler and the forked worker's SIGTERM handler both + * need to: CHECKPOINT the WAL for durability (skipping the native close — see + * closeLbugBeforeExit), but NOT hang behind an in-flight COPY that holds the + * connection lock, and then exit. Bounding the CHECKPOINT with a short timeout + * keeps a single Ctrl-C / cancel responsive; the WAL replays on the next analyze. + */ +import { closeLbugBeforeExit } from './lbug-adapter.js'; + +/** Default cap so a CHECKPOINT queued behind a long COPY can't wedge the signal. */ +export const DEFAULT_EXIT_CLEANUP_TIMEOUT_MS = 2000; + +export interface BoundedCheckpointExitOptions { + /** Exit code to terminate with (130 for SIGINT, 0 for a worker SIGTERM). */ + exitCode: number; + /** Cap on the CHECKPOINT; defaults to {@link DEFAULT_EXIT_CLEANUP_TIMEOUT_MS}. */ + timeoutMs?: number; + /** Report a CHECKPOINT failure (e.g. over IPC) rather than swallowing it. */ + onFlushError?: (err: unknown) => void; + /** Run just before exit (e.g. flush the logger synchronously). */ + beforeExit?: () => void | Promise; + /** @internal test seam — defaults to {@link closeLbugBeforeExit}. */ + checkpoint?: () => Promise; + /** @internal test seam — defaults to `process.exit`. */ + exit?: (code: number) => void; +} + +/** + * Best-effort CHECKPOINT bounded by a timeout, then exit. Never rejects — the + * exit always fires (in `finally`) even if the CHECKPOINT throws. Fire-and-forget + * from a signal handler (`void boundedCheckpointBeforeExit({...})`). + */ +export async function boundedCheckpointBeforeExit( + opts: BoundedCheckpointExitOptions, +): Promise { + const timeoutMs = opts.timeoutMs ?? DEFAULT_EXIT_CLEANUP_TIMEOUT_MS; + const checkpoint = opts.checkpoint ?? closeLbugBeforeExit; + const exit = opts.exit ?? ((code: number) => process.exit(code)); + let timer: ReturnType | undefined; + try { + await Promise.race([ + checkpoint().catch((err: unknown) => opts.onFlushError?.(err)), + new Promise((resolve) => { + timer = setTimeout(resolve, timeoutMs); + }), + ]); + } finally { + if (timer !== undefined) clearTimeout(timer); + await opts.beforeExit?.(); + exit(opts.exitCode); + } +} diff --git a/gitnexus/src/core/lbug/wal-checkpoint-driver.ts b/gitnexus/src/core/lbug/wal-checkpoint-driver.ts index 57dc6b2d6..458c63947 100644 --- a/gitnexus/src/core/lbug/wal-checkpoint-driver.ts +++ b/gitnexus/src/core/lbug/wal-checkpoint-driver.ts @@ -34,6 +34,7 @@ import { logger } from '../logger.js'; import { tryFlushWAL } from './lbug-adapter.js'; +import { markWalDriverActive } from './wal-driver-state.js'; import { isLbugCheckpointIoError } from './lbug-config.js'; /** @@ -162,8 +163,20 @@ export const startWalCheckpointDriver = ( let stopped = false; let inflight: Promise | null = null; + // Arm the streamQuery guard: while this driver runs, an unlocked streamQuery on + // the singleton connection could race a CHECKPOINT (#2264). Cleared in stop(). + markWalDriverActive(true); + const tick = async (): Promise => { if (stopped) return; + // Reentrancy guard: setInterval keeps firing on its fixed cadence even when + // the previous checkpoint has not settled (a CHECKPOINT can outlast the + // period during a large `--pdg` writeback). Without this, each overdue tick + // would queue another CHECKPOINT — they now serialize on the connection lock + // (lbug-adapter `withConnLock`), but letting them pile up is still pointless + // work and widens the window for a backlog at stop(). Skip while one is in + // flight; the next tick covers any WAL accumulated in the meantime. + if (inflight) return; inflight = runCheckpointWithRetry() .then(() => undefined) .catch((err) => { @@ -210,6 +223,9 @@ export const startWalCheckpointDriver = ( /* swallowed in tick() — surface path is the surrounding write */ } } + // Disarm AFTER the in-flight CHECKPOINT drains — clearing it earlier would + // briefly let a streamQuery race the still-finishing CHECKPOINT (#2264). + markWalDriverActive(false); }, }; }; diff --git a/gitnexus/src/core/lbug/wal-driver-state.ts b/gitnexus/src/core/lbug/wal-driver-state.ts new file mode 100644 index 000000000..c86022c71 --- /dev/null +++ b/gitnexus/src/core/lbug/wal-driver-state.ts @@ -0,0 +1,25 @@ +/** + * Shared "is the manual WAL-checkpoint driver running?" flag (#2264). + * + * Lives in its own tiny module — NOT in lbug-adapter — on purpose: the + * wal-checkpoint-driver toggles it and lbug-adapter's `streamQuery` reads it, and + * putting it here keeps that one-bit coupling out of the big, heavily-mocked + * lbug-adapter surface. (Importing it from lbug-adapter forced every test that + * mocks lbug-adapter + loads the real driver to also stub the toggle — a brittle + * ripple this module avoids.) + * + * streamQuery is deliberately not wrapped in withConnLock (its per-row callback can + * re-enter the adapter), so it must refuse to run while the driver is live — + * otherwise its unlocked per-row reads could race a CHECKPOINT on the shared + * connection. The serve/read path never starts the driver, so this stays false + * there. + */ +let walDriverActive = false; + +/** Toggled by the WAL-checkpoint driver's start (true) / stop (false). */ +export const markWalDriverActive = (active: boolean): void => { + walDriverActive = active; +}; + +/** True while the manual WAL-checkpoint driver is running. @see streamQuery */ +export const isWalDriverActive = (): boolean => walDriverActive; diff --git a/gitnexus/src/core/run-analyze.ts b/gitnexus/src/core/run-analyze.ts index cf22cf09e..0b5d9c9ba 100644 --- a/gitnexus/src/core/run-analyze.ts +++ b/gitnexus/src/core/run-analyze.ts @@ -21,6 +21,7 @@ import { executeQuery, executeWithReusedStatement, closeLbug, + closeLbugBeforeExit, loadCachedEmbeddings, deleteNodesForFile, deleteAllCommunitiesAndProcesses, @@ -42,6 +43,7 @@ import { loadMeta, ensureGitNexusIgnored, registerRepo, + isRepoRegistered, cleanupOldKuzuFiles, INCREMENTAL_SCHEMA_VERSION, type RepoMeta, @@ -232,6 +234,15 @@ export interface AnalyzeOptions { * consumer scan unchanged. */ fetchWrappers?: string[]; + /** + * The caller will `process.exit()` immediately after this analyze returns (the + * CLI `analyze` command). When set, the finalize/error close CHECKPOINTs for + * durability but skips the native `conn.close()`/`db.close()`, which can + * double-free in LadybugDB's `ClientContext` destructor after large `--pdg` + * writes (gdb-confirmed) — aborting the process AFTER a fully-written index. + * Process exit reclaims the handles. Long-lived callers (MCP server, tests) + * leave this unset so they get a real close. See `closeLbug`. */ + skipNativeCloseOnExit?: boolean; } export interface AnalyzeResult { @@ -768,7 +779,21 @@ export async function runFullAnalysis( return true; // conservative on git failure } })(); - if (!dirty) { + // Registration wrinkle around the fast path (#2264). A prior + // `analyze --name X` that hit a name collision writes meta.json (meta-save + // runs before registerRepo) then fails before registering, leaving the + // index up-to-date but UNREGISTERED. When the user re-runs with + // --allow-duplicate-name they explicitly want it registered, so fall + // through to the pipeline (which registers it, honoring the flag) instead + // of early-returning an unregistered repo the flag could never heal. + // For a PLAIN analyze we deliberately do NOT self-heal: an up-to-date but + // unregistered repo early-returns here and the CLI's assertAnalysisFinalized + // surfaces it as a hard failure (#1169) rather than silently registering a + // possibly half-finalized index. `isRepoRegistered` is only read on the + // opt-in branch so the common fast path keeps its single-stat cost. + const healUnregistered = + options.allowDuplicateName === true && !(await isRepoRegistered(repoPath)); + if (!dirty && !healUnregistered) { await ensureGitNexusIgnored(repoPath); return { // `resolveRepoIdentityRoot` collapses worktree roots to the @@ -1549,7 +1574,11 @@ export async function runFullAnalysis( // Stop the manual checkpoint driver before closeLbug so its // in-flight CHECKPOINT cannot race the `safeClose` CHECKPOINT. await walCheckpointDriver.stop(); - await closeLbug(); + // CLI callers (about to process.exit) skip the native close to dodge a + // LadybugDB destructor double-free after --pdg writes — closeLbugBeforeExit + // CHECKPOINTs for durability then leaves the handles for process exit to + // reclaim (#2264). Long-lived callers close for real. + await (options.skipNativeCloseOnExit ? closeLbugBeforeExit() : closeLbug()); progress('done', 100, 'Done'); @@ -1570,7 +1599,15 @@ export async function runFullAnalysis( /* swallow — surface path is the rethrow below */ } try { - await closeLbug(); + // Skip the native close on the error path too: a real conn.close() after + // large --pdg writes can itself abort in LadybugDB's ClientContext + // destructor (#2264 review P2), turning an actionable exit-1 into a raw + // SIGABRT. closeLbugBeforeExit leaves the handles open, but the CLI catch + // now force-exits when isLbugReady() (analyze.ts, #2264 review P1), so the + // process still terminates — no hang, no abort. flushWAL keeps the partial + // index durable; process exit reclaims the handles. Long-lived callers + // (skipNativeCloseOnExit unset) close for real. + await (options.skipNativeCloseOnExit ? closeLbugBeforeExit() : closeLbug()); } catch { /* swallow */ } diff --git a/gitnexus/src/server/analyze-job.ts b/gitnexus/src/server/analyze-job.ts index f7d97e97d..d62912abd 100644 --- a/gitnexus/src/server/analyze-job.ts +++ b/gitnexus/src/server/analyze-job.ts @@ -101,6 +101,12 @@ export class JobManager { const job = this.jobs.get(id); if (!job) return; + // Once a job is terminal (complete/failed) its outcome is immutable — drop any + // later update so a worker `complete` racing a SIGTERM-driven `error` (or vice + // versa) can't flip a reported result (#2264 P3). The transition INTO a terminal + // state still applies because `job.status` is not yet terminal at that point. + if (this.isTerminal(job.status)) return; + Object.assign(job, update); if (this.isTerminal(job.status)) { diff --git a/gitnexus/src/server/analyze-launch.ts b/gitnexus/src/server/analyze-launch.ts index 184c5a813..87a463ca0 100644 --- a/gitnexus/src/server/analyze-launch.ts +++ b/gitnexus/src/server/analyze-launch.ts @@ -81,6 +81,13 @@ export function createLaunchAnalysisWorker(deps: LaunchDeps) { }); child.on('message', (msg: WorkerMessage) => { + // Ignore any message once the job is terminal — a late worker message (a + // SIGTERM-driven `error` after `complete`, or vice versa) must not + // re-release the repo lock or flip the reported status. Mirrors the `exit` + // handler guard below; pairs with the worker's terminal-claim (#2264 P3). + const current = jobManager.getJob(job.id); + if (!current || current.status === 'complete' || current.status === 'failed') return; + if (msg.type === 'progress') { jobManager.updateJob(job.id, { status: 'analyzing', diff --git a/gitnexus/src/server/analyze-worker-core.ts b/gitnexus/src/server/analyze-worker-core.ts new file mode 100644 index 000000000..acb31e6d1 --- /dev/null +++ b/gitnexus/src/server/analyze-worker-core.ts @@ -0,0 +1,94 @@ +/** + * Side-effect-free core of the analyze worker's message handler. + * + * Extracted from `analyze-worker.ts` — a `fork()` entry module whose top-level + * `process.on(...)` handlers and `ready` handshake make it unsafe to import in a + * unit test. This module has no top-level side effects and takes its collaborators + * by dependency injection, so the worker's run → finalize → report contract is + * unit-testable without spawning a process. The entry module wires the real deps + * and owns the `process.exit` lifecycle. + * + * The `import type ... typeof import(...)` forms below are erased at runtime, so + * importing this module does NOT load `run-analyze`, `repo-manager`, or the entry + * worker — only the lightweight `analyze-worker-ipc` projection helper. + */ +import type { AnalyzeOptions } from '../core/run-analyze.js'; +import type { WorkerMessage } from './analyze-worker.js'; +import { projectAnalyzeResultForIpc } from './analyze-worker-ipc.js'; + +export interface WorkerAnalysisDeps { + runFullAnalysis: typeof import('../core/run-analyze.js').runFullAnalysis; + assertAnalysisFinalized: typeof import('../storage/repo-manager.js').assertAnalysisFinalized; + send: (msg: WorkerMessage) => void; + /** + * Claim the single terminal-outcome slot. Returns `true` for the first caller + * (which may then send its `complete`/`error`) and `false` for every caller + * after — so a SIGTERM cancellation and a near-simultaneous completion can't + * both report a terminal outcome (#2264 P3). See {@link createTerminalClaim}. + */ + claimTerminal: () => boolean; +} + +/** + * Run the analysis and report the outcome to the parent over IPC. Reports at most + * one terminal message (`complete` or `error`) — and none if a cancellation + * already claimed the terminal slot — and never throws; the caller schedules + * `process.exit` after this resolves. + */ +export async function runWorkerAnalysis( + repoPath: string, + options: AnalyzeOptions, + deps: WorkerAnalysisDeps, +): Promise { + let terminal: WorkerMessage; + try { + const result = await deps.runFullAnalysis( + repoPath, + // This worker force-exits right after reporting, so skip the native close + // (it can double-free in LadybugDB's ClientContext destructor after --pdg + // writes); flushWAL still persists the index, process.exit reclaims handles. + { ...options, skipNativeCloseOnExit: true }, + { + onProgress: (phase, percent, message) => + deps.send({ type: 'progress', phase, percent, message }), + onLog: (message) => deps.send({ type: 'progress', phase: 'log', percent: -1, message }), + }, + ); + // P2 (#2264): a half-finalized repo — meta.json written but the global + // registry entry missing (e.g. a prior collision-aborted run, or a wiped + // registry) — must NOT be reported as a successful analysis. Mirror the CLI's + // assertAnalysisFinalized guard so the worker surfaces it as an error instead + // of a false `complete` that leaves the repo invisible to list_repos. + await deps.assertAnalysisFinalized(repoPath); + + // Send a JSON-safe projection, NOT the raw result: the IPC channel is + // default-JSON serialization and `result.pipelineResult` carries the live + // KnowledgeGraph. See analyze-worker-ipc.ts. + terminal = { type: 'complete', result: projectAnalyzeResultForIpc(result) }; + } catch (err: unknown) { + // Report the failure to the parent over IPC (the parent surfaces the message). + const message = err instanceof Error ? err.message : 'Analysis failed'; + terminal = { type: 'error', message }; + } + + // P3 (#2264): only report if a SIGTERM cancellation hasn't already claimed the + // terminal slot — otherwise a cancel near the finish line would report the + // analysis as `complete` over the top of the cancellation. + if (deps.claimTerminal()) deps.send(terminal); +} + +/** + * Create the single-use terminal-outcome claim shared by the worker's message + * handler and its SIGTERM handler. The first call returns `true`; every later + * call returns `false`. This is the coordination point that prevents a cancel and + * a completion from both reporting a terminal status (#2264 P3). Single-threaded + * JS guarantees the check-and-set is atomic (no preemption mid-call). + */ +export function createTerminalClaim(): () => boolean { + let claimed = false; + return () => { + if (claimed) return false; + claimed = true; + return true; + }; +} diff --git a/gitnexus/src/server/analyze-worker.ts b/gitnexus/src/server/analyze-worker.ts index d14fb0d39..f8d583e71 100644 --- a/gitnexus/src/server/analyze-worker.ts +++ b/gitnexus/src/server/analyze-worker.ts @@ -12,8 +12,10 @@ */ import { runFullAnalysis, type AnalyzeOptions } from '../core/run-analyze.js'; -import { projectAnalyzeResultForIpc, type AnalyzeResultIpc } from './analyze-worker-ipc.js'; -import { closeLbug } from '../core/lbug/lbug-adapter.js'; +import { type AnalyzeResultIpc } from './analyze-worker-ipc.js'; +import { runWorkerAnalysis, createTerminalClaim } from './analyze-worker-core.js'; +import { assertAnalysisFinalized } from '../storage/repo-manager.js'; +import { boundedCheckpointBeforeExit } from '../core/lbug/shutdown-helpers.js'; interface StartMessage { type: 'start'; @@ -44,27 +46,62 @@ export interface ErrorMessage { export type WorkerMessage = ProgressMessage | CompleteMessage | ErrorMessage; function send(msg: WorkerMessage) { + // No try/catch: if the IPC channel is gone, process.send throws + // (ERR_IPC_CHANNEL_CLOSED) and that failure must NOT be swallowed. Every caller + // schedules its process.exit inside a `finally`, so a throw here still tears the + // worker down deterministically instead of wedging the event loop (#2264 P3). process.send?.(msg); } -// Catch uncaught exceptions and unhandled rejections — report to parent -process.on('uncaughtException', (err) => { - send({ type: 'error', message: err?.message || 'Uncaught exception in worker' }); - setTimeout(() => process.exit(1), 500); -}); +// Single terminal-outcome slot shared by the message handler and the SIGTERM +// handler: whoever claims it first reports its complete/error; the other skips its +// terminal send, so a cancel near the finish line can't also report success and a +// late SIGTERM can't flip an already-reported job (#2264 P3). +const claimTerminal = createTerminalClaim(); -process.on('unhandledRejection', (reason: any) => { - send({ type: 'error', message: reason?.message || 'Unhandled rejection in worker' }); - setTimeout(() => process.exit(1), 500); -}); - -// Handle graceful shutdown — notify parent before exit -process.on('SIGTERM', async () => { - send({ type: 'error', message: 'Analysis cancelled (worker received SIGTERM)' }); +// Catch uncaught exceptions and unhandled rejections — report them to the parent +// over IPC (the same channel the analysis path uses), then exit. The report runs +// in `try` and the exit in `finally` so a throw from send() on a closed channel +// can't skip the exit and leave the worker wedged (#2264 review P3). +process.on('uncaughtException', (err: unknown) => { try { - await closeLbug(); - } catch {} - process.exit(0); + const message = err instanceof Error ? err.message : 'Uncaught exception in worker'; + send({ type: 'error', message }); + } finally { + setTimeout(() => process.exit(1), 500); + } +}); + +process.on('unhandledRejection', (reason: unknown) => { + try { + const message = reason instanceof Error ? reason.message : 'Unhandled rejection in worker'; + send({ type: 'error', message }); + } finally { + setTimeout(() => process.exit(1), 500); + } +}); + +// Handle cancellation / timeout shutdown (analyze-job.ts `cancelJob` sends +// SIGTERM). Bounded CHECKPOINT-then-exit shared with the CLI SIGINT path (#2264): +// skip the native close (the LadybugDB destructor can double-free after --pdg +// writes), but don't block behind the in-flight COPY's connection lock — so a +// single cancel can't abort or hang the worker. A CHECKPOINT failure is reported +// to the parent over IPC, not swallowed; the exit always fires. +process.on('SIGTERM', () => { + // Only report the cancellation if the analysis hasn't already reported a + // terminal outcome (#2264 P3) — otherwise this would flip an already-complete + // job to failed. The cleanup + exit below run regardless. + if (claimTerminal()) { + send({ type: 'error', message: 'Analysis cancelled (worker received SIGTERM)' }); + } + void boundedCheckpointBeforeExit({ + exitCode: 0, + onFlushError: (err: unknown) => { + const message = + err instanceof Error ? err.message : 'Worker checkpoint failed during SIGTERM'; + send({ type: 'error', message }); + }, + }); }); // Listen for start command from parent — guarded against re-entry @@ -74,26 +111,20 @@ process.on('message', async (msg: StartMessage) => { started = true; try { - const result = await runFullAnalysis(msg.repoPath, msg.options, { - onProgress: (phase, percent, message) => { - send({ type: 'progress', phase, percent, message }); - }, - onLog: (message) => { - send({ type: 'progress', phase: 'log', percent: -1, message }); - }, + // The run → finalize → report contract lives in the side-effect-free + // analyze-worker-core seam (unit-testable without this entry module's + // process.on side effects). It reports exactly one terminal message and + // never throws. + await runWorkerAnalysis(msg.repoPath, msg.options, { + runFullAnalysis, + assertAnalysisFinalized, + send, + claimTerminal, }); - - // Send a JSON-safe projection, NOT the raw result: the IPC channel is - // default-JSON serialization and `result.pipelineResult` carries the live - // KnowledgeGraph (wasteful to materialize, silently corrupted by JSON, and - // a BigInt/circular value would throw and mis-report this success as a - // failure). See analyze-worker-ipc.ts. - send({ type: 'complete', result: projectAnalyzeResultForIpc(result) }); - } catch (err: any) { - send({ type: 'error', message: err?.message || 'Analysis failed' }); + } finally { + // LadybugDB's native module prevents clean exit — force it (same reason the + // CLI uses process.exit(0)). In `finally` so the exit still fires even if the + // report above throws on a closed IPC channel (#2264 review P3). + setTimeout(() => process.exit(0), 500); } - - // LadybugDB's native module prevents clean exit — force it - // (same reason the CLI uses process.exit(0)) - setTimeout(() => process.exit(0), 500); }); diff --git a/gitnexus/src/storage/repo-manager.ts b/gitnexus/src/storage/repo-manager.ts index 2a63d7fb5..8b18b0a17 100644 --- a/gitnexus/src/storage/repo-manager.ts +++ b/gitnexus/src/storage/repo-manager.ts @@ -63,6 +63,15 @@ export const canonicalizePath = (p: string): string => { } }; +/** + * Compare two already-canonicalised registry paths. Case-insensitive on Windows + * (its filesystem is), case-sensitive elsewhere. Both arguments must already be + * run through {@link canonicalizePath}; this is the single comparison the registry + * lookups/dedup/finalize checks all share so they answer identically. + */ +export const registryPathEquals = (a: string, b: string): boolean => + process.platform === 'win32' ? a.toLowerCase() === b.toLowerCase() : a === b; + export interface RepoMeta { repoPath: string; lastCommit: string; @@ -709,7 +718,7 @@ export const registerRepo = async ( // to a stable key instead of throwing. const a = canonicalizePath(e.path); const b = canonicalInput; - return process.platform === 'win32' ? a.toLowerCase() === b.toLowerCase() : a === b; + return registryPathEquals(a, b); }); const existing = existingIdx >= 0 ? entries[existingIdx] : null; @@ -817,9 +826,7 @@ export const registerRepo = async ( const fresh = await readRegistry(); const freshIdx = fresh.findIndex((e) => { const a = canonicalizePath(e.path); - return process.platform === 'win32' - ? a.toLowerCase() === canonicalInput.toLowerCase() - : a === canonicalInput; + return registryPathEquals(a, canonicalInput); }); const freshExisting = freshIdx >= 0 ? fresh[freshIdx] : null; let merged: RegistryEntry; @@ -858,9 +865,7 @@ export const unregisterRepo = async (repoPath: string): Promise => { // `resolveRegistryEntry` post-#1003 review. const resolved = canonicalizePath(repoPath); const entries = await readRegistry(); - const matches = (a: string, b: string) => - process.platform === 'win32' ? a.toLowerCase() === b.toLowerCase() : a === b; - const filtered = entries.filter((e) => !matches(canonicalizePath(e.path), resolved)); + const filtered = entries.filter((e) => !registryPathEquals(canonicalizePath(e.path), resolved)); await writeRegistry(filtered); }; @@ -874,10 +879,8 @@ export const unregisterRepo = async (repoPath: string): Promise => { */ export const removeBranchIndex = async (repoPath: string, branch: string): Promise => { const resolved = canonicalizePath(repoPath); - const matches = (a: string, b: string) => - process.platform === 'win32' ? a.toLowerCase() === b.toLowerCase() : a === b; const entries = await readRegistry(); - const idx = entries.findIndex((e) => matches(canonicalizePath(e.path), resolved)); + const idx = entries.findIndex((e) => registryPathEquals(canonicalizePath(e.path), resolved)); if (idx < 0) return false; const entry = entries[idx]; const before = entry.branches?.length ?? 0; @@ -978,6 +981,18 @@ export class AnalysisNotFinalizedError extends Error { } } +/** + * True when the global registry already contains an entry whose canonical path + * matches `repoPath`. Uses the same canonical, case-folded (Windows) comparison + * as {@link assertAnalysisFinalized} so "is it registered?" answers identically + * at the analyze fast-path gate and at the finalize assertion. Pure read. + */ +export const isRepoRegistered = async (repoPath: string): Promise => { + const entries = await readRegistry(); + const canonicalInput = canonicalizePath(path.resolve(repoPath)); + return entries.some((e) => registryPathEquals(canonicalizePath(e.path), canonicalInput)); +}; + /** * Verify that a successful `analyze` call actually produced an indexed, * registered repo on disk. Two checks, both strictly required: @@ -1002,14 +1017,7 @@ export const assertAnalysisFinalized = async (repoPath: string): Promise = throw new AnalysisNotFinalizedError(resolved, storagePath, 'meta', getGlobalRegistryPath()); } - const entries = await readRegistry(); - const canonicalInput = canonicalizePath(resolved); - const isWin = process.platform === 'win32'; - const found = entries.some((e) => { - const a = canonicalizePath(e.path); - return isWin ? a.toLowerCase() === canonicalInput.toLowerCase() : a === canonicalInput; - }); - if (!found) { + if (!(await isRepoRegistered(resolved))) { throw new AnalysisNotFinalizedError( resolved, storagePath, @@ -1125,7 +1133,7 @@ export const resolveRegistryEntry = (entries: RegistryEntry[], target: string): const pathMatch = entries.find((e) => { const a = canonicalizePath(e.path); const b = canonicalTarget; - return process.platform === 'win32' ? a.toLowerCase() === b.toLowerCase() : a === b; + return registryPathEquals(a, b); }); if (pathMatch) return pathMatch; diff --git a/gitnexus/test/integration/lbug-conn-serialization.test.ts b/gitnexus/test/integration/lbug-conn-serialization.test.ts new file mode 100644 index 000000000..57a1ec32e --- /dev/null +++ b/gitnexus/test/integration/lbug-conn-serialization.test.ts @@ -0,0 +1,155 @@ +/** + * Integration tests: every singleton-`conn` helper reachable during the + * WAL-checkpoint-driver window must route through `withConnLock` (PR #2264 + * tri-review, P1). These helpers issued raw `conn.query` on the shared + * connection while the driver could fire a concurrent CHECKPOINT — the same + * native double-free this branch fixes, on the incremental `--pdg` path. + * + * Mirrors `lbug-core-adapter.test.ts`: one isolated temp DB via `withTestLbugDB`. + * `withConnLock` is mocked to a call-through spy so we can assert each helper + * acquires the lock while the real serialization still runs. Routing assertions + * use an empty (initialized) DB — they prove the lock is taken regardless of + * whether any rows match. + */ +import { describe, it, expect, vi, beforeEach } from 'vitest'; +import { withTestLbugDB } from '../helpers/test-indexed-db.js'; +import { NODE_TABLES } from '../../src/core/lbug/schema.js'; + +// Spy `withConnLock` while preserving its real behavior (call-through). The +// adapter imports this module, so the spy observes every lock acquisition. +vi.mock('../../src/core/lbug/conn-lock.js', async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + withConnLock: vi.fn(actual.withConnLock) as typeof actual.withConnLock, + }; +}); +import { withConnLock } from '../../src/core/lbug/conn-lock.js'; +const lockSpy = vi.mocked(withConnLock); + +// Spy `closeQueryResults` (call-through) to prove the deleteAll* helpers now +// drain/close their DELETE result, not just the count result (P2 #2264). +vi.mock('../../src/core/lbug/query-result-utils.js', async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + closeQueryResults: vi.fn(actual.closeQueryResults) as typeof actual.closeQueryResults, + }; +}); +import { closeQueryResults } from '../../src/core/lbug/query-result-utils.js'; +const closeSpy = vi.mocked(closeQueryResults); + +withTestLbugDB('conn-serialization', () => { + describe('singleton-conn helpers acquire withConnLock (P1 #2264)', () => { + beforeEach(() => { + // Setup (clear/seed/flush) already exercised the lock; reset so each + // assertion reflects only the helper under test. + lockSpy.mockClear(); + }); + + it('U1: deleteAllCommunitiesAndProcesses routes through withConnLock', async () => { + const { deleteAllCommunitiesAndProcesses } = + await import('../../src/core/lbug/lbug-adapter.js'); + const result = await deleteAllCommunitiesAndProcesses(); + expect(lockSpy).toHaveBeenCalled(); + expect(result).toMatchObject({ nodesDeleted: 0 }); + }); + + it('U2: queryImporters routes through withConnLock', async () => { + const { queryImporters } = await import('../../src/core/lbug/lbug-adapter.js'); + const importers = await queryImporters('any/path.ts'); + expect(lockSpy).toHaveBeenCalled(); + expect(importers).toEqual([]); + }); + + it('U3: deleteNodesForFile (singleton) locks every per-table count query', async () => { + const { deleteNodesForFile } = await import('../../src/core/lbug/lbug-adapter.js'); + const result = await deleteNodesForFile('any/path.ts'); + // One locked count per filePath-bearing node table (Community/Process are + // skipped), proving the count read — not just the already-locked DELETE — + // now serializes. Baseline (count unlocked) would show ~1 lock call. + const filePathTables = NODE_TABLES.filter((t) => t !== 'Community' && t !== 'Process'); + expect(lockSpy.mock.calls.length).toBeGreaterThanOrEqual(filePathTables.length); + expect(result).toMatchObject({ deletedNodes: 0 }); + }); + + it('closeLbugBeforeExit() checkpoints but leaves the connection open (#2264 close-crash)', async () => { + const adapter = await import('../../src/core/lbug/lbug-adapter.js'); + await adapter.closeLbugBeforeExit(); + // The native conn/db are deliberately NOT torn down — that avoids LadybugDB's + // ClientContext destructor double-free after --pdg writes. The connection + // stays ready and queryable (the CHECKPOINT made the index durable; process + // exit reclaims the handles on the CLI path). + expect(adapter.isLbugReady()).toBe(true); + const rows = await adapter.executeQuery('RETURN 1 AS one'); + expect(rows).toHaveLength(1); + }); + + it('U3: loadCachedEmbeddings routes through withConnLock', async () => { + const { loadCachedEmbeddings } = await import('../../src/core/lbug/lbug-adapter.js'); + const cached = await loadCachedEmbeddings(); + expect(lockSpy).toHaveBeenCalled(); + expect(cached.embeddings).toEqual([]); + expect(cached.embeddingNodeIds.size).toBe(0); + }); + + it('U4: deleteAllInterprocTaintPaths routes through withConnLock', async () => { + const { deleteAllInterprocTaintPaths } = await import('../../src/core/lbug/lbug-adapter.js'); + const result = await deleteAllInterprocTaintPaths(); + expect(lockSpy).toHaveBeenCalled(); + expect(result).toMatchObject({ edgesDeleted: 0 }); + }); + + it('U4: deleteAllCallSummaries routes through withConnLock', async () => { + const { deleteAllCallSummaries } = await import('../../src/core/lbug/lbug-adapter.js'); + const result = await deleteAllCallSummaries(); + expect(lockSpy).toHaveBeenCalled(); + expect(result).toMatchObject({ edgesDeleted: 0 }); + }); + + it('U5: deleteNodesForFile on a temp dbPath does NOT take the lock (negative gate)', async () => { + // The targetConn === conn gate's negative branch: a per-file/temp connection + // (dbPath provided) must NOT take the singleton lock, so temp-conn callers + // can't contend with the singleton. Mirrors the positive U3 case above so a + // regression that unconditionally locks is caught. (#2264) + const { createTempDir } = await import('../helpers/test-db.js'); + const { deleteNodesForFile } = await import('../../src/core/lbug/lbug-adapter.js'); + const temp = await createTempDir('gn-negative-gate-'); + try { + lockSpy.mockClear(); + const result = await deleteNodesForFile('any/path.ts', temp.dbPath); + expect(lockSpy).not.toHaveBeenCalled(); + expect(result).toMatchObject({ deletedNodes: 0 }); + } finally { + await temp.cleanup(); + } + }); + }); +}); + +withTestLbugDB( + 'conn-serialization-drain', + () => { + describe('deleteAll* drain their DELETE result (P2 #2264)', () => { + beforeEach(() => { + closeSpy.mockClear(); + }); + + it('U4: deleteAllCommunitiesAndProcesses closes the DETACH DELETE result', async () => { + const { deleteAllCommunitiesAndProcesses } = + await import('../../src/core/lbug/lbug-adapter.js'); + const result = await deleteAllCommunitiesAndProcesses(); + // Seed has 1 Community, 0 Process. With the drain fix, closeQueryResults + // fires for: Community count, Community DETACH DELETE, Process count = 3. + // Without the fix the delete result is dropped → only 2 closes. + expect(result).toMatchObject({ nodesDeleted: 1 }); + expect(closeSpy.mock.calls.length).toBeGreaterThanOrEqual(3); + }); + }); + }, + { + seed: [ + "CREATE (c:Community {id: 'comm:drain', label: 'Drain', heuristicLabel: 'Drain', keywords: ['x'], description: 'd', enrichedBy: 'heuristic', cohesion: 0.5, symbolCount: 1})", + ], + }, +); diff --git a/gitnexus/test/unit/analyze-embedding-endpoint-flags.test.ts b/gitnexus/test/unit/analyze-embedding-endpoint-flags.test.ts index 69c51e34c..ee1a8b22c 100644 --- a/gitnexus/test/unit/analyze-embedding-endpoint-flags.test.ts +++ b/gitnexus/test/unit/analyze-embedding-endpoint-flags.test.ts @@ -8,6 +8,8 @@ vi.mock('../../src/core/run-analyze.js', () => ({ vi.mock('../../src/core/lbug/lbug-adapter.js', () => ({ closeLbug: vi.fn(async () => undefined), + closeLbugBeforeExit: vi.fn(async () => undefined), + isLbugReady: vi.fn(() => false), })); vi.mock('../../src/storage/repo-manager.js', () => ({ diff --git a/gitnexus/test/unit/analyze-embeddings-limit.test.ts b/gitnexus/test/unit/analyze-embeddings-limit.test.ts index 6fbc5af56..c7f12fe7f 100644 --- a/gitnexus/test/unit/analyze-embeddings-limit.test.ts +++ b/gitnexus/test/unit/analyze-embeddings-limit.test.ts @@ -8,6 +8,8 @@ vi.mock('../../src/core/run-analyze.js', () => ({ vi.mock('../../src/core/lbug/lbug-adapter.js', () => ({ closeLbug: vi.fn(async () => undefined), + closeLbugBeforeExit: vi.fn(async () => undefined), + isLbugReady: vi.fn(() => false), })); vi.mock('../../src/storage/repo-manager.js', () => ({ diff --git a/gitnexus/test/unit/analyze-finalize-failure-exits.test.ts b/gitnexus/test/unit/analyze-finalize-failure-exits.test.ts new file mode 100644 index 000000000..c87008726 --- /dev/null +++ b/gitnexus/test/unit/analyze-finalize-failure-exits.test.ts @@ -0,0 +1,180 @@ +/** + * Regression test for the #2264 review P1: when a full analyze succeeds (which + * skip-closes LadybugDB, leaving native handles open) and a post-finalize step + * THEN throws, the CLI's outer catch soft-returns (`process.exitCode = 1`). With + * native handles open, the event loop can't drain — the process would HANG. The + * `analyzeCommand` wrapper now force-exits when `isLbugReady()` is true after the + * soft return. This test drives that exact path and asserts termination. + * + * Test-safety: when `isLbugReady()` is false (the default in every analyze unit + * test that mocks run-analyze — the DB is never opened), the wrapper must NOT + * force-exit, preserving the soft return those tests rely on. + * + * Worker-safety (#2264 CI): the module is imported ONCE and the mocks are driven + * per-test via `mockReturnValue`. The earlier `vi.resetModules()` + per-test + * `await import('analyze.js')` re-instrumented the ENTIRE analyze module graph on + * every test; under `--coverage` on the memory-constrained CI runner that + * OOM/crashed the forked worker ("Worker exited unexpectedly"), even though it + * passed locally. `analyzeCommand` also installs global fatal handlers + * (installFatalHandlers) that call the REAL process.exit(1); we keep process.exit + * spied for the whole file so one firing can't kill the worker, strip the handlers + * it added in afterAll, and reset process.exitCode so the worker exits clean. + */ +import { + afterAll, + afterEach, + beforeAll, + beforeEach, + describe, + expect, + it, + vi, + type MockInstance, +} from 'vitest'; + +const { + runFullAnalysisMock, + assertAnalysisFinalizedMock, + isLbugReadyMock, + AnalysisNotFinalizedError, +} = vi.hoisted(() => { + class AnalysisNotFinalizedError extends Error { + storagePath = '.gitnexus'; + } + return { + runFullAnalysisMock: vi.fn(), + assertAnalysisFinalizedMock: vi.fn(), + isLbugReadyMock: vi.fn(() => false), + AnalysisNotFinalizedError, + }; +}); + +vi.mock('../../src/core/run-analyze.js', () => ({ runFullAnalysis: runFullAnalysisMock })); +vi.mock('../../src/cli/ai-context.js', () => ({ + generateAIContextFiles: vi.fn(async () => ({ files: [] as string[] })), + refreshBaseRefLine: vi.fn(async () => ({ files: [] as string[] })), +})); +vi.mock('../../src/cli/skill-gen.js', () => ({ generateSkillFiles: vi.fn() })); +vi.mock('../../src/cli/cli-message.js', () => ({ cliError: vi.fn() })); +vi.mock('../../src/core/lbug/lbug-adapter.js', () => ({ + closeLbug: vi.fn(async () => undefined), + closeLbugBeforeExit: vi.fn(async () => undefined), + isLbugReady: isLbugReadyMock, +})); +vi.mock('../../src/storage/repo-manager.js', () => ({ + getStoragePaths: vi.fn(() => ({ storagePath: '.gitnexus', lbugPath: '.gitnexus/lbug' })), + getGlobalRegistryPath: vi.fn(() => 'registry.json'), + RegistryNameCollisionError: class RegistryNameCollisionError extends Error {}, + AnalysisNotFinalizedError, + assertAnalysisFinalized: assertAnalysisFinalizedMock, +})); +vi.mock('../../src/storage/git.js', () => ({ + getGitRoot: vi.fn(() => '/repo'), + hasGitDir: vi.fn(() => true), + getDefaultBranch: vi.fn(() => null), +})); +vi.mock('../../src/core/ingestion/utils/max-file-size.js', () => ({ + getMaxFileSizeBannerMessage: vi.fn(() => null), +})); + +// Imported ONCE (not re-imported per test) — see the worker-safety note above. +import { analyzeCommand } from '../../src/cli/analyze.js'; + +describe('analyzeCommand — finalize-failure must terminate, not hang (#2264 P1)', () => { + // Snapshot the fatal-handler listeners present BEFORE this file ran (vitest's + // own) so afterAll strips only the ones installFatalHandlers added. + const baselineUnhandled = process.listeners('unhandledRejection'); + const baselineUncaught = process.listeners('uncaughtException'); + let exitSpy: MockInstance; + let savedNodeOptions: string | undefined; + + beforeAll(() => { + // analyzeCommand calls ensureHeap(), which RE-EXECS the process — spawning + // `node ` where argv is vitest's, killing the forked + // worker — UNLESS NODE_OPTIONS already carries a heap cap (analyze.ts:498). + // Locally a high V8 heap-size-limit also short-circuits it (analyze.ts:501), + // which is why this only crashed on the memory-constrained CI runner. Pre-set + // the cap so ensureHeap returns early — the same workaround cli-e2e uses + // (#2264 CI). Restored in afterAll so a reused worker's later files are clean. + savedNodeOptions = process.env.NODE_OPTIONS; + process.env.NODE_OPTIONS = `${process.env.NODE_OPTIONS ?? ''} --max-old-space-size=8192`.trim(); + // Mock process.exit for the WHOLE file — a fatal handler firing between tests + // (after a per-test spy would have been restored) can't really exit. + exitSpy = vi.spyOn(process, 'exit').mockImplementation(() => undefined as never); + }); + + afterAll(() => { + // Strip the handlers installFatalHandlers added BEFORE restoring the real + // process.exit, so no stray rejection during teardown fires a real exit. Only + // remove non-baseline (vitest's own) listeners. + process + .listeners('unhandledRejection') + .filter((l) => !baselineUnhandled.includes(l)) + .forEach((l) => process.removeListener('unhandledRejection', l)); + process + .listeners('uncaughtException') + .filter((l) => !baselineUncaught.includes(l)) + .forEach((l) => process.removeListener('uncaughtException', l)); + exitSpy.mockRestore(); + process.env.NODE_OPTIONS = savedNodeOptions ?? ''; + process.exitCode = 0; + }); + + beforeEach(() => { + exitSpy.mockClear(); + runFullAnalysisMock.mockReset(); + // Full analysis succeeded (NOT the alreadyUpToDate fast path) → skip-closed. + runFullAnalysisMock.mockResolvedValue({ + repoName: 'repo', + repoPath: '/repo', + stats: {}, + alreadyUpToDate: false, + ftsRepairedOnly: false, + pipelineResult: { communityResult: undefined }, + }); + assertAnalysisFinalizedMock.mockReset(); + // Post-finalize check throws (the documented silent-finalize state). + assertAnalysisFinalizedMock.mockRejectedValue(new AnalysisNotFinalizedError('not finalized')); + isLbugReadyMock.mockReset(); + process.exitCode = undefined; + }); + + afterEach(() => { + // Don't leak a non-zero exit code to the forked worker's natural exit. + process.exitCode = 0; + }); + + it('force-exits when native handles are still open (isLbugReady true)', async () => { + isLbugReadyMock.mockReturnValue(true); + await analyzeCommand(undefined, {}); + expect(exitSpy).toHaveBeenCalledWith(1); + }); + + it('does NOT force-exit when no handles are open (isLbugReady false) — soft return preserved', async () => { + isLbugReadyMock.mockReturnValue(false); + await analyzeCommand(undefined, {}); + expect(exitSpy).not.toHaveBeenCalled(); + expect(process.exitCode).toBe(1); + }); + + it('forwards a pre-set process.exitCode rather than the hardcoded fallback', async () => { + // The alreadyUpToDate path returns WITHOUT setting process.exitCode or calling + // process.exit (unlike the error catch, which always sets exitCode=1), so the + // wrapper's force-exit must forward whatever exitCode is already set — proving + // `process.exit(process.exitCode ?? 1)` reads exitCode and doesn't hardcode 1. + // isLbugReady is forced true to drive the wrapper's force-exit on this path. + isLbugReadyMock.mockReturnValue(true); + runFullAnalysisMock.mockResolvedValue({ + repoName: 'repo', + repoPath: '/repo', + stats: {}, + alreadyUpToDate: true, + ftsRepairedOnly: false, + pipelineResult: { communityResult: undefined }, + }); + assertAnalysisFinalizedMock.mockResolvedValue(undefined); + process.exitCode = 2; + await analyzeCommand(undefined, {}); + expect(exitSpy).toHaveBeenCalledWith(2); + }); +}); diff --git a/gitnexus/test/unit/analyze-gitnexusrc.test.ts b/gitnexus/test/unit/analyze-gitnexusrc.test.ts index cd299f870..fa1b3f9fb 100644 --- a/gitnexus/test/unit/analyze-gitnexusrc.test.ts +++ b/gitnexus/test/unit/analyze-gitnexusrc.test.ts @@ -39,7 +39,11 @@ vi.mock('../../src/cli/ai-context.js', () => ({ })); vi.mock('../../src/cli/skill-gen.js', () => ({ generateSkillFiles: generateSkillFilesMock })); vi.mock('../../src/cli/cli-message.js', () => ({ cliError: cliErrorMock })); -vi.mock('../../src/core/lbug/lbug-adapter.js', () => ({ closeLbug: vi.fn(async () => undefined) })); +vi.mock('../../src/core/lbug/lbug-adapter.js', () => ({ + closeLbug: vi.fn(async () => undefined), + closeLbugBeforeExit: vi.fn(async () => undefined), + isLbugReady: vi.fn(() => false), +})); vi.mock('../../src/storage/repo-manager.js', () => ({ getStoragePaths: vi.fn((repoPath: string) => ({ diff --git a/gitnexus/test/unit/analyze-heap-respawn.test.ts b/gitnexus/test/unit/analyze-heap-respawn.test.ts index 7d98ddbf4..e384e460c 100644 --- a/gitnexus/test/unit/analyze-heap-respawn.test.ts +++ b/gitnexus/test/unit/analyze-heap-respawn.test.ts @@ -25,6 +25,8 @@ vi.mock('os', async () => { vi.mock('../../src/core/lbug/lbug-adapter.js', () => ({ closeLbug: vi.fn(async () => undefined), + closeLbugBeforeExit: vi.fn(async () => undefined), + isLbugReady: vi.fn(() => false), })); const mockSpawnExit = ({ diff --git a/gitnexus/test/unit/analyze-job.test.ts b/gitnexus/test/unit/analyze-job.test.ts index 1fca6d5ce..4f40d93b9 100644 --- a/gitnexus/test/unit/analyze-job.test.ts +++ b/gitnexus/test/unit/analyze-job.test.ts @@ -128,4 +128,36 @@ describe('JobManager', () => { it('cancelJob returns false for unknown job', () => { expect(manager.cancelJob('nonexistent')).toBe(false); }); + + // #2264 P3: a job's terminal outcome is immutable, so a late worker message (a + // SIGTERM-driven `error` after `complete`, or vice versa) cannot flip it. + describe('terminal-state immutability (#2264 P3)', () => { + it('keeps complete when a later failed update arrives', () => { + const job = manager.createJob({ repoUrl: 'https://github.com/user/repo' }); + manager.updateJob(job.id, { status: 'analyzing' }); + manager.updateJob(job.id, { status: 'complete', repoName: 'repo' }); + manager.updateJob(job.id, { status: 'failed', error: 'Analysis cancelled' }); + expect(manager.getJob(job.id)!.status).toBe('complete'); + }); + + it('keeps failed when a later complete update arrives', () => { + const job = manager.createJob({ repoUrl: 'https://github.com/user/repo' }); + manager.updateJob(job.id, { status: 'analyzing' }); + manager.updateJob(job.id, { status: 'failed', error: 'Analysis cancelled' }); + manager.updateJob(job.id, { status: 'complete', repoName: 'repo' }); + const after = manager.getJob(job.id)!; + expect(after.status).toBe('failed'); + expect(after.error).toBe('Analysis cancelled'); + }); + + it('emits no further event for a post-terminal update', () => { + const job = manager.createJob({ repoUrl: 'https://github.com/user/repo' }); + const events: Array<{ phase: string }> = []; + manager.onProgress(job.id, (data) => events.push(data)); + manager.updateJob(job.id, { status: 'complete', repoName: 'repo' }); + manager.updateJob(job.id, { status: 'failed', error: 'late' }); + expect(events).toHaveLength(1); + expect(events[0].phase).toBe('complete'); + }); + }); }); diff --git a/gitnexus/test/unit/analyze-lbug-checkpoint-threshold.test.ts b/gitnexus/test/unit/analyze-lbug-checkpoint-threshold.test.ts index de55d4e71..5e7f5190c 100644 --- a/gitnexus/test/unit/analyze-lbug-checkpoint-threshold.test.ts +++ b/gitnexus/test/unit/analyze-lbug-checkpoint-threshold.test.ts @@ -8,6 +8,8 @@ vi.mock('../../src/core/run-analyze.js', () => ({ vi.mock('../../src/core/lbug/lbug-adapter.js', () => ({ closeLbug: vi.fn(async () => undefined), + closeLbugBeforeExit: vi.fn(async () => undefined), + isLbugReady: vi.fn(() => false), })); vi.mock('../../src/storage/repo-manager.js', () => ({ diff --git a/gitnexus/test/unit/analyze-local-embedding-error.test.ts b/gitnexus/test/unit/analyze-local-embedding-error.test.ts index 0b5e5de48..c90896be2 100644 --- a/gitnexus/test/unit/analyze-local-embedding-error.test.ts +++ b/gitnexus/test/unit/analyze-local-embedding-error.test.ts @@ -27,6 +27,8 @@ vi.mock('../../src/core/run-analyze.js', () => ({ vi.mock('../../src/core/lbug/lbug-adapter.js', () => ({ closeLbug: vi.fn(async () => undefined), + closeLbugBeforeExit: vi.fn(async () => undefined), + isLbugReady: vi.fn(() => false), })); vi.mock('../../src/storage/repo-manager.js', () => ({ diff --git a/gitnexus/test/unit/analyze-no-stats-bridge.test.ts b/gitnexus/test/unit/analyze-no-stats-bridge.test.ts index 629bfac02..e08098ff6 100644 --- a/gitnexus/test/unit/analyze-no-stats-bridge.test.ts +++ b/gitnexus/test/unit/analyze-no-stats-bridge.test.ts @@ -35,6 +35,8 @@ vi.mock('../../src/cli/cli-message.js', () => ({ vi.mock('../../src/core/lbug/lbug-adapter.js', () => ({ closeLbug: vi.fn(async () => undefined), + closeLbugBeforeExit: vi.fn(async () => undefined), + isLbugReady: vi.fn(() => false), })); vi.mock('../../src/storage/repo-manager.js', () => ({ diff --git a/gitnexus/test/unit/analyze-respawn-progress-terminal.test.ts b/gitnexus/test/unit/analyze-respawn-progress-terminal.test.ts index de90ae467..a5bfb16ee 100644 --- a/gitnexus/test/unit/analyze-respawn-progress-terminal.test.ts +++ b/gitnexus/test/unit/analyze-respawn-progress-terminal.test.ts @@ -40,6 +40,8 @@ vi.mock('../../src/core/run-analyze.js', () => ({ vi.mock('../../src/core/lbug/lbug-adapter.js', () => ({ closeLbug: vi.fn(async () => undefined), + closeLbugBeforeExit: vi.fn(async () => undefined), + isLbugReady: vi.fn(() => false), })); vi.mock('../../src/storage/repo-manager.js', () => ({ diff --git a/gitnexus/test/unit/analyze-wal-error.test.ts b/gitnexus/test/unit/analyze-wal-error.test.ts index 5264dfd3a..0cc768043 100644 --- a/gitnexus/test/unit/analyze-wal-error.test.ts +++ b/gitnexus/test/unit/analyze-wal-error.test.ts @@ -20,6 +20,8 @@ vi.mock('../../src/core/run-analyze.js', () => ({ vi.mock('../../src/core/lbug/lbug-adapter.js', () => ({ closeLbug: vi.fn(async () => undefined), + closeLbugBeforeExit: vi.fn(async () => undefined), + isLbugReady: vi.fn(() => false), })); vi.mock('../../src/storage/repo-manager.js', () => ({ diff --git a/gitnexus/test/unit/analyze-worker-core.test.ts b/gitnexus/test/unit/analyze-worker-core.test.ts new file mode 100644 index 000000000..a5a3f20d9 --- /dev/null +++ b/gitnexus/test/unit/analyze-worker-core.test.ts @@ -0,0 +1,143 @@ +/** + * Unit tests for the analyze-worker core seam (#2264). + * + * P2: the worker must NOT report `complete` for a half-finalized repo (meta.json + * written but the global registry entry missing) — it must surface that as an + * error, mirroring the CLI's assertAnalysisFinalized guard. + * + * P3: a SIGTERM cancellation and a near-simultaneous completion must not both + * report a terminal outcome — the `claimTerminal` slot coordinates them. + * + * Driven via the side-effect-free `runWorkerAnalysis` seam with injected fakes, so + * no fork()/process.on side effects of the entry module are touched. + */ +import { describe, it, expect, vi } from 'vitest'; +import { + runWorkerAnalysis, + createTerminalClaim, + type WorkerAnalysisDeps, +} from '../../src/server/analyze-worker-core.js'; +import type { AnalyzeResult } from '../../src/core/run-analyze.js'; +import type { WorkerMessage } from '../../src/server/analyze-worker.js'; + +const baseResult: AnalyzeResult = { + repoName: 'repo', + repoPath: '/repo', + stats: {}, + alreadyUpToDate: false, + ftsRepairedOnly: false, +}; + +const okRun: WorkerAnalysisDeps['runFullAnalysis'] = vi.fn(async () => baseResult); +const okFinalize: WorkerAnalysisDeps['assertAnalysisFinalized'] = vi.fn(async () => undefined); +const alwaysClaim: WorkerAnalysisDeps['claimTerminal'] = () => true; + +describe('runWorkerAnalysis — finalize guard (#2264 P2)', () => { + it('reports error (not complete) when finalization fails for an unregistered repo', async () => { + const send = vi.fn<(msg: WorkerMessage) => void>(); + const assertAnalysisFinalized: WorkerAnalysisDeps['assertAnalysisFinalized'] = vi.fn( + async () => { + throw new Error('registry entry for /repo was not added'); + }, + ); + + await runWorkerAnalysis( + '/repo', + {}, + { + runFullAnalysis: okRun, + assertAnalysisFinalized, + send, + claimTerminal: alwaysClaim, + }, + ); + + expect(send).toHaveBeenCalledWith({ + type: 'error', + message: 'registry entry for /repo was not added', + }); + expect(send).not.toHaveBeenCalledWith(expect.objectContaining({ type: 'complete' })); + }); + + it('reports complete exactly once when finalization succeeds', async () => { + const send = vi.fn<(msg: WorkerMessage) => void>(); + + await runWorkerAnalysis( + '/repo', + {}, + { + runFullAnalysis: okRun, + assertAnalysisFinalized: okFinalize, + send, + claimTerminal: alwaysClaim, + }, + ); + + const completes = send.mock.calls.filter((c) => c[0].type === 'complete'); + expect(completes).toHaveLength(1); + }); + + it('reports error when finalization passes but the analysis itself throws', async () => { + const send = vi.fn<(msg: WorkerMessage) => void>(); + const failingRun: WorkerAnalysisDeps['runFullAnalysis'] = vi.fn(async () => { + throw new Error('boom'); + }); + // Fresh local mock (not the shared okFinalize) so the "never called" assertion + // reflects only this test. + const finalize = vi.fn(async () => undefined); + + await runWorkerAnalysis( + '/repo', + {}, + { + runFullAnalysis: failingRun, + assertAnalysisFinalized: finalize, + send, + claimTerminal: alwaysClaim, + }, + ); + + expect(send).toHaveBeenCalledWith({ type: 'error', message: 'boom' }); + expect(finalize).not.toHaveBeenCalled(); + }); +}); + +describe('runWorkerAnalysis — terminal-claim coordination (#2264 P3)', () => { + it('sends NO terminal message when the slot is already claimed (cancellation won)', async () => { + const send = vi.fn<(msg: WorkerMessage) => void>(); + const alreadyClaimed: WorkerAnalysisDeps['claimTerminal'] = () => false; + + await runWorkerAnalysis( + '/repo', + {}, + { + runFullAnalysis: okRun, + assertAnalysisFinalized: okFinalize, + send, + claimTerminal: alreadyClaimed, + }, + ); + + const terminals = send.mock.calls.filter( + (c) => c[0].type === 'complete' || c[0].type === 'error', + ); + expect(terminals).toHaveLength(0); + }); +}); + +describe('createTerminalClaim (#2264 P3)', () => { + it('returns true for the first claim and false for every claim after', () => { + const claim = createTerminalClaim(); + expect(claim()).toBe(true); + expect(claim()).toBe(false); + expect(claim()).toBe(false); + }); + + it('gives independent claims separate slots', () => { + const a = createTerminalClaim(); + const b = createTerminalClaim(); + expect(a()).toBe(true); + expect(b()).toBe(true); + expect(a()).toBe(false); + }); +}); diff --git a/gitnexus/test/unit/analyze-worker-pool-size.test.ts b/gitnexus/test/unit/analyze-worker-pool-size.test.ts index 2c7a8bd3f..8af0f213f 100644 --- a/gitnexus/test/unit/analyze-worker-pool-size.test.ts +++ b/gitnexus/test/unit/analyze-worker-pool-size.test.ts @@ -8,6 +8,8 @@ vi.mock('../../src/core/run-analyze.js', () => ({ vi.mock('../../src/core/lbug/lbug-adapter.js', () => ({ closeLbug: vi.fn(async () => undefined), + closeLbugBeforeExit: vi.fn(async () => undefined), + isLbugReady: vi.fn(() => false), })); vi.mock('../../src/storage/repo-manager.js', () => ({ diff --git a/gitnexus/test/unit/analyze-worker-timeout.test.ts b/gitnexus/test/unit/analyze-worker-timeout.test.ts index aebe587e9..8a2dc04ef 100644 --- a/gitnexus/test/unit/analyze-worker-timeout.test.ts +++ b/gitnexus/test/unit/analyze-worker-timeout.test.ts @@ -8,6 +8,8 @@ vi.mock('../../src/core/run-analyze.js', () => ({ vi.mock('../../src/core/lbug/lbug-adapter.js', () => ({ closeLbug: vi.fn(async () => undefined), + closeLbugBeforeExit: vi.fn(async () => undefined), + isLbugReady: vi.fn(() => false), })); vi.mock('../../src/storage/repo-manager.js', () => ({ diff --git a/gitnexus/test/unit/conn-lock.test.ts b/gitnexus/test/unit/conn-lock.test.ts new file mode 100644 index 000000000..f3d2e588b --- /dev/null +++ b/gitnexus/test/unit/conn-lock.test.ts @@ -0,0 +1,109 @@ +/** + * Unit tests for the LadybugDB connection serialization lock (conn-lock.ts). + * + * This lock is the fix for the `analyze --pdg` native crash: the WAL-checkpoint + * driver's periodic CHECKPOINT was executing on the shared singleton connection + * concurrently with a long-running COPY, and LadybugDB's single-writer + * Connection corrupts native heap state under concurrent query execution + * (`double free or corruption (out)` / SIGSEGV). These tests assert the + * lock's one-at-a-time guarantee deterministically, with no native engine — + * the property that makes the otherwise-crashing overlap safe. + */ +import { afterEach, describe, expect, it } from 'vitest'; +import { withConnLock, _resetConnLockForTests } from '../../src/core/lbug/conn-lock.js'; + +afterEach(() => { + _resetConnLockForTests(); +}); + +describe('withConnLock — connection serialization', () => { + it('runs critical sections one at a time in FIFO order with no interleave', async () => { + const events: string[] = []; + const section = (id: string, yields: number) => async (): Promise => { + events.push(`${id}:enter`); + // Yield to the microtask queue repeatedly. Without serialization a later + // section's `enter` would slip in between these yields. + for (let i = 0; i < yields; i++) await Promise.resolve(); + events.push(`${id}:exit`); + }; + + // B and C are launched while A (which yields the most) is mid-flight. + await Promise.all([ + withConnLock(section('A', 5)), + withConnLock(section('B', 0)), + withConnLock(section('C', 0)), + ]); + + expect(events).toEqual(['A:enter', 'A:exit', 'B:enter', 'B:exit', 'C:enter', 'C:exit']); + }); + + it('never lets two critical sections overlap under heavy concurrency', async () => { + let active = 0; + const observedMax: number[] = []; + const op = () => async (): Promise => { + active++; + observedMax.push(active); + await Promise.resolve(); + await Promise.resolve(); + active--; + }; + + await Promise.all(Array.from({ length: 25 }, () => withConnLock(op()))); + + // The concurrency count observed at the top of every critical section was + // always exactly 1 — i.e. no two ran at once. This is precisely what stops + // the checkpoint driver from racing a COPY on the native connection. + expect(Math.max(...observedMax)).toBe(1); + }); + + it('releases the lock when a critical section throws (no permanent wedge)', async () => { + await expect( + withConnLock(async () => { + throw new Error('boom'); + }), + ).rejects.toThrow('boom'); + + // A failed op must not strand the lock — the next caller still acquires it. + await expect(withConnLock(async () => 'recovered')).resolves.toBe('recovered'); + }); + + it('returns the wrapped operation result', async () => { + await expect(withConnLock(async () => 42)).resolves.toBe(42); + }); +}); + +describe('withConnLock — re-entry guard', () => { + it('throws on a nested (wrapped-in-wrapped) call instead of deadlocking', async () => { + await expect(withConnLock(async () => withConnLock(async () => 'inner'))).rejects.toThrow( + /re-entry/, + ); + }); + + it('does NOT false-fire on sequential (non-nested) calls', async () => { + // Mirrors getLbugStats: many withConnLock calls in a loop, each awaited to + // completion before the next — distinct async contexts, never nested. + const results: number[] = []; + for (let i = 0; i < 5; i++) { + results.push(await withConnLock(async () => i)); + } + expect(results).toEqual([0, 1, 2, 3, 4]); + }); + + it('does NOT false-fire on concurrent top-level (queued) callers', async () => { + // Legitimate contention: B and C call while A holds the lock. They are + // separate async contexts (not nested in A's fn), so they queue, not throw. + const out = await Promise.all([ + withConnLock(async () => 'a'), + withConnLock(async () => 'b'), + withConnLock(async () => 'c'), + ]); + expect(out).toEqual(['a', 'b', 'c']); + }); + + it('releases the lock after a re-entry throw so later callers proceed', async () => { + await expect(withConnLock(async () => withConnLock(async () => 'inner'))).rejects.toThrow( + /re-entry/, + ); + await expect(withConnLock(async () => 'ok')).resolves.toBe('ok'); + }); +}); diff --git a/gitnexus/test/unit/group/http-route-extractor.test.ts b/gitnexus/test/unit/group/http-route-extractor.test.ts index 52e6f8a49..2d688c7e8 100644 --- a/gitnexus/test/unit/group/http-route-extractor.test.ts +++ b/gitnexus/test/unit/group/http-route-extractor.test.ts @@ -950,6 +950,8 @@ public class UserController implements UserApi { expect(toPosixPath(usersRoute!.symbolRef.filePath)).toBe( 'src/controller/UserController.java', ); + expect(usersRoute!.meta.framework).toBe('spring'); + expect(usersRoute!.confidence).toBe(0.8); }); it('does not duplicate inherited Spring prefixes already present on the controller', async () => { @@ -1156,6 +1158,37 @@ public class StatusController implements StatusApi { ).toHaveLength(0); }); + it('does not extract fully-qualified Java route annotations (documented limitation #2254)', async () => { + // JAVA_ROUTE_ANNOTATION_PATTERNS binds `name: (identifier)`; a FQN route + // annotation parses its name as `scoped_identifier` and is not matched, so + // its route is not extracted (only the route string — the controller itself + // is still recognised). This pins that documented limitation / asymmetry + // with Kotlin. If FQN matching is ever added, flip this assertion. + const dir = path.join(tmpDir, 'java-fqn-route-annotation'); + fs.mkdirSync(path.join(dir, 'src'), { recursive: true }); + fs.writeFileSync( + path.join(dir, 'src', 'FqnController.java'), + ` +@org.springframework.web.bind.annotation.RestController +@org.springframework.web.bind.annotation.RequestMapping("/api") +class FqnController { + @org.springframework.web.bind.annotation.GetMapping("/users") + Object users() { return null; } +} +`, + ); + + const contracts = await extractor.extract(null, dir, makeRepo(dir)); + const providers = contracts.filter((c) => c.role === 'provider'); + + // The FQN route annotation is not extracted (documented limitation). + expect(providers.find((c) => c.contractId === 'http::GET::/api/users')).toBeUndefined(); + // And it must not over-match into a bogus contract either. + expect( + providers.filter((c) => c.symbolRef.filePath.endsWith('FqnController.java')), + ).toHaveLength(0); + }); + it('extracts Express router.get patterns', async () => { const dir = path.join(tmpDir, 'express'); fs.mkdirSync(path.join(dir, 'src/routes'), { recursive: true }); @@ -1738,7 +1771,10 @@ class ApiClient { ).toBeDefined(); }); - it('does NOT match Java WebClient long-form method(HttpMethod).uri(...) yet', async () => { + it('extracts Java WebClient long-form method(HttpMethod.X).uri(...) — #2254 parity', async () => { + // Parity with the Kotlin plugin: a single structural query matches the + // verb (HttpMethod.X field access) and path. Previously deferred on the + // Java side; PR #2254 lifts it so .java and .kt detect it identically. const dir = path.join(tmpDir, 'java-web-client-long-form'); fs.mkdirSync(path.join(dir, 'src'), { recursive: true }); fs.writeFileSync( @@ -1749,6 +1785,10 @@ import org.springframework.web.reactive.function.client.WebClient; class LongFormClient { void run(WebClient webClient) { + webClient.method(HttpMethod.GET).uri("/api/get").retrieve(); + webClient.method(HttpMethod.POST).uri("/api/post").retrieve(); + webClient.method(HttpMethod.PUT).uri("/api/put").retrieve(); + webClient.method(HttpMethod.DELETE).uri("/api/delete").retrieve(); webClient.method(HttpMethod.PATCH).uri("/api/users/42").retrieve(); } } @@ -1758,11 +1798,105 @@ class LongFormClient { const contracts = await extractor.extract(null, dir, makeRepo(dir)); const consumers = contracts.filter((c) => c.role === 'consumer'); + for (const [verb, p] of [ + ['GET', '/api/get'], + ['POST', '/api/post'], + ['PUT', '/api/put'], + ['DELETE', '/api/delete'], + ['PATCH', '/api/users/{param}'], + ]) { + expect( + consumers.find( + (c) => + c.contractId === `http::${verb}::${p}` && + c.meta.framework === 'spring-web-client' && + c.confidence === 0.7, + ), + ).toBeDefined(); + } + // No double-emit: the short-form query cannot also fire on the long form. + expect(consumers.filter((c) => c.contractId === 'http::GET::/api/get')).toHaveLength(1); + }); + + it('does NOT match Java WebClient long-form with a variable-bound verb', async () => { + // The value carries a bare identifier, not a HttpMethod.X field access — + // source-scan can't follow the binding (anti-overreach, parity with Kotlin). + const dir = path.join(tmpDir, 'java-web-client-long-form-var'); + fs.mkdirSync(path.join(dir, 'src'), { recursive: true }); + fs.writeFileSync( + path.join(dir, 'src', 'VarVerbClient.java'), + ` +import org.springframework.http.HttpMethod; +import org.springframework.web.reactive.function.client.WebClient; + +class VarVerbClient { + void run(WebClient webClient, HttpMethod verb) { + webClient.method(verb).uri("/api/users/42").retrieve(); + } +} +`, + ); + + const contracts = await extractor.extract(null, dir, makeRepo(dir)); + const consumers = contracts.filter((c) => c.role === 'consumer'); + expect( - consumers.find((c) => c.contractId === 'http::PATCH::/api/users/{param}'), + consumers.find( + (c) => c.contractId.startsWith('http::') && c.contractId.includes('/api/users'), + ), ).toBeUndefined(); }); + it('handles Java array-form annotation paths ({"/x"}, key = {"/x"})', async () => { + const dir = path.join(tmpDir, 'java-array-paths'); + fs.mkdirSync(path.join(dir, 'src'), { recursive: true }); + // Provider: array class prefix + array method path. + fs.writeFileSync( + path.join(dir, 'src', 'ProductController.java'), + ` +import org.springframework.web.bind.annotation.RestController; +import org.springframework.web.bind.annotation.RequestMapping; +import org.springframework.web.bind.annotation.GetMapping; + +@RestController +@RequestMapping({"/api/products"}) +class ProductController { + @GetMapping(path = {"/{id}"}) + Product get(Integer id) { return null; } +} +`, + ); + // OpenFeign consumer with array positional path. + fs.writeFileSync( + path.join(dir, 'src', 'OrdersClient.java'), + ` +import org.springframework.cloud.openfeign.FeignClient; +import org.springframework.web.bind.annotation.PostMapping; + +@FeignClient(name = "orders") +interface OrdersClient { + @PostMapping({"/orders/search"}) + Object search(); +} +`, + ); + + const contracts = await extractor.extract(null, dir, makeRepo(dir)); + const providers = contracts.filter((c) => c.role === 'provider'); + const consumers = contracts.filter((c) => c.role === 'consumer'); + + // Array class prefix + array method path → provider. + expect( + providers.find((c) => c.contractId === 'http::GET::/api/products/{param}'), + ).toBeDefined(); + // @FeignClient with array positional path → consumer. + expect( + consumers.find( + (c) => c.contractId === 'http::POST::/orders/search' && c.meta.framework === 'openfeign', + ), + ).toBeDefined(); + }); + it('extracts OpenFeign clients as consumers, not providers', async () => { const dir = path.join(tmpDir, 'java-openfeign-consumer'); fs.mkdirSync(path.join(dir, 'src'), { recursive: true }); @@ -1805,6 +1939,46 @@ interface OrderClient { ).toBeUndefined(); }); + it('extracts Spring HTTP Interface @(Get|...)Exchange clients as consumers', async () => { + const dir = path.join(tmpDir, 'java-http-exchange-consumer'); + fs.mkdirSync(path.join(dir, 'src'), { recursive: true }); + fs.writeFileSync( + path.join(dir, 'src', 'InventoryApi.java'), + ` +import org.springframework.web.service.annotation.HttpExchange; +import org.springframework.web.service.annotation.GetExchange; +import org.springframework.web.service.annotation.PostExchange; + +@HttpExchange(url = "/items") +interface InventoryApi { + @GetExchange(url = "/{id}") + Item getItem(Integer id); + + @PostExchange("/search") + Page search(ItemFilter query); +} +`, + ); + + const contracts = await extractor.extract(null, dir, makeRepo(dir)); + const consumers = contracts.filter((c) => c.role === 'consumer'); + const providers = contracts.filter((c) => c.role === 'provider'); + + expect( + consumers.find( + (c) => + c.contractId === 'http::GET::/items/{param}' && + c.meta.framework === 'spring-http-interface' && + c.confidence === 0.75, + ), + ).toBeDefined(); + expect(consumers.find((c) => c.contractId === 'http::POST::/items/search')).toBeDefined(); + // Declarative HTTP-interface methods are consumers, never providers. + expect( + providers.find((c) => c.symbolRef.filePath.endsWith('InventoryApi.java')), + ).toBeUndefined(); + }); + it('extracts OpenFeign clients without an interface path prefix', async () => { const dir = path.join(tmpDir, 'java-openfeign-no-prefix'); fs.mkdirSync(path.join(dir, 'src'), { recursive: true }); @@ -2251,6 +2425,64 @@ interface ReversedPrecedenceClient { expect(consumers.find((c) => c.contractId === 'http::GET::/rm-path/orders')).toBeUndefined(); }); + it('a @FeignClient API interface implemented by a controller yields both a consumer and a provider (Java)', async () => { + // Java twin of the Kotlin dual-role case: an `api` module publishes a + // @FeignClient contract (consumer) that the service's @RestController + // implements (provider). + const dir = path.join(tmpDir, 'java-feign-api-implemented'); + fs.mkdirSync(path.join(dir, 'src/rest'), { recursive: true }); + fs.mkdirSync(path.join(dir, 'src/controller'), { recursive: true }); + fs.writeFileSync( + path.join(dir, 'src/rest/WarehouseApi.java'), + ` +package com.example.rest; +import org.springframework.cloud.openfeign.FeignClient; +import org.springframework.web.bind.annotation.*; + +@FeignClient(name = "catalog-service") +@RequestMapping("/warehouses") +public interface WarehouseApi { + @GetMapping("/{id}/stock") + Object listStock(); +} +`, + ); + fs.writeFileSync( + path.join(dir, 'src/controller/WarehouseController.java'), + ` +package com.example.controller; +import com.example.rest.WarehouseApi; +import org.springframework.web.bind.annotation.*; + +@RestController +public class WarehouseController implements WarehouseApi { + @Override + public Object listStock() { return null; } +} +`, + ); + + const contracts = await extractor.extract(null, dir, makeRepo(dir)); + const providers = contracts.filter((c) => c.role === 'provider'); + const consumers = contracts.filter((c) => c.role === 'consumer'); + + expect( + consumers.find( + (c) => + c.contractId === 'http::GET::/warehouses/{param}/stock' && + c.meta.framework === 'openfeign', + ), + ).toBeDefined(); + expect( + providers.find( + (c) => + c.contractId === 'http::GET::/warehouses/{param}/stock' && + c.symbolRef.filePath.endsWith('WarehouseController.java') && + c.confidence === 0.8, + ), + ).toBeDefined(); + }); + it('extracts Java and Apache HttpClient literal request construction', async () => { const dir = path.join(tmpDir, 'java-http-client-consumer'); fs.mkdirSync(path.join(dir, 'src'), { recursive: true }); @@ -2688,6 +2920,1903 @@ class CacheClient(private val cacheClient: SomeCache) { }, ); + // ─── Kotlin OpenFeign + Spring HTTP Interface consumers ────────────── + // `@FeignClient` interfaces (Spring MVC `@*Mapping` methods) and Spring 6 + // declarative HTTP Interfaces (`@(Get|...)Exchange`) are the dominant + // outbound-call patterns in Kotlin+Spring services. In tree-sitter-kotlin + // an `interface` is a `class_declaration`, so without a `@FeignClient` + // gate the `@*Mapping` methods would mis-classify as providers. + itKotlinConsumer( + 'extracts Kotlin @FeignClient methods as consumers, not providers', + async () => { + const dir = path.join(tmpDir, 'kotlin-feign-consumer'); + fs.mkdirSync(path.join(dir, 'src'), { recursive: true }); + fs.writeFileSync( + path.join(dir, 'src', 'InventoryClient.kt'), + `package com.example +import org.springframework.cloud.openfeign.FeignClient +import org.springframework.web.bind.annotation.GetMapping +import org.springframework.web.bind.annotation.PostMapping + +@FeignClient(name = "inventory-service", configuration = [InventoryFeignClientConfig::class]) +interface InventoryClient { + @GetMapping("items/{itemId}", consumes = [MediaType.APPLICATION_JSON_VALUE], produces = [MediaType.APPLICATION_JSON_VALUE]) + fun getItem(@PathVariable("itemId") itemId: Int): ItemDto + + @PostMapping("items/search", consumes = [MediaType.APPLICATION_JSON_VALUE]) + fun getItems(@RequestBody query: ItemFilter): Page +} +`, + ); + + const contracts = await extractor.extract(null, dir, makeRepo(dir)); + const consumers = contracts.filter((c) => c.role === 'consumer'); + const providers = contracts.filter((c) => c.role === 'provider'); + + expect( + consumers.find( + (c) => + c.contractId === 'http::GET::/items/{param}' && + c.meta.framework === 'openfeign' && + c.confidence === 0.7, + ), + ).toBeDefined(); + expect(consumers.find((c) => c.contractId === 'http::POST::/items/search')).toBeDefined(); + // The Feign interface methods must NOT leak into providers. + expect( + providers.find((c) => c.symbolRef.filePath.endsWith('InventoryClient.kt')), + ).toBeUndefined(); + }, + ); + + itKotlinConsumer('applies @FeignClient(path) and @RequestMapping prefixes', async () => { + // One interface per file — the real-world layout (e.g. InventoryClient.kt). + const dir = path.join(tmpDir, 'kotlin-feign-prefix'); + fs.mkdirSync(path.join(dir, 'src'), { recursive: true }); + fs.writeFileSync( + path.join(dir, 'src', 'PrecedenceClient.kt'), + `package com.example +import org.springframework.cloud.openfeign.FeignClient +import org.springframework.web.bind.annotation.GetMapping +import org.springframework.web.bind.annotation.RequestMapping + +@FeignClient(name = "a", path = "/feign-path") +@RequestMapping("/rm-path") +interface PrecedenceClient { + @GetMapping("/orders") + fun getOrders(): Any +} +`, + ); + fs.writeFileSync( + path.join(dir, 'src', 'InventoryClient.kt'), + `package com.example +import org.springframework.cloud.openfeign.FeignClient +import org.springframework.web.bind.annotation.GetMapping +import org.springframework.web.bind.annotation.RequestMapping + +@FeignClient(name = "b") +@RequestMapping(path = "/api") +interface InventoryClient { + @GetMapping("/inventory/{id}") + fun getInventory(id: String): Any +} +`, + ); + + const contracts = await extractor.extract(null, dir, makeRepo(dir)); + const consumers = contracts.filter((c) => c.role === 'consumer'); + + // @FeignClient(path) wins over @RequestMapping. + expect(consumers.find((c) => c.contractId === 'http::GET::/feign-path/orders')).toBeDefined(); + expect(consumers.find((c) => c.contractId === 'http::GET::/rm-path/orders')).toBeUndefined(); + // @RequestMapping is the fallback prefix when there is no @FeignClient(path). + expect( + consumers.find((c) => c.contractId === 'http::GET::/api/inventory/{param}'), + ).toBeDefined(); + }); + + itKotlinConsumer( + 'extracts Kotlin Spring HTTP Interface @(Get|...)Exchange consumers', + async () => { + const dir = path.join(tmpDir, 'kotlin-http-exchange'); + fs.mkdirSync(path.join(dir, 'src'), { recursive: true }); + fs.writeFileSync( + path.join(dir, 'src', 'InventoryApi.kt'), + `package com.example +import org.springframework.web.service.annotation.GetExchange +import org.springframework.web.service.annotation.PostExchange +import org.springframework.web.service.annotation.PutExchange +import org.springframework.web.service.annotation.PatchExchange +import org.springframework.web.service.annotation.DeleteExchange + +interface InventoryApi { + @GetExchange(url = "/items/{itemId}", accept = [MediaType.APPLICATION_JSON_VALUE]) + fun obtainItem(@PathVariable itemId: Int): Any + + @PostExchange(url = "/items/search") + fun search(): Any + + @PutExchange(url = "/items") + fun create(): Any + + @PatchExchange(url = "/items/update/{itemId}") + fun update(@PathVariable itemId: Int): Any + + @DeleteExchange(url = "/items/{itemId}") + fun remove(@PathVariable itemId: Int): Any +} +`, + ); + + const contracts = await extractor.extract(null, dir, makeRepo(dir)); + const consumers = contracts.filter((c) => c.role === 'consumer'); + const providers = contracts.filter((c) => c.role === 'provider'); + + expect( + consumers.find( + (c) => + c.contractId === 'http::GET::/items/{param}' && + c.meta.framework === 'spring-http-interface' && + c.confidence === 0.75, + ), + ).toBeDefined(); + expect(consumers.find((c) => c.contractId === 'http::POST::/items/search')).toBeDefined(); + expect(consumers.find((c) => c.contractId === 'http::PUT::/items')).toBeDefined(); + expect( + consumers.find((c) => c.contractId === 'http::PATCH::/items/update/{param}'), + ).toBeDefined(); + expect( + consumers.find((c) => c.contractId === 'http::DELETE::/items/{param}'), + ).toBeDefined(); + // Declarative HTTP-interface methods are consumers, never providers. + expect( + providers.find((c) => c.symbolRef.filePath.endsWith('InventoryApi.kt')), + ).toBeUndefined(); + }, + ); + + itKotlinConsumer( + 'applies class-level @HttpExchange(url) prefix and a positional @GetExchange', + async () => { + const dir = path.join(tmpDir, 'kotlin-http-exchange-prefix'); + fs.mkdirSync(path.join(dir, 'src'), { recursive: true }); + fs.writeFileSync( + path.join(dir, 'src', 'ProductApi.kt'), + `package com.example +import org.springframework.web.service.annotation.HttpExchange +import org.springframework.web.service.annotation.GetExchange + +@HttpExchange(url = "/products") +interface ProductApi { + @GetExchange("/{id}") + fun get(@PathVariable id: Int): Any +} +`, + ); + + const contracts = await extractor.extract(null, dir, makeRepo(dir)); + const consumers = contracts.filter((c) => c.role === 'consumer'); + + expect( + consumers.find( + (c) => + c.contractId === 'http::GET::/products/{param}' && + c.meta.framework === 'spring-http-interface', + ), + ).toBeDefined(); + }, + ); + + itKotlinConsumer('extracts Kotlin OpenFeign native @RequestLine consumers', async () => { + const dir = path.join(tmpDir, 'kotlin-feign-request-line'); + fs.mkdirSync(path.join(dir, 'src'), { recursive: true }); + fs.writeFileSync( + path.join(dir, 'src', 'AiClient.kt'), + `package com.example +import org.springframework.cloud.openfeign.FeignClient +import feign.RequestLine + +@FeignClient(name = "ai-backend") +interface AiClient { + @RequestLine("POST /ai/summarize") + fun summarize(): String + + @RequestLine("GET /ai/health") + fun health(): String +} +`, + ); + + const contracts = await extractor.extract(null, dir, makeRepo(dir)); + const consumers = contracts.filter((c) => c.role === 'consumer'); + + expect( + consumers.find( + (c) => + c.contractId === 'http::POST::/ai/summarize' && + c.meta.framework === 'openfeign' && + c.confidence === 0.75, + ), + ).toBeDefined(); + expect(consumers.find((c) => c.contractId === 'http::GET::/ai/health')).toBeDefined(); + }); + + itKotlinConsumer( + 'applies the @RequestMapping interface prefix to @RequestLine consumers (no @FeignClient path) — #2254 P2 parity', + async () => { + // Parity with java.ts, which merges the @RequestMapping prefix into + // feignPrefixByInterfaceId: an interface with @RequestMapping("/orders") + // and a @RequestLine method (no @FeignClient(path)) must apply the prefix. + // Kotlin previously dropped it (PR #2254 tri-review, kotlin.ts:978). + const dir = path.join(tmpDir, 'kotlin-request-line-rm-prefix'); + fs.mkdirSync(path.join(dir, 'src'), { recursive: true }); + fs.writeFileSync( + path.join(dir, 'src', 'OrderClient.kt'), + `package com.example +import org.springframework.cloud.openfeign.FeignClient +import org.springframework.web.bind.annotation.RequestMapping +import feign.RequestLine + +@FeignClient(name = "order-service") +@RequestMapping("/orders") +interface OrderClient { + @RequestLine("GET /{id}") + fun get(id: String): Any +} +`, + ); + + const contracts = await extractor.extract(null, dir, makeRepo(dir)); + const consumers = contracts.filter((c) => c.role === 'consumer'); + + expect( + consumers.find( + (c) => + c.contractId === 'http::GET::/orders/{param}' && + c.meta.framework === 'openfeign' && + c.confidence === 0.75, + ), + ).toBeDefined(); + // The un-prefixed form must NOT be emitted (the prefix was applied). + expect(consumers.find((c) => c.contractId === 'http::GET::/{param}')).toBeUndefined(); + }, + ); + + itKotlinConsumer( + 'prefers @FeignClient(path) over @RequestMapping for @RequestLine consumers', + async () => { + const dir = path.join(tmpDir, 'kotlin-request-line-feign-path-wins'); + fs.mkdirSync(path.join(dir, 'src'), { recursive: true }); + fs.writeFileSync( + path.join(dir, 'src', 'OrderClient.kt'), + `package com.example +import org.springframework.cloud.openfeign.FeignClient +import org.springframework.web.bind.annotation.RequestMapping +import feign.RequestLine + +@FeignClient(name = "order-service", path = "/feign-path") +@RequestMapping("/rm-path") +interface OrderClient { + @RequestLine("GET /orders/{id}") + fun get(id: String): Any +} +`, + ); + + const contracts = await extractor.extract(null, dir, makeRepo(dir)); + const consumers = contracts.filter((c) => c.role === 'consumer'); + + // @FeignClient(path) wins over @RequestMapping (parity with the @GetMapping path). + expect( + consumers.find((c) => c.contractId === 'http::GET::/feign-path/orders/{param}'), + ).toBeDefined(); + expect( + consumers.find((c) => c.contractId === 'http::GET::/rm-path/orders/{param}'), + ).toBeUndefined(); + }, + ); + + itKotlinConsumer( + 'extracts Kotlin @RequestLine written with the named "value" argument (#2254 P2)', + async () => { + const dir = path.join(tmpDir, 'kotlin-request-line-named-value'); + fs.mkdirSync(path.join(dir, 'src'), { recursive: true }); + fs.writeFileSync( + path.join(dir, 'src', 'AiClient.kt'), + `package com.example +import org.springframework.cloud.openfeign.FeignClient +import feign.RequestLine + +@FeignClient(name = "ai-backend") +interface AiClient { + @RequestLine(value = "POST /create") + fun create(): String +} +`, + ); + + const contracts = await extractor.extract(null, dir, makeRepo(dir)); + const consumers = contracts.filter((c) => c.role === 'consumer'); + + expect( + consumers.find( + (c) => + c.contractId === 'http::POST::/create' && + c.meta.framework === 'openfeign' && + c.confidence === 0.75, + ), + ).toBeDefined(); + }, + ); + + itKotlinConsumer( + 'ignores Kotlin @RequestLine whose named argument is not "value"', + async () => { + const dir = path.join(tmpDir, 'kotlin-request-line-non-value-key'); + fs.mkdirSync(path.join(dir, 'src'), { recursive: true }); + fs.writeFileSync( + path.join(dir, 'src', 'AiClient.kt'), + `package com.example +import org.springframework.cloud.openfeign.FeignClient +import feign.RequestLine + +@FeignClient(name = "ai-backend") +interface AiClient { + @RequestLine(name = "GET /should-not-extract") + fun nope(): String +} +`, + ); + + const contracts = await extractor.extract(null, dir, makeRepo(dir)); + const consumers = contracts.filter((c) => c.role === 'consumer'); + + expect( + consumers.find((c) => c.contractId === 'http::GET::/should-not-extract'), + ).toBeUndefined(); + }, + ); + + itKotlinConsumer( + 'strips query strings from Kotlin @RequestLine values when forming contract IDs', + async () => { + const dir = path.join(tmpDir, 'kotlin-request-line-query'); + fs.mkdirSync(path.join(dir, 'src'), { recursive: true }); + fs.writeFileSync( + path.join(dir, 'src', 'SearchClient.kt'), + `package com.example +import org.springframework.cloud.openfeign.FeignClient +import feign.RequestLine + +@FeignClient(name = "search-service") +interface SearchClient { + @RequestLine("GET /search?q={query}&limit={limit}") + fun search(): Any +} +`, + ); + + const contracts = await extractor.extract(null, dir, makeRepo(dir)); + const consumers = contracts.filter((c) => c.role === 'consumer'); + + expect(consumers.find((c) => c.contractId === 'http::GET::/search')).toBeDefined(); + expect( + consumers.find((c) => c.contractId.includes('?') || c.contractId.includes('limit')), + ).toBeUndefined(); + }, + ); + + itKotlinConsumer( + 'mixes Kotlin @RequestLine and @GetMapping methods on the same @FeignClient interface', + async () => { + const dir = path.join(tmpDir, 'kotlin-feign-mixed-annotations'); + fs.mkdirSync(path.join(dir, 'src'), { recursive: true }); + fs.writeFileSync( + path.join(dir, 'src', 'MixedClient.kt'), + `package com.example +import org.springframework.cloud.openfeign.FeignClient +import org.springframework.web.bind.annotation.GetMapping +import feign.RequestLine + +@FeignClient(name = "mixed-service", path = "/api") +interface MixedClient { + @GetMapping("/spring-style") + fun springStyle(): String + + @RequestLine("GET /native-style") + fun nativeStyle(): String +} +`, + ); + + const contracts = await extractor.extract(null, dir, makeRepo(dir)); + const consumers = contracts.filter((c) => c.role === 'consumer'); + + // @GetMapping → @FeignClient(path) prefix; confidence 0.7. + expect( + consumers.find( + (c) => + c.contractId === 'http::GET::/api/spring-style' && + c.meta.framework === 'openfeign' && + c.confidence === 0.7, + ), + ).toBeDefined(); + // @RequestLine → @FeignClient(path) prefix; confidence 0.75. + expect( + consumers.find( + (c) => + c.contractId === 'http::GET::/api/native-style' && + c.meta.framework === 'openfeign' && + c.confidence === 0.75, + ), + ).toBeDefined(); + }, + ); + + itKotlinConsumer( + 'ignores Kotlin @RequestLine values that are not a "VERB /path" line', + async () => { + const dir = path.join(tmpDir, 'kotlin-request-line-malformed'); + fs.mkdirSync(path.join(dir, 'src'), { recursive: true }); + fs.writeFileSync( + path.join(dir, 'src', 'MalformedClient.kt'), + `package com.example +import feign.RequestLine + +interface MalformedClient { + @RequestLine("not a request line at all") + fun noVerb(): String + + @RequestLine("GET relative/no/leading/slash") + fun noLeadingSlash(): String + + @RequestLine("FETCH /unknown-verb") + fun unknownVerb(): String +} +`, + ); + + const contracts = await extractor.extract(null, dir, makeRepo(dir)); + const consumers = contracts.filter((c) => c.role === 'consumer'); + + expect( + consumers.filter((c) => c.symbolRef.filePath.endsWith('MalformedClient.kt')), + ).toHaveLength(0); + }, + ); + + itKotlinConsumer( + 'prefers @FeignClient(path) over @RequestMapping when @RequestMapping appears first (Kotlin)', + async () => { + // Source-order independence twin: @FeignClient(path) wins even when + // @RequestMapping is the first annotation. + const dir = path.join(tmpDir, 'kotlin-feign-prefix-precedence-reversed'); + fs.mkdirSync(path.join(dir, 'src'), { recursive: true }); + fs.writeFileSync( + path.join(dir, 'src', 'ReversedClient.kt'), + `package com.example +import org.springframework.cloud.openfeign.FeignClient +import org.springframework.web.bind.annotation.GetMapping +import org.springframework.web.bind.annotation.RequestMapping + +@RequestMapping("/rm-path") +@FeignClient(name = "order-service", path = "/feign-path") +interface ReversedClient { + @GetMapping("/orders") + fun getOrders(): Any +} +`, + ); + + const contracts = await extractor.extract(null, dir, makeRepo(dir)); + const consumers = contracts.filter((c) => c.role === 'consumer'); + + expect( + consumers.find((c) => c.contractId === 'http::GET::/feign-path/orders'), + ).toBeDefined(); + expect( + consumers.find((c) => c.contractId === 'http::GET::/rm-path/orders'), + ).toBeUndefined(); + }, + ); + + itKotlinConsumer( + 'classifies a @RestController class as provider and a @FeignClient interface as consumer', + async () => { + const dir = path.join(tmpDir, 'kotlin-controller-vs-feign'); + fs.mkdirSync(path.join(dir, 'src'), { recursive: true }); + fs.writeFileSync( + path.join(dir, 'src', 'Mixed.kt'), + `package com.example +import org.springframework.web.bind.annotation.RestController +import org.springframework.web.bind.annotation.GetMapping +import org.springframework.cloud.openfeign.FeignClient + +@RestController +class OrdersController { + @GetMapping("/orders/{id}") + fun getOrder(@PathVariable id: Int): Any = TODO() +} + +@FeignClient(name = "pricing") +interface PricingClient { + @GetMapping("/prices/{id}") + fun getPrice(id: Int): Any +} +`, + ); + + const contracts = await extractor.extract(null, dir, makeRepo(dir)); + const providers = contracts.filter((c) => c.role === 'provider'); + const consumers = contracts.filter((c) => c.role === 'consumer'); + + // Controller method → provider (not a consumer). + expect( + providers.find( + (c) => c.contractId === 'http::GET::/orders/{param}' && c.meta.framework === 'spring', + ), + ).toBeDefined(); + expect( + consumers.find((c) => c.contractId === 'http::GET::/orders/{param}'), + ).toBeUndefined(); + // Feign interface method → consumer (not a provider). + expect( + consumers.find( + (c) => + c.contractId === 'http::GET::/prices/{param}' && c.meta.framework === 'openfeign', + ), + ).toBeDefined(); + expect( + providers.find((c) => c.contractId === 'http::GET::/prices/{param}'), + ).toBeUndefined(); + }, + ); + + itKotlinConsumer( + 'emits a provider for a class implementing a route interface (interface-based controller)', + async () => { + // One interface per file (real layout). Routes live on the interface; the + // @RestController override carries none → inherited via scanProject. + const dir = path.join(tmpDir, 'kotlin-interface-based-controller'); + fs.mkdirSync(path.join(dir, 'src'), { recursive: true }); + fs.writeFileSync( + path.join(dir, 'src', 'WarehouseApi.kt'), + `package com.example +import org.springframework.web.bind.annotation.RequestMapping +import org.springframework.web.bind.annotation.GetMapping + +@RequestMapping("/warehouses") +interface WarehouseApi { + @GetMapping("/{id}/stock") + fun listStock(id: String): Any +} +`, + ); + fs.writeFileSync( + path.join(dir, 'src', 'WarehouseController.kt'), + `package com.example +import org.springframework.web.bind.annotation.RestController + +@RestController +class WarehouseController(private val svc: Svc) : WarehouseApi { + override fun listStock(id: String): Any = TODO() +} +`, + ); + + const contracts = await extractor.extract(null, dir, makeRepo(dir)); + const providers = contracts.filter((c) => c.role === 'provider'); + + // The controller inherits the route declared on WarehouseApi → provider. + expect( + providers.find( + (c) => + c.contractId === 'http::GET::/warehouses/{param}/stock' && + c.symbolRef.filePath.endsWith('WarehouseController.kt') && + c.meta.framework === 'spring' && + c.confidence === 0.8, + ), + ).toBeDefined(); + }, + ); + + itKotlinConsumer( + 'recognises a fully-qualified @org…RestController as a controller (#2254 FQN parity)', + async () => { + // A FQN annotation parses to a user_type with one type_identifier per + // segment; the controller gate must read the trailing segment, not "org". + const dir = path.join(tmpDir, 'kotlin-fqn-controller'); + fs.mkdirSync(path.join(dir, 'src'), { recursive: true }); + fs.writeFileSync( + path.join(dir, 'src', 'WarehouseApi.kt'), + `package com.example +import org.springframework.web.bind.annotation.RequestMapping +import org.springframework.web.bind.annotation.GetMapping + +@RequestMapping("/warehouses") +interface WarehouseApi { + @GetMapping("/{id}/stock") + fun listStock(id: String): Any +} +`, + ); + fs.writeFileSync( + path.join(dir, 'src', 'WarehouseController.kt'), + `package com.example + +@org.springframework.web.bind.annotation.RestController +class WarehouseController(private val svc: Svc) : WarehouseApi { + override fun listStock(id: String): Any = TODO() +} +`, + ); + + const contracts = await extractor.extract(null, dir, makeRepo(dir)); + const providers = contracts.filter((c) => c.role === 'provider'); + + expect( + providers.find( + (c) => + c.contractId === 'http::GET::/warehouses/{param}/stock' && + c.symbolRef.filePath.endsWith('WarehouseController.kt'), + ), + ).toBeDefined(); + }, + ); + + itKotlinConsumer( + 'resolves a fully-qualified supertype to its trailing segment for interface inheritance', + async () => { + // `: com.example.WarehouseApi` must resolve to "WarehouseApi" (trailing + // segment), not "com", so the inherited interface route is matched. + const dir = path.join(tmpDir, 'kotlin-fqn-supertype'); + fs.mkdirSync(path.join(dir, 'src'), { recursive: true }); + fs.writeFileSync( + path.join(dir, 'src', 'WarehouseApi.kt'), + `package com.example +import org.springframework.web.bind.annotation.RequestMapping +import org.springframework.web.bind.annotation.GetMapping + +@RequestMapping("/warehouses") +interface WarehouseApi { + @GetMapping("/{id}/stock") + fun listStock(id: String): Any +} +`, + ); + fs.writeFileSync( + path.join(dir, 'src', 'WarehouseController.kt'), + `package com.example +import org.springframework.web.bind.annotation.RestController + +@RestController +class WarehouseController(private val svc: Svc) : com.example.WarehouseApi { + override fun listStock(id: String): Any = TODO() +} +`, + ); + + const contracts = await extractor.extract(null, dir, makeRepo(dir)); + const providers = contracts.filter((c) => c.role === 'provider'); + + expect( + providers.find( + (c) => + c.contractId === 'http::GET::/warehouses/{param}/stock' && + c.symbolRef.filePath.endsWith('WarehouseController.kt'), + ), + ).toBeDefined(); + }, + ); + + itKotlinConsumer( + 'a @FeignClient API interface implemented by a controller yields both a consumer and a provider', + async () => { + // catalog-service pattern: an `api` module publishes a @FeignClient contract that + // the service's own @RestController implements. The interface is the + // client SDK (consumer); the implementing controller is the provider. + const dir = path.join(tmpDir, 'kotlin-feign-api-implemented'); + fs.mkdirSync(path.join(dir, 'src'), { recursive: true }); + fs.writeFileSync( + path.join(dir, 'src', 'WarehouseApi.kt'), + `package com.example +import org.springframework.cloud.openfeign.FeignClient +import org.springframework.web.bind.annotation.RequestMapping +import org.springframework.web.bind.annotation.GetMapping + +@FeignClient(name = "catalog-service") +@RequestMapping("/warehouses") +interface WarehouseApi { + @GetMapping("/{id}/stock") + fun listStock(id: String): Any +} +`, + ); + fs.writeFileSync( + path.join(dir, 'src', 'WarehouseController.kt'), + `package com.example +import org.springframework.web.bind.annotation.RestController + +@RestController +class WarehouseController(private val svc: Svc) : WarehouseApi { + override fun listStock(id: String): Any = TODO() +} +`, + ); + + const contracts = await extractor.extract(null, dir, makeRepo(dir)); + const providers = contracts.filter((c) => c.role === 'provider'); + const consumers = contracts.filter((c) => c.role === 'consumer'); + + // Interface (published @FeignClient client SDK) → consumer. + expect( + consumers.find( + (c) => + c.contractId === 'http::GET::/warehouses/{param}/stock' && + c.meta.framework === 'openfeign', + ), + ).toBeDefined(); + // Implementing @RestController → provider (route inherited from the interface). + expect( + providers.find( + (c) => + c.contractId === 'http::GET::/warehouses/{param}/stock' && + c.symbolRef.filePath.endsWith('WarehouseController.kt'), + ), + ).toBeDefined(); + }, + ); + + itKotlinConsumer( + 'handles Kotlin array-form paths (["/x"], value = ["/x"]) for providers and consumers', + async () => { + // Spring path/value attributes are Array; the array literal form + // is common in Kotlin. Each route-bearing annotation must accept both a + // bare string and a single-element array (collection_literal). + const dir = path.join(tmpDir, 'kotlin-array-paths'); + fs.mkdirSync(path.join(dir, 'src'), { recursive: true }); + // Provider: array class prefix + array method path. + fs.writeFileSync( + path.join(dir, 'src', 'ProductsController.kt'), + `package com.example +import org.springframework.web.bind.annotation.RestController +import org.springframework.web.bind.annotation.RequestMapping +import org.springframework.web.bind.annotation.GetMapping + +@RestController +@RequestMapping(["/api/products"]) +class ProductsController { + @GetMapping(value = ["/{id}"]) + fun get(id: Int): Any = TODO() +} +`, + ); + // OpenFeign consumer: positional array path. + fs.writeFileSync( + path.join(dir, 'src', 'OrdersClient.kt'), + `package com.example +import org.springframework.cloud.openfeign.FeignClient +import org.springframework.web.bind.annotation.PostMapping + +@FeignClient(name = "orders") +interface OrdersClient { + @PostMapping(["/orders/search"]) + fun search(): Any +} +`, + ); + // Spring HTTP Interface consumer: named array url. + fs.writeFileSync( + path.join(dir, 'src', 'PricingApi.kt'), + `package com.example +import org.springframework.web.service.annotation.GetExchange + +interface PricingApi { + @GetExchange(url = ["/pricing/{id}"]) + fun price(id: Int): Any +} +`, + ); + + const contracts = await extractor.extract(null, dir, makeRepo(dir)); + const providers = contracts.filter((c) => c.role === 'provider'); + const consumers = contracts.filter((c) => c.role === 'consumer'); + + // Array class prefix + array method path → provider. + expect( + providers.find((c) => c.contractId === 'http::GET::/api/products/{param}'), + ).toBeDefined(); + // @FeignClient positional array path → consumer. + expect( + consumers.find( + (c) => + c.contractId === 'http::POST::/orders/search' && c.meta.framework === 'openfeign', + ), + ).toBeDefined(); + // @GetExchange(url = [...]) array → consumer. + expect( + consumers.find( + (c) => + c.contractId === 'http::GET::/pricing/{param}' && + c.meta.framework === 'spring-http-interface', + ), + ).toBeDefined(); + }, + ); + + itKotlinConsumer( + 'handles Kotlin arrayOf("/x") annotation arrays across families (#2254 P3)', + async () => { + // arrayOf(...) is the explicit (older) form of a Kotlin String[] arg, + // distinct from the ["/x"] collection_literal. Each route-bearing + // annotation must accept it, positional and named, in all families. + const dir = path.join(tmpDir, 'kotlin-array-of'); + fs.mkdirSync(path.join(dir, 'src'), { recursive: true }); + // Provider: positional arrayOf class prefix + named arrayOf method path. + fs.writeFileSync( + path.join(dir, 'src', 'ProductsController.kt'), + `package com.example +import org.springframework.web.bind.annotation.RestController +import org.springframework.web.bind.annotation.RequestMapping +import org.springframework.web.bind.annotation.GetMapping + +@RestController +@RequestMapping(arrayOf("/api/products")) +class ProductsController { + @GetMapping(value = arrayOf("/{id}")) + fun get(id: Int): Any = TODO() +} +`, + ); + // OpenFeign consumer: named arrayOf path prefix. + fs.writeFileSync( + path.join(dir, 'src', 'OrdersClient.kt'), + `package com.example +import org.springframework.cloud.openfeign.FeignClient +import org.springframework.web.bind.annotation.PostMapping + +@FeignClient(name = "orders", path = arrayOf("/feign")) +interface OrdersClient { + @PostMapping(arrayOf("/orders/search")) + fun search(): Any +} +`, + ); + // Spring HTTP Interface consumer: positional arrayOf class prefix + named arrayOf url. + fs.writeFileSync( + path.join(dir, 'src', 'PricingApi.kt'), + `package com.example +import org.springframework.web.service.annotation.HttpExchange +import org.springframework.web.service.annotation.GetExchange + +@HttpExchange(arrayOf("/pricing")) +interface PricingApi { + @GetExchange(url = arrayOf("/{id}")) + fun price(id: Int): Any +} +`, + ); + + const contracts = await extractor.extract(null, dir, makeRepo(dir)); + const providers = contracts.filter((c) => c.role === 'provider'); + const consumers = contracts.filter((c) => c.role === 'consumer'); + + // class @RequestMapping(arrayOf) + method @GetMapping(value = arrayOf) → provider. + expect( + providers.find((c) => c.contractId === 'http::GET::/api/products/{param}'), + ).toBeDefined(); + // @FeignClient(path = arrayOf) + @PostMapping(arrayOf) → consumer (path applied). + expect( + consumers.find( + (c) => + c.contractId === 'http::POST::/feign/orders/search' && + c.meta.framework === 'openfeign', + ), + ).toBeDefined(); + // @HttpExchange(arrayOf) + @GetExchange(url = arrayOf) → consumer (prefix applied). + expect( + consumers.find( + (c) => + c.contractId === 'http::GET::/pricing/{param}' && + c.meta.framework === 'spring-http-interface', + ), + ).toBeDefined(); + }, + ); + + itKotlinConsumer( + 'registers a multi-element arrayOf("/a","/b") under every element (cross-product)', + async () => { + const dir = path.join(tmpDir, 'kotlin-array-of-multi'); + fs.mkdirSync(path.join(dir, 'src'), { recursive: true }); + fs.writeFileSync( + path.join(dir, 'src', 'MultiController.kt'), + `package com.example +import org.springframework.web.bind.annotation.RestController +import org.springframework.web.bind.annotation.RequestMapping +import org.springframework.web.bind.annotation.GetMapping + +@RestController +@RequestMapping(arrayOf("/a", "/b")) +class MultiController { + @GetMapping(arrayOf("/x", "/y")) + fun get(): Any = TODO() +} +`, + ); + + const contracts = await extractor.extract(null, dir, makeRepo(dir)); + const providers = contracts.filter((c) => c.role === 'provider'); + + // 2 prefixes × 2 method paths → 4 contract IDs. + for (const id of [ + 'http::GET::/a/x', + 'http::GET::/a/y', + 'http::GET::/b/x', + 'http::GET::/b/y', + ]) { + expect(providers.find((c) => c.contractId === id)).toBeDefined(); + } + }, + ); + + itKotlinConsumer( + 'mixes arrayOf and collection-literal arrays without cannibalising either', + async () => { + // The dedicated arrayOf pattern must not drop the sibling ["/x"] + // collection_literal match (the tree-sitter 0.21.x predicate-bucket hazard). + const dir = path.join(tmpDir, 'kotlin-array-of-mixed'); + fs.mkdirSync(path.join(dir, 'src'), { recursive: true }); + fs.writeFileSync( + path.join(dir, 'src', 'ArrayOfController.kt'), + `package com.example +import org.springframework.web.bind.annotation.RestController +import org.springframework.web.bind.annotation.RequestMapping +import org.springframework.web.bind.annotation.GetMapping + +@RestController +@RequestMapping(arrayOf("/aof")) +class ArrayOfController { + @GetMapping(arrayOf("/x")) + fun get(): Any = TODO() +} +`, + ); + fs.writeFileSync( + path.join(dir, 'src', 'LiteralController.kt'), + `package com.example +import org.springframework.web.bind.annotation.RestController +import org.springframework.web.bind.annotation.RequestMapping +import org.springframework.web.bind.annotation.GetMapping + +@RestController +@RequestMapping(["/lit"]) +class LiteralController { + @GetMapping(["/y"]) + fun get(): Any = TODO() +} +`, + ); + + const contracts = await extractor.extract(null, dir, makeRepo(dir)); + const providers = contracts.filter((c) => c.role === 'provider'); + + expect(providers.find((c) => c.contractId === 'http::GET::/aof/x')).toBeDefined(); + expect(providers.find((c) => c.contractId === 'http::GET::/lit/y')).toBeDefined(); + }, + ); + + itKotlinConsumer( + 'does not treat a non-arrayOf call or a non-route arrayOf key as a route (anti-overreach)', + async () => { + const dir = path.join(tmpDir, 'kotlin-array-of-negative'); + fs.mkdirSync(path.join(dir, 'src'), { recursive: true }); + // buildPath(...) is a call_expression but not arrayOf → no prefix. + // produces = arrayOf(...) is a non-route key → no route. + // arrayOf() is empty → no phantom route. + fs.writeFileSync( + path.join(dir, 'src', 'NegController.kt'), + `package com.example +import org.springframework.web.bind.annotation.RestController +import org.springframework.web.bind.annotation.RequestMapping +import org.springframework.web.bind.annotation.GetMapping + +@RestController +@RequestMapping(buildPath("/built")) +class NegController { + @GetMapping(produces = arrayOf("application/json")) + fun a(): Any = TODO() + + @GetMapping(arrayOf()) + fun b(): Any = TODO() +} +`, + ); + + const contracts = await extractor.extract(null, dir, makeRepo(dir)); + const providers = contracts.filter((c) => + c.symbolRef.filePath.endsWith('NegController.kt'), + ); + + // No route should be produced from any of the three anti-overreach forms. + expect(providers).toHaveLength(0); + }, + ); + + itKotlinConsumer( + 'does not extract @RequestLine on a Kotlin class method (Feign proxies are interfaces only)', + async () => { + // Feign builds its proxy from an interface; a @RequestLine on a concrete + // class is not a client call. Anti-overreach guard, parity with java.ts. + const dir = path.join(tmpDir, 'kotlin-request-line-class'); + fs.mkdirSync(path.join(dir, 'src'), { recursive: true }); + fs.writeFileSync( + path.join(dir, 'src', 'AiClientImpl.kt'), + `package com.example +import feign.RequestLine + +class AiClientImpl { + @RequestLine("GET /should-not-extract") + fun health(): String = TODO() +} +`, + ); + + const contracts = await extractor.extract(null, dir, makeRepo(dir)); + expect( + contracts.find((c) => c.contractId === 'http::GET::/should-not-extract'), + ).toBeUndefined(); + }, + ); + + itKotlinConsumer( + 'extracts Kotlin @RequestLine on a plain interface without @FeignClient (Feign.builder())', + async () => { + // Core-Feign usage: a plain interface with @RequestLine wired via + // Feign.builder() — no @FeignClient. The structural interface check + // (not a @FeignClient gate) admits it, matching the Java plugin. + const dir = path.join(tmpDir, 'kotlin-request-line-plain-interface'); + fs.mkdirSync(path.join(dir, 'src'), { recursive: true }); + fs.writeFileSync( + path.join(dir, 'src', 'AiClient.kt'), + `package com.example +import feign.RequestLine + +interface AiClient { + @RequestLine("POST /ai/summarize") + fun summarize(): String + + @RequestLine("GET /ai/health") + fun health(): String +} +`, + ); + + const contracts = await extractor.extract(null, dir, makeRepo(dir)); + const consumers = contracts.filter((c) => c.role === 'consumer'); + expect( + consumers.find( + (c) => + c.contractId === 'http::POST::/ai/summarize' && + c.meta.framework === 'openfeign' && + c.confidence === 0.75, + ), + ).toBeDefined(); + expect( + consumers.find( + (c) => c.contractId === 'http::GET::/ai/health' && c.meta.framework === 'openfeign', + ), + ).toBeDefined(); + }, + ); + + itKotlinConsumer( + 'does not emit a provider for a non-controller class implementing a route interface', + async () => { + // Only a @RestController/@Controller implementer serves the interface's + // routes. A plain service/adapter implementing the same interface must + // NOT emit phantom providers (parity with Java's isController gate). + const dir = path.join(tmpDir, 'kotlin-noncontroller-impl'); + fs.mkdirSync(path.join(dir, 'src'), { recursive: true }); + fs.writeFileSync( + path.join(dir, 'src', 'WarehouseApi.kt'), + `package com.example +import org.springframework.web.bind.annotation.RequestMapping +import org.springframework.web.bind.annotation.GetMapping + +@RequestMapping("/warehouses") +interface WarehouseApi { + @GetMapping("/{id}/stock") + fun listStock(id: String): Any +} +`, + ); + fs.writeFileSync( + path.join(dir, 'src', 'WarehouseServiceImpl.kt'), + `package com.example + +class WarehouseServiceImpl(private val svc: Svc) : WarehouseApi { + override fun listStock(id: String): Any = TODO() +} +`, + ); + + const contracts = await extractor.extract(null, dir, makeRepo(dir)); + const providers = contracts.filter((c) => c.role === 'provider'); + expect( + providers.find((c) => c.contractId === 'http::GET::/warehouses/{param}/stock'), + ).toBeUndefined(); + }, + ); + + itKotlinConsumer( + 'detects a controller using the arg-form @RestController("bean")', + async () => { + // The arg-form @RestController("bean") attaches under the class + // `modifiers` as an `annotation` whose child is a `constructor_invocation` + // (NOT a detached sibling). The controller gate reads its trailing name so + // the inherited route is still emitted. + const dir = path.join(tmpDir, 'kotlin-argform-restcontroller'); + fs.mkdirSync(path.join(dir, 'src'), { recursive: true }); + fs.writeFileSync( + path.join(dir, 'src', 'WarehouseApi.kt'), + `package com.example +import org.springframework.web.bind.annotation.RequestMapping +import org.springframework.web.bind.annotation.GetMapping + +@RequestMapping("/warehouses") +interface WarehouseApi { + @GetMapping("/{id}/stock") + fun listStock(id: String): Any +} +`, + ); + fs.writeFileSync( + path.join(dir, 'src', 'WarehouseController.kt'), + `package com.example +import org.springframework.web.bind.annotation.RestController + +@RestController("warehouseController") +class WarehouseController(private val svc: Svc) : WarehouseApi { + override fun listStock(id: String): Any = TODO() +} +`, + ); + + const contracts = await extractor.extract(null, dir, makeRepo(dir)); + const providers = contracts.filter((c) => c.role === 'provider'); + expect( + providers.find( + (c) => + c.contractId === 'http::GET::/warehouses/{param}/stock' && + c.symbolRef.filePath.endsWith('WarehouseController.kt'), + ), + ).toBeDefined(); + }, + ); + + itKotlinConsumer( + 'treats @(Get|...)Exchange as a consumer even on a concrete class (parity with Java)', + async () => { + // @(Get|...)Exchange is definitionally a client (HttpServiceProxyFactory) + // annotation. Like java.ts, the extractor classifies it as a consumer + // regardless of the enclosing type — so even a (mis-)use on a concrete + // class yields a consumer, never a provider. Pins the accepted behavior. + const dir = path.join(tmpDir, 'kotlin-exchange-on-class'); + fs.mkdirSync(path.join(dir, 'src'), { recursive: true }); + fs.writeFileSync( + path.join(dir, 'src', 'ReportClient.kt'), + `package com.example +import org.springframework.stereotype.Component +import org.springframework.web.service.annotation.GetExchange + +@Component +class ReportClient { + @GetExchange("/reports/{id}") + fun report(id: Int): Any = TODO() +} +`, + ); + + const contracts = await extractor.extract(null, dir, makeRepo(dir)); + expect( + contracts.find( + (c) => + c.role === 'consumer' && + c.contractId === 'http::GET::/reports/{param}' && + c.meta.framework === 'spring-http-interface', + ), + ).toBeDefined(); + // Never a provider. + expect( + contracts.find( + (c) => c.role === 'provider' && c.contractId === 'http::GET::/reports/{param}', + ), + ).toBeUndefined(); + }, + ); + + itKotlinConsumer( + 'emits one contract per element of a multi-element method-level path array', + async () => { + // Spring registers `@GetMapping(["/a", "/b"])` under BOTH paths, so the + // extractor must emit N contracts (one per array element), not just one. + const dir = path.join(tmpDir, 'kotlin-multi-method-array'); + fs.mkdirSync(path.join(dir, 'src'), { recursive: true }); + fs.writeFileSync( + path.join(dir, 'src', 'AliasController.kt'), + `package com.example +import org.springframework.web.bind.annotation.RestController +import org.springframework.web.bind.annotation.RequestMapping +import org.springframework.web.bind.annotation.GetMapping + +@RestController +@RequestMapping("/api") +class AliasController { + @GetMapping(value = ["/primary", "/alias"]) + fun get(): Any = TODO() +} +`, + ); + + const contracts = await extractor.extract(null, dir, makeRepo(dir)); + const providers = contracts.filter((c) => c.role === 'provider'); + expect(providers.find((c) => c.contractId === 'http::GET::/api/primary')).toBeDefined(); + expect(providers.find((c) => c.contractId === 'http::GET::/api/alias')).toBeDefined(); + }, + ); + + itKotlinConsumer( + 'emits one contract per element of a multi-element class-level prefix array', + async () => { + // Spring registers a method under EVERY class-level prefix, so a + // `@RequestMapping(["/api/v1", "/api/v2"])` controller must yield a + // contract per (prefix × method-path) combination, not just the last prefix. + const dir = path.join(tmpDir, 'kotlin-multi-prefix-array'); + fs.mkdirSync(path.join(dir, 'src'), { recursive: true }); + fs.writeFileSync( + path.join(dir, 'src', 'VersionedController.kt'), + `package com.example +import org.springframework.web.bind.annotation.RestController +import org.springframework.web.bind.annotation.RequestMapping +import org.springframework.web.bind.annotation.GetMapping + +@RestController +@RequestMapping(["/base/one", "/base/two"]) +class VersionedController { + @GetMapping("/items") + fun get(): Any = TODO() +} +`, + ); + + const contracts = await extractor.extract(null, dir, makeRepo(dir)); + const providers = contracts.filter((c) => c.role === 'provider'); + expect(providers.find((c) => c.contractId === 'http::GET::/base/one/items')).toBeDefined(); + expect(providers.find((c) => c.contractId === 'http::GET::/base/two/items')).toBeDefined(); + }, + ); + + it('emits one contract per element of a multi-element Java path array', async () => { + // Java parity: @GetMapping({"/a", "/b"}) method array and a multi-element + // class-level @RequestMapping must both expand to N contracts. + const dir = path.join(tmpDir, 'java-multi-array'); + fs.mkdirSync(path.join(dir, 'src'), { recursive: true }); + fs.writeFileSync( + path.join(dir, 'src', 'AliasController.java'), + `package com.example; +import org.springframework.web.bind.annotation.RestController; +import org.springframework.web.bind.annotation.RequestMapping; +import org.springframework.web.bind.annotation.GetMapping; + +@RestController +@RequestMapping({"/base/one", "/base/two"}) +public class AliasController { + @GetMapping({"/primary", "/alias"}) + public Object get() { return null; } +} +`, + ); + + const contracts = await extractor.extract(null, dir, makeRepo(dir)); + const providers = contracts.filter((c) => c.role === 'provider'); + for (const id of [ + 'http::GET::/base/one/primary', + 'http::GET::/base/one/alias', + 'http::GET::/base/two/primary', + 'http::GET::/base/two/alias', + ]) { + expect(providers.find((c) => c.contractId === id)).toBeDefined(); + } + }); + + itKotlinConsumer( + 'combines a Kotlin controller class prefix with an inherited interface prefix', + async () => { + // Interface-based controller where BOTH the controller and the interface + // carry a class-level @RequestMapping: the inherited route must be prefixed + // by the controller prefix too (parity with java.ts joinInheritedSpringPath). + const dir = path.join(tmpDir, 'kotlin-controller-prefix-inherit'); + fs.mkdirSync(path.join(dir, 'src'), { recursive: true }); + fs.writeFileSync( + path.join(dir, 'src', 'WidgetApi.kt'), + `package com.example +import org.springframework.web.bind.annotation.RequestMapping +import org.springframework.web.bind.annotation.GetMapping + +@RequestMapping("/v1") +interface WidgetApi { + @GetMapping("/{id}") + fun fetch(id: String): Any +} +`, + ); + fs.writeFileSync( + path.join(dir, 'src', 'WidgetController.kt'), + `package com.example +import org.springframework.web.bind.annotation.RestController +import org.springframework.web.bind.annotation.RequestMapping + +@RestController +@RequestMapping("/api") +class WidgetController : WidgetApi { + override fun fetch(id: String): Any = TODO() +} +`, + ); + + const contracts = await extractor.extract(null, dir, makeRepo(dir)); + const providers = contracts.filter((c) => c.role === 'provider'); + expect( + providers.find( + (c) => + c.contractId === 'http::GET::/api/v1/{param}' && + c.symbolRef.filePath.endsWith('WidgetController.kt'), + ), + ).toBeDefined(); + }, + ); + + itKotlinConsumer( + 'does not double a shared prefix when a Kotlin controller repeats the interface prefix', + async () => { + // #2057 parity: controller prefix == interface prefix must not be prepended + // twice (no /shared/shared/...). + const dir = path.join(tmpDir, 'kotlin-controller-prefix-dedup'); + fs.mkdirSync(path.join(dir, 'src'), { recursive: true }); + fs.writeFileSync( + path.join(dir, 'src', 'LedgerApi.kt'), + `package com.example +import org.springframework.web.bind.annotation.RequestMapping +import org.springframework.web.bind.annotation.GetMapping + +@RequestMapping("/shared") +interface LedgerApi { + @GetMapping("/entries") + fun entries(): Any +} +`, + ); + fs.writeFileSync( + path.join(dir, 'src', 'LedgerController.kt'), + `package com.example +import org.springframework.web.bind.annotation.RestController +import org.springframework.web.bind.annotation.RequestMapping + +@RestController +@RequestMapping("/shared") +class LedgerController : LedgerApi { + override fun entries(): Any = TODO() +} +`, + ); + + const contracts = await extractor.extract(null, dir, makeRepo(dir)); + const providers = contracts.filter((c) => c.role === 'provider'); + expect(providers.find((c) => c.contractId === 'http::GET::/shared/entries')).toBeDefined(); + expect( + providers.find((c) => c.contractId === 'http::GET::/shared/shared/entries'), + ).toBeUndefined(); + }, + ); + + itKotlinConsumer( + 'still combines distinct inherited Kotlin prefixes that share a leading segment', + async () => { + // Twin of the Java 'shared leading segment' case: controller @RequestMapping("/open") + // + interface @RequestMapping("/open/ai") must combine to /open/open/ai/query, NOT + // dedup to /open/ai/query (the dedup only fires on an exact prefix match). + const dir = path.join(tmpDir, 'kotlin-shared-leading-prefix'); + fs.mkdirSync(path.join(dir, 'src'), { recursive: true }); + fs.writeFileSync( + path.join(dir, 'src', 'DataReleaseApi.kt'), + `package com.example +import org.springframework.web.bind.annotation.RequestMapping +import org.springframework.web.bind.annotation.GetMapping + +@RequestMapping("/open/ai") +interface DataReleaseApi { + @GetMapping("/query") + fun query(): Any +} +`, + ); + fs.writeFileSync( + path.join(dir, 'src', 'DataReleaseController.kt'), + `package com.example +import org.springframework.web.bind.annotation.RestController +import org.springframework.web.bind.annotation.RequestMapping + +@RestController +@RequestMapping("/open") +class DataReleaseController : DataReleaseApi { + override fun query(): Any = TODO() +} +`, + ); + + const contracts = await extractor.extract(null, dir, makeRepo(dir)); + const providers = contracts.filter((c) => c.role === 'provider'); + expect( + providers.find((c) => c.contractId === 'http::GET::/open/open/ai/query'), + ).toBeDefined(); + expect(providers.find((c) => c.contractId === 'http::GET::/open/ai/query')).toBeUndefined(); + }, + ); + + itKotlinConsumer( + 'keeps a Kotlin controller prefix when a prefix-less interface method starts with the same path', + async () => { + // Twin of the Java prefix-overlap case: controller @RequestMapping("/users") + // + interface @GetMapping("/users/{id}") (no interface prefix) → + // /users/users/{param}, not deduped to /users/{param}. + const dir = path.join(tmpDir, 'kotlin-method-prefix-overlap'); + fs.mkdirSync(path.join(dir, 'src'), { recursive: true }); + fs.writeFileSync( + path.join(dir, 'src', 'UserApi.kt'), + `package com.example +import org.springframework.web.bind.annotation.GetMapping + +interface UserApi { + @GetMapping("/users/{id}") + fun getUser(id: String): Any +} +`, + ); + fs.writeFileSync( + path.join(dir, 'src', 'UserController.kt'), + `package com.example +import org.springframework.web.bind.annotation.RestController +import org.springframework.web.bind.annotation.RequestMapping + +@RestController +@RequestMapping("/users") +class UserController : UserApi { + override fun getUser(id: String): Any = TODO() +} +`, + ); + + const contracts = await extractor.extract(null, dir, makeRepo(dir)); + const providers = contracts.filter((c) => c.role === 'provider'); + expect( + providers.find((c) => c.contractId === 'http::GET::/users/users/{param}'), + ).toBeDefined(); + expect(providers.find((c) => c.contractId === 'http::GET::/users/{param}')).toBeUndefined(); + }, + ); + + itKotlinConsumer( + 'skips ambiguous inherited Kotlin routes when interfaces share a simple name', + async () => { + // Twin of the Java simple-name-collision case: two distinct interfaces both + // named StatusApi → ambiguous, so the implementing controller emits nothing. + const dir = path.join(tmpDir, 'kotlin-iface-name-collision'); + fs.mkdirSync(path.join(dir, 'src', 'a'), { recursive: true }); + fs.mkdirSync(path.join(dir, 'src', 'b'), { recursive: true }); + fs.writeFileSync( + path.join(dir, 'src', 'a', 'StatusApi.kt'), + `package com.example.a +import org.springframework.web.bind.annotation.GetMapping + +interface StatusApi { + @GetMapping("/a/status") + fun getStatus(): Any +} +`, + ); + fs.writeFileSync( + path.join(dir, 'src', 'b', 'StatusApi.kt'), + `package com.example.b +import org.springframework.web.bind.annotation.GetMapping + +interface StatusApi { + @GetMapping("/b/status") + fun getStatus(): Any +} +`, + ); + fs.writeFileSync( + path.join(dir, 'src', 'StatusController.kt'), + `package com.example +import org.springframework.web.bind.annotation.RestController + +@RestController +class StatusController : StatusApi { + override fun getStatus(): Any = TODO() +} +`, + ); + + const contracts = await extractor.extract(null, dir, makeRepo(dir)); + const providers = contracts.filter((c) => c.role === 'provider'); + expect(providers.find((c) => c.contractId === 'http::GET::/a/status')).toBeUndefined(); + expect(providers.find((c) => c.contractId === 'http::GET::/b/status')).toBeUndefined(); + expect( + providers.filter((c) => c.symbolRef.filePath.endsWith('StatusController.kt')), + ).toHaveLength(0); + }, + ); + + itKotlinConsumer( + 'emits routes from every distinctly-named interface a Kotlin controller implements', + async () => { + // Positive multi-interface case (untested in both languages before #2254): + // a controller implementing two route interfaces emits both their routes. + const dir = path.join(tmpDir, 'kotlin-multi-iface'); + fs.mkdirSync(path.join(dir, 'src'), { recursive: true }); + fs.writeFileSync( + path.join(dir, 'src', 'Apis.kt'), + `package com.example +import org.springframework.web.bind.annotation.GetMapping + +interface OrdersApi { + @GetMapping("/orders") + fun orders(): Any +} + +interface UsersApi { + @GetMapping("/users") + fun users(): Any +} +`, + ); + fs.writeFileSync( + path.join(dir, 'src', 'GatewayController.kt'), + `package com.example +import org.springframework.web.bind.annotation.RestController + +@RestController +class GatewayController : OrdersApi, UsersApi { + override fun orders(): Any = TODO() + override fun users(): Any = TODO() +} +`, + ); + + const contracts = await extractor.extract(null, dir, makeRepo(dir)); + const providers = contracts.filter((c) => c.role === 'provider'); + expect(providers.find((c) => c.contractId === 'http::GET::/orders')).toBeDefined(); + expect(providers.find((c) => c.contractId === 'http::GET::/users')).toBeDefined(); + }, + ); + + // ─── Byte-identical Java↔Kotlin contract parity (set-equality harness) ───── + // Independent per-side twin tests can both pass while the emitted contract + // SETS differ (an extra contract on one side, or a confidence/framework + // drift). This harness feeds matched .java/.kt fixtures through both plugins + // and asserts the full projected contract set is equal across languages AND + // equal to the expected set — the only check that actually verifies the + // "byte-identical contract IDs" goal. Per-scenario twins above stay for + // readability and language-specific cases; this covers the parity-critical + // families. Drift in any covered family fails here directly. + describe('Java↔Kotlin contract parity (set-equality)', () => { + interface ParityFile { + name: string; + java: string; + kotlin: string; + } + interface ParityContract { + role: string; + contractId: string; + framework: unknown; + confidence: number; + } + interface ParityRow { + name: string; + files: ParityFile[]; + expected: ParityContract[]; + } + + const sortContracts = (contracts: ParityContract[]): ParityContract[] => + [...contracts].sort((a, b) => + `${a.role} ${a.contractId}`.localeCompare(`${b.role} ${b.contractId}`), + ); + + const projectContracts = ( + contracts: Awaited>, + ): ParityContract[] => + sortContracts( + contracts.map((c) => ({ + role: c.role, + contractId: c.contractId, + framework: c.meta.framework, + confidence: c.confidence, + })), + ); + + const rows: ParityRow[] = [ + { + name: '@RequestLine with @RequestMapping prefix fallback', + files: [ + { + name: 'OrderClient', + java: ` +import org.springframework.cloud.openfeign.FeignClient; +import org.springframework.web.bind.annotation.RequestMapping; +import feign.RequestLine; + +@FeignClient(name = "order-service") +@RequestMapping("/orders") +interface OrderClient { + @RequestLine("GET /{id}") + Object get(); +} +`, + kotlin: `package com.example +import org.springframework.cloud.openfeign.FeignClient +import org.springframework.web.bind.annotation.RequestMapping +import feign.RequestLine + +@FeignClient(name = "order-service") +@RequestMapping("/orders") +interface OrderClient { + @RequestLine("GET /{id}") + fun get(): Any +} +`, + }, + ], + expected: [ + { + role: 'consumer', + contractId: 'http::GET::/orders/{param}', + framework: 'openfeign', + confidence: 0.75, + }, + ], + }, + { + name: 'named @RequestLine(value=...)', + files: [ + { + name: 'CreateClient', + java: ` +import org.springframework.cloud.openfeign.FeignClient; +import feign.RequestLine; + +@FeignClient(name = "create-service") +interface CreateClient { + @RequestLine(value = "POST /create") + Object create(); +} +`, + kotlin: `package com.example +import org.springframework.cloud.openfeign.FeignClient +import feign.RequestLine + +@FeignClient(name = "create-service") +interface CreateClient { + @RequestLine(value = "POST /create") + fun create(): Any +} +`, + }, + ], + expected: [ + { + role: 'consumer', + contractId: 'http::POST::/create', + framework: 'openfeign', + confidence: 0.75, + }, + ], + }, + { + name: '@FeignClient(path) + @GetMapping', + files: [ + { + name: 'UsersClient', + java: ` +import org.springframework.cloud.openfeign.FeignClient; +import org.springframework.web.bind.annotation.GetMapping; + +@FeignClient(name = "users-service", path = "/api") +interface UsersClient { + @GetMapping("/users") + Object users(); +} +`, + kotlin: `package com.example +import org.springframework.cloud.openfeign.FeignClient +import org.springframework.web.bind.annotation.GetMapping + +@FeignClient(name = "users-service", path = "/api") +interface UsersClient { + @GetMapping("/users") + fun users(): Any +} +`, + }, + ], + expected: [ + { + role: 'consumer', + contractId: 'http::GET::/api/users', + framework: 'openfeign', + confidence: 0.7, + }, + ], + }, + { + name: '@HttpExchange(url) prefix + @GetExchange', + files: [ + { + name: 'ProductApi', + java: ` +import org.springframework.web.service.annotation.HttpExchange; +import org.springframework.web.service.annotation.GetExchange; + +@HttpExchange(url = "/products") +interface ProductApi { + @GetExchange("/{id}") + Object get(); +} +`, + kotlin: `package com.example +import org.springframework.web.service.annotation.HttpExchange +import org.springframework.web.service.annotation.GetExchange + +@HttpExchange(url = "/products") +interface ProductApi { + @GetExchange("/{id}") + fun get(): Any +} +`, + }, + ], + expected: [ + { + role: 'consumer', + contractId: 'http::GET::/products/{param}', + framework: 'spring-http-interface', + confidence: 0.75, + }, + ], + }, + { + name: 'WebClient long-form method(HttpMethod.X).uri(...)', + files: [ + { + name: 'LongFormClient', + java: ` +import org.springframework.http.HttpMethod; +import org.springframework.web.reactive.function.client.WebClient; + +class LongFormClient { + void run(WebClient webClient) { + webClient.method(HttpMethod.GET).uri("/api/items").retrieve(); + } +} +`, + kotlin: `package com.example +import org.springframework.http.HttpMethod +import org.springframework.web.reactive.function.client.WebClient + +class LongFormClient { + fun run(webClient: WebClient) { + webClient.method(HttpMethod.GET).uri("/api/items").retrieve() + } +} +`, + }, + ], + expected: [ + { + role: 'consumer', + contractId: 'http::GET::/api/items', + framework: 'spring-web-client', + confidence: 0.7, + }, + ], + }, + { + name: 'interface-based controller inheritance', + files: [ + { + name: 'WarehouseApi', + java: ` +import org.springframework.web.bind.annotation.RequestMapping; +import org.springframework.web.bind.annotation.GetMapping; + +@RequestMapping("/warehouses") +interface WarehouseApi { + @GetMapping("/{id}/stock") + Object listStock(); +} +`, + kotlin: `package com.example +import org.springframework.web.bind.annotation.RequestMapping +import org.springframework.web.bind.annotation.GetMapping + +@RequestMapping("/warehouses") +interface WarehouseApi { + @GetMapping("/{id}/stock") + fun listStock(): Any +} +`, + }, + { + name: 'WarehouseController', + java: ` +import org.springframework.web.bind.annotation.RestController; + +@RestController +class WarehouseController implements WarehouseApi { + @Override + public Object listStock() { return null; } +} +`, + kotlin: `package com.example +import org.springframework.web.bind.annotation.RestController + +@RestController +class WarehouseController : WarehouseApi { + override fun listStock(): Any = TODO() +} +`, + }, + ], + expected: [ + { + role: 'provider', + contractId: 'http::GET::/warehouses/{param}/stock', + framework: 'spring', + confidence: 0.8, + }, + ], + }, + ]; + + rows.forEach((row) => { + itKotlinConsumer(`emits identical contracts for ${row.name}`, async () => { + const base = path.join(tmpDir, `parity-${row.name.replace(/[^a-z0-9]+/gi, '-')}`); + const javaDir = path.join(base, 'java'); + const kotlinDir = path.join(base, 'kotlin'); + fs.mkdirSync(path.join(javaDir, 'src'), { recursive: true }); + fs.mkdirSync(path.join(kotlinDir, 'src'), { recursive: true }); + for (const file of row.files) { + fs.writeFileSync(path.join(javaDir, 'src', `${file.name}.java`), file.java); + fs.writeFileSync(path.join(kotlinDir, 'src', `${file.name}.kt`), file.kotlin); + } + + const javaContracts = projectContracts( + await extractor.extract(null, javaDir, makeRepo(javaDir)), + ); + const kotlinContracts = projectContracts( + await extractor.extract(null, kotlinDir, makeRepo(kotlinDir)), + ); + const expected = sortContracts(row.expected); + + // The two languages emit the same contract set... + expect(kotlinContracts).toEqual(javaContracts); + // ...and it is exactly the expected set (no extra/missing contracts). + expect(javaContracts).toEqual(expected); + }); + }); + }); + it('extracts Go stdlib and resty calls', async () => { const dir = path.join(tmpDir, 'go-consumer'); fs.mkdirSync(path.join(dir, 'cmd'), { recursive: true }); diff --git a/gitnexus/test/unit/lbug-checkpoint.test.ts b/gitnexus/test/unit/lbug-checkpoint.test.ts index 6eac7e73a..f898030e2 100644 --- a/gitnexus/test/unit/lbug-checkpoint.test.ts +++ b/gitnexus/test/unit/lbug-checkpoint.test.ts @@ -23,10 +23,20 @@ import { flushWAL } from '../../src/core/lbug/lbug-adapter.js'; describe('flushWAL / safeClose — consolidation guard (#1376)', () => { let adapterSource: string; + // Strip comments before the structural assertions so they reflect CODE only. + // Otherwise a `conn.close()` / `db.close()` / `.query('CHECKPOINT')` token + // mentioned in a doc comment would falsely trip (or vacuously satisfy) a guard, + // coupling the test to comment wording — exactly the brittleness flagged in the + // #2264 review (a prior commit had to reword a comment just to keep this green). + const codeOnly = (src: string): string => + src.replace(/\/\*[\s\S]*?\*\//g, '').replace(/\/\/[^\n]*/g, ''); + beforeAll(async () => { - adapterSource = await fs.readFile( - path.join(__dirname, '..', '..', 'src', 'core', 'lbug', 'lbug-adapter.ts'), - 'utf-8', + adapterSource = codeOnly( + await fs.readFile( + path.join(__dirname, '..', '..', 'src', 'core', 'lbug', 'lbug-adapter.ts'), + 'utf-8', + ), ); }); @@ -44,7 +54,8 @@ describe('flushWAL / safeClose — consolidation guard (#1376)', () => { }); it('closeLbug delegates to safeClose instead of inlining conn.close/db.close', () => { - const closeLbugBody = adapterSource.slice(adapterSource.indexOf('export const closeLbug')); + // Match `closeLbug =` precisely so we don't prefix-match `closeLbugBeforeExit`. + const closeLbugBody = adapterSource.slice(adapterSource.indexOf('export const closeLbug =')); expect(closeLbugBody).toMatch(/await safeClose\(\)/); // closeLbug must NOT contain its own conn.close() or db.close() — those // live exclusively inside safeClose now. @@ -53,8 +64,26 @@ describe('flushWAL / safeClose — consolidation guard (#1376)', () => { expect(closeLbugBlock).not.toMatch(/db\.close\(\)/); }); + it('exports closeLbugBeforeExit (CHECKPOINT-only, skips native close) (#2264)', () => { + expect(adapterSource).toMatch(/export const closeLbugBeforeExit/); + // closeLbugBeforeExit is declared immediately before closeLbug; its body must + // CHECKPOINT via flushWAL and NEVER do a native conn/db close (that's the + // whole point — it relies on a guaranteed process.exit). + const body = adapterSource.slice( + adapterSource.indexOf('export const closeLbugBeforeExit'), + adapterSource.indexOf('export const closeLbug ='), + ); + expect(body).toMatch(/await flushWAL\(\)/); + expect(body).not.toMatch(/conn\.close\(\)/); + expect(body).not.toMatch(/db\.close\(\)/); + }); + it('CHECKPOINT is issued only by flushWAL (best-effort) and tryFlushWAL (rethrows for the retry driver)', () => { - const matches = adapterSource.match(/conn\.query\('CHECKPOINT'\)/g) ?? []; + // Receiver-agnostic: since the connection-serialization refactor (#2264) + // both sites capture `const c = conn` and call `c.query('CHECKPOINT')` + // inside withConnLock, so match `.query('CHECKPOINT')` regardless of the + // receiver name rather than the literal `conn.query(...)`. + const matches = adapterSource.match(/\.query\('CHECKPOINT'\)/g) ?? []; // Two authorized sites: `flushWAL` (swallows errors — used by // `safeClose` and the server's best-effort flush) and `tryFlushWAL` // (rethrows so the manual checkpoint driver in `wal-checkpoint-driver.ts` diff --git a/gitnexus/test/unit/repo-manager-finalize-invariant.test.ts b/gitnexus/test/unit/repo-manager-finalize-invariant.test.ts index c4472a6de..d0e217365 100644 --- a/gitnexus/test/unit/repo-manager-finalize-invariant.test.ts +++ b/gitnexus/test/unit/repo-manager-finalize-invariant.test.ts @@ -18,6 +18,7 @@ import fs from 'fs/promises'; import { AnalysisNotFinalizedError, assertAnalysisFinalized, + isRepoRegistered, registerRepo, saveMeta, getStoragePaths, @@ -128,4 +129,17 @@ describe('assertAnalysisFinalized (#1169)', () => { const variant = process.platform === 'win32' ? tmpRepo.dbPath.toUpperCase() : tmpRepo.dbPath; // POSIX is case-sensitive; assertion uses canonical form await expect(assertAnalysisFinalized(variant)).resolves.toBeUndefined(); }); + + // isRepoRegistered backs the analyze up-to-date fast-path gate (#2264): the + // fast path must NOT short-circuit a repo that is indexed-but-unregistered + // (e.g. a prior --name collision wrote meta.json then failed before + // registerRepo), otherwise --allow-duplicate-name could never heal it. + it('isRepoRegistered is false when the repo has no registry entry', async () => { + expect(await isRepoRegistered(tmpRepo.dbPath)).toBe(false); + }); + + it('isRepoRegistered is true once a matching entry is written', async () => { + await registerRepo(tmpRepo.dbPath, meta); + expect(await isRepoRegistered(tmpRepo.dbPath)).toBe(true); + }); }); diff --git a/gitnexus/test/unit/shutdown-helpers.test.ts b/gitnexus/test/unit/shutdown-helpers.test.ts new file mode 100644 index 000000000..e4606383c --- /dev/null +++ b/gitnexus/test/unit/shutdown-helpers.test.ts @@ -0,0 +1,62 @@ +/** + * Unit tests for the shared bounded checkpoint-then-exit cleanup (#2264) used by + * the CLI SIGINT handler and the worker SIGTERM handler. The checkpoint + exit are + * injected so the helper is testable without touching the real LadybugDB close or + * the real process.exit. + */ +import { describe, it, expect, vi } from 'vitest'; +import { boundedCheckpointBeforeExit } from '../../src/core/lbug/shutdown-helpers.js'; + +describe('boundedCheckpointBeforeExit (#2264)', () => { + it('checkpoints, runs beforeExit, then exits with the given code (in order)', async () => { + const order: string[] = []; + const exit = vi.fn<(code: number) => void>((c) => { + order.push(`exit:${c}`); + }); + + await boundedCheckpointBeforeExit({ + exitCode: 130, + checkpoint: vi.fn(async () => { + order.push('checkpoint'); + }), + beforeExit: () => { + order.push('beforeExit'); + }, + exit, + }); + + expect(exit).toHaveBeenCalledWith(130); + expect(order).toEqual(['checkpoint', 'beforeExit', 'exit:130']); + }); + + it('reports a checkpoint failure via onFlushError and still exits', async () => { + const onFlushError = vi.fn<(err: unknown) => void>(); + const exit = vi.fn<(code: number) => void>(); + const err = new Error('checkpoint boom'); + + await boundedCheckpointBeforeExit({ + exitCode: 0, + checkpoint: vi.fn(async () => { + throw err; + }), + onFlushError, + exit, + }); + + expect(onFlushError).toHaveBeenCalledWith(err); + expect(exit).toHaveBeenCalledWith(0); + }); + + it('exits via the timeout when the checkpoint hangs', async () => { + const exit = vi.fn<(code: number) => void>(); + + await boundedCheckpointBeforeExit({ + exitCode: 0, + timeoutMs: 0, + checkpoint: () => new Promise(() => {}), // never resolves + exit, + }); + + expect(exit).toHaveBeenCalledWith(0); + }); +}); diff --git a/gitnexus/test/unit/stream-query-driver-guard.test.ts b/gitnexus/test/unit/stream-query-driver-guard.test.ts new file mode 100644 index 000000000..503c23a11 --- /dev/null +++ b/gitnexus/test/unit/stream-query-driver-guard.test.ts @@ -0,0 +1,53 @@ +/** + * Unit tests for the streamQuery WAL-driver guard (#2264). streamQuery is + * deliberately NOT wrapped in withConnLock (its per-row callback re-enters the + * adapter), so it must refuse to run while the WAL-checkpoint driver is live — + * otherwise its unlocked per-row reads could race a CHECKPOINT on the shared + * connection (the corruption window the lock serializes everything else against). + * Today the serve/read path never starts the driver; this guard fails loud if a + * future in-process analyze ever overlaps a stream. + */ +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; +import { streamQuery } from '../../src/core/lbug/lbug-adapter.js'; +import { markWalDriverActive } from '../../src/core/lbug/wal-driver-state.js'; +import { startWalCheckpointDriver } from '../../src/core/lbug/wal-checkpoint-driver.js'; + +describe('streamQuery WAL-driver guard (#2264)', () => { + beforeEach(() => { + // Manual checkpoint defaults on; pin it so the driver path is deterministic. + vi.stubEnv('GITNEXUS_WAL_MANUAL_CHECKPOINT', '1'); + }); + + afterEach(() => { + markWalDriverActive(false); + vi.unstubAllEnvs(); + }); + + it('throws when the WAL-checkpoint driver is active', async () => { + markWalDriverActive(true); + await expect(streamQuery('RETURN 1 AS one', () => undefined)).rejects.toThrow( + /WAL-checkpoint driver is active/, + ); + }); + + it('passes the guard when inactive (reaching the not-initialized check)', async () => { + markWalDriverActive(false); + await expect(streamQuery('RETURN 1 AS one', () => undefined)).rejects.toThrow( + /not initialized/, + ); + }); + + it('startWalCheckpointDriver arms the guard; stop() disarms it', async () => { + const driver = startWalCheckpointDriver({ periodMs: 1_000_000 }); + try { + await expect(streamQuery('RETURN 1 AS one', () => undefined)).rejects.toThrow( + /WAL-checkpoint driver is active/, + ); + } finally { + await driver.stop(); + } + await expect(streamQuery('RETURN 1 AS one', () => undefined)).rejects.toThrow( + /not initialized/, + ); + }); +}); diff --git a/gitnexus/test/unit/wal-checkpoint-driver-reentrancy.test.ts b/gitnexus/test/unit/wal-checkpoint-driver-reentrancy.test.ts new file mode 100644 index 000000000..289a5a23b --- /dev/null +++ b/gitnexus/test/unit/wal-checkpoint-driver-reentrancy.test.ts @@ -0,0 +1,66 @@ +/** + * Reentrancy-guard test for the manual WAL checkpoint driver. + * + * `setInterval` fires on a fixed cadence regardless of whether the previous + * checkpoint has settled. During a large `--pdg` writeback a CHECKPOINT can + * outlast the period; without a guard each overdue tick would launch ANOTHER + * concurrent CHECKPOINT on the singleton connection. The guard (`if (inflight) + * return`) ensures at most one checkpoint is ever in flight. + * + * `tryFlushWAL` is mocked so we can hold a checkpoint "in flight" and drive the + * interval with fake timers — no native engine involved. + */ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; + +vi.mock('../../src/core/lbug/lbug-adapter.js', () => ({ + tryFlushWAL: vi.fn(), +})); + +import { startWalCheckpointDriver } from '../../src/core/lbug/wal-checkpoint-driver.js'; +import { tryFlushWAL } from '../../src/core/lbug/lbug-adapter.js'; + +const mockedTryFlush = vi.mocked(tryFlushWAL); + +describe('startWalCheckpointDriver — reentrancy guard', () => { + let originalEnv: string | undefined; + + beforeEach(() => { + originalEnv = process.env.GITNEXUS_WAL_MANUAL_CHECKPOINT; + delete process.env.GITNEXUS_WAL_MANUAL_CHECKPOINT; // default = enabled + vi.useFakeTimers(); + mockedTryFlush.mockReset(); + }); + + afterEach(() => { + vi.useRealTimers(); + if (originalEnv === undefined) delete process.env.GITNEXUS_WAL_MANUAL_CHECKPOINT; + else process.env.GITNEXUS_WAL_MANUAL_CHECKPOINT = originalEnv; + }); + + it('starts only one checkpoint while a prior one is still in flight, then resumes', async () => { + let resolveFirst!: () => void; + mockedTryFlush + // First checkpoint is held open until we resolve it. + .mockImplementationOnce( + () => + new Promise((resolve) => { + resolveFirst = () => resolve(true); + }), + ) + // Any later checkpoint completes immediately. + .mockImplementation(() => Promise.resolve(true)); + + const driver = startWalCheckpointDriver({ periodMs: 10 }); + + // ~5 ticks fire while the first checkpoint is still pending. + await vi.advanceTimersByTimeAsync(55); + expect(mockedTryFlush).toHaveBeenCalledTimes(1); + + // Let the first settle; subsequent ticks may now fire a new checkpoint. + resolveFirst(); + await vi.advanceTimersByTimeAsync(25); + expect(mockedTryFlush.mock.calls.length).toBeGreaterThanOrEqual(2); + + await driver.stop(); + }); +}); diff --git a/gitnexus/vitest.config.ts b/gitnexus/vitest.config.ts index d8dc68835..89719cd80 100644 --- a/gitnexus/vitest.config.ts +++ b/gitnexus/vitest.config.ts @@ -70,6 +70,7 @@ export default defineConfig({ 'test/integration/lbug-readonly-init.test.ts', 'test/integration/analyze-wal-checkpoint-failure.test.ts', 'test/integration/lbug-non-ascii-path.test.ts', + 'test/integration/lbug-conn-serialization.test.ts', ], fileParallelism: false, sequence: { groupOrder: 1 }, @@ -103,6 +104,7 @@ export default defineConfig({ 'test/integration/lbug-readonly-init.test.ts', 'test/integration/analyze-wal-checkpoint-failure.test.ts', 'test/integration/lbug-non-ascii-path.test.ts', + 'test/integration/lbug-conn-serialization.test.ts', 'test/integration/skills-e2e.test.ts', ], },