Skip to content

Commit 612bf8c

Browse files
KartikJhaKartikJha
committed
fix: symlink review agent fixes
1 parent 1ca39ec commit 612bf8c

4 files changed

Lines changed: 156 additions & 69 deletions

File tree

‎internal/ai/skills/download.go‎

Lines changed: 0 additions & 61 deletions
Original file line numberDiff line numberDiff line change
@@ -322,67 +322,6 @@ func downloadViaZip(targetDir, ref string) (string, error) {
322322
return sha, nil
323323
}
324324

325-
// mergeDir recursively copies the contents of src into dst. Symlinks are preserved
326-
// (not dereferenced) so the layout matches what git sparse-checkout produces.
327-
func mergeDir(src, dst string) error {
328-
entries, err := os.ReadDir(src)
329-
if err != nil {
330-
return err
331-
}
332-
for _, entry := range entries {
333-
srcPath := filepath.Join(src, entry.Name())
334-
dstPath := filepath.Join(dst, entry.Name())
335-
switch {
336-
case entry.Type()&os.ModeSymlink != 0:
337-
target, err := os.Readlink(srcPath)
338-
if err != nil {
339-
return err
340-
}
341-
if err := os.Symlink(target, dstPath); err != nil {
342-
return err
343-
}
344-
case entry.IsDir():
345-
if err := os.MkdirAll(dstPath, 0o755); err != nil {
346-
return err
347-
}
348-
if err := mergeDir(srcPath, dstPath); err != nil {
349-
return err
350-
}
351-
default:
352-
info, err := entry.Info()
353-
if err != nil {
354-
return err
355-
}
356-
if err := copyFile(srcPath, dstPath, info.Mode()); err != nil {
357-
return err
358-
}
359-
}
360-
}
361-
return nil
362-
}
363-
364-
// copyFile copies src to dst with the given permission mode.
365-
func copyFile(src, dst string, mode os.FileMode) error {
366-
if err := os.MkdirAll(filepath.Dir(dst), 0o755); err != nil {
367-
return err
368-
}
369-
in, err := os.Open(src)
370-
if err != nil {
371-
return err
372-
}
373-
defer in.Close()
374-
out, err := os.OpenFile(dst, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, mode)
375-
if err != nil {
376-
return err
377-
}
378-
_, copyErr := io.Copy(out, in)
379-
closeErr := out.Close()
380-
if copyErr != nil {
381-
return copyErr
382-
}
383-
return closeErr
384-
}
385-
386325
// ExtractEntry writes a single archive entry to destDir. IsDir and mode describe the entry;
387326
// open returns a reader for its content (ignored when isDir is true). The name is checked
388327
// against prefix and any path-traversal attempt is rejected.

‎internal/ai/skills/fs_util.go‎

Lines changed: 68 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,68 @@
1+
package skills
2+
3+
import (
4+
"io"
5+
"os"
6+
"path/filepath"
7+
)
8+
9+
// mergeDir recursively copies the contents of src into dst. Symlinks are preserved
10+
// (not dereferenced) so the layout matches what git sparse-checkout produces.
11+
func mergeDir(src, dst string) error {
12+
entries, err := os.ReadDir(src)
13+
if err != nil {
14+
return err
15+
}
16+
for _, entry := range entries {
17+
srcPath := filepath.Join(src, entry.Name())
18+
dstPath := filepath.Join(dst, entry.Name())
19+
switch {
20+
case entry.Type()&os.ModeSymlink != 0:
21+
target, err := os.Readlink(srcPath)
22+
if err != nil {
23+
return err
24+
}
25+
if err := os.Symlink(target, dstPath); err != nil {
26+
return err
27+
}
28+
case entry.IsDir():
29+
if err := os.MkdirAll(dstPath, 0o755); err != nil {
30+
return err
31+
}
32+
if err := mergeDir(srcPath, dstPath); err != nil {
33+
return err
34+
}
35+
default:
36+
info, err := entry.Info()
37+
if err != nil {
38+
return err
39+
}
40+
if err := copyFile(srcPath, dstPath, info.Mode()); err != nil {
41+
return err
42+
}
43+
}
44+
}
45+
return nil
46+
}
47+
48+
// copyFile copies src to dst with the given permission mode.
49+
func copyFile(src, dst string, mode os.FileMode) error {
50+
if err := os.MkdirAll(filepath.Dir(dst), 0o755); err != nil {
51+
return err
52+
}
53+
in, err := os.Open(src)
54+
if err != nil {
55+
return err
56+
}
57+
defer in.Close()
58+
out, err := os.OpenFile(dst, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, mode)
59+
if err != nil {
60+
return err
61+
}
62+
_, copyErr := io.Copy(out, in)
63+
closeErr := out.Close()
64+
if copyErr != nil {
65+
return copyErr
66+
}
67+
return closeErr
68+
}

