From 94f167a90f8d1f83199fed00b852f80654e930fc Mon Sep 17 00:00:00 2001 From: azizur100389 Date: Fri, 16 Oct 2026 09:02:41 +0100 Subject: [PATCH] fix(routes): reject middleware and route metadata false positives (#3561) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(routes): reject middleware and route metadata false positives * fix(routes): invalidate cached phantom routes on upgrade * test(routes): cover cache upgrades across merged schema versions Verify that schemas 137 through 140 invalidate both parse stores after merging main into PR #3561 and advancing the schema to 141. --------- Co-authored-by: Gergő Magyar --- ARCHITECTURE.md | 6 ++ .../route-extractors/dispatch-guard.ts | 2 +- .../core/ingestion/workers/parse-worker.ts | 8 +-- gitnexus/src/storage/parse-cache.ts | 5 +- .../express-route-priority/src/server.ts | 6 ++ .../resolvers/express-routes.test.ts | 63 +++++++++++++++++++ .../test/unit/dispatch-guard-routes.test.ts | 12 ++++ .../test/unit/incremental-parse-cache.test.ts | 6 +- 8 files changed, 98 insertions(+), 10 deletions(-) diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index f546ba411..b6697200c 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -198,6 +198,12 @@ That extractor is deliberately **precision-weighted**: `route_map` presents its output as fact, so a `startsWith` namespace test, a bare `pathname === '/'` without a verb, and any regex it cannot translate exactly are all dropped rather than guessed at. A missing route is a coverage limit; an invented one is a lie. +Express `app.get(RegExp, handler)` registrations are likewise omitted: a regex +matcher does not have one finite URL to use as a Route node key. +Express `app.use(path, middleware)` is middleware attachment, not an endpoint; +it does not create a Route node. Dispatch-guard inference also ignores stored +`route.routePath` metadata: comparing a recorded route name with a literal +does not prove that the request path is being dispatched. Two rules there need more than one comparison to decide, and are worth knowing about before changing either: diff --git a/gitnexus/src/core/ingestion/route-extractors/dispatch-guard.ts b/gitnexus/src/core/ingestion/route-extractors/dispatch-guard.ts index d4a46a012..f1eba8760 100644 --- a/gitnexus/src/core/ingestion/route-extractors/dispatch-guard.ts +++ b/gitnexus/src/core/ingestion/route-extractors/dispatch-guard.ts @@ -242,7 +242,7 @@ function isPathExpression(node: SyntaxNode): boolean { if (node.type === 'member_expression') { const property = node.childForFieldName('property'); if (property === null) return false; - if (PATH_IDENTIFIERS.has(property.text)) return true; + if (property.text === 'pathname') return true; if (property.text === 'url') { const object = node.childForFieldName('object'); return object !== null && RAW_URL_RECEIVERS.has(object.text); diff --git a/gitnexus/src/core/ingestion/workers/parse-worker.ts b/gitnexus/src/core/ingestion/workers/parse-worker.ts index 29a443bc8..885134f5c 100644 --- a/gitnexus/src/core/ingestion/workers/parse-worker.ts +++ b/gitnexus/src/core/ingestion/workers/parse-worker.ts @@ -2054,7 +2054,7 @@ const processFileGroup = ( ) { const method = captureMap['express_route.method'].text; const routePath = captureMap['express_route.path'].text; - if (EXPRESS_ROUTE_METHODS.has(method) && routePath.startsWith('/')) { + if (EXPRESS_ROUTE_METHODS.has(method) && method !== 'use' && routePath.startsWith('/')) { // Extract the receiver (the object the method is called on) to filter out // HTTP client calls like axios.get('/api/users') that match the same pattern // as Express route registrations. @@ -2130,11 +2130,7 @@ const processFileGroup = ( } for (const registration of registrations) { const httpMethod = - registration.method === 'all' - ? '*' - : registration.method === 'use' - ? 'GET' - : registration.method.toUpperCase(); + registration.method === 'all' ? '*' : registration.method.toUpperCase(); result.decoratorRoutes.push({ filePath: file.path, routePath, diff --git a/gitnexus/src/storage/parse-cache.ts b/gitnexus/src/storage/parse-cache.ts index 57f2041d3..8fd0a2eaa 100644 --- a/gitnexus/src/storage/parse-cache.ts +++ b/gitnexus/src/storage/parse-cache.ts @@ -869,7 +869,10 @@ import { copyV8CacheIfPresent, tryLoadV8Cache, writeV8CacheFile } from './v8-sid // v142: fetch-wrapper consumers are AST candidates emitted by providers. // Re-extract v141 shards: main used 141 for detached Go ownership, while // this branch used it for wrapper candidates. Neither contains both changes. -const SCHEMA_BUMP = 142; +// v143 (#3561): combine v142 wrapper candidates with the route corrections. +// Older caches may retain phantom middleware and stored route-metadata edges. +// The PR also used v142 without wrapper candidates; invalidate both histories. +const SCHEMA_BUMP = 143; const GITNEXUS_PKG_VERSION = (() => { try { // package.json sits at gitnexus/package.json — two levels up from diff --git a/gitnexus/test/fixtures/lang-resolution/express-route-priority/src/server.ts b/gitnexus/test/fixtures/lang-resolution/express-route-priority/src/server.ts index 00a197f1f..0fe8140e9 100644 --- a/gitnexus/test/fixtures/lang-resolution/express-route-priority/src/server.ts +++ b/gitnexus/test/fixtures/lang-resolution/express-route-priority/src/server.ts @@ -5,6 +5,8 @@ function mcp() { return 'mcp'; } app.get('/api/info', realInfo); app.post('/api/query', realQuery); app.all('/api/mcp', mcp); +app.use('/api/mcp', middleware); +app.get(/^\/web(?:\/|$)/, realInfo); app.route('/api/chained').get(realInfo); app.route('/api/chained-post').post(realQuery); app.route('/api/chained-multi').get(realInfo).post(realQuery); @@ -22,3 +24,7 @@ counts.get('/ghost-block-comment' /* cached key */); counts.get( '/ghost-line-comment', // cached key ); + +function inspectRoute(route) { + return route.routePath === '/comparison-ghost'; +} diff --git a/gitnexus/test/integration/resolvers/express-routes.test.ts b/gitnexus/test/integration/resolvers/express-routes.test.ts index f4edca7a1..fcc2fe5c4 100644 --- a/gitnexus/test/integration/resolvers/express-routes.test.ts +++ b/gitnexus/test/integration/resolvers/express-routes.test.ts @@ -11,6 +11,7 @@ import { } from '../../../src/storage/parse-cache.js'; import { getDurableParsedFileDir, + loadDurableParsedFileIndex, pruneAndSaveDurableParsedFileStore, } from '../../../src/storage/parsedfile-store.js'; import { @@ -102,6 +103,10 @@ describe('Express route identity and source priority', () => { expect(mcpRoutes[0].properties.method).toBe('*'); }); + it('does not turn an Express RegExp matcher into an invented URL', () => { + expect(getNodesByLabel(result, 'Route')).not.toContain('/web'); + }); + it.each(['/ghost', '/ghost-block-comment', '/ghost-line-comment'])( 'does not classify the one-argument Map.get lookup %s as a route', (route) => { @@ -109,6 +114,10 @@ describe('Express route identity and source priority', () => { }, ); + it('does not turn stored route metadata into a dispatch guard', () => { + expect(getNodesByLabel(result, 'Route')).not.toContain('/comparison-ghost'); + }); + it.each([ ['/api/chained', ['GET']], ['/api/chained-post', ['POST']], @@ -234,4 +243,58 @@ describe('Express route identity and source priority', () => { fs.rmSync(tempDir, { recursive: true, force: true }); } }, 120_000); + + it.each([137, 138, 139, 140])( + 'reparses route captures from a same-package v%i cache on upgrade', + async (previousSchema) => { + const tempDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gitnexus-express-route-upgrade-')); + const repoDir = path.join(tempDir, 'repo'); + const storageDir = path.join(tempDir, 'storage'); + const previousVersion = `${previousSchema}+${PARSE_CACHE_VERSION.split('+').slice(1).join('+')}`; + try { + fs.cpSync(path.join(FIXTURES, 'express-route-priority'), repoDir, { recursive: true }); + const cache: ParseCache = { + version: PARSE_CACHE_VERSION, + entries: new Map(), + usedKeys: new Set(), + storagePath: storageDir, + onDiskKeys: new Set(), + }; + const initial = await runPipelineFromRepo(repoDir, () => {}, { + parseCache: cache, + workerPoolSize: 1, + }); + expect(initial.reparsedFileCount).toBe(2); + pruneCache(cache, cache.usedKeys); + cache.version = previousVersion; + const savedKeys = await saveParseCache(storageDir, cache); + expect(savedKeys).toHaveLength(2); + const durableDir = getDurableParsedFileDir(storageDir); + await pruneAndSaveDurableParsedFileStore(durableDir, previousVersion, new Set(savedKeys)); + expect((await loadDurableParsedFileIndex(durableDir, previousVersion)).size).toBe(2); + + const upgraded = await loadParseCache(storageDir); + expect(upgraded.onDiskKeys?.size ?? 0).toBe(0); + expect(await loadDurableParsedFileIndex(durableDir, PARSE_CACHE_VERSION)).toEqual( + new Map(), + ); + const result = await runPipelineFromRepo(repoDir, () => {}, { + parseCache: upgraded, + workerPoolSize: 1, + }); + expect(result.reparsedFileCount).toBe(2); + expect(result.parseCacheHitFileCount).toBe(0); + const routes = getNodesByLabel(result, 'Route'); + expect(routes).toContain('/api/info'); + expect(routes).not.toContain('/ghost'); + expect(routes).not.toContain('/comparison-ghost'); + expect( + getNodesByLabelFull(result, 'Route').filter((node) => node.name === '/api/mcp'), + ).toHaveLength(1); + } finally { + fs.rmSync(tempDir, { recursive: true, force: true }); + } + }, + 120_000, + ); }); diff --git a/gitnexus/test/unit/dispatch-guard-routes.test.ts b/gitnexus/test/unit/dispatch-guard-routes.test.ts index 66bc6a0e8..af13ee2ba 100644 --- a/gitnexus/test/unit/dispatch-guard-routes.test.ts +++ b/gitnexus/test/unit/dispatch-guard-routes.test.ts @@ -118,6 +118,12 @@ describe('dispatch-guard route extraction', () => { }); describe('what must NOT become a route', () => { + it('ignores comparisons against stored route metadata', () => { + expect(paths("function f(route) { return route.routePath === '/comparison-ghost' }")).toEqual( + [], + ); + }); + it('ignores a comparison against something that is not a request path', () => { // `mode` is not a path expression, so `/full` is just a string. expect(paths(`function f() { if (mode === '/full') { return 1 } }`)).toEqual([]); @@ -1092,6 +1098,12 @@ describe('dispatch-guard route extraction', () => { }); describe('path expressions the rule accepts', () => { + it('accepts a bare routePath request-path guard', () => { + expect(paths("function f(routePath) { if (routePath === '/api/x') return 1 }")).toEqual([ + '/api/x', + ]); + }); + it('accepts a member access ending in .pathname', () => { expect( paths(`function f(req) { if (new URL(req.url, base).pathname === '/api/x') { return 1 } }`), diff --git a/gitnexus/test/unit/incremental-parse-cache.test.ts b/gitnexus/test/unit/incremental-parse-cache.test.ts index a70c7d5d5..2fe2a0e2c 100644 --- a/gitnexus/test/unit/incremental-parse-cache.test.ts +++ b/gitnexus/test/unit/incremental-parse-cache.test.ts @@ -324,8 +324,9 @@ describe('PARSE_CACHE_VERSION', () => { // Moved 139 -> 140 for Ruby accessor ownership captures used by rename (#3557). // Moved 140 -> 141 for detached Go ownership captures used by rename (#3559). // Moved 141 -> 142 to combine Go ownership with fetch-wrapper candidates (#3567). - it('pins SCHEMA_BUMP to 142 so concurrent bumps cannot silently collide (#2766, #3015, #3088, #2885, #3128, #2865, #3130, #1432, #3161, #3179, #3219, #3190, #3253, #3273, #3339, #3354, #3371, #2965, #3390, #3398, #3396, #3394, #3399, #3414, #3408, #3402, #3446, #3450, #3499, #3502, #3487, #3505, #3504, #3497, #3486, #3559, #3567)', () => { - expect(Number(PARSE_CACHE_VERSION.split('+', 1)[0])).toBe(142); + // Moved 142 -> 143 to combine fetch-wrapper candidates with #3561 route corrections. + it('pins SCHEMA_BUMP to 143 so concurrent bumps cannot silently collide (#2766, #3015, #3088, #2885, #3128, #2865, #3130, #1432, #3161, #3179, #3219, #3190, #3253, #3273, #3339, #3354, #3371, #2965, #3390, #3398, #3396, #3394, #3399, #3414, #3408, #3402, #3446, #3450, #3499, #3502, #3487, #3505, #3504, #3497, #3486, #3559, #3567, #3561)', () => { + expect(Number(PARSE_CACHE_VERSION.split('+', 1)[0])).toBe(143); expect(PARSE_CACHE_BUCKET_COUNT).toBe(128); // The PREVIOUS version must fail the reuse gate, not merely differ from the // current one — a hardcoded number outside the conflict hunk rebases cleanly @@ -336,6 +337,7 @@ describe('PARSE_CACHE_VERSION', () => { 82, 83, 84, 85, 86, 87, 88, 89, 90, 91, 92, 93, 94, 95, 96, 97, 98, 99, 100, 101, 102, 103, 104, 105, 106, 107, 108, 109, 110, 111, 112, 113, 114, 115, 116, 117, 118, 119, 120, 121, 122, 123, 124, 125, 126, 127, 128, 129, 130, 131, 132, 133, 134, 135, 136, 137, 138, 139, 140, 141, + 142, ]) { expect(Number(PARSE_CACHE_VERSION.split('+', 1)[0])).not.toBe(taken); }