diff --git a/cmd/skillshare/install.go b/cmd/skillshare/install.go index 68045c0dc..ad165c707 100644 --- a/cmd/skillshare/install.go +++ b/cmd/skillshare/install.go @@ -201,7 +201,9 @@ func parseInstallArgs(args []string) (*installArgs, bool, error) { if result.opts.Branch != "" && result.sourceArg != "" { source, parseErr := install.ParseSource(result.sourceArg) - if parseErr == nil && !source.IsGit() { + trackableLocal := result.opts.Track && source != nil && + source.Type == install.SourceTypeLocalPath && install.IsLocalGitRepo(source.Path) + if parseErr == nil && !source.IsGit() && !trackableLocal { return nil, false, fmt.Errorf("--branch can only be used with git repository sources") } } @@ -597,7 +599,7 @@ func printInstallHelp() { {"-f, --force", "Overwrite existing skill; also continue if audit would block"}, {"-u, --update", "Update existing (git pull if possible, else reinstall)"}, {"-b, --branch ", "Git branch, tag, or commit SHA to install from (default: remote default)"}, - {"-t, --track", "Install as tracked repo (preserves .git for updates)"}, + {"-t, --track", "Install as tracked repo (preserves .git for updates; a local\npath works if it is a git repository)"}, {"-a, --agent ", "Select specific agents from a multi-agent repo (comma-separated)"}, {"-s, --skill ", "Select specific skills from multi-skill repo (comma-separated;\nsupports glob patterns like \"core-*\", \"test-?\")"}, {"--exclude ", "Skip specific skills during install (comma-separated;\nsupports glob patterns like \"test-*\")"}, diff --git a/internal/install/install.go b/internal/install/install.go index 4cfa9e03a..95c9d18cc 100644 --- a/internal/install/install.go +++ b/internal/install/install.go @@ -280,8 +280,8 @@ func (e *TrackKindAmbiguousError) Error() string { // the kind explicitly to avoid ambiguous install roots; in that case the // returned error is a *TrackKindAmbiguousError carrying the discovered counts. func InferTrackedKind(source *Source, explicitKind string) (string, error) { - if !source.IsGit() { - return "", fmt.Errorf("--track requires a git repository source") + if err := normalizeTrackSource(source); err != nil { + return "", err } if explicitKind == "skill" || explicitKind == "agent" { diff --git a/internal/install/install_git.go b/internal/install/install_git.go index 333fde9ee..1bf7dd6c5 100644 --- a/internal/install/install_git.go +++ b/internal/install/install_git.go @@ -35,6 +35,17 @@ func IsGitRepo(path string) bool { return err == nil && (info.IsDir() || info.Mode().IsRegular()) } +// IsLocalGitRepo reports whether path is the root of a git repository, either a +// working tree (.git) or a bare repository (HEAD and objects at the root). +func IsLocalGitRepo(path string) bool { + if IsGitRepo(path) { + return true + } + head, headErr := os.Stat(filepath.Join(path, "HEAD")) + objects, objErr := os.Stat(filepath.Join(path, "objects")) + return headErr == nil && head.Mode().IsRegular() && objErr == nil && objects.IsDir() +} + // IsTrackedCheckout reports whether path is a tracked repo checkout: a // _-prefixed directory that is a git repo. func IsTrackedCheckout(path string) bool { diff --git a/internal/install/install_tracked.go b/internal/install/install_tracked.go index 1ffc956d9..4874687a5 100644 --- a/internal/install/install_tracked.go +++ b/internal/install/install_tracked.go @@ -2,16 +2,83 @@ package install import ( "fmt" + "net/url" "os" "path/filepath" + "strconv" "strings" + "time" "skillshare/internal/sourcefs" ) +// localFileURL builds the file:// URL for a local path. A Windows drive path +// (C:/repo) needs the empty authority (file:///C:/repo), and characters such as +// spaces, '#' and '%' must be escaped because git decodes the URL. +func localFileURL(path string) string { + p := filepath.ToSlash(path) + if !strings.HasPrefix(p, "/") { + p = "/" + p + } + return (&url.URL{Scheme: "file", Path: p}).String() +} + +// localCloneSource returns the filesystem path a local source clones from: the +// plain path, or the path a file:// URL names, or "" for a remote source. +func localCloneSource(source *Source) string { + if source.Path != "" { + return source.Path + } + u, err := url.Parse(source.CloneURL) + if err != nil || u.Scheme != "file" { + return "" + } + p := u.Path + switch { + case u.Host != "" && !strings.EqualFold(u.Host, "localhost"): + p = "//" + u.Host + p // UNC: file://server/share/repo + case len(p) > 2 && p[0] == '/' && p[2] == ':': + p = p[1:] // file:///C:/repo + } + return filepath.FromSlash(p) +} + +// pathWithin reports whether path is dir itself or lies below it, after +// resolving links. +func pathWithin(dir, path string) bool { + resolve := func(p string) string { + if abs, err := filepath.Abs(p); err == nil { + p = abs + } + if r, err := filepath.EvalSymlinks(p); err == nil { + p = r + } + return p + } + rel, err := filepath.Rel(resolve(dir), resolve(path)) + return err == nil && rel != ".." && !strings.HasPrefix(rel, ".."+string(filepath.Separator)) +} + +// normalizeTrackSource turns a local path that is a git repository into the +// file:// form that parseFileURL builds, and rejects other non-git sources. +func normalizeTrackSource(source *Source) error { + if source.Type == SourceTypeLocalPath && IsLocalGitRepo(source.Path) { + source.Type = SourceTypeGitHTTPS + source.CloneURL = localFileURL(source.Path) + return validateCloneURL(source.CloneURL) + } + if source.IsGit() { + return nil + } + if source.Type == SourceTypeLocalPath { + return fmt.Errorf("--track requires a git repository source; %s is not a git repository root (use file:///path for a repository URL)", source.Path) + } + return fmt.Errorf("--track requires a git repository source") +} + func installTrackedRepoImpl(source *Source, sourceDir string, opts InstallOptions) (*TrackedRepoResult, error) { - if !source.IsGit() { - return nil, fmt.Errorf("--track requires a git repository source") + if err := normalizeTrackSource(source); err != nil { + return nil, err } if err := resolveWebRef(source); err != nil { return nil, err @@ -63,6 +130,7 @@ func installTrackedRepoImpl(source *Source, sourceDir string, opts InstallOption } // Check if already exists + replacing := false if _, err := os.Stat(destPath); err == nil { if opts.Update { return updateTrackedRepo(destPath, result, opts) @@ -80,16 +148,17 @@ func installTrackedRepoImpl(source *Source, sourceDir string, opts InstallOption } return nil, fmt.Errorf("tracked repo '%s' already exists. To overwrite: %s", trackedName, hint) } + // A local source that is, or lies inside, the destination would be + // replaced under itself; a clone cannot carry its uncommitted work. + if p := localCloneSource(source); p != "" && pathWithin(destPath, p) { + return nil, fmt.Errorf("source %s is inside the install destination %s; nothing to install", p, destPath) + } // Force mode - remove existing. A link is refused, even in a dry // run: removing it would disconnect the repo it points to. if err := src.CheckNoLink(destRel); err != nil { return nil, err } - if !opts.DryRun { - if err := src.RemoveAll(destRel); err != nil { - return nil, fmt.Errorf("failed to remove existing repo: %w", err) - } - } + replacing = true } if opts.DryRun { @@ -104,28 +173,37 @@ func installTrackedRepoImpl(source *Source, sourceDir string, opts InstallOption if cloneBranch == "" { cloneBranch = source.Branch } + // Clone beside the destination and move it in only after every check + // passed, so a failure never costs the existing repo, and cleanup never + // removes a destination another installer created meanwhile. + stamp := strconv.FormatInt(time.Now().UnixNano(), 36) + cloneRel := filepath.Join(filepath.Dir(destRel), ".skillshare-clone-"+stamp) + clonePath := filepath.Join(sourceDir, cloneRel) + installed := false + defer func() { + if !installed { + _ = src.RemoveAll(cloneRel) + } + }() // A tag or commit SHA has no branch for `git pull` to follow. A SHA fails // at clone (--branch rejects it); a tag clones but leaves HEAD detached. - if err := cloneTrackedRepoForSource(source, destPath, cloneBranch, opts.OnProgress); err != nil { + if err := cloneTrackedRepoForSource(source, clonePath, cloneBranch, opts.OnProgress); err != nil { if IsCommitSHA(cloneBranch) { return nil, errTrackedNeedsBranch(cloneBranch) } return nil, fmt.Errorf("failed to clone repository: %w", err) } - if isDetachedHead(destPath) { - _ = src.RemoveAll(destRel) + if isDetachedHead(clonePath) { return nil, errTrackedNeedsBranch(cloneBranch) } if source.Commit != "" { - if err := resetTrackedToCommit(destPath, source.Commit, source.authEnv()); err != nil { - _ = src.RemoveAll(destRel) + if err := resetTrackedToCommit(clonePath, source.Commit, source.authEnv()); err != nil { return nil, err } } if source.HasSubdir() { - if err := submoduleError(destPath, source.Subdir, source.authEnv()); err != nil { - _ = src.RemoveAll(destRel) + if err := submoduleError(clonePath, source.Subdir, source.authEnv()); err != nil { return nil, err } } @@ -133,15 +211,18 @@ func installTrackedRepoImpl(source *Source, sourceDir string, opts InstallOption // Discover skills in the cloned repo. Include root SKILL.md so the count // matches what `skillshare sync` will see: every SKILL.md inside a tracked // repo (root and nested alike) becomes an independent skill on sync. - skills := discoverSkills(destPath, true) + skills := discoverSkills(clonePath, true) result.SkillCount = len(skills) for _, skill := range skills { + if skill.Path == "." { + skill.Name = trackedName // the root skill is named after its directory, not the staging one + } result.Skills = append(result.Skills, skill.Name) } - result.Warnings = append(result.Warnings, SubmoduleWarnings(destPath, source.authEnv())...) + result.Warnings = append(result.Warnings, SubmoduleWarnings(clonePath, source.authEnv())...) // Also discover agents in the tracked repo - agents := discoverAgents(destPath, len(skills) > 0) + agents := discoverAgents(clonePath, len(skills) > 0) result.AgentCount = len(agents) if len(agents) > 0 { for _, agent := range agents { @@ -156,11 +237,33 @@ func installTrackedRepoImpl(source *Source, sourceDir string, opts InstallOption // Only agents found — not a warning, just informational } - // Security audit on the entire tracked repo - if err := auditTrackedRepo(destPath, result, opts); err != nil { + // Security audit on the entire tracked repo. Accepted findings are keyed by + // the final path, not the staging one. + opts.AuditAcceptRoot, opts.AuditAcceptPath = opts.auditAcceptTarget(destPath) + if err := auditTrackedRepo(clonePath, result, opts); err != nil { return nil, err } + if replacing { + // Keep the old repo until the new one is in place, as swapStagedIntoSource does. + backup := filepath.Join(filepath.Dir(destRel), ".skillshare-"+filepath.Base(destRel)+".old."+stamp) + if err := src.Rename(destRel, backup); err != nil { + return nil, fmt.Errorf("failed to move the existing repo aside: %w", err) + } + if err := src.Rename(cloneRel, destRel); err != nil { + if restoreErr := src.Rename(backup, destRel); restoreErr != nil { + return nil, fmt.Errorf("failed to move the new clone into place: %w; the previous repo is left at %s: %v", err, backup, restoreErr) + } + return nil, fmt.Errorf("failed to move the new clone into place: %w", err) + } + if err := src.RemoveAll(backup); err != nil { + result.Warnings = append(result.Warnings, fmt.Sprintf("failed to remove the previous repo %s: %v", backup, err)) + } + } else if err := src.Rename(cloneRel, destRel); err != nil { + return nil, fmt.Errorf("failed to move the new clone into place: %w", err) + } + installed = true + // Auto-add to .gitignore to prevent committing tracked repo contents gitignoreEntry := trackedName if opts.Into != "" { diff --git a/internal/install/install_tracked_test.go b/internal/install/install_tracked_test.go index 9ab71c09d..83dee8c70 100644 --- a/internal/install/install_tracked_test.go +++ b/internal/install/install_tracked_test.go @@ -241,3 +241,241 @@ func TestMissingTrackedReposFollowPolicy(t *testing.T) { t.Fatalf("present followed checkout listed for install: %+v", got) } } + +func TestInferTrackedKind_LocalGitPathBecomesFileURL(t *testing.T) { + if _, err := exec.LookPath("git"); err != nil { + t.Skip("git not available") + } + repo := t.TempDir() + mustRunGit(t, "", "init", "-b", "main", repo) + if err := os.WriteFile(filepath.Join(repo, "SKILL.md"), []byte("# s"), 0644); err != nil { + t.Fatal(err) + } + + source, err := ParseSource(repo) + if err != nil { + t.Fatal(err) + } + kind, err := InferTrackedKind(source, "skill") + if err != nil { + t.Fatalf("local git repo should be trackable: %v", err) + } + if kind != "skill" || source.CloneURL != fileURL(repo) { + t.Errorf("kind=%q cloneURL=%q, want skill / %q", kind, source.CloneURL, fileURL(repo)) + } +} + +func TestInferTrackedKind_PlainLocalDirHintsFileURL(t *testing.T) { + source, err := ParseSource(t.TempDir()) + if err != nil { + t.Fatal(err) + } + _, err = InferTrackedKind(source, "skill") + if err == nil || !strings.Contains(err.Error(), "file://") { + t.Fatalf("want error naming file://, got %v", err) + } +} + +func TestLocalFileURL_KeepsWindowsDriveInPath(t *testing.T) { + // "C:/repo" is what a Windows path looks like after filepath.ToSlash. + if got := localFileURL("C:/repo"); got != "file:///C:/repo" { + t.Errorf("localFileURL(C:/repo) = %q, want file:///C:/repo", got) + } + if got := localFileURL("/tmp/repo"); got != "file:///tmp/repo" { + t.Errorf("localFileURL(/tmp/repo) = %q, want file:///tmp/repo", got) + } + if got := localFileURL("/tmp/my repo%20x#1"); got != "file:///tmp/my%20repo%2520x%231" { + t.Errorf("localFileURL escaping = %q, want file:///tmp/my%%20repo%%2520x%%231", got) + } +} + +func TestParseSource_ExplicitUNCFileURLKeepsAuthority(t *testing.T) { + source, err := ParseSource("file://server/share/repo") + if err != nil { + t.Fatal(err) + } + if source.CloneURL != "file://server/share/repo" { + t.Errorf("CloneURL = %q, want file://server/share/repo", source.CloneURL) + } +} + +func TestInferTrackedKind_LocalBareRepoPath(t *testing.T) { + bare := strings.TrimPrefix(makeRemote(t, ""), "file://") + // A Windows file URL is file:///C:/dir; drop the slash before the drive. + if len(bare) > 2 && bare[0] == '/' && bare[2] == ':' { + bare = bare[1:] + } + + source, err := ParseSource(bare) + if err != nil { + t.Fatal(err) + } + if _, err := InferTrackedKind(source, "skill"); err != nil { + t.Fatalf("local bare repo should be trackable: %v", err) + } +} + +func TestInstallTrackedRepo_ForceNeverDestroysItsLocalSource(t *testing.T) { + cases := []struct { + name string + repo string // relative to the destination + source func(repo string) string + dryRun bool + }{ + {"same path", "", func(p string) string { return p }, false}, + {"nested path", "nested", func(p string) string { return p }, false}, + {"dry run", "nested", func(p string) string { return p }, true}, + {"explicit file URL", "nested", func(p string) string { return fileURL(p) }, false}, + {"localhost file URL", "nested", func(p string) string { return "file://localhost/" + strings.TrimPrefix(filepath.ToSlash(p), "/") }, false}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + sourceDir := t.TempDir() + dest := filepath.Join(sourceDir, "_foo") + repo := filepath.Join(dest, tc.repo) + mustRunGit(t, "", "init", "-b", "main", repo) + if err := os.WriteFile(filepath.Join(repo, "SKILL.md"), []byte("# skill"), 0644); err != nil { + t.Fatal(err) + } + mustRunGit(t, repo, "add", ".") + mustRunGit(t, repo, "-c", "user.email=a@b", "-c", "user.name=n", "commit", "-m", "i") + // Uncommitted work: a clone cannot carry it, so only refusing keeps it. + wip := filepath.Join(repo, "wip.txt") + if err := os.WriteFile(wip, []byte("unpushed"), 0644); err != nil { + t.Fatal(err) + } + + source, err := ParseSource(tc.source(repo)) + if err != nil { + t.Fatal(err) + } + _, err = InstallTrackedRepo(source, sourceDir, InstallOptions{Name: "foo", Force: true, DryRun: tc.dryRun}) + if err == nil { + t.Fatal("expected an error when the source is inside the install destination") + } + if _, err := os.Stat(wip); err != nil { + t.Fatalf("source must be left intact: %v", err) + } + }) + } +} + +func TestLocalCloneSource_UNCFileURL(t *testing.T) { + got := localCloneSource(&Source{CloneURL: "file://server/share/repo"}) + if want := filepath.FromSlash("//server/share/repo"); got != want { + t.Errorf("localCloneSource = %q, want %q", got, want) + } +} + +func TestInstallTrackedRepo_ForceKeepsExistingRepoWhenCloneFails(t *testing.T) { + remoteURL := makeRemote(t, "") + sourceDir := t.TempDir() + source := &Source{Type: SourceTypeGitHTTPS, Raw: remoteURL, CloneURL: remoteURL} + if _, err := InstallTrackedRepo(source, sourceDir, InstallOptions{Name: "foo"}); err != nil { + t.Fatalf("first install: %v", err) + } + + _, err := InstallTrackedRepo(source, sourceDir, InstallOptions{Name: "foo", Force: true, Branch: "no-such-branch"}) + if err == nil { + t.Fatal("expected the clone of a missing branch to fail") + } + if _, statErr := os.Stat(filepath.Join(sourceDir, "_foo", "SKILL.md")); statErr != nil { + t.Fatalf("the existing repo must survive a failed forced reinstall: %v", statErr) + } + entries, _ := os.ReadDir(sourceDir) + for _, e := range entries { + if strings.HasPrefix(e.Name(), ".skillshare-clone-") { + t.Errorf("staging dir %s was left behind", e.Name()) + } + } +} + +func TestInstallTrackedRepo_ForceReplacesExistingRepoInto(t *testing.T) { + remoteURL := makeRemote(t, "") + sourceDir := t.TempDir() + source := &Source{Type: SourceTypeGitHTTPS, Raw: remoteURL, CloneURL: remoteURL} + opts := InstallOptions{Name: "foo", Into: "team"} + if _, err := InstallTrackedRepo(source, sourceDir, opts); err != nil { + t.Fatalf("first install: %v", err) + } + stale := filepath.Join(sourceDir, "team", "_foo", "stale.txt") + if err := os.WriteFile(stale, []byte("x"), 0644); err != nil { + t.Fatal(err) + } + + opts.Force = true + if _, err := InstallTrackedRepo(source, sourceDir, opts); err != nil { + t.Fatalf("forced reinstall: %v", err) + } + if _, err := os.Stat(stale); !os.IsNotExist(err) { + t.Errorf("the previous checkout should be replaced, stat err = %v", err) + } + if _, err := os.Stat(filepath.Join(sourceDir, "team", "_foo", "SKILL.md")); err != nil { + t.Errorf("the new clone should be in place: %v", err) + } + entries, _ := os.ReadDir(filepath.Join(sourceDir, "team")) + for _, e := range entries { + if strings.HasPrefix(e.Name(), ".skillshare-") { + t.Errorf("leftover %s after a forced reinstall", e.Name()) + } + } +} + +func TestInstallTrackedRepo_UpdateAcceptsSourceThatIsTheDestination(t *testing.T) { + remoteURL := makeRemote(t, "") + sourceDir := t.TempDir() + remote := &Source{Type: SourceTypeGitHTTPS, Raw: remoteURL, CloneURL: remoteURL} + if _, err := InstallTrackedRepo(remote, sourceDir, InstallOptions{Name: "foo"}); err != nil { + t.Fatalf("first install: %v", err) + } + + source, err := ParseSource(filepath.Join(sourceDir, "_foo")) + if err != nil { + t.Fatal(err) + } + result, err := InstallTrackedRepo(source, sourceDir, InstallOptions{Name: "foo", Update: true}) + if err != nil { + t.Fatalf("--update pulls in place and must not be refused: %v", err) + } + if result.Action != "updated" { + t.Errorf("Action = %q, want updated", result.Action) + } +} + +func TestInstallTrackedRepo_ForceReportsRootSkillUnderFinalName(t *testing.T) { + remoteURL := makeRemote(t, "") + sourceDir := t.TempDir() + source := &Source{Type: SourceTypeGitHTTPS, Raw: remoteURL, CloneURL: remoteURL} + if _, err := InstallTrackedRepo(source, sourceDir, InstallOptions{Name: "foo"}); err != nil { + t.Fatalf("first install: %v", err) + } + + result, err := InstallTrackedRepo(source, sourceDir, InstallOptions{Name: "foo", Force: true}) + if err != nil { + t.Fatalf("forced reinstall: %v", err) + } + if len(result.Skills) != 1 || result.Skills[0] != "_foo" { + t.Errorf("Skills = %v, want [_foo]", result.Skills) + } +} + +func TestInstallTrackedRepo_KeepsDestinationClaimedByAnotherInstaller(t *testing.T) { + remoteURL := makeRemote(t, "") + sourceDir := t.TempDir() + source := &Source{Type: SourceTypeGitHTTPS, Raw: remoteURL, CloneURL: remoteURL} + theirs := filepath.Join(sourceDir, "_foo", "theirs.txt") + claim := func(string) { + // Another process creates the destination while this clone runs. + if err := os.MkdirAll(filepath.Dir(theirs), 0755); err == nil { + _ = os.WriteFile(theirs, []byte("x"), 0644) + } + } + + _, err := InstallTrackedRepo(source, sourceDir, InstallOptions{Name: "foo", OnProgress: claim}) + if err == nil { + t.Fatal("expected the install to fail when the destination was claimed meanwhile") + } + if _, statErr := os.Stat(theirs); statErr != nil { + t.Fatalf("the other installer's checkout must survive: %v", statErr) + } +} diff --git a/internal/server/handler_collect.go b/internal/server/handler_collect.go index d587adaf0..810841894 100644 --- a/internal/server/handler_collect.go +++ b/internal/server/handler_collect.go @@ -220,6 +220,10 @@ func (s *Server) handleCollect(w http.ResponseWriter, r *http.Request) { writeError(w, http.StatusBadRequest, "skill is not a directory: "+ref.Name) return } + if !ssync.HasSkillFile(skillPath) { + writeError(w, http.StatusBadRequest, "not a skill (no SKILL.md): "+ref.Name) + return + } resolved = append(resolved, ssync.LocalSkillInfo{ Name: ref.Name, diff --git a/internal/server/handler_collect_test.go b/internal/server/handler_collect_test.go index a9143d4a6..7b5c18e41 100644 --- a/internal/server/handler_collect_test.go +++ b/internal/server/handler_collect_test.go @@ -402,3 +402,24 @@ func TestHandleCollectScan_AgentKind_NoSource(t *testing.T) { t.Fatalf("expected totalCount=0 when no agents source, got %d", resp.TotalCount) } } + +func TestHandleCollect_RejectsFolderWithoutSkillMd(t *testing.T) { + home := filepath.Join(t.TempDir(), "home") + if err := os.MkdirAll(home, 0o755); err != nil { + t.Fatalf("mkdir home: %v", err) + } + t.Setenv("HOME", home) + + tgtPath := filepath.Join(t.TempDir(), "claude-skills") + s, _ := newTestServerWithTargets(t, map[string]string{"claude": tgtPath}) + os.MkdirAll(filepath.Join(tgtPath, "scratch-dir", "SKILL.md"), 0755) // a directory, not a file + + body := `{"skills":[{"name":"scratch-dir","targetName":"claude"}],"force":true}` + req := httptest.NewRequest(http.MethodPost, "/api/collect", strings.NewReader(body)) + rr := httptest.NewRecorder() + s.handler.ServeHTTP(rr, req) + + if rr.Code != http.StatusBadRequest { + t.Fatalf("expected 400, got %d: %s", rr.Code, rr.Body.String()) + } +} diff --git a/internal/server/handler_install_test.go b/internal/server/handler_install_test.go index 4601380d7..bb50b0247 100644 --- a/internal/server/handler_install_test.go +++ b/internal/server/handler_install_test.go @@ -347,3 +347,38 @@ func TestReloadSkillsStore_PicksUpNewEntry(t *testing.T) { t.Errorf("Type = %q, want github-subdir", got.Type) } } + +func TestHandleInstall_TrackAcceptsLocalGitPath(t *testing.T) { + s, skillsDir := newTestServer(t) + + repoDir := t.TempDir() + initGitRepo(t, repoDir) + if err := os.WriteFile(filepath.Join(repoDir, "SKILL.md"), []byte("---\nname: local-tracked\n---\n# s"), 0o644); err != nil { + t.Fatal(err) + } + for _, args := range [][]string{{"add", "SKILL.md"}, {"commit", "-m", "add skill"}} { + cmd := exec.Command("git", args...) + cmd.Dir = repoDir + if out, err := cmd.CombinedOutput(); err != nil { + t.Fatalf("git %v failed: %s %v", args, out, err) + } + } + + payload, _ := json.Marshal(map[string]any{"source": repoDir, "track": true, "skipAudit": true}) + req := httptest.NewRequest(http.MethodPost, "/api/install", bytes.NewReader(payload)) + rr := httptest.NewRecorder() + s.mux.ServeHTTP(rr, req) + + if rr.Code != http.StatusOK { + t.Fatalf("unexpected status: got %d, body=%s", rr.Code, rr.Body.String()) + } + var resp struct { + RepoName string `json:"repoName"` + } + if err := json.Unmarshal(rr.Body.Bytes(), &resp); err != nil { + t.Fatalf("failed to decode response: %v", err) + } + if _, err := os.Stat(filepath.Join(skillsDir, resp.RepoName, "SKILL.md")); err != nil { + t.Fatalf("expected tracked repo %q in skills source: %v", resp.RepoName, err) + } +} diff --git a/internal/sync/pull.go b/internal/sync/pull.go index 6e88de0fd..9d3ddbc45 100644 --- a/internal/sync/pull.go +++ b/internal/sync/pull.go @@ -42,6 +42,13 @@ type PullResult struct { Failed map[string]error } +// HasSkillFile reports whether dir holds a regular SKILL.md file; a folder +// without one (a scratch dir) is not a skill, and a FIFO would block a copy. +func HasSkillFile(dir string) bool { + info, err := os.Stat(filepath.Join(dir, "SKILL.md")) + return err == nil && info.Mode().IsRegular() +} + // FindLocalSkills finds all local (non-symlinked) skills in a target directory. // syncMode should be the target's current sync mode ("merge", "copy", or "symlink"). // In copy mode, skills listed in the manifest are considered managed and skipped. @@ -108,6 +115,10 @@ func FindLocalSkills(targetPath, sourcePath, syncMode string) ([]LocalSkillInfo, continue } + if !HasSkillFile(skillPath) { + continue + } + // Skip copy-mode managed skills. // Only relevant when the target is still in copy mode; after switching // to merge the old manifest entries are stale (the physical copies are diff --git a/internal/sync/pull_fifo_unix_test.go b/internal/sync/pull_fifo_unix_test.go new file mode 100644 index 000000000..1fd28c5b5 --- /dev/null +++ b/internal/sync/pull_fifo_unix_test.go @@ -0,0 +1,20 @@ +//go:build !windows + +package sync + +import ( + "path/filepath" + "syscall" + "testing" +) + +// A FIFO named SKILL.md would block collect when it is read. +func TestHasSkillFile_RejectsFifoSkillMd(t *testing.T) { + dir := t.TempDir() + if err := syscall.Mkfifo(filepath.Join(dir, "SKILL.md"), 0644); err != nil { + t.Skipf("mkfifo unavailable: %v", err) + } + if HasSkillFile(dir) { + t.Error("a FIFO named SKILL.md is not a skill file") + } +} diff --git a/internal/sync/pull_test.go b/internal/sync/pull_test.go index 700bd47f9..a9cdccfc4 100644 --- a/internal/sync/pull_test.go +++ b/internal/sync/pull_test.go @@ -83,6 +83,7 @@ func TestFindLocalSkills_SkipsCopyManaged(t *testing.T) { // Create a copy-mode managed skill managedSkill := filepath.Join(tgt, "managed") os.MkdirAll(managedSkill, 0755) + os.WriteFile(filepath.Join(managedSkill, "SKILL.md"), []byte("managed"), 0644) // Write manifest marking it as managed m := &Manifest{Managed: map[string]string{"managed": "abc123"}} @@ -93,6 +94,7 @@ func TestFindLocalSkills_SkipsCopyManaged(t *testing.T) { // Also create a truly local skill localSkill := filepath.Join(tgt, "local-only") os.MkdirAll(localSkill, 0755) + os.WriteFile(filepath.Join(localSkill, "SKILL.md"), []byte("local skill"), 0644) skills, err := FindLocalSkills(tgt, src, "copy") if err != nil { @@ -147,6 +149,7 @@ func TestFindLocalSkills_EmptyModePassedDirectly(t *testing.T) { // Physical dir with copy-mode manifest os.MkdirAll(filepath.Join(tgt, "skill-a"), 0755) + os.WriteFile(filepath.Join(tgt, "skill-a", "SKILL.md"), []byte("a"), 0644) m := &Manifest{Managed: map[string]string{"skill-a": "abc123"}} if err := WriteManifest(tgt, m); err != nil { t.Fatal(err) @@ -349,3 +352,32 @@ func TestPullSkill_ForceOverwrite(t *testing.T) { t.Errorf("expected 'new' content after force pull, got %q", string(data)) } } + +func TestFindLocalSkills_SkipsDirWithoutSkillMd(t *testing.T) { + tmp := t.TempDir() + src := filepath.Join(tmp, "source") + tgt := filepath.Join(tmp, "target") + os.MkdirAll(src, 0755) + os.MkdirAll(filepath.Join(tgt, "scratch-dir"), 0755) + os.MkdirAll(filepath.Join(tgt, "dir-named-skill-md", "SKILL.md"), 0755) + os.MkdirAll(filepath.Join(tgt, "my-local"), 0755) + os.WriteFile(filepath.Join(tgt, "my-local", "SKILL.md"), []byte("local skill"), 0644) + + skills, err := FindLocalSkills(tgt, src, "merge") + if err != nil { + t.Fatal(err) + } + if len(skills) != 1 || skills[0].Name != "my-local" { + t.Fatalf("expected only my-local, got %+v", skills) + } +} + +func TestHasSkillFile_RejectsDirectoryNamedSkillMd(t *testing.T) { + dir := t.TempDir() + if err := os.Mkdir(filepath.Join(dir, "SKILL.md"), 0755); err != nil { + t.Fatal(err) + } + if HasSkillFile(dir) { + t.Error("a directory named SKILL.md is not a skill file") + } +} diff --git a/skills/skillshare/references/install.md b/skills/skillshare/references/install.md index 2d5ff987e..633404d47 100644 --- a/skills/skillshare/references/install.md +++ b/skills/skillshare/references/install.md @@ -70,7 +70,7 @@ skillshare install user/repo --skip-audit # Skip security scan | `--branch, -b ` | Select a Git branch, tag or commit SHA (overrides a ref in a web URL) | | `--force, -f` | Overwrite existing and explicitly override audit blocking | | `--update, -u` | Update if exists | -| `--track, -t` | Track for updates (preserves .git) | +| `--track, -t` | Track for updates (preserves .git); a local path works if it is a git repository | | `--skill, -s ` | Select specific skills from multi-skill repo (comma-separated) | | `--into ` | Install into subdirectory (e.g., `--into frontend`) | | `--all` | Install all discovered skills without prompting | diff --git a/tests/integration/install_track_root_skill_test.go b/tests/integration/install_track_root_skill_test.go index 3f133cfdc..2dcd93cc5 100644 --- a/tests/integration/install_track_root_skill_test.go +++ b/tests/integration/install_track_root_skill_test.go @@ -6,6 +6,7 @@ import ( "encoding/json" "os" "path/filepath" + "strings" "testing" "skillshare/internal/install" @@ -349,3 +350,62 @@ func TestInstall_Track_EmptyRepo_NextSteps(t *testing.T) { result.AssertOutputNotContains(t, "Run 'skillshare sync'") } + +// TestInstall_Track_LocalGitPath verifies --track accepts a local path that is +// a git repository (also with --branch), the same as the file:// form. +func TestInstall_Track_LocalGitPath(t *testing.T) { + sb := testutil.NewSandbox(t) + defer sb.Cleanup() + setupGlobalConfig(sb) + + repo := filepath.Join(sb.Root, "local repo%20x") + run(t, "", "git", "init", "--initial-branch=main", repo) + os.WriteFile(filepath.Join(repo, "SKILL.md"), []byte("---\nname: local-repo\n---\n# s\n"), 0644) + run(t, repo, "git", "add", "-A") + run(t, repo, "git", "commit", "-m", "initial") + + result := sb.RunCLI("install", repo, "--track", "--name", "local-repo", "--branch", "main", "--skip-audit") + result.AssertSuccess(t) + + if !sb.FileExists(filepath.Join(sb.SourcePath, "_local-repo", "SKILL.md")) { + t.Fatalf("tracked repo should be cloned to _local-repo") + } +} + +// TestInstall_Track_LocalBareRepoPath verifies --track also takes a local bare +// repository path, as file:// does. +func TestInstall_Track_LocalBareRepoPath(t *testing.T) { + sb := testutil.NewSandbox(t) + defer sb.Cleanup() + setupGlobalConfig(sb) + + bare := strings.TrimPrefix(setupBareRepoWithRootSkill(t, sb, "bare-local"), "file://") + + result := sb.RunCLI("install", bare, "--track", "--name", "bare-local", "--skip-audit") + result.AssertSuccess(t) + + if !sb.FileExists(filepath.Join(sb.SourcePath, "_bare-local", "SKILL.md")) { + t.Fatalf("tracked repo should be cloned to _bare-local") + } +} + +// TestInstall_Track_Force_KeepsLocalSourceInsideDestination verifies that +// --force never removes the repository it is meant to clone from. +func TestInstall_Track_Force_KeepsLocalSourceInsideDestination(t *testing.T) { + sb := testutil.NewSandbox(t) + defer sb.Cleanup() + setupGlobalConfig(sb) + + dest := filepath.Join(sb.SourcePath, "_foo") + for _, repo := range []string{dest, filepath.Join(dest, "nested")} { + run(t, "", "git", "init", "--initial-branch=main", repo) + os.WriteFile(filepath.Join(repo, "SKILL.md"), []byte("---\nname: foo\n---\n# unpushed\n"), 0644) + + result := sb.RunCLI("install", repo, "--track", "--name", "foo", "--force", "--skip-audit") + result.AssertFailure(t) + result.AssertAnyOutputContains(t, "inside the install destination") + if !sb.FileExists(filepath.Join(repo, "SKILL.md")) { + t.Fatalf("source %s must be left intact", repo) + } + } +} diff --git a/website/docs/reference/commands/collect.md b/website/docs/reference/commands/collect.md index 2edcee98c..8b2fbdf53 100644 --- a/website/docs/reference/commands/collect.md +++ b/website/docs/reference/commands/collect.md @@ -26,6 +26,8 @@ Examples: - Skills: `~/.claude/skills/my-skill/` - Agents: `~/.claude/agents/tutor.md` +A skill folder must contain a `SKILL.md`; other folders in a target (such as scratch directories) are not collected. + ## What Happens ```mermaid diff --git a/website/docs/reference/commands/install.md b/website/docs/reference/commands/install.md index d09963929..59d30906e 100644 --- a/website/docs/reference/commands/install.md +++ b/website/docs/reference/commands/install.md @@ -413,6 +413,7 @@ skillshare install google-gemini/gemini-cli/.../skill-creator --name my-creator `--name` only works when install resolves to a single skill. In `--track` mode, custom names are stored as tracked repo directories (auto-prefixed with `_`) and must not contain path separators or `..`. +`--track` accepts a local path when that path is the root of a git repository (it is cloned like `file:///path`); for any other local folder, install it without `--track`. ```bash # ✅ Single skill (works) diff --git a/website/i18n/ja/docusaurus-plugin-content-docs/current/reference/commands/collect.md b/website/i18n/ja/docusaurus-plugin-content-docs/current/reference/commands/collect.md index 1569ce57c..9bd23a443 100644 --- a/website/i18n/ja/docusaurus-plugin-content-docs/current/reference/commands/collect.md +++ b/website/i18n/ja/docusaurus-plugin-content-docs/current/reference/commands/collect.md @@ -26,6 +26,8 @@ skillshare collect agents claude # Skill の代わりに agent を収集 - Skill: `~/.claude/skills/my-skill/` - Agent: `~/.claude/agents/tutor.md` +スキルのフォルダには `SKILL.md` が必要です。ターゲット内のそれ以外のフォルダ(作業用ディレクトリなど)は collect されません。 + ## 何が起きるか ```mermaid diff --git a/website/i18n/ja/docusaurus-plugin-content-docs/current/reference/commands/install.md b/website/i18n/ja/docusaurus-plugin-content-docs/current/reference/commands/install.md index 32b226392..32cb5f58a 100644 --- a/website/i18n/ja/docusaurus-plugin-content-docs/current/reference/commands/install.md +++ b/website/i18n/ja/docusaurus-plugin-content-docs/current/reference/commands/install.md @@ -409,6 +409,7 @@ skillshare install google-gemini/gemini-cli/.../skill-creator --name my-creator `--name` は、インストールが単一の Skill に解決される場合にのみ機能します。 `--track` モードでは、カスタム名はトラック対象リポジトリのディレクトリ名として保存され(自動的に `_` がプレフィックスされます)、パスセパレータや `..` を含んではいけません。 +`--track` は、パスが git リポジトリのルートであればローカルパスも受け付けます(`file:///path` と同様に clone されます)。それ以外のローカルフォルダは `--track` なしでインストールしてください。 ```bash # ✅ 単一 Skill(動作する) diff --git a/website/i18n/ko/docusaurus-plugin-content-docs/current/reference/commands/collect.md b/website/i18n/ko/docusaurus-plugin-content-docs/current/reference/commands/collect.md index 558d5ea52..c5841994d 100644 --- a/website/i18n/ko/docusaurus-plugin-content-docs/current/reference/commands/collect.md +++ b/website/i18n/ko/docusaurus-plugin-content-docs/current/reference/commands/collect.md @@ -26,6 +26,8 @@ target 디렉터리에서 직접 리소스를 생성하거나 수정했고, 이 - Skill: `~/.claude/skills/my-skill/` - Agent: `~/.claude/agents/tutor.md` +스킬 폴더에는 `SKILL.md`가 있어야 합니다. 타깃의 다른 폴더(임시 작업 디렉터리 등)는 collect되지 않습니다. + ## 동작 방식 ```mermaid diff --git a/website/i18n/ko/docusaurus-plugin-content-docs/current/reference/commands/install.md b/website/i18n/ko/docusaurus-plugin-content-docs/current/reference/commands/install.md index fdcdaf66a..db4ac75a8 100644 --- a/website/i18n/ko/docusaurus-plugin-content-docs/current/reference/commands/install.md +++ b/website/i18n/ko/docusaurus-plugin-content-docs/current/reference/commands/install.md @@ -409,6 +409,7 @@ skillshare install google-gemini/gemini-cli/.../skill-creator --name my-creator `--name`은 install이 단일 skill로 해석될 때만 동작합니다. `--track` 모드에서는 커스텀 이름이 tracked repo 디렉터리로 저장되며(자동으로 `_`가 접두사로 붙음), path separator나 `..`를 포함할 수 없습니다. +`--track`은 경로가 git 저장소의 루트이면 로컬 경로도 받습니다(`file:///path`처럼 clone됩니다). 그 외의 로컬 폴더는 `--track` 없이 설치하세요. ```bash # ✅ 단일 skill (동작함) diff --git a/website/i18n/zh-Hans/docusaurus-plugin-content-docs/current/reference/commands/collect.md b/website/i18n/zh-Hans/docusaurus-plugin-content-docs/current/reference/commands/collect.md index 2006173dc..e5f3eda9f 100644 --- a/website/i18n/zh-Hans/docusaurus-plugin-content-docs/current/reference/commands/collect.md +++ b/website/i18n/zh-Hans/docusaurus-plugin-content-docs/current/reference/commands/collect.md @@ -26,6 +26,8 @@ skillshare collect agents claude # 收集 agents 而不是 skills - Skills:`~/.claude/skills/my-skill/` - Agents:`~/.claude/agents/tutor.md` +技能文件夹必须包含 `SKILL.md`;目标中的其他文件夹(如临时目录)不会被 collect。 + ## 会发生什么 ```mermaid diff --git a/website/i18n/zh-Hans/docusaurus-plugin-content-docs/current/reference/commands/install.md b/website/i18n/zh-Hans/docusaurus-plugin-content-docs/current/reference/commands/install.md index e857364e1..82c2bf7f3 100644 --- a/website/i18n/zh-Hans/docusaurus-plugin-content-docs/current/reference/commands/install.md +++ b/website/i18n/zh-Hans/docusaurus-plugin-content-docs/current/reference/commands/install.md @@ -409,6 +409,7 @@ skillshare install google-gemini/gemini-cli/.../skill-creator --name my-creator `--name` 仅在安装解析为单个 Skill 时有效。 在 `--track` 模式下,自定义名称会作为 tracked repo 目录存储(自动加上 `_` 前缀),且不能包含路径分隔符或 `..`。 +`--track` 也接受本地路径,前提是该路径是 git 仓库的根目录(会像 `file:///path` 一样被 clone);其他本地文件夹请不要加 `--track` 安装。 ```bash # ✅ 单个 Skill(可行) diff --git a/website/i18n/zh-Hant/docusaurus-plugin-content-docs/current/reference/commands/collect.md b/website/i18n/zh-Hant/docusaurus-plugin-content-docs/current/reference/commands/collect.md index 3fcb46225..c1315d485 100644 --- a/website/i18n/zh-Hant/docusaurus-plugin-content-docs/current/reference/commands/collect.md +++ b/website/i18n/zh-Hant/docusaurus-plugin-content-docs/current/reference/commands/collect.md @@ -26,6 +26,8 @@ skillshare collect agents claude # Collect agents instead of skills - Skills: `~/.claude/skills/my-skill/` - Agents: `~/.claude/agents/tutor.md` +技能資料夾必須包含 `SKILL.md`;目標中的其他資料夾(例如暫存目錄)不會被 collect。 + ## 執行流程 ```mermaid diff --git a/website/i18n/zh-Hant/docusaurus-plugin-content-docs/current/reference/commands/install.md b/website/i18n/zh-Hant/docusaurus-plugin-content-docs/current/reference/commands/install.md index fb121381b..7648295a6 100644 --- a/website/i18n/zh-Hant/docusaurus-plugin-content-docs/current/reference/commands/install.md +++ b/website/i18n/zh-Hant/docusaurus-plugin-content-docs/current/reference/commands/install.md @@ -409,6 +409,7 @@ skillshare install google-gemini/gemini-cli/.../skill-creator --name my-creator `--name` 只在 install 解析出單一 skill 時才有效。 在 `--track` 模式下,自訂名稱會以 tracked repo 目錄儲存(自動加上 `_` 前綴),且不得包含路徑分隔符或 `..`。 +`--track` 也接受本機路徑,前提是該路徑是 git repository 的根目錄(會像 `file:///path` 一樣被 clone);其他本機資料夾請不要加 `--track` 安裝。 ```bash # ✅ 單一 skill(可行)