fix(extractors): resolve 3 silent contract mis-resolution bugs (#793)

gitnexus-web/src/hooks/useAutoScroll.ts

@@ -142,4 +142,4 @@ export function useAutoScroll<T>(
     isAtBottom,
     scrollToBottom,
   };
-}
+}

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(
-      <AutoScrollHarness messages={[]} isChatLoading={false} />,
-    );
+    const { rerender, unmount } = render(<AutoScrollHarness messages={[]} isChatLoading={false} />);
 
     expect(screen.queryByTestId('messages-container')).toBeNull();
     expect(resizeObserverInstances).toHaveLength(0);

gitnexus/src/core/group/extractors/grpc-extractor.ts

@@ -314,25 +314,37 @@ export async function buildProtoMap(repoPath: string): Promise<Map<string, Proto
 }
 
 export function resolveProtoConflict(
-  _serviceName: string,
+  serviceName: string,
   sourceFilePath: string,
   candidates: ProtoServiceInfo[],
 ): ProtoServiceInfo | null {
   if (candidates.length === 0) return null;
   if (candidates.length === 1) return candidates[0];
 
   const sourceDir = normalizeProtoPath(path.dirname(sourceFilePath));
-  let best = candidates[0];
-  let bestScore = -1;
-  for (const c of candidates) {
+  const scored = candidates.map((c) => {
     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<string, ProtoServiceInfo[]>,
-  ): 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)

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

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

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', () => {