Skip to content

fix(extension): disable symlinks on Windows during git clone to fix install failure#2286

Open
LaZzyMan wants to merge 1 commit intoQwenLM:mainfrom
LaZzyMan:fix/extension-install-symlink-windows
Open

fix(extension): disable symlinks on Windows during git clone to fix install failure#2286
LaZzyMan wants to merge 1 commit intoQwenLM:mainfrom
LaZzyMan:fix/extension-install-symlink-windows

Conversation

@LaZzyMan
Copy link
Collaborator

TLDR

Fix extension installation failure on Windows caused by Permission denied errors when git tries to create symlinks during checkout. On Windows, non-administrator users cannot create symlinks by default. This PR dynamically sets core.symlinks based on the current platform during git clone.

Dive Deeper

When installing an extension via /extensions install <url>, the cloneFromGit function was hardcoding -c core.symlinks=true in the git clone command. On Windows, creating symlinks requires the SeCreateSymbolicLinkPrivilege privilege, which is not granted to standard users by default.

This caused the following error on Windows:

error: unable to create symlink .claude-plugin/marketplace.json: Permission denied
fatal: unable to checkout working tree
warning: Clone succeeded, but checkout failed.

Fix: Detect the current platform using os.platform() and set core.symlinks accordingly:

  • win32core.symlinks=false (avoids symlink permission errors)
  • All other platforms → core.symlinks=true (preserves existing behavior)

The tradeoff is that symlinks in the repository will be materialized as regular files/directories on Windows, which is acceptable since extension installation does not depend on symlinks for functionality.

Reviewer Test Plan

  1. On Windows (as a standard non-admin user), run:

    /extensions install https://github.com/dotnet/skills
    

    Verify the extension installs successfully without Permission denied errors.

  2. On macOS/Linux, run the same command and verify existing behavior is unchanged.

  3. Run the unit tests:

    cd packages/core && npx vitest run src/extension/github.test.ts
    

    All 30 tests should pass.

Testing Matrix

🍏 🪟 🐧
npm run
npx
Docker
Podman - -
Seatbelt - -

Linked issues / bugs

Fixes #2243

On Windows, non-administrator users do not have permission to create
symlinks by default. Using core.symlinks=true during git clone causes
checkout to fail with 'Permission denied' errors when the repository
contains symlinks.

This fix dynamically sets core.symlinks based on the current platform:
- win32: core.symlinks=false (avoids permission errors)
- other platforms: core.symlinks=true (preserves existing behavior)

Fixes QwenLM#2243
@github-actions
Copy link
Contributor

📋 Review Summary

This PR addresses a critical Windows-specific issue where extension installation fails due to symlink permission errors during git clone. The fix dynamically sets core.symlinks based on the platform, disabling symlinks on Windows to avoid permission errors for non-administrator users. The implementation is clean, well-tested, and solves the reported issue (#2243) effectively.

🔍 General Feedback

  • The fix is minimal and targeted, changing only what's necessary to resolve the Windows symlink issue
  • Good use of os.platform() for platform detection, which is already imported in the file
  • Test coverage is comprehensive with three test cases covering the different platform scenarios
  • The tradeoff (symlinks materialized as regular files on Windows) is acceptable for extension installation use cases
  • Code follows existing project conventions and patterns

🎯 Specific Feedback

🟡 High

  • File: packages/core/src/extension/github.ts:78-82 - Consider extracting the platform check into a named constant or helper function for better readability and testability:
    const isWindows = os.platform() === 'win32';
    const symlinkValue = isWindows ? 'false' : 'true';
    This makes the intent clearer and would be easier to mock in tests if needed.

🟢 Medium

  • File: packages/core/src/extension/github.test.ts:59 - The existing test "should clone, fetch and checkout a repo" now mocks the platform to 'linux' but doesn't assert the core.symlinks=true value. Consider adding an explicit assertion or relying on the new platform-specific tests.

  • File: packages/core/src/extension/github.test.ts:83-125 - The two new platform-specific tests have some code duplication. Consider using a parameterized test approach:

    it.each([
      { platform: 'win32', expectedSymlink: 'false' },
      { platform: 'darwin', expectedSymlink: 'true' },
      { platform: 'linux', expectedSymlink: 'true' },
    ])('should use core.symlinks=$expectedSymlink on $platform', async ({ platform, expectedSymlink }) => {
      mockPlatform.mockReturnValue(platform as string);
      // ... test implementation
    });

🔵 Low

  • File: packages/core/src/extension/github.ts:78-79 - The comment is helpful but could be slightly expanded to mention the specific error being avoided:

    // On Windows, symlinks require elevated privileges by default, so we
    // disable them to avoid "Permission denied" errors during checkout.
    // Symlinks are materialized as regular files/directories on Windows.
  • File: packages/core/src/extension/github.test.ts - Consider adding a test case for 'linux' platform explicitly to ensure it behaves the same as 'darwin', since the original behavior was tested on Linux.

✅ Highlights

  • Excellent problem identification with clear error message documentation in the PR description
  • The fix preserves existing behavior on macOS/Linux while solving the Windows issue
  • Test additions are thorough and cover both Windows and non-Windows platforms
  • Good use of shallow clone (--depth 1) which is preserved in the fix
  • The PR description includes a comprehensive testing matrix and clear reviewer instructions

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.

安装扩展失败

1 participant