‎internal/ai/skills/symlink.go‎

Lines changed: 40 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -28,8 +28,15 @@ func CreateSkillLink(sourceSkillDir, agentSkillsDir, skillName string, useCopy b
2828
return fmt.Errorf("remove existing symlink %s: %w", linkPath, rmErr)
2929
}
3030
} else if info.IsDir() {
31-
// Prior --copy install. Skip: replacing a real dir silently is unsafe.
32-
return nil
31+
if !useCopy {
32+
fmt.Fprintf(os.Stderr,
33+
"warning: %s is a copied directory; remove it manually to switch to symlink mode\n",
34+
linkPath)
35+
return nil
36+
}
37+
// useCopy=true: fall through to re-copy with replace semantics.
38+
} else {
39+
return fmt.Errorf("%s exists as a regular file; remove it before installing skill %q", linkPath, skillName)
3340
}
3441
} else if !os.IsNotExist(err) {
3542
return fmt.Errorf("lstat %s: %w", linkPath, err)
@@ -80,12 +87,35 @@ func createSymlink(sourceSkillDir, agentSkillsDir, linkPath string) error {
8087
return copyDir(sourceSkillDir, linkPath)
8188
}
8289

83-
// copyDir recursively copies src into a newly created dst directory.
90+
// copyDir replaces dst with an exact copy of src.
91+
// Any files in dst that no longer exist in src are removed, so the installed copy
92+
// stays in sync with the canonical source on skill updates.
8493
func copyDir(src, dst string) error {
85-
if err := os.MkdirAll(dst, 0o755); err != nil {
86-
return fmt.Errorf("create copy dir: %w", err)
94+
tmpDst, err := os.MkdirTemp(filepath.Dir(dst), ".skill-copy-*")
95+
if err != nil {
96+
return fmt.Errorf("create temp copy dir: %w", err)
97+
}
98+
if err := mergeDir(src, tmpDst); err != nil {
99+
_ = os.RemoveAll(tmpDst)
100+
return err
101+
}
102+
if err := os.RemoveAll(dst); err != nil {
103+
_ = os.RemoveAll(tmpDst)
104+
return fmt.Errorf("remove stale copy dir: %w", err)
87105
}
88-
return mergeDir(src, dst)
106+
if err := os.Rename(tmpDst, dst); err != nil {
107+
// Cross-filesystem fallback: re-create dst from the temp copy.
108+
if mkErr := os.MkdirAll(dst, 0o755); mkErr != nil {
109+
_ = os.RemoveAll(tmpDst)
110+
return fmt.Errorf("create copy dir: %w", mkErr)
111+
}
112+
if mergeErr := mergeDir(tmpDst, dst); mergeErr != nil {
113+
_ = os.RemoveAll(tmpDst)
114+
return mergeErr
115+
}
116+
_ = os.RemoveAll(tmpDst)
117+
}
118+
return nil
89119
}
90120

91121
// RemoveSkillLink removes the skill entry (symlink or copied directory) at agentSkillsDir/skillName.
@@ -111,7 +141,10 @@ func CheckSkillLink(agentSkillsDir, skillName, expectedSourceDir string) string
111141
linkPath := filepath.Join(agentSkillsDir, skillName)
112142
info, err := os.Lstat(linkPath)
113143
if err != nil {
114-
return "missing"
144+
if os.IsNotExist(err) {
145+
return "missing"
146+
}
147+
return "broken"
115148
}
116149

117150
if info.Mode()&os.ModeSymlink == 0 {

‎internal/ai/skills/symlink_test.go‎

Lines changed: 48 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -74,6 +74,20 @@ func TestCheckSkillLink(t *testing.T) {
7474

7575
assert.Equal(t, "copy", CheckSkillLink(agentDir, "my-skill", "/any/source"))
7676
})
77+
78+
t.Run("broken on permission error (not missing)", func(t *testing.T) {
79+
if os.Getuid() == 0 {
80+
t.Skip("root bypasses permission checks")
81+
}
82+
parent := t.TempDir()
83+
agentDir := filepath.Join(parent, "locked")
84+
require.NoError(t, os.MkdirAll(filepath.Join(agentDir, "my-skill"), 0o755))
85+
require.NoError(t, os.Chmod(agentDir, 0o000))
86+
t.Cleanup(func() { _ = os.Chmod(agentDir, 0o755) })
87+
88+
result := CheckSkillLink(agentDir, "my-skill", "/any/source")
89+
assert.Equal(t, "broken", result)
90+
})
7791
}
7892

7993
// --- CreateSkillLink ---
@@ -168,18 +182,51 @@ func TestCreateSkillLink(t *testing.T) {
168182
assert.Equal(t, "copy", CheckSkillLink(agentDir, "my-skill", src))
169183
})
170184

