-
Notifications
You must be signed in to change notification settings - Fork 24
Fix Windows FS interop by replacing Unix shell commands with cross‑platform Node.js FS API #55
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
256thFission
wants to merge
1
commit into
chriswritescode-dev:main
Choose a base branch
from
256thFission:feat/windows-fs-fix
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -121,3 +121,38 @@ export async function listDirectory(dirPath: string): Promise<Array<{ | |||||||
| throw new Error(`Failed to list directory ${dirPath}: ${error}`) | ||||||||
| } | ||||||||
| } | ||||||||
|
|
||||||||
| 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 | ||||||||
| } | ||||||||
| } | ||||||||
|
|
||||||||
| 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}`) | ||||||||
| } | ||||||||
| } | ||||||||
|
Comment on lines
+135
to
+142
|
||||||||
|
|
||||||||
| export async function listDirectoryNames(dirPath: string): Promise<string[]> { | ||||||||
| try { | ||||||||
| const fullPath = path.isAbsolute(dirPath) ? dirPath : path.join(getReposPath(), dirPath) | ||||||||
| const entries = await fs.readdir(fullPath, { withFileTypes: true }) | ||||||||
| const directories: string[] = [] | ||||||||
| for (const entry of entries) { | ||||||||
| if (entry.isDirectory()) { | ||||||||
| directories.push(entry.name) | ||||||||
| } | ||||||||
| } | ||||||||
| return directories | ||||||||
| } catch { | ||||||||
|
||||||||
| } catch { | |
| } catch (error) { | |
| logger.error(`Failed to list directory names for ${dirPath}:`, error) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -1,5 +1,5 @@ | ||||||
| import { executeCommand } from '../utils/process' | ||||||
| import { ensureDirectoryExists } from './file-operations' | ||||||
| import { ensureDirectoryExists, directoryExists, removeDirectory, listDirectoryNames } from './file-operations' | ||||||
| import * as db from '../db/queries' | ||||||
| import type { Database } from 'bun:sqlite' | ||||||
| import type { Repo, CreateRepoInput } from '../types/repo' | ||||||
|
|
@@ -88,12 +88,7 @@ async function isValidGitRepo(repoPath: string): Promise<boolean> { | |||||
| async function checkRepoNameAvailable(name: string): Promise<boolean> { | ||||||
| const reposPath = getReposPath() | ||||||
| const targetPath = path.join(reposPath, name) | ||||||
| try { | ||||||
| await executeCommand(['test', '-e', targetPath], { silent: true }) | ||||||
| return false | ||||||
| } catch { | ||||||
| return true | ||||||
| } | ||||||
| return !(await directoryExists(targetPath)) | ||||||
| } | ||||||
|
|
||||||
| async function copyRepoToWorkspace(sourcePath: string, targetName: string): Promise<void> { | ||||||
|
|
@@ -186,9 +181,7 @@ export async function initLocalRepo( | |||||
| logger.info(`Absolute path detected: ${normalizedInputPath}`) | ||||||
|
|
||||||
| try { | ||||||
| const exists = await executeCommand(['test', '-d', normalizedInputPath], { silent: true }) | ||||||
| .then(() => true) | ||||||
| .catch(() => false) | ||||||
| const exists = await directoryExists(normalizedInputPath) | ||||||
|
|
||||||
| if (!exists) { | ||||||
| throw new Error(`No such file or directory: '${normalizedInputPath}'`) | ||||||
|
|
@@ -296,14 +289,14 @@ export async function initLocalRepo( | |||||
|
|
||||||
| if (directoryCreated && !sourceWasGitRepo) { | ||||||
| try { | ||||||
| await executeCommand(['rm', '-rf', repoLocalPath], getReposPath()) | ||||||
| await removeDirectory(path.join(getReposPath(), repoLocalPath)) | ||||||
| logger.info(`Rolled back directory: ${repoLocalPath}`) | ||||||
| } catch (fsError: any) { | ||||||
| logger.error(`Failed to rollback directory ${repoLocalPath}:`, fsError) | ||||||
| } | ||||||
| } else if (sourceWasGitRepo) { | ||||||
| try { | ||||||
| await executeCommand(['rm', '-rf', repoLocalPath], getReposPath()) | ||||||
| await removeDirectory(path.join(getReposPath(), repoLocalPath)) | ||||||
| logger.info(`Cleaned up copied directory: ${repoLocalPath}`) | ||||||
| } catch (fsError: any) { | ||||||
| logger.error(`Failed to clean up copied directory ${repoLocalPath}:`, fsError) | ||||||
|
|
@@ -333,9 +326,9 @@ export async function cloneRepo( | |||||
| } | ||||||
|
|
||||||
| await ensureDirectoryExists(getReposPath()) | ||||||
| const baseRepoExists = await executeCommand(['bash', '-c', `test -d ${baseRepoDirName} && echo exists || echo missing`], path.resolve(getReposPath())) | ||||||
| const baseRepoExists = await directoryExists(path.join(getReposPath(), baseRepoDirName)) | ||||||
|
|
||||||
| const shouldUseWorktree = useWorktree && branch && baseRepoExists.trim() === 'exists' | ||||||
| const shouldUseWorktree = useWorktree && branch && baseRepoExists | ||||||
|
|
||||||
| const createRepoInput: CreateRepoInput = { | ||||||
| repoUrl: normalizedRepoUrl, | ||||||
|
|
@@ -366,26 +359,24 @@ export async function cloneRepo( | |||||
|
|
||||||
| await createWorktreeSafely(baseRepoPath, worktreePath, branch) | ||||||
|
|
||||||
| const worktreeVerified = await executeCommand(['test', '-d', worktreePath]) | ||||||
| .then(() => true) | ||||||
| .catch(() => false) | ||||||
| const worktreeVerified = await directoryExists(worktreePath) | ||||||
|
|
||||||
| if (!worktreeVerified) { | ||||||
| throw new Error(`Worktree directory was not created at: ${worktreePath}`) | ||||||
| } | ||||||
|
|
||||||
| logger.info(`Worktree verified at: ${worktreePath}`) | ||||||
|
|
||||||
| } else if (branch && baseRepoExists.trim() === 'exists' && useWorktree) { | ||||||
| } else if (branch && baseRepoExists && useWorktree) { | ||||||
| logger.info(`Base repo exists but worktree creation failed, cloning branch separately`) | ||||||
|
|
||||||
| const worktreeExists = await executeCommand(['bash', '-c', `test -d ${worktreeDirName} && echo exists || echo missing`], path.resolve(getReposPath())) | ||||||
| if (worktreeExists.trim() === 'exists') { | ||||||
| const worktreeExists = await directoryExists(path.join(getReposPath(), worktreeDirName)) | ||||||
| if (worktreeExists) { | ||||||
| logger.info(`Workspace directory exists, removing it: ${worktreeDirName}`) | ||||||
| try { | ||||||
| await executeCommand(['rm', '-rf', worktreeDirName], getReposPath()) | ||||||
| const verifyRemoved = await executeCommand(['bash', '-c', `test -d ${worktreeDirName} && echo exists || echo removed`], getReposPath()) | ||||||
| if (verifyRemoved.trim() === 'exists') { | ||||||
| await removeDirectory(path.join(getReposPath(), worktreeDirName)) | ||||||
| const verifyRemoved = !(await directoryExists(path.join(getReposPath(), worktreeDirName))) | ||||||
| if (!verifyRemoved) { | ||||||
| throw new Error(`Failed to remove existing directory: ${worktreeDirName}`) | ||||||
| } | ||||||
| } catch (cleanupError: any) { | ||||||
|
|
@@ -402,27 +393,32 @@ export async function cloneRepo( | |||||
| throw new Error(`Workspace directory ${worktreeDirName} already exists. Please delete it manually or contact support.`) | ||||||
| } | ||||||
|
|
||||||
| logger.info(`Branch '${branch}' not found during clone, cloning default branch and creating branch locally`) | ||||||
| await executeGitWithFallback(['git', 'clone', normalizedRepoUrl, worktreeDirName], { cwd: getReposPath(), env }) | ||||||
| let localBranchExists = 'missing' | ||||||
| try { | ||||||
| await executeCommand(['git', '-C', path.resolve(getReposPath(), worktreeDirName), 'rev-parse', '--verify', `refs/heads/${branch}`]) | ||||||
| localBranchExists = 'exists' | ||||||
| } catch { | ||||||
| localBranchExists = 'missing' | ||||||
| } | ||||||
| if (localBranchExists.trim() === 'missing') { | ||||||
| if (branch && (error.message.includes('Remote branch') || error.message.includes('not found'))) { | ||||||
|
||||||
| if (branch && (error.message.includes('Remote branch') || error.message.includes('not found'))) { | |
| if (error.message.includes('Remote branch') || error.message.includes('not found')) { |
Copilot
AI
Jan 13, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The initial value of localBranchExists is unused, since it is always overwritten.
Suggested change
| let localBranchExists = 'missing' | |
| let localBranchExists: 'exists' | 'missing' |
Copilot
AI
Jan 13, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
directoryExistsfunction's automatic path resolution behavior (prependinggetReposPath()for relative paths) is inconsistent with how callers use it. All callers inrepo.tsalready construct absolute paths usingpath.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.