refactor(group): address Claude review feedback (bugs + dedup + hygiene) on #796

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 <noreply@anthropic.com>

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;
+  }
+}

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}`;

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';
 

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`,

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<ExtractedContract[]> {
     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;