From d2f768245ec3ce5c5d4e9afd78318aaa106598e6 Mon Sep 17 00:00:00 2001 From: ChunxueLi Date: Fri, 21 Aug 2026 01:18:50 +0800 Subject: [PATCH] =?UTF-8?q?fix(routes):=20address=20round-2=20review=20?= =?UTF-8?q?=E2=80=94=20provider=20hooks,=20FQN=20fold,=20interface=20nesti?= =?UTF-8?q?ng?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit F1 (High): production harvest silently dropped routes when the constants class is not named *Constants (e.g. ApiPaths). The content gate is now SYNTAX-driven (static-final String field or any class import) and lives in the provider (moduleConstantHeuristic), not a shared-layer regex. F2: shared ingestion layers no longer branch on language. The harvest and the qualified-ref fold run through new provider hooks (extractModuleConstants / foldRoutePathOperands); parse-impl resolves the provider by filePath (getProviderForFile). Python wires the same hooks for architecture parity. F3: multi-segment FQN chains (com.example.ApiPaths.USERS) now flatten recursively; verified via tree-sitter that the existing query already captures the whole nested field_access — the gap was resolver-side only. F4: implicit-final interface semantics no longer leak into nested classes at type boundaries (JLS 9.5). F5: nested same-name shadowing now drops the stale entry (rebind-drop, matching Python #2391 semantics) instead of keeping the first binding. Tests: 9 new unit tests (27/27) + real-pipeline e2e over a reviewer-shaped fixture (non-*Constants class, cold run + warm parse-cache replay) — the exact production gap unit tests missed. --- gitnexus/package-lock.json | 30 --- .../src/core/ingestion/language-provider.ts | 51 +++++ gitnexus/src/core/ingestion/languages/java.ts | 19 ++ .../src/core/ingestion/languages/python.ts | 10 + .../ingestion/pipeline-phases/parse-impl.ts | 13 +- .../route-extractors/java-const-resolver.ts | 85 ++++++-- .../core/ingestion/workers/parse-worker.ts | 37 +--- .../java-route-const-pipeline-e2e.test.ts | 206 ++++++++++++++++++ .../unit/java-route-const-resolver.test.ts | 130 +++++++++++ 9 files changed, 500 insertions(+), 81 deletions(-) create mode 100644 gitnexus/test/unit/java-route-const-pipeline-e2e.test.ts diff --git a/gitnexus/package-lock.json b/gitnexus/package-lock.json index e4a0624ad..189e898a2 100644 --- a/gitnexus/package-lock.json +++ b/gitnexus/package-lock.json @@ -1660,9 +1660,6 @@ "arm64" ], "dev": true, - "libc": [ - "glibc" - ], "license": "MIT", "optional": true, "os": [ @@ -1680,9 +1677,6 @@ "arm64" ], "dev": true, - "libc": [ - "musl" - ], "license": "MIT", "optional": true, "os": [ @@ -1700,9 +1694,6 @@ "ppc64" ], "dev": true, - "libc": [ - "glibc" - ], "license": "MIT", "optional": true, "os": [ @@ -1720,9 +1711,6 @@ "s390x" ], "dev": true, - "libc": [ - "glibc" - ], "license": "MIT", "optional": true, "os": [ @@ -1740,9 +1728,6 @@ "x64" ], "dev": true, - "libc": [ - "glibc" - ], "license": "MIT", "optional": true, "os": [ @@ -1760,9 +1745,6 @@ "x64" ], "dev": true, - "libc": [ - "musl" - ], "license": "MIT", "optional": true, "os": [ @@ -3893,9 +3875,6 @@ "arm64" ], "dev": true, - "libc": [ - "glibc" - ], "license": "MPL-2.0", "optional": true, "os": [ @@ -3917,9 +3896,6 @@ "arm64" ], "dev": true, - "libc": [ - "musl" - ], "license": "MPL-2.0", "optional": true, "os": [ @@ -3941,9 +3917,6 @@ "x64" ], "dev": true, - "libc": [ - "glibc" - ], "license": "MPL-2.0", "optional": true, "os": [ @@ -3965,9 +3938,6 @@ "x64" ], "dev": true, - "libc": [ - "musl" - ], "license": "MPL-2.0", "optional": true, "os": [ diff --git a/gitnexus/src/core/ingestion/language-provider.ts b/gitnexus/src/core/ingestion/language-provider.ts index ec9006450..3aa92ce37 100644 --- a/gitnexus/src/core/ingestion/language-provider.ts +++ b/gitnexus/src/core/ingestion/language-provider.ts @@ -39,6 +39,11 @@ import type { CfgVisitor } from './cfg/types.js'; import type { NodeLabel } from 'gitnexus-shared'; import type { ExtractedRoute } from './route-extractors/laravel.js'; import type { SharedSpringType } from './route-extractors/spring-shared.js'; +import type { + ModuleConstants, + Operand, + RepoConstants, +} from './route-extractors/constant-resolver.js'; import type Parser from 'tree-sitter'; import type { ExtractedDecoratorRoute } from './workers/parse-worker.js'; @@ -336,6 +341,52 @@ interface LanguageProviderConfig { filePath: string, ) => SharedSpringType[]; + /** + * Harvest this file's module-level string constants (#2391 core, #2980 Java + * parity) into the language-agnostic {@link ModuleConstants} shape, so the + * 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 extraction yields something resolvable (a literal, an expression, or + * an import binding), keeping the aggregate bounded on large repos. + * + * Default: undefined (no constant harvest; non-literal route paths of this + * language floor to skip). + */ + readonly extractModuleConstants?: (tree: Parser.Tree) => ModuleConstants; + + /** + * Cheap content heuristic deciding whether the worker should run + * {@link extractModuleConstants} on a file. Guards the harvest cost on huge + * 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. + */ + readonly moduleConstantHeuristic?: (content: string) => boolean; + + /** + * Fold one file's non-literal route-path operand list + * (`routePathExpr`/`routePathOperands` of an `ExtractedDecoratorRoute`) + * against the repo-wide, file-path-keyed constant map, or null when it cannot + * be fully folded (skip floor — never a phantom path). Languages whose + * qualified refs resolve through class imports (`Outer.CONST`, + * `com.example.ApiPaths.USERS`) need this hook because the shared fold has no + * notion of qualified names; Python's bare-name refs use the shared default. + * + * Default: undefined (the parse phase falls back to the shared + * language-agnostic operand fold). + */ + readonly foldRoutePathOperands?: ( + filePath: string, + operands: readonly Operand[], + repo: RepoConstants, + ) => string | null; + // ── Noise filtering ──────────────────────────────────────────────── /** Built-in/stdlib names that should be filtered from the call graph for this language. * Default: undefined (no language-specific filtering). */ diff --git a/gitnexus/src/core/ingestion/languages/java.ts b/gitnexus/src/core/ingestion/languages/java.ts index 317a853ac..7d8726917 100644 --- a/gitnexus/src/core/ingestion/languages/java.ts +++ b/gitnexus/src/core/ingestion/languages/java.ts @@ -15,6 +15,10 @@ import type { AstFrameworkPatternConfig } from '../language-provider.js'; import { createLeadingDocDescriptionExtractor } from '../utils/ast-helpers.js'; import { javaTypeConfig } from '../type-extractors/jvm.js'; import { extractSpringRoutes, extractSpringTypes } from '../route-extractors/spring.js'; +import { + extractJavaModuleConstants, + foldJavaOperands, +} from '../route-extractors/java-const-resolver.js'; import { javaExportChecker } from '../export-detection.js'; import { createImportResolver } from '../import-resolvers/resolver-factory.js'; import { javaImportConfig } from '../import-resolvers/configs/jvm.js'; @@ -216,4 +220,19 @@ export const javaProvider = defineLanguage({ // ── Route extraction ── extractDecoratorRoutes: extractSpringRoutes, extractRouteInheritanceTypes: extractSpringTypes, + + // ── #2980: constant harvest + qualified-ref fold for non-literal mapping + // paths (`@WinPostMapping(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, + moduleConstantHeuristic: (content) => + /\bstatic\s+final\s+String\s/.test(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. + /\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 f9c345b4b..7aacdc0c6 100644 --- a/gitnexus/src/core/ingestion/languages/python.ts +++ b/gitnexus/src/core/ingestion/languages/python.ts @@ -44,6 +44,7 @@ import { } from './python/index.js'; import { extractDjangoRoutes } from '../route-extractors/django.js'; import { discoverDjangoRootUrls } from '../route-extractors/django-root-discovery.js'; +import { extractPythonModuleConstants } from '../route-extractors/python-const-resolver.js'; const BUILT_INS: ReadonlySet = new Set([ 'print', @@ -158,4 +159,13 @@ export const pythonProvider = defineLanguage({ receiverBinding: pythonReceiverBinding, arityCompatibility: pythonArityCompatibility, resolveImportTarget: resolvePythonImportTarget, + + // ── #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). + 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/pipeline-phases/parse-impl.ts b/gitnexus/src/core/ingestion/pipeline-phases/parse-impl.ts index 64f246121..0f4ba1b2e 100644 --- a/gitnexus/src/core/ingestion/pipeline-phases/parse-impl.ts +++ b/gitnexus/src/core/ingestion/pipeline-phases/parse-impl.ts @@ -60,7 +60,7 @@ import { createParserForLanguage, } from '../../tree-sitter/parser-loader.js'; import { parseSourceSafe } from '../../tree-sitter/safe-parse.js'; -import { getProvider, providers } from '../languages/index.js'; +import { getProvider, getProviderForFile, providers } from '../languages/index.js'; import { SCOPE_RESOLVERS } from '../scope-resolution/pipeline/registry.js'; import { DATA_ROUTE_TABLE_SOURCE } from '../route-extractors/data-route-table.js'; import type Parser from 'tree-sitter'; @@ -92,7 +92,6 @@ import { resolveOperands, type ModuleConstants, } from '../route-extractors/python-const-resolver.js'; -import { foldJavaOperands } from '../route-extractors/java-const-resolver.js'; import { resolveInheritedSpringRoutes, type SharedSpringType, @@ -1304,10 +1303,14 @@ export async function runChunkedParseAndResolve( resolvedRoutes.push(dr); continue; } - const isJavaRoute = dr.filePath.endsWith('.java'); + // Provider-driven fold (#2980): languages with qualified-ref semantics + // (Java `ApiPaths.X` / `com.example.ApiPaths.X`) fold through their + // provider hook; everything else uses the shared language-agnostic + // operand fold. No language names in the shared layer. + const fold = getProviderForFile(dr.filePath)?.foldRoutePathOperands; const value = dr.routePathOperands - ? isJavaRoute - ? foldJavaOperands(dr.filePath, dr.routePathOperands, repoConstants) + ? fold + ? fold(dr.filePath, dr.routePathOperands, repoConstants) : resolveOperands(dr.filePath, dr.routePathOperands, repoConstants) : null; if (value === null) { 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 0629bd177..436fc42ce 100644 --- a/gitnexus/src/core/ingestion/route-extractors/java-const-resolver.ts +++ b/gitnexus/src/core/ingestion/route-extractors/java-const-resolver.ts @@ -133,6 +133,24 @@ function stringLiteralValue(node: Parser.SyntaxNode): string | null { return parts.map((c) => c.text).join(''); } +/** + * Flatten a qualified-name expression (`ApiPaths`, `com.example.ApiPaths`) to + * its dotted text, or null when any segment is not a plain identifier (calls, + * `this`, array access, generics — not a static constant shape). + */ +function flattenQualifiedIdentifier(node: Parser.SyntaxNode): string | null { + if (node.type === 'identifier') return node.text; + if (node.type === 'field_access') { + const object = node.childForFieldName('object'); + const field = node.childForFieldName('field'); + if (object && field) { + const head = flattenQualifiedIdentifier(object); + return head === null ? null : `${head}.${field.text}`; + } + } + return null; +} + /** * Parse a Java constant initializer into an operand list, or null when it is * not a foldable string expression. Handles a bare string literal, a bare @@ -155,12 +173,17 @@ export function parseJavaConstOperands( if (node.type === 'identifier') { return [{ kind: 'ref', name: node.text }]; } - // `CONSTS.FIELD` — field_access in tree-sitter-java for expressions. + // `CONSTS.FIELD` — field_access in tree-sitter-java for expressions. The + // object side may itself be a chain (`com.example.ApiPaths` parses as + // nested field_access), so flatten recursively: every segment must be a + // plain identifier/keyword to qualify (a call `f().X`, `this.X`, or an + // array access object side is not a constant shape → null, skip floor). if (node.type === 'field_access') { const object = node.childForFieldName('object'); const field = node.childForFieldName('field'); - if (object && field && object.type === 'identifier') { - return [{ kind: 'ref', name: `${object.text}.${field.text}` }]; + if (object && field) { + const objectName = flattenQualifiedIdentifier(object); + if (objectName !== null) return [{ kind: 'ref', name: `${objectName}.${field.text}` }]; } return null; } @@ -261,25 +284,45 @@ export function extractJavaModuleConstants(tree: Parser.Tree): ModuleConstants { const nameNode = decl.childForFieldName('name'); const valueNode = decl.childForFieldName('value'); if (!nameNode) continue; - const operands = parseJavaConstOperands(valueNode); - if (operands === null) continue; const name = nameNode.text; - // Java guarantees one initializer per `static final` field (duplicate - // declarations are compile errors), so a redeclaration cannot smuggle - // a stale value past a non-foldable one — no shadowing cleanup needed - // (unlike Python, where #2391 drops rebinding names from the map). - if (operands.length === 1 && operands[0].kind === 'literal') { - literals.set(name, (operands[0] as { value: string }).value); + const operands = parseJavaConstOperands(valueNode); + // Same-name shadowing across nested types (legal Java, unlike + // same-class redeclaration): a later binding must REPLACE the earlier + // flattened simple-name entry — including dropping it to unresolvable + // when the new initializer is not foldable (`X = compute()`) — rather + // than leave the stale outer literal resolvable. Skip floor, mirroring + // Python #2391's rebind-drop. Qualified `Class.FIELD` aliases are + // per-type-keyed but same-named nested types can still collide, so + // they get the same replace/drop treatment. + const qname = declaringClass ? `${declaringClass}.${name}` : null; + if (operands === null) { + literals.delete(name); + exprs.delete(name); + if (qname) { + literals.delete(qname); + exprs.delete(qname); + } + continue; + } + const literalValue = + operands.length === 1 && operands[0].kind === 'literal' + ? (operands[0] as { value: string }).value + : null; + if (literalValue !== null) { + literals.set(name, literalValue); + exprs.delete(name); } else { exprs.set(name, operands); + literals.delete(name); } // Qualified alias: `CONSTS.X` refs (folded refs carry the class name). - if (declaringClass) { - const qname = `${declaringClass}.${name}`; - if (operands.length === 1 && operands[0].kind === 'literal') { - literals.set(qname, (operands[0] as { value: string }).value); + if (qname) { + if (literalValue !== null) { + literals.set(qname, literalValue); + exprs.delete(qname); } else { exprs.set(qname, operands); + literals.delete(qname); } } } @@ -296,9 +339,15 @@ export function extractJavaModuleConstants(tree: Parser.Tree): ModuleConstants { const body = child.children.find( (c) => c.type === 'class_body' || c.type === 'interface_body', ); - if (body && className) - collectFieldConstants(body, isInterface || insideInterface, className); - if (body) walkTypes(body, isInterface || insideInterface); + // 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 { diff --git a/gitnexus/src/core/ingestion/workers/parse-worker.ts b/gitnexus/src/core/ingestion/workers/parse-worker.ts index ee698b6e7..3e6dd5079 100644 --- a/gitnexus/src/core/ingestion/workers/parse-worker.ts +++ b/gitnexus/src/core/ingestion/workers/parse-worker.ts @@ -1421,12 +1421,10 @@ export function extractORMQueries( import { extractFastAPIRouterBindings } from '../route-extractors/fastapi-router-bindings.js'; import { - extractPythonModuleConstants, parseConstOperands, type ModuleConstants, type Operand, } from '../route-extractors/python-const-resolver.js'; -import { extractJavaModuleConstants } from '../route-extractors/java-const-resolver.js'; /** * Report a non-fatal worker issue to the pool over IPC so a caught error is not @@ -2964,34 +2962,17 @@ const processFileGroup = ( (result.routerModuleAliases ??= []), (result.routerConstructorPrefixes ??= []), ); - // #2391: harvest module-level string constants + from-imports so parse-impl - // can resolve non-literal decorator route paths cross-file. Only emit for - // files that carry something resolvable (a constant definition or an import - // binding) to keep the aggregate bounded on large repos. - const constants = extractPythonModuleConstants(tree); - if (constants.literals.size > 0 || constants.exprs.size > 0 || constants.imports.size > 0) { - (result.moduleConstants ??= []).push({ filePath: file.path, constants }); - } } - // Java parity of the #2391 constant harvest: static-final String fields + - // class/static imports, folded cross-file by parse-impl for non-literal - // Spring mapping paths (`@WinPostMapping(ApiPathConstants.SAVE_V1)`). - // Cost-gated on file content — a file with no `static final String` and no - // constants-bearing import is not parsed for constants. - if (language === SupportedLanguages.Java) { - if ( - /static\s+final\s+String\s/.test(parseContent) || - /import\s+(static\s+)?[\w.]*Constants/.test(parseContent) - ) { - const javaConstants = extractJavaModuleConstants(tree); - if ( - javaConstants.literals.size > 0 || - javaConstants.exprs.size > 0 || - javaConstants.imports.size > 0 - ) { - (result.moduleConstants ??= []).push({ filePath: file.path, constants: javaConstants }); - } + // #2391/#2980: harvest module-level string constants + import bindings via + // the provider hook so parse-impl can resolve non-literal decorator route + // 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)) { + 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/test/unit/java-route-const-pipeline-e2e.test.ts b/gitnexus/test/unit/java-route-const-pipeline-e2e.test.ts new file mode 100644 index 000000000..42b5afec1 --- /dev/null +++ b/gitnexus/test/unit/java-route-const-pipeline-e2e.test.ts @@ -0,0 +1,206 @@ +/** + * #2980 review round-2: COLD and WARM pipeline e2e for the provider-hook + * constant harvest (`extractModuleConstants` / `moduleConstantHeuristic` / + * `foldRoutePathOperands`). + * + * The maintainer's blocking finding: unit tests only exercised worker-gated + * helpers — never the REAL pipeline. A controller referencing constants from + * a class NOT named `*Constants` (e.g. `ApiPaths`) was silently dropped: + * the old content gate `/import ... [\\w.]*Constants/` never matched, the + * constants file never entered the import map, the route resolved to null and + * got skipped. + * + * This file drives the REAL `runChunkedParseAndResolve` with the REAL compiled + * dist worker (vitest auto-falls back to dist/core/ingestion/workers/ + * parse-worker.js) over a fixture repo shaped like the reviewer's example: + * + * repo/ + * src/main/java/com/example/ApiPaths.java — constants class NOT named + * *Constants (the High bug) + * src/main/java/com/example/UserController.java — @RequestMapping prefix + + * @PostMapping(ApiPaths.X) + + * FQN form + inline concat + + * static import + * + * 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). + * + * 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 + * asserting against the old binary. (CI builds before vitest, so it runs.) + */ +import { beforeEach, afterEach, describe, expect, it } from 'vitest'; +import fs from 'node:fs'; +import os from 'node:os'; +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'; + +// ── dist freshness gate ─────────────────────────────────────────────────── +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 maybeDescribe = distStale ? describe.skip : describe; + +// ── fixture repo (reviewer's exact High-finding shape) ──────────────────── +const API_PATHS = `package com.example.common; + +public class ApiPaths { + public static final String USERS = "/api/v1/users"; + public static final String ORDERS = "/api/v1/orders"; + public static final String V1 = "/api/v1"; +} +`; + +const USER_CONTROLLER = `package com.example; + +import com.example.common.ApiPaths; +import org.springframework.web.bind.annotation.RequestMapping; +import org.springframework.web.bind.annotation.PostMapping; +import org.springframework.web.bind.annotation.GetMapping; + +@RequestMapping("/users") +public class UserController { + + // Qualified ref via a class NOT named *Constants (High finding): the old + // gate dropped the whole route because ApiPaths fails the name pattern. + @PostMapping(ApiPaths.USERS) + public void create() {} + + // FQN-qualified form (F3): multi-segment field_access chain. + @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") + public void createOrders() {} +} +`; + +let repoDir: string; +let storageDir: string; + +function writeFixture(): { path: string; size: number }[] { + const files: [string, string][] = [ + ['src/main/java/com/example/common/ApiPaths.java', API_PATHS], + ['src/main/java/com/example/UserController.java', USER_CONTROLLER], + ]; + const out: { path: string; size: number }[] = []; + for (const [rel, content] of files) { + const full = path.join(repoDir, rel); + fs.mkdirSync(path.dirname(full), { recursive: true }); + fs.writeFileSync(full, content); + out.push({ path: rel, size: Buffer.byteLength(content) }); + } + return out; +} + +/** + * The parse phase does not emit Route nodes itself — it returns the folded + * `decoratorRoutes` (the routes phase emits them downstream). Asserting on the + * folded paths at THIS seam is exactly the regression the maintainer asked + * for: the worker's harvest → provider heuristic → parse-impl fold, with the + * real dist worker. + */ +type PipelineResult = Awaited>; + +function foldedRoutesOf(result: PipelineResult): Array<{ path: string; method: string }> { + return (result.allDecoratorRoutes ?? []) + .filter((r) => typeof r.routePath === 'string') + .map((r) => ({ path: r.routePath, method: r.httpMethod })); +} + +async function runPipeline( + cache: ParseCache, + files: { path: string; size: number }[], +): Promise { + const kg = createKnowledgeGraph(); + return await runChunkedParseAndResolve( + kg, + files, + files.map((f) => f.path), + files.length, + repoDir, + Date.now(), + () => {}, + { workerPoolSize: 1, parseCache: cache }, + ); +} + +maybeDescribe('#2980 provider-hook constant harvest — real pipeline (cold + warm)', () => { + beforeEach(() => { + repoDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gnx-2980-cold-')); + storageDir = path.join(repoDir, '.gitnexus'); + }); + afterEach(() => { + for (const d of [repoDir]) fs.rmSync(d, { recursive: true, force: true }); + }); + + it('cold run: folds qualified / FQN / concat paths from a non-*Constants class', async () => { + const files = writeFixture(); + const cache: ParseCache = { + version: PARSE_CACHE_VERSION, + entries: new Map(), + usedKeys: new Set(), + storagePath: storageDir, + onDiskKeys: new Set(), + }; + + const result = await runPipeline(cache, files); + const routes = foldedRoutesOf(result); + + // All three non-literal shapes resolve to folded literals. (The class-level + // @RequestMapping("/users") prefix join happens in the downstream routes + // phase — at this seam we assert the method-level folded paths.) + const paths = routes.map((r) => r.path).sort(); + expect(paths).toContain('/api/v1/users'); // qualified ref via import + expect(paths).toContain('/api/v1/orders'); // FQN multi-segment chain + // The concat route folds to the same literal as the FQN route. + expect(paths.filter((p) => p === '/api/v1/orders').length).toBeGreaterThanOrEqual(2); + // Skip floor: no phantom empty/raw-expr paths. + for (const p of paths) { + expect(p.length).toBeGreaterThan(1); + expect(p).not.toContain('ApiPaths'); + expect(p).not.toContain('com.example'); + } + }, 120_000); + + it('warm run: parse-cache replay yields the identical folded route set', async () => { + const files = writeFixture(); + const cache: ParseCache = { + version: PARSE_CACHE_VERSION, + entries: new Map(), + usedKeys: new Set(), + storagePath: storageDir, + onDiskKeys: new Set(), + }; + + // Run #1 populates the cache; persist it like run-analyze does. + const run1 = await runPipeline(cache, files); + const { saveParseCache, pruneCache } = await import('../../src/storage/parse-cache.js'); + pruneCache(cache, cache.usedKeys); + await saveParseCache(storageDir, cache); + + // 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); + const run2 = await runPipeline(warm, files); + + const cold = foldedRoutesOf(run1).map((r) => `${r.method} ${r.path}`).sort(); + const hot = foldedRoutesOf(run2).map((r) => `${r.method} ${r.path}`).sort(); + expect(hot).toEqual(cold); + expect(hot.length).toBeGreaterThan(0); + }, 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 335fc59d7..4649a1a02 100644 --- a/gitnexus/test/unit/java-route-const-resolver.test.ts +++ b/gitnexus/test/unit/java-route-const-resolver.test.ts @@ -369,3 +369,133 @@ public class BConsts { ).toBeNull(); }); }); + +// ─── Review round 2 regressions (#2980) ─────────────────────────────────── + +describe('F4: class nested in an interface is NOT implicitly final', () => { + const SRC = `package p; +public interface Api { + String BASE = "/api"; + class Holder { + String mutable = "/mutable"; + static final String OK = "/ok"; + } + interface Inner { + String IMPLICIT = "/implicit"; + class Deep { + String alsoMutable = "/also"; + } + } +}`; + + it('harvests the interface own fields and explicit static final nested fields', () => { + const mc = extractJavaModuleConstants(parse(SRC)); + expect(mc.literals.get('BASE')).toBe('/api'); + expect(mc.literals.get('OK')).toBe('/ok'); + expect(mc.literals.get('Holder.OK')).toBe('/ok'); + }); + + it('does NOT harvest mutable fields of a class nested in an interface', () => { + const mc = extractJavaModuleConstants(parse(SRC)); + expect(mc.literals.has('mutable')).toBe(false); + expect(mc.literals.has('alsoMutable')).toBe(false); + expect(mc.literals.has('Holder.mutable')).toBe(false); + expect(mc.exprs.has('mutable')).toBe(false); + }); + + it('still harvests a class directly nested in an interface (own implicit semantics recomputed at each boundary)', () => { + const mc = extractJavaModuleConstants(parse(SRC)); + expect(mc.literals.get('IMPLICIT')).toBe('/implicit'); + expect(mc.literals.get('Inner.IMPLICIT')).toBe('/implicit'); + }); +}); + +describe('F5: same-name shadowing across nested types drops the stale entry', () => { + const SRC = `package p; +public class Outer { + public static final String PATH = "/v1"; + static class Inner { + // shadows Outer.PATH with a non-foldable initializer + public static final String PATH = compute(); + static String compute() { return "/v2"; } + } +}`; + + it('a non-foldable shadow must drop the outer literal, not keep it (skip floor)', () => { + const mc = extractJavaModuleConstants(parse(SRC)); + expect(mc.literals.has('PATH')).toBe(false); + expect(mc.exprs.has('PATH')).toBe(false); + }); + + it('qualified aliases survive per class (Outer.PATH resolvable, Inner.PATH not)', () => { + const mc = extractJavaModuleConstants(parse(SRC)); + expect(mc.literals.get('Outer.PATH')).toBe('/v1'); + expect(mc.literals.has('Inner.PATH')).toBe(false); + }); + + it('a foldable shadow REPLACES the outer value (last binding wins in source order)', () => { + const src = `package p; +public class Outer { + public static final String PATH = "/v1"; + static class Inner { + public static final String PATH = "/v2"; + } +}`; + const mc = extractJavaModuleConstants(parse(src)); + expect(mc.literals.get('PATH')).toBe('/v2'); + expect(mc.literals.get('Outer.PATH')).toBe('/v1'); + expect(mc.literals.get('Inner.PATH')).toBe('/v2'); + }); +}); + +describe('F3: multi-segment FQN annotation values and constant initializers', () => { + const constValueOf = (src: string): Parser.SyntaxNode => { + const cls = parse(src).rootNode.descendantsOfType('class_declaration')[0]!; + const body = cls.childForFieldName('body')!; + const field = body.children.find((c) => c.type === 'field_declaration')!; + const decl = field.children.find((c) => c.type === 'variable_declarator')!; + return decl.childForFieldName('value')!; + }; + + it('parses com.example.ApiPaths.USERS as ONE ref (nested field_access chain flattened)', () => { + const ops = parseJavaConstOperands(constValueOf(`package p; +public class W { + public static final String X = com.example.ApiPaths.USERS; +}`)); + expect(ops).toEqual([{ kind: 'ref', name: 'com.example.ApiPaths.USERS' }]); + }); + + it('still rejects call/object-side chains: f().X, this.X, arr[0].X', () => { + expect(parseJavaConstOperands(constValueOf(`package p; +public class W { public static final String A = f().X; static Object f(){return null;} }`))).toBeNull(); + expect(parseJavaConstOperands(constValueOf(`package p; +public class W { public static final String B = this.Y; String Y = "y"; }`))).toBeNull(); + expect(parseJavaConstOperands(constValueOf(`package p; +public class W { public static final String C = arr[0].Z; }`))).toBeNull(); + }); + + it('resolves an FQN-qualified annotation constant end-to-end (query → operands → fold)', () => { + const repo = repoOf({ + 'src/main/java/com/example/ApiPaths.java': `package com.example; +public class ApiPaths { + public static final String USERS = "/api/v1/users"; +}`, + 'src/main/java/com/example/Ctl.java': `package com.example; +import org.springframework.web.bind.annotation.PostMapping; +public class Ctl { + @PostMapping(com.example.ApiPaths.USERS) + public void list() {} +}`, + }); + // The whole FQN arrives as one ref operand (verified against the real + // tree-sitter-java parse shape); the resolver must follow it via the + // longest-prefix import fallback. + expect( + resolveJavaConstant( + 'src/main/java/com/example/Ctl.java', + 'com.example.ApiPaths.USERS', + repo, + ), + ).toBe('/api/v1/users'); + }); +});