From 78fe5d5e47cfe3edb9862204a6061b97df2ec914 Mon Sep 17 00:00:00 2001 From: Ola Hungerford Date: Wed, 18 Jun 2025 07:29:25 -0700 Subject: [PATCH] Address PR review comments: fix UNC path handling, improve test coverage, remove problematic test files - Fixed UNC path handling bug in normalizePath function to preserve leading double backslashes - Added comprehensive test coverage for drive letter capitalization and UNC paths - Removed file-operations.test.ts and core-functionality.test.ts as they were testing their own code rather than actual server functionality - All path-utils tests now pass with 100% coverage of the actual utility functions --- .../__tests__/core-functionality.test.ts | 119 --------- .../__tests__/file-operations.test.ts | 252 ------------------ src/filesystem/__tests__/path-utils.test.ts | 29 ++ src/filesystem/path-utils.ts | 23 +- 4 files changed, 49 insertions(+), 374 deletions(-) delete mode 100644 src/filesystem/__tests__/core-functionality.test.ts delete mode 100644 src/filesystem/__tests__/file-operations.test.ts diff --git a/src/filesystem/__tests__/core-functionality.test.ts b/src/filesystem/__tests__/core-functionality.test.ts deleted file mode 100644 index b3927ea1..00000000 --- a/src/filesystem/__tests__/core-functionality.test.ts +++ /dev/null @@ -1,119 +0,0 @@ -import { describe, it, expect, beforeEach, afterEach } from '@jest/globals'; -import fs from 'fs/promises'; -import path from 'path'; -import os from 'os'; - -describe('Core Filesystem Functionality', () => { - let tempDir: string; - let testFile: string; - let testDir: string; - - beforeEach(async () => { - // Create a temporary directory for testing - tempDir = await fs.mkdtemp(path.join(os.tmpdir(), 'filesystem-core-test-')); - testFile = path.join(tempDir, 'test.txt'); - testDir = path.join(tempDir, 'testdir'); - }); - - afterEach(async () => { - // Clean up temporary files - try { - await fs.rm(tempDir, { recursive: true, force: true }); - } catch (error) { - // Ignore cleanup errors - } - }); - - describe('Core File Operations', () => { - it('should read a simple text file', async () => { - const content = 'Hello, world!\nThis is a test file.'; - await fs.writeFile(testFile, content); - - const readContent = await fs.readFile(testFile, 'utf-8'); - expect(readContent).toBe(content); - }); - - it('should write and create a file', async () => { - const content = 'This is test content for writing.'; - - // Write file - await fs.writeFile(testFile, content); - - // Verify it was written correctly - const readContent = await fs.readFile(testFile, 'utf-8'); - expect(readContent).toBe(content); - - // Verify file exists - const stats = await fs.stat(testFile); - expect(stats.isFile()).toBe(true); - }); - - it('should create a directory', async () => { - // Create directory - await fs.mkdir(testDir); - - // Verify directory was created - const stats = await fs.stat(testDir); - expect(stats.isDirectory()).toBe(true); - }); - - it('should list directory contents', async () => { - // Create test directory and files - await fs.mkdir(testDir); - await fs.writeFile(path.join(testDir, 'file1.txt'), 'content1'); - await fs.writeFile(path.join(testDir, 'file2.txt'), 'content2'); - await fs.mkdir(path.join(testDir, 'subdir')); - - // List directory contents - const entries = await fs.readdir(testDir, { withFileTypes: true }); - - expect(entries.length).toBe(3); - - const names = entries.map(e => e.name).sort(); - expect(names).toEqual(['file1.txt', 'file2.txt', 'subdir']); - - // Verify types - const files = entries.filter(e => e.isFile()).map(e => e.name).sort(); - const dirs = entries.filter(e => e.isDirectory()).map(e => e.name); - - expect(files).toEqual(['file1.txt', 'file2.txt']); - expect(dirs).toEqual(['subdir']); - }); - - it('should get file info and metadata', async () => { - const content = 'Test content for file info'; - await fs.writeFile(testFile, content); - - const stats = await fs.stat(testFile); - - expect(stats.isFile()).toBe(true); - expect(stats.isDirectory()).toBe(false); - expect(stats.size).toBe(content.length); - expect(stats.mtime).toBeDefined(); - expect(stats.birthtime).toBeDefined(); - expect(typeof stats.mtime.getTime()).toBe('number'); - expect(typeof stats.birthtime.getTime()).toBe('number'); - }); - }); - - describe('Error Handling', () => { - it('should handle reading non-existent file', async () => { - const nonExistentFile = path.join(tempDir, 'does-not-exist.txt'); - - await expect(fs.readFile(nonExistentFile, 'utf-8')).rejects.toThrow(); - }); - - it('should handle writing to invalid path', async () => { - // Try to write to a path that doesn't exist (parent directory missing) - const invalidPath = path.join(tempDir, 'nonexistent', 'subdir', 'file.txt'); - - await expect(fs.writeFile(invalidPath, 'content')).rejects.toThrow(); - }); - - it('should handle listing non-existent directory', async () => { - const nonExistentDir = path.join(tempDir, 'does-not-exist'); - - await expect(fs.readdir(nonExistentDir)).rejects.toThrow(); - }); - }); -}); diff --git a/src/filesystem/__tests__/file-operations.test.ts b/src/filesystem/__tests__/file-operations.test.ts deleted file mode 100644 index 516e1e10..00000000 --- a/src/filesystem/__tests__/file-operations.test.ts +++ /dev/null @@ -1,252 +0,0 @@ -import { describe, it, expect, beforeEach, afterEach } from '@jest/globals'; -import fs from 'fs/promises'; -import path from 'path'; -import os from 'os'; - -// Test the actual functions by importing them directly -// We'll create a minimal version to test the core logic - -describe('File Operations', () => { - let tempDir: string; - let testFile: string; - let testDir: string; - - beforeEach(async () => { - // Create a temporary directory for testing - tempDir = await fs.mkdtemp(path.join(os.tmpdir(), 'filesystem-test-')); - testFile = path.join(tempDir, 'test.txt'); - testDir = path.join(tempDir, 'testdir'); - - // Create test directory - await fs.mkdir(testDir); - }); - - afterEach(async () => { - // Clean up temporary files - try { - await fs.rm(tempDir, { recursive: true, force: true }); - } catch (error) { - // Ignore cleanup errors - } - }); - - describe('formatSize helper function', () => { - // Test the formatSize function logic directly - it('should format bytes correctly', () => { - // Recreate the formatSize function logic for testing - function formatSize(bytes: number): string { - const units = ['B', 'KB', 'MB', 'GB', 'TB']; - if (bytes === 0) return '0 B'; - - const i = Math.floor(Math.log(bytes) / Math.log(1024)); - if (i === 0) return `${bytes} ${units[i]}`; - - return `${(bytes / Math.pow(1024, i)).toFixed(2)} ${units[i]}`; - } - - const testCases = [ - { bytes: 0, expected: '0 B' }, - { bytes: 500, expected: '500 B' }, - { bytes: 1024, expected: '1.00 KB' }, - { bytes: 1536, expected: '1.50 KB' }, - { bytes: 1048576, expected: '1.00 MB' }, - { bytes: 1073741824, expected: '1.00 GB' } - ]; - - testCases.forEach(testCase => { - expect(formatSize(testCase.bytes)).toBe(testCase.expected); - }); - }); - }); - - describe('headFile function', () => { - // Test the head functionality logic - async function simulateHeadFile(filePath: string, numLines: number): Promise { - const fileHandle = await fs.open(filePath, 'r'); - try { - const lines: string[] = []; - let buffer = ''; - let bytesRead = 0; - const chunk = Buffer.alloc(1024); - - while (lines.length < numLines) { - const result = await fileHandle.read(chunk, 0, chunk.length, bytesRead); - if (result.bytesRead === 0) break; - bytesRead += result.bytesRead; - buffer += chunk.slice(0, result.bytesRead).toString('utf-8'); - - const newLineIndex = buffer.lastIndexOf('\n'); - if (newLineIndex !== -1) { - const completeLines = buffer.slice(0, newLineIndex).split('\n'); - buffer = buffer.slice(newLineIndex + 1); - for (const line of completeLines) { - lines.push(line); - if (lines.length >= numLines) break; - } - } - } - - if (buffer.length > 0 && lines.length < numLines) { - lines.push(buffer); - } - - return lines.join('\n'); - } finally { - await fileHandle.close(); - } - } - - it('should read first N lines of a file', async () => { - const content = 'Line 1\nLine 2\nLine 3\nLine 4\nLine 5\n'; - await fs.writeFile(testFile, content); - - const result = await simulateHeadFile(testFile, 3); - expect(result).toBe('Line 1\nLine 2\nLine 3'); - }); - - it('should handle empty files', async () => { - await fs.writeFile(testFile, ''); - - const result = await simulateHeadFile(testFile, 3); - expect(result).toBe(''); - }); - - it('should handle files with fewer lines than requested', async () => { - const content = 'Line 1\nLine 2'; - await fs.writeFile(testFile, content); - - const result = await simulateHeadFile(testFile, 5); - expect(result).toBe('Line 1\nLine 2'); - }); - }); - - describe('tailFile function', () => { - // Test the tail functionality logic - async function simulateTailFile(filePath: string, numLines: number): Promise { - function normalizeLineEndings(text: string): string { - return text.replace(/\r\n/g, '\n'); - } - - const CHUNK_SIZE = 1024; - const stats = await fs.stat(filePath); - const fileSize = stats.size; - - if (fileSize === 0) return ''; - - const fileHandle = await fs.open(filePath, 'r'); - try { - const lines: string[] = []; - let position = fileSize; - let chunk = Buffer.alloc(CHUNK_SIZE); - let linesFound = 0; - let remainingText = ''; - - while (position > 0 && linesFound < numLines) { - const size = Math.min(CHUNK_SIZE, position); - position -= size; - - const { bytesRead } = await fileHandle.read(chunk, 0, size, position); - if (!bytesRead) break; - - const readData = chunk.slice(0, bytesRead).toString('utf-8'); - const chunkText = readData + remainingText; - - const chunkLines = normalizeLineEndings(chunkText).split('\n'); - - if (position > 0) { - remainingText = chunkLines[0]; - chunkLines.shift(); - } - - for (let i = chunkLines.length - 1; i >= 0 && linesFound < numLines; i--) { - lines.unshift(chunkLines[i]); - linesFound++; - } - } - - return lines.join('\n'); - } finally { - await fileHandle.close(); - } - } - - it('should read last N lines of a file', async () => { - const content = 'Line 1\nLine 2\nLine 3\nLine 4\nLine 5'; - await fs.writeFile(testFile, content); - - const result = await simulateTailFile(testFile, 3); - expect(result).toBe('Line 3\nLine 4\nLine 5'); - }); - - it('should handle empty files', async () => { - await fs.writeFile(testFile, ''); - - const result = await simulateTailFile(testFile, 3); - expect(result).toBe(''); - }); - }); - - describe('head and tail parameter validation', () => { - it('should reject when both head and tail are specified', () => { - // This tests the validation logic that should throw an error - // when both parameters are provided - const hasHead = true; - const hasTail = true; - - if (hasHead && hasTail) { - expect(() => { - throw new Error("Cannot specify both head and tail parameters simultaneously"); - }).toThrow("Cannot specify both head and tail parameters simultaneously"); - } - }); - }); - - describe('list_directory_with_sizes functionality', () => { - it('should list directory contents with sizes', async () => { - // Create test files with known content - await fs.writeFile(path.join(testDir, 'small.txt'), 'hello'); - await fs.writeFile(path.join(testDir, 'large.txt'), 'x'.repeat(1024)); - await fs.mkdir(path.join(testDir, 'subdir')); - - // Read the directory and verify structure - const entries = await fs.readdir(testDir, { withFileTypes: true }); - - expect(entries.length).toBe(3); - - const fileNames = entries.map(e => e.name).sort(); - expect(fileNames).toEqual(['large.txt', 'small.txt', 'subdir']); - - // Verify we can distinguish files from directories - const dirs = entries.filter(e => e.isDirectory()).map(e => e.name); - const files = entries.filter(e => e.isFile()).map(e => e.name); - - expect(dirs).toEqual(['subdir']); - expect(files.sort()).toEqual(['large.txt', 'small.txt']); - }); - - it('should handle sorting by name and size', async () => { - // Create files with different sizes - await fs.writeFile(path.join(testDir, 'big.txt'), 'x'.repeat(2000)); - await fs.writeFile(path.join(testDir, 'small.txt'), 'hi'); - await fs.writeFile(path.join(testDir, 'medium.txt'), 'x'.repeat(1000)); - - const entries = await fs.readdir(testDir, { withFileTypes: true }); - - // Get file sizes - const filesWithSizes = await Promise.all( - entries.filter(e => e.isFile()).map(async (entry) => { - const stats = await fs.stat(path.join(testDir, entry.name)); - return { name: entry.name, size: stats.size }; - }) - ); - - // Test name sorting - const sortedByName = [...filesWithSizes].sort((a, b) => a.name.localeCompare(b.name)); - expect(sortedByName.map(f => f.name)).toEqual(['big.txt', 'medium.txt', 'small.txt']); - - // Test size sorting (descending) - const sortedBySize = [...filesWithSizes].sort((a, b) => b.size - a.size); - expect(sortedBySize.map(f => f.name)).toEqual(['big.txt', 'medium.txt', 'small.txt']); - }); - }); -}); diff --git a/src/filesystem/__tests__/path-utils.test.ts b/src/filesystem/__tests__/path-utils.test.ts index 7bf2c5cd..00df4e04 100644 --- a/src/filesystem/__tests__/path-utils.test.ts +++ b/src/filesystem/__tests__/path-utils.test.ts @@ -124,6 +124,35 @@ describe('Path Utilities', () => { expect(normalizePath('C:\\Path with @at+plus$dollar%percent')) .toBe('C:\\Path with @at+plus$dollar%percent'); }); + + it('capitalizes lowercase drive letters for Windows paths', () => { + expect(normalizePath('c:/windows/system32')) + .toBe('C:\\windows\\system32'); + expect(normalizePath('/mnt/d/my/folder')) // WSL path with lowercase drive + .toBe('D:\\my\\folder'); + expect(normalizePath('/e/another/folder')) // Unix-style Windows path with lowercase drive + .toBe('E:\\another\\folder'); + }); + + it('handles UNC paths correctly', () => { + // UNC paths should preserve the leading double backslash + const uncPath = '\\\\SERVER\\share\\folder'; + expect(normalizePath(uncPath)).toBe('\\\\SERVER\\share\\folder'); + + // Test UNC path with double backslashes that need normalization + const uncPathWithDoubles = '\\\\\\\\SERVER\\\\share\\\\folder'; + expect(normalizePath(uncPathWithDoubles)).toBe('\\\\SERVER\\share\\folder'); + }); + + it('returns normalized non-Windows/WSL/Unix-style Windows paths as is after basic normalization', () => { + // Relative path + const relativePath = 'some/relative/path'; + expect(normalizePath(relativePath)).toBe(relativePath.replace(/\//g, '\\')); + + // A path that looks somewhat absolute but isn't a drive or recognized Unix root for Windows conversion + const otherAbsolutePath = '\\someserver\\share\\file'; + expect(normalizePath(otherAbsolutePath)).toBe(otherAbsolutePath); + }); }); describe('expandHome', () => { diff --git a/src/filesystem/path-utils.ts b/src/filesystem/path-utils.ts index 50e48d51..5297c0d2 100644 --- a/src/filesystem/path-utils.ts +++ b/src/filesystem/path-utils.ts @@ -53,11 +53,28 @@ export function normalizePath(p: string): string { // Convert WSL or Unix-style Windows paths to Windows format p = convertToWindowsPath(p); - // Handle double backslashes and ensure proper escaping - p = p.replace(/\\\\/g, '\\'); + // Check if this is a UNC path before normalization + const isUNCPath = p.startsWith('\\\\'); + + // For non-UNC paths, normalize double backslashes first + if (!isUNCPath) { + p = p.replace(/\\\\/g, '\\'); + } // Use Node's path normalization, which handles . and .. segments - const normalized = path.normalize(p); + let normalized = path.normalize(p); + + // Handle UNC paths after normalization to preserve the leading \\ + if (isUNCPath) { + // Ensure UNC path starts with exactly two backslashes + if (normalized.startsWith('\\') && !normalized.startsWith('\\\\')) { + normalized = '\\' + normalized; + } + // Normalize any remaining double backslashes in the rest of the path + normalized = normalized.replace(/^(\\\\)(.*)/, (match, leading, rest) => { + return leading + rest.replace(/\\\\/g, '\\'); + }); + } // Handle Windows paths: convert slashes and ensure drive letter is capitalized if (normalized.match(/^[a-zA-Z]:|^\/mnt\/[a-z]\/|^\/[a-z]\//i)) {