From 759c983dce35b67bca75d1aac4fadf5a85b8181b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gerg=C5=91=20Magyar?= Date: Tue, 14 Apr 2026 08:03:02 +0100 Subject: [PATCH 1/3] fix(extractors): resolve 3 silent contract mis-resolution bugs (#793) (#817) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(extractors): resolve 3 silent contract mis-resolution bugs (#793) Addresses Codex adversarial review findings for extractor contract resolution on the new group extractor surface. F1 (manifest-extractor): resolveSymbol passed the full "METHOD::path" contract string through normalizeRoutePath, producing "/GET::/api/orders" which never matches Route.name. Adds parseHttpContract() helper that strips the METHOD:: prefix before path normalization. Contract ID construction (buildContractId) is unchanged. F2 (http-route-extractor): graph-assisted backfill used path-only detections.find(), so multi-verb same-URL files attached the wrong verb/handler to provider rows and inferred the wrong verb on FETCHES consumer edges. Now requires path+method match when method is known, and skips backfill when method is unknown and multiple detections tie on path. F3 (grpc-extractor): resolveProtoConflict seeded bestScore=-1 and only replaced on strict >, so all-zero-score ties silently selected candidates[0]. Now computes all scores, counts ties at the top score, and returns null on ambiguity (caller skips contract emission and warns with service name + candidate paths). All three fixes are test-first; 73 tests pass across the three suites. No schema changes, no new dependencies, contract ID wire format (http::METHOD::path, grpc::pkg.Service/Method, http::*::path) preserved. * fix(extractors): address PR #817 review — ambiguous symbol pick + contract id casing 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. * chore: prettier formatting --- gitnexus-web/src/hooks/useAutoScroll.ts | 2 +- .../test/unit/use-auto-scroll.test.tsx | 4 +- .../core/group/extractors/grpc-extractor.ts | 45 +- .../group/extractors/http-route-extractor.ts | 40 +- .../group/extractors/manifest-extractor.ts | 49 ++- .../test/unit/group/grpc-extractor.test.ts | 116 +++++- .../unit/group/http-route-multi-verb.test.ts | 393 ++++++++++++++++++ .../unit/group/manifest-extractor.test.ts | 278 +++++++++++++ 8 files changed, 901 insertions(+), 26 deletions(-) create mode 100644 gitnexus/test/unit/group/http-route-multi-verb.test.ts diff --git a/gitnexus-web/src/hooks/useAutoScroll.ts b/gitnexus-web/src/hooks/useAutoScroll.ts index 58a0aedcc..55c2946f7 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 a8bc1a795..e58227d67 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 c6af9138a..b379a4dbd 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 0b07090e1..f2914613d 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 29c8f9b21..4c0d737b7 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 82d79cbd6..1664bd1a7 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 000000000..b2884fc60 --- /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 c2c67a33a..42ed86b84 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); From 1a597f3cc6a6d4ac1eed61fc6bb904050285655b Mon Sep 17 00:00:00 2001 From: Copilot <198982749+Copilot@users.noreply.github.com> Date: Tue, 14 Apr 2026 08:57:29 +0100 Subject: [PATCH 2/3] Fix npm arborist crash caused by tree-sitter-dart tarball URL format (#820) * Initial plan * fix: change tree-sitter-dart from tarball URL to git URL to fix npm arborist crash, add error handling and troubleshooting docs Agent-Logs-Url: https://github.com/abhigyanpatwari/GitNexus/sessions/382c76c6-89c3-463a-8631-2a5d6510be4c Co-authored-by: magyargergo <11230420+magyargergo@users.noreply.github.com> * refine error handler patterns and troubleshooting docs for arborist crash Agent-Logs-Url: https://github.com/abhigyanpatwari/GitNexus/sessions/909b319b-c367-40aa-8033-32dfb6231d4e Co-authored-by: magyargergo <11230420+magyargergo@users.noreply.github.com> * style: run prettier on changed files Agent-Logs-Url: https://github.com/abhigyanpatwari/GitNexus/sessions/50eb6b94-9300-4bf2-9b61-c2d78f637fc6 * fix: use github: shorthand for tree-sitter-dart to avoid SSH in CI Agent-Logs-Url: https://github.com/abhigyanpatwari/GitNexus/sessions/2ce3f4b3-4c1e-4c39-b824-c25cfe145529 Co-authored-by: magyargergo <11230420+magyargergo@users.noreply.github.com> * revert: use git+https:// for tree-sitter-dart instead of github: shorthand (fixes arborist crash from PR #811) Agent-Logs-Url: https://github.com/abhigyanpatwari/GitNexus/sessions/b786b68d-6c76-4054-88eb-ad46ea9f5b81 Co-authored-by: magyargergo <11230420+magyargergo@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: magyargergo <11230420+magyargergo@users.noreply.github.com> --- gitnexus/README.md | 50 +++++++++++++++++++++++++++++++++++++ gitnexus/package-lock.json | 6 ++--- gitnexus/package.json | 2 +- gitnexus/src/cli/analyze.ts | 24 +++++++++++++++++- 4 files changed, 77 insertions(+), 5 deletions(-) diff --git a/gitnexus/README.md b/gitnexus/README.md index 7e87c93b4..8c7888d66 100644 --- a/gitnexus/README.md +++ b/gitnexus/README.md @@ -234,6 +234,56 @@ Installed automatically by both `gitnexus analyze` (per-repo) and `gitnexus setu - Node.js >= 18 - Git repository (uses git for commit tracking) +## Troubleshooting + +### `Cannot destructure property 'package' of 'node.target' as it is null` + +This crash was caused by a dependency URL format that is incompatible with +certain npm/arborist versions ([npm/cli#8126](https://github.com/npm/cli/issues/8126)). +It is fixed in **gitnexus v1.6.2+**. Upgrade to the latest version: + +```bash +npx gitnexus@latest analyze # always uses the newest release +# — or — +npm install -g gitnexus@latest # upgrade a global install +``` + +If you still hit npm install issues after upgrading, these generic workarounds +may help: + +```bash +npm install -g npm@latest # update npm itself +npm cache clean --force # clear a possibly corrupt cache +``` + +### Installation fails with native module errors + +Some optional language grammars (Dart, Kotlin, Swift) require native compilation. If they fail, GitNexus still works — those languages will be skipped. + +If `npm install -g gitnexus` fails on native modules: + +```bash +# Ensure build tools are available (Linux/macOS) +# Ubuntu/Debian: sudo apt install python3 make g++ +# macOS: xcode-select --install + +# Retry installation +npm install -g gitnexus +``` + +### Analysis runs out of memory + +For very large repositories: + +```bash +# Increase Node.js heap size +NODE_OPTIONS="--max-old-space-size=16384" npx gitnexus analyze + +# Exclude large directories +echo "vendor/" >> .gitnexusignore +echo "dist/" >> .gitnexusignore +``` + ## Privacy - All processing happens locally on your machine diff --git a/gitnexus/package-lock.json b/gitnexus/package-lock.json index c9ca9017f..bc4fc7b20 100644 --- a/gitnexus/package-lock.json +++ b/gitnexus/package-lock.json @@ -62,7 +62,7 @@ "node": ">=20.0.0" }, "optionalDependencies": { - "tree-sitter-dart": "https://github.com/UserNobody14/tree-sitter-dart/archive/80e23c07b64494f7e21090bb3450223ef0b192f4.tar.gz", + "tree-sitter-dart": "git+https://github.com/UserNobody14/tree-sitter-dart.git#80e23c07b64494f7e21090bb3450223ef0b192f4", "tree-sitter-kotlin": "^0.3.8", "tree-sitter-proto": "file:./vendor/tree-sitter-proto", "tree-sitter-swift": "^0.6.0" @@ -5132,8 +5132,8 @@ }, "node_modules/tree-sitter-dart": { "version": "1.0.0", - "resolved": "https://github.com/UserNobody14/tree-sitter-dart/archive/80e23c07b64494f7e21090bb3450223ef0b192f4.tar.gz", - "integrity": "sha512-aqLZTEji2vAZPdbaCSjR0SXJGzFRKD//7VtrSV3st9bgrCM2tsXxXAHZlMlQLOCt7K2yKxM5K3gNXYph8TCjCQ==", + "resolved": "git+https://github.com/UserNobody14/tree-sitter-dart.git#80e23c07b64494f7e21090bb3450223ef0b192f4", + "integrity": "sha512-Bs/1wAOIJ2akPEXlE/XVpuES19Oo3NqoSJRJ/0N2r38qAd9nTXdqmaGHQ44/JXnA6QHcbgD2YzCCc4wUc98cyQ==", "hasInstallScript": true, "license": "ISC", "optional": true, diff --git a/gitnexus/package.json b/gitnexus/package.json index d0d0704f5..571a341c5 100644 --- a/gitnexus/package.json +++ b/gitnexus/package.json @@ -84,7 +84,7 @@ "uuid": "^13.0.0" }, "optionalDependencies": { - "tree-sitter-dart": "https://github.com/UserNobody14/tree-sitter-dart/archive/80e23c07b64494f7e21090bb3450223ef0b192f4.tar.gz", + "tree-sitter-dart": "git+https://github.com/UserNobody14/tree-sitter-dart.git#80e23c07b64494f7e21090bb3450223ef0b192f4", "tree-sitter-kotlin": "^0.3.8", "tree-sitter-proto": "file:./vendor/tree-sitter-proto", "tree-sitter-swift": "^0.6.0" diff --git a/gitnexus/src/cli/analyze.ts b/gitnexus/src/cli/analyze.ts index dc8fd87a9..26d1ae8c6 100644 --- a/gitnexus/src/cli/analyze.ts +++ b/gitnexus/src/cli/analyze.ts @@ -297,7 +297,7 @@ export const analyzeCommand = async (inputPath?: string, options?: AnalyzeOption const msg = err.message || String(err); console.error(`\n Analysis failed: ${msg}\n`); - // Provide helpful guidance for known large-repo failure modes + // Provide helpful guidance for known failure modes if ( msg.includes('Maximum call stack size exceeded') || msg.includes('call stack') || @@ -314,6 +314,28 @@ export const analyzeCommand = async (inputPath?: string, options?: AnalyzeOption console.error(' 2. Increase Node.js heap: NODE_OPTIONS="--max-old-space-size=16384"'); console.error(' 3. Increase stack size: NODE_OPTIONS="--stack-size=4096"'); console.error(''); + } else if (msg.includes('ERESOLVE') || msg.includes('Could not resolve dependency')) { + // Note: the original arborist "Cannot destructure property 'package' of + // 'node.target'" crash happens inside npm *before* gitnexus code runs, + // so it can't be caught here. This branch handles dependency-resolution + // errors that surface at runtime (e.g. dynamic require failures). + console.error(' This looks like an npm dependency resolution issue.'); + console.error(' Suggestions:'); + console.error(' 1. Clear the npm cache: npm cache clean --force'); + console.error(' 2. Update npm: npm install -g npm@latest'); + console.error(' 3. Reinstall gitnexus: npm install -g gitnexus@latest'); + console.error(' 4. Or try npx directly: npx gitnexus@latest analyze'); + console.error(''); + } else if ( + msg.includes('MODULE_NOT_FOUND') || + msg.includes('Cannot find module') || + msg.includes('ERR_MODULE_NOT_FOUND') + ) { + console.error(' A required module could not be loaded. The installation may be corrupt.'); + console.error(' Suggestions:'); + console.error(' 1. Reinstall: npm install -g gitnexus@latest'); + console.error(' 2. Clear cache: npm cache clean --force && npx gitnexus@latest analyze'); + console.error(''); } process.exitCode = 1; From 9ad1984b17e4c53f7e3085226f0e4ce17ad9354b Mon Sep 17 00:00:00 2001 From: "Filipe Oliveira (Redis)" Date: Tue, 14 Apr 2026 09:39:17 +0100 Subject: [PATCH 3/3] fix: resolve C/C++ cross-file calls through transitive #include chains (#816) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix: resolve C/C++ cross-file calls through transitive #include chains In C/C++, #include is transitive: if a.c includes b.h and b.h includes c.h, then a.c can call any function declared in c.h. The wildcard import synthesis only walked direct imports (1 hop), missing symbols reachable through transitive header chains. This is the dominant pattern in large C codebases — Redis's db.c includes server.h which includes dict.h, so db.c should resolve calls to dictFind() declared in dict.h and defined in dict.c. Before this fix, those cross-file call edges were missing entirely. The fix expands the import closure transitively for C/C++ files before synthesizing wildcard bindings. A BFS walks ctx.importMap and graphImports to collect all transitively reachable headers, then passes the full closure to synthesizeForFile. Tested on Redis (github.com/redis/redis): - Before: dictFetchValue had 0 cross-file callers, processCommand had 0 - After: dictFetchValue has 9 callers, processCommand has 1, +1946 edges total Fixes #813 * refactor(ingestion): dispatch wildcard synthesis by import-semantics strategy Generalize PR #816's C/C++ transitive #include fix into a language-agnostic strategy pattern. The `wildcard-synthesis.ts` pipeline phase no longer references `SupportedLanguages.C` / `SupportedLanguages.CPlusPlus` — it dispatches on `provider.importSemantics` via an exhaustive `switch`. Also fixes a correctness bug the original BFS introduced: `queue.pop()` (LIFO/DFS) reversed the iteration order of `#include` directives, which — combined with first-seen-wins dedup in `synthesizeForFile` — silently bound overloaded symbols to the wrong header. For the `cpp-calls` fixture, `write_audit("hello")` was being resolved to `zero.h`'s arity-0 overload instead of `one.h`'s arity-1 overload, breaking arity narrowing. Switched to FIFO (`queue.shift()`) with direct imports seeded in declaration order. Taxonomy (researched across 20+ languages + stack-graphs / SCIP prior art): | Tag | Traversal | Languages | |---------------------|-----------------|------------------------------------| | named | none | TS, JS, Java, C#, Rust, PHP, Kotlin| | wildcard-transitive | BFS closure | C, C++ | | wildcard-leaf | single hop | Go, Ruby, Swift, Dart | | namespace | none at import | Python | | explicit-reexport | topological DAG | (scaffold; TS `export *` future) | Changes: - Widen `ImportSemantics` union from 3 to 5 tags with full taxonomy JSDoc - Retag 5 providers: c-cpp (x2) → wildcard-transitive; dart, go, ruby, swift → wildcard-leaf - Move BFS closure into `wildcard-synthesis.ts` as `expandTransitiveIncludeClosure` (pipeline-owned; providers stay pure declarations) - Replace `if (lang === C || CPP)` with `dispatchSynthesis` helper called by both Loop 1 (ctx.importMap) and Loop 2 (graphImports) so a future transitive language whose edges arrive via graphImports gets closure expansion consistently - `never`-assertion default arm forces compile-time exhaustiveness - `explicit-reexport` arm falls through to leaf behavior (scaffold; TODO: implement re-export DAG walk for TS `export *` / Rust `pub use`) - New unit tests covering circular includes, deep chains, diamond dedup, graphImports-only paths, and order-preservation (the regression fix) Verification: - All existing C/C++ transitive tests pass unchanged - Previously failing `cpp.test.ts > resolves run → write_audit to one.h via arity narrowing` now passes - `tsc --noEmit` clean - 225/225 tests pass across wildcard-synthesis, cross-file-binding, cpp resolver, and new closure unit tests * fix(ingestion): bound closure size, O(1) dequeue, track Strategy 4 (#816 review) Address @xkonjin's review feedback on the import-resolution strategy refactor: 1. **DoS guard**: cap transitive closures at 5,000 files via `MAX_TRANSITIVE_CLOSURE_SIZE`. Pathological codebases (boost-style headers, monoheader kernels) could previously produce closures with tens of thousands of entries per translation unit. BFS now stops early and returns a partial closure rather than risking OOM. The closest-headers-first BFS ordering means the partial closure still contains the files overload resolution cares about. 2. **Perf**: replace `Array.prototype.shift()` (O(n)) with a head-index queue (O(1) dequeue). Deep chains previously had quadratic BFS behavior; now linear in closure size. 3. **Strategy 4 tracking**: change TODO in `dispatchSynthesis` to `TODO(#821)` referencing the filed issue for TS `export *` / Rust `pub use` DAG-walk implementation, and clarify that today's leaf fallthrough preserves correctness for direct imports — only the extra re-export traversal is missing. 4. **Test**: new unit test exercising the 5,000-file cap on a 10k-file synthetic chain, verifying partial-closure invariants (starts from importer side, bounded, deep nodes excluded). Not addressed in this commit (followups): - Review point 3 (graphImports-only deep-chain *integration* fixture): unit tests already exercise the `graphImports` traversal path directly in isolation and combined with `importMap`. A fixture that stresses graphImports-only transitive resolution is valuable but requires understanding when the pipeline populates graphImports distinctly from ctx.importMap — tracking as a followup rather than blocking this PR. --------- Co-authored-by: Gergo Magyar --- .../src/core/ingestion/language-provider.ts | 34 +++- .../src/core/ingestion/languages/c-cpp.ts | 4 +- gitnexus/src/core/ingestion/languages/dart.ts | 4 +- gitnexus/src/core/ingestion/languages/go.ts | 4 +- gitnexus/src/core/ingestion/languages/ruby.ts | 2 +- .../src/core/ingestion/languages/swift.ts | 4 +- .../pipeline-phases/wildcard-synthesis.ts | 162 +++++++++++++++++- .../cross-file-binding/c-cross-file/src/db.c | 12 ++ .../c-cross-file/src/dict.c | 12 ++ .../c-cross-file/src/dict.h | 12 ++ .../c-cross-file/src/server.h | 8 + .../integration/cross-file-binding.test.ts | 36 ++++ .../unit/transitive-include-closure.test.ts | 101 +++++++++++ 13 files changed, 375 insertions(+), 20 deletions(-) create mode 100644 gitnexus/test/fixtures/cross-file-binding/c-cross-file/src/db.c create mode 100644 gitnexus/test/fixtures/cross-file-binding/c-cross-file/src/dict.c create mode 100644 gitnexus/test/fixtures/cross-file-binding/c-cross-file/src/dict.h create mode 100644 gitnexus/test/fixtures/cross-file-binding/c-cross-file/src/server.h create mode 100644 gitnexus/test/unit/transitive-include-closure.test.ts diff --git a/gitnexus/src/core/ingestion/language-provider.ts b/gitnexus/src/core/ingestion/language-provider.ts index fae696be6..736c3b666 100644 --- a/gitnexus/src/core/ingestion/language-provider.ts +++ b/gitnexus/src/core/ingestion/language-provider.ts @@ -30,8 +30,30 @@ export type CaptureMap = Record; // so `core/ingestion/model/resolve.ts` can consume it without importing from // this file (which would pull in the full language-registry dependency graph). -/** How a language handles imports — determines wildcard synthesis behavior. */ -export type ImportSemantics = 'named' | 'wildcard' | 'namespace'; +/** + * How a language handles imports — determines wildcard synthesis behavior. + * + * Import resolution is a graph-traversal policy with multiple distinct strategies, + * analogous to MRO for method resolution. Each tag picks a strategy: + * + * | Tag | Mechanism | Traversal | Languages | + * |-----------------------|------------------------------------------------|---------------------|--------------------------------------------| + * | `named` | Per-symbol imports | None (use-site) | JS/TS, Java, C#, Rust, PHP, Kotlin, Vue | + * | `wildcard-transitive` | Textual paste, symbols chain through files | BFS closure | C, C++ (future: Obj-C, Fortran, Nim) | + * | `wildcard-leaf` | Whole public API, single hop | None (direct only) | Go, Ruby, Swift, Dart | + * | `namespace` | Qualified handle; symbols resolved at call site| None at import | Python | + * | `explicit-reexport` | Opt-in per-symbol re-export (SCAFFOLD) | Topological DAG | (future: TS `export *`, Rust `pub use`) | + * + * The `explicit-reexport` tag is a compile-time scaffold; no provider claims it yet. + * It falls through to `wildcard-leaf` behavior in synthesis so today's TS/Rust + * handling is unchanged. A future PR will implement the DAG walk for `export *`. + */ +export type ImportSemantics = + | 'named' + | 'wildcard-transitive' + | 'wildcard-leaf' + | 'namespace' + | 'explicit-reexport'; /** * Everything a language needs to provide. @@ -68,10 +90,12 @@ interface LanguageProviderConfig { /** Named binding extraction from import statements. * Default: undefined (language uses wildcard/whole-module imports). */ readonly namedBindingExtractor?: NamedBindingExtractorFn; - /** How this language handles imports. + /** How this language handles imports. See `ImportSemantics` for the full taxonomy. * - 'named': per-symbol imports (JS/TS, Java, C#, Rust, PHP, Kotlin) - * - 'wildcard': whole-module imports, needs synthesis (Go, Ruby, C/C++, Swift) - * - 'namespace': namespace imports, needs moduleAliasMap (Python) + * - 'wildcard-transitive': textual-include closure; imports chain through files (C, C++) + * - 'wildcard-leaf': whole-module single-hop imports; no transitive chaining (Go, Ruby, Swift, Dart) + * - 'namespace': qualified namespace imports, needs moduleAliasMap (Python) + * - 'explicit-reexport': opt-in per-symbol re-export (scaffold; no provider uses yet) * Default: 'named'. */ readonly importSemantics?: ImportSemantics; /** Language-specific transformation of raw import path text before resolution. diff --git a/gitnexus/src/core/ingestion/languages/c-cpp.ts b/gitnexus/src/core/ingestion/languages/c-cpp.ts index a0508ff5b..5b887634f 100644 --- a/gitnexus/src/core/ingestion/languages/c-cpp.ts +++ b/gitnexus/src/core/ingestion/languages/c-cpp.ts @@ -321,7 +321,7 @@ export const cProvider = defineLanguage({ typeConfig: cCppConfig, exportChecker: cCppExportChecker, importResolver: resolveCImport, - importSemantics: 'wildcard', + importSemantics: 'wildcard-transitive', fieldExtractor: createFieldExtractor(cFieldConfig), methodExtractor: createMethodExtractor({ ...cMethodConfig, @@ -339,7 +339,7 @@ export const cppProvider = defineLanguage({ typeConfig: cCppConfig, exportChecker: cCppExportChecker, importResolver: resolveCppImport, - importSemantics: 'wildcard', + importSemantics: 'wildcard-transitive', mroStrategy: 'leftmost-base', fieldExtractor: createFieldExtractor(cppFieldConfig), methodExtractor: createMethodExtractor({ diff --git a/gitnexus/src/core/ingestion/languages/dart.ts b/gitnexus/src/core/ingestion/languages/dart.ts index 591519895..7dc6769c6 100644 --- a/gitnexus/src/core/ingestion/languages/dart.ts +++ b/gitnexus/src/core/ingestion/languages/dart.ts @@ -2,7 +2,7 @@ * Dart Language Provider * * Dart traits: - * - importSemantics: 'wildcard' (Dart imports bring everything public into scope) + * - importSemantics: 'wildcard-leaf' (Dart imports bring everything public into scope) * - exportChecker: public if no leading underscore * - Dart SDK imports (dart:*) and external packages are skipped * - enclosingFunctionFinder: Dart's tree-sitter grammar places function_body @@ -90,7 +90,7 @@ export const dartProvider = defineLanguage({ typeConfig: dartConfig, exportChecker: dartExportChecker, importResolver: resolveDartImport, - importSemantics: 'wildcard', + importSemantics: 'wildcard-leaf', fieldExtractor: createFieldExtractor(dartFieldConfig), methodExtractor: createMethodExtractor(dartMethodConfig), classExtractor: createClassExtractor({ diff --git a/gitnexus/src/core/ingestion/languages/go.ts b/gitnexus/src/core/ingestion/languages/go.ts index 803e70bbb..2a2b35f50 100644 --- a/gitnexus/src/core/ingestion/languages/go.ts +++ b/gitnexus/src/core/ingestion/languages/go.ts @@ -5,7 +5,7 @@ * LanguageProvider, following the Strategy pattern used by the pipeline. * * Key Go traits: - * - importSemantics: 'wildcard' (Go imports entire packages) + * - importSemantics: 'wildcard-leaf' (Go imports entire packages) * - callRouter: present (Go method calls may need routing) */ @@ -28,7 +28,7 @@ export const goProvider = defineLanguage({ typeConfig: goConfig, exportChecker: goExportChecker, importResolver: resolveGoImport, - importSemantics: 'wildcard', + importSemantics: 'wildcard-leaf', fieldExtractor: createFieldExtractor(goFieldConfig), methodExtractor: createMethodExtractor(goMethodConfig), classExtractor: createClassExtractor({ diff --git a/gitnexus/src/core/ingestion/languages/ruby.ts b/gitnexus/src/core/ingestion/languages/ruby.ts index 00b566068..a15b5439c 100644 --- a/gitnexus/src/core/ingestion/languages/ruby.ts +++ b/gitnexus/src/core/ingestion/languages/ruby.ts @@ -107,7 +107,7 @@ export const rubyProvider = defineLanguage({ exportChecker: rubyExportChecker, importResolver: resolveRubyImport, callRouter: routeRubyCall, - importSemantics: 'wildcard', + importSemantics: 'wildcard-leaf', resolveEnclosingOwner(node) { // Ruby singleton_class (class << self) should resolve to the enclosing // class or module for owner/container resolution (HAS_METHOD edges, class IDs). diff --git a/gitnexus/src/core/ingestion/languages/swift.ts b/gitnexus/src/core/ingestion/languages/swift.ts index 7d01b4912..314c27c1d 100644 --- a/gitnexus/src/core/ingestion/languages/swift.ts +++ b/gitnexus/src/core/ingestion/languages/swift.ts @@ -5,7 +5,7 @@ * LanguageProvider, following the Strategy pattern used by the pipeline. * * Key Swift traits: - * - importSemantics: 'wildcard' (Swift imports entire modules) + * - importSemantics: 'wildcard-leaf' (Swift imports entire modules) * - heritageDefaultEdge: 'IMPLEMENTS' (protocols are more common than class inheritance) * - implicitImportWirer: all files in the same SPM target see each other */ @@ -238,7 +238,7 @@ export const swiftProvider = defineLanguage({ typeConfig: swiftConfig, exportChecker: swiftExportChecker, importResolver: resolveSwiftImport, - importSemantics: 'wildcard', + importSemantics: 'wildcard-leaf', heritageDefaultEdge: 'IMPLEMENTS', fieldExtractor: createFieldExtractor(swiftFieldConfig), methodExtractor: createMethodExtractor({ diff --git a/gitnexus/src/core/ingestion/pipeline-phases/wildcard-synthesis.ts b/gitnexus/src/core/ingestion/pipeline-phases/wildcard-synthesis.ts index c2c0f986d..83f38bc60 100644 --- a/gitnexus/src/core/ingestion/pipeline-phases/wildcard-synthesis.ts +++ b/gitnexus/src/core/ingestion/pipeline-phases/wildcard-synthesis.ts @@ -15,8 +15,10 @@ import type { KnowledgeGraph } from '../../graph/types.js'; import type { createResolutionContext } from '../model/resolution-context.js'; -import { getLanguageFromFilename, SupportedLanguages } from 'gitnexus-shared'; +import { getLanguageFromFilename } from 'gitnexus-shared'; +import type { SupportedLanguages } from 'gitnexus-shared'; import { providers, getProviderForFile } from '../languages/index.js'; +import type { LanguageProvider, ImportSemantics } from '../language-provider.js'; // ── Constants ────────────────────────────────────────────────────────────── @@ -41,10 +43,29 @@ const IMPORTABLE_SYMBOL_LABELS = new Set([ * for C/C++ files that include many large headers. */ const MAX_SYNTHETIC_BINDINGS_PER_FILE = 1000; +/** Max files allowed in a single transitive include closure. Guards against + * OOM on pathological C/C++ codebases (boost, Linux kernel-style monoheaders) + * where a single translation unit can transitively reach many thousands of + * headers. When the cap is hit, BFS expansion stops early — the file still + * synthesizes bindings from the partial closure rather than failing. */ +const MAX_TRANSITIVE_CLOSURE_SIZE = 5000; + +/** Import semantics tags whose languages need synthesis of whole-module imports. + * `wildcard-transitive` (C/C++) and `wildcard-leaf` (Go, Ruby, Swift, Dart) are + * the file-based wildcard strategies. `explicit-reexport` is a scaffold tag — + * no provider uses it yet, but it goes through the same leaf-style synthesis + * path today because a re-exporter is still an importer; only the extra DAG + * walk to surface re-exported symbols is missing (future work). */ +const WILDCARD_SEMANTICS: ReadonlySet = new Set([ + 'wildcard-transitive', + 'wildcard-leaf', + 'explicit-reexport', +]); + /** Languages with whole-module import semantics (derived from providers at module load). */ const WILDCARD_LANGUAGES = new Set( Object.values(providers) - .filter((p) => p.importSemantics === 'wildcard') + .filter((p) => WILDCARD_SEMANTICS.has(p.importSemantics)) .map((p) => p.id), ); @@ -66,6 +87,84 @@ export function needsSynthesis(lang: SupportedLanguages): boolean { return SYNTHESIS_LANGUAGES.has(lang); } +// ── Strategy implementations ─────────────────────────────────────────────── + +/** + * Strategy implementation for `importSemantics: 'wildcard-transitive'` (C, C++). + * + * Textual-include languages chain symbols through files: if `dict.c` includes + * `server.h` and `server.h` includes `dict.h`, then `dict.c` sees symbols from + * all three files. This helper walks the include graph (combining both the + * ingestion-context `importMap` and the graph-level IMPORTS edges) until the + * closure is stable. + * + * **Order matters.** The returned `Set` preserves iteration order (insertion + * order). `synthesizeWildcardImportBindings` dedupes bindings by symbol name + * on a first-seen-wins basis, so this closure's ordering determines which + * declaration wins when multiple headers export the same name (e.g. overloaded + * free functions like `write_audit()` vs `write_audit(const char*)` in + * different headers). We therefore: + * 1. Seed the closure with direct imports in declaration order (matches the + * order of `#include` directives in the source file). + * 2. Use FIFO / true BFS (`queue.shift()`) for transitive expansion, so + * closer headers are seen before deeper ones. + * + * Cycle-safe: the `closure.has(file)` guard prevents infinite loops on circular + * header includes, which are valid C/C++ when paired with `#pragma once` or + * include guards. + * + * Size-bounded: the closure is capped at `MAX_TRANSITIVE_CLOSURE_SIZE` files to + * prevent OOM on pathological codebases (e.g. boost, monoheader kernel code) + * where one translation unit can transitively reach tens of thousands of + * headers. Partial closures still yield useful bindings for the cluster of + * headers closest to the importer, which is what overload resolution and + * cross-file call resolution care about. + * + * Queue implementation: uses a head-index over a growing array (O(1) dequeue) + * instead of `Array.prototype.shift()` (O(n)) so deep chains stay linear. + */ +export function expandTransitiveIncludeClosure( + directImports: Iterable, + importMap: ReadonlyMap>, + graphImports: ReadonlyMap>, +): Set { + const closure = new Set(); + const queue: string[] = []; + let head = 0; // O(1) dequeue: advance the head index instead of shift()-ing. + + const tryEnqueue = (file: string): boolean => { + if (closure.has(file)) return true; + if (closure.size >= MAX_TRANSITIVE_CLOSURE_SIZE) return false; + closure.add(file); + queue.push(file); + return true; + }; + + // Seed direct imports in declaration order (see JSDoc on order-sensitivity). + for (const f of directImports) { + if (!tryEnqueue(f)) break; + } + // True BFS for transitive reach: head-index FIFO preserves the "closer + // headers first" ordering that overload resolution depends on. + while (head < queue.length) { + if (closure.size >= MAX_TRANSITIVE_CLOSURE_SIZE) break; + const file = queue[head++]!; + const nested = importMap.get(file); + if (nested) { + for (const n of nested) { + if (!tryEnqueue(n)) break; + } + } + const nestedGraph = graphImports.get(file); + if (nestedGraph) { + for (const n of nestedGraph) { + if (!tryEnqueue(n)) break; + } + } + } + return closure; +} + // ── Main synthesis function ──────────────────────────────────────────────── /** @@ -149,16 +248,67 @@ export function synthesizeWildcardImportBindings( } }; - // Synthesize from ctx.importMap (Ruby, C/C++, Swift file-based imports) + /** + * Dispatch wildcard synthesis by the file's language provider strategy. + * + * Strategy tags (see `ImportSemantics`): + * - `wildcard-transitive`: expand the include closure first (C/C++ #include + * chains — e.g. `dict.c` → `server.h` → `dict.h` so `dictFind` resolves + * across header chains) + * - `wildcard-leaf`: synthesize from direct imports only (Go, Ruby, Swift, Dart) + * - `explicit-reexport`: scaffold tag; falls through to leaf behavior. + * TODO(#821): implement re-export DAG walk for TS `export *` / Rust + * `pub use`. The leaf fallthrough preserves today's TS/Rust behavior + * (their direct imports still synthesize correctly); only the extra + * re-export DAG walk for barrel-file correctness is missing. + * - `namespace` / `named`: no-op here (namespace handled in Loop 3 below, + * named needs no synthesis). + * + * Used by both Loop 1 (ctx.importMap) and Loop 2 (graphImports) so a future + * transitive-import language whose edges arrive via graphImports gets closure + * expansion consistently regardless of edge source. + */ + const dispatchSynthesis = ( + filePath: string, + importedFiles: ReadonlySet, + provider: LanguageProvider, + ) => { + switch (provider.importSemantics) { + case 'wildcard-transitive': + synthesizeForFile( + filePath, + expandTransitiveIncludeClosure(importedFiles, ctx.importMap, graphImports), + ); + return; + case 'wildcard-leaf': + case 'explicit-reexport': + synthesizeForFile(filePath, importedFiles); + return; + case 'namespace': + case 'named': + return; + default: { + const _exhaustive: never = provider.importSemantics; + void _exhaustive; + } + } + }; + + // Loop 1: synthesize from ctx.importMap (Ruby, C/C++, Swift, Dart file-based imports). for (const [filePath, importedFiles] of ctx.importMap) { const lang = getLanguageFromFilename(filePath); if (!lang || !isWildcardImportLanguage(lang)) continue; - synthesizeForFile(filePath, importedFiles); + const provider = getProviderForFile(filePath); + if (!provider) continue; + dispatchSynthesis(filePath, importedFiles, provider); } - // Synthesize from graph IMPORTS edges (Go and other wildcard-import languages) + // Loop 2: synthesize from graph IMPORTS edges (Go and other wildcard-import + // languages whose edges live in the graph rather than ctx.importMap). for (const [filePath, importedFiles] of graphImports) { - synthesizeForFile(filePath, importedFiles); + const provider = getProviderForFile(filePath); + if (!provider) continue; + dispatchSynthesis(filePath, importedFiles, provider); } // Build Python module-alias maps for namespace-import languages. diff --git a/gitnexus/test/fixtures/cross-file-binding/c-cross-file/src/db.c b/gitnexus/test/fixtures/cross-file-binding/c-cross-file/src/db.c new file mode 100644 index 000000000..deb76c90e --- /dev/null +++ b/gitnexus/test/fixtures/cross-file-binding/c-cross-file/src/db.c @@ -0,0 +1,12 @@ +#include "server.h" + +void lookupKey(const char *key) { + dictEntry *entry = dictFind(key); + if (entry) { + void *val = entry->val; + } +} + +void dbGet(const char *key) { + void *val = dictFetchValue(key); +} diff --git a/gitnexus/test/fixtures/cross-file-binding/c-cross-file/src/dict.c b/gitnexus/test/fixtures/cross-file-binding/c-cross-file/src/dict.c new file mode 100644 index 000000000..514a6c866 --- /dev/null +++ b/gitnexus/test/fixtures/cross-file-binding/c-cross-file/src/dict.c @@ -0,0 +1,12 @@ +#include "dict.h" +#include + +dictEntry *dictFind(const char *key) { + return NULL; +} + +void *dictFetchValue(const char *key) { + dictEntry *entry = dictFind(key); + if (entry) return entry->val; + return NULL; +} diff --git a/gitnexus/test/fixtures/cross-file-binding/c-cross-file/src/dict.h b/gitnexus/test/fixtures/cross-file-binding/c-cross-file/src/dict.h new file mode 100644 index 000000000..9d17d19bf --- /dev/null +++ b/gitnexus/test/fixtures/cross-file-binding/c-cross-file/src/dict.h @@ -0,0 +1,12 @@ +#ifndef DICT_H +#define DICT_H + +typedef struct dictEntry { + void *key; + void *val; +} dictEntry; + +dictEntry *dictFind(const char *key); +void *dictFetchValue(const char *key); + +#endif diff --git a/gitnexus/test/fixtures/cross-file-binding/c-cross-file/src/server.h b/gitnexus/test/fixtures/cross-file-binding/c-cross-file/src/server.h new file mode 100644 index 000000000..e7ccf7fdd --- /dev/null +++ b/gitnexus/test/fixtures/cross-file-binding/c-cross-file/src/server.h @@ -0,0 +1,8 @@ +#ifndef SERVER_H +#define SERVER_H + +#include "dict.h" + +void processCommand(const char *cmd); + +#endif diff --git a/gitnexus/test/integration/cross-file-binding.test.ts b/gitnexus/test/integration/cross-file-binding.test.ts index 33063f94f..88ba776ed 100644 --- a/gitnexus/test/integration/cross-file-binding.test.ts +++ b/gitnexus/test/integration/cross-file-binding.test.ts @@ -436,6 +436,42 @@ describe('Phase 9 — Cross-File Call-Result Binding: C++', () => { }); }); +describe('Cross-File Call Resolution: pure C transitive #include', () => { + let result: PipelineResult; + + beforeAll(async () => { + result = await runPipelineFromRepo(path.join(CROSS_FILE_FIXTURES, 'c-cross-file'), () => {}); + }, 60000); + + it('detects dictFind and dictFetchValue functions', () => { + expect(getNodesByLabel(result, 'Function')).toContain('dictFind'); + expect(getNodesByLabel(result, 'Function')).toContain('dictFetchValue'); + }); + + it('detects lookupKey and dbGet in db.c', () => { + expect(getNodesByLabel(result, 'Function')).toContain('lookupKey'); + expect(getNodesByLabel(result, 'Function')).toContain('dbGet'); + }); + + it('resolves dictFind() call in db.c to dict via transitive header chain', () => { + const calls = getRelationships(result, 'CALLS'); + const crossFileCall = calls.find( + (c) => + c.target === 'dictFind' && c.source === 'lookupKey' && c.targetFilePath.includes('dict'), + ); + expect(crossFileCall).toBeDefined(); + }); + + it('resolves dictFetchValue() call in db.c to dict via transitive header chain', () => { + const calls = getRelationships(result, 'CALLS'); + const crossFileCall = calls.find( + (c) => + c.target === 'dictFetchValue' && c.source === 'dbGet' && c.targetFilePath.includes('dict'), + ); + expect(crossFileCall).toBeDefined(); + }); +}); + describe('Phase 9 — Cross-File Call-Result Binding: C#', () => { let result: PipelineResult; diff --git a/gitnexus/test/unit/transitive-include-closure.test.ts b/gitnexus/test/unit/transitive-include-closure.test.ts new file mode 100644 index 000000000..fa9c0e6c9 --- /dev/null +++ b/gitnexus/test/unit/transitive-include-closure.test.ts @@ -0,0 +1,101 @@ +/** + * Unit tests for `expandTransitiveIncludeClosure` — the C/C++ Strategy 1 + * (`wildcard-transitive`) implementation extracted from `wildcard-synthesis.ts`. + * + * These tests exercise the BFS/DFS closure algorithm in isolation, without + * running the full pipeline. They cover edge cases flagged in PR #816 review: + * circular header includes, deep chains, and graphImports-only transitive paths. + */ + +import { describe, it, expect } from 'vitest'; +import { expandTransitiveIncludeClosure } from '../../src/core/ingestion/pipeline-phases/wildcard-synthesis.js'; + +const EMPTY = new Map>(); + +describe('expandTransitiveIncludeClosure', () => { + it('returns the direct imports when none are chained', () => { + const direct = new Set(['a.h', 'b.h']); + const closure = expandTransitiveIncludeClosure(direct, EMPTY, EMPTY); + expect([...closure].sort()).toEqual(['a.h', 'b.h']); + }); + + it('expands a two-hop chain via importMap (a.c → b.h → c.h)', () => { + const importMap = new Map>([['b.h', new Set(['c.h'])]]); + const closure = expandTransitiveIncludeClosure(new Set(['b.h']), importMap, EMPTY); + expect([...closure].sort()).toEqual(['b.h', 'c.h']); + }); + + it('expands a deep 5-level chain (A → B → C → D → E)', () => { + const importMap = new Map>([ + ['B.h', new Set(['C.h'])], + ['C.h', new Set(['D.h'])], + ['D.h', new Set(['E.h'])], + ]); + const closure = expandTransitiveIncludeClosure(new Set(['B.h']), importMap, EMPTY); + expect([...closure].sort()).toEqual(['B.h', 'C.h', 'D.h', 'E.h']); + }); + + it('terminates on circular header includes (A.h ↔ B.h)', () => { + const importMap = new Map>([ + ['A.h', new Set(['B.h'])], + ['B.h', new Set(['A.h'])], + ]); + const closure = expandTransitiveIncludeClosure(new Set(['A.h']), importMap, EMPTY); + expect([...closure].sort()).toEqual(['A.h', 'B.h']); + }); + + it('terminates on self-referential include (A.h includes A.h)', () => { + const importMap = new Map>([['A.h', new Set(['A.h'])]]); + const closure = expandTransitiveIncludeClosure(new Set(['A.h']), importMap, EMPTY); + expect([...closure]).toEqual(['A.h']); + }); + + it('expands through graphImports edges when importMap is empty', () => { + const graphImports = new Map>([['b.h', new Set(['c.h'])]]); + const closure = expandTransitiveIncludeClosure(new Set(['b.h']), EMPTY, graphImports); + expect([...closure].sort()).toEqual(['b.h', 'c.h']); + }); + + it('combines importMap and graphImports in one traversal', () => { + const importMap = new Map>([['b.h', new Set(['c.h'])]]); + const graphImports = new Map>([['c.h', new Set(['d.h'])]]); + const closure = expandTransitiveIncludeClosure(new Set(['b.h']), importMap, graphImports); + expect([...closure].sort()).toEqual(['b.h', 'c.h', 'd.h']); + }); + + it('returns an empty set when given no direct imports', () => { + const closure = expandTransitiveIncludeClosure(new Set(), EMPTY, EMPTY); + expect(closure.size).toBe(0); + }); + + it('caps closure size to prevent OOM on pathological codebases', () => { + // Build a synthetic include graph of 10,000 files, each including the next. + // The cap (5000) should halt BFS early with a partial but bounded closure. + const importMap = new Map>(); + for (let i = 0; i < 10_000; i++) { + importMap.set(`h${i}.h`, new Set([`h${i + 1}.h`])); + } + const closure = expandTransitiveIncludeClosure(new Set(['h0.h']), importMap, EMPTY); + expect(closure.size).toBe(5000); + // Partial closure still starts from the importer's side (BFS ordering). + expect(closure.has('h0.h')).toBe(true); + expect(closure.has('h1.h')).toBe(true); + expect(closure.has('h9999.h')).toBe(false); + }); + + it('deduplicates when a file is reachable through multiple paths (diamond)', () => { + // A + // / \ + // B C + // \ / + // D + const importMap = new Map>([ + ['A.h', new Set(['B.h', 'C.h'])], + ['B.h', new Set(['D.h'])], + ['C.h', new Set(['D.h'])], + ]); + const closure = expandTransitiveIncludeClosure(new Set(['A.h']), importMap, EMPTY); + expect([...closure].sort()).toEqual(['A.h', 'B.h', 'C.h', 'D.h']); + expect(closure.size).toBe(4); // D.h appears once + }); +});