From 1f12719ba67b10aa0ddec532428d732eabe68e31 Mon Sep 17 00:00:00 2001 From: Bo Zhang <187063395+Tethys0@users.noreply.github.com> Date: Wed, 2 Sep 2026 18:28:32 -0500 Subject: [PATCH] fix(skills-init): skip existing git skills Signed-off-by: Bo Zhang <187063395+Tethys0@users.noreply.github.com> --- go/core/internal/skillsinit/git.go | 43 ++++++++++++++++++++ go/core/internal/skillsinit/git_test.go | 53 +++++++++++++++++++++++++ 2 files changed, 96 insertions(+) diff --git a/go/core/internal/skillsinit/git.go b/go/core/internal/skillsinit/git.go index cbc83369f..c5b43b1cb 100644 --- a/go/core/internal/skillsinit/git.go +++ b/go/core/internal/skillsinit/git.go @@ -10,6 +10,41 @@ import ( var immutableGitCommit = regexp.MustCompile(`^([0-9a-fA-F]{40}|[0-9a-fA-F]{64})$`) +// existingGitSkill reports whether destination already contains a materialized +// skill. A destination without a regular SKILL.md is not safe to treat as a +// completed download, because Run would otherwise continue with an unusable +// skill after a partial initialization. +func existingGitSkill(destination string) (bool, error) { + info, err := os.Lstat(destination) + if os.IsNotExist(err) { + return false, nil + } + if err != nil { + return false, fmt.Errorf("inspect git destination: %w", err) + } + if info.Mode()&os.ModeSymlink != 0 { + return false, fmt.Errorf("existing git destination is not a skill: destination is a symbolic link") + } + if !info.IsDir() { + return false, fmt.Errorf("existing git destination is not a skill: destination is not a directory") + } + + skill, err := os.Lstat(filepath.Join(destination, "SKILL.md")) + if os.IsNotExist(err) { + return false, fmt.Errorf("existing git destination is not a skill: SKILL.md is required") + } + if err != nil { + return false, fmt.Errorf("inspect existing git skill: %w", err) + } + if skill.Mode()&os.ModeSymlink != 0 { + return false, fmt.Errorf("existing git destination is not a skill: SKILL.md is a symbolic link") + } + if !skill.Mode().IsRegular() { + return false, fmt.Errorf("existing git destination is not a skill: SKILL.md is not a regular file") + } + return true, nil +} + // CloneGit fetches a single git ref into ref.Dest. All user-controlled // strings (URL, Ref, SubPath) are passed to git as separate argv entries via // exec.Command — they never pass through a shell, so metacharacters in any of @@ -22,6 +57,14 @@ var immutableGitCommit = regexp.MustCompile(`^([0-9a-fA-F]{40}|[0-9a-fA-F]{64})$ // SubPath, if set, rewrites the destination so the final layout matches the // requested in-repo subdirectory. func CloneGit(ref GitRef) error { + exists, err := existingGitSkill(ref.Dest) + if err != nil { + return err + } + if exists { + return nil + } + if ref.Full { if err := runGit("clone", "--", ref.URL, ref.Dest); err != nil { return err diff --git a/go/core/internal/skillsinit/git_test.go b/go/core/internal/skillsinit/git_test.go index ce29d5a4c..dee423c51 100644 --- a/go/core/internal/skillsinit/git_test.go +++ b/go/core/internal/skillsinit/git_test.go @@ -47,3 +47,56 @@ func TestCloneGitCommitRejectsMutableRef(t *testing.T) { err := CloneGitCommit("https://example.com/repository.git", "main", t.TempDir()) require.ErrorContains(t, err, "full SHA") } + +func TestCloneGitSkipsExistingDestination(t *testing.T) { + dest := filepath.Join(t.TempDir(), "existing-skill") + require.NoError(t, os.MkdirAll(dest, 0o755)) + existingSkill := filepath.Join(dest, "SKILL.md") + require.NoError(t, os.WriteFile(existingSkill, []byte("existing skill\n"), 0o644)) + + err := CloneGit(GitRef{ + URL: filepath.Join(t.TempDir(), "unreachable-source"), + Ref: "main", + Dest: dest, + }) + + require.NoError(t, err) + content, err := os.ReadFile(existingSkill) + require.NoError(t, err) + assert.Equal(t, "existing skill\n", string(content)) +} + +func TestCloneGitRejectsExistingDestinationWithoutSkill(t *testing.T) { + dest := filepath.Join(t.TempDir(), "incomplete-skill") + require.NoError(t, os.MkdirAll(dest, 0o755)) + + err := CloneGit(GitRef{ + URL: filepath.Join(t.TempDir(), "unreachable-source"), + Ref: "main", + Dest: dest, + }) + + require.ErrorContains(t, err, "existing git destination is not a skill") +} + +func TestExistingGitSkillRejectsSymlinks(t *testing.T) { + t.Run("destination", func(t *testing.T) { + target := t.TempDir() + require.NoError(t, os.WriteFile(filepath.Join(target, "SKILL.md"), []byte("skill\n"), 0o644)) + destination := filepath.Join(t.TempDir(), "skill") + require.NoError(t, os.Symlink(target, destination)) + + _, err := existingGitSkill(destination) + require.ErrorContains(t, err, "symbolic link") + }) + + t.Run("skill file", func(t *testing.T) { + destination := t.TempDir() + target := filepath.Join(t.TempDir(), "SKILL.md") + require.NoError(t, os.WriteFile(target, []byte("skill\n"), 0o644)) + require.NoError(t, os.Symlink(target, filepath.Join(destination, "SKILL.md"))) + + _, err := existingGitSkill(destination) + require.ErrorContains(t, err, "symbolic link") + }) +}