From 2f28bfc3b9ec3876b054f6bf328c132893561b9f Mon Sep 17 00:00:00 2001 From: ivkond Date: Mon, 13 Apr 2026 07:16:44 +0000 Subject: [PATCH] refactor(group): address Copilot review feedback on #796 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Six fixes suggested by the Copilot AI review: 1. **`normalizeHttpPath` root-path edge case** — stripping trailing slashes on the input `/` produced an empty string, yielding malformed contract ids like `http::GET::`. Now preserves `/` for the root handler/fetch case. 2. **Dedupe `scanFiles` call** — `extract()` was globbing the source-scan file list twice (once for the provider fallback, once for the consumer fallback). Moved to a single lazy call that memoizes the result for the rest of the method. 3. **HTTP `scanFiles` now ignores `**/vendor/**`** — every other extractor's glob already ignored vendored sources; the HTTP one didn't. Fixed for consistency. 4. **`loadPackageDefinition` check is now structural** — was calling `tree.rootNode.text.includes('loadPackageDefinition')` which forces materialization of the entire file text from the parse tree (expensive on large files). Replaced with a dedicated compiled query on `(call_expression function: [(identifier) | (member_expression)])` so the check stays in the AST domain. 5. **`grpc-extractor.ts` header docstring updated** — still claimed ".proto parsing is not tree-sitter-based because no grammar is installed". Now describes the actual behaviour: tree-sitter when `tree-sitter-proto` is available (optionalDependency), manual fallback otherwise. 6. **Eliminated the double proto file parse on the fallback path** — `buildProtoContext` already globs + parses every `.proto` file to build `servicesByName`. On the `!hasProtoPlugin` branch the extractor was globbing + parsing again via the now-removed `parseProtoFile` helper. The fallback branch now iterates the map that `buildProtoContext` already produced to emit provider contracts directly — single pass per proto file. ## Tests - `topic-extractor.test.ts` — 30/30 pass - `http-route-extractor.test.ts` — 18/18 pass - `grpc-extractor.test.ts` — 43/43 pass - `manifest-extractor.test.ts` — 8/8 pass - `npx tsc -p tsconfig.json --noEmit` clean Co-authored-by: Claude --- .../core/group/extractors/grpc-extractor.ts | 78 ++++++++----------- .../group/extractors/grpc-patterns/node.ts | 23 +++++- .../group/extractors/http-route-extractor.ts | 20 ++++- 3 files changed, 68 insertions(+), 53 deletions(-) diff --git a/gitnexus/src/core/group/extractors/grpc-extractor.ts b/gitnexus/src/core/group/extractors/grpc-extractor.ts index 584b1e49e..0c914b782 100644 --- a/gitnexus/src/core/group/extractors/grpc-extractor.ts +++ b/gitnexus/src/core/group/extractors/grpc-extractor.ts @@ -17,21 +17,22 @@ import { * * Two parts: * - * 1. **`.proto` parsing** — done in-process by a small string-sanitizing - * parser (see `stripProtoCommentsAndStrings` + `extractServiceBlocks` - * below). NOT tree-sitter-based because no `tree-sitter-proto` - * grammar is installed in the repo. The parser preserves offsets so + * 1. **`.proto` parsing** — tree-sitter when `tree-sitter-proto` is + * installed (optionalDependency vendored in `vendor/tree-sitter-proto/`), + * via the `.proto` entry in `grpc-patterns/` and `hasProtoPlugin`. + * When the grammar isn't available (platform incompatibility, native + * build failure) the orchestrator falls back to the in-process + * string-sanitizing parser defined below (`stripProtoCommentsAndStrings` + * + `extractServiceBlocks`). The fallback preserves offsets so any * downstream regex scans run against a sanitized copy without - * affecting the line numbers of the original. Kept as a pragmatic - * exception to the "no regex in extractors" rule until / unless - * maintainers want to add a proto grammar. + * affecting line numbers of the original. * * 2. **Source-scan providers / consumers** — delegated to per-language * plugins in `./grpc-patterns/`. The orchestrator imports NO * tree-sitter grammars or query strings — each plugin owns its own. */ -// ─── .proto parsing (not tree-sitter) ──────────────────────────────── +// ─── .proto fallback parser (used only when tree-sitter-proto is absent) ─── function readSafe(repoPath: string, rel: string): string | null { const abs = path.resolve(repoPath, rel); @@ -372,23 +373,29 @@ export class GrpcExtractor implements ContractExtractor { // ─── Proto files — definitive provider source ───────────────── // When tree-sitter-proto is available, .proto files are handled by // the plugin loop below (they're in GRPC_SCAN_GLOB). Otherwise - // fall back to the manual string-sanitizing parser. + // emit provider contracts directly from the proto map that + // `buildProtoContext` already built — no second glob / parse pass. if (!hasProtoPlugin) { - const protoFiles = await glob('**/*.proto', { - cwd: repoPath, - ignore: ['**/node_modules/**', '**/.git/**', '**/vendor/**'], - nodir: true, - }); - for (const rel of protoFiles) { - const content = readSafe(repoPath, rel); - if (content) { - out.push( - ...this.parseProtoFile( - content, - rel, - protoContext.packagesByProto.get(normalizeProtoPath(rel)) ?? '', - ), - ); + for (const infos of protoMap.values()) { + for (const info of infos) { + for (const methodName of info.methods) { + const cid = contractId(info.package, info.serviceName, methodName); + out.push( + makeContract( + cid, + 'provider', + info.protoPath, + `${info.serviceName}.${methodName}`, + 0.85, + { + package: info.package, + service: info.serviceName, + method: methodName, + source: 'proto', + }, + ), + ); + } } } } @@ -422,29 +429,6 @@ export class GrpcExtractor implements ContractExtractor { return this.dedupe(out); } - private parseProtoFile(content: string, filePath: string, pkg: string): ExtractedContract[] { - const out: ExtractedContract[] = []; - - for (const { name: serviceName, body } of extractServiceBlocks(content)) { - const rpcRe = /rpc\s+(\w+)\s*\(/g; - let rpcMatch: RegExpExecArray | null; - while ((rpcMatch = rpcRe.exec(body)) !== null) { - const methodName = rpcMatch[1]; - const cid = contractId(pkg, serviceName, methodName); - out.push( - makeContract(cid, 'provider', filePath, `${serviceName}.${methodName}`, 0.85, { - package: pkg, - service: serviceName, - method: methodName, - source: 'proto', - }), - ); - } - } - - return out; - } - /** * Convert a plugin `GrpcDetection` into a concrete `ExtractedContract` * by resolving the short service name against the proto map, building diff --git a/gitnexus/src/core/group/extractors/grpc-patterns/node.ts b/gitnexus/src/core/group/extractors/grpc-patterns/node.ts index 2abe33ea3..033962206 100644 --- a/gitnexus/src/core/group/extractors/grpc-patterns/node.ts +++ b/gitnexus/src/core/group/extractors/grpc-patterns/node.ts @@ -82,12 +82,28 @@ const NEW_QUALIFIED_CTOR_SPEC: PatternSpec> = { `, }; +// Detect whether the file uses `loadPackageDefinition` (gRPC dynamic +// proto loader). Matches either a bare call or an `obj.loadPackageDefinition(...)` +// call. Plugin gates the qualified-constructor consumer on this — +// structural check avoids materializing `tree.rootNode.text` for every file. +const LOAD_PACKAGE_DEFINITION_SPEC: PatternSpec> = { + meta: {}, + query: ` + (call_expression + function: [ + (identifier) @fn (#eq? @fn "loadPackageDefinition") + (member_expression property: (property_identifier) @fn (#eq? @fn "loadPackageDefinition")) + ]) + `, +}; + interface NodeGrpcPatternBundle { grpcMethod: CompiledPatterns>; grpcClient: CompiledPatterns>; getService: CompiledPatterns>; newSimpleCtor: CompiledPatterns>; newQualifiedCtor: CompiledPatterns>; + loadPackageDefinition: CompiledPatterns>; } function compileBundle(language: unknown, name: string): NodeGrpcPatternBundle { @@ -103,6 +119,7 @@ function compileBundle(language: unknown, name: string): NodeGrpcPatternBundle { getService: mk(GET_SERVICE_SPEC, 'get-service'), newSimpleCtor: mk(NEW_SIMPLE_CTOR_SPEC, 'new-simple-ctor'), newQualifiedCtor: mk(NEW_QUALIFIED_CTOR_SPEC, 'new-qualified-ctor'), + loadPackageDefinition: mk(LOAD_PACKAGE_DEFINITION_SPEC, 'load-package-definition'), }; } @@ -255,8 +272,10 @@ function scanBundle(bundle: NodeGrpcPatternBundle, tree: Parser.Tree): GrpcDetec // ─── Consumer: loadPackageDefinition dynamic proto loader ──────── // Only emit when the file uses loadPackageDefinition, otherwise a // generic `new foo.bar.Something()` in unrelated code would falsely - // register as a gRPC consumer. - const usesLoadPackage = tree.rootNode.text.includes('loadPackageDefinition'); + // register as a gRPC consumer. Check structurally via a dedicated + // query — avoids materializing `tree.rootNode.text` for the whole + // file (expensive on large files). + const usesLoadPackage = runCompiledPatterns(bundle.loadPackageDefinition, tree).length > 0; if (usesLoadPackage) { for (const match of runCompiledPatterns(bundle.newQualifiedCtor, tree)) { const ctorNode = match.captures.ctor; diff --git a/gitnexus/src/core/group/extractors/http-route-extractor.ts b/gitnexus/src/core/group/extractors/http-route-extractor.ts index 96a7ffe20..52ad5e390 100644 --- a/gitnexus/src/core/group/extractors/http-route-extractor.ts +++ b/gitnexus/src/core/group/extractors/http-route-extractor.ts @@ -63,7 +63,10 @@ export function normalizeHttpPath(p: string): string { s = s.replace(/:\w+/g, '{param}'); s = s.replace(/\{[^}]+\}/g, '{param}'); s = s.replace(/\[[^\]]+\]/g, '{param}'); - return s; + // Preserve root: after stripping trailing slashes, the root "/" + // collapses to "" which would produce malformed contract ids like + // `http::GET::`. Restore a single slash for the root case. + return s === '' ? '/' : s; } /** @@ -192,19 +195,28 @@ export class HttpRouteExtractor implements ContractExtractor { } }; + // Glob the source-scan file list at most once per extract() — + // both provider and consumer fallback paths share the same list. + let scannedFiles: string[] | null = null; + const getScannedFiles = async (): Promise => { + if (scannedFiles) return scannedFiles; + scannedFiles = await this.scanFiles(repoPath); + return scannedFiles; + }; + const graphProviders = dbExecutor != null ? await this.extractProvidersGraph(dbExecutor, getDetections) : []; const providers = graphProviders.length > 0 ? graphProviders - : this.extractProvidersSourceScan(await this.scanFiles(repoPath), getDetections); + : this.extractProvidersSourceScan(await getScannedFiles(), getDetections); const graphConsumers = dbExecutor != null ? await this.extractConsumersGraph(dbExecutor, getDetections) : []; const consumers = graphConsumers.length > 0 ? graphConsumers - : this.extractConsumersSourceScan(await this.scanFiles(repoPath), getDetections); + : this.extractConsumersSourceScan(await getScannedFiles(), getDetections); return [...providers, ...consumers]; } @@ -212,7 +224,7 @@ export class HttpRouteExtractor implements ContractExtractor { private async scanFiles(repoPath: string): Promise { return glob(HTTP_SCAN_GLOB, { cwd: repoPath, - ignore: ['**/node_modules/**', '**/.git/**', '**/dist/**', '**/build/**'], + ignore: ['**/node_modules/**', '**/.git/**', '**/dist/**', '**/build/**', '**/vendor/**'], nodir: true, }); }