Skip to content
Merged
135 changes: 135 additions & 0 deletions packages/core/src/tools/read-many-files.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -425,4 +425,139 @@ describe('ReadManyFilesTool', () => {
expect(result.returnDisplay).toContain('bar.ts');
});
});

describe('Batch Processing', () => {
const createMultipleFiles = (count: number, contentPrefix = 'Content') => {
const files: string[] = [];
for (let i = 0; i < count; i++) {
const fileName = `file${i}.txt`;
createFile(fileName, `${contentPrefix} ${i}`);
files.push(fileName);
}
return files;
};

const createFile = (filePath: string, content = '') => {
const fullPath = path.join(tempRootDir, filePath);
fs.mkdirSync(path.dirname(fullPath), { recursive: true });
fs.writeFileSync(fullPath, content);
};

it('should process files in parallel for performance', async () => {
// Mock detectFileType to add artificial delay to simulate I/O
const detectFileTypeSpy = vi.spyOn(
await import('../utils/fileUtils.js'),
'detectFileType',
);

// Create files
const fileCount = 4;
const files = createMultipleFiles(fileCount, 'Batch test');

// Mock with 100ms delay per file to simulate I/O operations
detectFileTypeSpy.mockImplementation(async (_filePath: string) => {
await new Promise((resolve) => setTimeout(resolve, 100));
return 'text';
});

const startTime = Date.now();
const params = { paths: files };
const result = await tool.execute(params, new AbortController().signal);
const endTime = Date.now();

const processingTime = endTime - startTime;

console.log(
`Processing time: ${processingTime}ms for ${fileCount} files`,
);

// Verify parallel processing performance improvement
// Parallel processing should complete in ~100ms (single file time)
// Sequential would take ~400ms (4 files × 100ms each)
expect(processingTime).toBeLessThan(200); // Should PASS with parallel implementation

// Verify all files were processed
const content = result.llmContent as string[];
expect(content).toHaveLength(fileCount);

// Cleanup mock
detectFileTypeSpy.mockRestore();
});

it('should handle batch processing errors gracefully', async () => {
// Create mix of valid and problematic files
createFile('valid1.txt', 'Valid content 1');
createFile('valid2.txt', 'Valid content 2');
createFile('valid3.txt', 'Valid content 3');

const params = {
paths: [
'valid1.txt',
'valid2.txt',
'nonexistent-file.txt', // This will fail
'valid3.txt',
],
};

const result = await tool.execute(params, new AbortController().signal);
const content = result.llmContent as string[];

// Should successfully process valid files despite one failure
expect(content.length).toBeGreaterThanOrEqual(3);
expect(result.returnDisplay).toContain('Successfully read');

// Verify valid files were processed
const expectedPath1 = path.join(tempRootDir, 'valid1.txt');
const expectedPath3 = path.join(tempRootDir, 'valid3.txt');
expect(content.some((c) => c.includes(expectedPath1))).toBe(true);
expect(content.some((c) => c.includes(expectedPath3))).toBe(true);
});

it('should execute file operations concurrently', async () => {
// Track execution order to verify concurrency
const executionOrder: string[] = [];
const detectFileTypeSpy = vi.spyOn(
await import('../utils/fileUtils.js'),
'detectFileType',
);

const files = ['file1.txt', 'file2.txt', 'file3.txt'];
files.forEach((file) => createFile(file, 'test content'));

// Mock to track concurrent vs sequential execution
detectFileTypeSpy.mockImplementation(async (filePath: string) => {
const fileName = filePath.split('/').pop() || '';
executionOrder.push(`start:${fileName}`);

// Add delay to make timing differences visible
await new Promise((resolve) => setTimeout(resolve, 50));

executionOrder.push(`end:${fileName}`);
return 'text';
});

await tool.execute({ paths: files }, new AbortController().signal);

console.log('Execution order:', executionOrder);

// Verify concurrent execution pattern
// In parallel execution: all "start:" events should come before all "end:" events
// In sequential execution: "start:file1", "end:file1", "start:file2", "end:file2", etc.

const startEvents = executionOrder.filter((e) =>
e.startsWith('start:'),
).length;
const firstEndIndex = executionOrder.findIndex((e) =>
e.startsWith('end:'),
);
const startsBeforeFirstEnd = executionOrder
.slice(0, firstEndIndex)
.filter((e) => e.startsWith('start:')).length;

// For parallel processing, ALL start events should happen before the first end event
expect(startsBeforeFirstEnd).toBe(startEvents); // Should PASS with parallel implementation

detectFileTypeSpy.mockRestore();
});
});
});
99 changes: 68 additions & 31 deletions packages/core/src/tools/read-many-files.ts
Original file line number Diff line number Diff line change
Expand Up @@ -400,7 +400,8 @@ Use this tool when the user's query implies needing the content of several files

