Skip to content

Conversation

@c0rychu
Copy link
Owner

@c0rychu c0rychu commented Nov 28, 2025

Great work by @ldkv

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR adds comprehensive Windows support to uvlink by implementing Windows junction support as a fallback when symlinks cannot be created (typically due to permission restrictions on Windows).

Key Changes:

  • Introduced new path_utils.py module with Windows-specific utilities for junction creation and cross-platform path checking
  • Updated symlink/junction detection logic throughout the codebase to handle both symlinks and Windows junctions
  • Added Windows to the CI test matrix and updated workflow triggers

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 10 comments.

Show a summary per file
File Description
src/uvlink/path_utils.py New utility module providing Windows junction support, cross-platform symlink creation with fallback, and helper functions for detecting links/junctions
src/uvlink/project.py Updated rm_rf() and get_list() to handle Windows junctions; improved get_uvlink_dir() Windows path handling
src/uvlink/cli.py Replaced direct symlink operations with new utilities from path_utils; fixed grammar in user-facing messages
tests/test_path_utils.py New test file covering junction creation, symlink fallback behavior, and error handling for invalid targets
tests/test_project.py Enhanced tests for Windows compatibility with platform-specific hash expectations; refactored tilde expansion tests; replaced direct symlink creation with utility function
tests/test_cli.py Updated assertions to check for both symlinks and junctions; simplified symlink removal logic; fixed cache location verification
.github/workflows/ci-tests.yml Added Windows to test matrix and enabled CI on support-windows branch
.github/workflows/ci-checks.yml Updated workflow triggers to include support-windows branch

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.

return path.is_symlink() or path.is_junction()


def path_exists(path: Path) -> bool:
Copy link

Copilot AI Nov 28, 2025

Choose a reason for hiding this comment

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

Missing docstring for path_exists() function. Consider adding documentation:

def path_exists(path: Path) -> bool:
    \"\"\"Check if a path exists, including broken symlinks or junctions.
    
    Args:
        path: The path to check.
        
    Returns:
        bool: True if the path exists or is a symlink/junction (even if broken), False otherwise.
    \"\"\"
Suggested change
def path_exists(path: Path) -> bool:
def path_exists(path: Path) -> bool:
"""
Check if a path exists, including broken symlinks or junctions.
Args:
path (Path): The path to check.
Returns:
bool: True if the path exists or is a symlink/junction (even if broken), False otherwise.
"""

Copilot uses AI. Check for mistakes.
symlink2 = project2_dir / ".venv"
if symlink2.exists() or symlink2.is_symlink():
symlink2.unlink()
symlink2.unlink(missing_ok=True)
Copy link

Copilot AI Nov 28, 2025

Choose a reason for hiding this comment

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

Using unlink(missing_ok=True) is cleaner than the previous conditional, but consider also verifying that the symlink was successfully removed by adding assert not symlink2.exists() after the unlink call to ensure the test state is as expected.

Copilot uses AI. Check for mistakes.
Comment on lines +16 to +24
def test_project_init_path_resolution_with_tilde(self) -> None:
"""Test with tilde expansion and parent directory (e.g., "~/../xxx")."""
user_home_path = Path.home()
test_dir = user_home_path / "test_project"

# Test ~/test_project resolves correctly
p2 = Project(project_dir="~/test_project")
assert p2.project_dir == test_dir.resolve()
assert p2.project_dir.is_absolute()
Copy link

Copilot AI Nov 28, 2025

Choose a reason for hiding this comment

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

This test creates a Project with path "~/test_project" but never creates the actual directory. While the test checks path resolution, it doesn't verify that the Project initialization works correctly when the directory doesn't exist. Consider either creating the directory with test_dir.mkdir(parents=True, exist_ok=True) before creating the Project, or add a separate test case to verify behavior with non-existent directories.

Copilot uses AI. Check for mistakes.
subprocess.check_call(cmd, shell=True)


def create_symlink(symlink: Path, target: Path) -> None:
Copy link

Copilot AI Nov 28, 2025

Choose a reason for hiding this comment

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

Missing docstring for create_symlink() function. Consider adding comprehensive documentation:

def create_symlink(symlink: Path, target: Path) -> None:
    \"\"\"Create a symlink to the target directory, with Windows junction fallback.
    
    Creates the target directory if it doesn't exist. On Windows, falls back to
    creating a junction if symlink creation fails (e.g., due to permissions).
    
    Args:
        symlink: The path where the symlink should be created.
        target: The target directory the symlink should point to.
        
    Raises:
        OSError: If symlink creation fails on non-Windows platforms.
    \"\"\"
Suggested change
def create_symlink(symlink: Path, target: Path) -> None:
def create_symlink(symlink: Path, target: Path) -> None:
"""
Create a symlink to the target directory, with Windows junction fallback.
Ensures the target directory exists. On Windows, if symlink creation fails
(e.g., due to insufficient permissions), falls back to creating a junction.
Args:
symlink (Path): The path where the symlink should be created.
target (Path): The target directory the symlink should point to.
Raises:
OSError: If symlink creation fails on non-Windows platforms.
ValueError: If the target is not a directory when creating a junction.
"""

