Fix Windows FS interop by replacing Unix shell commands with cross‑platform Node.js FS API #55
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes Windows filesystem interoperability by replacing Unix-specific shell commands (test, rm -rf, ls, bash -c) with cross-platform Node.js filesystem APIs. The changes ensure the backend can operate on Windows without relying on Unix commands that produce invalid mixed paths.
Changes:
- Added three new cross-platform helper functions in
file-operations.ts:directoryExists,removeDirectory, andlistDirectoryNames - Replaced all Unix shell command invocations in
repo.tswith the new helper functions - Removed all
bash -cwrapper commands
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| backend/src/services/file-operations.ts | Added three new helper functions for directory operations using Node.js fs promises API |
| backend/src/services/repo.ts | Replaced Unix shell commands with cross-platform helper functions across all repository operations |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| export async function directoryExists(dirPath: string): Promise<boolean> { | ||
| try { | ||
| const fullPath = path.isAbsolute(dirPath) ? dirPath : path.join(getReposPath(), dirPath) | ||
| const stats = await fs.stat(fullPath) | ||
| return stats.isDirectory() | ||
| } catch { | ||
| return false | ||
| } | ||
| } |
There was a problem hiding this comment.
The directoryExists function's automatic path resolution behavior (prepending getReposPath() for relative paths) is inconsistent with how callers use it. All callers in repo.ts already construct absolute paths using path.join(getReposPath(), ...) before passing them to this function, making the internal path resolution redundant. This could lead to confusion or bugs if a caller passes a relative path expecting different behavior. Consider either: (1) removing the automatic path resolution and requiring absolute paths, or (2) documenting this behavior clearly and updating callers to pass relative paths directly.
| export async function removeDirectory(dirPath: string): Promise<void> { | ||
| try { | ||
| const fullPath = path.isAbsolute(dirPath) ? dirPath : path.join(getReposPath(), dirPath) | ||
| await fs.rm(fullPath, { recursive: true, force: true }) | ||
| } catch (error) { | ||
| throw new Error(`Failed to remove directory ${dirPath}: ${error}`) | ||
| } | ||
| } |
There was a problem hiding this comment.
Same path resolution issue as directoryExists. All callers construct absolute paths before calling this function (e.g., removeDirectory(path.join(getReposPath(), dirName))), making the internal path resolution logic redundant and potentially confusing. Consider standardizing the approach across all file operation functions.
| } | ||
| } | ||
| return directories | ||
| } catch { |
There was a problem hiding this comment.
The error handling silently returns an empty array on any error, which could mask actual problems (e.g., permission issues, invalid paths). This differs from removeDirectory which throws errors. Consider either throwing errors like the other functions, or at minimum logging the error before returning an empty array so failures aren't completely silent.
| } catch { | |
| } catch (error) { | |
| logger.error(`Failed to list directory names for ${dirPath}:`, error) |
| localBranchExists = 'missing' | ||
| } | ||
| if (localBranchExists.trim() === 'missing') { | ||
| if (branch && (error.message.includes('Remote branch') || error.message.includes('not found'))) { |
There was a problem hiding this comment.
The condition checks if branch exists before entering the error handling block, but branch is already guaranteed to be truthy at this point in the code flow (line 370 condition is branch && baseRepoExists && useWorktree). The redundant check reduces code clarity.
| if (branch && (error.message.includes('Remote branch') || error.message.includes('not found'))) { | |
| if (error.message.includes('Remote branch') || error.message.includes('not found')) { |
| localBranchExists = 'missing' | ||
| } | ||
|
|
||
| if (localBranchExists === 'missing') { |
There was a problem hiding this comment.
The changes from string-based existence checks ('exists'/'missing') to boolean checks are correct, but the logic at lines 399-411 for handling local branch creation when a remote branch is not found lacks test coverage. This is a critical code path for branch creation fallback that should be tested to ensure the Windows compatibility fix works correctly in this scenario.
| if (branch && (error.message.includes('Remote branch') || error.message.includes('not found'))) { | ||
| logger.info(`Branch '${branch}' not found, cloning default branch and creating branch locally`) | ||
| await executeGitWithFallback(['git', 'clone', normalizedRepoUrl, worktreeDirName], { cwd: getReposPath(), env }) | ||
| let localBranchExists = 'missing' |
There was a problem hiding this comment.
The initial value of localBranchExists is unused, since it is always overwritten.
| let localBranchExists = 'missing' | |
| let localBranchExists: 'exists' | 'missing' |
1 participant
Problem
On Windows, repo operations failed because the backend invoked Unix commands like
test,rm -rf,ls, andbash -c, which produced invalid mixed paths (ie./workspace/repos/C:\Users\...).Fix
bash -cwrappersTesting