From 71646490f1df3d5f26a9fa6122bbf72fd9e06172 Mon Sep 17 00:00:00 2001 From: tanzhenxin Date: Tue, 18 Nov 2025 19:38:30 +0800 Subject: [PATCH] Fix: Improve ripgrep binary detection and cross-platform compatibility (#1060) --- packages/core/scripts/postinstall.js | 119 ++++++++------- packages/core/src/tools/ripGrep.test.ts | 15 +- packages/core/src/tools/ripGrep.ts | 23 +-- packages/core/src/utils/ripgrepUtils.test.ts | 150 ++++++++----------- packages/core/src/utils/ripgrepUtils.ts | 100 +++++++------ 5 files changed, 203 insertions(+), 204 deletions(-) diff --git a/packages/core/scripts/postinstall.js b/packages/core/scripts/postinstall.js index 38f6d548..641b89a3 100644 --- a/packages/core/scripts/postinstall.js +++ b/packages/core/scripts/postinstall.js @@ -20,66 +20,81 @@ const vendorDir = path.join(packageRoot, 'vendor', 'ripgrep'); /** * Remove quarantine attribute and set executable permissions on macOS/Linux + * This script never throws errors to avoid blocking npm workflows. */ function setupRipgrepBinaries() { - if (!fs.existsSync(vendorDir)) { - console.log('Vendor directory not found, skipping ripgrep setup'); - return; - } - - const platform = process.platform; - const arch = process.arch; - - // Determine the binary directory based on platform and architecture - let binaryDir; - if (platform === 'darwin' || platform === 'linux') { - const archStr = arch === 'x64' || arch === 'arm64' ? arch : null; - if (archStr) { - binaryDir = path.join(vendorDir, `${archStr}-${platform}`); - } - } else if (platform === 'win32') { - // Windows doesn't need these fixes - return; - } - - if (!binaryDir || !fs.existsSync(binaryDir)) { - console.log( - `Binary directory not found for ${platform}-${arch}, skipping ripgrep setup`, - ); - return; - } - - const rgBinary = path.join(binaryDir, 'rg'); - - if (!fs.existsSync(rgBinary)) { - console.log(`Ripgrep binary not found at ${rgBinary}`); - return; - } - try { - // Set executable permissions - fs.chmodSync(rgBinary, 0o755); - console.log(`✓ Set executable permissions on ${rgBinary}`); + if (!fs.existsSync(vendorDir)) { + console.log('ℹ Vendor directory not found, skipping ripgrep setup'); + return; + } - // On macOS, remove quarantine attribute - if (platform === 'darwin') { - try { - execSync(`xattr -d com.apple.quarantine "${rgBinary}"`, { - stdio: 'pipe', - }); - console.log(`✓ Removed quarantine attribute from ${rgBinary}`); - } catch (error) { - // Quarantine attribute might not exist, which is fine - if (error.message && !error.message.includes('No such xattr')) { - console.warn( - `Warning: Could not remove quarantine attribute: ${error.message}`, - ); + const platform = process.platform; + const arch = process.arch; + + // Determine the binary directory based on platform and architecture + let binaryDir; + if (platform === 'darwin' || platform === 'linux') { + const archStr = arch === 'x64' || arch === 'arm64' ? arch : null; + if (archStr) { + binaryDir = path.join(vendorDir, `${archStr}-${platform}`); + } + } else if (platform === 'win32') { + // Windows doesn't need these fixes + console.log('ℹ Windows detected, skipping ripgrep setup'); + return; + } + + if (!binaryDir || !fs.existsSync(binaryDir)) { + console.log( + `ℹ Binary directory not found for ${platform}-${arch}, skipping ripgrep setup`, + ); + return; + } + + const rgBinary = path.join(binaryDir, 'rg'); + + if (!fs.existsSync(rgBinary)) { + console.log(`ℹ Ripgrep binary not found at ${rgBinary}, skipping setup`); + return; + } + + try { + // Set executable permissions + fs.chmodSync(rgBinary, 0o755); + console.log(`✓ Set executable permissions on ${rgBinary}`); + + // On macOS, remove quarantine attribute + if (platform === 'darwin') { + try { + execSync(`xattr -d com.apple.quarantine "${rgBinary}"`, { + stdio: 'pipe', + }); + console.log(`✓ Removed quarantine attribute from ${rgBinary}`); + } catch { + // Quarantine attribute might not exist, which is fine + console.log('ℹ Quarantine attribute not present or already removed'); } } + } catch (error) { + console.log( + `⚠ Could not complete ripgrep setup: ${error.message || 'Unknown error'}`, + ); + console.log(' This is not critical - ripgrep may still work correctly'); } } catch (error) { - console.error(`Error setting up ripgrep binary: ${error.message}`); + console.log( + `⚠ Ripgrep setup encountered an issue: ${error.message || 'Unknown error'}`, + ); + console.log(' Continuing anyway - this should not affect functionality'); } } -setupRipgrepBinaries(); +// Wrap the entire execution to ensure no errors escape to npm +try { + setupRipgrepBinaries(); +} catch { + // Last resort catch - never let errors block npm + console.log('⚠ Postinstall script encountered an unexpected error'); + console.log(' This will not affect the installation'); +} diff --git a/packages/core/src/tools/ripGrep.test.ts b/packages/core/src/tools/ripGrep.test.ts index a2f813f4..1b7dfe2d 100644 --- a/packages/core/src/tools/ripGrep.test.ts +++ b/packages/core/src/tools/ripGrep.test.ts @@ -22,12 +22,12 @@ import type { Config } from '../config/config.js'; import { createMockWorkspaceContext } from '../test-utils/mockWorkspaceContext.js'; import type { ChildProcess } from 'node:child_process'; import { spawn } from 'node:child_process'; -import { ensureRipgrepPath } from '../utils/ripgrepUtils.js'; +import { getRipgrepCommand } from '../utils/ripgrepUtils.js'; import { DEFAULT_FILE_FILTERING_OPTIONS } from '../config/constants.js'; // Mock ripgrepUtils vi.mock('../utils/ripgrepUtils.js', () => ({ - ensureRipgrepPath: vi.fn(), + getRipgrepCommand: vi.fn(), })); // Mock child_process for ripgrep calls @@ -109,7 +109,7 @@ describe('RipGrepTool', () => { beforeEach(async () => { vi.clearAllMocks(); - (ensureRipgrepPath as Mock).mockResolvedValue('/mock/path/to/rg'); + (getRipgrepCommand as Mock).mockResolvedValue('/mock/path/to/rg'); mockSpawn.mockReset(); tempRootDir = await fs.mkdtemp(path.join(os.tmpdir(), 'grep-tool-root-')); fileExclusionsMock = { @@ -588,18 +588,15 @@ describe('RipGrepTool', () => { }); it('should throw an error if ripgrep is not available', async () => { - // Make ensureRipgrepBinary throw - (ensureRipgrepPath as Mock).mockRejectedValue( - new Error('Ripgrep binary not found'), - ); + (getRipgrepCommand as Mock).mockResolvedValue(null); const params: RipGrepToolParams = { pattern: 'world' }; const invocation = grepTool.build(params); expect(await invocation.execute(abortSignal)).toStrictEqual({ llmContent: - 'Error during grep search operation: Ripgrep binary not found', - returnDisplay: 'Error: Ripgrep binary not found', + 'Error during grep search operation: ripgrep binary not found.', + returnDisplay: 'Error: ripgrep binary not found.', }); }); }); diff --git a/packages/core/src/tools/ripGrep.ts b/packages/core/src/tools/ripGrep.ts index 80273f31..402a5d3c 100644 --- a/packages/core/src/tools/ripGrep.ts +++ b/packages/core/src/tools/ripGrep.ts @@ -6,7 +6,6 @@ import fs from 'node:fs'; import path from 'node:path'; -import { EOL } from 'node:os'; import { spawn } from 'node:child_process'; import type { ToolInvocation, ToolResult } from './tools.js'; import { BaseDeclarativeTool, BaseToolInvocation, Kind } from './tools.js'; @@ -14,7 +13,7 @@ import { ToolNames } from './tool-names.js'; import { resolveAndValidatePath } from '../utils/paths.js'; import { getErrorMessage } from '../utils/errors.js'; import type { Config } from '../config/config.js'; -import { ensureRipgrepPath } from '../utils/ripgrepUtils.js'; +import { getRipgrepCommand } from '../utils/ripgrepUtils.js'; import { SchemaValidator } from '../utils/schemaValidator.js'; import type { FileFilteringOptions } from '../config/constants.js'; import { DEFAULT_FILE_FILTERING_OPTIONS } from '../config/constants.js'; @@ -88,7 +87,7 @@ class GrepToolInvocation extends BaseToolInvocation< } // Split into lines and count total matches - const allLines = rawOutput.split(EOL).filter((line) => line.trim()); + const allLines = rawOutput.split('\n').filter((line) => line.trim()); const totalMatches = allLines.length; const matchTerm = totalMatches === 1 ? 'match' : 'matches'; @@ -159,7 +158,7 @@ class GrepToolInvocation extends BaseToolInvocation< returnDisplay: displayMessage, }; } catch (error) { - console.error(`Error during GrepLogic execution: ${error}`); + console.error(`Error during ripgrep search operation: ${error}`); const errorMessage = getErrorMessage(error); return { llmContent: `Error during grep search operation: ${errorMessage}`, @@ -210,11 +209,15 @@ class GrepToolInvocation extends BaseToolInvocation< rgArgs.push(absolutePath); try { - const rgPath = this.config.getUseBuiltinRipgrep() - ? await ensureRipgrepPath() - : 'rg'; + const rgCommand = await getRipgrepCommand( + this.config.getUseBuiltinRipgrep(), + ); + if (!rgCommand) { + throw new Error('ripgrep binary not found.'); + } + const output = await new Promise((resolve, reject) => { - const child = spawn(rgPath, rgArgs, { + const child = spawn(rgCommand, rgArgs, { windowsHide: true, }); @@ -234,7 +237,7 @@ class GrepToolInvocation extends BaseToolInvocation< child.on('error', (err) => { options.signal.removeEventListener('abort', cleanup); - reject(new Error(`Failed to start ripgrep: ${err.message}.`)); + reject(new Error(`failed to start ripgrep: ${err.message}.`)); }); child.on('close', (code) => { @@ -256,7 +259,7 @@ class GrepToolInvocation extends BaseToolInvocation< return output; } catch (error: unknown) { - console.error(`GrepLogic: ripgrep failed: ${getErrorMessage(error)}`); + console.error(`Ripgrep failed: ${getErrorMessage(error)}`); throw error; } } diff --git a/packages/core/src/utils/ripgrepUtils.test.ts b/packages/core/src/utils/ripgrepUtils.test.ts index 59180aa8..47bba8a5 100644 --- a/packages/core/src/utils/ripgrepUtils.test.ts +++ b/packages/core/src/utils/ripgrepUtils.test.ts @@ -7,8 +7,8 @@ import { describe, it, expect, beforeEach, vi, type Mock } from 'vitest'; import { canUseRipgrep, - ensureRipgrepPath, - getRipgrepPath, + getRipgrepCommand, + getBuiltinRipgrep, } from './ripgrepUtils.js'; import { fileExists } from './fileUtils.js'; import path from 'node:path'; @@ -27,7 +27,7 @@ describe('ripgrepUtils', () => { vi.clearAllMocks(); }); - describe('getRipgrepPath', () => { + describe('getBulltinRipgrepPath', () => { it('should return path with .exe extension on Windows', () => { const originalPlatform = process.platform; const originalArch = process.arch; @@ -36,7 +36,7 @@ describe('ripgrepUtils', () => { Object.defineProperty(process, 'platform', { value: 'win32' }); Object.defineProperty(process, 'arch', { value: 'x64' }); - const rgPath = getRipgrepPath(); + const rgPath = getBuiltinRipgrep(); expect(rgPath).toContain('x64-win32'); expect(rgPath).toContain('rg.exe'); @@ -55,7 +55,7 @@ describe('ripgrepUtils', () => { Object.defineProperty(process, 'platform', { value: 'darwin' }); Object.defineProperty(process, 'arch', { value: 'arm64' }); - const rgPath = getRipgrepPath(); + const rgPath = getBuiltinRipgrep(); expect(rgPath).toContain('arm64-darwin'); expect(rgPath).toContain('rg'); @@ -75,7 +75,7 @@ describe('ripgrepUtils', () => { Object.defineProperty(process, 'platform', { value: 'linux' }); Object.defineProperty(process, 'arch', { value: 'x64' }); - const rgPath = getRipgrepPath(); + const rgPath = getBuiltinRipgrep(); expect(rgPath).toContain('x64-linux'); expect(rgPath).toContain('rg'); @@ -87,7 +87,7 @@ describe('ripgrepUtils', () => { Object.defineProperty(process, 'arch', { value: originalArch }); }); - it('should throw error for unsupported platform', () => { + it('should return null for unsupported platform', () => { const originalPlatform = process.platform; const originalArch = process.arch; @@ -95,14 +95,14 @@ describe('ripgrepUtils', () => { Object.defineProperty(process, 'platform', { value: 'freebsd' }); Object.defineProperty(process, 'arch', { value: 'x64' }); - expect(() => getRipgrepPath()).toThrow('Unsupported platform: freebsd'); + expect(getBuiltinRipgrep()).toBeNull(); // Restore original values Object.defineProperty(process, 'platform', { value: originalPlatform }); Object.defineProperty(process, 'arch', { value: originalArch }); }); - it('should throw error for unsupported architecture', () => { + it('should return null for unsupported architecture', () => { const originalPlatform = process.platform; const originalArch = process.arch; @@ -110,7 +110,7 @@ describe('ripgrepUtils', () => { Object.defineProperty(process, 'platform', { value: 'darwin' }); Object.defineProperty(process, 'arch', { value: 'ia32' }); - expect(() => getRipgrepPath()).toThrow('Unsupported architecture: ia32'); + expect(getBuiltinRipgrep()).toBeNull(); // Restore original values Object.defineProperty(process, 'platform', { value: originalPlatform }); @@ -136,7 +136,7 @@ describe('ripgrepUtils', () => { Object.defineProperty(process, 'platform', { value: platform }); Object.defineProperty(process, 'arch', { value: arch }); - const rgPath = getRipgrepPath(); + const rgPath = getBuiltinRipgrep(); const binaryName = platform === 'win32' ? 'rg.exe' : 'rg'; const expectedPathSegment = path.join( `${arch}-${platform}`, @@ -169,107 +169,77 @@ describe('ripgrepUtils', () => { expect(result).toBe(true); expect(fileExists).toHaveBeenCalledOnce(); }); - - it('should fall back to system rg if bundled ripgrep binary does not exist', async () => { - (fileExists as Mock).mockResolvedValue(false); - // When useBuiltin is true but bundled binary doesn't exist, - // it should fall back to checking system rg (which will spawn a process) - // In this test environment, system rg is likely available, so result should be true - // unless spawn fails - - const result = await canUseRipgrep(); - - // The test may pass or fail depending on system rg availability - // Just verify that fileExists was called to check bundled binary first - expect(fileExists).toHaveBeenCalledOnce(); - // Result depends on whether system rg is installed - expect(typeof result).toBe('boolean'); - }); - - // Note: Tests for system ripgrep detection (useBuiltin=false) would require mocking - // the child_process spawn function, which is complex in ESM. These cases are tested - // indirectly through integration tests. - - it('should return false if platform is unsupported', async () => { - const originalPlatform = process.platform; - - // Mock unsupported platform - Object.defineProperty(process, 'platform', { value: 'aix' }); - - const result = await canUseRipgrep(); - - expect(result).toBe(false); - expect(fileExists).not.toHaveBeenCalled(); - - // Restore original value - Object.defineProperty(process, 'platform', { value: originalPlatform }); - }); - - it('should return false if architecture is unsupported', async () => { - const originalArch = process.arch; - - // Mock unsupported architecture - Object.defineProperty(process, 'arch', { value: 's390x' }); - - const result = await canUseRipgrep(); - - expect(result).toBe(false); - expect(fileExists).not.toHaveBeenCalled(); - - // Restore original value - Object.defineProperty(process, 'arch', { value: originalArch }); - }); }); - describe('ensureRipgrepBinary', () => { - it('should return ripgrep path if binary exists', async () => { + describe('ensureRipgrepPath', () => { + it('should return bundled ripgrep path if binary exists (useBuiltin=true)', async () => { (fileExists as Mock).mockResolvedValue(true); - const rgPath = await ensureRipgrepPath(); + const rgPath = await getRipgrepCommand(true); expect(rgPath).toBeDefined(); expect(rgPath).toContain('rg'); + expect(rgPath).not.toBe('rg'); // Should be full path, not just 'rg' expect(fileExists).toHaveBeenCalledOnce(); expect(fileExists).toHaveBeenCalledWith(rgPath); }); - it('should throw error if binary does not exist', async () => { - (fileExists as Mock).mockResolvedValue(false); + it('should return bundled ripgrep path if binary exists (default)', async () => { + (fileExists as Mock).mockResolvedValue(true); - await expect(ensureRipgrepPath()).rejects.toThrow( - /Ripgrep binary not found/, - ); - await expect(ensureRipgrepPath()).rejects.toThrow(/Platform:/); - await expect(ensureRipgrepPath()).rejects.toThrow(/Architecture:/); + const rgPath = await getRipgrepCommand(); - expect(fileExists).toHaveBeenCalled(); + expect(rgPath).toBeDefined(); + expect(rgPath).toContain('rg'); + expect(fileExists).toHaveBeenCalledOnce(); }); - it('should throw error with correct path information', async () => { + it('should fall back to system rg if bundled binary does not exist', async () => { (fileExists as Mock).mockResolvedValue(false); + // When useBuiltin is true but bundled binary doesn't exist, + // it should fall back to checking system rg + // The test result depends on whether system rg is actually available + + const rgPath = await getRipgrepCommand(true); + + expect(fileExists).toHaveBeenCalledOnce(); + // If system rg is available, it should return 'rg' (or 'rg.exe' on Windows) + // This test will pass if system ripgrep is installed + expect(rgPath).toBeDefined(); + }); + + it('should use system rg when useBuiltin=false', async () => { + // When useBuiltin is false, should skip bundled check and go straight to system rg + const rgPath = await getRipgrepCommand(false); + + // Should not check for bundled binary + expect(fileExists).not.toHaveBeenCalled(); + // If system rg is available, it should return 'rg' (or 'rg.exe' on Windows) + expect(rgPath).toBeDefined(); + }); + + it('should throw error if neither bundled nor system ripgrep is available', async () => { + // This test only makes sense in an environment where system rg is not installed + // We'll skip this test in CI/local environments where rg might be available + // Instead, we test the error message format + const originalPlatform = process.platform; + + // Use an unsupported platform to trigger the error path + Object.defineProperty(process, 'platform', { value: 'freebsd' }); try { - await ensureRipgrepPath(); - // Should not reach here - expect(true).toBe(false); + await getRipgrepCommand(); + // If we get here without error, system rg was available, which is fine } catch (error) { expect(error).toBeInstanceOf(Error); const errorMessage = (error as Error).message; - expect(errorMessage).toContain('Ripgrep binary not found at'); - expect(errorMessage).toContain(process.platform); - expect(errorMessage).toContain(process.arch); + // Should contain helpful error information + expect( + errorMessage.includes('Ripgrep binary not found') || + errorMessage.includes('Failed to locate ripgrep') || + errorMessage.includes('Unsupported platform'), + ).toBe(true); } - }); - - it('should throw error if platform is unsupported', async () => { - const originalPlatform = process.platform; - - // Mock unsupported platform - Object.defineProperty(process, 'platform', { value: 'openbsd' }); - - await expect(ensureRipgrepPath()).rejects.toThrow( - 'Unsupported platform: openbsd', - ); // Restore original value Object.defineProperty(process, 'platform', { value: originalPlatform }); diff --git a/packages/core/src/utils/ripgrepUtils.ts b/packages/core/src/utils/ripgrepUtils.ts index 467bf2ca..c6d795a3 100644 --- a/packages/core/src/utils/ripgrepUtils.ts +++ b/packages/core/src/utils/ripgrepUtils.ts @@ -18,37 +18,42 @@ type Architecture = 'x64' | 'arm64'; /** * Maps process.platform values to vendor directory names */ -function getPlatformString(platform: string): Platform { +function getPlatformString(platform: string): Platform | undefined { switch (platform) { case 'darwin': case 'linux': case 'win32': return platform; default: - throw new Error(`Unsupported platform: ${platform}`); + return undefined; } } /** * Maps process.arch values to vendor directory names */ -function getArchitectureString(arch: string): Architecture { +function getArchitectureString(arch: string): Architecture | undefined { switch (arch) { case 'x64': case 'arm64': return arch; default: - throw new Error(`Unsupported architecture: ${arch}`); + return undefined; } } /** * Returns the path to the bundled ripgrep binary for the current platform + * @returns The path to the bundled ripgrep binary, or null if not available */ -export function getRipgrepPath(): string { +export function getBuiltinRipgrep(): string | null { const platform = getPlatformString(process.platform); const arch = getArchitectureString(process.arch); + if (!platform || !arch) { + return null; + } + // Binary name includes .exe on Windows const binaryName = platform === 'win32' ? 'rg.exe' : 'rg'; @@ -83,6 +88,51 @@ export function getRipgrepPath(): string { return vendorPath; } +/** + * Checks if system ripgrep is available and returns the command to use + * @returns The ripgrep command ('rg' or 'rg.exe') if available, or null if not found + */ +export async function getSystemRipgrep(): Promise { + try { + const { spawn } = await import('node:child_process'); + const rgCommand = process.platform === 'win32' ? 'rg.exe' : 'rg'; + const isAvailable = await new Promise((resolve) => { + const proc = spawn(rgCommand, ['--version']); + proc.on('error', () => resolve(false)); + proc.on('exit', (code) => resolve(code === 0)); + }); + return isAvailable ? rgCommand : null; + } catch (_error) { + return null; + } +} + +/** + * Checks if ripgrep binary exists and returns its path + * @param useBuiltin If true, tries bundled ripgrep first, then falls back to system ripgrep. + * If false, only checks for system ripgrep. + * @returns The path to ripgrep binary ('rg' or 'rg.exe' for system ripgrep, or full path for bundled), or null if not available + */ +export async function getRipgrepCommand( + useBuiltin: boolean = true, +): Promise { + try { + if (useBuiltin) { + // Try bundled ripgrep first + const rgPath = getBuiltinRipgrep(); + if (rgPath && (await fileExists(rgPath))) { + return rgPath; + } + // Fallback to system rg if bundled binary is not available + } + + // Check for system ripgrep + return await getSystemRipgrep(); + } catch (_error) { + return null; + } +} + /** * Checks if ripgrep binary is available * @param useBuiltin If true, tries bundled ripgrep first, then falls back to system ripgrep. @@ -91,42 +141,6 @@ export function getRipgrepPath(): string { export async function canUseRipgrep( useBuiltin: boolean = true, ): Promise { - try { - if (useBuiltin) { - // Try bundled ripgrep first - const rgPath = getRipgrepPath(); - if (await fileExists(rgPath)) { - return true; - } - // Fallback to system rg if bundled binary is not available - } - - // Check for system ripgrep by trying to spawn 'rg --version' - const { spawn } = await import('node:child_process'); - return await new Promise((resolve) => { - const proc = spawn('rg', ['--version']); - proc.on('error', () => resolve(false)); - proc.on('exit', (code) => resolve(code === 0)); - }); - } catch (_error) { - // Unsupported platform/arch or other error - return false; - } -} - -/** - * Ensures ripgrep binary exists and returns its path - * @throws Error if ripgrep binary is not available - */ -export async function ensureRipgrepPath(): Promise { - const rgPath = getRipgrepPath(); - - if (!(await fileExists(rgPath))) { - throw new Error( - `Ripgrep binary not found at ${rgPath}. ` + - `Platform: ${process.platform}, Architecture: ${process.arch}`, - ); - } - - return rgPath; + const rgPath = await getRipgrepCommand(useBuiltin); + return rgPath !== null; }