mirror of
https://github.com/abhigyanpatwari/GitNexus.git
synced 2026-08-28 05:25:25 +00:00
Answers every reproduced finding still open on #2980, plus the defects an
adversarial pass found in the first round of those fixes. The wrong-path group
each turned a *missing* fact into a *wrong* one, which is what this module's
skip-or-correct contract exists to prevent.
Wrong-path fixes
* Escapes were deleted from constant values. tree-sitter-java splits a
`string_literal` around its `escape_sequence` children, so joining
`string_fragment`s alone folded `"/user/{id:\\d+}"` — the standard Spring
path-variable constraint — to `/user/{id:d+}`, and a pure-escape literal to
the empty string. Worse, the LITERAL path keeps escapes verbatim, so one Java
route had two irreconcilable spellings. `stringLiteralValue` now reuses
`unquoteSpringLiteral`, the helper that literal path already uses. Java text
blocks are excluded: that helper's `"""` arm would hand back the raw block,
newline and incidental indentation included, so they keep the old skip.
* A constant-valued class prefix produced a truncated route. The new
`@value_expr` query branches were `method_declaration`-only, so
`@RequestMapping(ApiPaths.BASE)` left the prefix empty and the method route
was emitted unprefixed — a path the application does not serve, where the base
emitted nothing at all. Both subsystems now detect such a class and suppress
its method routes, the rule `classesWithArrayPrefix` already encodes for the
array form. The suppression covers ingestion's separate no-argument-mapping
loop too, without which a bare `@GetMapping` under a constant prefix still
shipped an empty-path Route while the group emitted nothing.
* A shadowed static import survived a non-foldable rebind. The rebind-drop
deleted `literals`/`exprs` but not `imports`, so a name both static-imported
and locally redeclared resolved through the stale import to the imported
value instead of skipping (#2393's Python defect, reproduced for Java).
* `resolveJavaImport` guessed where its own docstring promised null. The
nearest-shared-directory tie-break is gone: javac resolves duplicate FQNs by
classpath order, so proximity can return a src/test fixture copy.
Parity and coverage fixes
* One constant-file gate, exported as `isJavaConstantFile` and used by both the
ingestion provider and the group `prepareRepo` pre-pass. The two spellings
disagreed on a constant INTERFACE — implicitly `public static final`, so it
carries neither keyword — which the group admitted and ingestion rejected, so
the group published a contract while the graph got no Route node. It is also
modifier-order agnostic now, and its interface arm requires a String
assignment so a javadoc mentioning "interface" no longer costs a parse.
* Import ambiguity is measured over constant-DEFINING files on both sides.
Ingestion's harvest gate also admits import-only files, so handing
`resolveJavaImport` every repo key let a duplicate FQN that defines nothing
make ingestion alone floor to skip — reopening the same parity break in the
same losing direction.
* Python's constant harvest is unconditional again. The gate added here
required NAME immediately followed by `=`, so it dropped `API: str = "/api"`,
`API: Final[str] = "/api"` and every composed constant whose RHS starts with
an identifier — routes that already resolve on main. The worker now treats a
missing heuristic as "harvest" rather than "skip".
* Enum and record declarations were traversed but never collected, so a
`static final String` declared in one was absent from the map. The walk still
descends the whole body, so a type nested in an enum-constant body is kept.
* Constants composed across files through a qualified ref never resolved:
operands found inside an initializer went to the agnostic core, which only
knows bare names, so `X = BConsts.Y + "/tail"` floored to null even
acyclically. The Java binding now folds its own expressions — and carries the
core's guards with them: a `visited` stack popped on unwind, a memo of
successes, and `MAX_FOLD_LENGTH`. Without the memo a shared-descendant DAG
re-folds each child per reference; because a chain of empty strings never
accumulates output, the length cap could not stop it, and one route over a
31-line constants file took 11 s at 28 levels on the main thread.
* Dropped the dead `com.java.lang.` type normalization.
Cache
* `SCHEMA_BUMP` 70 -> 72. Leaving it at 70 was justified by "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 base. With
package.json untouched, `PARSE_CACHE_VERSION` was byte-identical across the
merge, so every same-version warm cache replayed pre-feature captures and the
feature was inert. 72 rather than 71 because open PR #3017 already claims 71
with an identical pin test — the ledger's rule is the next value above every
in-flight claim, not above origin/main.
Tests
* Regression cover for each fix above, including a gate-level test (the gate
itself had none), an import-ambiguity test, a text-block test, and a 30-level
shared-descendant DAG that fails by timeout if the memo is ever removed.
* New `group/java-const-route-parity.test.ts` drives `prepareRepo` + a
three-argument `scan`. Every existing Spring parity guard calls `scan(tree)`
with ONE argument, and the plugin drops constant-valued routes without a repo
context — so those guards were structurally blind to this whole feature.
* The pipeline e2e now proves the warm run is a REPLAY (`usedWorkerPool` false)
instead of only comparing route sets. It was not one: the test never persisted
the durable ParsedFile store, so the "warm" run reparsed through the workers
and would have passed with the cache round-trip completely broken.
* Its dist freshness gate covers every source the pipeline loads, not just
parse-worker.ts, and prints the loud message the docblock promised.
* The self-import cycle fixture now actually self-imports, so it reaches the
qualified-ref recursion and its depth cap.
* Removed the dead `WIN_POST_MAPPING` fixture and the claim behind it: Spring
alias recognition is an exact-name map on this base, so `@WinPostMapping`
extracts zero routes no matter how its value folds (#2883 is still open).
Fixtures now use annotations this branch actually recognises.
264 lines
11 KiB
TypeScript
264 lines
11 KiB
TypeScript
/**
|
|
* #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 + 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 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
|
|
* 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';
|
|
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 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;
|
|
|
|
// ── 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 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;
|
|
|
|
@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-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() {}
|
|
}
|
|
`;
|
|
|
|
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<ReturnType<typeof runChunkedParseAndResolve>>;
|
|
|
|
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<PipelineResult> {
|
|
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);
|
|
expect(result.usedWorkerPool).toBe(true);
|
|
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 — 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);
|
|
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)
|
|
.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);
|
|
// 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);
|
|
});
|