fix: use os.samefile for symlink check, remove tmo file in copy dir

Author: KartikJha

internal/ai/skills/fs_util.go

@@ -22,6 +22,9 @@ func mergeDir(src, dst string) error {
 			if err != nil {
 				return err
 			}
+			// os.Symlink is not idempotent (returns EEXIST). Remove any existing
+			// entry so the call is safe under concurrent writes or repeated merges.
+			_ = os.Remove(dstPath)
 			if err := os.Symlink(target, dstPath); err != nil {
 				return err
 			}

internal/ai/skills/fs_util_test.go

@@ -0,0 +1,76 @@
+package skills
+
+import (
+	"os"
+	"path/filepath"
+	"runtime"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+func TestMergeDir(t *testing.T) {
+	if runtime.GOOS == "windows" {
+		t.Skip("symlink tests skipped on windows")
+	}
+
+	t.Run("copies regular files", func(t *testing.T) {
+		src := t.TempDir()
+		dst := t.TempDir()
+		require.NoError(t, os.WriteFile(filepath.Join(src, "file.txt"), []byte("hello"), 0o644))
+
+		require.NoError(t, mergeDir(src, dst))
+
+		data, err := os.ReadFile(filepath.Join(dst, "file.txt"))
+		require.NoError(t, err)
+		assert.Equal(t, "hello", string(data))
+	})
+
+	t.Run("preserves symlinks", func(t *testing.T) {
+		src := t.TempDir()
+		dst := t.TempDir()
+		target := filepath.Join(src, "target.txt")
+		require.NoError(t, os.WriteFile(target, []byte("target"), 0o644))
+		require.NoError(t, os.Symlink(target, filepath.Join(src, "link")))
+
+		require.NoError(t, mergeDir(src, dst))
+
+		linkDst := filepath.Join(dst, "link")
+		info, err := os.Lstat(linkDst)
+		require.NoError(t, err)
+		assert.NotZero(t, info.Mode()&os.ModeSymlink, "should be a symlink")
+	})
+
+	t.Run("symlink overwrite is idempotent (no EEXIST)", func(t *testing.T) {
+		src := t.TempDir()
+		dst := t.TempDir()
+		target := filepath.Join(src, "target.txt")
+		require.NoError(t, os.WriteFile(target, []byte("target"), 0o644))
+		require.NoError(t, os.Symlink(target, filepath.Join(src, "link")))
+
+		// First merge creates the symlink.
+		require.NoError(t, mergeDir(src, dst))
+		// Second merge must not fail with EEXIST.
+		require.NoError(t, mergeDir(src, dst))
+
+		linkDst := filepath.Join(dst, "link")
+		info, err := os.Lstat(linkDst)
+		require.NoError(t, err)
+		assert.NotZero(t, info.Mode()&os.ModeSymlink)
+	})
+
+	t.Run("recurses into subdirectories", func(t *testing.T) {
+		src := t.TempDir()
+		dst := t.TempDir()
+		sub := filepath.Join(src, "sub")
+		require.NoError(t, os.MkdirAll(sub, 0o755))
+		require.NoError(t, os.WriteFile(filepath.Join(sub, "nested.txt"), []byte("nested"), 0o644))
+
+		require.NoError(t, mergeDir(src, dst))
+
+		data, err := os.ReadFile(filepath.Join(dst, "sub", "nested.txt"))
+		require.NoError(t, err)
+		assert.Equal(t, "nested", string(data))
+	})
+}

internal/ai/skills/symlink.go

@@ -2,12 +2,16 @@ package skills
 
 import (
 	"fmt"
+	"io"
 	"os"
 	"os/exec"
 	"path/filepath"
 	"runtime"
 )
 
+// stderrWriter is the target for diagnostic output. Replaced in tests.
+var stderrWriter io.Writer = os.Stderr
+
 // CreateSkillLink installs skillName from sourceSkillDir into agentSkillsDir.
 // useCopy=true copies files recursively; useCopy=false creates a symlink.
 // The operation is idempotent: a correct existing symlink or copy is left unchanged.
@@ -21,15 +25,17 @@ func CreateSkillLink(sourceSkillDir, agentSkillsDir, skillName string, useCopy b
 	info, err := os.Lstat(linkPath)
 	if err == nil {
 		if info.Mode()&os.ModeSymlink != 0 {
-			if isSymlinkCorrect(linkPath, agentSkillsDir, sourceSkillDir) {
+			// For useCopy=false: skip if already pointing to the right place.
+			// For useCopy=true: remove the symlink so we can replace it with a copy.
+			if !useCopy && isSymlinkCorrect(linkPath, sourceSkillDir) {
 				return nil
 			}
 			if rmErr := os.Remove(linkPath); rmErr != nil {
 				return fmt.Errorf("remove existing symlink %s: %w", linkPath, rmErr)
 			}
 		} else if info.IsDir() {
 			if !useCopy {
-				fmt.Fprintf(os.Stderr,
+				fmt.Fprintf(stderrWriter,
 					"warning: %s is a copied directory; remove it manually to switch to symlink mode\n",
 					linkPath)
 				return nil
@@ -49,19 +55,17 @@ func CreateSkillLink(sourceSkillDir, agentSkillsDir, skillName string, useCopy b
 }
 
 // isSymlinkCorrect returns true if linkPath is a non-broken symlink resolving to sourceSkillDir.
-func isSymlinkCorrect(linkPath, agentSkillsDir, sourceSkillDir string) bool {
-	target, err := os.Readlink(linkPath)
+// os.SameFile is used instead of string comparison to handle case-insensitive filesystems (e.g. macOS APFS).
+func isSymlinkCorrect(linkPath, sourceSkillDir string) bool {
+	linkInfo, err := os.Stat(linkPath)
 	if err != nil {
-		return false
-	}
-	if !filepath.IsAbs(target) {
-		target = filepath.Join(agentSkillsDir, target)
+		return false // broken symlink
 	}
-	if filepath.Clean(target) != filepath.Clean(sourceSkillDir) {
+	srcInfo, err := os.Stat(sourceSkillDir)
+	if err != nil {
 		return false
 	}
-	_, err = os.Stat(linkPath)
-	return err == nil
+	return os.SameFile(linkInfo, srcInfo)
 }
 
 // createSymlink creates a symlink at linkPath pointing to sourceSkillDir.
@@ -83,7 +87,7 @@ func createSymlink(sourceSkillDir, agentSkillsDir, linkPath string) error {
 	if err := exec.Command("cmd", "/C", "mklink", "/J", linkPath, sourceSkillDir).Run(); err == nil {
 		return nil
 	}
-	fmt.Fprintf(os.Stderr, "warning: symlink and junction unavailable; copying %s to %s\n", sourceSkillDir, linkPath)
+	fmt.Fprintf(stderrWriter, "warning: symlink and junction unavailable; copying %s to %s\n", sourceSkillDir, linkPath)
 	return copyDir(sourceSkillDir, linkPath)
 }
 
@@ -95,25 +99,31 @@ func copyDir(src, dst string) error {
 	if err != nil {
 		return fmt.Errorf("create temp copy dir: %w", err)
 	}
+	// Always clean up the temp dir so it is never left as an orphan in agentSkillsDir.
+	tmpRemoved := false
+	defer func() {
+		if !tmpRemoved {
+			_ = os.RemoveAll(tmpDst)
+		}
+	}()
+
 	if err := mergeDir(src, tmpDst); err != nil {
-		_ = os.RemoveAll(tmpDst)
 		return err
 	}
 	if err := os.RemoveAll(dst); err != nil {
-		_ = os.RemoveAll(tmpDst)
 		return fmt.Errorf("remove stale copy dir: %w", err)
 	}
 	if err := os.Rename(tmpDst, dst); err != nil {
 		// Cross-filesystem fallback: re-create dst from the temp copy.
+		fmt.Fprintf(stderrWriter, "warning: rename %s → %s failed (%v); falling back to copy\n", tmpDst, dst, err)
 		if mkErr := os.MkdirAll(dst, 0o755); mkErr != nil {
-			_ = os.RemoveAll(tmpDst)
 			return fmt.Errorf("create copy dir: %w", mkErr)
 		}
 		if mergeErr := mergeDir(tmpDst, dst); mergeErr != nil {
-			_ = os.RemoveAll(tmpDst)
 			return mergeErr
 		}
-		_ = os.RemoveAll(tmpDst)
+	} else {
+		tmpRemoved = true // rename succeeded; temp dir is now dst
 	}
 	return nil
 }
@@ -151,20 +161,18 @@ func CheckSkillLink(agentSkillsDir, skillName, expectedSourceDir string) string
 		return "copy"
 	}
 
-	// It's a symlink. Verify the target exists.
-	if _, err := os.Stat(linkPath); err != nil {
+	// It's a symlink. Verify the target exists by following the link.
+	resolvedInfo, err := os.Stat(linkPath)
+	if err != nil {
 		return "broken"
 	}
 
-	// Target exists. Check if it points to the expected place.
-	target, err := os.Readlink(linkPath)
+	// Use os.SameFile to handle case-insensitive filesystems (e.g. macOS APFS).
+	srcInfo, err := os.Stat(expectedSourceDir)
 	if err != nil {
-		return "broken"
-	}
-	if !filepath.IsAbs(target) {
-		target = filepath.Join(agentSkillsDir, target)
+		return "wrong_target"
 	}
-	if filepath.Clean(target) == filepath.Clean(expectedSourceDir) {
+	if os.SameFile(resolvedInfo, srcInfo) {
 		return "ok"
 	}
 	return "wrong_target"

internal/ai/skills/symlink_test.go

@@ -1,15 +1,27 @@
 package skills
 
 import (
+	"bytes"
 	"os"
 	"path/filepath"
 	"runtime"
+	"strings"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )
 
+// captureStderr replaces stderrWriter with a buffer for the duration of the test.
+func captureStderr(t *testing.T) *bytes.Buffer {
+	t.Helper()
+	buf := &bytes.Buffer{}
+	orig := stderrWriter
+	stderrWriter = buf
+	t.Cleanup(func() { stderrWriter = orig })
+	return buf
+}
+
 // makeSkillSource creates a temporary directory with a SKILL.md file inside.
 func makeSkillSource(t *testing.T) string {
 	t.Helper()
@@ -183,6 +195,7 @@ func TestCreateSkillLink(t *testing.T) {
 	})
 
 	t.Run("warns and skips real directory when useCopy is false", func(t *testing.T) {
+		buf := captureStderr(t)
 		agentDir := t.TempDir()
 		linkPath := filepath.Join(agentDir, "my-skill")
 		require.NoError(t, os.MkdirAll(linkPath, 0o755))
@@ -200,6 +213,8 @@ func TestCreateSkillLink(t *testing.T) {
 		info, err := os.Lstat(linkPath)
 		require.NoError(t, err)
 		assert.Zero(t, info.Mode()&os.ModeSymlink, "entry should remain a directory")
+		// Warning must be emitted to stderr.
+		assert.True(t, strings.Contains(buf.String(), "warning:"), "expected warning on stderr, got: %q", buf.String())
 	})
 
 	t.Run("errors on regular file at linkPath", func(t *testing.T) {
@@ -228,6 +243,29 @@ func TestCreateSkillLink(t *testing.T) {
 		_, err := os.Stat(staleFile)
 		assert.True(t, os.IsNotExist(err), "stale file should be removed after re-install")
 	})
+
+	t.Run("converts existing correct symlink to copy when useCopy switches to true", func(t *testing.T) {
+		src := makeSkillSource(t)
+		agentDir := t.TempDir()
+
+		// Install as symlink first.
+		require.NoError(t, CreateSkillLink(src, agentDir, "my-skill", false))
+		assert.Equal(t, "ok", CheckSkillLink(agentDir, "my-skill", src))
+		info, err := os.Lstat(filepath.Join(agentDir, "my-skill"))
+		require.NoError(t, err)
+		assert.NotZero(t, info.Mode()&os.ModeSymlink, "should be a symlink after first install")
+
+		// Re-install with useCopy=true; the symlink must be replaced by a real directory.
+		require.NoError(t, CreateSkillLink(src, agentDir, "my-skill", true))
+		assert.Equal(t, "copy", CheckSkillLink(agentDir, "my-skill", src))
+		info, err = os.Lstat(filepath.Join(agentDir, "my-skill"))
+		require.NoError(t, err)
+		assert.Zero(t, info.Mode()&os.ModeSymlink, "should be a real directory after copy install")
+		data, err := os.ReadFile(filepath.Join(agentDir, "my-skill", "SKILL.md"))
+		require.NoError(t, err)
+		assert.Equal(t, "# skill", string(data))
+	})
+
 }
 
 // --- RemoveSkillLink ---