diff --git a/gitnexus-web/src/hooks/useAutoScroll.ts b/gitnexus-web/src/hooks/useAutoScroll.ts index 58a0aedcc1..55c2946f7c 100644 --- a/gitnexus-web/src/hooks/useAutoScroll.ts +++ b/gitnexus-web/src/hooks/useAutoScroll.ts @@ -142,4 +142,4 @@ export function useAutoScroll( isAtBottom, scrollToBottom, }; -} \ No newline at end of file +} diff --git a/gitnexus-web/test/unit/use-auto-scroll.test.tsx b/gitnexus-web/test/unit/use-auto-scroll.test.tsx index a8bc1a7954..e58227d67f 100644 --- a/gitnexus-web/test/unit/use-auto-scroll.test.tsx +++ b/gitnexus-web/test/unit/use-auto-scroll.test.tsx @@ -267,9 +267,7 @@ describe('useAutoScroll', () => { }); it('attaches the observer when the messages wrapper first appears and disconnects on unmount', () => { - const { rerender, unmount } = render( - , - ); + const { rerender, unmount } = render(); expect(screen.queryByTestId('messages-container')).toBeNull(); expect(resizeObserverInstances).toHaveLength(0); diff --git a/gitnexus/src/core/group/extractors/grpc-extractor.ts b/gitnexus/src/core/group/extractors/grpc-extractor.ts index c6af9138a1..b379a4dbd8 100644 --- a/gitnexus/src/core/group/extractors/grpc-extractor.ts +++ b/gitnexus/src/core/group/extractors/grpc-extractor.ts @@ -314,7 +314,7 @@ export async function buildProtoMap(repoPath: string): Promise { const protoDir = normalizeProtoPath(path.dirname(c.protoPath)); - const sharedRun = longestSharedSegmentRun(sourceDir, protoDir); - if (sharedRun > bestScore) { - bestScore = sharedRun; - best = c; - } + return { candidate: c, score: longestSharedSegmentRun(sourceDir, protoDir) }; + }); + + let maxScore = -1; + for (const s of scored) { + if (s.score > maxScore) maxScore = s.score; } - return best; + const winners = scored.filter((s) => s.score === maxScore); + + // Path heuristic cannot uniquely identify a winner — refuse to guess. + // Ties (including all-zero ties) would otherwise silently merge unrelated + // services under a fabricated package-qualified contract id. + if (winners.length !== 1) { + const paths = candidates.map((c) => c.protoPath).join(', '); + console.warn( + `[grpc-extractor] Ambiguous proto resolution for service "${serviceName}" from ${sourceFilePath}: ${winners.length} candidates tied at score ${maxScore} among [${paths}] — skipping canonical contract`, + ); + return null; + } + + return winners[0].candidate; } export function serviceContractId(pkg: string, serviceName: string): string { @@ -410,7 +422,8 @@ export class GrpcExtractor implements ContractExtractor { continue; } for (const d of detections) { - out.push(this.detectionToContract(d, rel, protoMap)); + const contract = this.detectionToContract(d, rel, protoMap); + if (contract) out.push(contract); } } @@ -428,9 +441,13 @@ export class GrpcExtractor implements ContractExtractor { d: GrpcDetection, filePath: string, protoMap: Map, - ): ExtractedContract { - const candidates = protoMap.get(d.serviceName); - const proto = resolveProtoConflict(d.serviceName, filePath, candidates ?? []); + ): ExtractedContract | null { + const candidates = protoMap.get(d.serviceName) ?? []; + const proto = resolveProtoConflict(d.serviceName, filePath, candidates); + // If there were proto candidates but resolution was ambiguous, skip + // contract emission rather than fabricating a package-qualified id from + // an arbitrary candidate. resolveProtoConflict already warned. + if (candidates.length > 0 && proto === null) return null; const pkg = proto?.package ?? ''; const cid = d.methodName ? contractId(pkg, d.serviceName, d.methodName) diff --git a/gitnexus/src/core/group/extractors/http-route-extractor.ts b/gitnexus/src/core/group/extractors/http-route-extractor.ts index 0b07090e15..f2914613dc 100644 --- a/gitnexus/src/core/group/extractors/http-route-extractor.ts +++ b/gitnexus/src/core/group/extractors/http-route-extractor.ts @@ -245,7 +245,30 @@ export class HttpRouteExtractor implements ContractExtractor { const providerDetections = detections.filter((d) => d.role === 'provider'); let handlerName: string | null = null; const normalizedRoute = normalizeHttpPath(routePath); - const match = providerDetections.find((d) => normalizeHttpPath(d.path) === normalizedRoute); + // Candidates share the same normalized path. When multiple + // detections at the same path exist (e.g. GET + POST /api/orders + // in one router), a blind `.find()` silently returned the first + // verb — attaching the wrong handler and, when method was not + // already pinned by the route reason, the wrong method too. + // Disambiguate by method when we know it; refuse to guess when + // we don't. + const candidates = providerDetections.filter( + (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 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; @@ -259,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) { @@ -347,10 +370,19 @@ export class HttpRouteExtractor implements ContractExtractor { // Prefer the plugin's detected method if we can find a matching // fetch/axios call in the same file. const detections = filePath ? getDetections(filePath) : []; - const inferred = detections.find( + // Symmetric to the provider path: if multiple consumer calls in + // the same file share the same normalized path (e.g. a GET + // fetch AND a POST fetch to `/api/orders`), `.find()` silently + // picked the first verb and keyed the contract id on the wrong + // method. With no upstream method signal here, refuse to guess + // when candidates are ambiguous — leave `method` at its + // conservative 'GET' default. + const consumerCandidates = detections.filter( (d) => d.role === 'consumer' && normalizeConsumerPath(d.path) === pathNorm, ); - if (inferred) method = inferred.method; + if (consumerCandidates.length === 1) { + method = consumerCandidates[0].method; + } const cid = contractIdFor(method, pathNorm); let symbolUid = ''; diff --git a/gitnexus/src/core/group/extractors/manifest-extractor.ts b/gitnexus/src/core/group/extractors/manifest-extractor.ts index 29c8f9b21d..4c0d737b79 100644 --- a/gitnexus/src/core/group/extractors/manifest-extractor.ts +++ b/gitnexus/src/core/group/extractors/manifest-extractor.ts @@ -23,6 +23,34 @@ function normalizeRoutePath(raw: string): string { return collapsed.replace(/\/+$/, ''); } +/** + * Split a manifest HTTP contract into its optional `METHOD::` prefix and + * its path portion. + * + * `buildContractId` recommends the explicit-method form `GET::/api/orders` + * in group.yaml; if we hand that raw string to `normalizeRoutePath` we get + * `/GET::/api/orders`, which can never match `Route.name = "/api/orders"` + * in the graph. This helper extracts the path portion so the Cypher + * lookup uses the canonical route name. + * + * The method prefix regex mirrors `buildContractId` (line ~251) for + * symmetry: case-insensitive `[A-Za-z]+` followed by `::`. The captured + * method is upper-cased for downstream use; method-constrained matching + * against `HANDLES_ROUTE` is a future enhancement (not yet wired). + * + * Edge cases: + * - `"::/api/orders"` — empty method portion, no alpha prefix match, so + * the whole string is treated as a bare path (matches buildContractId + * which also requires `[A-Za-z]+`). + * - `"GET::"` — method with empty path, returns `{ method: 'GET', path: '' }`; + * `normalizeRoutePath('')` resolves to `/` for caller. + */ +function parseHttpContract(raw: string): { method: string | null; path: string } { + const match = raw.match(/^([A-Za-z]+)::/); + if (!match) return { method: null, path: raw }; + return { method: match[1].toUpperCase(), path: raw.slice(match[0].length) }; +} + /** * Stable synthetic symbolUid for a manifest-declared contract whose target * symbol could not be resolved against the per-repo graph (resolveSymbol @@ -134,7 +162,15 @@ export class ManifestExtractor { // core/ingestion/pipeline.ts ensureSlash + generateId('Route', ...)). // Normalize the manifest contract the same way so a user-written // "/api/orders" matches "api/orders" in the graph. - const normalized = normalizeRoutePath(link.contract); + // + // The contract may also use the explicit-method form "GET::/api/orders" + // recommended by buildContractId. Strip the METHOD:: prefix before + // normalizing — otherwise `normalizeRoutePath('GET::/api/orders')` + // returns `/GET::/api/orders` and never matches Route.name. The + // captured method is not yet used to constrain the Cypher query + // (method-aware HANDLES_ROUTE matching is a future enhancement). + const parsed = parseHttpContract(link.contract); + const normalized = normalizeRoutePath(parsed.path); rows = await executor( `MATCH (handler)-[r:CodeRelation {type: 'HANDLES_ROUTE'}]->(route:Route) WHERE route.name = $normalized @@ -248,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/grpc-extractor.test.ts b/gitnexus/test/unit/group/grpc-extractor.test.ts index 82d79cbd69..1664bd1a7b 100644 --- a/gitnexus/test/unit/group/grpc-extractor.test.ts +++ b/gitnexus/test/unit/group/grpc-extractor.test.ts @@ -1,4 +1,4 @@ -import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; import * as fs from 'node:fs'; import fsp from 'node:fs/promises'; import * as path from 'node:path'; @@ -732,6 +732,120 @@ describe('resolveProtoConflict', () => { it('test_no_candidates_returns_null', () => { expect(resolveProtoConflict('Svc', 'src/main.go', [])).toBeNull(); }); + + it('test_all_zero_tie_returns_null', () => { + const warnSpy = vi.spyOn(console, 'warn').mockImplementation(() => {}); + const candidates = [ + makeInfo('pkgA', 'totally/unrelated/a/svc.proto'), + makeInfo('pkgB', 'completely/different/b/svc.proto'), + ]; + const result = resolveProtoConflict('Svc', 'src/main.go', candidates); + expect(result).toBeNull(); + warnSpy.mockRestore(); + }); + + it('test_positive_score_tie_returns_null', () => { + const warnSpy = vi.spyOn(console, 'warn').mockImplementation(() => {}); + // Both candidates share `src/proto` with the source dir — equal shared runs. + const candidates = [ + makeInfo('pkgA', 'src/proto/a/svc.proto'), + makeInfo('pkgB', 'src/proto/b/svc.proto'), + ]; + const result = resolveProtoConflict('Svc', 'src/proto/main.go', candidates); + expect(result).toBeNull(); + warnSpy.mockRestore(); + }); + + it('test_three_way_zero_tie_returns_null', () => { + const warnSpy = vi.spyOn(console, 'warn').mockImplementation(() => {}); + const candidates = [ + makeInfo('pkgA', 'aaa/svc.proto'), + makeInfo('pkgB', 'bbb/svc.proto'), + makeInfo('pkgC', 'ccc/svc.proto'), + ]; + const result = resolveProtoConflict('Svc', 'src/main.go', candidates); + expect(result).toBeNull(); + warnSpy.mockRestore(); + }); + + it('test_unique_winner_among_ties', () => { + // Winner with shared run 2 (services/auth), two losers with score 0. + const candidates = [ + makeInfo('winner', 'services/auth/proto/svc.proto'), + makeInfo('loserA', 'totally/unrelated/a/svc.proto'), + makeInfo('loserB', 'elsewhere/b/svc.proto'), + ]; + const result = resolveProtoConflict('Svc', 'services/auth/src/server.ts', candidates); + expect(result?.package).toBe('winner'); + }); + + it('test_ambiguous_emits_single_warn_with_service_and_paths', () => { + const warnSpy = vi.spyOn(console, 'warn').mockImplementation(() => {}); + const candidates = [ + makeInfo('pkgA', 'totally/unrelated/a/svc.proto'), + makeInfo('pkgB', 'completely/different/b/svc.proto'), + ]; + resolveProtoConflict('MyService', 'src/main.go', candidates); + expect(warnSpy).toHaveBeenCalledTimes(1); + const msg = String(warnSpy.mock.calls[0][0]); + expect(msg).toContain('MyService'); + expect(msg).toContain('src/main.go'); + expect(msg).toContain('totally/unrelated/a/svc.proto'); + expect(msg).toContain('completely/different/b/svc.proto'); + warnSpy.mockRestore(); + }); +}); + +describe('GrpcExtractor.extract ambiguous proto resolution', () => { + let tmpDir: string; + let extractor: GrpcExtractor; + + beforeEach(async () => { + tmpDir = await fsp.mkdtemp(path.join(os.tmpdir(), 'gitnexus-grpc-ambig-')); + extractor = new GrpcExtractor(); + }); + afterEach(async () => { + await fsp.rm(tmpDir, { recursive: true, force: true }); + }); + + const makeRepo = (repoPath: string): RepoHandle => ({ + id: 'test-repo', + path: '', + repoPath, + storagePath: '', + }); + + it('test_ambiguous_short_name_across_unrelated_protos_yields_no_source_contract', async () => { + const warnSpy = vi.spyOn(console, 'warn').mockImplementation(() => {}); + // Two unrelated proto files defining the same short name `UserService` in + // unrelated directories, neither sharing path segments with the Go source. + await fsp.mkdir(path.join(tmpDir, 'billing-team', 'proto'), { recursive: true }); + await fsp.writeFile( + path.join(tmpDir, 'billing-team', 'proto', 'user.proto'), + 'package billing.v1;\nservice UserService { rpc GetUser (R) returns (R); }', + ); + await fsp.mkdir(path.join(tmpDir, 'auth-team', 'proto'), { recursive: true }); + await fsp.writeFile( + path.join(tmpDir, 'auth-team', 'proto', 'user.proto'), + 'package auth.v1;\nservice UserService { rpc GetUser (R) returns (R); }', + ); + // Consumer in an unrelated directory. + await fsp.mkdir(path.join(tmpDir, 'apps', 'gateway'), { recursive: true }); + await fsp.writeFile( + path.join(tmpDir, 'apps', 'gateway', 'client.go'), + 'package main\nfunc init() { client := pb.NewUserServiceClient(conn) }', + ); + + const contracts = await extractor.extract(null, tmpDir, makeRepo(tmpDir)); + + // No source-attributed contract for UserService should be emitted. + const sourceContracts = contracts.filter( + (c) => c.meta.source === 'go_client' && c.meta.service === 'UserService', + ); + expect(sourceContracts).toHaveLength(0); + expect(warnSpy).toHaveBeenCalled(); + warnSpy.mockRestore(); + }); }); describe('serviceContractId', () => { diff --git a/gitnexus/test/unit/group/http-route-multi-verb.test.ts b/gitnexus/test/unit/group/http-route-multi-verb.test.ts new file mode 100644 index 0000000000..b2884fc608 --- /dev/null +++ b/gitnexus/test/unit/group/http-route-multi-verb.test.ts @@ -0,0 +1,393 @@ +/** + * Coverage tests for `HttpRouteExtractor` graph-assisted paths — + * specifically the multi-verb same-path regression (Codex finding F2). + * + * The bug: `extractProvidersGraph` / `extractConsumersGraph` used + * `detections.find(d => normalizeHttpPath(d.path) === routePath)` to + * backfill handler name and (for providers) method. On a file with + * multiple verbs at the same normalized path (e.g. `GET /api/orders` + * and `POST /api/orders` in one router), `.find()` returned the first + * match, silently attaching the wrong handler and/or method. + * + * Strategy: mock `./http-patterns/index.js` + `./fs-utils.js` so we + * can inject a synthetic `HttpDetection[]` per file without needing + * real tree-sitter grammars. The `db` executor is a vi.fn() that + * returns stubbed rows for the HANDLES_ROUTE / FETCHES / CONTAINS + * queries. + */ +import { describe, it, expect, vi, beforeEach } from 'vitest'; +import type Parser from 'tree-sitter'; +import type { HttpDetection } from '../../../src/core/group/extractors/http-patterns/types.js'; + +// Per-file detections injected into the mocked plugin. +const FILE_DETECTIONS = new Map(); + +vi.mock('../../../src/core/group/extractors/fs-utils.js', () => ({ + readSafe: (_repo: string, _rel: string) => 'stub content', +})); + +vi.mock('../../../src/core/group/extractors/http-patterns/index.js', () => { + return { + HTTP_SCAN_GLOB: '**/*.fake', + getPluginForFile: (rel: string) => ({ + name: 'fake', + language: {}, + scan: (_tree: Parser.Tree) => FILE_DETECTIONS.get(rel) ?? [], + }), + }; +}); + +// Patch tree-sitter Parser so `.setLanguage()` + `.parse()` don't +// require a real grammar — the mocked plugin's scan() ignores the +// tree anyway. +vi.mock('tree-sitter', () => { + class FakeParser { + setLanguage(_lang: unknown) {} + parse(_src: string) { + return {} as Parser.Tree; + } + } + return { default: FakeParser }; +}); + +import { HttpRouteExtractor } from '../../../src/core/group/extractors/http-route-extractor.js'; + +function detection( + role: 'provider' | 'consumer', + method: string, + p: string, + name: string | null, +): HttpDetection { + return { role, framework: 'test', method, path: p, name, confidence: 0.8 }; +} + +describe('HttpRouteExtractor — graph-assisted multi-verb disambiguation', () => { + beforeEach(() => { + FILE_DETECTIONS.clear(); + }); + + // Helper to build a CONTAINS response covering all handler names in a file. + const containsFor = (names: string[]) => + names.map((name, i) => ({ + uid: `uid-${name}`, + name, + filePath: 'routes.ts', + labels: ['Function'], + 0: `uid-${name}`, + 1: name, + 2: 'routes.ts', + 3: ['Function'], + })); + + // ── Provider: happy path (single match) ──────────────────────────── + it('provider: single detection backfills handler name as today', async () => { + FILE_DETECTIONS.set('routes.ts', [detection('provider', 'GET', '/api/orders', 'listOrders')]); + + const db = vi.fn(async (query: string) => { + if (query.includes('HANDLES_ROUTE')) { + return [ + { + fileId: 'f1', + filePath: 'routes.ts', + routePath: '/api/orders', + routeId: 'r1', + responseKeys: [], + routeSource: 'decorator-Get', + }, + ]; + } + if (query.includes('CONTAINS')) return containsFor(['listOrders']); + return []; + }); + + const ex = new HttpRouteExtractor(); + const out = await ex.extract(db, '/repo', { name: 'r', url: 'r' } as never); + expect(out).toHaveLength(1); + expect(out[0].symbolName).toBe('listOrders'); + expect(out[0].meta.method).toBe('GET'); + }); + + // ── Provider: multi-verb, method KNOWN (POST) ────────────────────── + it('provider: multi-verb with method known picks the matching verb (POST)', async () => { + 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: 'decorator-Post', + }, + ]; + } + 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); + expect(out[0].symbolName).toBe('createOrder'); + expect(out[0].meta.method).toBe('POST'); + }); + + it('provider: multi-verb with method known picks the matching verb (GET)', async () => { + 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: 'decorator-Get', + }, + ]; + } + 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); + expect(out[0].symbolName).toBe('listOrders'); + expect(out[0].meta.method).toBe('GET'); + }); + + // ── Provider: multi-verb, method UNKNOWN → refuse to guess ───────── + it('provider: multi-verb with method unknown skips backfill (no silent inheritance)', async () => { + 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', // methodFromRouteReason → null + }, + ]; + } + return []; + }); + + const out = await new HttpRouteExtractor().extract(db, '/repo', { + name: 'r', + url: 'r', + } as never); + expect(out).toHaveLength(1); + // CRITICAL: must NOT silently inherit POST from createOrder via .find() + expect(out[0].meta.method).toBe('GET'); // conservative default + // CRITICAL: must NOT silently attach createOrder as handler + expect(out[0].symbolName).not.toBe('createOrder'); + // With no CONTAINS rows, handlerName stays null and file-basename fallback wins. + 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', [ + detection('provider', 'GET', '/api/orders', 'listOrders'), + detection('provider', 'POST', '/api/orders', 'createOrder'), + detection('provider', 'PUT', '/api/orders', 'replaceOrder'), + ]); + + const db = vi.fn(async (query: string) => { + if (query.includes('HANDLES_ROUTE')) { + return [ + { + fileId: 'f1', + filePath: 'routes.ts', + routePath: '/api/orders', + routeSource: 'decorator-Put', + }, + ]; + } + if (query.includes('CONTAINS')) + return containsFor(['listOrders', 'createOrder', 'replaceOrder']); + return []; + }); + + const out = await new HttpRouteExtractor().extract(db, '/repo', { + name: 'r', + url: 'r', + } as never); + expect(out[0].symbolName).toBe('replaceOrder'); + expect(out[0].meta.method).toBe('PUT'); + }); + + // ── Provider: unrelated path detections don't false-positive ─────── + it('provider: detection for unrelated path does not backfill', async () => { + FILE_DETECTIONS.set('routes.ts', [detection('provider', 'POST', '/api/users', 'createUser')]); + + const db = vi.fn(async (query: string) => { + if (query.includes('HANDLES_ROUTE')) { + return [ + { + fileId: 'f1', + filePath: 'routes.ts', + routePath: '/api/orders', + routeSource: 'unknown', + }, + ]; + } + return []; + }); + + const out = await new HttpRouteExtractor().extract(db, '/repo', { + name: 'r', + url: 'r', + } as never); + expect(out[0].meta.method).toBe('GET'); + expect(out[0].symbolName).not.toBe('createUser'); + }); + + // ── Integration: one row, two detections, one out.push ───────────── + it('integration: one db row with two same-path detections yields exactly one contract', async () => { + 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: 'decorator-Post', + }, + ]; + } + 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); + expect(out[0].meta.method).toBe('POST'); + expect(out[0].symbolName).toBe('createOrder'); + }); + + // ── Consumer: single match ───────────────────────────────────────── + it('consumer: single detection backfills method as today', async () => { + FILE_DETECTIONS.set('client.ts', [detection('consumer', 'POST', '/api/orders', null)]); + + const db = vi.fn(async (query: string) => { + if (query.includes('FETCHES')) { + return [ + { + fileId: 'f1', + filePath: 'client.ts', + routePath: '/api/orders', + fetchReason: 'fetch', + }, + ]; + } + return []; + }); + + const out = await new HttpRouteExtractor().extract(db, '/repo', { + name: 'r', + url: 'r', + } as never); + expect(out).toHaveLength(1); + expect(out[0].meta.method).toBe('POST'); + }); + + // ── Consumer: multi-verb skips backfill ──────────────────────────── + it('consumer: multi-verb at same path skips backfill (conservative GET)', async () => { + FILE_DETECTIONS.set('client.ts', [ + detection('consumer', 'GET', '/api/orders', null), + detection('consumer', 'POST', '/api/orders', null), + ]); + + const db = vi.fn(async (query: string) => { + if (query.includes('FETCHES')) { + return [ + { + fileId: 'f1', + filePath: 'client.ts', + routePath: '/api/orders', + fetchReason: 'fetch', + }, + ]; + } + return []; + }); + + const out = await new HttpRouteExtractor().extract(db, '/repo', { + name: 'r', + url: 'r', + } as never); + expect(out).toHaveLength(1); + // CRITICAL: must NOT silently pick POST (first/last via .find) + expect(out[0].meta.method).toBe('GET'); // conservative default + expect(out[0].contractId).toBe('http::GET::/api/orders'); + }); +}); diff --git a/gitnexus/test/unit/group/manifest-extractor.test.ts b/gitnexus/test/unit/group/manifest-extractor.test.ts index c2c67a33ad..42ed86b848 100644 --- a/gitnexus/test/unit/group/manifest-extractor.test.ts +++ b/gitnexus/test/unit/group/manifest-extractor.test.ts @@ -300,6 +300,284 @@ describe('ManifestExtractor', () => { } }); + it('resolves http contract with explicit METHOD prefix (GET::/api/orders)', async () => { + // Regression test for Codex finding F1: resolveSymbol was passing the + // raw `link.contract` through normalizeRoutePath, which turned + // "GET::/api/orders" into "/GET::/api/orders" and never matched + // Route.name = "/api/orders". The extractor must strip the METHOD:: + // prefix and pass only the path portion to the Cypher executor. + const links: GroupManifestLink[] = [ + { + from: 'gateway', + to: 'orders-svc', + type: 'http', + contract: 'GET::/api/orders', + role: 'consumer', + }, + ]; + + let seenParam: string | undefined; + const dbExecutors = new Map< + string, + (cypher: string, params?: Record) => Promise[]> + >([ + [ + 'orders-svc', + async (_cypher, params) => { + seenParam = params?.normalized as string; + if (seenParam === '/api/orders') { + return [ + { + uid: 'uid-orders-list', + name: 'listOrders', + filePath: 'src/orders.ts', + }, + ]; + } + return []; + }, + ], + ['gateway', async () => []], + ]); + + const result = await extractor.extractFromManifest(links, dbExecutors); + + // The key assertion: $normalized must be the path only, NOT "/GET::/api/orders". + expect(seenParam).toBe('/api/orders'); + + const provider = result.contracts.find((c) => c.role === 'provider'); + expect(provider?.symbolUid).toBe('uid-orders-list'); + expect(provider?.symbolRef.filePath).toBe('src/orders.ts'); + }); + + it('resolves http contract with parameterised path (POST::/users/:id)', async () => { + const links: GroupManifestLink[] = [ + { + from: 'gateway', + to: 'users-svc', + type: 'http', + contract: 'POST::/users/:id', + role: 'consumer', + }, + ]; + + let seenParam: string | undefined; + const dbExecutors = new Map< + string, + (cypher: string, params?: Record) => Promise[]> + >([ + [ + 'users-svc', + async (_cypher, params) => { + seenParam = params?.normalized as string; + if (seenParam === '/users/:id') { + return [ + { + uid: 'uid-update-user', + name: 'updateUser', + filePath: 'src/users.ts', + }, + ]; + } + return []; + }, + ], + ['gateway', async () => []], + ]); + + const result = await extractor.extractFromManifest(links, dbExecutors); + expect(seenParam).toBe('/users/:id'); + const provider = result.contracts.find((c) => c.role === 'provider'); + expect(provider?.symbolUid).toBe('uid-update-user'); + }); + + it('handles http contract with empty path after METHOD:: (GET::)', async () => { + // Edge case: "GET::" (empty path after prefix). Normalizer produces "/" + // — either resolves to a root route or returns null cleanly. + const links: GroupManifestLink[] = [ + { + from: 'gateway', + to: 'orders-svc', + type: 'http', + contract: 'GET::', + role: 'consumer', + }, + ]; + + let seenParam: string | undefined; + const dbExecutors = new Map< + string, + (cypher: string, params?: Record) => Promise[]> + >([ + [ + 'orders-svc', + async (_cypher, params) => { + seenParam = params?.normalized as string; + return []; + }, + ], + ['gateway', async () => []], + ]); + + const result = await extractor.extractFromManifest(links, dbExecutors); + expect(seenParam).toBe('/'); + // No match → synthetic uid, no crash. + const provider = result.contracts.find((c) => c.role === 'provider'); + // 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 () => { + const links: GroupManifestLink[] = [ + { + from: 'gateway', + to: 'orders-svc', + type: 'http', + contract: '::/api/orders', + role: 'consumer', + }, + ]; + + let seenParam: string | undefined; + const dbExecutors = new Map< + string, + (cypher: string, params?: Record) => Promise[]> + >([ + [ + 'orders-svc', + async (_cypher, params) => { + seenParam = params?.normalized as string; + return []; + }, + ], + ['gateway', async () => []], + ]); + + await extractor.extractFromManifest(links, dbExecutors); + // "::/api/orders" has no method prefix per buildContractId's regex + // (`[A-Za-z]+::`), so the whole string is treated as a bare path. + // Normalizer collapses leading slashes, so "::/api/orders" stays + // essentially as-is (no alpha prefix match). + expect(seenParam).toBe('/::/api/orders'); + }); + + it('resolves http contract with lowercase verb (get::/api/orders)', async () => { + const links: GroupManifestLink[] = [ + { + from: 'gateway', + to: 'orders-svc', + type: 'http', + contract: 'get::/api/orders', + role: 'consumer', + }, + ]; + + let seenParam: string | undefined; + const dbExecutors = new Map< + string, + (cypher: string, params?: Record) => Promise[]> + >([ + [ + 'orders-svc', + async (_cypher, params) => { + seenParam = params?.normalized as string; + if (seenParam === '/api/orders') { + return [ + { + uid: 'uid-orders-list', + name: 'listOrders', + filePath: 'src/orders.ts', + }, + ]; + } + return []; + }, + ], + ['gateway', async () => []], + ]); + + const result = await extractor.extractFromManifest(links, dbExecutors); + expect(seenParam).toBe('/api/orders'); + const provider = result.contracts.find((c) => c.role === 'provider'); + expect(provider?.symbolUid).toBe('uid-orders-list'); + }); + + it('returns null cleanly when no Route matches explicit-method http contract', async () => { + const links: GroupManifestLink[] = [ + { + from: 'gateway', + to: 'orders-svc', + type: 'http', + contract: 'GET::/api/orders', + role: 'consumer', + }, + ]; + + const dbExecutors = new Map< + string, + (cypher: string, params?: Record) => Promise[]> + >([ + ['orders-svc', async () => []], + ['gateway', async () => []], + ]); + + const result = await extractor.extractFromManifest(links, dbExecutors); + const provider = result.contracts.find((c) => c.role === 'provider'); + // No match → synthetic uid, caller falls back as today. + expect(provider?.symbolUid).toBe('manifest::orders-svc::http::GET::/api/orders'); + }); + + it('buildContractId round-trip regression for GET::/api/orders', async () => { + // Verifies buildContractId still produces http::GET::/api/orders for + // explicit-method form — i.e. the fix to resolveSymbol did not touch + // buildContractId. + const links: GroupManifestLink[] = [ + { + from: 'gateway', + to: 'orders-svc', + type: 'http', + contract: 'GET::/api/orders', + role: 'consumer', + }, + ]; + + const result = await extractor.extractFromManifest(links); + const provider = result.contracts.find((c) => c.role === 'provider'); + 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);