const sortedFiles = Array.from(filesToConsider).sort();

for (const filePath of sortedFiles) {
// Process files in parallel using Promise.allSettled
const fileProcessingPromises = sortedFiles.map(async (filePath) => {
const relativePathForDisplay = path
.relative(this.config.getTargetDir(), filePath)
.replace(/\\/g, '/');
Expand All @@ -417,12 +418,13 @@ Use this tool when the user's query implies needing the content of several files
);

if (!requestedExplicitly) {
skippedFiles.push({
path: relativePathForDisplay,
return {
success: false,
filePath,
relativePathForDisplay,
reason:
'asset file (image/pdf) was not explicitly requested by name or extension',
});
continue;
};
}
}

Expand All @@ -432,34 +434,69 @@ Use this tool when the user's query implies needing the content of several files
this.config.getTargetDir(),
);

if (fileReadResult.error) {
skippedFiles.push({
path: relativePathForDisplay,
reason: `Read error: ${fileReadResult.error}`,
});
} else {
if (typeof fileReadResult.llmContent === 'string') {
const separator = DEFAULT_OUTPUT_SEPARATOR_FORMAT.replace(
'{filePath}',
filePath,
);
contentParts.push(`${separator}\n\n${fileReadResult.llmContent}\n\n`);
return {
success: !fileReadResult.error,
filePath,
relativePathForDisplay,
fileReadResult,
reason: fileReadResult.error
? `Read error: ${fileReadResult.error}`
: undefined,
};
});

// Wait for all file processing to complete
const results = await Promise.allSettled(fileProcessingPromises);

// Process results
for (const result of results) {
if (result.status === 'fulfilled') {
const fileResult = result.value;

if (!fileResult.success) {
// Handle skipped files (images/PDFs not requested or read errors)
skippedFiles.push({
path: fileResult.relativePathForDisplay,
reason: fileResult.reason!,
});
} else {
contentParts.push(fileReadResult.llmContent); // This is a Part for image/pdf
// Handle successfully processed files
const { filePath, relativePathForDisplay, fileReadResult } =
fileResult;

if (typeof fileReadResult!.llmContent === 'string') {
const separator = DEFAULT_OUTPUT_SEPARATOR_FORMAT.replace(
Copy link
Collaborator

@jacob314 jacob314 Jul 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tweak so you don't need the ! after fileReadResult here and elsewhere. likely you can fix by a union type so fileReadResult is required when success is true. alternately add a case above that should never be it where you report an error if fileReadResult is undefined but success is true with a comment that it shouldn't occur.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jacob314
Fixed! Replaced non-null assertions with union types for better type safety. TypeScript now properly narrows types based on the success field. All tests passing ✅

'{filePath}',
filePath,
);
contentParts.push(
`${separator}\n\n${fileReadResult!.llmContent}\n\n`,
);
} else {
contentParts.push(fileReadResult!.llmContent); // This is a Part for image/pdf
}

processedFilesRelativePaths.push(relativePathForDisplay);

const lines =
typeof fileReadResult!.llmContent === 'string'
? fileReadResult!.llmContent.split('\n').length
: undefined;
const mimetype = getSpecificMimeType(filePath);
recordFileOperationMetric(
this.config,
FileOperation.READ,
lines,
mimetype,
path.extname(filePath),
);
}
processedFilesRelativePaths.push(relativePathForDisplay);
const lines =
typeof fileReadResult.llmContent === 'string'
? fileReadResult.llmContent.split('\n').length
: undefined;
const mimetype = getSpecificMimeType(filePath);
recordFileOperationMetric(
this.config,
FileOperation.READ,
lines,
mimetype,
path.extname(filePath),
);
} else {
// Handle Promise rejection (unexpected errors)
skippedFiles.push({
path: 'unknown',
reason: `Unexpected error: ${result.reason}`,
});
}
}

Expand Down