src/core/mcpClient.ts (1)

33-91: Consider extracting regex patterns as constants for maintainability.

The regex patterns are complex and repeated across multiple files. Consider extracting them as named constants to improve readability and maintainability.

+const PACKAGE_PATTERNS = {
+  NPX_VERSION: /npx\s+(?:-[ygc]\s+)?(?:--\S+\s+)*([^@\s]+)@([^\s]+)/,
+  NPX_SCOPED: /npx\s+(?:-[ygc]\s+)?(?:--\S+\s+)*(@[^/]+\/[^@\s]+)(?:@([^\s]+))?/,
+  NPX_BASIC: /npx\s+(?:-[ygc]\s+)?(?:--\S+\s+)*([^@\s]+)/,
+  BUNX_VERSION: /bunx\s+(?:-[ygc]\s+)?(?:--\S+\s+)*([^@\s]+)@([^\s]+)/,
+  BUNX_SCOPED: /bunx\s+(?:-[ygc]\s+)?(?:--\S+\s+)*(@[^/]+\/[^@\s]+)(?:@([^\s]+))?/,
+  BUNX_BASIC: /bunx\s+(?:-[ygc]\s+)?(?:--\S+\s+)*([^@\s]+)/,
+  PIPX_SPEC: /pipx\s+run\s+--spec\s+([^=\s]+)==([^\s]+)/,
+  PIPX_RUN: /pipx\s+run\s+(?:--python\s+[^\s]+\s+)?([^@\s]+)/,
+  UVX_BASIC: /uvx\s+(?:--from\s+[^\s]+\s+)?([^@\s]+)/
+};

 function parsePackageFromCommand(command: string, args: string[] = []): PackageInfo | null {
   const fullCommand = [command, ...args].join(' ');

   // Pattern 1: npx package-name@version
-  const npxVersionMatch = fullCommand.match(/npx\s+(?:-[ygc]\s+)?(?:--\S+\s+)*([^@\s]+)@([^\s]+)/);
+  const npxVersionMatch = fullCommand.match(PACKAGE_PATTERNS.NPX_VERSION);
   // ... apply similar changes to other patterns

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Learnt from: CR
PR: agentinit/agentinit#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-24T11:44:03.031Z
Learning: Include integration tests for critical paths

Learnt from: CR
PR: agentinit/agentinit#0
File: .cursorrules:0-0
Timestamp: 2025-09-19T20:17:12.331Z
Learning: Write both unit tests for individual components and integration tests

Learnt from: CR
PR: agentinit/agentinit#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-24T11:44:03.031Z
Learning: Applies to **/*.{test,spec}.ts : Update tests when modifying functionality

Learnt from: CR
PR: agentinit/agentinit#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-24T11:44:03.031Z
Learning: Applies to **/*.{test,spec}.ts : Test edge cases and error scenarios

src/core/mcpClient.ts (1)

247-253: Avoid blocking server verification on registry lookups; add abort-aware, short-circuit fallback.

Fetching the package version here can add up to 5s latency and ignores the provided abortSignal. Prefer racing it with a short timeout and the abortSignal so verification isn't delayed by registry slowness.

Apply this diff to make version resolution non-blocking and abort-aware:

-      // Get server info and fetch package version
-      const packageVersion = await getPackageVersion(server);
+      // Get server info and fetch package version with a short cap and abort awareness
+      const packageVersion = await Promise.race<string>([
+        getPackageVersion(server),
+        new Promise<string>(resolve => setTimeout(() => resolve('unknown'), 1500)),
+        new Promise<string>(resolve => {
+          if (abortSignal?.aborted) return resolve('unknown');
+          abortSignal?.addEventListener('abort', () => resolve('unknown'), { once: true });
+        }),
+      ]);

If you want fuller cancellation, consider extending getPackageVersion to accept an AbortSignal and propagate it downstream. As per coding guidelines.

tests/core/packageVersion.test.ts (1)

1-92: Solid unit coverage; add UVX flag variants.

Great breadth across managers and scoped packages. Consider adding a UVX case with extra flags to guard future regex tweaks.

Apply this diff to extend coverage:

   it('should parse uvx with --from flag', () => {
     const result = parsePackageFromCommand('uvx', ['--from', 'git+https://github.com/user/repo.git', 'package-name']);
     expect(result).toEqual({ name: 'package-name' });
   });
+
+  it('should parse uvx with additional flags', () => {
+    const result = parsePackageFromCommand('uvx', ['-q', '--isolated', 'black']);
+    expect(result).toEqual({ name: 'black' });
+  });

Based on learnings

tests/core/packageVersionIntegration.test.ts (2)

1-8: Prefer stubbing globals via Vitest helpers.

Direct assignment works but is brittle across environments. Use vi.stubGlobal/vi.unstubAllGlobals for safer setup/teardown.

Example:

const mockFetch = vi.fn();

beforeAll(() => {
  vi.stubGlobal('fetch', mockFetch);
});

afterAll(() => {
  vi.unstubAllGlobals();
});

Based on learnings

---

18-88: Nice npm-path coverage; add ‘latest’ deref case.

You test direct versions and error paths. Add a case where the command specifies @latest to assert the registry is consulted.

Proposed test:

it('should dereference @latest via npm registry', async () => {
  mockFetch.mockResolvedValueOnce({ ok: true, json: async () => ({ 'dist-tags': { latest: '9.9.9' } }) });
  const server = { name: 'x', type: MCPServerType.STDIO, command: 'npx', args: ['pkg@latest'] };
  const version = await getPackageVersion(server);
  expect(version).toBe('9.9.9');
});

src/core/packageVersionUtils.ts (4)

129-132: Stricter manager detection to avoid false positives.

includes('uvx') could mis-route if 'uvx' appears in args. Check the first token instead.

Apply this diff:

-export function isPythonPackageManager(command: string, args: string[] = []): boolean {
-  const fullCommand = [command, ...args].join(' ');
-  return fullCommand.includes('pipx') || fullCommand.includes('uvx');
-}
+export function isPythonPackageManager(command: string, _args: string[] = []): boolean {
+  const firstToken = String(command || '').trim().split(/\s+/)[0];
+  return firstToken === 'pipx' || firstToken === 'uvx';
+}

---

63-67: UVX regex doesn’t handle additional flags.

Support generic long/short flags (with or without values) before the package name.

Apply this diff:

-  const uvxMatch = fullCommand.match(/uvx\s+(?:--from\s+[^\s]+\s+)?([^@\s]+)/);
+  const uvxMatch = fullCommand.match(/uvx\s+(?:--\S+(?:\s+[^\s]+)?\s+|-{1}\S+\s+)*([^@\s]+)/);

This skips sequences like: uvx -q --isolated --from https://... package-name

---

12-70: Reduce regex duplication for npx/bunx patterns (optional).

npx and bunx branches are identical. Consider a small helper to apply one set of patterns for a given manager to ease maintenance.

Sketch:

function matchJsManager(full: string, manager: 'npx'|'bunx'): PackageInfo | null { /* shared regex with variable */ }

---

158-169: Tag semantics: consider dereferencing non-semver tags (e.g., next, beta).

Current logic returns non-'latest' tags verbatim. Optionally, detect non-semver tags and resolve via registry for a concrete version.

Example:

const isSemver = /^\d+\.\d+\.\d+(-[\w.-]+)?$/.test(packageInfo.version);
if (packageInfo.version && isSemver) return packageInfo.version;
// else fall through to registry lookup

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Learnt from: CR
PR: agentinit/agentinit#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-24T11:44:03.031Z
Learning: Applies to **/*.{test,spec}.ts : Update tests when modifying functionality

Learnt from: CR
PR: agentinit/agentinit#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-24T11:44:03.046Z
Learning: Applies to **/*.{test,spec}.ts : Test edge cases and error scenarios

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

tests/core/packageVersionIntegration.test.ts (2)

1-1: Stabilize fetch mocking: type-safe assignment and restore original fetch

Avoid leaking the mocked fetch across files and fix TS typing by assigning/restoring in hooks.

Apply this diff:

-import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest';
+import { describe, it, expect, vi, beforeEach, afterEach, beforeAll, afterAll } from 'vitest';
@@
-// Mock fetch globally
-const mockFetch = vi.fn();
-global.fetch = mockFetch;
+// Mock fetch globally
+const mockFetch = vi.fn() as unknown as typeof fetch;
+let originalFetch: typeof global.fetch;
+
+beforeAll(() => {
+  originalFetch = global.fetch;
+});
@@
 afterEach(() => {
   vi.restoreAllMocks();
 });
+
+afterAll(() => {
+  // Restore original fetch to avoid leaking mocks to other test files
+  global.fetch = originalFetch;
+});

And ensure we set the mock per test run (so we don’t rely on file-load timing):

   beforeEach(() => {
-    mockFetch.mockClear();
+    mockFetch.mockClear();
+    global.fetch = mockFetch;
   });

As per coding guidelines

Also applies to: 5-8, 14-16

---

261-273: Add a scoped package test for npx -p to verify encoding

We currently don’t cover scoped packages for the npm registry path. Add a case like @scope/cli to ensure encodeURIComponent handling is correct.

Example test to add after this block:

it('should handle scoped packages with npx -p without API calls', async () => {
  const server = {
    name: 'test-server',
    type: MCPServerType.STDIO,
    command: 'npx',
    args: ['-p', '@scope/[email protected]', 'cli', '--help']
  };

  const version = await getPackageVersion(server);
  expect(mockFetch).not.toHaveBeenCalled();
  expect(version).toBe('2.1.0');
});

src/core/packageVersionUtils.ts (4)

90-115: Broaden timeout type for Node/browser parity

Use ReturnType instead of NodeJS.Timeout to keep types portable across environments.

Apply this diff in both functions:

-export async function fetchPackageVersionFromNpm(packageName: string, timeoutMs = 5000): Promise<string | null> {
-  let timeoutId: NodeJS.Timeout | undefined;
+export async function fetchPackageVersionFromNpm(packageName: string, timeoutMs = 5000): Promise<string | null> {
+  let timeoutId: ReturnType<typeof setTimeout> | undefined;
@@
 export async function fetchPackageVersionFromPyPI(packageName: string, timeoutMs = 5000): Promise<string | null> {
-  let timeoutId: NodeJS.Timeout | undefined;
+  let timeoutId: ReturnType<typeof setTimeout> | undefined;

As per coding guidelines

Also applies to: 120-145

---

150-153: Detect Python managers from args too (env wrappers, absolute paths)

When command is an interpreter or env shim, we may still have pipx/uvx in args. Check both for robustness.

Apply this diff:

-export function isPythonPackageManager(command: string, _args: string[] = []): boolean {
-  const firstToken = String(command || '').trim().split(/\s+/)[0];
-  return firstToken === 'pipx' || firstToken === 'uvx';
-}
+export function isPythonPackageManager(command: string, args: string[] = []): boolean {
+  const tokens = [String(command || '').trim(), ...(args ?? [])].filter(Boolean);
+  // Match exact tokens only to avoid false positives on similarly named binaries
+  return tokens.some(t => t === 'pipx' || t === 'uvx');
+}

---

169-207: Prefer cached lookups for “latest” registry queries

Repeated same-package lookups can be memoized to reduce latency and registry load. Cache only when we perform a registry fetch (i.e., when no version is specified or it’s a dist tag).

Apply this diff around the fetch call:

-  const usePyPI = isPythonPackageManager(server.command, server.args);
-  const latestVersion = usePyPI
-    ? await fetchPackageVersionFromPyPI(packageInfo.name)
-    : await fetchPackageVersionFromNpm(packageInfo.name);
-
-  return latestVersion || "unknown";
+  const usePyPI = isPythonPackageManager(server.command, server.args);
+  const cacheKey = `${usePyPI ? 'pypi' : 'npm'}:${packageInfo.name}`;
+  const cached = versionCache.get(cacheKey);
+  if (cached) return cached;
+
+  const latestVersion = usePyPI
+    ? await fetchPackageVersionFromPyPI(packageInfo.name)
+    : await fetchPackageVersionFromNpm(packageInfo.name);
+
+  if (latestVersion) {
+    versionCache.set(cacheKey, latestVersion);
+    return latestVersion;
+  }
+  return "unknown";

Add this at module scope:

const versionCache = new Map<string, string>();

Note: For long-running processes, consider a small TTL or size cap if package churn is expected.

---

27-41: Dynamic RegExp is safe here but consider literals for clarity

The interpolated manager is a closed union ('npx' | 'bunx'), so ReDoS risk is negligible. If you want to quiet scanners and improve readability, duplicate the patterns as regex literals for each manager or prebuild them once.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Learnt from: CR
PR: agentinit/agentinit#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-24T11:44:03.046Z
Learning: Applies to **/*.{test,spec}.ts : Update tests when modifying functionality