From c829f2a7aa84196063a944acc5457bf5fb07865c Mon Sep 17 00:00:00 2001 From: ivkond Date: Sun, 19 Apr 2026 17:30:52 +0300 Subject: [PATCH] refactor(group): address PR #984 review feedback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - 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 | 18 ++- gitnexus/src/core/group/group-path-utils.ts | 31 +++- gitnexus/src/core/group/service.ts | 137 +++++++++--------- .../test/unit/group/group-path-utils.test.ts | 87 +++++++++++ 4 files changed, 195 insertions(+), 78 deletions(-) create mode 100644 gitnexus/test/unit/group/group-path-utils.test.ts diff --git a/gitnexus/src/core/group/cross-impact.ts b/gitnexus/src/core/group/cross-impact.ts index fe9db5cc8..8739584c7 100644 --- a/gitnexus/src/core/group/cross-impact.ts +++ b/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(); for (const n of neighbors) { diff --git a/gitnexus/src/core/group/group-path-utils.ts b/gitnexus/src/core/group/group-path-utils.ts index e41163d64..ab6a1effb 100644 --- a/gitnexus/src/core/group/group-path-utils.ts +++ b/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}/`); } diff --git a/gitnexus/src/core/group/service.ts b/gitnexus/src/core/group/service.ts index 3111849f3..afbb66e0e 100644 --- a/gitnexus/src/core/group/service.ts +++ b/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; } -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; @@ -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>; - process_symbols?: Array>; - }; - 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>; + process_symbols?: Array>; + }; + 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>); 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 }; diff --git a/gitnexus/test/unit/group/group-path-utils.test.ts b/gitnexus/test/unit/group/group-path-utils.test.ts new file mode 100644 index 000000000..0f57e0adc --- /dev/null +++ b/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); + }); + }); +});