Copilot uses AI. Check for mistakes.
Comment on lines +8 to +15
def test_create_symlink(tmp_path: Path):
target_dir = tmp_path / "target"
symlink_dir = tmp_path / "symlink"
create_symlink(symlink=symlink_dir, target=target_dir)

assert symlink_dir.exists()
assert symlink_dir.is_symlink() or (is_windows() and symlink_dir.is_junction())
assert symlink_dir.resolve() == target_dir.resolve()
Copy link

Copilot AI Nov 28, 2025

Choose a reason for hiding this comment

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

The test test_create_symlink doesn't verify that the target directory is created by create_symlink. Consider adding an assertion before calling create_symlink to verify the target doesn't exist, and after to verify both symlink and target were created:

assert not target_dir.exists()  # Before
create_symlink(symlink=symlink_dir, target=target_dir)
assert target_dir.exists()  # After

Copilot uses AI. Check for mistakes.
def create_windows_junction(symlink: Path, target: Path) -> None:
"""
Windows junctions are similar to symlinks but specifically for directories.
Does not require admin privileges as symlink_to.
Copy link

Copilot AI Nov 28, 2025

Choose a reason for hiding this comment

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

Grammar error: "as symlink_to" should be "as symlink_to does" or "unlike symlink_to" for clarity. The sentence is missing a verb.

Suggested change
Does not require admin privileges as symlink_to.
Does not require admin privileges, unlike symlink_to.

Copilot uses AI. Check for mistakes.
Comment on lines +27 to +28
cmd = f'mklink /J "{symlink.absolute()}" "{target.absolute()}"'
subprocess.check_call(cmd, shell=True)
Copy link

Copilot AI Nov 28, 2025

Choose a reason for hiding this comment

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

Security risk: Using shell=True with f-string formatting can lead to command injection vulnerabilities if symlink or target paths contain shell metacharacters. Use subprocess.run() with a list of arguments instead:

subprocess.check_call(['cmd', '/c', 'mklink', '/J', str(symlink.absolute()), str(target.absolute())])
Suggested change
cmd = f'mklink /J "{symlink.absolute()}" "{target.absolute()}"'
subprocess.check_call(cmd, shell=True)
cmd = ['cmd', '/c', 'mklink', '/J', str(symlink.absolute()), str(target.absolute())]
subprocess.check_call(cmd)

Copilot uses AI. Check for mistakes.
Comment on lines +31 to +35
def test_create_windows_junction_invalid_target(tmp_path: Path):
with pytest.raises(ValueError):
create_windows_junction(
symlink=tmp_path / "any", target=tmp_path / "nonexistent"
)
Copy link

Copilot AI Nov 28, 2025

Choose a reason for hiding this comment

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

Test test_create_windows_junction_invalid_target should be skipped on non-Windows platforms since create_windows_junction is Windows-specific. Add @pytest.mark.skipif(not is_windows(), reason="Windows junctions are only applicable on Windows.") decorator to match the pattern used in test_create_windows_junction.

Copilot uses AI. Check for mistakes.
from pathlib import Path


def is_windows() -> bool:
Copy link

Copilot AI Nov 28, 2025

Choose a reason for hiding this comment

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

Missing docstring for is_windows() function. Consider adding documentation explaining what this function checks and when it returns True:

def is_windows() -> bool:
    \"\"\"Check if the current platform is Windows.
    
    Returns:
        bool: True if running on Windows (win32 or nt), False otherwise.
    \"\"\"
Suggested change
def is_windows() -> bool:
def is_windows() -> bool:
"""
Check if the current platform is Windows.
Returns:
bool: True if running on Windows (win32 or nt), False otherwise.
"""

Copilot uses AI. Check for mistakes.
return sys.platform == "win32" or os.name == "nt"


def is_link_or_junction(path: Path) -> bool:
Copy link

Copilot AI Nov 28, 2025

Choose a reason for hiding this comment

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

Missing docstring for is_link_or_junction() function. Consider adding documentation:

def is_link_or_junction(path: Path) -> bool:
    \"\"\"Check if a path is a symlink or Windows junction.
    
    Args:
        path: The path to check.
        
    Returns:
        bool: True if the path is a symlink or junction, False otherwise.
    \"\"\"
Suggested change
def is_link_or_junction(path: Path) -> bool:
def is_link_or_junction(path: Path) -> bool:
"""
Check if a path is a symlink or Windows junction.
Args:
path (Path): The path to check.
Returns:
bool: True if the path is a symlink or junction, False otherwise.
"""

Copilot uses AI. Check for mistakes.
@c0rychu c0rychu merged commit 6f9dce5 into main Nov 28, 2025
29 checks passed
@c0rychu c0rychu deleted the support-windows branch November 28, 2025 22:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants