diff --git a/server/cmd/multica/cmd_skill.go b/server/cmd/multica/cmd_skill.go index c6b08fcf50..23da0a03e4 100644 --- a/server/cmd/multica/cmd_skill.go +++ b/server/cmd/multica/cmd_skill.go @@ -7,7 +7,6 @@ import ( "errors" "fmt" "io" - "net/http" "net/url" "os" "strings" @@ -141,6 +140,7 @@ func init() { // skill import skillImportCmd.Flags().String("url", "", "URL to import from (required)") + skillImportCmd.Flags().String("on-conflict", "fail", "Conflict strategy when a skill with the same name exists: fail, overwrite, rename, or skip") skillImportCmd.Flags().String("output", "json", "Output format: table or json") // skill search @@ -419,9 +419,14 @@ func runSkillImport(cmd *cobra.Command, _ []string) error { if importURL == "" { return fmt.Errorf("--url is required") } + onConflict, _ := cmd.Flags().GetString("on-conflict") + if !validSkillImportConflictStrategy(onConflict) { + return fmt.Errorf("--on-conflict must be one of: fail, overwrite, rename, skip") + } body := map[string]any{ - "url": importURL, + "url": importURL, + "on_conflict": onConflict, } ctx, cancel := context.WithTimeout(context.Background(), cli.AtLeastAPITimeout(60*time.Second)) @@ -429,44 +434,107 @@ func runSkillImport(cmd *cobra.Command, _ []string) error { var result map[string]any if err := client.PostJSON(ctx, "/api/skills/import", body, &result); err != nil { - if handleSkillImportConflict(cmd, err) { - return nil + if handledErr := handleSkillImportError(cmd, err); handledErr != nil { + return handledErr } return fmt.Errorf("import skill: %w", err) } + return printSkillImportResult(cmd, result) +} + +func validSkillImportConflictStrategy(strategy string) bool { + switch strategy { + case "fail", "overwrite", "rename", "skip": + return true + } + return false +} + +func handleSkillImportError(cmd *cobra.Command, err error) error { + var httpErr *cli.HTTPError + if !errors.As(err, &httpErr) || strings.TrimSpace(httpErr.Body) == "" { + return nil + } + + var body map[string]any + if json.Unmarshal([]byte(httpErr.Body), &body) != nil { + return nil + } + if _, ok := body["status"]; !ok { + if _, hasExisting := body["existing_skill"]; !hasExisting { + return nil + } + body = normalizeLegacySkillImportConflict(body) + } + + if err := printSkillImportResult(cmd, body); err != nil { + return err + } + reason := strVal(body, "reason") + if reason == "" { + reason = strVal(body, "error") + } + if reason == "" { + reason = "skill import conflict" + } + return errors.New(reason) +} + +func normalizeLegacySkillImportConflict(body map[string]any) map[string]any { + reason := strVal(body, "error") + if reason == "" { + reason = "a skill with this name already exists" + } + reason += "; use --on-conflict overwrite to replace it or --on-conflict rename to import a copy" + return map[string]any{ + "status": "conflict", + "reason": reason, + "existing_skill": body["existing_skill"], + } +} + +func printSkillImportResult(cmd *cobra.Command, result map[string]any) error { output, _ := cmd.Flags().GetString("output") if output == "json" { return cli.PrintJSON(os.Stdout, result) } - fmt.Printf("Skill imported: %s (%s)\n", strVal(result, "name"), strVal(result, "id")) + status := strVal(result, "status") + if status == "" { + fmt.Printf("Skill imported: %s (%s)\n", strVal(result, "name"), strVal(result, "id")) + return nil + } + + skill := nestedMap(result, "skill") + existing := nestedMap(result, "existing_skill") + reason := strVal(result, "reason") + switch status { + case "created": + fmt.Printf("Skill imported: %s (%s)\n", strVal(skill, "name"), strVal(skill, "id")) + case "updated": + fmt.Printf("Skill updated: %s (%s)\n", strVal(skill, "name"), strVal(skill, "id")) + case "skipped": + fmt.Printf("Skill skipped: %s (%s)\n", strVal(existing, "name"), strVal(existing, "id")) + case "conflict": + fmt.Printf("Skill import conflict: %s (%s)\n", strVal(existing, "name"), strVal(existing, "id")) + case "failed": + fmt.Printf("Skill import failed: %s\n", reason) + default: + fmt.Printf("Skill import %s\n", status) + } + if reason != "" && status != "failed" { + fmt.Printf("Reason: %s\n", reason) + } return nil } -func handleSkillImportConflict(cmd *cobra.Command, err error) bool { - var httpErr *cli.HTTPError - if !errors.As(err, &httpErr) || httpErr.StatusCode != http.StatusConflict || strings.TrimSpace(httpErr.Body) == "" { - return false +func nestedMap(m map[string]any, key string) map[string]any { + nested, _ := m[key].(map[string]any) + if nested == nil { + return map[string]any{} } - - var body map[string]any - if json.Unmarshal([]byte(httpErr.Body), &body) != nil { - return false - } - if _, ok := body["existing_skill"]; !ok { - return false - } - - output, _ := cmd.Flags().GetString("output") - if output == "json" { - _ = cli.PrintJSON(os.Stdout, body) - return true - } - - existing, _ := body["existing_skill"].(map[string]any) - fmt.Printf("Skill already exists: %s (%s)\n", strVal(existing, "name"), strVal(existing, "id")) - return true + return nested } func runSkillSearch(cmd *cobra.Command, args []string) error { diff --git a/server/cmd/multica/cmd_skill_test.go b/server/cmd/multica/cmd_skill_test.go index 1efbb41521..aedd39a74a 100644 --- a/server/cmd/multica/cmd_skill_test.go +++ b/server/cmd/multica/cmd_skill_test.go @@ -18,6 +18,7 @@ func newSkillImportTestCmd() *cobra.Command { cmd.Flags().String("workspace-id", "", "") cmd.Flags().String("profile", "", "") cmd.Flags().String("url", "", "") + cmd.Flags().String("on-conflict", "fail", "") cmd.Flags().String("output", "json", "") return cmd } @@ -43,7 +44,7 @@ func captureStdout(t *testing.T, fn func() error) (string, error) { return string(out), runErr } -func TestRunSkillImportJsonTreatsDuplicateAsStructuredResult(t *testing.T) { +func TestRunSkillImportJsonTreatsDuplicateAsConflictResult(t *testing.T) { t.Setenv("HOME", t.TempDir()) t.Setenv("MULTICA_TOKEN", "test-token") t.Setenv("MULTICA_WORKSPACE_ID", "workspace-123") @@ -58,10 +59,21 @@ func TestRunSkillImportJsonTreatsDuplicateAsStructuredResult(t *testing.T) { if r.Header.Get("X-Workspace-ID") != "workspace-123" { t.Fatalf("X-Workspace-ID = %q, want workspace-123", r.Header.Get("X-Workspace-ID")) } + var body map[string]any + if err := json.NewDecoder(r.Body).Decode(&body); err != nil { + t.Fatalf("decode request body: %v", err) + } + if body["url"] != "https://skills.sh/acme/review-helper" { + t.Fatalf("url = %v", body["url"]) + } + if body["on_conflict"] != "fail" { + t.Fatalf("on_conflict = %v, want fail", body["on_conflict"]) + } w.Header().Set("Content-Type", "application/json") w.WriteHeader(http.StatusConflict) _ = json.NewEncoder(w).Encode(map[string]any{ - "error": "a skill with this name already exists", + "status": "conflict", + "reason": "a skill with this name already exists; use --on-conflict overwrite to replace it or --on-conflict rename to import a copy", "existing_skill": map[string]any{ "id": "skill-123", "name": "review-helper", @@ -78,16 +90,19 @@ func TestRunSkillImportJsonTreatsDuplicateAsStructuredResult(t *testing.T) { out, err := captureStdout(t, func() error { return runSkillImport(cmd, nil) }) - if err != nil { - t.Fatalf("runSkillImport returned error for duplicate import: %v", err) + if err == nil { + t.Fatal("expected duplicate import to return an error") } var got map[string]any if err := json.Unmarshal([]byte(out), &got); err != nil { t.Fatalf("decode stdout JSON %q: %v", out, err) } - if got["error"] != "a skill with this name already exists" { - t.Fatalf("error = %v", got["error"]) + if got["status"] != "conflict" { + t.Fatalf("status = %v", got["status"]) + } + if !strings.Contains(strVal(got, "reason"), "--on-conflict overwrite") { + t.Fatalf("reason = %v", got["reason"]) } existing, ok := got["existing_skill"].(map[string]any) if !ok { @@ -98,6 +113,52 @@ func TestRunSkillImportJsonTreatsDuplicateAsStructuredResult(t *testing.T) { } } +func TestRunSkillImportSendsOnConflictAndPrintsStructuredResult(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + t.Setenv("MULTICA_TOKEN", "test-token") + t.Setenv("MULTICA_WORKSPACE_ID", "workspace-123") + + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + var body map[string]any + if err := json.NewDecoder(r.Body).Decode(&body); err != nil { + t.Fatalf("decode request body: %v", err) + } + if body["on_conflict"] != "overwrite" { + t.Fatalf("on_conflict = %v, want overwrite", body["on_conflict"]) + } + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusOK) + _ = json.NewEncoder(w).Encode(map[string]any{ + "status": "updated", + "skill": map[string]any{ + "id": "skill-123", + "name": "review-helper", + }, + }) + })) + defer srv.Close() + t.Setenv("MULTICA_SERVER_URL", srv.URL) + + cmd := newSkillImportTestCmd() + _ = cmd.Flags().Set("url", "https://skills.sh/acme/review-helper") + _ = cmd.Flags().Set("on-conflict", "overwrite") + _ = cmd.Flags().Set("output", "json") + + out, err := captureStdout(t, func() error { + return runSkillImport(cmd, nil) + }) + if err != nil { + t.Fatalf("runSkillImport returned error: %v", err) + } + var got map[string]any + if err := json.Unmarshal([]byte(out), &got); err != nil { + t.Fatalf("decode stdout JSON %q: %v", out, err) + } + if got["status"] != "updated" { + t.Fatalf("status = %v", got["status"]) + } +} + func TestRunSkillSearchRequestsSearchEndpoint(t *testing.T) { var gotPath string srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { diff --git a/server/internal/handler/skill.go b/server/internal/handler/skill.go index 9db861057c..cd045426dd 100644 --- a/server/internal/handler/skill.go +++ b/server/internal/handler/skill.go @@ -101,9 +101,18 @@ type SkillWithFilesResponse struct { Files []SkillFileResponse `json:"files"` } +type SkillImportResult struct { + Status string `json:"status"` + Reason string `json:"reason,omitempty"` + Skill *SkillWithFilesResponse `json:"skill,omitempty"` + ExistingSkill *ExistingSkillIdentity `json:"existing_skill,omitempty"` +} + type ExistingSkillIdentity struct { - ID string `json:"id"` - Name string `json:"name"` + ID string `json:"id"` + Name string `json:"name"` + CreatedBy string `json:"created_by,omitempty"` + CanOverwrite bool `json:"can_overwrite,omitempty"` } func writeSkillImportDuplicateConflict(w http.ResponseWriter, existing ExistingSkillIdentity) { @@ -138,7 +147,19 @@ func (h *Handler) existingSkillIdentityByName(ctx context.Context, workspaceID p } return ExistingSkillIdentity{}, false, err } - return ExistingSkillIdentity{ID: uuidToString(skill.ID), Name: skill.Name}, true, nil + return existingSkillIdentity(skill, ""), true, nil +} + +func existingSkillIdentity(skill db.Skill, userID string) ExistingSkillIdentity { + identity := ExistingSkillIdentity{ + ID: uuidToString(skill.ID), + Name: skill.Name, + CanOverwrite: canOverwriteSkillByLocalImport(userID, skill), + } + if skill.CreatedBy.Valid { + identity.CreatedBy = uuidToString(skill.CreatedBy) + } + return identity } // decodeSkillConfig decodes a JSONB skill.config blob, defaulting to {} when @@ -530,7 +551,25 @@ func (h *Handler) DeleteSkill(w http.ResponseWriter, r *http.Request) { // --- Skill import --- type ImportSkillRequest struct { - URL string `json:"url"` + URL string `json:"url"` + OnConflict string `json:"on_conflict,omitempty"` +} + +const ( + importOnConflictFail = "fail" + importOnConflictOverwrite = "overwrite" + importOnConflictRename = "rename" + importOnConflictSkip = "skip" +) + +const maxImportRenameAttempts = 50 + +func validImportOnConflict(strategy string) bool { + switch strategy { + case "", importOnConflictFail, importOnConflictOverwrite, importOnConflictRename, importOnConflictSkip: + return true + } + return false } // Per-import bundle limits. These mirror the local-runtime importer so that @@ -1728,6 +1767,116 @@ func skillMdNotFoundError(owner, repo, skillName string) error { return fmt.Errorf("SKILL.md not found in repository %s/%s for skill %s", owner, repo, skillName) } +func skillImportConflictReason() string { + return "a skill with this name already exists; use --on-conflict overwrite to replace it or --on-conflict rename to import a copy" +} + +func (h *Handler) createImportedSkillWithName(ctx context.Context, workspaceID, creatorID pgtype.UUID, name string, imported *importedSkill, config map[string]any, files []CreateSkillFileRequest) (SkillWithFilesResponse, error) { + return h.createSkillWithFiles(ctx, skillCreateInput{ + WorkspaceID: workspaceID, + CreatorID: creatorID, + Name: name, + Description: imported.description, + Content: imported.content, + Config: config, + Files: files, + }) +} + +func (h *Handler) createRenamedImportedSkill(ctx context.Context, workspaceID, creatorID pgtype.UUID, baseName string, imported *importedSkill, config map[string]any, files []CreateSkillFileRequest) (SkillWithFilesResponse, error) { + for suffix := 2; suffix < maxImportRenameAttempts+2; suffix++ { + candidate := fmt.Sprintf("%s-%d", baseName, suffix) + resp, err := h.createImportedSkillWithName(ctx, workspaceID, creatorID, candidate, imported, config, files) + if err == nil { + return resp, nil + } + if !isUniqueViolation(err) { + return SkillWithFilesResponse{}, err + } + } + return SkillWithFilesResponse{}, fmt.Errorf("failed to find an available renamed skill name after %d attempts", maxImportRenameAttempts) +} + +func skillImportOverwriteFailure(err error) (int, string) { + switch { + case errors.Is(err, errSkillOverwriteNotFound): + return http.StatusConflict, "target skill no longer exists" + case errors.Is(err, errSkillOverwriteForbidden): + return http.StatusForbidden, "only the skill creator can overwrite this skill" + case errors.Is(err, errSkillOverwriteNameMismatch): + return http.StatusConflict, "target skill name no longer matches the imported skill" + default: + return http.StatusInternalServerError, "failed to overwrite skill: " + err.Error() + } +} + +func (h *Handler) resolveImportSkillConflict(w http.ResponseWriter, r *http.Request, strategy string, workspaceID string, workspaceUUID, creatorUUID pgtype.UUID, creatorID string, name string, imported *importedSkill, config map[string]any, files []CreateSkillFileRequest, existing db.Skill) { + existingInfo := existingSkillIdentity(existing, creatorID) + switch strategy { + case importOnConflictSkip: + writeJSON(w, http.StatusOK, SkillImportResult{ + Status: "skipped", + Reason: "a skill with this name already exists", + ExistingSkill: &existingInfo, + }) + case importOnConflictOverwrite: + if !canOverwriteSkillByLocalImport(creatorID, existing) { + writeJSON(w, http.StatusForbidden, SkillImportResult{ + Status: "failed", + Reason: "only the skill creator can overwrite this skill", + ExistingSkill: &existingInfo, + }) + return + } + resp, err := h.overwriteSkillWithFiles(r.Context(), skillOverwriteInput{ + WorkspaceID: workspaceUUID, + TargetSkillID: existing.ID, + UserID: creatorID, + ExpectedName: name, + Description: imported.description, + Content: imported.content, + Config: config, + Files: files, + }) + if err != nil { + status, reason := skillImportOverwriteFailure(err) + writeJSON(w, status, SkillImportResult{ + Status: "failed", + Reason: reason, + ExistingSkill: &existingInfo, + }) + return + } + actorType, actorID := h.resolveActor(r, creatorID, workspaceID) + h.publish(protocol.EventSkillUpdated, workspaceID, actorType, actorID, map[string]any{"skill": resp}) + writeJSON(w, http.StatusOK, SkillImportResult{Status: "updated", Skill: &resp}) + case importOnConflictRename: + resp, err := h.createRenamedImportedSkill(r.Context(), workspaceUUID, creatorUUID, name, imported, config, files) + if err != nil { + writeJSON(w, http.StatusInternalServerError, SkillImportResult{ + Status: "failed", + Reason: "failed to create renamed skill: " + err.Error(), + ExistingSkill: &existingInfo, + }) + return + } + actorType, actorID := h.resolveActor(r, creatorID, workspaceID) + h.publish(protocol.EventSkillCreated, workspaceID, actorType, actorID, map[string]any{"skill": resp}) + writeJSON(w, http.StatusCreated, SkillImportResult{ + Status: "created", + Reason: "renamed to avoid an existing skill", + Skill: &resp, + ExistingSkill: &existingInfo, + }) + default: + writeJSON(w, http.StatusConflict, SkillImportResult{ + Status: "conflict", + Reason: skillImportConflictReason(), + ExistingSkill: &existingInfo, + }) + } +} + // --- Import handler --- func (h *Handler) ImportSkill(w http.ResponseWriter, r *http.Request) { @@ -1748,6 +1897,15 @@ func (h *Handler) ImportSkill(w http.ResponseWriter, r *http.Request) { writeError(w, http.StatusBadRequest, "invalid request body") return } + if !validImportOnConflict(req.OnConflict) { + writeError(w, http.StatusBadRequest, "on_conflict must be one of: fail, overwrite, rename, skip") + return + } + structuredResult := req.OnConflict != "" + strategy := req.OnConflict + if strategy == "" { + strategy = importOnConflictFail + } source, normalized, err := detectImportSource(req.URL) if err != nil { @@ -1788,19 +1946,31 @@ func (h *Handler) ImportSkill(w http.ResponseWriter, r *http.Request) { if imported.origin != nil { config["origin"] = imported.origin } + name := sanitizeNullBytes(imported.name) - resp, err := h.createSkillWithFiles(r.Context(), skillCreateInput{ - WorkspaceID: workspaceUUID, - CreatorID: creatorUUID, - Name: imported.name, - Description: imported.description, - Content: imported.content, - Config: config, - Files: files, - }) + if structuredResult { + if existing, found, lerr := h.lookupSkillByName(r.Context(), workspaceUUID, name); lerr != nil { + writeJSON(w, http.StatusInternalServerError, SkillImportResult{ + Status: "failed", + Reason: "failed to check for existing skill: " + lerr.Error(), + }) + return + } else if found { + h.resolveImportSkillConflict(w, r, strategy, workspaceID, workspaceUUID, creatorUUID, creatorID, name, imported, config, files, existing) + return + } + } + + resp, err := h.createImportedSkillWithName(r.Context(), workspaceUUID, creatorUUID, name, imported, config, files) if err != nil { if isUniqueViolation(err) { - if existing, found, findErr := h.existingSkillIdentityByName(r.Context(), workspaceUUID, imported.name); findErr == nil && found { + if structuredResult { + if existing, found, lerr := h.lookupSkillByName(r.Context(), workspaceUUID, name); lerr == nil && found { + h.resolveImportSkillConflict(w, r, strategy, workspaceID, workspaceUUID, creatorUUID, creatorID, name, imported, config, files, existing) + return + } + } + if existing, found, findErr := h.existingSkillIdentityByName(r.Context(), workspaceUUID, name); findErr == nil && found { writeSkillImportDuplicateConflict(w, existing) } else { writeError(w, http.StatusConflict, "a skill with this name already exists") @@ -1812,6 +1982,10 @@ func (h *Handler) ImportSkill(w http.ResponseWriter, r *http.Request) { } actorType, actorID := h.resolveActor(r, creatorID, workspaceID) h.publish(protocol.EventSkillCreated, workspaceID, actorType, actorID, map[string]any{"skill": resp}) + if structuredResult { + writeJSON(w, http.StatusCreated, SkillImportResult{Status: "created", Skill: &resp}) + return + } writeJSON(w, http.StatusCreated, resp) } diff --git a/server/internal/handler/skill_import_duplicate_test.go b/server/internal/handler/skill_import_duplicate_test.go index 1c4312ebb0..f2e3e8e4cd 100644 --- a/server/internal/handler/skill_import_duplicate_test.go +++ b/server/internal/handler/skill_import_duplicate_test.go @@ -3,6 +3,7 @@ package handler import ( "context" "encoding/json" + "net/http" "net/http/httptest" "testing" ) @@ -46,3 +47,103 @@ func TestWriteSkillImportDuplicateConflictIncludesExistingSkill(t *testing.T) { t.Fatalf("existing_skill = %#v", existing) } } + +func withMockClawHubImport(t *testing.T, skillName string) string { + t.Helper() + slug := "review-helper" + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case "/api/v1/skills/" + slug: + _ = json.NewEncoder(w).Encode(map[string]any{ + "skill": map[string]any{ + "slug": slug, + "displayName": skillName, + "summary": "Imported test skill", + "tags": map[string]string{"latest": "1.0.0"}, + }, + }) + case "/api/v1/skills/" + slug + "/versions/1.0.0": + _ = json.NewEncoder(w).Encode(map[string]any{ + "version": map[string]any{ + "version": "1.0.0", + "files": []map[string]any{ + {"path": "SKILL.md", "size": 16}, + }, + }, + }) + case "/api/v1/skills/" + slug + "/file": + _, _ = w.Write([]byte("# Imported\n")) + default: + t.Fatalf("unexpected ClawHub path: %s", r.URL.String()) + } + })) + prev := clawHubAPIBase + clawHubAPIBase = srv.URL + "/api/v1" + t.Cleanup(func() { + clawHubAPIBase = prev + srv.Close() + }) + return "https://clawhub.ai/acme/" + slug +} + +func TestImportSkillOnConflictSkipReturnsStructuredResult(t *testing.T) { + if testHandler == nil || testPool == nil { + t.Skip("handler test DB not configured") + } + namePrefix := "url-import-skip" + skillName := namePrefix + "-" + t.Name() + existingID := insertHandlerTestSkill(t, namePrefix, "# Existing") + importURL := withMockClawHubImport(t, skillName) + + w := httptest.NewRecorder() + req := newRequestAsUser(testUserID, http.MethodPost, "/api/skills/import", map[string]any{ + "url": importURL, + "on_conflict": "skip", + }) + testHandler.ImportSkill(w, req) + + if w.Code != http.StatusOK { + t.Fatalf("status = %d, want 200: %s", w.Code, w.Body.String()) + } + var body SkillImportResult + if err := json.Unmarshal(w.Body.Bytes(), &body); err != nil { + t.Fatalf("decode body: %v", err) + } + if body.Status != "skipped" { + t.Fatalf("status = %q", body.Status) + } + if body.ExistingSkill == nil || body.ExistingSkill.ID != existingID || body.ExistingSkill.Name != skillName { + t.Fatalf("existing_skill = %#v", body.ExistingSkill) + } +} + +func TestImportSkillOnConflictRenameCreatesSuffixedSkill(t *testing.T) { + if testHandler == nil || testPool == nil { + t.Skip("handler test DB not configured") + } + namePrefix := "url-import-rename" + skillName := namePrefix + "-" + t.Name() + insertHandlerTestSkill(t, namePrefix, "# Existing") + importURL := withMockClawHubImport(t, skillName) + + w := httptest.NewRecorder() + req := newRequestAsUser(testUserID, http.MethodPost, "/api/skills/import", map[string]any{ + "url": importURL, + "on_conflict": "rename", + }) + testHandler.ImportSkill(w, req) + + if w.Code != http.StatusCreated { + t.Fatalf("status = %d, want 201: %s", w.Code, w.Body.String()) + } + var body SkillImportResult + if err := json.Unmarshal(w.Body.Bytes(), &body); err != nil { + t.Fatalf("decode body: %v", err) + } + if body.Status != "created" || body.Skill == nil { + t.Fatalf("body = %#v", body) + } + if body.Skill.Name != skillName+"-2" { + t.Fatalf("created skill name = %q, want %q", body.Skill.Name, skillName+"-2") + } +}