From 7b22776560636bef9df007ad2b83cb146c7cac1d Mon Sep 17 00:00:00 2001 From: ivkond Date: Mon, 13 Apr 2026 07:24:03 +0000 Subject: [PATCH] refactor(group): address Claude review feedback (bugs + dedup + hygiene) on #796 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follows up `2f28bfc` with the remaining items from the Claude AI review: ## Bugs **Bug 2 — Label-unaware Cypher queries in `resolveSymbol`.** The manifest-extractor's lookup queries were `MATCH (n) WHERE n.name = $x` with no label filter, so a topic/service/package name could silently match any node type (File, Variable, Import, Folder, …). Added label filters: - `topic` → `(n:Function|Method|Class|Interface)` (topics are best-effort symbol-name matches against listener/publisher symbols) - `grpc` method → `(n:Function|Method)` - `grpc` service → `(n:Class|Interface)` - `lib` → `(n:Package|Module)` All 8 manifest-extractor tests still pass (mock executor is label-agnostic, but the production LadybugDB graph now gets correctly scoped queries). **Bug 8 — Tautological `!handlerName` condition.** `http-route-extractor.ts:extractProvidersGraph` had `let handlerName = null; if (!method || !handlerName) { ... }` — the `!handlerName` clause was always true since there was no intervening assignment. Simplified to always run the plugin-scan lookup (we need the handler name even when `methodFromRouteReason` already resolved the method). ## Clean code / dedup **Design 7 — `readSafe` was copy-pasted in all three orchestrators.** Extracted to `extractors/fs-utils.ts` as the single source of truth for the path-traversal guard. Dropped the three local copies and the now-unused `fs`/`path` imports from topic-extractor. **Style 10 — Language-specific `_test.go` skip in the topic orchestrator.** Was `if (rel.endsWith('_test.go')) continue;` inside the language- agnostic extraction loop. Pushed into the glob's ignore list (`'**/*_test.go'`) alongside the existing `node_modules`, `vendor`, `dist`, `build` entries, with a comment explaining that other languages' test file conventions either live in separate directories (Python `tests/`, Java `src/test/`) or are already covered by the existing ignores. ## Already addressed in `2f28bfc` (mentioned again in Claude review) - Bug 3: `normalizeHttpPath('/')` returns `''` — fixed - Bug 4: double glob + double parse of `.proto` — fixed - Bug 5: `scanFiles` called twice in HTTP — fixed - Bug 6: missing `**/vendor/**` in HTTP glob — fixed - Design 9 partially: `tree.rootNode.text.includes('loadPackageDefinition')` replaced with a dedicated structural query ## Deferred - Bug 1 (`http::*::path` vs `http::GET::path` matching) — out of scope; sync.ts matching logic lands in #793, manifest extractor already emits correct synthetic uids for unresolved HTTP contracts. - Design 9 full (change plugin `scan(tree)` → `scan(tree, source)`) — the only real use case (`loadPackageDefinition` gate) is already fixed via a structural query, so the interface change would be cosmetic churn without a concrete consumer. ## 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 --- .../src/core/group/extractors/fs-utils.ts | 23 +++++++++++ .../core/group/extractors/grpc-extractor.ts | 14 +------ .../group/extractors/http-route-extractor.ts | 38 ++++++------------- .../group/extractors/manifest-extractor.ts | 20 +++++++--- .../core/group/extractors/topic-extractor.ts | 32 ++++++++-------- 5 files changed, 66 insertions(+), 61 deletions(-) create mode 100644 gitnexus/src/core/group/extractors/fs-utils.ts diff --git a/gitnexus/src/core/group/extractors/fs-utils.ts b/gitnexus/src/core/group/extractors/fs-utils.ts new file mode 100644 index 000000000..384f63203 --- /dev/null +++ b/gitnexus/src/core/group/extractors/fs-utils.ts @@ -0,0 +1,23 @@ +import * as fs from 'node:fs'; +import * as path from 'node:path'; + +/** + * Safely read a file inside a repo, rejecting any path that escapes + * `repoPath` via `..` traversal or absolute segments. Returns `null` if + * the path is outside the repo or the file can't be read. + * + * Used by every source-scan extractor under this directory. Kept as a + * single shared implementation so the path-traversal guard (security- + * sensitive) lives in exactly one place. + */ +export function readSafe(repoPath: string, rel: string): string | null { + const abs = path.resolve(repoPath, rel); + const base = path.resolve(repoPath); + const relToBase = path.relative(base, abs); + if (relToBase.startsWith('..') || path.isAbsolute(relToBase)) return null; + try { + return fs.readFileSync(abs, 'utf-8'); + } catch { + return null; + } +} diff --git a/gitnexus/src/core/group/extractors/grpc-extractor.ts b/gitnexus/src/core/group/extractors/grpc-extractor.ts index 0c914b782..c6af9138a 100644 --- a/gitnexus/src/core/group/extractors/grpc-extractor.ts +++ b/gitnexus/src/core/group/extractors/grpc-extractor.ts @@ -1,9 +1,9 @@ -import * as fs from 'node:fs'; import * as path from 'node:path'; import { glob } from 'glob'; import Parser from 'tree-sitter'; import type { ContractExtractor, CypherExecutor } from '../contract-extractor.js'; import type { ExtractedContract, RepoHandle } from '../types.js'; +import { readSafe } from './fs-utils.js'; import { GRPC_SCAN_GLOB, getPluginForFile, @@ -34,18 +34,6 @@ import { // ─── .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); - const base = path.resolve(repoPath); - const relToBase = path.relative(base, abs); - if (relToBase.startsWith('..') || path.isAbsolute(relToBase)) return null; - try { - return fs.readFileSync(abs, 'utf-8'); - } catch { - return null; - } -} - function contractId(pkg: string, service: string, method: string): string { const prefix = pkg ? `${pkg}.${service}` : service; return `grpc::${prefix}/${method}`; diff --git a/gitnexus/src/core/group/extractors/http-route-extractor.ts b/gitnexus/src/core/group/extractors/http-route-extractor.ts index 52ad5e390..0b07090e1 100644 --- a/gitnexus/src/core/group/extractors/http-route-extractor.ts +++ b/gitnexus/src/core/group/extractors/http-route-extractor.ts @@ -1,9 +1,9 @@ -import * as fs from 'node:fs'; import * as path from 'node:path'; import { glob } from 'glob'; import Parser from 'tree-sitter'; import type { ContractExtractor, CypherExecutor } from '../contract-extractor.js'; import type { ExtractedContract, RepoHandle } from '../types.js'; +import { readSafe } from './fs-utils.js'; import { getPluginForFile, HTTP_SCAN_GLOB, type HttpDetection } from './http-patterns/index.js'; /** @@ -97,20 +97,6 @@ function contractIdFor(method: string, pathNorm: string): string { return `http::${method.toUpperCase()}::${pathNorm}`; } -// ─── File read helper (path-traversal safe) ────────────────────────── - -function readSafe(repoPath: string, rel: string): string | null { - const abs = path.resolve(repoPath, rel); - const base = path.resolve(repoPath); - const relToBase = path.relative(base, abs); - if (relToBase.startsWith('..') || path.isAbsolute(relToBase)) return null; - try { - return fs.readFileSync(abs, 'utf-8'); - } catch { - return null; - } -} - // ─── Graph row helpers ─────────────────────────────────────────────── function methodFromRouteReason(reason: string): string | null { @@ -249,20 +235,20 @@ export class HttpRouteExtractor implements ContractExtractor { const routeSource = String(row.routeSource ?? row.routeReason ?? ''); let method = methodFromRouteReason(routeSource); - // Fallback: look up method + handler name from the plugin's scan - // of the handler file. This replaces the old regex-based - // `inferMethodFromFileScan` and `pickJavaHandlerName` helpers — - // tree-sitter gives both pieces of information structurally. + // Look up handler name (and backfill method if missing) from the + // plugin's scan of the handler file. This replaces the old + // regex-based `inferMethodFromFileScan` and `pickJavaHandlerName` + // helpers — tree-sitter gives both pieces of information + // structurally. Always run the lookup: even when method is set by + // `methodFromRouteReason`, we still need the handler name. const detections = filePath ? getDetections(filePath) : []; const providerDetections = detections.filter((d) => d.role === 'provider'); let handlerName: string | null = null; - if (!method || !handlerName) { - const normalizedRoute = normalizeHttpPath(routePath); - const match = providerDetections.find((d) => normalizeHttpPath(d.path) === normalizedRoute); - if (match) { - if (!method) method = match.method; - handlerName = match.name; - } + const normalizedRoute = normalizeHttpPath(routePath); + const match = providerDetections.find((d) => normalizeHttpPath(d.path) === normalizedRoute); + if (match) { + if (!method) method = match.method; + handlerName = match.name; } if (!method) method = 'GET'; diff --git a/gitnexus/src/core/group/extractors/manifest-extractor.ts b/gitnexus/src/core/group/extractors/manifest-extractor.ts index 09817b356..4cfcd5478 100644 --- a/gitnexus/src/core/group/extractors/manifest-extractor.ts +++ b/gitnexus/src/core/group/extractors/manifest-extractor.ts @@ -141,8 +141,13 @@ export class ManifestExtractor { { normalized }, ); } else if (link.type === 'topic') { + // Topic names aren't a first-class NodeLabel in the graph — + // topics are referenced by function/method symbols (Kafka + // listeners, publishers). Restrict to symbol-like labels to + // avoid cross-matching Files/Variables/Imports that happen to + // share the topic name. rows = await executor( - `MATCH (n) WHERE n.name = $contract + `MATCH (n:Function|Method|Class|Interface) WHERE n.name = $contract RETURN n.id AS uid, n.name AS name, n.filePath AS filePath ORDER BY n.filePath ASC LIMIT 1`, @@ -153,12 +158,15 @@ export class ManifestExtractor { // variants). Prefer matching by method name when present, otherwise // by service name. NO .proto path fallback — that's guaranteed to // return a wrong symbol in any repo with more than one proto file. + // Label filters scope lookups: methods → Function|Method, services + // → Class|Interface (no label match = no silent wrong hits on + // File/Variable nodes that happen to share the name). const parts = link.contract.split('/'); const serviceName = parts[0]?.trim() ?? ''; const methodName = parts[1]?.trim() ?? ''; if (methodName) { rows = await executor( - `MATCH (n) WHERE n.name = $methodName + `MATCH (n:Function|Method) WHERE n.name = $methodName RETURN n.id AS uid, n.name AS name, n.filePath AS filePath ORDER BY n.filePath ASC LIMIT 1`, @@ -166,7 +174,7 @@ export class ManifestExtractor { ); } else if (serviceName) { rows = await executor( - `MATCH (n) WHERE n.name = $serviceName + `MATCH (n:Class|Interface) WHERE n.name = $serviceName RETURN n.id AS uid, n.name AS name, n.filePath AS filePath ORDER BY n.filePath ASC LIMIT 1`, @@ -178,9 +186,11 @@ export class ManifestExtractor { } else if (link.type === 'lib') { // Only exact match on the symbol's name. Previous fallback to // CONTAINS on n.filePath would promote "react" to "react-native" - // or "@types/react" — silent wrong attribution. + // or "@types/react" — silent wrong attribution. Restrict to + // package-level labels so we don't return arbitrary symbols + // named after a library. rows = await executor( - `MATCH (n) WHERE n.name = $contract + `MATCH (n:Package|Module) WHERE n.name = $contract RETURN n.id AS uid, n.name AS name, n.filePath AS filePath ORDER BY n.filePath ASC LIMIT 1`, diff --git a/gitnexus/src/core/group/extractors/topic-extractor.ts b/gitnexus/src/core/group/extractors/topic-extractor.ts index faab19cf3..1fbccac8a 100644 --- a/gitnexus/src/core/group/extractors/topic-extractor.ts +++ b/gitnexus/src/core/group/extractors/topic-extractor.ts @@ -1,9 +1,8 @@ -import * as fs from 'node:fs'; -import * as path from 'node:path'; import { glob } from 'glob'; import Parser from 'tree-sitter'; import type { ContractExtractor, CypherExecutor } from '../contract-extractor.js'; import type { ExtractedContract, RepoHandle } from '../types.js'; +import { readSafe } from './fs-utils.js'; import { scanFile, unquoteLiteral } from './tree-sitter-scanner.js'; import { TOPIC_SCAN_GLOB, @@ -28,18 +27,6 @@ import { * Adding a new language is a one-file edit in `topic-patterns/index.ts`. */ -function readSafe(repoPath: string, rel: string): string | null { - const abs = path.resolve(repoPath, rel); - const base = path.resolve(repoPath); - const relToBase = path.relative(base, abs); - if (relToBase.startsWith('..') || path.isAbsolute(relToBase)) return null; - try { - return fs.readFileSync(abs, 'utf-8'); - } catch { - return null; - } -} - function makeContract(topicName: string, meta: TopicMeta, filePath: string): ExtractedContract { return { contractId: `topic::${topicName}`, @@ -71,7 +58,20 @@ export class TopicExtractor implements ContractExtractor { ): Promise { const files = await glob(TOPIC_SCAN_GLOB, { cwd: repoPath, - ignore: ['**/node_modules/**', '**/.git/**', '**/vendor/**', '**/dist/**', '**/build/**'], + ignore: [ + '**/node_modules/**', + '**/.git/**', + '**/vendor/**', + '**/dist/**', + '**/build/**', + // Language-level test file conventions. Go test files + // `*_test.go` live next to source; other languages either use + // separate test directories (Python's `tests/`, Java's + // `src/test/`) or are already covered by the dist/build ignores. + // Pushed to the glob level so the orchestrator stays + // language-agnostic. + '**/*_test.go', + ], nodir: true, }); @@ -81,8 +81,6 @@ export class TopicExtractor implements ContractExtractor { const out: ExtractedContract[] = []; for (const rel of files) { - if (rel.endsWith('_test.go')) continue; - const provider = getProviderForFile(rel); if (!provider) continue;