refactor(group): address PR #984 review feedback

- Extract `repoInSubgroup` to `group-path-utils.ts` so `cross-impact.ts`
  and `service.ts` share a single implementation; the file-prefix
  helpers now also accept Windows-style backslashes by normalizing to
  POSIX before comparison (NB#1, NB#5).
- `groupStatus` no longer redundantly `await import()`s `node:fs/promises`
  and `node:path` — the static imports at the top of `service.ts` are
  reused, removing the local-shadow risk (NB#2).
- `groupContext` and `groupQuery` now fan their per-repo work out via
  `Promise.all` instead of a sequential `for...of` await loop. Errors
  remain isolated per repo and the result order matches the filtered
  member list (NB#3).
- `runGroupImpact` now derives a single shared `deadline` from the
  caller-supplied `timeoutMs` before Phase 1, and Phase 2's fan-out
  reuses it instead of starting a fresh budget — total wall-clock can
  no longer exceed `timeoutMs` (NB#4).
- New unit suite `test/unit/group/group-path-utils.test.ts` covers the
  shared helpers, including Windows-path normalization.

Made-with: Cursor

gitnexus/src/core/group/cross-impact.ts

@@ -16,7 +16,11 @@ import type {
 } from './types.js';
 import type { GroupRepoHandle, GroupToolPort } from './service.js';
 import { loadGroupConfig } from './config-parser.js';
-import { fileMatchesServicePrefix, normalizeServicePrefix } from './group-path-utils.js';
+import {
+  fileMatchesServicePrefix,
+  normalizeServicePrefix,
+  repoInSubgroup,
+} from './group-path-utils.js';
 import { getGroupDir } from './storage.js';
 import { closeBridgeDb, openBridgeDbReadOnly, queryBridge, readBridgeMeta } from './bridge-db.js';
 import { BRIDGE_SCHEMA_VERSION } from './bridge-schema.js';
@@ -70,12 +74,6 @@ export interface RunGroupImpactDeps {
   gitnexusDir: string;
 }
 
-function repoInSubgroup(repoPath: string, subgroup?: string): boolean {
-  if (!subgroup?.trim()) return true;
-  const s = subgroup.replace(/\/+$/, '');
-  return repoPath === s || repoPath.startsWith(`${s}/`);
-}
-
 function parseDirection(raw: unknown): 'upstream' | 'downstream' | null {
   if (raw === 'upstream' || raw === 'downstream') return raw;
   return null;
@@ -346,6 +344,11 @@ export async function runGroupImpact(
     minConfidence,
   };
 
+  // Single shared deadline for Phase 1 (local walk) + Phase 2 (bridge fan-out).
+  // Phase 1 still gets the full budget; Phase 2 only uses whatever wall-clock
+  // time is left, so total work cannot exceed `timeoutMs`.
+  const deadline = Date.now() + Math.max(0, timeoutMs);
+
   const { value: local, timedOut: localTimedOut } = await safeLocalImpact(
     deps.port,
     resolved,
@@ -465,7 +468,6 @@ export async function runGroupImpact(
     }
     neighbors.sort((a, b) => b.confidence - a.confidence);
 
-    const deadline = Date.now() + Math.max(0, timeoutMs);
     const seen = new Set<string>();
 
     for (const n of neighbors) {

gitnexus/src/core/group/group-path-utils.ts

@@ -1,11 +1,19 @@
 /**
- * Shared service-path normalization for group tools (`service` monorepo filter).
- * Segments are compared case-sensitively (typical POSIX-style repo paths).
+ * Shared service-path normalization for group tools (`service` monorepo filter)
+ * and subgroup membership checks.
+ *
+ * Inputs may originate from tree-sitter, the OS file API, or user-supplied
+ * MCP arguments, so both `\` and `/` separators are accepted. Internally we
+ * normalize to POSIX-style `/` for case-sensitive segment comparisons.
  */
 
+function toPosix(p: string): string {
+  return p.replace(/\\/g, '/');
+}
+
 export function normalizeServicePrefix(service: unknown): string | undefined {
   if (service === undefined || service === null) return undefined;
-  const s = String(service).trim().replace(/\/+$/, '');
+  const s = toPosix(String(service)).trim().replace(/\/+$/, '');
   return s.length > 0 ? s : undefined;
 }
 
@@ -15,5 +23,20 @@ export function fileMatchesServicePrefix(
 ): boolean {
   if (!prefix) return true;
   if (!filePath) return false;
-  return filePath === prefix || filePath.startsWith(`${prefix}/`);
+  const normalized = toPosix(filePath);
+  return normalized === prefix || normalized.startsWith(`${prefix}/`);
+}
+
+/**
+ * True if `repoPath` is at or beneath `subgroup` (member-path prefix in
+ * `group.yaml`). Empty / missing `subgroup` matches every repo.
+ *
+ * @param exact When set, requires an exact equality match (no descendant repos).
+ */
+export function repoInSubgroup(repoPath: string, subgroup?: string, exact?: boolean): boolean {
+  if (!subgroup?.trim()) return true;
+  const s = toPosix(subgroup).replace(/\/+$/, '');
+  const r = toPosix(repoPath);
+  if (exact) return r === s;
+  return r === s || r.startsWith(`${s}/`);
 }

gitnexus/src/core/group/service.ts

@@ -7,7 +7,11 @@ import fsp from 'node:fs/promises';
 import path from 'node:path';
 import { checkStaleness } from '../git-staleness.js';
 import { loadGroupConfig } from './config-parser.js';
-import { fileMatchesServicePrefix, normalizeServicePrefix } from './group-path-utils.js';
+import {
+  fileMatchesServicePrefix,
+  normalizeServicePrefix,
+  repoInSubgroup,
+} from './group-path-utils.js';
 import { getDefaultGitnexusDir, getGroupDir, listGroups, readContractRegistry } from './storage.js';
 import { syncGroup } from './sync.js';
 import type {
@@ -73,13 +77,6 @@ export interface GroupToolPort {
   ): Promise<unknown>;
 }
 
-function repoInSubgroup(repoPath: string, subgroup?: string, exact?: boolean): boolean {
-  if (!subgroup?.trim()) return true;
-  const s = subgroup.replace(/\/+$/, '');
-  if (exact) return repoPath === s;
-  return repoPath === s || repoPath.startsWith(`${s}/`);
-}
-
 function isStoredContract(raw: unknown): raw is StoredContract {
   if (!raw || typeof raw !== 'object') return false;
   const o = raw as Record<string, unknown>;
@@ -325,37 +322,42 @@ export class GroupService {
       };
     }
 
-    const results: GroupContextResult['results'] = [];
+    const memberEntries = Object.entries(config.repos).filter(([repoPath]) =>
+      repoInSubgroup(repoPath, subgroup, subgroupExact),
+    );
 
-    for (const [repoPath, registryName] of Object.entries(config.repos)) {
-      if (!repoInSubgroup(repoPath, subgroup, subgroupExact)) continue;
-      try {
-        const repoObj = await this.port.resolveRepo(registryName);
-        const payload = await this.port.context(repoObj, {
-          name: target || undefined,
-          uid,
-          file_path,
-          include_content,
-        });
+    // Per-repo work is independent (each repo opens its own DB handle and the
+    // group-level result preserves repo iteration order via the indexed map).
+    // Errors are caught per repo so one slow/failed member does not block the rest.
+    const results: GroupContextResult['results'] = await Promise.all(
+      memberEntries.map(async ([repoPath, registryName]) => {
+        try {
+          const repoObj = await this.port.resolveRepo(registryName);
+          const payload = await this.port.context(repoObj, {
+            name: target || undefined,
+            uid,
+            file_path,
+            include_content,
+          });
 
-        if (servicePrefix) {
-          const st = (payload as { status?: string })?.status;
-          const sym = (payload as { symbol?: { filePath?: string } })?.symbol;
-          if (st === 'found' && !fileMatchesServicePrefix(sym?.filePath, servicePrefix)) {
-            results.push({ repoPath, registryName, payload: {} });
-            continue;
+          if (servicePrefix) {
+            const st = (payload as { status?: string })?.status;
+            const sym = (payload as { symbol?: { filePath?: string } })?.symbol;
+            if (st === 'found' && !fileMatchesServicePrefix(sym?.filePath, servicePrefix)) {
+              return { repoPath, registryName, payload: {} };
+            }
           }
-        }
 
-        results.push({ repoPath, registryName, payload });
-      } catch (e) {
-        results.push({
-          repoPath,
-          registryName,
-          payload: { error: e instanceof Error ? e.message : String(e) },
-        });
-      }
-    }
+          return { repoPath, registryName, payload };
+        } catch (e) {
+          return {
+            repoPath,
+            registryName,
+            payload: { error: e instanceof Error ? e.message : String(e) },
+          };
+        }
+      }),
+    );
 
     return {
       group: name,
@@ -384,33 +386,39 @@ export class GroupService {
     const groupDir = getGroupDir(getDefaultGitnexusDir(), name);
     const config = await loadGroupConfig(groupDir);
 
-    const perRepo: Array<{ repo: string; score: number; processes: unknown[] }> = [];
-    for (const [repoPath, registryName] of Object.entries(config.repos)) {
-      if (!repoInSubgroup(repoPath, subgroup, subgroupExact)) continue;
-      try {
-        const repoObj = await this.port.resolveRepo(registryName);
-        const queryResult = (await this.port.query(repoObj, {
-          query: queryText,
-          limit,
-          max_symbols: 10,
-          include_content: false,
-        })) as {
-          processes?: Array<Record<string, unknown>>;
-          process_symbols?: Array<Record<string, unknown>>;
-        };
-        const processes = servicePrefix
-          ? filterQueryByServicePrefix(queryResult, servicePrefix).processes
-          : queryResult.processes || [];
-        const scored = processes.map((p, idx) => ({
-          ...p,
-          _rrf_score: 1 / (idx + 1 + 60),
-          _repo: repoPath,
-        }));
-        perRepo.push({ repo: repoPath, score: 0, processes: scored });
-      } catch {
-        perRepo.push({ repo: repoPath, score: 0, processes: [] });
-      }
-    }
+    const memberEntries = Object.entries(config.repos).filter(([repoPath]) =>
+      repoInSubgroup(repoPath, subgroup, subgroupExact),
+    );
+
+    // Per-repo query is independent; run them concurrently and isolate
+    // failures so one slow/failed member does not block the rest.
+    const perRepo = await Promise.all(
+      memberEntries.map(async ([repoPath, registryName]) => {
+        try {
+          const repoObj = await this.port.resolveRepo(registryName);
+          const queryResult = (await this.port.query(repoObj, {
+            query: queryText,
+            limit,
+            max_symbols: 10,
+            include_content: false,
+          })) as {
+            processes?: Array<Record<string, unknown>>;
+            process_symbols?: Array<Record<string, unknown>>;
+          };
+          const processes = servicePrefix
+            ? filterQueryByServicePrefix(queryResult, servicePrefix).processes
+            : queryResult.processes || [];
+          const scored = processes.map((p, idx) => ({
+            ...p,
+            _rrf_score: 1 / (idx + 1 + 60),
+            _repo: repoPath,
+          }));
+          return { repo: repoPath, score: 0, processes: scored as unknown[] };
+        } catch {
+          return { repo: repoPath, score: 0, processes: [] as unknown[] };
+        }
+      }),
+    );
 
     const allProcesses = perRepo.flatMap((r) => r.processes as Array<Record<string, unknown>>);
     allProcesses.sort((a, b) => (b._rrf_score as number) - (a._rrf_score as number));
@@ -441,13 +449,10 @@ export class GroupService {
       }
     > = {};
 
-    const fsp = await import('node:fs/promises');
-    const pathMod = await import('node:path');
-
     for (const [repoPath, registryName] of Object.entries(config.repos)) {
       try {
         const repoObj = await this.port.resolveRepo(registryName);
-        const metaPath = pathMod.join(repoObj.storagePath, 'meta.json');
+        const metaPath = path.join(repoObj.storagePath, 'meta.json');
         const metaRaw = await fsp.readFile(metaPath, 'utf-8').catch(() => '{}');
         const meta = JSON.parse(metaRaw) as { lastCommit?: string; indexedAt?: string };
 

gitnexus/test/unit/group/group-path-utils.test.ts

@@ -0,0 +1,87 @@
+import { describe, it, expect } from 'vitest';
+import {
+  fileMatchesServicePrefix,
+  normalizeServicePrefix,
+  repoInSubgroup,
+} from '../../../src/core/group/group-path-utils.js';
+
+describe('group-path-utils', () => {
+  describe('normalizeServicePrefix', () => {
+    it('returns undefined for null/undefined/empty', () => {
+      expect(normalizeServicePrefix(undefined)).toBeUndefined();
+      expect(normalizeServicePrefix(null)).toBeUndefined();
+      expect(normalizeServicePrefix('')).toBeUndefined();
+      expect(normalizeServicePrefix('   ')).toBeUndefined();
+    });
+
+    it('strips trailing slashes', () => {
+      expect(normalizeServicePrefix('services/auth/')).toBe('services/auth');
+      expect(normalizeServicePrefix('services/auth///')).toBe('services/auth');
+    });
+
+    it('normalizes Windows-style backslashes to POSIX', () => {
+      expect(normalizeServicePrefix('services\\auth')).toBe('services/auth');
+      expect(normalizeServicePrefix('app\\backend\\')).toBe('app/backend');
+    });
+  });
+
+  describe('fileMatchesServicePrefix', () => {
+    it('returns true when prefix is empty/undefined', () => {
+      expect(fileMatchesServicePrefix('any/file.ts', undefined)).toBe(true);
+      expect(fileMatchesServicePrefix('any/file.ts', '')).toBe(true);
+    });
+
+    it('returns false when filePath is missing but prefix is set', () => {
+      expect(fileMatchesServicePrefix(undefined, 'services/auth')).toBe(false);
+    });
+
+    it('matches exact prefix and descendants', () => {
+      expect(fileMatchesServicePrefix('services/auth', 'services/auth')).toBe(true);
+      expect(fileMatchesServicePrefix('services/auth/a.ts', 'services/auth')).toBe(true);
+    });
+
+    it('rejects partial-segment matches', () => {
+      expect(fileMatchesServicePrefix('services/aut', 'services/auth')).toBe(false);
+      expect(fileMatchesServicePrefix('services/authz/a.ts', 'services/auth')).toBe(false);
+    });
+
+    it('matches Windows-style file paths against POSIX prefix', () => {
+      expect(fileMatchesServicePrefix('services\\auth\\a.ts', 'services/auth')).toBe(true);
+      expect(fileMatchesServicePrefix('services\\authz\\a.ts', 'services/auth')).toBe(false);
+    });
+  });
+
+  describe('repoInSubgroup', () => {
+    it('matches every repo when subgroup is empty/undefined', () => {
+      expect(repoInSubgroup('any/repo', undefined)).toBe(true);
+      expect(repoInSubgroup('any/repo', '')).toBe(true);
+      expect(repoInSubgroup('any/repo', '   ')).toBe(true);
+    });
+
+    it('matches exact path and descendants by default', () => {
+      expect(repoInSubgroup('app/backend', 'app/backend')).toBe(true);
+      expect(repoInSubgroup('app/backend/sub', 'app/backend')).toBe(true);
+      expect(repoInSubgroup('app/frontend', 'app/backend')).toBe(false);
+    });
+
+    it('strips trailing slashes from subgroup', () => {
+      expect(repoInSubgroup('app/backend', 'app/backend/')).toBe(true);
+      expect(repoInSubgroup('app/backend/x', 'app/backend///')).toBe(true);
+    });
+
+    it('with exact=true matches only the exact repo', () => {
+      expect(repoInSubgroup('app/backend', 'app/backend', true)).toBe(true);
+      expect(repoInSubgroup('app/backend/sub', 'app/backend', true)).toBe(false);
+    });
+
+    it('rejects partial-segment matches', () => {
+      expect(repoInSubgroup('app/backendz', 'app/backend')).toBe(false);
+    });
+
+    it('normalizes Windows-style paths on both sides', () => {
+      expect(repoInSubgroup('app\\backend', 'app/backend')).toBe(true);
+      expect(repoInSubgroup('app/backend/x', 'app\\backend')).toBe(true);
+      expect(repoInSubgroup('app\\backend\\sub', 'app\\backend', true)).toBe(false);
+    });
+  });
+});