diff --git a/gitnexus/src/core/group/extractors/http-patterns/java.ts b/gitnexus/src/core/group/extractors/http-patterns/java.ts index 0df914d40..081b71805 100644 --- a/gitnexus/src/core/group/extractors/http-patterns/java.ts +++ b/gitnexus/src/core/group/extractors/http-patterns/java.ts @@ -31,6 +31,7 @@ import { import { extractJavaModuleConstants, foldJavaOperands, + isJavaConstantFile, parseJavaConstOperands, type RepoConstants, } from '../../../ingestion/route-extractors/java-const-resolver.js'; @@ -171,6 +172,19 @@ const JAVA_ROUTE_ANNOTATION_PATTERNS = compilePatterns({ key: (identifier) @key value: [(string_literal) @value (element_value_array_initializer (string_literal) @value)])))) name: (identifier) @member) @node + (class_declaration + (modifiers + (annotation + name: [(identifier) (scoped_identifier)] @ann + arguments: (annotation_argument_list [(identifier) @value_expr (field_access) @value_expr (binary_expression) @value_expr])))) @node + (class_declaration + (modifiers + (annotation + name: [(identifier) (scoped_identifier)] @ann + arguments: (annotation_argument_list + (element_value_pair + key: (identifier) @key + value: [(identifier) @value_expr (field_access) @value_expr (binary_expression) @value_expr]))))) @node (method_declaration (modifiers (annotation @@ -511,6 +525,16 @@ interface RouteAnnotationScan { feignPrefixByInterfaceId: Map; /** Spring HTTP Interface `@HttpExchange(url|value)` type-level prefixes per class/interface node id. */ httpExchangePrefixByTypeId: Map; + /** + * Class node ids whose `@RequestMapping` prefix is a constant reference or + * concat rather than a literal. Folding a TYPE-level prefix would need the + * repo constant map inside `scanRouteAnnotations`, which has no access to it, + * so `scan()` suppresses every method route under such a class instead of + * emitting it with the prefix silently dropped (a wrong path, not a missing + * one). Ingestion's `extractSpringRoutes` applies the identical rule — R4 + * parity. + */ + typesWithUnfoldablePrefix: Set; /** Resolved Spring shortcut/`@RequestMapping` routes — paths × verbs yield one entry each. */ methodRoutes: MethodRouteAnnotation[]; /** One entry per OpenFeign `@RequestLine` whose value parses to a verb + path. */ @@ -538,6 +562,7 @@ function scanRouteAnnotations(tree: Parser.Tree): RouteAnnotationScan { // 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 typesWithUnfoldablePrefix = new Set(); const feignPrefixByInterfaceId = new Map(); const httpExchangePrefixByTypeId = new Map(); const methodRoutes: MethodRouteAnnotation[] = []; @@ -649,6 +674,11 @@ function scanRouteAnnotations(tree: Parser.Tree): RouteAnnotationScan { // — on an interface — an OpenFeign `@FeignClient(path = "...")` prefix. if (ann === 'RequestMapping') { if (!isRouteMemberKey(keyNode)) continue; + if (!valueNode) { + // Constant-valued class prefix — see `typesWithUnfoldablePrefix`. + typesWithUnfoldablePrefix.add(node.id); + continue; + } const prefix = unquoteLiteral(valueNode.text); if (prefix !== null) { pushPrefix(prefixByTypeId, node.id, prefix); @@ -659,13 +689,13 @@ function scanRouteAnnotations(tree: Parser.Tree): RouteAnnotationScan { } else if (ann === 'FeignClient' && node.type === 'interface_declaration') { // 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); + const prefix = valueNode ? unquoteLiteral(valueNode.text) : null; 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); + const prefix = valueNode ? unquoteLiteral(valueNode.text) : null; if (prefix !== null) pushPrefix(httpExchangePrefixByTypeId, node.id, prefix); } } @@ -715,6 +745,7 @@ function scanRouteAnnotations(tree: Parser.Tree): RouteAnnotationScan { return { prefixByTypeId, + typesWithUnfoldablePrefix, feignPrefixByInterfaceId, httpExchangePrefixByTypeId, methodRoutes: constrainedMethodRoutes, @@ -760,9 +791,14 @@ function collectImplementedInterfaces(typeNode: Parser.SyntaxNode): string[] { } function collectSpringTypes(filePath: string, tree: Parser.Tree): SharedSpringType[] { - const { prefixByTypeId, methodRoutes } = scanRouteAnnotations(tree); + const { prefixByTypeId, typesWithUnfoldablePrefix, methodRoutes } = scanRouteAnnotations(tree); const routesByMethodId = new Map>(); for (const route of methodRoutes) { + // Constant-valued class prefix: no single prefix string exists here, so the + // inheritance view would publish this route unprefixed. Skip — same rule as + // scan() and as ingestion (R4 parity). + const owner = findEnclosingClass(route.methodNode); + if (owner && typesWithUnfoldablePrefix.has(owner.id)) continue; // A constant-referencing route still carries `rawPath: ''` here — folding // happens in scan() against the repo constant map, which this // inheritance-view collector has no access to. Emitting it as an empty @@ -870,7 +906,12 @@ export const JAVA_HTTP_PLUGIN: HttpLanguagePlugin = { // A gate that also matched `import ...;` would parse the entire // repository here (tens of thousands of files) just to build import // tables the fold can derive per-file on demand. - if (!src || !/static\s+final\s+String\s|interface\s+[A-Z]\w*\s*\{/.test(src)) { + // + // The predicate is the SHARED one the ingestion provider uses, so the + // two subsystems agree on which files define constants. Its previous + // local spelling missed `final static String` and lowercase interface + // names, and admitted an interface that ingestion's gate rejected. + if (!src || !isJavaConstantFile(src)) { continue; } const tree = args.parseSource(args.parser, src); @@ -898,6 +939,7 @@ export const JAVA_HTTP_PLUGIN: HttpLanguagePlugin = { // `@RequestLine`s — from a single `matches()` pass over the tree. const { prefixByTypeId, + typesWithUnfoldablePrefix, feignPrefixByInterfaceId, httpExchangePrefixByTypeId, methodRoutes, @@ -936,6 +978,12 @@ export const JAVA_HTTP_PLUGIN: HttpLanguagePlugin = { }; for (const route of methodRoutes) { + // A constant-valued CLASS prefix cannot be folded here, so every method + // route under such a class is suppressed rather than emitted at a wrong + // (unprefixed) path — the same rule `classesWithArrayPrefix` already + // encodes for the array form, and the same rule ingestion applies. + const owner = findEnclosingClass(route.methodNode); + if (owner && typesWithUnfoldablePrefix.has(owner.id)) continue; // Non-literal route path: fold the operand list against the repo-wide // constant map. Skip (never a guessed path) when the fold fails or the // repo context is absent (context-less fallback scanning). diff --git a/gitnexus/src/core/ingestion/language-provider.ts b/gitnexus/src/core/ingestion/language-provider.ts index 3aa92ce37..d5ef03c1e 100644 --- a/gitnexus/src/core/ingestion/language-provider.ts +++ b/gitnexus/src/core/ingestion/language-provider.ts @@ -347,11 +347,11 @@ interface LanguageProviderConfig { * parse phase can resolve non-literal decorator route paths cross-file. * * The worker calls this when BOTH hold: - * - the file is cheap to harvest (provider-declared `moduleConstantHeuristic` - * matched — syntax-driven, e.g. a `static final String` field or a - * constants-bearing import; NEVER a class-name pattern like `*Constants`, - * which silently drops route constants living in classes named e.g. - * `ApiPaths`/`Routes`), and + * - the provider declares no `moduleConstantHeuristic`, or the one it + * declares matched — syntax-driven, e.g. a `static final String` field or + * a constants-bearing import; NEVER a class-name pattern like + * `*Constants`, which silently drops route constants living in classes + * named e.g. `ApiPaths`/`Routes`, and * - the extraction yields something resolvable (a literal, an expression, or * an import binding), keeping the aggregate bounded on large repos. * @@ -366,6 +366,12 @@ interface LanguageProviderConfig { * repos: files that cannot contribute (no constant-bearing syntax) are not * walked. Must be syntax-driven (field/import shape), not identifier * pattern-matching on class names. + * + * Default: undefined — harvest EVERY file of this language. A gate is opt-in + * because getting it wrong silently drops routes that already resolve, and a + * missed gate only costs time. Declare one only where the cost bites (Java's + * Maven monorepos) and only after checking it against every shape + * {@link extractModuleConstants} accepts. */ readonly moduleConstantHeuristic?: (content: string) => boolean; diff --git a/gitnexus/src/core/ingestion/languages/java.ts b/gitnexus/src/core/ingestion/languages/java.ts index 7d8726917..0fddb6657 100644 --- a/gitnexus/src/core/ingestion/languages/java.ts +++ b/gitnexus/src/core/ingestion/languages/java.ts @@ -18,6 +18,7 @@ import { extractSpringRoutes, extractSpringTypes } from '../route-extractors/spr import { extractJavaModuleConstants, foldJavaOperands, + isJavaConstantFile, } from '../route-extractors/java-const-resolver.js'; import { javaExportChecker } from '../export-detection.js'; import { createImportResolver } from '../import-resolvers/resolver-factory.js'; @@ -222,17 +223,24 @@ export const javaProvider = defineLanguage({ extractRouteInheritanceTypes: extractSpringTypes, // ── #2980: constant harvest + qualified-ref fold for non-literal mapping - // paths (`@WinPostMapping(ApiPaths.SAVE_V1)`) — kept behind provider hooks so + // paths (`@PostMapping(ApiPaths.SAVE_V1)`) — kept behind provider hooks so // the shared ingestion layers stay language-agnostic. The heuristic is // SYNTAX-driven (field/import shape), never a class-name pattern: constant // classes are routinely named `ApiPaths`/`Routes`/`Paths`, which a // `*Constants`-style gate would silently drop (review round-2 High finding). extractModuleConstants: extractJavaModuleConstants, + // One gate, shared with the group side's `prepareRepo` pre-pass so the two + // subsystems cannot disagree about which files define constants (see + // JAVA_CONSTANT_FILE_RE — the previous divergence dropped constant + // INTERFACES on this side only, which cost the graph its Route nodes while + // the group still published the contract). moduleConstantHeuristic: (content) => - /\bstatic\s+final\s+String\s/.test(content) || + isJavaConstantFile(content) || // `import com.winning.opt.common.ApiPaths;` — ANY class import can bind a // constant ref (`ApiPaths.X` at an annotation site), so gate on the - // general import shape, not on the imported name. + // general import shape, not on the imported name. Ingestion-only: this + // side needs the importing controller's own import table, which the group + // side instead derives lazily from the tree it already holds. /\bimport\s+(?:static\s+)?[\w.]+\s*;/.test(content), foldRoutePathOperands: foldJavaOperands, }); diff --git a/gitnexus/src/core/ingestion/languages/python.ts b/gitnexus/src/core/ingestion/languages/python.ts index 7aacdc0c6..ca40a4566 100644 --- a/gitnexus/src/core/ingestion/languages/python.ts +++ b/gitnexus/src/core/ingestion/languages/python.ts @@ -163,9 +163,13 @@ export const pythonProvider = defineLanguage({ // ── #2391 constant harvest, provider-hook form (#2980): module-level string // constants + from-imports for non-literal decorator route paths. Bare-name // refs fold through the shared resolver (no foldRoutePathOperands needed). + // No `moduleConstantHeuristic`: Python harvests unconditionally, exactly as + // #2391 shipped it. A content gate was tried here and removed on review — it + // required `NAME` immediately followed by `=`, so it silently dropped the two + // idiomatic typed-FastAPI shapes (`API: str = "/api"`, + // `API: Final[str] = "/api"`) and every composed constant whose RHS starts + // with an identifier (`USERS = BASE + "/users"`), i.e. it REGRESSED routes + // that already resolve on main. The worker treats a missing heuristic as + // default-open; only Java opts into a gate, where the cost actually bites. extractModuleConstants: extractPythonModuleConstants, - moduleConstantHeuristic: (content) => - // Module-level assignment (`X = "/path"`, possibly typed/annotated) or a - // from-import that can bind a constant name at a decorator site. - /^[^\S\n]*(?:[A-Za-z_]\w*\s*=\s*['"]|from\s+[\w.]+\s+import\s)/m.test(content), }); diff --git a/gitnexus/src/core/ingestion/route-extractors/constant-resolver.ts b/gitnexus/src/core/ingestion/route-extractors/constant-resolver.ts index 509d1de99..c96406d87 100644 --- a/gitnexus/src/core/ingestion/route-extractors/constant-resolver.ts +++ b/gitnexus/src/core/ingestion/route-extractors/constant-resolver.ts @@ -33,7 +33,7 @@ const MAX_RESOLVE_DEPTH = 8; * whose true value is genuinely huge — building it risks a `RangeError`/heap OOM, * so we floor to `null` (skip) instead (#2393). The depth cap bounds recursion but * NOT output size, which grows multiplicatively; this bounds the output. */ -const MAX_FOLD_LENGTH = 8192; +export const MAX_FOLD_LENGTH = 8192; /** * One term of a constant's right-hand side. A `+`-concatenation diff --git a/gitnexus/src/core/ingestion/route-extractors/java-const-resolver.ts b/gitnexus/src/core/ingestion/route-extractors/java-const-resolver.ts index 436fc42ce..cbd7c1fdc 100644 --- a/gitnexus/src/core/ingestion/route-extractors/java-const-resolver.ts +++ b/gitnexus/src/core/ingestion/route-extractors/java-const-resolver.ts @@ -19,10 +19,16 @@ * } * * Reference shapes at annotation sites this binding resolves: - * @WinPostMapping(ApiPathConstants.DIAGNOSIS_SAVE_V1) // qualified - * @WinPostMapping(com.winning.opt.X.ApiPathConstants.Y) // FQN-qualified - * @WinPostMapping(DIAGNOSIS_SAVE_V1) // static-imported - * @WinPostMapping(API_CIS_V1 + "summary/save") // inline concat + * @PostMapping(ApiPathConstants.DIAGNOSIS_SAVE_V1) // qualified + * @PostMapping(com.winning.opt.X.ApiPathConstants.Y) // FQN-qualified + * @PostMapping(DIAGNOSIS_SAVE_V1) // static-imported + * @PostMapping(API_CIS_V1 + "summary/save") // inline concat + * + * Which ANNOTATIONS count as routes is a separate question this module has no + * say in: `spring-shared.ts` holds an exact-name map, so a vendor alias like + * `@WinPostMapping` yields no route on this base regardless of how its value + * folds (#2883). Folding and alias recognition compose; neither implies the + * other. * * Import shapes consumed: * import com.winning.opt.diagnosis.api.constants.ApiPathConstants; @@ -35,8 +41,9 @@ */ import type Parser from 'tree-sitter'; +import { unquoteSpringLiteral } from './spring-shared.js'; import { - resolveConstant as foldConstant, + MAX_FOLD_LENGTH, type ImportBinding, type ImportResolver, type ModuleConstants, @@ -51,6 +58,43 @@ export type { RepoConstants, } from './constant-resolver.js'; +/** + * Cheap content gate: can this Java file DEFINE a string constant that a route + * annotation might reference? + * + * Exported so BOTH sides of the pipeline use the same predicate and cannot + * disagree about which files carry constants — the ingestion provider + * (`languages/java.ts`, as `moduleConstantHeuristic`) and the group extractor's + * `prepareRepo` pre-pass (`group/extractors/http-patterns/java.ts`). They used + * to spell it differently, and the two spellings disagreed on a constant + * INTERFACE: the group admitted it and published a provider contract at the + * folded path, while ingestion rejected the file and emitted no Route node for + * it — an R4 parity break in the losing direction, since ingestion is the side + * that drives the graph and `api_impact`. + * + * Arms: + * - `static final String` / `final static String` — Java lets modifiers appear + * in any order, and `public final static String X = "/a";` is legal. + * - any `interface` declaration — interface fields are implicitly + * `public static final` (JLS 9.3), so a pure constant interface carries + * neither keyword and no import. `@interface` (annotation type) matches too, + * costing one parse that yields nothing. + */ +const EXPLICIT_STRING_CONSTANT_RE = /\b(?:static\s+final|final\s+static)\s+String\s/; +const INTERFACE_DECL_RE = /\binterface\s+\w/; +const STRING_ASSIGNMENT_RE = /\bString\s+\w+\s*=/; + +export function isJavaConstantFile(source: string): boolean { + if (EXPLICIT_STRING_CONSTANT_RE.test(source)) return true; + // The interface arm is a bare word match, so on its own it admits any file + // whose PROSE mentions "interface " — and every admitted file costs the group + // side a full extra parse. Requiring a String assignment as well keeps every + // shape `extractJavaModuleConstants` accepts in an interface body (bare + // `String`, `java.lang.String`, no space before `=`, multi-declarator) while + // dropping the comment-only matches. + return INTERFACE_DECL_RE.test(source) && STRING_ASSIGNMENT_RE.test(source); +} + /** * The Java {@link ImportResolver}: map a fully-qualified import specifier to * the unique file key it refers to, or null when it cannot be pinned to @@ -61,14 +105,18 @@ export type { * file-path-keyed and Maven multi-module trees repeat package roots across * modules (`winning-opt-a/.../api/constants/ApiPathConstants.java` and * `winning-opt-b/.../api/constants/ApiPathConstants.java`), suffix matching - * must stay UNIQUE-suffix: an import whose class name matches N files in N - * different modules cannot be pinned by package alone — unless exactly one of - * them ALSO matches the full package path. We therefore rank candidates: - * 1. exact full-suffix match (`/.java` as a path suffix) - * 2. class-name-only suffix (`**/.java`) when exactly one exists - * and return null when both attempts are ambiguous. + * stays UNIQUE-suffix: an import whose full package+class path matches N files + * in N different modules cannot be pinned, so it returns null — the skip floor + * this module promises, never a wrong path. + * + * A nearest-shared-directory tie-break was tried here and removed on review: + * javac resolves duplicate FQNs by CLASSPATH ORDER, not directory proximity, so + * a `src/test` fixture copy or a module that merely sits closer in the tree can + * outrank the real dependency and yield a silently wrong literal. In a resolver + * whose whole contract is skip-or-correct, a plausible guess is the one answer + * that cannot be allowed. */ -export const resolveJavaImport: ImportResolver = (importingFileKey, moduleSpec, repoKeys) => { +export const resolveJavaImport: ImportResolver = (_importingFileKey, moduleSpec, repoKeys) => { // A static import `a.b.C.CONST` names the class as all-but-last segment; // a plain import `a.b.C` names the class as last segment. Both resolve to // a file ending `a/b/C.java`; treating the whole spec as a path and @@ -76,61 +124,42 @@ export const resolveJavaImport: ImportResolver = (importingFileKey, moduleSpec, const asPath = moduleSpec.replace(/\./g, '/'); const classFile = `${asPath}.java`; - // 1. Exact package-path suffix match. + // Exact package-path suffix match, unique or nothing. let hit: string | null = null; - let ambiguity = false; for (const key of repoKeys) { if (key === classFile || key.endsWith(`/${classFile}`)) { - if (hit !== null) { - ambiguity = true; - break; - } + if (hit !== null) return null; // 2+ modules carry this FQN — unresolvable hit = key; } } - if (!ambiguity) return hit; - - // Ambiguous full-path match (same package+class in 2+ modules is legal in - // separated-source monorepos but pathological for route constants). Try - // disambiguating by proximity to the importing file: prefer the candidate - // sharing the longest leading directory prefix with the importer. This - // mirrors how Maven/Gradle resolve classpath collisions in practice (nearest - // module wins) without ever guessing across unrelated trees. - const candidates: string[] = []; - for (const key of repoKeys) { - if (key === classFile || key.endsWith(`/${classFile}`)) candidates.push(key); - } - if (candidates.length > 1) { - const importerDirs = importingFileKey.split('/').slice(0, -1); - let best: string | null = null; - let bestDepth = -1; - let tie = false; - for (const c of candidates) { - const cDirs = c.split('/'); - let d = 0; - while (d < importerDirs.length && d < cDirs.length && importerDirs[d] === cDirs[d]) d++; - if (d > bestDepth) { - bestDepth = d; - best = c; - tie = false; - } else if (d === bestDepth) { - tie = true; - } - } - if (best !== null && !tie) return best; - } - return null; + return hit; }; -/** Is `node` a Java string literal (`"..."`) with its unquoted value? */ +/** + * Is `node` a Java string literal (`"..."`), and if so what value does the + * route layer give it? + * + * tree-sitter-java splits a `string_literal` AROUND its `escape_sequence` + * children, so joining `string_fragment`s alone silently DELETES every escape: + * `"/user/{id:\\d+}"` — the standard Spring path-variable regex constraint — + * folded to `/user/{id:d+}`, and a pure-escape literal (`"\\t"`) folded to the + * empty string. Slicing the quotes off the raw text keeps the source spelling, + * which is precisely what the LITERAL path does + * ({@link unquoteSpringLiteral}) — so `@GetMapping(ApiPaths.USER_REGEX)` and + * `@GetMapping("/user/{id:\\d+}")` now emit the same path for the same Java + * source instead of two spellings the graph cannot reconcile. Same + * `string_fragment`-join trap as the NestJS one in #3017. + */ function stringLiteralValue(node: Parser.SyntaxNode): string | null { if (node.type !== 'string_literal') return null; - const parts = node.children.filter((c) => c.type === 'string_fragment'); - if (parts.length === 0) { - // Empty literal `""` has no string_fragment child. - return ''; - } - return parts.map((c) => c.text).join(''); + // A Java text block is also a `string_literal` here, and `unquoteSpringLiteral` + // has a `"""` arm that would hand back the raw block — leading newline and + // incidental indentation included, both of which Java strips. Nothing + // downstream normalizes that, so it would publish a Route at a path like + // "\n /api/v1/x\n ". The old fragment-join returned '' here, which + // floored to skip; keep that floor rather than trade it for a wrong path. + if (node.text.startsWith('"""')) return null; + return unquoteSpringLiteral(node.text); } /** @@ -276,7 +305,7 @@ export function extractJavaModuleConstants(tree: Parser.Tree): ModuleConstants { // Type must be String (java.lang.String is implicit-imported). const typeNode = member.childForFieldName('type'); if (!typeNode) continue; - const typeText = typeNode.text.replace(/^com\.java\.lang\./, ''); + const typeText = typeNode.text; if (typeText !== 'String' && typeText !== 'java.lang.String') continue; const declarators = member.children.filter((c) => c.type === 'variable_declarator'); @@ -298,6 +327,23 @@ export function extractJavaModuleConstants(tree: Parser.Tree): ModuleConstants { if (operands === null) { literals.delete(name); exprs.delete(name); + // …and the static IMPORT of the same simple name. A local + // `static final String` shadows `import static a.b.C.PATH` inside + // that class (JLS 6.4.1), so the correct answer for a non-foldable + // rebind is "unresolvable" — leaving the import alive makes the fold + // fall through it (computeFold: literals → exprs → imports) and + // return the IMPORTED value, i.e. a wrong path where the skip floor + // is owed. #2393's Python defect, reproduced for Java. + // + // The delete is file-scoped because these maps are (see the header: + // nested types flatten into one file-level namespace). So a SIBLING + // top-level class in the same file that legitimately uses the import + // loses it too and floors to skip, where javac would resolve it. + // That direction is the acceptable one — a missing route, not a wrong + // one — and the shape (two top-level classes, one shadowing a static + // import with a non-foldable initializer) is vanishingly rare next to + // the wrong-value it prevents. + imports.delete(name); if (qname) { literals.delete(qname); exprs.delete(qname); @@ -331,28 +377,45 @@ export function extractJavaModuleConstants(tree: Parser.Tree): ModuleConstants { const walkTypes = (node: Parser.SyntaxNode, insideInterface: boolean): void => { for (const child of node.children ?? []) { - const isClass = child.type === 'class_declaration'; const isInterface = child.type === 'interface_declaration'; - if (isClass || isInterface) { - const nameNode = child.childForFieldName('name'); - const className = nameNode?.text ?? null; - const body = child.children.find( - (c) => c.type === 'class_body' || c.type === 'interface_body', - ); - // Recompute implicit interface semantics at each type boundary: a - // class nested in an interface is a normal class whose fields need - // explicit `static final` (JLS 9.5 — only the interface's own fields - // are implicitly public static final). Propagating the outer - // `insideInterface` flag in would harvest mutable nested fields as - // constants and let a same-name nested field shadow a real interface - // constant with a stale value. - if (body && className) collectFieldConstants(body, isInterface, className); - if (body) walkTypes(body, isInterface); - } else if (child.type === 'enum_declaration' || child.type === 'record_declaration') { - walkTypes(child, insideInterface); - } else { + // Enums and records are ordinary type declarations for constant + // purposes — their fields need an explicit `static final` (JLS 8.9/8.10), + // unlike an interface's implicitly-constant ones. They used to be only + // RECURSED into, never collected, so a `static final String` declared + // directly in an enum or record was silently absent from the map. + const isTypeDecl = + isInterface || + child.type === 'class_declaration' || + child.type === 'enum_declaration' || + child.type === 'record_declaration'; + if (!isTypeDecl) { walkTypes(child, insideInterface); + continue; } + const className = child.childForFieldName('name')?.text ?? null; + const body = child.children.find( + (c) => c.type === 'class_body' || c.type === 'interface_body' || c.type === 'enum_body', + ); + if (!body) continue; + // An enum's members hang one level deeper, under `enum_body_declarations` + // (the `enum_body` itself holds only the enum constants). + const memberBody = body.children.find((c) => c.type === 'enum_body_declarations') ?? body; + // Recompute implicit interface semantics at each type boundary: a + // class nested in an interface is a normal class whose fields need + // explicit `static final` (JLS 9.5 — only the interface's own fields + // are implicitly public static final). Propagating the outer + // `insideInterface` flag in would harvest mutable nested fields as + // constants and let a same-name nested field shadow a real interface + // constant with a stale value. + if (className) collectFieldConstants(memberBody, isInterface, className); + // Recurse over the WHOLE body, not just `memberBody`: an enum's constants + // are siblings of `enum_body_declarations`, so narrowing here dropped any + // type nested inside an enum-constant body whenever the enum also had + // member declarations. For a class/interface/record the two are the same + // node; for an enum `body` is a strict superset, and the extra visit to + // `enum_body_declarations` collects nothing twice (collectFieldConstants + // is still called on `memberBody` alone). + walkTypes(body, isInterface); } }; walkTypes(tree.rootNode, false); @@ -360,6 +423,43 @@ export function extractJavaModuleConstants(tree: Parser.Tree): ModuleConstants { return { literals, exprs, imports: imports as Map }; } +/** + * Per-fold state. Mirrors the guards the agnostic core carries in `foldName`, + * which this binding stopped delegating to once it had to resolve qualified + * operands itself: + * + * - `memo` caches SUCCESSES only and is never popped. Without it a + * shared-descendant DAG (`X_k = X_{k+1} + X_{k+1}`) re-folds each child once + * per reference — O(2^depth) — and {@link MAX_FOLD_LENGTH} cannot save it, + * because a chain whose intermediate values are the empty string never + * accumulates any output. Measured before this state existed: one route over + * a 31-line constants file took 2.7 s at 26 levels and 11 s at 28, on the + * main thread, per file. A `null` may be transient (a name that cycles on one + * branch can resolve on another), so caching it would be unsound. + * - `visited` is the ACTIVE resolution stack, popped on unwind, so diamonds + * fold instead of false-cycling while true cycles still terminate. + * - `constantKeys` is the candidate set import ambiguity is measured over: + * files that actually DEFINE a constant. Handing `resolveJavaImport` every + * repo key made the two subsystems disagree — ingestion's map also holds + * import-only files (its gate has an import arm), so a duplicate FQN that + * defines nothing was invisible to the group and made ingestion alone floor + * to skip. Hoisting it also stops rebuilding the set on every qualified ref. + */ +interface JavaFoldState { + readonly repo: RepoConstants; + readonly constantKeys: ReadonlySet; + readonly visited: Set; + readonly memo: Map; +} + +function newFoldState(repo: RepoConstants): JavaFoldState { + const constantKeys = new Set(); + for (const [key, mc] of repo) { + if (mc.literals.size > 0 || mc.exprs.size > 0) constantKeys.add(key); + } + return { repo, constantKeys, visited: new Set(), memo: new Map() }; +} + /** * Resolve a single Java constant referenced in `fileKey` to its literal string * value, folding `+` concatenation and following import chains via @@ -375,28 +475,54 @@ export function resolveJavaConstant( repo: RepoConstants, depth = 0, ): string | null { - // Cycle guard for the Java-qualified walk (self/mutual imports). The shared - // fold's visited-stack only guards the bare-name path below; qualified refs - // recurse through resolveJavaConstant directly, so bound the recursion here. + return resolveWithState(fileKey, name, newFoldState(repo), depth); +} + +function resolveWithState( + fileKey: string, + name: string, + state: JavaFoldState, + depth: number, +): string | null { if (depth > 32) return null; - // Qualified ref (`ApiPathConstants.FIELD`): the fold layer keys imports and - // constants by their IN-FILE name, so a dotted name never hits directly. - // Split head.tail: resolve the head through the importing file's class - // import, then look the tail up in the target file — first as the - // class-qualified alias `Head.TAIL` (what extractJavaModuleConstants - // records), then as a bare `TAIL` (same-file nested/interface constant). + const guard = `${fileKey}::${name}`; + const memoized = state.memo.get(guard); + if (memoized !== undefined) return memoized; + if (state.visited.has(guard)) return null; // cycle: `name` is on the active stack + state.visited.add(guard); + try { + const result = computeJavaFold(fileKey, name, state, depth); + if (result !== null) state.memo.set(guard, result); + return result; + } finally { + state.visited.delete(guard); + } +} + +function computeJavaFold( + fileKey: string, + name: string, + state: JavaFoldState, + depth: number, +): string | null { + const { repo, constantKeys } = state; + // Qualified ref (`ApiPathConstants.FIELD`): constants and imports are keyed by + // their IN-FILE name, so a dotted name never hits directly. Split head.tail: + // resolve the head through the importing file's class import, then look the + // tail up in the target file — first as the class-qualified alias `Head.TAIL` + // (what extractJavaModuleConstants records), then as a bare `TAIL` (same-file + // nested/interface constant). const dot = name.indexOf('.'); if (dot > 0) { const head = name.slice(0, dot); const tail = name.slice(dot + 1); - const importing = repo.get(fileKey); - const imp = importing?.imports.get(head); + const imp = repo.get(fileKey)?.imports.get(head); if (imp) { - const targetFile = resolveJavaImport(fileKey, imp.module, new Set(repo.keys())); + const targetFile = resolveJavaImport(fileKey, imp.module, constantKeys); if (targetFile !== null) { - const qualified = resolveJavaConstant(targetFile, `${head}.${tail}`, repo, depth + 1); + const qualified = resolveWithState(targetFile, `${head}.${tail}`, state, depth + 1); if (qualified !== null) return qualified; - const bare = resolveJavaConstant(targetFile, tail, repo, depth + 1); + const bare = resolveWithState(targetFile, tail, state, depth + 1); if (bare !== null) return bare; } return null; @@ -406,41 +532,83 @@ export function resolveJavaConstant( const parts = name.split('.'); for (let cut = parts.length - 2; cut >= 1; cut--) { const fqn = parts.slice(0, cut + 1).join('.'); - const targetFile = resolveJavaImport(fileKey, fqn, new Set(repo.keys())); + const targetFile = resolveJavaImport(fileKey, fqn, constantKeys); if (targetFile !== null) { const field = parts.slice(cut + 1).join('.'); const declaring = parts[cut]; - const qualified = resolveJavaConstant(targetFile, `${declaring}.${field}`, repo, depth + 1); + const qualified = resolveWithState(targetFile, `${declaring}.${field}`, state, depth + 1); if (qualified !== null) return qualified; - return resolveJavaConstant(targetFile, field, repo, depth + 1); + return resolveWithState(targetFile, field, state, depth + 1); } } + // No import bound the head and no FQN prefix resolved — fall through. A + // dotted name is ALSO a valid key in this file's own maps: + // `extractJavaModuleConstants` records every constant under + // `.` as well as its simple name, so a same-file + // qualified reference (`ApiPaths.X` inside ApiPaths.java) resolves below. } - return foldConstant(fileKey, name, repo, resolveJavaImport); + + // Name lookup: literals, then same-file expressions, then the import chase. + // Reached for a bare name and for a dotted name that named no import. + // Expressions are folded HERE rather than handed to the agnostic core because + // an operand of a Java initializer may itself be a QUALIFIED ref + // (`X = BConsts.Y + "/tail"`) and the core only knows bare names: it looks + // `BConsts.Y` up in maps keyed by simple name, misses, and floors the whole + // chain to null. Recursing through this function gives every operand the same + // qualified treatment the entry-point name got. + const mc = repo.get(fileKey); + if (!mc) return null; + const literal = mc.literals.get(name); + if (literal !== undefined) return literal; + const expr = mc.exprs.get(name); + if (expr !== undefined) return foldOperands(fileKey, expr, state, depth + 1); + const imp = mc.imports.get(name); + if (imp !== undefined) { + const targetFile = resolveJavaImport(fileKey, imp.module, constantKeys); + if (targetFile === null) return null; + return resolveWithState(targetFile, imp.originalName, state, depth + 1); + } + return null; +} + +/** + * Concatenate an operand list, resolving each `ref` through the qualified-aware + * walk so `Class.CONST` works at every position, not just at the entry point. + * + * Bounded by {@link MAX_FOLD_LENGTH}: the depth cap bounds RECURSION but not + * OUTPUT, which grows multiplicatively (`X = A + A; A = B + B; …`), so a + * pathological chain would build a gigabyte-scale string before any cap fired. + * Overrun floors to null (#2393). + */ +function foldOperands( + fileKey: string, + operands: readonly Operand[], + state: JavaFoldState, + depth: number, +): string | null { + let out = ''; + for (const op of operands) { + if (op.kind === 'literal') { + out += op.value; + } else { + const piece = resolveWithState(fileKey, op.name, state, depth); + if (piece === null) return null; + out += piece; + } + if (out.length > MAX_FOLD_LENGTH) return null; + } + return out; } /** * Fold an inline operand list (e.g. `API_CIS_V1 + "summary/save"`) against - * `fileKey`. Unlike the Python binding, refs are resolved through - * {@link resolveJavaConstant} first — the agnostic fold has no notion of - * Java's `Class.CONST` qualified names (its import indirection only covers - * bare names), so each `ref` operand is resolved individually and the pieces - * are concatenated here. + * `fileKey`, or null when any piece is unresolvable (skip floor). */ export function foldJavaOperands( fileKey: string, operands: readonly Operand[], repo: RepoConstants, ): string | null { - let out = ''; - for (const op of operands) { - if (op.kind === 'literal') { - out += op.value; - continue; - } - const piece = resolveJavaConstant(fileKey, op.name, repo); - if (piece === null) return null; - out += piece; - } + const out = foldOperands(fileKey, operands, newFoldState(repo), 0); return out === '' ? null : out; } diff --git a/gitnexus/src/core/ingestion/route-extractors/spring.ts b/gitnexus/src/core/ingestion/route-extractors/spring.ts index 48933ef3f..36828a1f5 100644 --- a/gitnexus/src/core/ingestion/route-extractors/spring.ts +++ b/gitnexus/src/core/ingestion/route-extractors/spring.ts @@ -54,6 +54,13 @@ import { parseJavaConstOperands } from './java-const-resolver.js'; * suppresses that class's method-level array routes rather than emit them with a * dropped prefix (a wrong route). Full class-array cross-product support is left * to a follow-up (#2280). + * + * The class-level `@value_expr` branches exist for the same reason: a + * CONSTANT-valued class prefix (`@RequestMapping(ApiPaths.BASE)`) cannot be + * folded here — the repo-wide constant map only exists in the parse phase — so + * they only DETECT it, and Phase 2 suppresses every method route under such a + * class. Without them the prefix was invisible and the method route was emitted + * unprefixed, i.e. at a path the application does not serve. */ const ROUTE_ANNOTATION_QUERY = new Parser.Query( Java, @@ -91,6 +98,24 @@ const ROUTE_ANNOTATION_QUERY = new Parser.Query( key: (identifier) @key value: [(string_literal) @value (element_value_array_initializer (string_literal) @value)]))))) @node + (class_declaration + (modifiers + (annotation + name: [(identifier) (scoped_identifier)] @ann + arguments: (annotation_argument_list + [(identifier) @value_expr + (field_access) @value_expr + (binary_expression) @value_expr])))) @node + (class_declaration + (modifiers + (annotation + name: [(identifier) (scoped_identifier)] @ann + arguments: (annotation_argument_list + (element_value_pair + key: (identifier) @key + value: [(identifier) @value_expr + (field_access) @value_expr + (binary_expression) @value_expr]))))) @node (method_declaration (modifiers (annotation @@ -141,6 +166,11 @@ export function extractSpringRoutes( // class-array cross-product support is out of scope here. const prefixByClassId = new Map(); const classesWithArrayPrefix = new Set(); + // Classes whose `@RequestMapping` prefix is a constant reference or concat. + // Same treatment as the array form, for the same reason: no single prefix + // string is knowable at extraction time, so emitting the methods below would + // publish them at a WRONG (unprefixed) path rather than not at all. + const classesWithUnfoldablePrefix = new Set(); const classHttpMethodsById = new Map(); for (const match of TYPE_DECLARATION_QUERY.matches(tree.rootNode)) { const typeNode = match.captures.find((capture) => capture.name === 'type')?.node; @@ -158,11 +188,16 @@ export function extractSpringRoutes( const node = caps['node']; const valueNode = caps['value']; const keyNode = caps['key']; - if (!annNode || !node || !valueNode) continue; + const valueExprNode = caps['value_expr']; + if (!annNode || !node || (!valueNode && !valueExprNode)) continue; const capturedAnnotationName = annNode.text.split('.').pop() ?? annNode.text; if (node.type === 'class_declaration' && capturedAnnotationName === 'RequestMapping') { if (!isRouteMemberKey(keyNode)) continue; + if (!valueNode) { + classesWithUnfoldablePrefix.add(node.id); + continue; + } if (valueNode.parent?.type === 'element_value_array_initializer') { classesWithArrayPrefix.add(node.id); continue; @@ -237,6 +272,16 @@ export function extractSpringRoutes( if (isArrayElement && enclosingClass && classesWithArrayPrefix.has(enclosingClass.id)) { continue; } + // Same rule for a CONSTANT-valued class prefix (`@RequestMapping(ApiPaths.BASE)`), + // and for every method route under it — not just array-form ones. The prefix + // needs the repo-wide constant map, which does not exist at extraction time, + // so the prefix would simply be dropped and the route emitted at a path the + // application never serves. On base such a route was not emitted at all; + // turning a missing fact into a wrong one is the failure this module's skip + // floor exists to prevent. Folding class prefixes cross-file is a follow-up. + if (enclosingClass && classesWithUnfoldablePrefix.has(enclosingClass.id)) { + continue; + } const classPrefix = enclosingClass ? (prefixByClassId.get(enclosingClass.id) ?? '') : ''; // `node` is the annotated `method_declaration`; its name field is the @@ -279,6 +324,13 @@ export function extractSpringRoutes( for (const match of TYPE_DECLARATION_QUERY.matches(tree.rootNode)) { const typeNode = match.captures.find((capture) => capture.name === 'type')?.node; if (typeNode?.type !== 'class_declaration') continue; + // A no-argument `@GetMapping` IS the class prefix, so a class prefix that + // cannot be folded here leaves nothing to emit — the route would ship with + // `routePath: ''` and no prefix, i.e. an empty-path Route. The Phase 2 loop + // above already suppresses these classes; this loop needs the same guard, or + // the suppression is one-sided and the group side (which routes both shapes + // through `methodRoutes`) disagrees with ingestion. + if (classesWithUnfoldablePrefix.has(typeNode.id)) continue; const classPrefix = prefixByClassId.get(typeNode.id) ?? ''; const classMethods = classHttpMethodsById.get(typeNode.id) ?? ['*']; for (const methodNode of directMethods(typeNode)) { diff --git a/gitnexus/src/core/ingestion/workers/parse-worker.ts b/gitnexus/src/core/ingestion/workers/parse-worker.ts index 3e6dd5079..2638aa66a 100644 --- a/gitnexus/src/core/ingestion/workers/parse-worker.ts +++ b/gitnexus/src/core/ingestion/workers/parse-worker.ts @@ -2969,7 +2969,12 @@ const processFileGroup = ( // paths cross-file. Cost-gated by the provider's syntax-driven heuristic; // only files that carry something resolvable (a constant definition or an // import binding) are emitted, keeping the aggregate bounded on large repos. - if (provider.extractModuleConstants && provider.moduleConstantHeuristic?.(parseContent)) { + // A provider that declares no heuristic harvests unconditionally (`?.` + // would have read `undefined` as "skip" and disabled the hook outright). + if ( + provider.extractModuleConstants && + (!provider.moduleConstantHeuristic || provider.moduleConstantHeuristic(parseContent)) + ) { const constants = provider.extractModuleConstants(tree); if (constants.literals.size > 0 || constants.exprs.size > 0 || constants.imports.size > 0) { (result.moduleConstants ??= []).push({ filePath: file.path, constants }); diff --git a/gitnexus/src/storage/parse-cache.ts b/gitnexus/src/storage/parse-cache.ts index f10985262..6de50176a 100644 --- a/gitnexus/src/storage/parse-cache.ts +++ b/gitnexus/src/storage/parse-cache.ts @@ -539,14 +539,37 @@ import type { ParseWorkerResult } from '../core/ingestion/workers/parse-worker.j // then adds Spring non-HTTP handler side-channel facts (#2417 / #2891), so Java // and Kotlin caches persist scheduled, event, messaging, and managed-job facts. // -// This PR originally carried its own bump (47 -> 48) for the Java constant-route -// capture set change (route-extractors/java-const-resolver.ts + the spring.ts -// operand branch + the parse-worker Java constant harvest): a warm pre-feature -// cache replays `moduleConstants=0` captures verbatim and silently drops every -// constant-based Spring route on unchanged files. After rebasing onto current -// main the ledger already sits at 70, whose capture set post-dates and includes -// this harvest — v70 invalidates those caches, so no additional bump is needed. -const SCHEMA_BUMP = 70; +// 70 -> 71 adds the Java constant-route capture set (#2980): +// `route-extractors/java-const-resolver.ts`, the `spring.ts` operand branch, +// and the parse-worker's provider-driven constant harvest. A warm pre-feature +// cache replays those files' worker results with `moduleConstants` absent and +// `routePathOperands` unset, so every constant-based Spring route on an +// unchanged file is silently dropped — the feature is inert until something +// else invalidates the cache. +// +// This branch briefly reasoned that no bump was needed because the ledger +// "already sits at 70, whose capture set post-dates and includes this harvest". +// It does not: v70 was cut by fe3d7e56b for Spring non-HTTP handler facts +// (#2417 / #2891), an ancestor of this PR's base, and it cannot include a +// harvest that does not exist on main. Because +// `PARSE_CACHE_VERSION = ${SCHEMA_BUMP}+${GITNEXUS_PKG_VERSION}` and +// package.json is untouched here, leaving 70 makes the key BYTE-IDENTICAL +// before and after this merge — precisely the inert-feature trap the v33/v34 +// notes above warn about. Exposure is bounded by the package version (a +// released upgrade invalidates anyway), but same-version warm caches — dev +// builds, CI caches, anyone who indexed with an unreleased build — replay the +// stale captures. +// +// 72, not 71: open PR #3017 (`fix/nest-decorator-routes`, NestJS decorator route +// indexing) already claims 71, with an identical pin test. Re-checking +// origin/main alone would not catch that — main is 70 and stays 70 until one of +// the two merges, at which point the second lands a byte-identical +// PARSE_CACHE_VERSION and is inert. This is exactly the rule the ledger states +// and the v37/v38 clash it was written for: the next free value above every +// IN-FLIGHT claim, not above origin/main. Every open PR touching gitnexus/ was +// scanned; #3017 is the only other claimant. +// RE-CHECK AGAINST origin/main AND OPEN PRs IMMEDIATELY BEFORE MERGING. +const SCHEMA_BUMP = 72; const GITNEXUS_PKG_VERSION = (() => { try { // package.json sits at gitnexus/package.json — two levels up from diff --git a/gitnexus/test/unit/group/java-const-route-parity.test.ts b/gitnexus/test/unit/group/java-const-route-parity.test.ts new file mode 100644 index 000000000..cfdf652a3 --- /dev/null +++ b/gitnexus/test/unit/group/java-const-route-parity.test.ts @@ -0,0 +1,213 @@ +/** + * Group ↔ ingestion parity for Java constant-valued Spring route paths (#2980). + * + * Drives `JAVA_HTTP_PLUGIN.prepareRepo` + `scan(tree, ctx, rel)` with all three + * arguments and compares the result against what `extractSpringRoutes` + the + * Java operand fold produce on the ingestion side. The existing Spring parity + * guards call `scan(tree)` with ONE argument, which makes them structurally + * blind here: without a repo context the plugin drops every constant-valued + * route, so no fixture they carry can exercise this feature. + * + * Asserted: + * • a constant-valued mapping resolves to the SAME path on both sides; + * • a CONSTANT class prefix suppresses the method route on both sides — the + * prefix cannot be folded at extraction time, and emitting the route + * unprefixed would publish a path the application does not serve; + * • without a repo context the group side emits nothing (the documented skip + * floor, and the branch that makes the 1-arg guards blind); + * • literal routes are untouched. + */ + +import { describe, it, expect } from 'vitest'; +import Parser from 'tree-sitter'; +import Java from 'tree-sitter-java'; +import { JAVA_HTTP_PLUGIN } from '../../../src/core/group/extractors/http-patterns/java.js'; +import type { HttpDetection } from '../../../src/core/group/extractors/http-patterns/types.js'; +import { extractSpringRoutes } from '../../../src/core/ingestion/route-extractors/spring.js'; +import { javaProvider } from '../../../src/core/ingestion/languages/java.js'; +import { + extractJavaModuleConstants, + foldJavaOperands, + type RepoConstants, +} from '../../../src/core/ingestion/route-extractors/java-const-resolver.js'; + +const parser = new Parser(); +const parseSource = (p: Parser, src: string): Parser.Tree => { + p.setLanguage(Java); + return p.parse(src); +}; +const parse = (src: string): Parser.Tree => parseSource(parser, src); + +/** Group side: prepareRepo + a 3-argument scan over every .java file. */ +function groupProviders(files: Record): string[] { + const ctx = JAVA_HTTP_PLUGIN.prepareRepo?.({ + files: Object.keys(files), + parser: new Parser(), + readFile: (rel: string) => files[rel] ?? null, + parseSource, + }); + const out: string[] = []; + for (const rel of Object.keys(files)) { + const detections: HttpDetection[] = JAVA_HTTP_PLUGIN.scan(parse(files[rel]), ctx, rel); + for (const d of detections) { + if (d.role === 'provider') out.push(`${d.method} ${d.path}`); + } + } + return out.sort(); +} + +/** Ingestion side: extract routes, then fold operands against the same map. */ +function ingestionRoutes(files: Record): string[] { + const repo: RepoConstants = new Map(); + for (const [rel, src] of Object.entries(files)) { + repo.set(rel, extractJavaModuleConstants(parse(src))); + } + const out: string[] = []; + for (const [rel, src] of Object.entries(files)) { + for (const route of extractSpringRoutes(parse(src), rel, 0)) { + const path = route.routePathOperands + ? foldJavaOperands(rel, route.routePathOperands, repo) + : route.routePath; + if (path === null) continue; + out.push(`${route.httpMethod} ${`${route.prefix ?? ''}${path}`.replace(/\/{2,}/g, '/')}`); + } + } + return out.sort(); +} + +const CONSTS = 'src/main/java/com/example/ApiPaths.java'; +const CTL = 'src/main/java/com/example/OrderController.java'; + +const CONSTS_SRC = `package com.example; +public class ApiPaths { + public static final String BASE = "/api/v1"; + public static final String ORDERS = "/api/v1/orders"; +}`; + +describe('Java constant-valued routes: group ↔ ingestion parity (#2980)', () => { + it('resolves a constant-valued mapping to the same path on both sides', () => { + const files = { + [CONSTS]: CONSTS_SRC, + [CTL]: `package com.example; +import com.example.ApiPaths; +public class OrderController { + @GetMapping(ApiPaths.ORDERS) + public void list() {} +}`, + }; + expect(groupProviders(files)).toEqual(['GET /api/v1/orders']); + expect(ingestionRoutes(files)).toEqual(groupProviders(files)); + }); + + it('suppresses the method route under a CONSTANT class prefix on both sides', () => { + // The class prefix needs the repo-wide constant map, which does not exist + // at extraction time on either side. Emitting the method route would drop + // the prefix and publish `GET /api/v1/orders`-without-its-base — a path the + // application never serves. On base such a route was not emitted at all, so + // shipping it unprefixed would turn a missing fact into a wrong one. + const files = { + [CONSTS]: CONSTS_SRC, + [CTL]: `package com.example; +import com.example.ApiPaths; +@RequestMapping(ApiPaths.BASE) +public class OrderController { + @GetMapping(ApiPaths.ORDERS) + public void list() {} + + @GetMapping("/literal") + public void literal() {} +}`, + }; + expect(groupProviders(files)).toEqual([]); + expect(ingestionRoutes(files)).toEqual([]); + }); + + it('still applies a LITERAL class prefix', () => { + const files = { + [CONSTS]: CONSTS_SRC, + [CTL]: `package com.example; +import com.example.ApiPaths; +@RequestMapping("/api/v1") +public class OrderController { + @GetMapping("/orders") + public void list() {} +}`, + }; + expect(groupProviders(files)).toEqual(['GET /api/v1/orders']); + expect(ingestionRoutes(files)).toEqual(['GET /api/v1/orders']); + }); + + it('emits nothing for a constant route when scanned without a repo context', () => { + // This is the branch that makes the 1-argument parity guards blind to the + // whole feature; pin it so it is not silently dead in the suite. + const src = `package com.example; +import com.example.ApiPaths; +public class OrderController { + @GetMapping(ApiPaths.ORDERS) + public void list() {} +}`; + const detections = JAVA_HTTP_PLUGIN.scan(parse(src)); + expect(detections.filter((d) => d.role === 'provider')).toEqual([]); + }); + + it('suppresses a NO-ARGUMENT mapping under a constant class prefix on both sides', () => { + // A bare `@GetMapping` IS the class prefix, so an unfoldable class prefix + // leaves nothing to emit. Ingestion routes these through a separate loop + // from the path-carrying ones, and that loop needs the same guard — without + // it ingestion emitted an empty-path Route where the group emitted nothing. + const files = { + [CONSTS]: CONSTS_SRC, + [CTL]: `package com.example; +import com.example.ApiPaths; +@RequestMapping(ApiPaths.BASE) +public class OrderController { + @GetMapping public void list() {} + @PostMapping public void create() {} +}`, + }; + expect(ingestionRoutes(files)).toEqual([]); + expect(groupProviders(files)).toEqual([]); + }); + + it('measures import ambiguity over the same candidate set on both sides', () => { + // Ingestion's harvest gate also admits import-only files, so its repo map is + // a superset of the group's. When ambiguity was measured over every key, a + // duplicate FQN belonging to a class that defines NOTHING was invisible to + // the group and made ingestion alone floor to skip — reopening the very + // parity break this feature exists to close. Both sides now measure over + // constant-DEFINING files only. + const files = { + 'svc-a/src/main/java/com/x/ApiPaths.java': `package com.x; +public class ApiPaths { public static final String ORDERS = "/api/v1/orders"; }`, + // Same FQN, different module, defines no constant — must not create ambiguity. + 'svc-b/src/main/java/com/x/ApiPaths.java': `package com.x; +import java.util.List; +public class ApiPaths {}`, + 'svc-a/src/main/java/com/x/web/OrderController.java': `package com.x.web; +import com.x.ApiPaths; +public class OrderController { + @GetMapping(ApiPaths.ORDERS) + public void list() {} +}`, + }; + // Guard the premise: the two maps really are different sizes. + const ingestionKeys = Object.entries(files).filter(([, src]) => + javaProvider.moduleConstantHeuristic?.(src), + ).length; + expect(ingestionKeys).toBe(3); + expect(groupProviders(files)).toEqual(['GET /api/v1/orders']); + expect(ingestionRoutes(files)).toEqual(['GET /api/v1/orders']); + }); + + it('leaves literal routes unchanged with no constant map at all', () => { + const files = { + [CTL]: `package com.example; +public class OrderController { + @PostMapping("/api/v1/orders") + public void create() {} +}`, + }; + expect(groupProviders(files)).toEqual(['POST /api/v1/orders']); + expect(ingestionRoutes(files)).toEqual(['POST /api/v1/orders']); + }); +}); diff --git a/gitnexus/test/unit/incremental-parse-cache.test.ts b/gitnexus/test/unit/incremental-parse-cache.test.ts index 17595f15b..cd353e800 100644 --- a/gitnexus/test/unit/incremental-parse-cache.test.ts +++ b/gitnexus/test/unit/incremental-parse-cache.test.ts @@ -221,14 +221,23 @@ describe('PARSE_CACHE_VERSION', () => { // Version 69 added #2969's JS/TS data-route-table decoratorRoutes. Version 70 // adds Spring non-HTTP handler side-channel facts (#2417 / #2891), so it is // the next free value after both cache payload changes. - it('pins SCHEMA_BUMP to 70 so concurrent bumps cannot silently collide (#2766)', () => { - expect(Number(PARSE_CACHE_VERSION.split('+', 1)[0])).toBe(70); + // Moved 70 -> 71 for #2980's Java constant-route capture set (moduleConstants + // + routePathOperands). This branch first argued no bump was needed because + // "the ledger already sits at 70, whose capture set post-dates and includes + // this harvest" — it does not: 70 was cut by fe3d7e56b for #2417/#2891, an + // ancestor of this PR's base. Leaving it made PARSE_CACHE_VERSION byte- + // identical across the merge, so every same-package-version warm cache + // replayed pre-feature captures and the feature was inert. 71 is the next + // free value above every claim at this merge — origin/main is 70 and open + // PR #3017 already claims 71, so 71 would have collided. + it('pins SCHEMA_BUMP to 72 so concurrent bumps cannot silently collide (#2766)', () => { + expect(Number(PARSE_CACHE_VERSION.split('+', 1)[0])).toBe(72); // The PREVIOUS version must fail the reuse gate, not merely differ from the // current one — a hardcoded number outside the conflict hunk rebases cleanly // while being wrong, which is exactly how the 37/38 exact clashes landed. // Every nearby historical or in-flight value is rejected, including 69, // which carried the route-table payload before this merge. - for (const taken of [59, 60, 61, 62, 63, 64, 65, 66, 67, 68, 69]) { + for (const taken of [59, 60, 61, 62, 63, 64, 65, 66, 67, 68, 69, 70, 71]) { expect(Number(PARSE_CACHE_VERSION.split('+', 1)[0])).not.toBe(taken); } }); diff --git a/gitnexus/test/unit/java-route-const-pipeline-e2e.test.ts b/gitnexus/test/unit/java-route-const-pipeline-e2e.test.ts index dad728579..4370edc31 100644 --- a/gitnexus/test/unit/java-route-const-pipeline-e2e.test.ts +++ b/gitnexus/test/unit/java-route-const-pipeline-e2e.test.ts @@ -19,16 +19,18 @@ * *Constants (the High bug) * src/main/java/com/example/UserController.java — @RequestMapping prefix + * @PostMapping(ApiPaths.X) + - * FQN form + inline concat + - * static import + * FQN form + concat over a + * static-imported bare ref * * Assertions (both runs): * - the emitted Route node carries the FOLDED literal path, not the expr; * - ALL THREE non-literal shapes survive (qualified, FQN-qualified, concat); * - a phantom `POST ` / empty path never appears (skip floor); - * - the warm run (parse-cache replay, no worker spawn) yields the IDENTICAL - * route set — the harvest result survives the structured-clone cache round - * trip (ModuleConstants uses Map, exercised through mapReplacer/mapReviver). + * - the warm run yields the IDENTICAL route set AND is a genuine replay + * (`usedWorkerPool === false`) — the harvest result survives the + * structured-clone cache round trip (ModuleConstants uses Map, exercised + * through mapReplacer/mapReviver). Asserting the route set alone would pass + * on a cache MISS that silently reparsed. * * Rebuild gate: this test requires dist/ to be current; when dist/ is stale * (older than src/) it self-skips with a loud message rather than silently @@ -42,13 +44,43 @@ import path from 'node:path'; import { createKnowledgeGraph } from '../../src/core/graph/graph.js'; import { runChunkedParseAndResolve } from '../../src/core/ingestion/pipeline-phases/parse-impl.js'; import { PARSE_CACHE_VERSION, type ParseCache } from '../../src/storage/parse-cache.js'; +import { + getDurableParsedFileDir, + pruneAndSaveDurableParsedFileStore, +} from '../../src/storage/parsedfile-store.js'; // ── dist freshness gate ─────────────────────────────────────────────────── +// The worker is one emitted file among many: TypeScript emits every module in +// this feature separately, so comparing dist/parse-worker.js against +// src/parse-worker.ts alone passes while the resolver, the Spring extractor or +// the provider behind it are stale — and the test then asserts against the +// PREVIOUS build's harvest. Gate on the newest mtime across every source this +// pipeline actually loads. const repoRoot = path.resolve(__dirname, '..', '..'); const distWorker = path.join(repoRoot, 'dist', 'core', 'ingestion', 'workers', 'parse-worker.js'); -const srcWorker = path.join(repoRoot, 'src', 'core', 'ingestion', 'workers', 'parse-worker.ts'); -const distStale = - !fs.existsSync(distWorker) || fs.statSync(distWorker).mtimeMs < fs.statSync(srcWorker).mtimeMs; +const GATED_SOURCES = [ + 'core/ingestion/workers/parse-worker.ts', + 'core/ingestion/route-extractors/java-const-resolver.ts', + 'core/ingestion/route-extractors/constant-resolver.ts', + 'core/ingestion/route-extractors/spring.ts', + 'core/ingestion/languages/java.ts', + 'core/ingestion/languages/python.ts', + 'core/ingestion/language-provider.ts', + 'core/ingestion/pipeline-phases/parse-impl.ts', +]; +const newestSourceMs = Math.max( + ...GATED_SOURCES.map((rel) => fs.statSync(path.join(repoRoot, 'src', rel)).mtimeMs), +); +const distStale = !fs.existsSync(distWorker) || fs.statSync(distWorker).mtimeMs < newestSourceMs; + +if (distStale) { + // `describe.skip` prints only vitest's ordinary skip marker, so without this + // the docblock's promised "loud message" did not exist and a stale/absent + // dist/ looked like a passing run. + console.warn( + '[#2980 e2e] SKIPPED: dist/ is missing or older than src/ — run `npm run build` to exercise the real pipeline.', + ); +} const maybeDescribe = distStale ? describe.skip : describe; @@ -65,6 +97,7 @@ public class ApiPaths { const USER_CONTROLLER = `package com.example; import com.example.common.ApiPaths; +import static com.example.common.ApiPaths.V1; import org.springframework.web.bind.annotation.RequestMapping; import org.springframework.web.bind.annotation.PostMapping; import org.springframework.web.bind.annotation.GetMapping; @@ -81,9 +114,11 @@ public class UserController { @GetMapping(com.example.common.ApiPaths.ORDERS) public void list() {} - // Inline concat with a static-import-style bare ref (same-file constant - // through the composed-operand fold). - @PostMapping(com.example.common.ApiPaths.V1 + "/orders") + // Inline concat with a STATIC-IMPORTED bare ref — the shape this fixture + // used to only claim: it spelled the operand as the full FQN chain, which + // just re-tested the FQN branch above, so bare-name resolution through the + // import table had no coverage anywhere in the suite. + @PostMapping(V1 + "/orders") public void createOrders() {} } `; @@ -158,6 +193,7 @@ maybeDescribe('#2980 provider-hook constant harvest — real pipeline (cold + wa }; const result = await runPipeline(cache, files); + expect(result.usedWorkerPool).toBe(true); const routes = foldedRoutesOf(result); // All three non-literal shapes resolve to folded literals. (The class-level @@ -186,16 +222,27 @@ maybeDescribe('#2980 provider-hook constant harvest — real pipeline (cold + wa onDiskKeys: new Set(), }; - // Run #1 populates the cache; persist it like run-analyze does. + // Run #1 populates the cache; persist it like run-analyze does — BOTH the + // chunk shards and the durable ParsedFile store. `slimParseWorkerResultsForCache` + // blanks `parsedFiles` before writing a shard, so a warm run without the + // durable store cannot replay the chunk and silently falls back to the + // workers — which is what this test used to do while still passing. const run1 = await runPipeline(cache, files); const { saveParseCache, pruneCache } = await import('../../src/storage/parse-cache.js'); pruneCache(cache, cache.usedKeys); - await saveParseCache(storageDir, cache); + const savedKeys = await saveParseCache(storageDir, cache); + expect(savedKeys.length).toBeGreaterThan(0); + await pruneAndSaveDurableParsedFileStore( + getDurableParsedFileDir(storageDir), + PARSE_CACHE_VERSION, + new Set(savedKeys), + ); // Run #2 — warm: every chunk is a cache HIT, no worker spawn, the cached // ParseWorkerResult (moduleConstants included) is replayed from disk. const { loadParseCache } = await import('../../src/storage/parse-cache.js'); const warm = await loadParseCache(storageDir); + expect(warm.onDiskKeys).toEqual(new Set(savedKeys)); const run2 = await runPipeline(warm, files); const cold = foldedRoutesOf(run1) @@ -206,5 +253,12 @@ maybeDescribe('#2980 provider-hook constant harvest — real pipeline (cold + wa .sort(); expect(hot).toEqual(cold); expect(hot.length).toBeGreaterThan(0); + // Without this the test proves nothing about the cache: `loadParseCache` + // returns an EMPTY cache on any failure (missing file, corrupt JSON, + // version mismatch) and never throws, so a broken Map round-trip through + // mapReplacer/mapReviver — the exact regression this test exists for — + // would silently reparse through the workers and produce the same routes. + expect(run1.usedWorkerPool).toBe(true); + expect(run2.usedWorkerPool).toBe(false); }, 120_000); }); diff --git a/gitnexus/test/unit/java-route-const-resolver.test.ts b/gitnexus/test/unit/java-route-const-resolver.test.ts index dca8de6db..0d1cb7c83 100644 --- a/gitnexus/test/unit/java-route-const-resolver.test.ts +++ b/gitnexus/test/unit/java-route-const-resolver.test.ts @@ -6,19 +6,28 @@ * fixtures missed the dominant real-world spelling — 1198 constant-ref * routes vs 2 literals in the real repo). * - * Real shapes covered (counts from the live repo): - * - `@WinPostMapping(ApiPathConstants.DIAGNOSIS_SAVE_V1)` — qualified ref, - * 1063 occurrences - * - `@WinPostMapping(value = ApiPathConstants.X)` / `(path = X)` — named + * Value shapes covered, spelled with the Spring annotations this branch + * actually recognises (`@PostMapping` & co., bare or fully qualified): + * - `@PostMapping(ApiPathConstants.DIAGNOSIS_SAVE_V1)` — qualified ref, the + * dominant real-world spelling (1063 occurrences in the source corpus) + * - `@PostMapping(value = ApiPathConstants.X)` / `(path = X)` — named * argument, 414+ occurrences - * - `@WinPostMapping(API_CIS_GET_TREATMENT_ORDER_V1)` — static-imported bare + * - `@PostMapping(API_CIS_GET_TREATMENT_ORDER_V1)` — static-imported bare * name, 79 files * - `public static final String API = OTHER + "suffix"` — composed constant * - interface constants (implicitly static final) - * - same-package simple-name collision handled by unique-suffix import - * resolution across Maven modules + * - escaped characters survive folding identically to the literal path + * - same-package simple-name collision floors to skip across Maven modules * - FQN-qualified annotation value (4 occurrences) * - unresolvable references floor to skip (never a phantom path) + * + * NOT covered, deliberately: the vendor alias `@WinPostMapping`. The corpus is + * dominated by it, but Spring alias recognition is an EXACT-NAME map + * (`spring-shared.ts`) on this base — there is no `*Mapping`-suffix rule, #2883 + * is still open — so `@PostMapping(...)` extracts zero routes here no matter + * how the constant folds. A fixture written in that spelling would be dead + * (one was, and CodeQL flagged it). Constant folding and alias recognition are + * independent: when #2883 lands, every shape below works unchanged for aliases. */ import { describe, expect, it } from 'vitest'; @@ -26,11 +35,15 @@ import Parser from 'tree-sitter'; import Java from 'tree-sitter-java'; import { extractJavaModuleConstants, + foldJavaOperands, + isJavaConstantFile, parseJavaConstOperands, resolveJavaConstant, resolveJavaImport, type RepoConstants, } from '../../src/core/ingestion/route-extractors/java-const-resolver.js'; +import { javaProvider } from '../../src/core/ingestion/languages/java.js'; +import { unquoteSpringLiteral } from '../../src/core/ingestion/route-extractors/spring-shared.js'; const parser = new Parser(); parser.setLanguage(Java); @@ -79,13 +92,13 @@ import com.winning.opt.diagnosis.api.constants.ApiPathConstants; public class DiagnosisController { - @WinPostMapping(ApiPathConstants.DIAGNOSIS_SAVE_V1) + @PostMapping(ApiPathConstants.DIAGNOSIS_SAVE_V1) public String save() { return "{}"; } - @WinPostMapping(value = ApiPathConstants.DIAGNOSIS_SAVE_V2) + @PostMapping(value = ApiPathConstants.DIAGNOSIS_SAVE_V2) public String saveV2() { return "{}"; } - @WinPostMapping(path = ApiPathConstants.API_CIS_SAVE_SUMMARY) + @PostMapping(path = ApiPathConstants.API_CIS_SAVE_SUMMARY) public String saveSummary() { return "{}"; } }`; @@ -95,7 +108,7 @@ import static com.winning.opt.diagnosis.api.constants.ApiPathConstants.DIAGNOSIS public class CisController { - @WinPostMapping(DIAGNOSIS_SAVE_V1) + @PostMapping(DIAGNOSIS_SAVE_V1) public String save() { return "{}"; } }`; @@ -105,16 +118,6 @@ public interface LabApiPath { String LAB_QUERY_V1 = "/api/v1/labtest/query"; }`; -const WIN_POST_MAPPING = `package com.winning.opt.annotations; - -public @interface WinPostMapping { - String value() default ""; - String path() default ""; -}`; - -// Fake minimal annotation so fixtures parse — the alias layer treats any -// *Mapping-suffixed annotation as a route annotation. - describe('extractJavaModuleConstants', () => { it('collects static final String literals with class-qualified aliases', () => { const mc = extractJavaModuleConstants(parse(CONSTANTS_FILE)); @@ -312,11 +315,11 @@ public class DemoController { public String save() { return "ok"; } }`); const routes = extractSpringRoutes(tree, 'DemoController.java', 0); - assert.strictEqual(routes.length, 1); - assert.strictEqual(routes[0].httpMethod, 'POST'); - assert.strictEqual(routes[0].routePathExpr, 'ApiPathConstants.DIAGNOSIS_SAVE_V1'); - assert.ok(routes[0].routePathOperands && routes[0].routePathOperands.length > 0); - assert.strictEqual(routes[0].routePath, ''); + expect(routes.length).toBe(1); + expect(routes[0].httpMethod).toBe('POST'); + expect(routes[0].routePathExpr).toBe('ApiPathConstants.DIAGNOSIS_SAVE_V1'); + expect(routes[0].routePathOperands && routes[0].routePathOperands.length > 0).toBeTruthy(); + expect(routes[0].routePath).toBe(''); }); it('keeps literal routes unchanged', async () => { @@ -329,9 +332,9 @@ public class DemoController { public String save() { return "ok"; } }`); const routes = extractSpringRoutes(tree, 'DemoController.java', 0); - assert.strictEqual(routes.length, 1); - assert.strictEqual(routes[0].routePath, '/literal/path'); - assert.strictEqual(routes[0].routePathExpr, undefined); + expect(routes.length).toBe(1); + expect(routes[0].routePath).toBe('/literal/path'); + expect(routes[0].routePathExpr).toBe(undefined); }); }); @@ -339,6 +342,7 @@ describe('qualified-ref recursion cycle guard (maintainer point 5)', () => { it('self-import: qualified self-reference terminates with null, not a stack overflow', () => { const repo = repoOf({ 'src/main/java/com/example/SelfConsts.java': `package com.example; +import com.example.SelfConsts; public class SelfConsts { public static final String X = SelfConsts.X + "/x"; }`, @@ -509,3 +513,252 @@ public class Ctl { ).toBe('/api/v1/users'); }); }); + +describe('escaped characters survive folding (review P1)', () => { + // tree-sitter-java splits a string_literal AROUND its escape_sequence + // children, so a string_fragment-only join silently deleted every escape: + // the standard Spring path-variable constraint `{id:\\d+}` folded to + // `{id:d+}` and a pure-escape literal folded to ''. Worse, the LITERAL path + // keeps escapes verbatim, so one Java route had two spellings. + const cases = [ + ['"/user/{id:\\d+}"', '/user/{id:\\d+}'], + ['"/a\\tb"', '/a\\tb'], + ['"/a\\u002Fb"', '/a\\u002Fb'], + ['"\\t"', '\\t'], + ['""', ''], + ['"/plain"', '/plain'], + ] as const; + + it.each(cases)('keeps %s intact through the constant path', (literal, expected) => { + const mc = extractJavaModuleConstants( + parse(`public class C { public static final String X = ${literal}; }`), + ); + expect(mc.literals.get('X')).toBe(expected); + }); + + it.each(cases)('agrees with the literal path for %s', (literal, expected) => { + // The constant path and `unquoteSpringLiteral` (what a literal-valued + // @GetMapping goes through) must produce the SAME string, or the graph + // carries two irreconcilable spellings of one route. + expect(unquoteSpringLiteral(literal)).toBe(expected); + }); +}); + +describe('a non-foldable rebind drops the static import too (review P1)', () => { + it('returns null rather than the shadowed imported value', () => { + const repo = repoOf({ + 'src/main/java/com/x/Base.java': `package com.x; +public class Base { public static final String PATH = "/WRONG-imported"; }`, + 'src/main/java/com/y/C.java': `package com.y; +import static com.x.Base.PATH; +public class C { public static final String PATH = compute(); }`, + }); + // A local `static final` shadows a static import of the same simple name + // inside that class (JLS 6.4.1), so the only correct answer is + // "unresolvable". Leaving the import alive made the fold fall through to + // it and return the imported literal — a wrong path where the skip floor + // is owed (#2393's Python defect, reproduced for Java). + expect(repo.get('src/main/java/com/y/C.java')!.imports.has('PATH')).toBe(false); + expect( + foldJavaOperands('src/main/java/com/y/C.java', [{ kind: 'ref', name: 'PATH' }], repo), + ).toBeNull(); + }); + + it('the drop is file-scoped: a sibling class floors to skip, never to a wrong value', () => { + // These maps are file-level by design (nested types flatten into one + // namespace), so dropping the import costs a sibling class that + // legitimately uses it. javac would answer `/imported/b` here; we answer + // null. Pinned deliberately — the alternative direction is a wrong path. + const repo = repoOf({ + 'src/main/java/com/x/Base.java': `package com.x; +public class Base { public static final String PATH = "/imported"; }`, + 'src/main/java/com/y/Two.java': `package com.y; +import static com.x.Base.PATH; +class A { public static final String PATH = compute(); } +class B { public static final String USE = PATH + "/b"; }`, + }); + expect( + foldJavaOperands('src/main/java/com/y/Two.java', [{ kind: 'ref', name: 'B.USE' }], repo), + ).toBeNull(); + }); + + it('a FOLDABLE rebind still wins over the import', () => { + const repo = repoOf({ + 'src/main/java/com/x/Base.java': `package com.x; +public class Base { public static final String PATH = "/imported"; }`, + 'src/main/java/com/y/C.java': `package com.y; +import static com.x.Base.PATH; +public class C { public static final String PATH = "/local"; }`, + }); + expect( + foldJavaOperands('src/main/java/com/y/C.java', [{ kind: 'ref', name: 'PATH' }], repo), + ).toBe('/local'); + }); +}); + +describe('isJavaConstantFile — one gate, both subsystems (review P1)', () => { + // The ingestion provider and the group extractor's prepareRepo pre-pass used + // to spell this gate differently. A constant INTERFACE passed the group's and + // failed ingestion's, so the group published a provider contract while the + // graph got no Route node — an R4 parity break in the losing direction. + const shapes = [ + [ + 'constant interface (implicitly static final, no import)', + `package com.x; +public interface ApiPathConstants { String SAVE = "/api/v1/save"; }`, + ], + [ + 'lowercase interface name', + `package com.x; +public interface apiPaths { String SAVE = "/api/v1/save"; }`, + ], + [ + 'reversed modifier order', + `package com.x; +public class P { public final static String SAVE = "/api/v1/save"; }`, + ], + [ + 'conventional order', + `package com.x; +public class P { public static final String SAVE = "/api/v1/save"; }`, + ], + ] as const; + + it.each(shapes)('admits %s on BOTH sides', (_name, src) => { + expect(isJavaConstantFile(src)).toBe(true); + // The provider hook is what the parse worker actually calls — drive it, + // not just the regex, so the gate itself is covered and not only the + // extractor behind it. + expect(javaProvider.moduleConstantHeuristic?.(src)).toBe(true); + expect(extractJavaModuleConstants(parse(src)).literals.get('SAVE')).toBe('/api/v1/save'); + }); + + it('still skips a file with no constant-bearing syntax', () => { + const src = `package com.x; +public class P { void run() { System.out.println("/not-a-constant"); } }`; + expect(isJavaConstantFile(src)).toBe(false); + }); +}); + +describe('resolveJavaImport honours the documented skip floor (review P2)', () => { + it('returns null when the same package+class exists in two modules', () => { + // A nearest-shared-directory tie-break used to pick one. javac resolves + // duplicate FQNs by classpath order, so proximity can hand back a + // src/test fixture copy — a silently wrong literal in a resolver whose + // contract is skip-or-correct. + const keys = new Set([ + 'svc-order/src/main/java/com/x/ApiPaths.java', + 'svc-user/src/main/java/com/x/ApiPaths.java', + ]); + expect( + resolveJavaImport( + 'svc-order/src/main/java/com/x/web/OrderController.java', + 'com.x.ApiPaths', + keys, + ), + ).toBeNull(); + }); + + it('still resolves a unique full-suffix match', () => { + const keys = new Set([ + 'svc-order/src/main/java/com/x/ApiPaths.java', + 'svc-user/src/main/java/com/y/ApiPaths.java', + ]); + expect( + resolveJavaImport( + 'svc-order/src/main/java/com/x/web/OrderController.java', + 'com.x.ApiPaths', + keys, + ), + ).toBe('svc-order/src/main/java/com/x/ApiPaths.java'); + }); +}); + +describe('enum and record constants are collected', () => { + it.each([ + ['enum', 'public enum E { A, B; public static final String P = "/e"; }', 'E'], + ['record', 'public record R(int x) { public static final String P = "/r"; }', 'R'], + ])('harvests a static final String declared in a %s', (_kind, src, owner) => { + const mc = extractJavaModuleConstants(parse(src)); + expect(mc.literals.get('P')).toBe(src.includes('enum') ? '/e' : '/r'); + expect(mc.literals.get(`${owner}.P`)).toBe(src.includes('enum') ? '/e' : '/r'); + }); + + it('does not harvest a non-static field of a record', () => { + const mc = extractJavaModuleConstants(parse('public record R(int x) { String p = "/r"; }')); + expect(mc.literals.has('p')).toBe(false); + }); +}); + +describe('constants composed across files through a qualified ref', () => { + it('folds `X = BConsts.Y + "/tail"` across the import', () => { + // Operands found INSIDE an initializer used to go straight to the agnostic + // fold, which only knows bare names — so a qualified operand missed and + // floored the whole chain to null, even acyclically. + const repo = repoOf({ + 'src/com/example/AConsts.java': `package com.example; +import com.example.BConsts; +public class AConsts { public static final String X = BConsts.Y + "/tail"; }`, + 'src/com/example/BConsts.java': `package com.example; +public class BConsts { public static final String Y = "/y"; }`, + }); + expect(resolveJavaConstant('src/com/example/AConsts.java', 'X', repo)).toBe('/y/tail'); + expect( + foldJavaOperands('src/com/example/AConsts.java', [{ kind: 'ref', name: 'AConsts.X' }], repo), + ).toBe('/y/tail'); + }); + + it('a missing link in the chain still floors to null', () => { + const repo = repoOf({ + 'src/com/example/AConsts.java': `package com.example; +import com.example.BConsts; +public class AConsts { public static final String X = BConsts.MISSING + "/tail"; }`, + 'src/com/example/BConsts.java': `package com.example; +public class BConsts { public static final String Y = "/y"; }`, + }); + expect(resolveJavaConstant('src/com/example/AConsts.java', 'X', repo)).toBeNull(); + }); +}); + +describe('the fold is bounded in time as well as depth', () => { + it('folds a 30-level shared-descendant DAG instead of exploring 2^30 paths', () => { + // `X_k = X_{k+1} + X_{k+1}` re-folds each child once per reference without a + // memo — O(2^depth). MAX_FOLD_LENGTH cannot save it here because every + // intermediate value is the EMPTY string, so nothing ever accumulates. + // Un-memoized this took 2.7 s at 26 levels and 11 s at 28, on the main + // thread, for one route. The assertion is the explicit timeout below: a + // regression does not fail this test slowly, it fails it. + const lines = ['public static final String X30 = "";']; + for (let i = 29; i >= 0; i--) { + lines.push(`public static final String X${i} = X${i + 1} + X${i + 1};`); + } + const repo = repoOf({ 'C.java': `public class C {\n${lines.join('\n')}\n}` }); + expect(resolveJavaConstant('C.java', 'X0', repo)).toBe(''); + }, 5_000); + + it('still caps a chain that genuinely produces a huge string', () => { + const lines = ['public static final String X30 = "a";']; + for (let i = 29; i >= 0; i--) { + lines.push(`public static final String X${i} = X${i + 1} + X${i + 1};`); + } + const repo = repoOf({ 'C.java': `public class C {\n${lines.join('\n')}\n}` }); + expect(resolveJavaConstant('C.java', 'X0', repo)).toBeNull(); + }, 5_000); +}); + +describe('text blocks keep the skip floor', () => { + it('does not fold a text-block constant into a path with newlines and indentation', () => { + // `unquoteSpringLiteral` has a `"""` arm that slices 3/-3, which would hand + // back the raw block — leading newline and incidental indentation included, + // both of which Java strips — and nothing downstream normalizes it. The old + // fragment-join returned '' here, i.e. a skip; keep the skip. + const src = [ + 'public class C {', + ' public static final String X = """', + ' /api/v1/tb', + ' """;', + '}', + ].join('\n'); + expect(extractJavaModuleConstants(parse(src)).literals.has('X')).toBe(false); + }); +}); diff --git a/gitnexus/test/unit/python-const-resolver.test.ts b/gitnexus/test/unit/python-const-resolver.test.ts index 1f850567e..2a98b926d 100644 --- a/gitnexus/test/unit/python-const-resolver.test.ts +++ b/gitnexus/test/unit/python-const-resolver.test.ts @@ -23,6 +23,7 @@ import { type ImportBinding, type RepoConstants, } from '../../src/core/ingestion/route-extractors/python-const-resolver.js'; +import { pythonProvider } from '../../src/core/ingestion/languages/python.js'; const lit = (value: string): Operand => ({ kind: 'literal', value }); const ref = (name: string): Operand => ({ kind: 'ref', name }); @@ -377,3 +378,28 @@ describe('extractPythonModuleConstants — source-order snapshot (#2393)', () => expect(resolveConstant('m.py', 'C', r)).toBe('/a/b/c'); }); }); + +describe('the Python provider harvests unconditionally (#2980 review P2)', () => { + // A cheap content gate was added on the provider here and removed on review. + // It required NAME immediately followed by `=`, so it silently dropped the + // idiomatic typed-FastAPI shapes and every composed constant whose RHS starts + // with an identifier — i.e. it REGRESSED routes that already resolve on main. + // The parse worker treats a missing heuristic as "harvest"; pin that here so + // the gate cannot come back without a decision. + it('declares no moduleConstantHeuristic', () => { + expect(pythonProvider.moduleConstantHeuristic).toBeUndefined(); + }); + + it.each([ + ['plain', 'API = "/api/v1"\nUSERS = API + "/users"\n'], + ['PEP 526 annotated', 'API: str = "/api/v1"\nUSERS: str = API + "/users"\n'], + [ + 'Final-annotated', + 'from typing import Final\nAPI: Final[str] = "/api/v1"\nUSERS: Final[str] = API + "/users"\n', + ], + ['composed, identifier RHS', 'API = _base()\nUSERS = API + "/users"\n'], + ])('still reaches the extractor for the %s shape', (_name, src) => { + const mc = extract(src); + expect(mc.literals.size + mc.exprs.size + mc.imports.size).toBeGreaterThan(0); + }); +});