From e31309fe7c0dca0932071a722ccc5967d11d162e Mon Sep 17 00:00:00 2001 From: Willie Date: Fri, 9 Oct 2026 23:31:15 +0800 Subject: [PATCH 01/15] fix(collect,install): collect only real skills; let --track take a local git path collect treated every directory in a target as a local skill, so a scratch folder was listed and `collect --force` copied it into the source. A folder now counts only when it holds SKILL.md; the dashboard scan shares FindLocalSkills and the dashboard collect request rejects such a folder. `install /path/to/repo --track` failed with "requires a git repository source" while file:///path worked. A local path that is a git repository root is now normalized to the same file:// clone URL for --track (also past the --branch guard); other local folders get an error that names file://. Help text, docs (English and i18n) and the built-in skill reference follow. --- cmd/skillshare/install.go | 6 ++-- internal/install/install.go | 4 +-- internal/install/install_tracked.go | 21 ++++++++++-- internal/install/install_tracked_test.go | 34 +++++++++++++++++++ internal/install/source.go | 7 +++- internal/server/handler_collect.go | 4 +++ internal/server/handler_collect_test.go | 21 ++++++++++++ internal/sync/pull.go | 5 +++ internal/sync/pull_test.go | 21 ++++++++++++ skills/skillshare/references/install.md | 2 +- .../install_track_root_skill_test.go | 21 ++++++++++++ website/docs/reference/commands/collect.md | 2 ++ website/docs/reference/commands/install.md | 1 + .../current/reference/commands/collect.md | 2 ++ .../current/reference/commands/install.md | 1 + .../current/reference/commands/collect.md | 2 ++ .../current/reference/commands/install.md | 1 + .../current/reference/commands/collect.md | 2 ++ .../current/reference/commands/install.md | 1 + .../current/reference/commands/collect.md | 2 ++ .../current/reference/commands/install.md | 1 + 21 files changed, 153 insertions(+), 8 deletions(-) diff --git a/cmd/skillshare/install.go b/cmd/skillshare/install.go index 68045c0dc..8b8cf866f 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.IsGitRepo(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_tracked.go b/internal/install/install_tracked.go index 1ffc956d9..8ce7a46f8 100644 --- a/internal/install/install_tracked.go +++ b/internal/install/install_tracked.go @@ -9,9 +9,26 @@ import ( "skillshare/internal/sourcefs" ) +// 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 && IsGitRepo(source.Path) { + source.Type = SourceTypeGitHTTPS + source.CloneURL = fileCloneURL(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 diff --git a/internal/install/install_tracked_test.go b/internal/install/install_tracked_test.go index 9ab71c09d..99f2ecaf6 100644 --- a/internal/install/install_tracked_test.go +++ b/internal/install/install_tracked_test.go @@ -241,3 +241,37 @@ 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) + } +} diff --git a/internal/install/source.go b/internal/install/source.go index 309dd2393..6b2f15d3d 100644 --- a/internal/install/source.go +++ b/internal/install/source.go @@ -550,6 +550,11 @@ func parseSSHURL(matches []string, source *Source) (*Source, error) { return source, nil } +// fileCloneURL builds the file:// URL git clones a local repository from. +func fileCloneURL(path string) string { + return "file://" + filepath.ToSlash(path) +} + func parseFileURL(matches []string, source *Source) (*Source, error) { // matches: [full, path, subdir] path := filepath.Clean(matches[1]) @@ -562,7 +567,7 @@ func parseFileURL(matches []string, source *Source) (*Source, error) { // filepath.Clean rewrites the separators for the local OS, but a file:// URL // keeps forward slashes everywhere; on Windows the raw path would produce // `file://\path\to\repo`, which git does not accept as a local repository. - source.CloneURL = "file://" + filepath.ToSlash(path) + source.CloneURL = fileCloneURL(path) if err := validateCloneURL(source.CloneURL); err != nil { return nil, err diff --git a/internal/server/handler_collect.go b/internal/server/handler_collect.go index d587adaf0..2161b4e5c 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 _, err := os.Stat(filepath.Join(skillPath, "SKILL.md")); err != nil { + 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..ecf7ebbd3 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"), 0755) + + 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/sync/pull.go b/internal/sync/pull.go index 6e88de0fd..e01505171 100644 --- a/internal/sync/pull.go +++ b/internal/sync/pull.go @@ -108,6 +108,11 @@ func FindLocalSkills(targetPath, sourcePath, syncMode string) ([]LocalSkillInfo, continue } + // A folder without SKILL.md (a scratch dir) is not a skill. + if _, err := os.Stat(filepath.Join(skillPath, "SKILL.md")); err != nil { + 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_test.go b/internal/sync/pull_test.go index 700bd47f9..0a53429a8 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,21 @@ 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, "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) + } +} 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..65bf5057c 100644 --- a/tests/integration/install_track_root_skill_test.go +++ b/tests/integration/install_track_root_skill_test.go @@ -349,3 +349,24 @@ 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") + 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", "--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") + } +} 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（可行） From aa9670fb0751a7ad9b8f7594bfb03debfc71958f Mon Sep 17 00:00:00 2001 From: Willie Date: Fri, 9 Oct 2026 23:42:24 +0800 Subject: [PATCH 02/15] fix(install): keep the drive letter in file:// clone URLs for local --track paths A Windows path such as C:/repo produced file://C:/repo, which makes C: the URI authority instead of part of the path, so a tracked install from a local Windows repository could clone the wrong location. Prefix the missing slash so the URL is file:///C:/repo; POSIX paths are unchanged. Also cover the dashboard install endpoint with a local-path --track test. --- internal/install/install_tracked_test.go | 10 +++++++ internal/install/source.go | 7 ++++- internal/server/handler_install_test.go | 35 ++++++++++++++++++++++++ 3 files changed, 51 insertions(+), 1 deletion(-) diff --git a/internal/install/install_tracked_test.go b/internal/install/install_tracked_test.go index 99f2ecaf6..73ae69fde 100644 --- a/internal/install/install_tracked_test.go +++ b/internal/install/install_tracked_test.go @@ -275,3 +275,13 @@ func TestInferTrackedKind_PlainLocalDirHintsFileURL(t *testing.T) { t.Fatalf("want error naming file://, got %v", err) } } + +func TestFileCloneURL_KeepsWindowsDriveInPath(t *testing.T) { + // Linux ToSlash leaves "/" alone, so this is the form a Windows path takes after ToSlash. + if got := fileCloneURL("C:/repo"); got != "file:///C:/repo" { + t.Errorf("fileCloneURL(C:/repo) = %q, want file:///C:/repo", got) + } + if got := fileCloneURL("/tmp/repo"); got != "file:///tmp/repo" { + t.Errorf("fileCloneURL(/tmp/repo) = %q, want file:///tmp/repo", got) + } +} diff --git a/internal/install/source.go b/internal/install/source.go index 6b2f15d3d..55ecc599a 100644 --- a/internal/install/source.go +++ b/internal/install/source.go @@ -552,7 +552,12 @@ func parseSSHURL(matches []string, source *Source) (*Source, error) { // fileCloneURL builds the file:// URL git clones a local repository from. func fileCloneURL(path string) string { - return "file://" + filepath.ToSlash(path) + p := filepath.ToSlash(path) + // A Windows drive path (C:/repo) needs the empty authority: file:///C:/repo. + if !strings.HasPrefix(p, "/") { + p = "/" + p + } + return "file://" + p } func parseFileURL(matches []string, source *Source) (*Source, error) { 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) + } +} From eb72272b37a3c7d7d761ebc988576f99cf81472b Mon Sep 17 00:00:00 2001 From: Willie Date: Fri, 9 Oct 2026 23:47:32 +0800 Subject: [PATCH 03/15] fix(collect,install): keep UNC file URLs intact; require SKILL.md to be a file The drive-letter fix lived in the helper shared with explicit file:// URLs, which rewrote file://server/share/repo into a local path. Keep the slash logic in the local-path --track case only and leave explicit URLs as parsed. A subdirectory named SKILL.md passed the existence check, so a scratch folder holding one was still collected. Both collect entry points (scan and the dashboard request) now share sync.HasSkillFile, which rejects directories. --- internal/install/install_tracked.go | 12 +++++++++++- internal/install/install_tracked_test.go | 22 ++++++++++++++++------ internal/install/source.go | 12 +----------- internal/server/handler_collect.go | 2 +- internal/server/handler_collect_test.go | 2 +- internal/sync/pull.go | 10 ++++++++-- internal/sync/pull_test.go | 1 + 7 files changed, 39 insertions(+), 22 deletions(-) diff --git a/internal/install/install_tracked.go b/internal/install/install_tracked.go index 8ce7a46f8..5427dea90 100644 --- a/internal/install/install_tracked.go +++ b/internal/install/install_tracked.go @@ -9,12 +9,22 @@ import ( "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. +func localFileURL(path string) string { + p := filepath.ToSlash(path) + if !strings.HasPrefix(p, "/") { + p = "/" + p + } + return "file://" + p +} + // 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 && IsGitRepo(source.Path) { source.Type = SourceTypeGitHTTPS - source.CloneURL = fileCloneURL(source.Path) + source.CloneURL = localFileURL(source.Path) return validateCloneURL(source.CloneURL) } if source.IsGit() { diff --git a/internal/install/install_tracked_test.go b/internal/install/install_tracked_test.go index 73ae69fde..9def3d1bc 100644 --- a/internal/install/install_tracked_test.go +++ b/internal/install/install_tracked_test.go @@ -276,12 +276,22 @@ func TestInferTrackedKind_PlainLocalDirHintsFileURL(t *testing.T) { } } -func TestFileCloneURL_KeepsWindowsDriveInPath(t *testing.T) { - // Linux ToSlash leaves "/" alone, so this is the form a Windows path takes after ToSlash. - if got := fileCloneURL("C:/repo"); got != "file:///C:/repo" { - t.Errorf("fileCloneURL(C:/repo) = %q, want file:///C:/repo", got) +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 := fileCloneURL("/tmp/repo"); got != "file:///tmp/repo" { - t.Errorf("fileCloneURL(/tmp/repo) = %q, want file:///tmp/repo", got) + if got := localFileURL("/tmp/repo"); got != "file:///tmp/repo" { + t.Errorf("localFileURL(/tmp/repo) = %q, want file:///tmp/repo", 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) } } diff --git a/internal/install/source.go b/internal/install/source.go index 55ecc599a..309dd2393 100644 --- a/internal/install/source.go +++ b/internal/install/source.go @@ -550,16 +550,6 @@ func parseSSHURL(matches []string, source *Source) (*Source, error) { return source, nil } -// fileCloneURL builds the file:// URL git clones a local repository from. -func fileCloneURL(path string) string { - p := filepath.ToSlash(path) - // A Windows drive path (C:/repo) needs the empty authority: file:///C:/repo. - if !strings.HasPrefix(p, "/") { - p = "/" + p - } - return "file://" + p -} - func parseFileURL(matches []string, source *Source) (*Source, error) { // matches: [full, path, subdir] path := filepath.Clean(matches[1]) @@ -572,7 +562,7 @@ func parseFileURL(matches []string, source *Source) (*Source, error) { // filepath.Clean rewrites the separators for the local OS, but a file:// URL // keeps forward slashes everywhere; on Windows the raw path would produce // `file://\path\to\repo`, which git does not accept as a local repository. - source.CloneURL = fileCloneURL(path) + source.CloneURL = "file://" + filepath.ToSlash(path) if err := validateCloneURL(source.CloneURL); err != nil { return nil, err diff --git a/internal/server/handler_collect.go b/internal/server/handler_collect.go index 2161b4e5c..810841894 100644 --- a/internal/server/handler_collect.go +++ b/internal/server/handler_collect.go @@ -220,7 +220,7 @@ func (s *Server) handleCollect(w http.ResponseWriter, r *http.Request) { writeError(w, http.StatusBadRequest, "skill is not a directory: "+ref.Name) return } - if _, err := os.Stat(filepath.Join(skillPath, "SKILL.md")); err != nil { + if !ssync.HasSkillFile(skillPath) { writeError(w, http.StatusBadRequest, "not a skill (no SKILL.md): "+ref.Name) return } diff --git a/internal/server/handler_collect_test.go b/internal/server/handler_collect_test.go index ecf7ebbd3..7b5c18e41 100644 --- a/internal/server/handler_collect_test.go +++ b/internal/server/handler_collect_test.go @@ -412,7 +412,7 @@ func TestHandleCollect_RejectsFolderWithoutSkillMd(t *testing.T) { tgtPath := filepath.Join(t.TempDir(), "claude-skills") s, _ := newTestServerWithTargets(t, map[string]string{"claude": tgtPath}) - os.MkdirAll(filepath.Join(tgtPath, "scratch-dir"), 0755) + 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)) diff --git a/internal/sync/pull.go b/internal/sync/pull.go index e01505171..4505599a1 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 SKILL.md file; a folder without one +// (a scratch dir) is not a skill. +func HasSkillFile(dir string) bool { + info, err := os.Stat(filepath.Join(dir, "SKILL.md")) + return err == nil && !info.IsDir() +} + // 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,8 +115,7 @@ func FindLocalSkills(targetPath, sourcePath, syncMode string) ([]LocalSkillInfo, continue } - // A folder without SKILL.md (a scratch dir) is not a skill. - if _, err := os.Stat(filepath.Join(skillPath, "SKILL.md")); err != nil { + if !HasSkillFile(skillPath) { continue } diff --git a/internal/sync/pull_test.go b/internal/sync/pull_test.go index 0a53429a8..6215e9bcb 100644 --- a/internal/sync/pull_test.go +++ b/internal/sync/pull_test.go @@ -359,6 +359,7 @@ func TestFindLocalSkills_SkipsDirWithoutSkillMd(t *testing.T) { 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) From 26e4c5279beb14209d68e45246b6ed4d1d2ec470 Mon Sep 17 00:00:00 2001 From: Willie Date: Fri, 9 Oct 2026 23:52:36 +0800 Subject: [PATCH 04/15] fix(install): escape the local path in the file:// URL used by --track Git decodes a file:// URL, so a repository path containing %20, %2F, a space or # was cloned from a different path than the one IsGitRepo accepted. Build the URL with net/url so those characters are escaped; drive-letter and POSIX paths keep their form. The integration test now installs from a directory whose name contains a space and a literal %20. --- internal/install/install_tracked.go | 8 +++++--- internal/install/install_tracked_test.go | 3 +++ tests/integration/install_track_root_skill_test.go | 4 ++-- 3 files changed, 10 insertions(+), 5 deletions(-) diff --git a/internal/install/install_tracked.go b/internal/install/install_tracked.go index 5427dea90..96ab34c2d 100644 --- a/internal/install/install_tracked.go +++ b/internal/install/install_tracked.go @@ -2,6 +2,7 @@ package install import ( "fmt" + "net/url" "os" "path/filepath" "strings" @@ -9,14 +10,15 @@ import ( "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. +// 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 "file://" + p + return (&url.URL{Scheme: "file", Path: p}).String() } // normalizeTrackSource turns a local path that is a git repository into the diff --git a/internal/install/install_tracked_test.go b/internal/install/install_tracked_test.go index 9def3d1bc..6b2a1b270 100644 --- a/internal/install/install_tracked_test.go +++ b/internal/install/install_tracked_test.go @@ -284,6 +284,9 @@ func TestLocalFileURL_KeepsWindowsDriveInPath(t *testing.T) { 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) { diff --git a/tests/integration/install_track_root_skill_test.go b/tests/integration/install_track_root_skill_test.go index 65bf5057c..8bebcd4ae 100644 --- a/tests/integration/install_track_root_skill_test.go +++ b/tests/integration/install_track_root_skill_test.go @@ -357,13 +357,13 @@ func TestInstall_Track_LocalGitPath(t *testing.T) { defer sb.Cleanup() setupGlobalConfig(sb) - repo := filepath.Join(sb.Root, "local-repo") + 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", "--branch", "main", "--skip-audit") + 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")) { From b021544065cda522deb51d2abe546dfa8bd0f644 Mon Sep 17 00:00:00 2001 From: Willie Date: Fri, 9 Oct 2026 23:57:57 +0800 Subject: [PATCH 05/15] fix(install): accept a local bare repository path for --track A bare repository has HEAD and objects at its root but no .git entry, so the local-path check rejected it while file:///same/path cloned fine. Add IsLocalGitRepo (working tree or bare) and use it for both the --track normalization and the --branch guard. --- cmd/skillshare/install.go | 2 +- internal/install/install_git.go | 11 +++++++++++ internal/install/install_tracked.go | 2 +- internal/install/install_tracked_test.go | 12 ++++++++++++ .../install_track_root_skill_test.go | 18 ++++++++++++++++++ 5 files changed, 43 insertions(+), 2 deletions(-) diff --git a/cmd/skillshare/install.go b/cmd/skillshare/install.go index 8b8cf866f..ad165c707 100644 --- a/cmd/skillshare/install.go +++ b/cmd/skillshare/install.go @@ -202,7 +202,7 @@ func parseInstallArgs(args []string) (*installArgs, bool, error) { if result.opts.Branch != "" && result.sourceArg != "" { source, parseErr := install.ParseSource(result.sourceArg) trackableLocal := result.opts.Track && source != nil && - source.Type == install.SourceTypeLocalPath && install.IsGitRepo(source.Path) + 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") } 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 96ab34c2d..f1ed7c1a5 100644 --- a/internal/install/install_tracked.go +++ b/internal/install/install_tracked.go @@ -24,7 +24,7 @@ func localFileURL(path string) string { // 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 && IsGitRepo(source.Path) { + if source.Type == SourceTypeLocalPath && IsLocalGitRepo(source.Path) { source.Type = SourceTypeGitHTTPS source.CloneURL = localFileURL(source.Path) return validateCloneURL(source.CloneURL) diff --git a/internal/install/install_tracked_test.go b/internal/install/install_tracked_test.go index 6b2a1b270..395eaa70c 100644 --- a/internal/install/install_tracked_test.go +++ b/internal/install/install_tracked_test.go @@ -298,3 +298,15 @@ func TestParseSource_ExplicitUNCFileURLKeepsAuthority(t *testing.T) { t.Errorf("CloneURL = %q, want file://server/share/repo", source.CloneURL) } } + +func TestInferTrackedKind_LocalBareRepoPath(t *testing.T) { + bare := strings.TrimPrefix(makeRemote(t, ""), "file://") + + 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) + } +} diff --git a/tests/integration/install_track_root_skill_test.go b/tests/integration/install_track_root_skill_test.go index 8bebcd4ae..99c9605d4 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" @@ -370,3 +371,20 @@ func TestInstall_Track_LocalGitPath(t *testing.T) { 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") + } +} From 6c613d7342bf24876fb05e1e4bada8b94763530f Mon Sep 17 00:00:00 2001 From: Willie Date: Sat, 10 Oct 2026 00:03:49 +0800 Subject: [PATCH 06/15] test(install): build the bare repo path from a Windows file URL correctly makeRemote returns file:///C:/dir on Windows; trimming only file:// left /C:/dir, which filepath.Abs turned into D:\C:\dir, so the bare-repo test failed on the Windows runner. Drop the slash before the drive letter. --- internal/install/install_tracked_test.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/internal/install/install_tracked_test.go b/internal/install/install_tracked_test.go index 395eaa70c..c5fd0982f 100644 --- a/internal/install/install_tracked_test.go +++ b/internal/install/install_tracked_test.go @@ -301,6 +301,10 @@ func TestParseSource_ExplicitUNCFileURLKeepsAuthority(t *testing.T) { 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 { From 958561690d0c660554cbeedd5c8c89a496bf8b0f Mon Sep 17 00:00:00 2001 From: Willie Date: Sat, 10 Oct 2026 00:10:08 +0800 Subject: [PATCH 07/15] fix(install): refuse a --track local source that is the install destination --force removes the tracked destination before cloning. When the local path given to --track is that same directory, the removal deleted the only clone source (and any unpushed work) before the clone failed. Compare the two with os.SameFile, which also covers symlinked paths, and stop before touching the destination. --- internal/install/install_tracked.go | 6 +++++- internal/install/install_tracked_test.go | 20 ++++++++++++++++++++ 2 files changed, 25 insertions(+), 1 deletion(-) diff --git a/internal/install/install_tracked.go b/internal/install/install_tracked.go index f1ed7c1a5..2153f3950 100644 --- a/internal/install/install_tracked.go +++ b/internal/install/install_tracked.go @@ -92,7 +92,11 @@ func installTrackedRepoImpl(source *Source, sourceDir string, opts InstallOption } // Check if already exists - if _, err := os.Stat(destPath); err == nil { + if destInfo, err := os.Stat(destPath); err == nil { + // --force removes the destination before cloning; never do that to the clone source. + if srcInfo, srcErr := os.Stat(source.Path); source.Path != "" && srcErr == nil && os.SameFile(srcInfo, destInfo) { + return nil, fmt.Errorf("source %s is the install destination; nothing to install", source.Path) + } if opts.Update { return updateTrackedRepo(destPath, result, opts) } diff --git a/internal/install/install_tracked_test.go b/internal/install/install_tracked_test.go index c5fd0982f..9fce9848d 100644 --- a/internal/install/install_tracked_test.go +++ b/internal/install/install_tracked_test.go @@ -314,3 +314,23 @@ func TestInferTrackedKind_LocalBareRepoPath(t *testing.T) { t.Fatalf("local bare repo should be trackable: %v", err) } } + +func TestInstallTrackedRepo_RefusesLocalSourceThatIsTheDestination(t *testing.T) { + sourceDir := t.TempDir() + dest := filepath.Join(sourceDir, "_foo") + mustRunGit(t, "", "init", "-b", "main", dest) + if err := os.WriteFile(filepath.Join(dest, "SKILL.md"), []byte("# unpushed"), 0644); err != nil { + t.Fatal(err) + } + + source, err := ParseSource(dest) + if err != nil { + t.Fatal(err) + } + if _, err := InstallTrackedRepo(source, sourceDir, InstallOptions{Name: "foo", Force: true}); err == nil { + t.Fatal("expected an error when the source is the install destination") + } + if _, err := os.Stat(filepath.Join(dest, "SKILL.md")); err != nil { + t.Fatalf("destination must be left intact: %v", err) + } +} From 6adeea6409e60fdade7584553fd707994fda8b7f Mon Sep 17 00:00:00 2001 From: Willie Date: Sat, 10 Oct 2026 00:15:50 +0800 Subject: [PATCH 08/15] fix(install,collect): keep a tracked source inside its destination; reject special SKILL.md files --track --force removed the destination before cloning, so a local source nested below it (or given as an explicit file:// URL) was deleted along with any unpushed work, and the clone then failed. Resolve the source and refuse when it is the destination or lies under it, for dry runs too. HasSkillFile used a not-a-directory test, so a FIFO named SKILL.md counted as a skill and the collect copy blocked on it. Require a regular file; a symlinked SKILL.md still resolves through os.Stat. --- internal/install/install_tracked.go | 39 ++++++++++++-- internal/install/install_tracked_test.go | 52 +++++++++++++------ internal/sync/pull.go | 6 +-- internal/sync/pull_fifo_unix_test.go | 20 +++++++ internal/sync/pull_test.go | 10 ++++ .../install_track_root_skill_test.go | 21 ++++++++ 6 files changed, 125 insertions(+), 23 deletions(-) create mode 100644 internal/sync/pull_fifo_unix_test.go diff --git a/internal/install/install_tracked.go b/internal/install/install_tracked.go index 2153f3950..b738ca5b6 100644 --- a/internal/install/install_tracked.go +++ b/internal/install/install_tracked.go @@ -21,6 +21,39 @@ func localFileURL(path string) string { return (&url.URL{Scheme: "file", Path: p}).String() } +// localCloneSource returns the filesystem path a local source clones from, or +// "" for a remote one. +func localCloneSource(source *Source) string { + if source.Path != "" { + return source.Path + } + u, err := url.Parse(source.CloneURL) + if err != nil || u.Scheme != "file" || u.Host != "" { + return "" + } + // file:///C:/repo carries the drive after a leading slash. + if p := u.Path; len(p) > 2 && p[0] == '/' && p[2] == ':' { + return filepath.FromSlash(p[1:]) + } + return filepath.FromSlash(u.Path) +} + +// 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 { @@ -92,10 +125,10 @@ func installTrackedRepoImpl(source *Source, sourceDir string, opts InstallOption } // Check if already exists - if destInfo, err := os.Stat(destPath); err == nil { + if _, err := os.Stat(destPath); err == nil { // --force removes the destination before cloning; never do that to the clone source. - if srcInfo, srcErr := os.Stat(source.Path); source.Path != "" && srcErr == nil && os.SameFile(srcInfo, destInfo) { - return nil, fmt.Errorf("source %s is the install destination; nothing to install", source.Path) + 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) } if opts.Update { return updateTrackedRepo(destPath, result, opts) diff --git a/internal/install/install_tracked_test.go b/internal/install/install_tracked_test.go index 9fce9848d..d8f81b2dd 100644 --- a/internal/install/install_tracked_test.go +++ b/internal/install/install_tracked_test.go @@ -315,22 +315,40 @@ func TestInferTrackedKind_LocalBareRepoPath(t *testing.T) { } } -func TestInstallTrackedRepo_RefusesLocalSourceThatIsTheDestination(t *testing.T) { - sourceDir := t.TempDir() - dest := filepath.Join(sourceDir, "_foo") - mustRunGit(t, "", "init", "-b", "main", dest) - if err := os.WriteFile(filepath.Join(dest, "SKILL.md"), []byte("# unpushed"), 0644); err != nil { - t.Fatal(err) - } - - source, err := ParseSource(dest) - if err != nil { - t.Fatal(err) - } - if _, err := InstallTrackedRepo(source, sourceDir, InstallOptions{Name: "foo", Force: true}); err == nil { - t.Fatal("expected an error when the source is the install destination") - } - if _, err := os.Stat(filepath.Join(dest, "SKILL.md")); err != nil { - t.Fatalf("destination must be left intact: %v", err) +func TestInstallTrackedRepo_RefusesLocalSourceInsideTheDestination(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}, + {"explicit file URL", "nested", func(p string) string { return fileURL(p) }, false}, + {"dry run", "nested", func(p string) string { return p }, true}, + } + 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) + marker := filepath.Join(repo, "SKILL.md") + if err := os.WriteFile(marker, []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(marker); err != nil { + t.Fatalf("source must be left intact: %v", err) + } + }) } } diff --git a/internal/sync/pull.go b/internal/sync/pull.go index 4505599a1..9d3ddbc45 100644 --- a/internal/sync/pull.go +++ b/internal/sync/pull.go @@ -42,11 +42,11 @@ type PullResult struct { Failed map[string]error } -// HasSkillFile reports whether dir holds a SKILL.md file; a folder without one -// (a scratch dir) is not a skill. +// 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.IsDir() + return err == nil && info.Mode().IsRegular() } // FindLocalSkills finds all local (non-symlinked) skills in a target directory. 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 6215e9bcb..a9cdccfc4 100644 --- a/internal/sync/pull_test.go +++ b/internal/sync/pull_test.go @@ -371,3 +371,13 @@ func TestFindLocalSkills_SkipsDirWithoutSkillMd(t *testing.T) { 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/tests/integration/install_track_root_skill_test.go b/tests/integration/install_track_root_skill_test.go index 99c9605d4..2dcd93cc5 100644 --- a/tests/integration/install_track_root_skill_test.go +++ b/tests/integration/install_track_root_skill_test.go @@ -388,3 +388,24 @@ func TestInstall_Track_LocalBareRepoPath(t *testing.T) { 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) + } + } +} From e9ea9a8e07771b23bf474a53758b4db53a57bd63 Mon Sep 17 00:00:00 2001 From: Willie Date: Sat, 10 Oct 2026 00:22:54 +0800 Subject: [PATCH 09/15] fix(install): treat file://localhost URLs as local clone sources Git clones file://localhost/path from the local filesystem, but the containment guard only recognised an empty authority, so --track --force could still delete a source given that way. Accept localhost (any case) as local. --- internal/install/install_tracked.go | 3 ++- internal/install/install_tracked_test.go | 1 + 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/internal/install/install_tracked.go b/internal/install/install_tracked.go index b738ca5b6..4d41e6e0b 100644 --- a/internal/install/install_tracked.go +++ b/internal/install/install_tracked.go @@ -28,7 +28,8 @@ func localCloneSource(source *Source) string { return source.Path } u, err := url.Parse(source.CloneURL) - if err != nil || u.Scheme != "file" || u.Host != "" { + // file://localhost/path is the same local path as file:///path. + if err != nil || u.Scheme != "file" || (u.Host != "" && !strings.EqualFold(u.Host, "localhost")) { return "" } // file:///C:/repo carries the drive after a leading slash. diff --git a/internal/install/install_tracked_test.go b/internal/install/install_tracked_test.go index d8f81b2dd..f4d082500 100644 --- a/internal/install/install_tracked_test.go +++ b/internal/install/install_tracked_test.go @@ -325,6 +325,7 @@ func TestInstallTrackedRepo_RefusesLocalSourceInsideTheDestination(t *testing.T) {"same path", "", func(p string) string { return p }, false}, {"nested path", "nested", func(p string) string { return p }, false}, {"explicit file URL", "nested", func(p string) string { return fileURL(p) }, false}, + {"localhost file URL", "nested", func(p string) string { return "file://localhost" + p }, false}, {"dry run", "nested", func(p string) string { return p }, true}, } for _, tc := range cases { From 9da5f9bf1daebc571e758bc23939ddd04cbcbbca Mon Sep 17 00:00:00 2001 From: Willie Date: Sat, 10 Oct 2026 00:31:36 +0800 Subject: [PATCH 10/15] test(install): build the localhost file URL from a slash path On Windows the temp path starts with a drive letter, so prefixing it onto file://localhost produced file://localhostC:\... instead of a valid URL and the case failed on the Windows runner. Convert to slashes and add the separator explicitly. --- internal/install/install_tracked_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/install/install_tracked_test.go b/internal/install/install_tracked_test.go index f4d082500..18eff9328 100644 --- a/internal/install/install_tracked_test.go +++ b/internal/install/install_tracked_test.go @@ -325,7 +325,7 @@ func TestInstallTrackedRepo_RefusesLocalSourceInsideTheDestination(t *testing.T) {"same path", "", func(p string) string { return p }, false}, {"nested path", "nested", func(p string) string { return p }, false}, {"explicit file URL", "nested", func(p string) string { return fileURL(p) }, false}, - {"localhost file URL", "nested", func(p string) string { return "file://localhost" + p }, false}, + {"localhost file URL", "nested", func(p string) string { return "file://localhost/" + strings.TrimPrefix(filepath.ToSlash(p), "/") }, false}, {"dry run", "nested", func(p string) string { return p }, true}, } for _, tc := range cases { From 7375400e0d9f99df00c8a15bee1db694670511ff Mon Sep 17 00:00:00 2001 From: Willie Date: Sat, 10 Oct 2026 00:37:35 +0800 Subject: [PATCH 11/15] fix(install): clone --track --force into a staging dir and swap it in last --force removed the existing tracked repo before cloning, so any source that lived under it (plain path, file:///, file://localhost, UNC, symlink or 8.3 alias) was destroyed before git could read it, and a failed clone also lost the old repo. Clone beside the destination, run the checks and the audit on the staging copy, then move the old repo aside, rename the new one in and drop the backup; on any failure the staging copy is removed and the old repo stays. The pre-removal containment refusal now covers plain local paths only (the url.Parse special-casing is gone). --- internal/install/install_tracked.go | 89 ++++++++++++++---------- internal/install/install_tracked_test.go | 84 +++++++++++++++++++--- 2 files changed, 126 insertions(+), 47 deletions(-) diff --git a/internal/install/install_tracked.go b/internal/install/install_tracked.go index 4d41e6e0b..1e08f3d72 100644 --- a/internal/install/install_tracked.go +++ b/internal/install/install_tracked.go @@ -5,7 +5,9 @@ import ( "net/url" "os" "path/filepath" + "strconv" "strings" + "time" "skillshare/internal/sourcefs" ) @@ -21,24 +23,6 @@ func localFileURL(path string) string { return (&url.URL{Scheme: "file", Path: p}).String() } -// localCloneSource returns the filesystem path a local source clones from, or -// "" for a remote one. -func localCloneSource(source *Source) string { - if source.Path != "" { - return source.Path - } - u, err := url.Parse(source.CloneURL) - // file://localhost/path is the same local path as file:///path. - if err != nil || u.Scheme != "file" || (u.Host != "" && !strings.EqualFold(u.Host, "localhost")) { - return "" - } - // file:///C:/repo carries the drive after a leading slash. - if p := u.Path; len(p) > 2 && p[0] == '/' && p[2] == ':' { - return filepath.FromSlash(p[1:]) - } - return filepath.FromSlash(u.Path) -} - // pathWithin reports whether path is dir itself or lies below it, after // resolving links. func pathWithin(dir, path string) bool { @@ -126,9 +110,10 @@ func installTrackedRepoImpl(source *Source, sourceDir string, opts InstallOption } // Check if already exists + replacing := false if _, err := os.Stat(destPath); err == nil { - // --force removes the destination before cloning; never do that to the clone source. - if p := localCloneSource(source); p != "" && pathWithin(destPath, p) { + // A plain local path that is, or lies inside, the destination would be replaced under itself. + if p := source.Path; p != "" && pathWithin(destPath, p) { return nil, fmt.Errorf("source %s is inside the install destination %s; nothing to install", p, destPath) } if opts.Update { @@ -152,11 +137,7 @@ func installTrackedRepoImpl(source *Source, sourceDir string, opts InstallOption 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 { @@ -171,28 +152,40 @@ func installTrackedRepoImpl(source *Source, sourceDir string, opts InstallOption if cloneBranch == "" { cloneBranch = source.Branch } + // --force clones beside the destination and swaps it in only after every + // check passed, so a failure (or a source that lives under the destination, + // however its URL is spelled) never costs the existing repo. + stamp := strconv.FormatInt(time.Now().UnixNano(), 36) + cloneRel, clonePath := destRel, destPath + if replacing { + 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 } } @@ -200,15 +193,15 @@ 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 { 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 { @@ -223,11 +216,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. + if replacing { + 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)) + } + } + 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 18eff9328..e5d391946 100644 --- a/internal/install/install_tracked_test.go +++ b/internal/install/install_tracked_test.go @@ -315,18 +315,20 @@ func TestInferTrackedKind_LocalBareRepoPath(t *testing.T) { } } -func TestInstallTrackedRepo_RefusesLocalSourceInsideTheDestination(t *testing.T) { +func TestInstallTrackedRepo_ForceNeverDestroysItsLocalSource(t *testing.T) { cases := []struct { name string repo string // relative to the destination source func(repo string) string dryRun bool + refuse bool // a plain local path inside the destination is refused outright }{ - {"same path", "", func(p string) string { return p }, false}, - {"nested path", "nested", func(p string) string { return p }, false}, - {"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}, - {"dry run", "nested", func(p string) string { return p }, true}, + {"same path", "", func(p string) string { return p }, false, true}, + {"nested path", "nested", func(p string) string { return p }, false, true}, + {"dry run", "nested", func(p string) string { return p }, true, true}, + // URL forms are not matched by path; the clone must finish before anything is removed. + {"explicit file URL", "nested", func(p string) string { return fileURL(p) }, false, false}, + {"localhost file URL", "nested", func(p string) string { return "file://localhost/" + strings.TrimPrefix(filepath.ToSlash(p), "/") }, false, false}, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { @@ -338,18 +340,80 @@ func TestInstallTrackedRepo_RefusesLocalSourceInsideTheDestination(t *testing.T) if err := os.WriteFile(marker, []byte("# unpushed"), 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") 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 { + switch { + case tc.refuse && err == nil: t.Fatal("expected an error when the source is inside the install destination") - } - if _, err := os.Stat(marker); err != nil { - t.Fatalf("source must be left intact: %v", err) + case err != nil: + if _, statErr := os.Stat(marker); statErr != nil { + t.Fatalf("source must be left intact after a failure: %v", statErr) + } + default: + if _, statErr := os.Stat(filepath.Join(dest, "SKILL.md")); statErr != nil { + t.Fatalf("a successful install must leave the cloned repo in place: %v", statErr) + } } }) } } + +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()) + } + } +} From c1e6c21f2451eeedf52b0a403c468faa6bfa65b2 Mon Sep 17 00:00:00 2001 From: Willie Date: Sat, 10 Oct 2026 00:44:04 +0800 Subject: [PATCH 12/15] fix(install): refuse a file:// --track source inside the forced destination The staged swap in 7375400 still lost data for a file:///, file://localhost or UNC source nested under the destination: the clone succeeded, the old repo (with the source inside it) was renamed to the backup, and removing the backup deleted the source's uncommitted work, which a clone cannot carry. Restore the file URL to local path mapping (now including UNC hosts) so the containment refusal covers every local spelling, not only plain paths. The test now keeps an uncommitted file in the source and requires it to survive. --- internal/install/install_tracked.go | 28 +++++++++++++--- internal/install/install_tracked_test.go | 41 +++++++++++++----------- 2 files changed, 46 insertions(+), 23 deletions(-) diff --git a/internal/install/install_tracked.go b/internal/install/install_tracked.go index 1e08f3d72..ffaa38cb4 100644 --- a/internal/install/install_tracked.go +++ b/internal/install/install_tracked.go @@ -23,6 +23,26 @@ func localFileURL(path string) string { 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 { @@ -112,8 +132,9 @@ func installTrackedRepoImpl(source *Source, sourceDir string, opts InstallOption // Check if already exists replacing := false if _, err := os.Stat(destPath); err == nil { - // A plain local path that is, or lies inside, the destination would be replaced under itself. - if p := source.Path; p != "" && pathWithin(destPath, p) { + // 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) } if opts.Update { @@ -153,8 +174,7 @@ func installTrackedRepoImpl(source *Source, sourceDir string, opts InstallOption cloneBranch = source.Branch } // --force clones beside the destination and swaps it in only after every - // check passed, so a failure (or a source that lives under the destination, - // however its URL is spelled) never costs the existing repo. + // check passed, so a failure never costs the existing repo. stamp := strconv.FormatInt(time.Now().UnixNano(), 36) cloneRel, clonePath := destRel, destPath if replacing { diff --git a/internal/install/install_tracked_test.go b/internal/install/install_tracked_test.go index e5d391946..b9bc804cb 100644 --- a/internal/install/install_tracked_test.go +++ b/internal/install/install_tracked_test.go @@ -321,14 +321,12 @@ func TestInstallTrackedRepo_ForceNeverDestroysItsLocalSource(t *testing.T) { repo string // relative to the destination source func(repo string) string dryRun bool - refuse bool // a plain local path inside the destination is refused outright }{ - {"same path", "", func(p string) string { return p }, false, true}, - {"nested path", "nested", func(p string) string { return p }, false, true}, - {"dry run", "nested", func(p string) string { return p }, true, true}, - // URL forms are not matched by path; the clone must finish before anything is removed. - {"explicit file URL", "nested", func(p string) string { return fileURL(p) }, false, false}, - {"localhost file URL", "nested", func(p string) string { return "file://localhost/" + strings.TrimPrefix(filepath.ToSlash(p), "/") }, false, false}, + {"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) { @@ -336,34 +334,39 @@ func TestInstallTrackedRepo_ForceNeverDestroysItsLocalSource(t *testing.T) { dest := filepath.Join(sourceDir, "_foo") repo := filepath.Join(dest, tc.repo) mustRunGit(t, "", "init", "-b", "main", repo) - marker := filepath.Join(repo, "SKILL.md") - if err := os.WriteFile(marker, []byte("# unpushed"), 0644); err != nil { + 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}) - switch { - case tc.refuse && err == nil: + if err == nil { t.Fatal("expected an error when the source is inside the install destination") - case err != nil: - if _, statErr := os.Stat(marker); statErr != nil { - t.Fatalf("source must be left intact after a failure: %v", statErr) - } - default: - if _, statErr := os.Stat(filepath.Join(dest, "SKILL.md")); statErr != nil { - t.Fatalf("a successful install must leave the cloned repo in place: %v", statErr) - } + } + 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() From 98e833ae8342a91cf9e7c68262c2022f90460224 Mon Sep 17 00:00:00 2001 From: Willie Date: Sat, 10 Oct 2026 00:49:13 +0800 Subject: [PATCH 13/15] fix(install): apply the source-inside-destination refusal only to --force The refusal ran before the --update branch, so `install --track --update` was rejected even though an update pulls the existing checkout in place and removes nothing. Only the forced replacement can destroy a source that lives under the destination, so check there. --- internal/install/install_tracked.go | 10 +++++----- internal/install/install_tracked_test.go | 21 +++++++++++++++++++++ 2 files changed, 26 insertions(+), 5 deletions(-) diff --git a/internal/install/install_tracked.go b/internal/install/install_tracked.go index ffaa38cb4..206e69838 100644 --- a/internal/install/install_tracked.go +++ b/internal/install/install_tracked.go @@ -132,11 +132,6 @@ func installTrackedRepoImpl(source *Source, sourceDir string, opts InstallOption // Check if already exists replacing := false if _, err := os.Stat(destPath); err == nil { - // 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) - } if opts.Update { return updateTrackedRepo(destPath, result, opts) } @@ -153,6 +148,11 @@ 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 { diff --git a/internal/install/install_tracked_test.go b/internal/install/install_tracked_test.go index b9bc804cb..a0529b144 100644 --- a/internal/install/install_tracked_test.go +++ b/internal/install/install_tracked_test.go @@ -420,3 +420,24 @@ func TestInstallTrackedRepo_ForceReplacesExistingRepoInto(t *testing.T) { } } } + +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) + } +} From c09098368340cc0d17971f4fa2d27cc95faa6453 Mon Sep 17 00:00:00 2001 From: Willie Date: Sat, 10 Oct 2026 00:54:19 +0800 Subject: [PATCH 14/15] fix(install): report a forced clone's root skill under its tracked name With --force the repo is discovered in the .skillshare-clone- staging directory, and discoverSkills names a root SKILL.md after the directory, so the CLI summary, dashboard response and oplog listed a random staging name instead of _foo. Use the tracked name for the root entry. --- internal/install/install_tracked.go | 3 +++ internal/install/install_tracked_test.go | 17 +++++++++++++++++ 2 files changed, 20 insertions(+) diff --git a/internal/install/install_tracked.go b/internal/install/install_tracked.go index 206e69838..a7389312f 100644 --- a/internal/install/install_tracked.go +++ b/internal/install/install_tracked.go @@ -216,6 +216,9 @@ func installTrackedRepoImpl(source *Source, sourceDir string, opts InstallOption 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(clonePath, source.authEnv())...) diff --git a/internal/install/install_tracked_test.go b/internal/install/install_tracked_test.go index a0529b144..3647e2af2 100644 --- a/internal/install/install_tracked_test.go +++ b/internal/install/install_tracked_test.go @@ -441,3 +441,20 @@ func TestInstallTrackedRepo_UpdateAcceptsSourceThatIsTheDestination(t *testing.T 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) + } +} From 890463abaa1c4608c983932b46b348c13c0a156b Mon Sep 17 00:00:00 2001 From: Willie Date: Sat, 10 Oct 2026 01:00:46 +0800 Subject: [PATCH 15/15] fix(install): stage fresh tracked clones so cleanup never hits another installer A fresh tracked install cloned straight into the destination, and the deferred cleanup removed it on any failure. If another process created the same tracked repo between the existence check and the clone, the failing install deleted that process's checkout. Clone every install into its own staging directory and rename it in last; a claimed destination makes the rename fail and only the staging copy is removed. --- internal/install/install_tracked.go | 18 ++++++++---------- internal/install/install_tracked_test.go | 21 +++++++++++++++++++++ 2 files changed, 29 insertions(+), 10 deletions(-) diff --git a/internal/install/install_tracked.go b/internal/install/install_tracked.go index a7389312f..4874687a5 100644 --- a/internal/install/install_tracked.go +++ b/internal/install/install_tracked.go @@ -173,14 +173,12 @@ func installTrackedRepoImpl(source *Source, sourceDir string, opts InstallOption if cloneBranch == "" { cloneBranch = source.Branch } - // --force clones beside the destination and swaps it in only after every - // check passed, so a failure never costs the existing repo. + // 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, clonePath := destRel, destPath - if replacing { - cloneRel = filepath.Join(filepath.Dir(destRel), ".skillshare-clone-"+stamp) - clonePath = filepath.Join(sourceDir, cloneRel) - } + cloneRel := filepath.Join(filepath.Dir(destRel), ".skillshare-clone-"+stamp) + clonePath := filepath.Join(sourceDir, cloneRel) installed := false defer func() { if !installed { @@ -241,9 +239,7 @@ func installTrackedRepoImpl(source *Source, sourceDir string, opts InstallOption // Security audit on the entire tracked repo. Accepted findings are keyed by // the final path, not the staging one. - if replacing { - opts.AuditAcceptRoot, opts.AuditAcceptPath = opts.auditAcceptTarget(destPath) - } + opts.AuditAcceptRoot, opts.AuditAcceptPath = opts.auditAcceptTarget(destPath) if err := auditTrackedRepo(clonePath, result, opts); err != nil { return nil, err } @@ -263,6 +259,8 @@ func installTrackedRepoImpl(source *Source, sourceDir string, opts InstallOption 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 diff --git a/internal/install/install_tracked_test.go b/internal/install/install_tracked_test.go index 3647e2af2..83dee8c70 100644 --- a/internal/install/install_tracked_test.go +++ b/internal/install/install_tracked_test.go @@ -458,3 +458,24 @@ func TestInstallTrackedRepo_ForceReportsRootSkillUnderFinalName(t *testing.T) { 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) + } +}