Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion cmd/skillshare/doctor.go
Original file line number Diff line number Diff line change
Expand Up @@ -242,7 +242,7 @@ func runDoctorChecks(cfg *config.Config, result *doctorResult, isProject bool) {
checkSkillTargetsField(result, discovered, targetNamesFromConfig(cfg.Targets))
targetCache := checkTargets(cfg, result, isProject)
printSymlinkCompatHint(cfg.Targets, cfg.Mode, isProject)
checkSharedTargetPaths(cfg, result)
checkSharedTargetPaths(cfg, result, isProject)
checkCrossTargetDiscovery(cfg, result, isProject)
checkSyncDrift(cfg, result, discovered, targetCache)
checkBrokenSymlinks(cfg, result)
Expand Down
30 changes: 29 additions & 1 deletion cmd/skillshare/doctor_json_test.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
package main

import "testing"
import (
"encoding/json"
"testing"
)

func TestFetchDoctorUpdateResultDevModeSimulatesUpdate(t *testing.T) {
oldVersion := version
Expand Down Expand Up @@ -33,3 +36,28 @@ func TestFinalizeDoctorJSONDevModeVersion(t *testing.T) {
t.Fatalf("doctor version = %#v", out.Version)
}
}

func TestDoctorCheckSuggestionsJSON(t *testing.T) {
result := &doctorResult{}
result.addCheckWithSuggestions(
"cross_target_discovery",
checkWarning,
"overlap",
[]string{"codex also scans universal"},
[]string{"Choose one authoritative route."},
)

out := buildDoctorOutput(result)
data, err := json.Marshal(out)
if err != nil {
t.Fatal(err)
}

var decoded doctorOutput
if err := json.Unmarshal(data, &decoded); err != nil {
t.Fatal(err)
}
if got := decoded.Checks[0].Suggestions; len(got) != 1 || got[0] != "Choose one authoritative route." {
t.Fatalf("doctor JSON suggestions = %v", got)
}
Comment on lines +50 to +62
}
24 changes: 21 additions & 3 deletions cmd/skillshare/doctor_shared_paths.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,14 +10,32 @@ import (
"skillshare/internal/ui"
)

func targetRemoveDryRunCommand(isProject bool) string {
modeFlag := "--global"
if isProject {
modeFlag = "--project"
}
return fmt.Sprintf("skillshare target remove <name> %s --dry-run", modeFlag)
}

func sharedTargetPathsSuggestion(path string, targets []string, isProject bool) string {
return fmt.Sprintf("Choose one authoritative target for %s; preview removing duplicate targets with `%s` (currently: %s).",
path, targetRemoveDryRunCommand(isProject), strings.Join(targets, ", "))
}

func crossTargetDiscoverySuggestion(scanner string, writers []string, isProject bool) string {
return fmt.Sprintf("Choose one authoritative route for %s-visible skills; keep %s's primary target or preview removing overlapping writer target(s) with `%s`: %s.",
scanner, scanner, targetRemoveDryRunCommand(isProject), strings.Join(writers, ", "))
}

// checkSharedTargetPaths warns when two or more enabled targets resolve to the
// same filesystem path after tilde expansion.
//
// Catches the "shared root" class of duplicate-skill problems (issue #135):
// e.g., enabling both `universal` and `warp` writes the same skill twice to
// ~/.agents/skills, and any runtime that scans that directory sees duplicates.
// Pure metadata check — no runtime probing required.
func checkSharedTargetPaths(cfg *config.Config, result *doctorResult) {
func checkSharedTargetPaths(cfg *config.Config, result *doctorResult, isProject bool) {
pathTargets := make(map[string][]string)
for name, target := range cfg.Targets {
raw := target.SkillsConfig().Path
Expand Down Expand Up @@ -55,7 +73,7 @@ func checkSharedTargetPaths(cfg *config.Config, result *doctorResult) {
for _, c := range collisions {
ui.Warning("Shared path %s ← %s", c.path, strings.Join(c.targets, ", "))
details = append(details, fmt.Sprintf("%s ← %s", c.path, strings.Join(c.targets, ", ")))
suggestion := fmt.Sprintf("Choose one authoritative target for %s and disable or reconfigure the rest (currently: %s)", c.path, strings.Join(c.targets, ", "))
suggestion := sharedTargetPathsSuggestion(c.path, c.targets, isProject)
fmt.Println(ui.DimText(" suggestion: " + suggestion))
suggestions = append(suggestions, suggestion)
result.addWarning()
Expand Down Expand Up @@ -166,7 +184,7 @@ func checkCrossTargetDiscovery(cfg *config.Config, result *doctorResult, isProje
details = append(details, fmt.Sprintf("%s (%s) also scans %s ← %s",
so.scanner, so.scannerPath, p.sharedPath, strings.Join(p.writers, ", ")))
}
suggestion := fmt.Sprintf("Choose one authoritative route for %s-visible skills; disable or reconfigure overlapping writer target(s): %s", so.scanner, strings.Join(writers, ", "))
suggestion := crossTargetDiscoverySuggestion(so.scanner, writers, isProject)
fmt.Println(ui.DimText(" suggestion: " + suggestion))
suggestions = append(suggestions, suggestion)
result.addWarning()
Expand Down
34 changes: 28 additions & 6 deletions cmd/skillshare/doctor_shared_paths_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ func TestCheckSharedTargetPaths_NoCollision(t *testing.T) {
},
}
r := &doctorResult{}
checkSharedTargetPaths(cfg, r)
checkSharedTargetPaths(cfg, r, false)

if r.warnings != 0 {
t.Errorf("expected 0 warnings, got %d", r.warnings)
Expand All @@ -35,7 +35,7 @@ func TestCheckSharedTargetPaths_Collision(t *testing.T) {
},
}
r := &doctorResult{}
checkSharedTargetPaths(cfg, r)
checkSharedTargetPaths(cfg, r, false)

if r.warnings != 1 {
t.Errorf("expected 1 warning (1 collision row), got %d", r.warnings)
Expand All @@ -53,7 +53,7 @@ func TestCheckSharedTargetPaths_Collision(t *testing.T) {
t.Fatalf("expected one suggestion, got %v", r.checks[0].Suggestions)
}
suggestion := r.checks[0].Suggestions[0]
for _, want := range []string{"Choose one authoritative target", "universal", "warp", "witsy", "/tmp/.agents/skills"} {
for _, want := range []string{"Choose one authoritative target", "skillshare target remove <name> --global --dry-run", "universal", "warp", "witsy", "/tmp/.agents/skills"} {
if !strings.Contains(suggestion, want) {
t.Errorf("suggestion %q missing %q", suggestion, want)
}
Expand All @@ -71,13 +71,35 @@ func TestCheckSharedTargetPaths_TildeExpansion(t *testing.T) {
},
}
r := &doctorResult{}
checkSharedTargetPaths(cfg, r)
checkSharedTargetPaths(cfg, r, false)

if r.warnings != 1 {
t.Errorf("expected collision after tilde+trailing-slash normalization, got %d warnings", r.warnings)
}
}

func TestCheckSharedTargetPaths_ProjectSuggestionUsesProjectFlag(t *testing.T) {
cfg := &config.Config{
Targets: map[string]config.TargetConfig{
"codex": {Skills: &config.ResourceTargetConfig{Path: ".agents/skills"}},
"cursor": {Skills: &config.ResourceTargetConfig{Path: ".agents/skills"}},
},
}
r := &doctorResult{}
checkSharedTargetPaths(cfg, r, true)

if len(r.checks) != 1 || len(r.checks[0].Suggestions) != 1 {
t.Fatalf("expected one suggestion, got %+v", r.checks)
}
suggestion := r.checks[0].Suggestions[0]
if !strings.Contains(suggestion, "skillshare target remove <name> --project --dry-run") {
t.Fatalf("suggestion %q missing project target remove command", suggestion)
}
if strings.Contains(suggestion, "skillshare target remove <name> --global --dry-run") {
t.Fatalf("suggestion %q should not use global target remove command", suggestion)
}
}

func TestCheckCrossTargetDiscovery_CodexSeesUniversal(t *testing.T) {
// codex's also_scans.global includes ~/.agents/skills (from targets.yaml).
// universal writes to ~/.agents/skills. Enabling both should warn.
Expand All @@ -103,7 +125,7 @@ func TestCheckCrossTargetDiscovery_CodexSeesUniversal(t *testing.T) {
t.Fatalf("expected one suggestion, got %v", r.checks[0].Suggestions)
}
suggestion := r.checks[0].Suggestions[0]
for _, want := range []string{"Choose one authoritative route", "codex", "universal"} {
for _, want := range []string{"Choose one authoritative route", "skillshare target remove <name> --global --dry-run", "codex", "universal"} {
if !strings.Contains(suggestion, want) {
t.Errorf("suggestion %q missing %q", suggestion, want)
}
Expand Down Expand Up @@ -181,7 +203,7 @@ func TestCheckSharedTargetPaths_EmptyPathSkipped(t *testing.T) {
},
}
r := &doctorResult{}
checkSharedTargetPaths(cfg, r)
checkSharedTargetPaths(cfg, r, false)

if r.warnings != 0 {
t.Errorf("empty paths must not collide; got %d warnings", r.warnings)
Expand Down
88 changes: 84 additions & 4 deletions tests/integration/doctor_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -522,10 +522,11 @@ type doctorJSON struct {
}

type doctorJSONCheck struct {
Name string `json:"name"`
Status string `json:"status"`
Message string `json:"message"`
Details []string `json:"details,omitempty"`
Name string `json:"name"`
Status string `json:"status"`
Message string `json:"message"`
Details []string `json:"details,omitempty"`
Suggestions []string `json:"suggestions,omitempty"`
}

type doctorJSONSummary struct {
Expand Down Expand Up @@ -659,6 +660,85 @@ targets: {}
}
}

func TestDoctor_SharedDiscoveryShowsSuggestion(t *testing.T) {
sb := testutil.NewSandbox(t)
defer sb.Cleanup()

os.MkdirAll(filepath.Join(sb.Home, ".agents", "skills"), 0755)
sb.WriteConfig(`source: ` + sb.SourcePath + `
targets:
codex:
path: ~/.codex/skills
universal:
path: ~/.agents/skills
`)

result := sb.RunCLI("doctor")

result.AssertSuccess(t)
result.AssertOutputContains(t, "codex will see content from: universal")
result.AssertOutputContains(t, "suggestion: Choose one authoritative route for codex")
result.AssertOutputContains(t, "skillshare target remove <name> --global --dry-run")
}

func TestDoctor_JSON_SharedDiscoveryIncludesSuggestion(t *testing.T) {
sb := testutil.NewSandbox(t)
defer sb.Cleanup()

os.MkdirAll(filepath.Join(sb.Home, ".agents", "skills"), 0755)
sb.WriteConfig(`source: ` + sb.SourcePath + `
targets:
codex:
path: ~/.codex/skills
universal:
path: ~/.agents/skills
`)

result := sb.RunCLI("doctor", "--json")

result.AssertSuccess(t)
out := parseDoctorJSON(t, result.Stdout)
for _, check := range out.Checks {
if check.Name != "cross_target_discovery" {
continue
}
if len(check.Suggestions) == 0 {
t.Fatalf("cross_target_discovery suggestions are empty: %+v", check)
}
suggestion := check.Suggestions[0]
if !strings.Contains(suggestion, "authoritative route") {
t.Fatalf("cross_target_discovery suggestion = %q, want authoritative route guidance", suggestion)
}
if !strings.Contains(suggestion, "skillshare target remove <name> --global --dry-run") {
t.Fatalf("cross_target_discovery suggestion = %q, want target remove dry-run guidance", suggestion)
}
return
}
t.Fatal("missing cross_target_discovery check")
}

func TestDoctor_GlobalModeInsideProjectShowsGlobalTargetRemoveSuggestion(t *testing.T) {
sb := testutil.NewSandbox(t)
defer sb.Cleanup()

projectRoot := sb.SetupProjectDir("claude")
os.MkdirAll(filepath.Join(sb.Home, ".agents", "skills"), 0755)
sb.WriteConfig(`source: ` + sb.SourcePath + `
targets:
codex:
path: ~/.codex/skills
universal:
path: ~/.agents/skills
`)

result := sb.RunCLIInDir(projectRoot, "doctor", "--global")

result.AssertSuccess(t)
result.AssertOutputNotContains(t, "(project)")
result.AssertOutputContains(t, "skillshare target remove <name> --global --dry-run")
result.AssertOutputNotContains(t, "skillshare target remove <name> --project --dry-run")
}

func TestDoctor_JSON_ProjectMode(t *testing.T) {
sb := testutil.NewSandbox(t)
defer sb.Cleanup()
Expand Down
Loading