From 6b68fa02523a69ba2f1b8abe45b966171fb2973a Mon Sep 17 00:00:00 2001 From: Gergo Magyar Date: Tue, 14 Apr 2026 07:42:32 +0100 Subject: [PATCH] =?UTF-8?q?fix(extractors):=20address=20PR=20#817=20review?= =?UTF-8?q?=20=E2=80=94=20ambiguous=20symbol=20pick=20+=20contract=20id=20?= =?UTF-8?q?casing?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Copilot + Claude review on PR #817 flagged two follow-up bugs on top of the F1/F2/F3 fixes: 1. http-route-extractor: ambiguous multi-verb case left handlerName null but still ran the CONTAINS DB query. pickSymbolUid(syms, null) then silently picked pool[0] — reintroducing handler mis-attribution via a different route than the .find() bug F2 fixed. Now gates symbol enrichment on an ambiguousCandidates flag so the file-basename fallback wins instead. 2. manifest-extractor: buildContractId passed raw user casing through for the explicit-method form, so get::/api/orders and GET::/api/orders produced different contract ids even though parseHttpContract upper-cases during lookup. Now reuses parseHttpContract + normalizeRoutePath to canonicalize both method and path, so logically equivalent manifest inputs share a contract id (and share a manifestSymbolUid fallback). Adds one regression test per bug: lowercase vs uppercase manifest contract ids must match, and ambiguous multi-verb with CONTAINS rows must not silently attach a real handler or call the CONTAINS query at all. 75 tests pass across the three extractor suites. --- .../group/extractors/http-route-extractor.ts | 12 ++++-- .../group/extractors/manifest-extractor.ts | 11 ++++- .../unit/group/http-route-multi-verb.test.ts | 43 +++++++++++++++++++ .../unit/group/manifest-extractor.test.ts | 35 ++++++++++++++- 4 files changed, 94 insertions(+), 7 deletions(-) diff --git a/gitnexus/src/core/group/extractors/http-route-extractor.ts b/gitnexus/src/core/group/extractors/http-route-extractor.ts index f089bd248..f2914613d 100644 --- a/gitnexus/src/core/group/extractors/http-route-extractor.ts +++ b/gitnexus/src/core/group/extractors/http-route-extractor.ts @@ -256,15 +256,19 @@ export class HttpRouteExtractor implements ContractExtractor { (d) => normalizeHttpPath(d.path) === normalizedRoute, ); let match: (typeof candidates)[number] | undefined; + const ambiguousCandidates = !method && candidates.length > 1; if (method) { match = candidates.find((d) => d.method === method); } else if (candidates.length === 1) { match = candidates[0]; } // else: multiple candidates + unknown method → leave match - // undefined so handlerName stays null (file-basename fallback - // via pickSymbolUid) and method stays at the conservative 'GET' - // default set below. + // undefined so handlerName stays null and skip symbol + // enrichment below, keeping the file-basename fallback instead + // of letting pickSymbolUid silently pick the first Function / + // Method in the file (which reintroduces the mis-attribution + // we were trying to avoid). Method stays at the conservative + // 'GET' default set below. if (match) { if (!method) method = match.method; handlerName = match.name; @@ -278,7 +282,7 @@ export class HttpRouteExtractor implements ContractExtractor { let symbolName = path.basename(filePath) || 'handler'; let symPath = filePath; const fileId = row.fileId ?? row[0]; - if (fileId) { + if (fileId && !ambiguousCandidates) { try { const syms = await db(CONTAINS_QUERY, { fileId }); if (syms.length > 0) { diff --git a/gitnexus/src/core/group/extractors/manifest-extractor.ts b/gitnexus/src/core/group/extractors/manifest-extractor.ts index e3bb0ba4c..4c0d737b7 100644 --- a/gitnexus/src/core/group/extractors/manifest-extractor.ts +++ b/gitnexus/src/core/group/extractors/manifest-extractor.ts @@ -284,8 +284,15 @@ export class ManifestExtractor { private buildContractId(type: ContractType, contract: string): string { switch (type) { case 'http': { - if (/^[A-Za-z]+::/.test(contract)) return `http::${contract}`; - return `http::*::${contract}`; + // Canonicalize method casing and path separators so logically + // equivalent inputs (`get::/api/orders` vs `GET::/api/orders`, + // or trailing-slash variants) produce the same contractId and + // matching `manifestSymbolUid` fallback. Without this, raw + // user casing leaks into cross-impact join keys and fragments + // matches across repos. + const { method, path: rawPath } = parseHttpContract(contract); + const normalizedPath = normalizeRoutePath(rawPath); + return method ? `http::${method}::${normalizedPath}` : `http::*::${normalizedPath}`; } case 'grpc': return `grpc::${contract}`; diff --git a/gitnexus/test/unit/group/http-route-multi-verb.test.ts b/gitnexus/test/unit/group/http-route-multi-verb.test.ts index b42de8244..b2884fc60 100644 --- a/gitnexus/test/unit/group/http-route-multi-verb.test.ts +++ b/gitnexus/test/unit/group/http-route-multi-verb.test.ts @@ -202,6 +202,49 @@ describe('HttpRouteExtractor — graph-assisted multi-verb disambiguation', () = expect(out[0].symbolName).toBe('routes.ts'); }); + // ── Provider: multi-verb + CONTAINS rows → must still refuse to guess ── + it('provider: ambiguous multi-verb skips CONTAINS enrichment (no silent pool[0] pick)', async () => { + // Regression test for Copilot's review on PR #817. Before the fix, + // the ambiguous-case code path left `handlerName` null but still ran + // the CONTAINS DB query, and `pickSymbolUid(syms, null)` silently + // picked pool[0] — reintroducing handler mis-attribution via a + // different route than `.find()`. + FILE_DETECTIONS.set('routes.ts', [ + detection('provider', 'GET', '/api/orders', 'listOrders'), + detection('provider', 'POST', '/api/orders', 'createOrder'), + ]); + + const db = vi.fn(async (query: string) => { + if (query.includes('HANDLES_ROUTE')) { + return [ + { + fileId: 'f1', + filePath: 'routes.ts', + routePath: '/api/orders', + routeSource: 'unknown-reason', + }, + ]; + } + if (query.includes('CONTAINS')) return containsFor(['listOrders', 'createOrder']); + return []; + }); + + const out = await new HttpRouteExtractor().extract(db, '/repo', { + name: 'r', + url: 'r', + } as never); + expect(out).toHaveLength(1); + // Ambiguous → do not attribute to any real handler in the file. + expect(out[0].symbolName).not.toBe('listOrders'); + expect(out[0].symbolName).not.toBe('createOrder'); + expect(out[0].symbolUid).toBe(''); + expect(out[0].symbolName).toBe('routes.ts'); + expect(out[0].meta.method).toBe('GET'); + // CONTAINS query must have been skipped entirely under ambiguity. + const calls = db.mock.calls.map(([q]) => q as string); + expect(calls.some((q) => q.includes('CONTAINS'))).toBe(false); + }); + // ── Provider: three-verb method known ────────────────────────────── it('provider: three verbs at same path with method known still matches correctly', async () => { FILE_DETECTIONS.set('routes.ts', [ diff --git a/gitnexus/test/unit/group/manifest-extractor.test.ts b/gitnexus/test/unit/group/manifest-extractor.test.ts index aa8566b21..42ed86b84 100644 --- a/gitnexus/test/unit/group/manifest-extractor.test.ts +++ b/gitnexus/test/unit/group/manifest-extractor.test.ts @@ -423,7 +423,9 @@ describe('ManifestExtractor', () => { expect(seenParam).toBe('/'); // No match → synthetic uid, no crash. const provider = result.contracts.find((c) => c.role === 'provider'); - expect(provider?.symbolUid).toBe('manifest::orders-svc::http::GET::'); + // buildContractId canonicalizes the empty path to `/` so contract ids + // match regardless of trailing-slash variants in the manifest input. + expect(provider?.symbolUid).toBe('manifest::orders-svc::http::GET::/'); }); it('treats empty method portion (::/api/orders) as a bare path', async () => { @@ -545,6 +547,37 @@ describe('ManifestExtractor', () => { expect(provider?.contractId).toBe('http::GET::/api/orders'); }); + it('canonicalizes method casing so get::/api/orders and GET::/api/orders share a contractId', async () => { + // Regression for Copilot's review on PR #817: without canonicalization, + // `buildContractId` passed raw casing through (`http::get::/api/orders`) + // while `parseHttpContract` upper-cased during lookup, fragmenting + // cross-impact joins between providers and consumers that happened to + // use different casing conventions in their group.yaml. + const lower = await extractor.extractFromManifest([ + { + from: 'gateway', + to: 'orders-svc', + type: 'http', + contract: 'get::/api/orders', + role: 'consumer', + }, + ]); + const upper = await extractor.extractFromManifest([ + { + from: 'gateway', + to: 'orders-svc', + type: 'http', + contract: 'GET::/api/orders', + role: 'consumer', + }, + ]); + const lowerContractId = lower.contracts.find((c) => c.role === 'provider')?.contractId; + const upperContractId = upper.contracts.find((c) => c.role === 'provider')?.contractId; + expect(lowerContractId).toBe('http::GET::/api/orders'); + expect(upperContractId).toBe('http::GET::/api/orders'); + expect(lowerContractId).toBe(upperContractId); + }); + it('returns empty for no links', async () => { const result = await extractor.extractFromManifest([]); expect(result.contracts).toHaveLength(0);