From 86e737b7f7f9b6304d7c44d9319863e8f248fdc9 Mon Sep 17 00:00:00 2001 From: Javi Palacios Date: Thu, 9 Jul 2026 21:12:17 +0200 Subject: [PATCH 1/2] fix(filemerge): preserve symlinked config files --- internal/components/filemerge/writer.go | 41 +++++++++++-- internal/components/filemerge/writer_test.go | 53 ++++++++++++++-- internal/components/uninstall/cleaners.go | 11 +++- .../components/uninstall/cleaners_test.go | 60 +++++++++++++++++-- 4 files changed, 148 insertions(+), 17 deletions(-) diff --git a/internal/components/filemerge/writer.go b/internal/components/filemerge/writer.go index 9e198bd4a..7a8e0ca2a 100644 --- a/internal/components/filemerge/writer.go +++ b/internal/components/filemerge/writer.go @@ -44,8 +44,13 @@ func WriteFileAtomic(path string, content []byte, perm fs.FileMode) (WriteResult perm = 0o644 } + writePath, err := resolveExistingSymlink(path) + if err != nil { + return WriteResult{}, err + } + created := false - existing, err := readComparableFile(path) + existing, err := readComparableFile(writePath) if err == nil { if bytes.Equal(existing, content) { return WriteResult{}, nil @@ -56,8 +61,8 @@ func WriteFileAtomic(path string, content []byte, perm fs.FileMode) (WriteResult created = true } - dir := filepath.Dir(path) - if err := ensureAtomicParentDir(dir, path); err != nil { + dir := filepath.Dir(writePath) + if err := ensureAtomicParentDir(dir, writePath); err != nil { return WriteResult{}, err } @@ -93,7 +98,7 @@ func WriteFileAtomic(path string, content []byte, perm fs.FileMode) (WriteResult return WriteResult{}, fmt.Errorf("close temp file for %q: %w", path, err) } - if err := os.Rename(tmpPath, path); err != nil { + if err := os.Rename(tmpPath, writePath); err != nil { return WriteResult{}, fmt.Errorf("replace %q atomically: %w", path, err) } @@ -110,13 +115,39 @@ func WriteFileAtomic(path string, content []byte, perm fs.FileMode) (WriteResult return WriteResult{Changed: true, Created: created}, nil } +func resolveExistingSymlink(path string) (string, error) { + info, err := os.Lstat(path) + if err != nil { + if os.IsNotExist(err) { + return path, nil + } + return "", fmt.Errorf("stat file %q: %w", path, err) + } + if info.Mode()&os.ModeSymlink == 0 { + return path, nil + } + resolved, err := filepath.EvalSymlinks(path) + if err != nil { + return "", fmt.Errorf("resolve symlink %q: %w", path, err) + } + return resolved, nil +} + func readComparableFile(path string) ([]byte, error) { info, err := os.Lstat(path) if err != nil { return nil, err } if info.Mode()&os.ModeSymlink != 0 { - return nil, fmt.Errorf("refusing to read symlink %q", path) + original := path + path, err = filepath.EvalSymlinks(path) + if err != nil { + return nil, fmt.Errorf("resolve symlink %q: %w", original, err) + } + info, err = os.Stat(path) + if err != nil { + return nil, err + } } if info.Size() > maxAtomicFileSize { return nil, fmt.Errorf("file %q exceeds max atomic compare size %d bytes", path, maxAtomicFileSize) diff --git a/internal/components/filemerge/writer_test.go b/internal/components/filemerge/writer_test.go index 678fc900f..41bfc7a54 100644 --- a/internal/components/filemerge/writer_test.go +++ b/internal/components/filemerge/writer_test.go @@ -86,7 +86,7 @@ func TestWriteFileAtomicCreatesAndIsIdempotent(t *testing.T) { } } -func TestWriteFileAtomicRejectsExistingSymlink(t *testing.T) { +func TestWriteFileAtomicPreservesExistingSymlink(t *testing.T) { dir := t.TempDir() target := filepath.Join(dir, "target.txt") if err := os.WriteFile(target, []byte("old\n"), 0o644); err != nil { @@ -103,12 +103,53 @@ func TestWriteFileAtomicRejectsExistingSymlink(t *testing.T) { t.Fatalf("Symlink() error = %v", err) } - _, err := WriteFileAtomic(path, []byte("new\n"), 0o644) - if err == nil || err.Error() == "" { - t.Fatalf("WriteFileAtomic(symlink) error = %v, want rejection", err) + result, err := WriteFileAtomic(path, []byte("new\n"), 0o644) + if err != nil { + t.Fatalf("WriteFileAtomic(symlink) error = %v, want success", err) + } + if !result.Changed || result.Created { + t.Fatalf("WriteFileAtomic(symlink) result = %+v, want Changed=true Created=false", result) + } + info, err := os.Lstat(path) + if err != nil { + t.Fatalf("Lstat(symlink) error = %v", err) + } + if info.Mode()&os.ModeSymlink == 0 { + t.Fatal("WriteFileAtomic replaced symlink, want symlink preserved") + } + if got, readErr := os.ReadFile(target); readErr != nil || string(got) != "new\n" { + t.Fatalf("target content = %q err=%v, want updated target", got, readErr) + } +} + +func TestWriteFileAtomicSymlinkNoopPreservesLink(t *testing.T) { + dir := t.TempDir() + target := filepath.Join(dir, "target.txt") + content := []byte("same\n") + if err := os.WriteFile(target, content, 0o644); err != nil { + t.Fatalf("WriteFile(target) error = %v", err) + } + path := filepath.Join(dir, "linked.txt") + if err := os.Symlink(target, path); err != nil { + if isSymlinkPrivilegeError(err) { + t.Skipf("skipping: SeCreateSymbolicLinkPrivilege not held on this Windows build: %v", err) + } + t.Fatalf("Symlink() error = %v", err) + } + + result, err := WriteFileAtomic(path, content, 0o644) + if err != nil { + t.Fatalf("WriteFileAtomic(symlink noop) error = %v", err) + } + if result.Changed || result.Created { + t.Fatalf("WriteFileAtomic(symlink noop) result = %+v, want no change", result) + } + info, err := os.Lstat(path) + if err != nil { + t.Fatalf("Lstat(symlink) error = %v", err) } - if got, readErr := os.ReadFile(target); readErr != nil || string(got) != "old\n" { - t.Fatalf("target content changed through symlink: got %q err=%v", got, readErr) + if info.Mode()&os.ModeSymlink == 0 { + t.Fatal("WriteFileAtomic replaced symlink on noop write") } } diff --git a/internal/components/uninstall/cleaners.go b/internal/components/uninstall/cleaners.go index f039bf50b..8272ffaec 100644 --- a/internal/components/uninstall/cleaners.go +++ b/internal/components/uninstall/cleaners.go @@ -6,6 +6,7 @@ import ( "fmt" "io" "os" + "path/filepath" "strings" "github.com/gentleman-programming/gentle-ai/internal/components/filemerge" @@ -237,7 +238,15 @@ func readManagedFile(path string) ([]byte, error) { return nil, fmt.Errorf("stat file %q: %w", path, err) } if info.Mode()&os.ModeSymlink != 0 { - return nil, fmt.Errorf("refusing to read symlink %q", path) + original := path + path, err = filepath.EvalSymlinks(path) + if err != nil { + return nil, fmt.Errorf("resolve symlink %q: %w", original, err) + } + info, err = os.Stat(path) + if err != nil { + return nil, fmt.Errorf("stat symlink target %q: %w", path, err) + } } if info.Size() > maxManagedFileSize { return nil, fmt.Errorf("file %q exceeds max managed size %d bytes", path, maxManagedFileSize) diff --git a/internal/components/uninstall/cleaners_test.go b/internal/components/uninstall/cleaners_test.go index 033be7b82..58cc2888f 100644 --- a/internal/components/uninstall/cleaners_test.go +++ b/internal/components/uninstall/cleaners_test.go @@ -2,12 +2,25 @@ package uninstall import ( "encoding/json" + "errors" "os" "path/filepath" "strings" + "syscall" "testing" ) +func isSymlinkPrivilegeError(err error) bool { + var le *os.LinkError + if errors.As(err, &le) { + var errno syscall.Errno + if errors.As(le.Err, &errno) { + return errno == 1314 // ERROR_PRIVILEGE_NOT_HELD + } + } + return false +} + func FuzzNormalizeJSON_NoPanic(f *testing.F) { seeds := [][]byte{ []byte(`{"mcpServers":{"engram":{"command":"engram"}}}`), @@ -270,20 +283,57 @@ func TestRemoveJSONPaths_PreservesCRLF(t *testing.T) { } } -func TestReadManagedFile_RejectsSymlink(t *testing.T) { +func TestReadManagedFile_FollowsSymlink(t *testing.T) { dir := t.TempDir() target := filepath.Join(dir, "target.json") - if err := os.WriteFile(target, []byte(`{"ok":true}`), 0o644); err != nil { + want := []byte(`{"ok":true}`) + if err := os.WriteFile(target, want, 0o644); err != nil { t.Fatalf("WriteFile(target) error = %v", err) } link := filepath.Join(dir, "link.json") if err := os.Symlink(target, link); err != nil { + if isSymlinkPrivilegeError(err) { + t.Skipf("skipping: SeCreateSymbolicLinkPrivilege not held on this Windows build: %v", err) + } t.Fatalf("Symlink() error = %v", err) } - _, err := readManagedFile(link) - if err == nil || !strings.Contains(err.Error(), "refusing to read symlink") { - t.Fatalf("readManagedFile(symlink) error = %v, want symlink rejection", err) + got, err := readManagedFile(link) + if err != nil { + t.Fatalf("readManagedFile(symlink) error = %v, want success", err) + } + if string(got) != string(want) { + t.Fatalf("readManagedFile(symlink) = %q, want %q", got, want) + } +} + +func TestRemoveFileIfExists_RemovesSymlinkNotTarget(t *testing.T) { + dir := t.TempDir() + target := filepath.Join(dir, "target.md") + content := []byte("keep target\n") + if err := os.WriteFile(target, content, 0o644); err != nil { + t.Fatalf("WriteFile(target) error = %v", err) + } + link := filepath.Join(dir, "link.md") + if err := os.Symlink(target, link); err != nil { + if isSymlinkPrivilegeError(err) { + t.Skipf("skipping: SeCreateSymbolicLinkPrivilege not held on this Windows build: %v", err) + } + t.Fatalf("Symlink() error = %v", err) + } + + if err := removeFileIfExists(link); err != nil { + t.Fatalf("removeFileIfExists(symlink) error = %v", err) + } + if _, err := os.Lstat(link); !os.IsNotExist(err) { + t.Fatalf("symlink still exists after remove: %v", err) + } + got, err := os.ReadFile(target) + if err != nil { + t.Fatalf("ReadFile(target) error = %v", err) + } + if string(got) != string(content) { + t.Fatalf("target content = %q, want %q", got, content) } } From 8b6879426f815c19cd8811ee864aa1ed1d54adb3 Mon Sep 17 00:00:00 2001 From: Javi Palacios Date: Thu, 9 Jul 2026 21:25:47 +0200 Subject: [PATCH 2/2] test(filemerge): cover dangling symlink handling --- internal/components/filemerge/writer.go | 12 ++++++- internal/components/filemerge/writer_test.go | 35 +++++++++++++++++++ internal/components/uninstall/cleaners.go | 3 ++ .../components/uninstall/cleaners_test.go | 20 +++++++++++ 4 files changed, 69 insertions(+), 1 deletion(-) diff --git a/internal/components/filemerge/writer.go b/internal/components/filemerge/writer.go index 7a8e0ca2a..4a40b6d63 100644 --- a/internal/components/filemerge/writer.go +++ b/internal/components/filemerge/writer.go @@ -128,7 +128,17 @@ func resolveExistingSymlink(path string) (string, error) { } resolved, err := filepath.EvalSymlinks(path) if err != nil { - return "", fmt.Errorf("resolve symlink %q: %w", path, err) + if !os.IsNotExist(err) { + return "", fmt.Errorf("resolve symlink %q: %w", path, err) + } + target, readErr := os.Readlink(path) + if readErr != nil { + return "", fmt.Errorf("read symlink %q: %w", path, readErr) + } + if !filepath.IsAbs(target) { + target = filepath.Join(filepath.Dir(path), target) + } + return filepath.Clean(target), nil } return resolved, nil } diff --git a/internal/components/filemerge/writer_test.go b/internal/components/filemerge/writer_test.go index 41bfc7a54..21ec704b3 100644 --- a/internal/components/filemerge/writer_test.go +++ b/internal/components/filemerge/writer_test.go @@ -153,6 +153,41 @@ func TestWriteFileAtomicSymlinkNoopPreservesLink(t *testing.T) { } } +func TestWriteFileAtomicCreatesDanglingSymlinkTarget(t *testing.T) { + dir := t.TempDir() + target := filepath.Join(dir, "missing", "target.txt") + path := filepath.Join(dir, "linked.txt") + if err := os.Symlink(target, path); err != nil { + if isSymlinkPrivilegeError(err) { + t.Skipf("skipping: SeCreateSymbolicLinkPrivilege not held on this Windows build: %v", err) + } + t.Fatalf("Symlink() error = %v", err) + } + + content := []byte("created\n") + result, err := WriteFileAtomic(path, content, 0o644) + if err != nil { + t.Fatalf("WriteFileAtomic(dangling symlink) error = %v, want success", err) + } + if !result.Changed || !result.Created { + t.Fatalf("WriteFileAtomic(dangling symlink) result = %+v, want Changed=true Created=true", result) + } + info, err := os.Lstat(path) + if err != nil { + t.Fatalf("Lstat(symlink) error = %v", err) + } + if info.Mode()&os.ModeSymlink == 0 { + t.Fatal("WriteFileAtomic replaced dangling symlink, want symlink preserved") + } + got, err := os.ReadFile(target) + if err != nil { + t.Fatalf("ReadFile(target) error = %v", err) + } + if !bytes.Equal(got, content) { + t.Fatalf("target content = %q, want %q", got, content) + } +} + func TestWriteFileAtomicRejectsOversizedExistingFile(t *testing.T) { dir := t.TempDir() path := filepath.Join(dir, "big.txt") diff --git a/internal/components/uninstall/cleaners.go b/internal/components/uninstall/cleaners.go index 8272ffaec..202666094 100644 --- a/internal/components/uninstall/cleaners.go +++ b/internal/components/uninstall/cleaners.go @@ -241,6 +241,9 @@ func readManagedFile(path string) ([]byte, error) { original := path path, err = filepath.EvalSymlinks(path) if err != nil { + if os.IsNotExist(err) { + return nil, &os.PathError{Op: "resolve symlink", Path: original, Err: os.ErrNotExist} + } return nil, fmt.Errorf("resolve symlink %q: %w", original, err) } info, err = os.Stat(path) diff --git a/internal/components/uninstall/cleaners_test.go b/internal/components/uninstall/cleaners_test.go index 58cc2888f..f078513d0 100644 --- a/internal/components/uninstall/cleaners_test.go +++ b/internal/components/uninstall/cleaners_test.go @@ -307,6 +307,26 @@ func TestReadManagedFile_FollowsSymlink(t *testing.T) { } } +func TestReadManagedFile_BrokenSymlinkReturnsNotExist(t *testing.T) { + dir := t.TempDir() + target := filepath.Join(dir, "target.json") + link := filepath.Join(dir, "link.json") + if err := os.Symlink(target, link); err != nil { + if isSymlinkPrivilegeError(err) { + t.Skipf("skipping: SeCreateSymbolicLinkPrivilege not held on this Windows build: %v", err) + } + t.Fatalf("Symlink() error = %v", err) + } + + _, err := readManagedFile(link) + if err == nil { + t.Fatal("readManagedFile(broken symlink) error = nil, want error") + } + if !os.IsNotExist(err) { + t.Fatalf("readManagedFile(broken symlink) error = %v, want os.IsNotExist", err) + } +} + func TestRemoveFileIfExists_RemovesSymlinkNotTarget(t *testing.T) { dir := t.TempDir() target := filepath.Join(dir, "target.md")