171-
t.Run("skips real directory when useCopy is false", func(t *testing.T) {
185+
t.Run("warns and skips real directory when useCopy is false", func(t *testing.T) {
172186
agentDir := t.TempDir()
173187
linkPath := filepath.Join(agentDir, "my-skill")
174188
require.NoError(t, os.MkdirAll(linkPath, 0o755))
175189
require.NoError(t, os.WriteFile(filepath.Join(linkPath, "SKILL.md"), []byte("original"), 0o644))
176190

177191
src := makeSkillSource(t)
192+
// Should succeed (skip) but warn to stderr.
178193
require.NoError(t, CreateSkillLink(src, agentDir, "my-skill", false))
179194

195+
// Original directory content must be preserved.
180196
data, err := os.ReadFile(filepath.Join(linkPath, "SKILL.md"))
181197
require.NoError(t, err)
182198
assert.Equal(t, "original", string(data), "original directory should be preserved")
199+
// Entry must still be a real directory, not a symlink.
200+
info, err := os.Lstat(linkPath)
201+
require.NoError(t, err)
202+
assert.Zero(t, info.Mode()&os.ModeSymlink, "entry should remain a directory")
203+
})
204+
205+
t.Run("errors on regular file at linkPath", func(t *testing.T) {
206+
agentDir := t.TempDir()
207+
linkPath := filepath.Join(agentDir, "my-skill")
208+
require.NoError(t, os.WriteFile(linkPath, []byte("not a dir"), 0o644))
209+
210+
src := makeSkillSource(t)
211+
err := CreateSkillLink(src, agentDir, "my-skill", false)
212+
assert.Error(t, err)
213+
})
214+
215+
t.Run("copy is replaced on re-install (replace semantics)", func(t *testing.T) {
216+
src := makeSkillSource(t)
217+
agentDir := t.TempDir()
218+
219+
require.NoError(t, CreateSkillLink(src, agentDir, "my-skill", true))
220+
221+
// Add a stale file directly into the copy.
222+
staleFile := filepath.Join(agentDir, "my-skill", "stale.txt")
223+
require.NoError(t, os.WriteFile(staleFile, []byte("stale"), 0o644))
224+
225+
// Re-run copy install; the stale file should be gone.
226+
require.NoError(t, CreateSkillLink(src, agentDir, "my-skill", true))
227+
228+
_, err := os.Stat(staleFile)
229+
assert.True(t, os.IsNotExist(err), "stale file should be removed after re-install")
183230
})
184231
}
185232

0 commit comments

Comments
 (0)