From b1431b9056294486177ab06ac5f9e5afee685569 Mon Sep 17 00:00:00 2001 From: J Date: Fri, 29 May 2026 11:28:12 +0800 Subject: [PATCH 1/4] feat(skills): structured conflict + overwrite path for local skill re-import MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Local-skill re-import previously failed (or silently skipped) on a same-name collision and, on delete+reimport, changed the skill UUID and dropped agent bindings. This adds a structured conflict result and a creator-only overwrite write path so a re-import can update the existing skill in place. - New terminal import status `conflict` carrying { existing_skill_id, existing_created_by, can_overwrite }; can_overwrite = requester is the skill creator (canOverwriteSkillByLocalImport — intentionally narrower than canManageSkill: admins edit in-app, not via re-import). - Conflict is detected at daemon-report time (the effective name is only known once the bundle arrives) via GetSkillByWorkspaceAndName, with the unique constraint as a race backstop. - Import requests carry action=overwrite + target_skill_id, persisted through both the in-memory and Redis LocalSkillImportStore (the heartbeat → daemon payload is unchanged; overwrite is resolved server-side). - overwriteSkillWithFiles updates by target_skill_id in one tx: re-checks existence (workspace-scoped) and creator permission, then replaces description/content/config and fully replaces files (pruning files absent from the new bundle). Preserves id, created_by, created_at, name, and agent_skill bindings. Publishes skill:updated (not skill:created). - Boundaries: target deleted or permission lost → failed (no fallback to create-by-name); any mid-write error rolls back the tx, leaving the original skill untouched. Retrying a terminal request is a no-op. Tests cover: creator/non-creator conflict (can_overwrite), overwrite preserves UUID + agent binding + prunes removed files, non-creator overwrite fails, deleted target fails without create fallback, retry idempotency, and Redis round-trip of the new fields. Backend half of MUL-2701. Contract change: same-name local imports now return status `conflict` instead of `failed` — the Desktop/core client must be updated to consume it (sibling task). MUL-2800 Co-authored-by: multica-agent --- .../internal/handler/runtime_local_skills.go | 281 ++++++++++--- .../runtime_local_skills_overwrite_test.go | 375 ++++++++++++++++++ .../runtime_local_skills_redis_store.go | 37 +- .../runtime_local_skills_redis_store_test.go | 43 +- .../handler/runtime_local_skills_test.go | 8 +- server/internal/handler/skill.go | 9 + server/internal/handler/skill_create.go | 112 ++++++ 7 files changed, 796 insertions(+), 69 deletions(-) create mode 100644 server/internal/handler/runtime_local_skills_overwrite_test.go diff --git a/server/internal/handler/runtime_local_skills.go b/server/internal/handler/runtime_local_skills.go index ef7357d5a..2044466e8 100644 --- a/server/internal/handler/runtime_local_skills.go +++ b/server/internal/handler/runtime_local_skills.go @@ -3,6 +3,7 @@ package handler import ( "context" "encoding/json" + "errors" "log/slog" "net/http" "sort" @@ -11,6 +12,8 @@ import ( "time" "github.com/go-chi/chi/v5" + "github.com/jackc/pgx/v5" + "github.com/jackc/pgx/v5/pgtype" "github.com/multica-ai/multica/server/internal/util" db "github.com/multica-ai/multica/server/pkg/db/generated" "github.com/multica-ai/multica/server/pkg/protocol" @@ -24,8 +27,35 @@ const ( RuntimeLocalSkillCompleted RuntimeLocalSkillRequestStatus = "completed" RuntimeLocalSkillFailed RuntimeLocalSkillRequestStatus = "failed" RuntimeLocalSkillTimeout RuntimeLocalSkillRequestStatus = "timeout" + // RuntimeLocalSkillConflict is a terminal state set when a fresh import + // hits an existing same-name skill. It is not an error: the request carries + // structured Conflict metadata so the caller (Desktop UI / CLI) can offer + // overwrite / rename / skip instead of silently failing. See MUL-2800. + RuntimeLocalSkillConflict RuntimeLocalSkillRequestStatus = "conflict" ) +// LocalSkillImportAction selects how a runtime-local-skill import resolves when +// a skill with the same name already exists in the workspace. +type LocalSkillImportAction string + +const ( + // LocalSkillImportActionCreate is the default: create a new skill, and + // surface a structured `conflict` if the name is already taken. + LocalSkillImportActionCreate LocalSkillImportAction = "" + // LocalSkillImportActionOverwrite re-imports onto an existing skill, + // identified by TargetSkillID. Only the skill's creator may overwrite. + LocalSkillImportActionOverwrite LocalSkillImportAction = "overwrite" +) + +// LocalSkillImportConflict is the structured result attached to a request that +// terminated in RuntimeLocalSkillConflict. CanOverwrite reflects the +// creator-only re-import policy (canOverwriteSkillByLocalImport). +type LocalSkillImportConflict struct { + ExistingSkillID string `json:"existing_skill_id"` + ExistingCreatedBy string `json:"existing_created_by,omitempty"` + CanOverwrite bool `json:"can_overwrite"` +} + const ( // runtimeLocalSkillPendingTimeout bounds how long a request can sit in // pending before the server marks it timed out. The value must accommodate @@ -66,7 +96,7 @@ type LocalSkillListStore interface { // runtime-local-skill import requests. Kept as a separate interface because the // Create signature carries import-specific fields (skill_key, optional rename). type LocalSkillImportStore interface { - Create(ctx context.Context, runtimeID, creatorID, skillKey string, name, description *string) (*RuntimeLocalSkillImportRequest, error) + Create(ctx context.Context, runtimeID, creatorID, skillKey string, name, description *string, action LocalSkillImportAction, targetSkillID string) (*RuntimeLocalSkillImportRequest, error) Get(ctx context.Context, id string) (*RuntimeLocalSkillImportRequest, error) HasPending(ctx context.Context, runtimeID string) (bool, error) PopPending(ctx context.Context, runtimeID string) (*RuntimeLocalSkillImportRequest, error) @@ -75,6 +105,9 @@ type LocalSkillImportStore interface { // multiple imports per heartbeat cycle. PopPendingBatch(ctx context.Context, runtimeID string, limit int) ([]*RuntimeLocalSkillImportRequest, error) Complete(ctx context.Context, id string, skill SkillResponse) error + // Conflict transitions a request to the terminal RuntimeLocalSkillConflict + // state, attaching structured conflict metadata for the caller to act on. + Conflict(ctx context.Context, id string, info LocalSkillImportConflict) error Fail(ctx context.Context, id string, errMsg string) error } @@ -143,18 +176,21 @@ type RuntimeLocalSkillListRequest struct { } type RuntimeLocalSkillImportRequest struct { - ID string `json:"id"` - RuntimeID string `json:"runtime_id"` - SkillKey string `json:"skill_key"` - Name *string `json:"name,omitempty"` - Description *string `json:"description,omitempty"` - Status RuntimeLocalSkillRequestStatus `json:"status"` - Skill *SkillResponse `json:"skill,omitempty"` - Error string `json:"error,omitempty"` - CreatedAt time.Time `json:"created_at"` - UpdatedAt time.Time `json:"updated_at"` - CreatorID string `json:"-"` - RunStartedAt *time.Time `json:"-"` + ID string `json:"id"` + RuntimeID string `json:"runtime_id"` + SkillKey string `json:"skill_key"` + Name *string `json:"name,omitempty"` + Description *string `json:"description,omitempty"` + Action LocalSkillImportAction `json:"action,omitempty"` + TargetSkillID string `json:"target_skill_id,omitempty"` + Status RuntimeLocalSkillRequestStatus `json:"status"` + Skill *SkillResponse `json:"skill,omitempty"` + Conflict *LocalSkillImportConflict `json:"conflict,omitempty"` + Error string `json:"error,omitempty"` + CreatedAt time.Time `json:"created_at"` + UpdatedAt time.Time `json:"updated_at"` + CreatorID string `json:"-"` + RunStartedAt *time.Time `json:"-"` } // InMemoryLocalSkillListStore is the single-node implementation — good enough @@ -277,7 +313,7 @@ func NewInMemoryLocalSkillImportStore() *InMemoryLocalSkillImportStore { return &InMemoryLocalSkillImportStore{requests: make(map[string]*RuntimeLocalSkillImportRequest)} } -func (s *InMemoryLocalSkillImportStore) Create(_ context.Context, runtimeID, creatorID, skillKey string, name, description *string) (*RuntimeLocalSkillImportRequest, error) { +func (s *InMemoryLocalSkillImportStore) Create(_ context.Context, runtimeID, creatorID, skillKey string, name, description *string, action LocalSkillImportAction, targetSkillID string) (*RuntimeLocalSkillImportRequest, error) { s.mu.Lock() defer s.mu.Unlock() @@ -288,15 +324,17 @@ func (s *InMemoryLocalSkillImportStore) Create(_ context.Context, runtimeID, cre } req := &RuntimeLocalSkillImportRequest{ - ID: randomID(), - RuntimeID: runtimeID, - SkillKey: skillKey, - Name: name, - Description: description, - Status: RuntimeLocalSkillPending, - CreatedAt: time.Now(), - UpdatedAt: time.Now(), - CreatorID: creatorID, + ID: randomID(), + RuntimeID: runtimeID, + SkillKey: skillKey, + Name: name, + Description: description, + Action: action, + TargetSkillID: targetSkillID, + Status: RuntimeLocalSkillPending, + CreatedAt: time.Now(), + UpdatedAt: time.Now(), + CreatorID: creatorID, } s.requests[req.ID] = req return req, nil @@ -396,6 +434,19 @@ func (s *InMemoryLocalSkillImportStore) Complete(_ context.Context, id string, s return nil } +func (s *InMemoryLocalSkillImportStore) Conflict(_ context.Context, id string, info LocalSkillImportConflict) error { + s.mu.Lock() + defer s.mu.Unlock() + + if req, ok := s.requests[id]; ok { + req.Status = RuntimeLocalSkillConflict + conflict := info + req.Conflict = &conflict + req.UpdatedAt = time.Now() + } + return nil +} + func (s *InMemoryLocalSkillImportStore) Fail(_ context.Context, id string, errMsg string) error { s.mu.Lock() defer s.mu.Unlock() @@ -412,6 +463,10 @@ type CreateRuntimeLocalSkillImportRequest struct { SkillKey string `json:"skill_key"` Name *string `json:"name,omitempty"` Description *string `json:"description,omitempty"` + // Action selects create (default) vs overwrite. When overwrite, + // TargetSkillID must reference the existing same-name skill. + Action LocalSkillImportAction `json:"action,omitempty"` + TargetSkillID string `json:"target_skill_id,omitempty"` } type reportedRuntimeLocalSkill struct { @@ -435,7 +490,8 @@ func cleanOptionalString(value *string) *string { } func runtimeLocalSkillRequestTerminal(status RuntimeLocalSkillRequestStatus) bool { - return status == RuntimeLocalSkillCompleted || status == RuntimeLocalSkillFailed || status == RuntimeLocalSkillTimeout + return status == RuntimeLocalSkillCompleted || status == RuntimeLocalSkillFailed || + status == RuntimeLocalSkillTimeout || status == RuntimeLocalSkillConflict } func (h *Handler) requireRuntimeLocalSkillAccess(w http.ResponseWriter, r *http.Request, runtimeID string) (runtimeIDAndWorkspace, bool) { @@ -542,6 +598,24 @@ func (h *Handler) InitiateImportLocalSkill(w http.ResponseWriter, r *http.Reques return } + targetSkillID := "" + switch req.Action { + case LocalSkillImportActionCreate: + // nothing extra + case LocalSkillImportActionOverwrite: + // Existence + creator permission are re-verified authoritatively at + // report time (the skill may change between confirm and write); here we + // only require a well-formed target so we never enqueue a doomed write. + uuid, ok := parseUUIDOrBadRequest(w, strings.TrimSpace(req.TargetSkillID), "target_skill_id") + if !ok { + return + } + targetSkillID = uuidToString(uuid) + default: + writeError(w, http.StatusBadRequest, "invalid action") + return + } + importReq, err := h.LocalSkillImportStore.Create( r.Context(), rt.runtimeID, @@ -549,6 +623,8 @@ func (h *Handler) InitiateImportLocalSkill(w http.ResponseWriter, r *http.Reques strings.TrimSpace(req.SkillKey), cleanOptionalString(req.Name), cleanOptionalString(req.Description), + req.Action, + targetSkillID, ) if err != nil { writeError(w, http.StatusInternalServerError, "failed to enqueue local skill import: "+err.Error()) @@ -671,21 +747,11 @@ func (h *Handler) ReportLocalSkillImportResult(w http.ResponseWriter, r *http.Re } if body.Status != "completed" { - if err := h.LocalSkillImportStore.Fail(r.Context(), requestID, body.Error); err != nil { - slog.Error("local skill import Fail failed", "error", err, "request_id", requestID) - writeError(w, http.StatusInternalServerError, "failed to persist failure") - return - } - writeJSON(w, http.StatusOK, map[string]string{"status": "ok"}) + h.failLocalSkillImport(w, r, requestID, body.Error) return } if body.Skill == nil { - if err := h.LocalSkillImportStore.Fail(r.Context(), requestID, "daemon returned an empty skill bundle"); err != nil { - slog.Error("local skill import Fail failed", "error", err, "request_id", requestID) - writeError(w, http.StatusInternalServerError, "failed to persist failure") - return - } - writeJSON(w, http.StatusOK, map[string]string{"status": "ok"}) + h.failLocalSkillImport(w, r, requestID, "daemon returned an empty skill bundle") return } creatorUUID, err := util.ParseUUID(req.CreatorID) @@ -715,33 +781,95 @@ func (h *Handler) ReportLocalSkillImportResult(w http.ResponseWriter, r *http.Re files = append(files, f) } + config := map[string]any{ + "origin": map[string]any{ + "type": "runtime_local", + "runtime_id": runtimeID, + "provider": body.Skill.Provider, + "source_path": body.Skill.SourcePath, + }, + } + + // Overwrite path: re-import onto an existing skill. Existence and creator + // permission are re-verified inside overwriteSkillWithFiles, in the same tx + // as the write, so a target deleted (or a creator change) between the user's + // confirm and this report fails cleanly without falling back to create. + if req.Action == LocalSkillImportActionOverwrite { + targetUUID, perr := util.ParseUUID(req.TargetSkillID) + if perr != nil { + failMsg := "stored target_skill_id is invalid" + if ferr := h.LocalSkillImportStore.Fail(r.Context(), requestID, failMsg); ferr != nil { + slog.Error("local skill import Fail failed", "error", ferr, "request_id", requestID) + } + writeError(w, http.StatusInternalServerError, failMsg) + return + } + resp, oerr := h.overwriteSkillWithFiles(r.Context(), skillOverwriteInput{ + WorkspaceID: rt.WorkspaceID, + TargetSkillID: targetUUID, + UserID: req.CreatorID, + Description: description, + Content: body.Skill.Content, + Config: config, + Files: files, + }) + if oerr != nil { + failMsg := oerr.Error() + switch { + case errors.Is(oerr, errSkillOverwriteNotFound): + failMsg = "target skill no longer exists" + case errors.Is(oerr, errSkillOverwriteForbidden): + failMsg = "you no longer have permission to overwrite this skill" + } + h.failLocalSkillImport(w, r, requestID, failMsg) + return + } + if err := h.LocalSkillImportStore.Complete(r.Context(), requestID, resp.SkillResponse); err != nil { + // The overwrite already committed; unlike the create path we must + // NOT delete the skill to "roll back" (that would destroy a + // pre-existing skill and its agent bindings). Surface 5xx so the + // daemon retries — the retry re-applies the same UPDATE idempotently. + slog.Error("local skill import overwrite Complete failed", + "error", err, "request_id", requestID, "skill_id", resp.ID) + writeError(w, http.StatusInternalServerError, "failed to persist import completion") + return + } + h.publish(protocol.EventSkillUpdated, uuidToString(rt.WorkspaceID), "member", req.CreatorID, map[string]any{"skill": resp}) + slog.Debug("runtime local skill overwritten", "runtime_id", runtimeID, "request_id", requestID, "skill_id", resp.ID) + writeJSON(w, http.StatusOK, map[string]string{"status": "ok"}) + return + } + + // Create path: detect a same-name conflict before writing. A conflict is a + // structured terminal state (not a failure) so the caller can offer + // overwrite / rename / skip. + if existing, found, lerr := h.lookupSkillByName(r.Context(), rt.WorkspaceID, sanitizeNullBytes(name)); lerr != nil { + h.failLocalSkillImport(w, r, requestID, "failed to check for existing skill: "+lerr.Error()) + return + } else if found { + h.reportLocalSkillConflict(w, r, requestID, req.CreatorID, existing) + return + } + resp, err := h.createSkillWithFiles(r.Context(), skillCreateInput{ WorkspaceID: rt.WorkspaceID, CreatorID: creatorUUID, Name: name, Description: description, Content: body.Skill.Content, - Config: map[string]any{ - "origin": map[string]any{ - "type": "runtime_local", - "runtime_id": runtimeID, - "provider": body.Skill.Provider, - "source_path": body.Skill.SourcePath, - }, - }, - Files: files, + Config: config, + Files: files, }) if err != nil { - failMsg := err.Error() + // A unique-violation here means another import won the race between our + // lookup and the insert — surface it as a conflict, not a hard failure. if isUniqueViolation(err) { - failMsg = "a skill with this name already exists" + if existing, found, lerr := h.lookupSkillByName(r.Context(), rt.WorkspaceID, sanitizeNullBytes(name)); lerr == nil && found { + h.reportLocalSkillConflict(w, r, requestID, req.CreatorID, existing) + return + } } - if ferr := h.LocalSkillImportStore.Fail(r.Context(), requestID, failMsg); ferr != nil { - slog.Error("local skill import Fail failed", "error", ferr, "request_id", requestID) - writeError(w, http.StatusInternalServerError, "failed to persist failure") - return - } - writeJSON(w, http.StatusOK, map[string]string{"status": "ok"}) + h.failLocalSkillImport(w, r, requestID, err.Error()) return } @@ -766,3 +894,50 @@ func (h *Handler) ReportLocalSkillImportResult(w http.ResponseWriter, r *http.Re slog.Debug("runtime local skill imported", "runtime_id", runtimeID, "request_id", requestID, "skill_id", resp.ID) writeJSON(w, http.StatusOK, map[string]string{"status": "ok"}) } + +// failLocalSkillImport marks the request failed and writes the standard daemon +// response (200 ok). If the store write itself fails it returns 500 so the +// daemon retries. +func (h *Handler) failLocalSkillImport(w http.ResponseWriter, r *http.Request, requestID, failMsg string) { + if err := h.LocalSkillImportStore.Fail(r.Context(), requestID, failMsg); err != nil { + slog.Error("local skill import Fail failed", "error", err, "request_id", requestID) + writeError(w, http.StatusInternalServerError, "failed to persist failure") + return + } + writeJSON(w, http.StatusOK, map[string]string{"status": "ok"}) +} + +// reportLocalSkillConflict records a same-name conflict as the terminal +// RuntimeLocalSkillConflict state with structured metadata the caller uses to +// offer overwrite / rename / skip. +func (h *Handler) reportLocalSkillConflict(w http.ResponseWriter, r *http.Request, requestID, creatorID string, existing db.Skill) { + info := LocalSkillImportConflict{ + ExistingSkillID: uuidToString(existing.ID), + CanOverwrite: canOverwriteSkillByLocalImport(creatorID, existing), + } + if existing.CreatedBy.Valid { + info.ExistingCreatedBy = uuidToString(existing.CreatedBy) + } + if err := h.LocalSkillImportStore.Conflict(r.Context(), requestID, info); err != nil { + slog.Error("local skill import Conflict failed", "error", err, "request_id", requestID) + writeError(w, http.StatusInternalServerError, "failed to persist conflict") + return + } + writeJSON(w, http.StatusOK, map[string]string{"status": "ok"}) +} + +// lookupSkillByName resolves a skill by (workspace, name). found=false with a +// nil error means there is no such skill — i.e. no conflict. +func (h *Handler) lookupSkillByName(ctx context.Context, workspaceID pgtype.UUID, name string) (db.Skill, bool, error) { + skill, err := h.Queries.GetSkillByWorkspaceAndName(ctx, db.GetSkillByWorkspaceAndNameParams{ + WorkspaceID: workspaceID, + Name: name, + }) + if err != nil { + if errors.Is(err, pgx.ErrNoRows) { + return db.Skill{}, false, nil + } + return db.Skill{}, false, err + } + return skill, true, nil +} diff --git a/server/internal/handler/runtime_local_skills_overwrite_test.go b/server/internal/handler/runtime_local_skills_overwrite_test.go new file mode 100644 index 000000000..d59ad1e95 --- /dev/null +++ b/server/internal/handler/runtime_local_skills_overwrite_test.go @@ -0,0 +1,375 @@ +package handler + +import ( + "context" + "encoding/json" + "fmt" + "net/http" + "net/http/httptest" + "testing" + "time" +) + +// createImportTargetSkill inserts a skill (owned by ownerID) plus the given +// path->content files directly into the DB, returning its id. Used as the +// pre-existing skill that conflict / overwrite imports collide with. +func createImportTargetSkill(t *testing.T, name, ownerID string, files map[string]string) string { + t.Helper() + + var skillID string + if err := testPool.QueryRow(context.Background(), ` + INSERT INTO skill (workspace_id, name, description, content, config, created_by) + VALUES ($1, $2, 'original description', '# original', '{}'::jsonb, $3) + RETURNING id + `, testWorkspaceID, name, ownerID).Scan(&skillID); err != nil { + t.Fatalf("create target skill: %v", err) + } + for path, content := range files { + if _, err := testPool.Exec(context.Background(), ` + INSERT INTO skill_file (skill_id, path, content) VALUES ($1, $2, $3) + `, skillID, path, content); err != nil { + t.Fatalf("create skill file: %v", err) + } + } + t.Cleanup(func() { + testPool.Exec(context.Background(), `DELETE FROM skill WHERE id = $1`, skillID) + }) + return skillID +} + +// bindAgentToSkill creates a workspace agent and binds it to skillID via +// agent_skill, returning the agent id. Lets overwrite tests assert the binding +// survives the re-import. +func bindAgentToSkill(t *testing.T, skillID string) string { + t.Helper() + + agentName := fmt.Sprintf("overwrite-test-agent-%d", time.Now().UnixNano()) + var agentID string + if err := testPool.QueryRow(context.Background(), ` + INSERT INTO agent ( + workspace_id, name, description, runtime_mode, runtime_config, + runtime_id, visibility, max_concurrent_tasks, owner_id + ) + VALUES ($1, $2, '', 'cloud', '{}'::jsonb, $3, 'workspace', 1, $4) + RETURNING id + `, testWorkspaceID, agentName, testRuntimeID, testUserID).Scan(&agentID); err != nil { + t.Fatalf("create agent: %v", err) + } + if _, err := testPool.Exec(context.Background(), ` + INSERT INTO agent_skill (agent_id, skill_id) VALUES ($1, $2) + `, agentID, skillID); err != nil { + t.Fatalf("bind agent skill: %v", err) + } + t.Cleanup(func() { + testPool.Exec(context.Background(), `DELETE FROM agent WHERE id = $1`, agentID) + }) + return agentID +} + +func countAgentSkillBindings(t *testing.T, skillID string) int { + t.Helper() + + var count int + if err := testPool.QueryRow(context.Background(), ` + SELECT count(*) FROM agent_skill WHERE skill_id = $1 + `, skillID).Scan(&count); err != nil { + t.Fatalf("count agent_skill: %v", err) + } + return count +} + +func getSkillRow(t *testing.T, skillID string) (name, description, content, createdBy string) { + t.Helper() + + if err := testPool.QueryRow(context.Background(), ` + SELECT name, description, content, COALESCE(created_by::text, '') + FROM skill WHERE id = $1 + `, skillID).Scan(&name, &description, &content, &createdBy); err != nil { + t.Fatalf("get skill row: %v", err) + } + return +} + +// reportBundleBody builds the daemon "completed" report body for an import. +func reportBundleBody(name, description, content string, files map[string]string) map[string]any { + fileList := make([]map[string]any, 0, len(files)) + for p, c := range files { + fileList = append(fileList, map[string]any{"path": p, "content": c}) + } + return map[string]any{ + "status": "completed", + "skill": map[string]any{ + "name": name, + "description": description, + "content": content, + "source_path": "~/.claude/skills/review-helper", + "provider": "claude", + "files": fileList, + }, + } +} + +func initiateLocalSkillImport(t *testing.T, runtimeID string, body map[string]any) string { + t.Helper() + + w := httptest.NewRecorder() + req := withURLParams( + newRequestAsUser(testUserID, http.MethodPost, "/api/runtimes/"+runtimeID+"/local-skills/import", body), + "runtimeId", runtimeID, + ) + testHandler.InitiateImportLocalSkill(w, req) + if w.Code != http.StatusOK { + t.Fatalf("InitiateImportLocalSkill: expected 200, got %d: %s", w.Code, w.Body.String()) + } + var importReq RuntimeLocalSkillImportRequest + if err := json.NewDecoder(w.Body).Decode(&importReq); err != nil { + t.Fatalf("decode import request: %v", err) + } + return importReq.ID +} + +func reportLocalSkillImport(t *testing.T, runtimeID, requestID string, body map[string]any) { + t.Helper() + + w := httptest.NewRecorder() + req := withURLParams( + newDaemonTokenRequest(http.MethodPost, "/api/daemon/runtimes/"+runtimeID+"/local-skills/import/"+requestID+"/result", body, testWorkspaceID, "overwrite-test-daemon"), + "runtimeId", runtimeID, + "requestId", requestID, + ) + testHandler.ReportLocalSkillImportResult(w, req) + if w.Code != http.StatusOK { + t.Fatalf("ReportLocalSkillImportResult: expected 200, got %d: %s", w.Code, w.Body.String()) + } +} + +func pollLocalSkillImport(t *testing.T, runtimeID, requestID string) RuntimeLocalSkillImportRequest { + t.Helper() + + w := httptest.NewRecorder() + req := withURLParams( + newRequestAsUser(testUserID, http.MethodGet, "/api/runtimes/"+runtimeID+"/local-skills/import/"+requestID, nil), + "runtimeId", runtimeID, + "requestId", requestID, + ) + testHandler.GetLocalSkillImportRequest(w, req) + if w.Code != http.StatusOK { + t.Fatalf("GetLocalSkillImportRequest: expected 200, got %d: %s", w.Code, w.Body.String()) + } + var got RuntimeLocalSkillImportRequest + if err := json.NewDecoder(w.Body).Decode(&got); err != nil { + t.Fatalf("decode poll response: %v", err) + } + return got +} + +// runLocalSkillImport drives initiate -> report -> poll and returns the +// terminal request. +func runLocalSkillImport(t *testing.T, runtimeID string, initBody, reportBody map[string]any) RuntimeLocalSkillImportRequest { + t.Helper() + requestID := initiateLocalSkillImport(t, runtimeID, initBody) + reportLocalSkillImport(t, runtimeID, requestID, reportBody) + return pollLocalSkillImport(t, runtimeID, requestID) +} + +func TestRuntimeLocalSkillImport_ConflictCreatorCanOverwrite(t *testing.T) { + if testHandler == nil { + t.Skip("database not available") + } + + runtimeID := createRuntimeLocalSkillTestRuntime(t, testUserID) + name := fmt.Sprintf("conflict-creator-%d", time.Now().UnixNano()) + existingID := createImportTargetSkill(t, name, testUserID, nil) + + got := runLocalSkillImport(t, runtimeID, + map[string]any{"skill_key": "review-helper"}, + reportBundleBody(name, "incoming description", "# incoming", map[string]string{"a.md": "A"}), + ) + + if got.Status != RuntimeLocalSkillConflict { + t.Fatalf("status = %s, want conflict", got.Status) + } + if got.Conflict == nil { + t.Fatal("expected conflict metadata") + } + if got.Conflict.ExistingSkillID != existingID { + t.Fatalf("existing_skill_id = %q, want %q", got.Conflict.ExistingSkillID, existingID) + } + if got.Conflict.ExistingCreatedBy != testUserID { + t.Fatalf("existing_created_by = %q, want %q", got.Conflict.ExistingCreatedBy, testUserID) + } + if !got.Conflict.CanOverwrite { + t.Fatal("creator should be allowed to overwrite") + } + // A conflict must neither create a second skill nor mutate the original. + if n := countSkillsByName(t, name); n != 1 { + t.Fatalf("expected exactly 1 skill named %q, got %d", name, n) + } + if _, desc, _, _ := getSkillRow(t, existingID); desc != "original description" { + t.Fatalf("conflict must not modify the existing skill, description = %q", desc) + } +} + +func TestRuntimeLocalSkillImport_ConflictNonCreatorCannotOverwrite(t *testing.T) { + if testHandler == nil { + t.Skip("database not available") + } + + runtimeID := createRuntimeLocalSkillTestRuntime(t, testUserID) + otherUserID := createRuntimeLocalSkillTestMember(t, "member") + name := fmt.Sprintf("conflict-noncreator-%d", time.Now().UnixNano()) + existingID := createImportTargetSkill(t, name, otherUserID, nil) + + got := runLocalSkillImport(t, runtimeID, + map[string]any{"skill_key": "review-helper"}, + reportBundleBody(name, "incoming description", "# incoming", nil), + ) + + if got.Status != RuntimeLocalSkillConflict { + t.Fatalf("status = %s, want conflict", got.Status) + } + if got.Conflict == nil { + t.Fatal("expected conflict metadata") + } + if got.Conflict.ExistingSkillID != existingID { + t.Fatalf("existing_skill_id = %q, want %q", got.Conflict.ExistingSkillID, existingID) + } + if got.Conflict.ExistingCreatedBy != otherUserID { + t.Fatalf("existing_created_by = %q, want %q", got.Conflict.ExistingCreatedBy, otherUserID) + } + if got.Conflict.CanOverwrite { + t.Fatal("a non-creator must not be allowed to overwrite") + } +} + +func TestRuntimeLocalSkillImport_OverwritePreservesIdentityAndBindings(t *testing.T) { + if testHandler == nil { + t.Skip("database not available") + } + + runtimeID := createRuntimeLocalSkillTestRuntime(t, testUserID) + name := fmt.Sprintf("overwrite-keep-%d", time.Now().UnixNano()) + existingID := createImportTargetSkill(t, name, testUserID, map[string]string{ + "keep.md": "old keep", + "prune.md": "should be removed", + }) + bindAgentToSkill(t, existingID) + + got := runLocalSkillImport(t, runtimeID, + map[string]any{"skill_key": "review-helper", "action": "overwrite", "target_skill_id": existingID}, + reportBundleBody(name, "overwritten description", "# overwritten", map[string]string{"keep.md": "new keep"}), + ) + + if got.Status != RuntimeLocalSkillCompleted { + t.Fatalf("status = %s, want completed (error=%q)", got.Status, got.Error) + } + if got.Skill == nil { + t.Fatal("expected overwritten skill in response") + } + // Same row: UUID and creator preserved. + if got.Skill.ID != existingID { + t.Fatalf("overwrite must preserve UUID: got %q, want %q", got.Skill.ID, existingID) + } + if got.Skill.CreatedBy == nil || *got.Skill.CreatedBy != testUserID { + t.Fatalf("created_by not preserved: %v", got.Skill.CreatedBy) + } + if got.Skill.Description != "overwritten description" { + t.Fatalf("description not replaced: %q", got.Skill.Description) + } + // Files fully replaced: prune.md (absent from the new bundle) is gone. + if n := countSkillFiles(t, existingID); n != 1 { + t.Fatalf("expected 1 file after overwrite, got %d", n) + } + // Agent binding preserved — the agent must NOT need to re-add the skill. + if n := countAgentSkillBindings(t, existingID); n != 1 { + t.Fatalf("expected agent binding to survive overwrite, got %d", n) + } +} + +func TestRuntimeLocalSkillImport_OverwriteNonCreatorFails(t *testing.T) { + if testHandler == nil { + t.Skip("database not available") + } + + runtimeID := createRuntimeLocalSkillTestRuntime(t, testUserID) + otherUserID := createRuntimeLocalSkillTestMember(t, "member") + name := fmt.Sprintf("overwrite-forbidden-%d", time.Now().UnixNano()) + existingID := createImportTargetSkill(t, name, otherUserID, nil) + + got := runLocalSkillImport(t, runtimeID, + map[string]any{"skill_key": "review-helper", "action": "overwrite", "target_skill_id": existingID}, + reportBundleBody(name, "incoming description", "# incoming", nil), + ) + + if got.Status != RuntimeLocalSkillFailed { + t.Fatalf("status = %s, want failed", got.Status) + } + // Original skill (owned by someone else) must be untouched. + if _, desc, _, _ := getSkillRow(t, existingID); desc != "original description" { + t.Fatalf("forbidden overwrite must not mutate the skill, description = %q", desc) + } +} + +func TestRuntimeLocalSkillImport_OverwriteTargetDeletedFails(t *testing.T) { + if testHandler == nil { + t.Skip("database not available") + } + + runtimeID := createRuntimeLocalSkillTestRuntime(t, testUserID) + name := fmt.Sprintf("overwrite-deleted-%d", time.Now().UnixNano()) + deletedID := createImportTargetSkill(t, name, testUserID, nil) + // Simulate the target being deleted between the user's confirm and the + // daemon report. + if _, err := testPool.Exec(context.Background(), `DELETE FROM skill WHERE id = $1`, deletedID); err != nil { + t.Fatalf("delete target skill: %v", err) + } + + got := runLocalSkillImport(t, runtimeID, + map[string]any{"skill_key": "review-helper", "action": "overwrite", "target_skill_id": deletedID}, + reportBundleBody(name, "incoming description", "# incoming", map[string]string{"a.md": "A"}), + ) + + if got.Status != RuntimeLocalSkillFailed { + t.Fatalf("status = %s, want failed", got.Status) + } + // Must NOT fall back to creating a new skill by name. + if n := countSkillsByName(t, name); n != 0 { + t.Fatalf("deleted-target overwrite must not create a skill, got %d", n) + } +} + +func TestRuntimeLocalSkillImport_OverwriteRetryIsIdempotent(t *testing.T) { + if testHandler == nil { + t.Skip("database not available") + } + + runtimeID := createRuntimeLocalSkillTestRuntime(t, testUserID) + name := fmt.Sprintf("overwrite-idempotent-%d", time.Now().UnixNano()) + existingID := createImportTargetSkill(t, name, testUserID, map[string]string{"old.md": "old"}) + + requestID := initiateLocalSkillImport(t, runtimeID, map[string]any{ + "skill_key": "review-helper", + "action": "overwrite", + "target_skill_id": existingID, + }) + + // First report wins and overwrites the skill. + reportLocalSkillImport(t, runtimeID, requestID, + reportBundleBody(name, "first overwrite", "# first", map[string]string{"first.md": "1"})) + + // A retry of the SAME request id with a different bundle must be ignored + // (the request is already terminal) — no second write. + reportLocalSkillImport(t, runtimeID, requestID, + reportBundleBody(name, "second overwrite", "# second", map[string]string{"second.md": "2", "extra.md": "3"})) + + got := pollLocalSkillImport(t, runtimeID, requestID) + if got.Status != RuntimeLocalSkillCompleted { + t.Fatalf("status = %s, want completed", got.Status) + } + if _, desc, _, _ := getSkillRow(t, existingID); desc != "first overwrite" { + t.Fatalf("retry must not re-apply, description = %q", desc) + } + if n := countSkillFiles(t, existingID); n != 1 { + t.Fatalf("retry must not re-write files, got %d files", n) + } +} diff --git a/server/internal/handler/runtime_local_skills_redis_store.go b/server/internal/handler/runtime_local_skills_redis_store.go index 61657f194..eb3fd0b4c 100644 --- a/server/internal/handler/runtime_local_skills_redis_store.go +++ b/server/internal/handler/runtime_local_skills_redis_store.go @@ -262,18 +262,20 @@ func NewRedisLocalSkillImportStore(rdb *redis.Client) *RedisLocalSkillImportStor return &RedisLocalSkillImportStore{rdb: rdb} } -func (s *RedisLocalSkillImportStore) Create(ctx context.Context, runtimeID, creatorID, skillKey string, name, description *string) (*RuntimeLocalSkillImportRequest, error) { +func (s *RedisLocalSkillImportStore) Create(ctx context.Context, runtimeID, creatorID, skillKey string, name, description *string, action LocalSkillImportAction, targetSkillID string) (*RuntimeLocalSkillImportRequest, error) { now := time.Now() req := &RuntimeLocalSkillImportRequest{ - ID: randomID(), - RuntimeID: runtimeID, - SkillKey: skillKey, - Name: name, - Description: description, - Status: RuntimeLocalSkillPending, - CreatedAt: now, - UpdatedAt: now, - CreatorID: creatorID, + ID: randomID(), + RuntimeID: runtimeID, + SkillKey: skillKey, + Name: name, + Description: description, + Action: action, + TargetSkillID: targetSkillID, + Status: RuntimeLocalSkillPending, + CreatedAt: now, + UpdatedAt: now, + CreatorID: creatorID, } data, err := s.marshalImport(req) if err != nil { @@ -494,6 +496,21 @@ func (s *RedisLocalSkillImportStore) Complete(ctx context.Context, id string, sk return s.persistImportRequest(ctx, req) } +func (s *RedisLocalSkillImportStore) Conflict(ctx context.Context, id string, info LocalSkillImportConflict) error { + req, err := s.loadImportRequest(ctx, id) + if err != nil { + return err + } + if req == nil { + return nil + } + req.Status = RuntimeLocalSkillConflict + conflict := info + req.Conflict = &conflict + req.UpdatedAt = time.Now() + return s.persistImportRequest(ctx, req) +} + func (s *RedisLocalSkillImportStore) Fail(ctx context.Context, id string, errMsg string) error { req, err := s.loadImportRequest(ctx, id) if err != nil { diff --git a/server/internal/handler/runtime_local_skills_redis_store_test.go b/server/internal/handler/runtime_local_skills_redis_store_test.go index 4c782c969..8c12a0503 100644 --- a/server/internal/handler/runtime_local_skills_redis_store_test.go +++ b/server/internal/handler/runtime_local_skills_redis_store_test.go @@ -225,7 +225,7 @@ func TestRedisLocalSkillImportStore_PreservesCreatorID(t *testing.T) { name := "Review Helper" desc := "Desc" - req, err := store.Create(ctx, "runtime-1", "user-42", "review-helper", &name, &desc) + req, err := store.Create(ctx, "runtime-1", "user-42", "review-helper", &name, &desc, LocalSkillImportActionOverwrite, "target-skill-99") if err != nil { t.Fatalf("create: %v", err) } @@ -249,6 +249,43 @@ func TestRedisLocalSkillImportStore_PreservesCreatorID(t *testing.T) { if got.Description == nil || *got.Description != desc { t.Fatalf("description lost: %v", got.Description) } + // The overwrite intent must survive the round trip — it is consumed at + // report time, not delivered to the daemon. + if got.Action != LocalSkillImportActionOverwrite { + t.Fatalf("action lost round trip: %q", got.Action) + } + if got.TargetSkillID != "target-skill-99" { + t.Fatalf("target_skill_id lost round trip: %q", got.TargetSkillID) + } +} + +func TestRedisLocalSkillImportStore_PreservesConflict(t *testing.T) { + rdb := newRedisTestClient(t) + ctx := context.Background() + store := NewRedisLocalSkillImportStore(rdb) + + req, err := store.Create(ctx, "runtime-1", "user-1", "review-helper", nil, nil, LocalSkillImportActionCreate, "") + if err != nil { + t.Fatalf("create: %v", err) + } + info := LocalSkillImportConflict{ExistingSkillID: "skill-7", ExistingCreatedBy: "user-2", CanOverwrite: false} + if err := store.Conflict(ctx, req.ID, info); err != nil { + t.Fatalf("conflict: %v", err) + } + + got, err := store.Get(ctx, req.ID) + if err != nil { + t.Fatalf("get: %v", err) + } + if got.Status != RuntimeLocalSkillConflict { + t.Fatalf("status = %s, want conflict", got.Status) + } + if got.Conflict == nil { + t.Fatalf("conflict metadata lost round trip") + } + if got.Conflict.ExistingSkillID != "skill-7" || got.Conflict.ExistingCreatedBy != "user-2" || got.Conflict.CanOverwrite { + t.Fatalf("conflict metadata corrupted: %+v", got.Conflict) + } } func TestRedisLocalSkillImportStore_PopPendingAcrossInstances(t *testing.T) { @@ -258,7 +295,7 @@ func TestRedisLocalSkillImportStore_PopPendingAcrossInstances(t *testing.T) { nodeA := NewRedisLocalSkillImportStore(rdb) nodeB := NewRedisLocalSkillImportStore(rdb) - req, err := nodeA.Create(ctx, "runtime-import", "user-1", "review-helper", nil, nil) + req, err := nodeA.Create(ctx, "runtime-import", "user-1", "review-helper", nil, nil, LocalSkillImportActionCreate, "") if err != nil { t.Fatalf("create: %v", err) } @@ -365,7 +402,7 @@ func TestRedisLocalSkillImportStore_PopPendingBatch(t *testing.T) { // Create 5 pending imports. ids := make([]string, 5) for i := range ids { - req, err := store.Create(ctx, "runtime-batch", "user-1", fmt.Sprintf("skill-%d", i), nil, nil) + req, err := store.Create(ctx, "runtime-batch", "user-1", fmt.Sprintf("skill-%d", i), nil, nil, LocalSkillImportActionCreate, "") if err != nil { t.Fatalf("create %d: %v", i, err) } diff --git a/server/internal/handler/runtime_local_skills_test.go b/server/internal/handler/runtime_local_skills_test.go index cd13cce70..2f1c52763 100644 --- a/server/internal/handler/runtime_local_skills_test.go +++ b/server/internal/handler/runtime_local_skills_test.go @@ -187,7 +187,7 @@ func TestInMemoryLocalSkillListStore_TimesOutRunningRequests(t *testing.T) { func TestInMemoryLocalSkillImportStore_TimesOutRunningRequests(t *testing.T) { ctx := context.Background() store := NewInMemoryLocalSkillImportStore() - req, err := store.Create(ctx, "runtime-xyz", "user-1", "review-helper", nil, nil) + req, err := store.Create(ctx, "runtime-xyz", "user-1", "review-helper", nil, nil, LocalSkillImportActionCreate, "") if err != nil { t.Fatalf("create: %v", err) } @@ -237,7 +237,7 @@ func TestGetLocalSkillImportRequest_RequiresRuntimeOwner(t *testing.T) { runtimeID := createRuntimeLocalSkillTestRuntime(t, testUserID) adminUserID := createRuntimeLocalSkillTestMember(t, "admin") - importReq, err := testHandler.LocalSkillImportStore.Create(context.Background(), runtimeID, testUserID, "review-helper", nil, nil) + importReq, err := testHandler.LocalSkillImportStore.Create(context.Background(), runtimeID, testUserID, "review-helper", nil, nil, LocalSkillImportActionCreate, "") if err != nil { t.Fatalf("create import request: %v", err) } @@ -484,6 +484,8 @@ func TestReportLocalSkillImportResult_IgnoresTimedOutRequests(t *testing.T) { "review-helper", cleanOptionalString(ptr("Timed Out Import")), cleanOptionalString(ptr("Should not be created")), + LocalSkillImportActionCreate, + "", ) if err != nil { t.Fatalf("create import request: %v", err) @@ -534,7 +536,7 @@ func TestReportLocalSkillImportResult_RejectsCrossWorkspaceDaemonToken(t *testin } runtimeID := createRuntimeLocalSkillTestRuntime(t, testUserID) - importReq, err := testHandler.LocalSkillImportStore.Create(context.Background(), runtimeID, testUserID, "review-helper", nil, nil) + importReq, err := testHandler.LocalSkillImportStore.Create(context.Background(), runtimeID, testUserID, "review-helper", nil, nil, LocalSkillImportActionCreate, "") if err != nil { t.Fatalf("create import request: %v", err) } diff --git a/server/internal/handler/skill.go b/server/internal/handler/skill.go index 29372fb89..93d157ab7 100644 --- a/server/internal/handler/skill.go +++ b/server/internal/handler/skill.go @@ -328,6 +328,15 @@ func (h *Handler) canManageSkill(w http.ResponseWriter, r *http.Request, skill d return true } +// canOverwriteSkillByLocalImport reports whether userID may overwrite skill via +// a runtime-local-skill re-import. This is intentionally NARROWER than +// canManageSkill: only the original creator may overwrite by re-importing. +// Workspace owners/admins who want to change a skill they did not create must +// edit it in-app instead. See MUL-2701 / MUL-2800. +func canOverwriteSkillByLocalImport(userID string, skill db.Skill) bool { + return skill.CreatedBy.Valid && uuidToString(skill.CreatedBy) == userID +} + func (h *Handler) UpdateSkill(w http.ResponseWriter, r *http.Request) { id := chi.URLParam(r, "id") skill, ok := h.loadSkillForUser(w, r, id) diff --git a/server/internal/handler/skill_create.go b/server/internal/handler/skill_create.go index 554ef5491..3b782bb30 100644 --- a/server/internal/handler/skill_create.go +++ b/server/internal/handler/skill_create.go @@ -3,7 +3,9 @@ package handler import ( "context" "encoding/json" + "errors" + "github.com/jackc/pgx/v5" "github.com/jackc/pgx/v5/pgtype" db "github.com/multica-ai/multica/server/pkg/db/generated" ) @@ -83,3 +85,113 @@ func (h *Handler) createSkillWithFiles(ctx context.Context, input skillCreateInp return result, nil } + +// errSkillOverwriteNotFound / errSkillOverwriteForbidden are the terminal +// boundary cases of overwriteSkillWithFiles: the target was deleted (or moved +// out of the workspace) or the caller lost overwrite permission between the +// user's confirm and this write. Callers map them to a failed import and must +// NOT fall back to creating a new skill. +var ( + errSkillOverwriteNotFound = errors.New("target skill not found") + errSkillOverwriteForbidden = errors.New("not permitted to overwrite target skill") +) + +type skillOverwriteInput struct { + WorkspaceID pgtype.UUID + TargetSkillID pgtype.UUID + UserID string // re-checked against the skill creator inside the tx + Description string + Content string + Config any + Files []CreateSkillFileRequest +} + +// overwriteSkillWithFiles re-imports a bundle onto an existing skill in a single +// transaction. It re-verifies, inside that tx, that the target still exists in +// the workspace and that UserID may overwrite it (creator-only — see +// canOverwriteSkillByLocalImport). A target deleted or a creator change between +// the user's confirm and this write fails cleanly via errSkillOverwriteNotFound +// / errSkillOverwriteForbidden rather than falling back to create. +// +// Preserved: id, created_by, created_at, name, and agent_skill bindings (the +// row identity and the binding table are never touched). Replaced: description, +// content, config (origin), and the full file set — files absent from the new +// bundle are pruned via DeleteSkillFilesBySkill. On any error the tx rolls back, +// leaving the original skill unchanged. +func (h *Handler) overwriteSkillWithFiles(ctx context.Context, input skillOverwriteInput) (SkillWithFilesResponse, error) { + config, err := json.Marshal(input.Config) + if err != nil { + return SkillWithFilesResponse{}, err + } + if input.Config == nil { + config = []byte("{}") + } + + tx, err := h.TxStarter.Begin(ctx) + if err != nil { + return SkillWithFilesResponse{}, err + } + defer tx.Rollback(ctx) + + qtx := h.Queries.WithTx(tx) + + existing, err := qtx.GetSkillInWorkspace(ctx, db.GetSkillInWorkspaceParams{ + ID: input.TargetSkillID, + WorkspaceID: input.WorkspaceID, + }) + if err != nil { + if errors.Is(err, pgx.ErrNoRows) { + return SkillWithFilesResponse{}, errSkillOverwriteNotFound + } + return SkillWithFilesResponse{}, err + } + if !canOverwriteSkillByLocalImport(input.UserID, existing) { + return SkillWithFilesResponse{}, errSkillOverwriteForbidden + } + + // Name is intentionally left unset (COALESCE keeps the existing name): the + // overwrite targets the same-name skill, so preserving it avoids any + // unique-name churn. + skill, err := qtx.UpdateSkill(ctx, db.UpdateSkillParams{ + ID: existing.ID, + Description: pgtype.Text{String: sanitizeNullBytes(input.Description), Valid: true}, + Content: pgtype.Text{String: sanitizeNullBytes(input.Content), Valid: true}, + Config: config, + }) + if err != nil { + // A committed concurrent DELETE can land between the read above and this + // UPDATE (READ COMMITTED), so UpdateSkill matches 0 rows. Classify it as + // the same "target gone" terminal case rather than a generic failure. + if errors.Is(err, pgx.ErrNoRows) { + return SkillWithFilesResponse{}, errSkillOverwriteNotFound + } + return SkillWithFilesResponse{}, err + } + + // Full replace: drop every existing file, then re-insert the new set so + // files no longer present in the local source are removed. + if err := qtx.DeleteSkillFilesBySkill(ctx, skill.ID); err != nil { + return SkillWithFilesResponse{}, err + } + fileResps := make([]SkillFileResponse, 0, len(input.Files)) + for _, f := range input.Files { + sf, err := qtx.UpsertSkillFile(ctx, db.UpsertSkillFileParams{ + SkillID: skill.ID, + Path: sanitizeNullBytes(f.Path), + Content: sanitizeNullBytes(f.Content), + }) + if err != nil { + return SkillWithFilesResponse{}, err + } + fileResps = append(fileResps, skillFileToResponse(sf)) + } + + if err := tx.Commit(ctx); err != nil { + return SkillWithFilesResponse{}, err + } + + return SkillWithFilesResponse{ + SkillResponse: skillToResponse(skill), + Files: fileResps, + }, nil +} From fe15052b4918acd2bc36d2fd09d03eb8d3ac8b6a Mon Sep 17 00:00:00 2001 From: J Date: Fri, 29 May 2026 11:46:40 +0800 Subject: [PATCH 2/4] fix(skills): gate structured conflict behind client opt-in; guard overwrite target name MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses review feedback on PR #3498 (MUL-2800). Backward compatibility: a same-name local import now returns the new `conflict` status only when the initiating client opts in via `supports_conflict` (an overwrite request implies it). Older clients — already-installed Desktop builds whose poll loop only understands `failed`/`timeout` — keep the legacy `failed` + "a skill with this name already exists" behavior, so upgrading the backend ahead of the client no longer regresses the import UX. This is the installed-app API-compat boundary the repo's CLAUDE.md calls out. Also: the overwrite write path now verifies the incoming effective name matches the target skill's current name (errSkillOverwriteNameMismatch -> failed), preventing a stale/wrong target_skill_id from writing one skill's content onto another. Creator-only + workspace scoping already prevent privilege escalation; this narrows the API so it can't be misused. Refactored LocalSkillImportStore.Create to a LocalSkillImportRequestInput params struct (the signature had grown to 8 positional args; the opt-in flag pushed it over). supports_conflict is persisted in both the in-memory and Redis stores. Tests: conflict tests now opt in; added a legacy-client test (no flag -> failed + legacy message) and an overwrite name-mismatch test. MUL-2800 Co-authored-by: multica-agent --- .../internal/handler/runtime_local_skills.go | 136 ++++++++++++------ .../runtime_local_skills_overwrite_test.go | 63 +++++++- .../runtime_local_skills_redis_store.go | 29 ++-- .../runtime_local_skills_redis_store_test.go | 28 +++- .../handler/runtime_local_skills_test.go | 35 +++-- server/internal/handler/skill_create.go | 25 +++- 6 files changed, 234 insertions(+), 82 deletions(-) diff --git a/server/internal/handler/runtime_local_skills.go b/server/internal/handler/runtime_local_skills.go index 2044466e8..6aa6bc727 100644 --- a/server/internal/handler/runtime_local_skills.go +++ b/server/internal/handler/runtime_local_skills.go @@ -92,11 +92,28 @@ type LocalSkillListStore interface { Fail(ctx context.Context, id string, errMsg string) error } +// LocalSkillImportRequestInput carries the fields needed to enqueue a +// runtime-local-skill import. SupportsConflict gates the structured-conflict +// contract: only clients that opt in receive the `conflict` terminal status; +// older clients keep the legacy `failed` ("a skill with this name already +// exists") behavior so an already-installed Desktop build doesn't regress when +// it talks to an upgraded backend. See MUL-2800. +type LocalSkillImportRequestInput struct { + RuntimeID string + CreatorID string + SkillKey string + Name *string + Description *string + Action LocalSkillImportAction + TargetSkillID string + SupportsConflict bool +} + // LocalSkillImportStore is the same contract as LocalSkillListStore but for // runtime-local-skill import requests. Kept as a separate interface because the // Create signature carries import-specific fields (skill_key, optional rename). type LocalSkillImportStore interface { - Create(ctx context.Context, runtimeID, creatorID, skillKey string, name, description *string, action LocalSkillImportAction, targetSkillID string) (*RuntimeLocalSkillImportRequest, error) + Create(ctx context.Context, input LocalSkillImportRequestInput) (*RuntimeLocalSkillImportRequest, error) Get(ctx context.Context, id string) (*RuntimeLocalSkillImportRequest, error) HasPending(ctx context.Context, runtimeID string) (bool, error) PopPending(ctx context.Context, runtimeID string) (*RuntimeLocalSkillImportRequest, error) @@ -176,21 +193,25 @@ type RuntimeLocalSkillListRequest struct { } type RuntimeLocalSkillImportRequest struct { - ID string `json:"id"` - RuntimeID string `json:"runtime_id"` - SkillKey string `json:"skill_key"` - Name *string `json:"name,omitempty"` - Description *string `json:"description,omitempty"` - Action LocalSkillImportAction `json:"action,omitempty"` - TargetSkillID string `json:"target_skill_id,omitempty"` - Status RuntimeLocalSkillRequestStatus `json:"status"` - Skill *SkillResponse `json:"skill,omitempty"` - Conflict *LocalSkillImportConflict `json:"conflict,omitempty"` - Error string `json:"error,omitempty"` - CreatedAt time.Time `json:"created_at"` - UpdatedAt time.Time `json:"updated_at"` - CreatorID string `json:"-"` - RunStartedAt *time.Time `json:"-"` + ID string `json:"id"` + RuntimeID string `json:"runtime_id"` + SkillKey string `json:"skill_key"` + Name *string `json:"name,omitempty"` + Description *string `json:"description,omitempty"` + Action LocalSkillImportAction `json:"action,omitempty"` + TargetSkillID string `json:"target_skill_id,omitempty"` + // SupportsConflict records whether the initiating client opted into the + // structured-conflict contract; consulted at report time to decide between + // the new `conflict` status and the legacy `failed` behavior. + SupportsConflict bool `json:"supports_conflict,omitempty"` + Status RuntimeLocalSkillRequestStatus `json:"status"` + Skill *SkillResponse `json:"skill,omitempty"` + Conflict *LocalSkillImportConflict `json:"conflict,omitempty"` + Error string `json:"error,omitempty"` + CreatedAt time.Time `json:"created_at"` + UpdatedAt time.Time `json:"updated_at"` + CreatorID string `json:"-"` + RunStartedAt *time.Time `json:"-"` } // InMemoryLocalSkillListStore is the single-node implementation — good enough @@ -313,7 +334,7 @@ func NewInMemoryLocalSkillImportStore() *InMemoryLocalSkillImportStore { return &InMemoryLocalSkillImportStore{requests: make(map[string]*RuntimeLocalSkillImportRequest)} } -func (s *InMemoryLocalSkillImportStore) Create(_ context.Context, runtimeID, creatorID, skillKey string, name, description *string, action LocalSkillImportAction, targetSkillID string) (*RuntimeLocalSkillImportRequest, error) { +func (s *InMemoryLocalSkillImportStore) Create(_ context.Context, input LocalSkillImportRequestInput) (*RuntimeLocalSkillImportRequest, error) { s.mu.Lock() defer s.mu.Unlock() @@ -324,17 +345,18 @@ func (s *InMemoryLocalSkillImportStore) Create(_ context.Context, runtimeID, cre } req := &RuntimeLocalSkillImportRequest{ - ID: randomID(), - RuntimeID: runtimeID, - SkillKey: skillKey, - Name: name, - Description: description, - Action: action, - TargetSkillID: targetSkillID, - Status: RuntimeLocalSkillPending, - CreatedAt: time.Now(), - UpdatedAt: time.Now(), - CreatorID: creatorID, + ID: randomID(), + RuntimeID: input.RuntimeID, + SkillKey: input.SkillKey, + Name: input.Name, + Description: input.Description, + Action: input.Action, + TargetSkillID: input.TargetSkillID, + SupportsConflict: input.SupportsConflict, + Status: RuntimeLocalSkillPending, + CreatedAt: time.Now(), + UpdatedAt: time.Now(), + CreatorID: input.CreatorID, } s.requests[req.ID] = req return req, nil @@ -467,6 +489,10 @@ type CreateRuntimeLocalSkillImportRequest struct { // TargetSkillID must reference the existing same-name skill. Action LocalSkillImportAction `json:"action,omitempty"` TargetSkillID string `json:"target_skill_id,omitempty"` + // SupportsConflict opts the client into the structured-conflict contract. + // Omit it (older clients) to keep the legacy `failed` behavior on a + // same-name collision. An overwrite request implies the new contract. + SupportsConflict bool `json:"supports_conflict,omitempty"` } type reportedRuntimeLocalSkill struct { @@ -616,16 +642,18 @@ func (h *Handler) InitiateImportLocalSkill(w http.ResponseWriter, r *http.Reques return } - importReq, err := h.LocalSkillImportStore.Create( - r.Context(), - rt.runtimeID, - creatorID, - strings.TrimSpace(req.SkillKey), - cleanOptionalString(req.Name), - cleanOptionalString(req.Description), - req.Action, - targetSkillID, - ) + importReq, err := h.LocalSkillImportStore.Create(r.Context(), LocalSkillImportRequestInput{ + RuntimeID: rt.runtimeID, + CreatorID: creatorID, + SkillKey: strings.TrimSpace(req.SkillKey), + Name: cleanOptionalString(req.Name), + Description: cleanOptionalString(req.Description), + Action: req.Action, + TargetSkillID: targetSkillID, + // An overwrite request is inherently a new-client action, so it implies + // the structured-conflict contract even if the flag is omitted. + SupportsConflict: req.SupportsConflict || req.Action == LocalSkillImportActionOverwrite, + }) if err != nil { writeError(w, http.StatusInternalServerError, "failed to enqueue local skill import: "+err.Error()) return @@ -808,6 +836,7 @@ func (h *Handler) ReportLocalSkillImportResult(w http.ResponseWriter, r *http.Re WorkspaceID: rt.WorkspaceID, TargetSkillID: targetUUID, UserID: req.CreatorID, + ExpectedName: sanitizeNullBytes(name), Description: description, Content: body.Skill.Content, Config: config, @@ -820,6 +849,8 @@ func (h *Handler) ReportLocalSkillImportResult(w http.ResponseWriter, r *http.Re failMsg = "target skill no longer exists" case errors.Is(oerr, errSkillOverwriteForbidden): failMsg = "you no longer have permission to overwrite this skill" + case errors.Is(oerr, errSkillOverwriteNameMismatch): + failMsg = "target skill name no longer matches the imported skill" } h.failLocalSkillImport(w, r, requestID, failMsg) return @@ -840,14 +871,15 @@ func (h *Handler) ReportLocalSkillImportResult(w http.ResponseWriter, r *http.Re return } - // Create path: detect a same-name conflict before writing. A conflict is a - // structured terminal state (not a failure) so the caller can offer - // overwrite / rename / skip. + // Create path: detect a same-name conflict before writing. For opted-in + // clients this is a structured terminal state (not a failure) so the caller + // can offer overwrite / rename / skip; older clients keep the legacy + // `failed` behavior (see resolveLocalSkillConflict). if existing, found, lerr := h.lookupSkillByName(r.Context(), rt.WorkspaceID, sanitizeNullBytes(name)); lerr != nil { h.failLocalSkillImport(w, r, requestID, "failed to check for existing skill: "+lerr.Error()) return } else if found { - h.reportLocalSkillConflict(w, r, requestID, req.CreatorID, existing) + h.resolveLocalSkillConflict(w, r, req, existing) return } @@ -865,9 +897,13 @@ func (h *Handler) ReportLocalSkillImportResult(w http.ResponseWriter, r *http.Re // lookup and the insert — surface it as a conflict, not a hard failure. if isUniqueViolation(err) { if existing, found, lerr := h.lookupSkillByName(r.Context(), rt.WorkspaceID, sanitizeNullBytes(name)); lerr == nil && found { - h.reportLocalSkillConflict(w, r, requestID, req.CreatorID, existing) + h.resolveLocalSkillConflict(w, r, req, existing) return } + // Lost the row again (deleted between insert-fail and re-lookup): + // fall through to the legacy unique-violation message. + h.failLocalSkillImport(w, r, requestID, "a skill with this name already exists") + return } h.failLocalSkillImport(w, r, requestID, err.Error()) return @@ -907,6 +943,20 @@ func (h *Handler) failLocalSkillImport(w http.ResponseWriter, r *http.Request, r writeJSON(w, http.StatusOK, map[string]string{"status": "ok"}) } +// resolveLocalSkillConflict terminates a same-name create import. Clients that +// opted into the structured-conflict contract (SupportsConflict) receive the +// `conflict` status plus metadata so they can offer overwrite / rename / skip; +// older clients keep the legacy `failed` ("a skill with this name already +// exists") behavior so an installed Desktop build that predates the contract +// doesn't regress when it hits an upgraded backend. +func (h *Handler) resolveLocalSkillConflict(w http.ResponseWriter, r *http.Request, req *RuntimeLocalSkillImportRequest, existing db.Skill) { + if req.SupportsConflict { + h.reportLocalSkillConflict(w, r, req.ID, req.CreatorID, existing) + return + } + h.failLocalSkillImport(w, r, req.ID, "a skill with this name already exists") +} + // reportLocalSkillConflict records a same-name conflict as the terminal // RuntimeLocalSkillConflict state with structured metadata the caller uses to // offer overwrite / rename / skip. diff --git a/server/internal/handler/runtime_local_skills_overwrite_test.go b/server/internal/handler/runtime_local_skills_overwrite_test.go index d59ad1e95..bf467e207 100644 --- a/server/internal/handler/runtime_local_skills_overwrite_test.go +++ b/server/internal/handler/runtime_local_skills_overwrite_test.go @@ -182,7 +182,7 @@ func TestRuntimeLocalSkillImport_ConflictCreatorCanOverwrite(t *testing.T) { existingID := createImportTargetSkill(t, name, testUserID, nil) got := runLocalSkillImport(t, runtimeID, - map[string]any{"skill_key": "review-helper"}, + map[string]any{"skill_key": "review-helper", "supports_conflict": true}, reportBundleBody(name, "incoming description", "# incoming", map[string]string{"a.md": "A"}), ) @@ -221,7 +221,7 @@ func TestRuntimeLocalSkillImport_ConflictNonCreatorCannotOverwrite(t *testing.T) existingID := createImportTargetSkill(t, name, otherUserID, nil) got := runLocalSkillImport(t, runtimeID, - map[string]any{"skill_key": "review-helper"}, + map[string]any{"skill_key": "review-helper", "supports_conflict": true}, reportBundleBody(name, "incoming description", "# incoming", nil), ) @@ -373,3 +373,62 @@ func TestRuntimeLocalSkillImport_OverwriteRetryIsIdempotent(t *testing.T) { t.Fatalf("retry must not re-write files, got %d files", n) } } + +// TestRuntimeLocalSkillImport_LegacyClientGetsFailedOnConflict verifies the +// installed-app compatibility gate: a client that does NOT opt into the +// structured-conflict contract keeps the legacy `failed` + "already exists" +// behavior on a same-name collision, instead of the new `conflict` status its +// older poll loop wouldn't understand. +func TestRuntimeLocalSkillImport_LegacyClientGetsFailedOnConflict(t *testing.T) { + if testHandler == nil { + t.Skip("database not available") + } + + runtimeID := createRuntimeLocalSkillTestRuntime(t, testUserID) + name := fmt.Sprintf("legacy-conflict-%d", time.Now().UnixNano()) + createImportTargetSkill(t, name, testUserID, nil) + + got := runLocalSkillImport(t, runtimeID, + // No supports_conflict (and no action) — an old client. + map[string]any{"skill_key": "review-helper"}, + reportBundleBody(name, "incoming description", "# incoming", nil), + ) + + if got.Status != RuntimeLocalSkillFailed { + t.Fatalf("status = %s, want failed (legacy contract)", got.Status) + } + if got.Conflict != nil { + t.Fatalf("legacy client must not receive structured conflict metadata: %+v", got.Conflict) + } + if got.Error != "a skill with this name already exists" { + t.Fatalf("error = %q, want legacy already-exists message", got.Error) + } +} + +// TestRuntimeLocalSkillImport_OverwriteNameMismatchFails verifies the guard +// against a stale / wrong target_skill_id: if the target's name no longer +// matches the imported skill, the overwrite fails instead of writing one +// skill's content onto another. +func TestRuntimeLocalSkillImport_OverwriteNameMismatchFails(t *testing.T) { + if testHandler == nil { + t.Skip("database not available") + } + + runtimeID := createRuntimeLocalSkillTestRuntime(t, testUserID) + targetName := fmt.Sprintf("overwrite-target-%d", time.Now().UnixNano()) + otherName := fmt.Sprintf("overwrite-other-%d", time.Now().UnixNano()) + targetID := createImportTargetSkill(t, targetName, testUserID, nil) + + // Overwrite targets targetID but the imported bundle is named otherName. + got := runLocalSkillImport(t, runtimeID, + map[string]any{"skill_key": "review-helper", "action": "overwrite", "target_skill_id": targetID}, + reportBundleBody(otherName, "incoming description", "# incoming", map[string]string{"a.md": "A"}), + ) + + if got.Status != RuntimeLocalSkillFailed { + t.Fatalf("status = %s, want failed (name mismatch)", got.Status) + } + if _, desc, _, _ := getSkillRow(t, targetID); desc != "original description" { + t.Fatalf("name-mismatch overwrite must not mutate the target, description = %q", desc) + } +} diff --git a/server/internal/handler/runtime_local_skills_redis_store.go b/server/internal/handler/runtime_local_skills_redis_store.go index eb3fd0b4c..b67c4eaaf 100644 --- a/server/internal/handler/runtime_local_skills_redis_store.go +++ b/server/internal/handler/runtime_local_skills_redis_store.go @@ -262,20 +262,21 @@ func NewRedisLocalSkillImportStore(rdb *redis.Client) *RedisLocalSkillImportStor return &RedisLocalSkillImportStore{rdb: rdb} } -func (s *RedisLocalSkillImportStore) Create(ctx context.Context, runtimeID, creatorID, skillKey string, name, description *string, action LocalSkillImportAction, targetSkillID string) (*RuntimeLocalSkillImportRequest, error) { +func (s *RedisLocalSkillImportStore) Create(ctx context.Context, input LocalSkillImportRequestInput) (*RuntimeLocalSkillImportRequest, error) { now := time.Now() req := &RuntimeLocalSkillImportRequest{ - ID: randomID(), - RuntimeID: runtimeID, - SkillKey: skillKey, - Name: name, - Description: description, - Action: action, - TargetSkillID: targetSkillID, - Status: RuntimeLocalSkillPending, - CreatedAt: now, - UpdatedAt: now, - CreatorID: creatorID, + ID: randomID(), + RuntimeID: input.RuntimeID, + SkillKey: input.SkillKey, + Name: input.Name, + Description: input.Description, + Action: input.Action, + TargetSkillID: input.TargetSkillID, + SupportsConflict: input.SupportsConflict, + Status: RuntimeLocalSkillPending, + CreatedAt: now, + UpdatedAt: now, + CreatorID: input.CreatorID, } data, err := s.marshalImport(req) if err != nil { @@ -284,11 +285,11 @@ func (s *RedisLocalSkillImportStore) Create(ctx context.Context, runtimeID, crea pipe := s.rdb.TxPipeline() pipe.Set(ctx, localSkillImportKey(req.ID), data, runtimeLocalSkillStoreRetention) - pipe.ZAdd(ctx, localSkillImportPendingKey(runtimeID), redis.Z{ + pipe.ZAdd(ctx, localSkillImportPendingKey(input.RuntimeID), redis.Z{ Score: float64(now.UnixNano()), Member: req.ID, }) - pipe.Expire(ctx, localSkillImportPendingKey(runtimeID), runtimeLocalSkillStoreRetention*2) + pipe.Expire(ctx, localSkillImportPendingKey(input.RuntimeID), runtimeLocalSkillStoreRetention*2) if _, err := pipe.Exec(ctx); err != nil { return nil, fmt.Errorf("persist import request: %w", err) } diff --git a/server/internal/handler/runtime_local_skills_redis_store_test.go b/server/internal/handler/runtime_local_skills_redis_store_test.go index 8c12a0503..2faaea130 100644 --- a/server/internal/handler/runtime_local_skills_redis_store_test.go +++ b/server/internal/handler/runtime_local_skills_redis_store_test.go @@ -225,7 +225,15 @@ func TestRedisLocalSkillImportStore_PreservesCreatorID(t *testing.T) { name := "Review Helper" desc := "Desc" - req, err := store.Create(ctx, "runtime-1", "user-42", "review-helper", &name, &desc, LocalSkillImportActionOverwrite, "target-skill-99") + req, err := store.Create(ctx, LocalSkillImportRequestInput{ + RuntimeID: "runtime-1", + CreatorID: "user-42", + SkillKey: "review-helper", + Name: &name, + Description: &desc, + Action: LocalSkillImportActionOverwrite, + TargetSkillID: "target-skill-99", + }) if err != nil { t.Fatalf("create: %v", err) } @@ -264,7 +272,11 @@ func TestRedisLocalSkillImportStore_PreservesConflict(t *testing.T) { ctx := context.Background() store := NewRedisLocalSkillImportStore(rdb) - req, err := store.Create(ctx, "runtime-1", "user-1", "review-helper", nil, nil, LocalSkillImportActionCreate, "") + req, err := store.Create(ctx, LocalSkillImportRequestInput{ + RuntimeID: "runtime-1", + CreatorID: "user-1", + SkillKey: "review-helper", + }) if err != nil { t.Fatalf("create: %v", err) } @@ -295,7 +307,11 @@ func TestRedisLocalSkillImportStore_PopPendingAcrossInstances(t *testing.T) { nodeA := NewRedisLocalSkillImportStore(rdb) nodeB := NewRedisLocalSkillImportStore(rdb) - req, err := nodeA.Create(ctx, "runtime-import", "user-1", "review-helper", nil, nil, LocalSkillImportActionCreate, "") + req, err := nodeA.Create(ctx, LocalSkillImportRequestInput{ + RuntimeID: "runtime-import", + CreatorID: "user-1", + SkillKey: "review-helper", + }) if err != nil { t.Fatalf("create: %v", err) } @@ -402,7 +418,11 @@ func TestRedisLocalSkillImportStore_PopPendingBatch(t *testing.T) { // Create 5 pending imports. ids := make([]string, 5) for i := range ids { - req, err := store.Create(ctx, "runtime-batch", "user-1", fmt.Sprintf("skill-%d", i), nil, nil, LocalSkillImportActionCreate, "") + req, err := store.Create(ctx, LocalSkillImportRequestInput{ + RuntimeID: "runtime-batch", + CreatorID: "user-1", + SkillKey: fmt.Sprintf("skill-%d", i), + }) if err != nil { t.Fatalf("create %d: %v", i, err) } diff --git a/server/internal/handler/runtime_local_skills_test.go b/server/internal/handler/runtime_local_skills_test.go index 2f1c52763..d92afe634 100644 --- a/server/internal/handler/runtime_local_skills_test.go +++ b/server/internal/handler/runtime_local_skills_test.go @@ -187,7 +187,11 @@ func TestInMemoryLocalSkillListStore_TimesOutRunningRequests(t *testing.T) { func TestInMemoryLocalSkillImportStore_TimesOutRunningRequests(t *testing.T) { ctx := context.Background() store := NewInMemoryLocalSkillImportStore() - req, err := store.Create(ctx, "runtime-xyz", "user-1", "review-helper", nil, nil, LocalSkillImportActionCreate, "") + req, err := store.Create(ctx, LocalSkillImportRequestInput{ + RuntimeID: "runtime-xyz", + CreatorID: "user-1", + SkillKey: "review-helper", + }) if err != nil { t.Fatalf("create: %v", err) } @@ -237,7 +241,11 @@ func TestGetLocalSkillImportRequest_RequiresRuntimeOwner(t *testing.T) { runtimeID := createRuntimeLocalSkillTestRuntime(t, testUserID) adminUserID := createRuntimeLocalSkillTestMember(t, "admin") - importReq, err := testHandler.LocalSkillImportStore.Create(context.Background(), runtimeID, testUserID, "review-helper", nil, nil, LocalSkillImportActionCreate, "") + importReq, err := testHandler.LocalSkillImportStore.Create(context.Background(), LocalSkillImportRequestInput{ + RuntimeID: runtimeID, + CreatorID: testUserID, + SkillKey: "review-helper", + }) if err != nil { t.Fatalf("create import request: %v", err) } @@ -477,16 +485,13 @@ func TestReportLocalSkillImportResult_IgnoresTimedOutRequests(t *testing.T) { runtimeID := createRuntimeLocalSkillTestRuntime(t, testUserID) ctx := context.Background() - importReq, err := testHandler.LocalSkillImportStore.Create( - ctx, - runtimeID, - testUserID, - "review-helper", - cleanOptionalString(ptr("Timed Out Import")), - cleanOptionalString(ptr("Should not be created")), - LocalSkillImportActionCreate, - "", - ) + importReq, err := testHandler.LocalSkillImportStore.Create(ctx, LocalSkillImportRequestInput{ + RuntimeID: runtimeID, + CreatorID: testUserID, + SkillKey: "review-helper", + Name: cleanOptionalString(ptr("Timed Out Import")), + Description: cleanOptionalString(ptr("Should not be created")), + }) if err != nil { t.Fatalf("create import request: %v", err) } @@ -536,7 +541,11 @@ func TestReportLocalSkillImportResult_RejectsCrossWorkspaceDaemonToken(t *testin } runtimeID := createRuntimeLocalSkillTestRuntime(t, testUserID) - importReq, err := testHandler.LocalSkillImportStore.Create(context.Background(), runtimeID, testUserID, "review-helper", nil, nil, LocalSkillImportActionCreate, "") + importReq, err := testHandler.LocalSkillImportStore.Create(context.Background(), LocalSkillImportRequestInput{ + RuntimeID: runtimeID, + CreatorID: testUserID, + SkillKey: "review-helper", + }) if err != nil { t.Fatalf("create import request: %v", err) } diff --git a/server/internal/handler/skill_create.go b/server/internal/handler/skill_create.go index 3b782bb30..28385edf2 100644 --- a/server/internal/handler/skill_create.go +++ b/server/internal/handler/skill_create.go @@ -92,18 +92,24 @@ func (h *Handler) createSkillWithFiles(ctx context.Context, input skillCreateInp // user's confirm and this write. Callers map them to a failed import and must // NOT fall back to creating a new skill. var ( - errSkillOverwriteNotFound = errors.New("target skill not found") - errSkillOverwriteForbidden = errors.New("not permitted to overwrite target skill") + errSkillOverwriteNotFound = errors.New("target skill not found") + errSkillOverwriteForbidden = errors.New("not permitted to overwrite target skill") + errSkillOverwriteNameMismatch = errors.New("target skill name does not match the imported skill") ) type skillOverwriteInput struct { WorkspaceID pgtype.UUID TargetSkillID pgtype.UUID UserID string // re-checked against the skill creator inside the tx - Description string - Content string - Config any - Files []CreateSkillFileRequest + // ExpectedName, when non-empty, must equal the target's current name. Guards + // against a client sending the wrong target_skill_id and overwriting a + // different skill than the one the conflict dialog showed the user. The + // caller passes the sanitized effective import name. + ExpectedName string + Description string + Content string + Config any + Files []CreateSkillFileRequest } // overwriteSkillWithFiles re-imports a bundle onto an existing skill in a single @@ -148,6 +154,13 @@ func (h *Handler) overwriteSkillWithFiles(ctx context.Context, input skillOverwr if !canOverwriteSkillByLocalImport(input.UserID, existing) { return SkillWithFilesResponse{}, errSkillOverwriteForbidden } + // The overwrite is keyed on target_skill_id, but the conflict the user + // confirmed was a same-name collision; reject if the target's name no longer + // matches the imported skill so a stale/wrong target_skill_id can't write + // one skill's content onto another. + if input.ExpectedName != "" && existing.Name != input.ExpectedName { + return SkillWithFilesResponse{}, errSkillOverwriteNameMismatch + } // Name is intentionally left unset (COALESCE keeps the existing name): the // overwrite targets the same-name skill, so preserving it avoids any From c8bf74c1f35f909961298a9a0927e0e8a060ed32 Mon Sep 17 00:00:00 2001 From: J Date: Wed, 10 Jun 2026 15:09:57 +0800 Subject: [PATCH 3/4] feat(skills): resolve local import conflicts in desktop Co-authored-by: multica-agent --- packages/core/runtimes/local-skills.ts | 15 +- packages/core/types/agent.ts | 20 +- packages/core/types/index.ts | 2 + packages/views/locales/en/skills.json | 22 +- packages/views/locales/zh-Hans/skills.json | 22 +- .../runtime-local-skill-import-panel.test.tsx | 86 ++- .../runtime-local-skill-import-panel.tsx | 520 ++++++++++++++++-- 7 files changed, 619 insertions(+), 68 deletions(-) diff --git a/packages/core/runtimes/local-skills.ts b/packages/core/runtimes/local-skills.ts index 20f7fe0b3..f0c711f84 100644 --- a/packages/core/runtimes/local-skills.ts +++ b/packages/core/runtimes/local-skills.ts @@ -67,6 +67,16 @@ export async function resolveRuntimeLocalSkillImport( current = await api.getImportLocalSkillResult(runtimeId, initial.id); } + if (current.status === "conflict") { + if (!current.conflict) { + throw new Error("runtime local skill import conflict missing details"); + } + return { + status: "conflict", + conflict: current.conflict, + }; + } + if (current.status === "failed" || current.status === "timeout") { throw new Error(current.error || "runtime local skill import failed"); } @@ -74,7 +84,10 @@ export async function resolveRuntimeLocalSkillImport( throw new Error("runtime local skill import did not return a skill"); } - return { skill: current.skill }; + return { + status: current.action === "overwrite" ? "updated" : "created", + skill: current.skill, + }; } export function runtimeLocalSkillsOptions(runtimeId: string | null | undefined) { diff --git a/packages/core/types/agent.ts b/packages/core/types/agent.ts index 6f0bce8da..5211e39df 100644 --- a/packages/core/types/agent.ts +++ b/packages/core/types/agent.ts @@ -615,9 +615,18 @@ export type RuntimeLocalSkillStatus = | "pending" | "running" | "completed" + | "conflict" | "failed" | "timeout"; +export type RuntimeLocalSkillImportAction = "overwrite"; + +export interface RuntimeLocalSkillImportConflict { + existing_skill_id: string; + existing_created_by?: string; + can_overwrite: boolean; +} + export interface RuntimeLocalSkillSummary { key: string; name: string; @@ -642,6 +651,9 @@ export interface CreateRuntimeLocalSkillImportRequest { skill_key: string; name?: string; description?: string; + action?: RuntimeLocalSkillImportAction; + target_skill_id?: string; + supports_conflict?: boolean; } export interface RuntimeLocalSkillImportRequest { @@ -650,8 +662,12 @@ export interface RuntimeLocalSkillImportRequest { skill_key: string; name?: string; description?: string; + action?: RuntimeLocalSkillImportAction; + target_skill_id?: string; + supports_conflict?: boolean; status: RuntimeLocalSkillStatus; skill?: Skill; + conflict?: RuntimeLocalSkillImportConflict; error?: string; created_at: string; updated_at: string; @@ -663,5 +679,7 @@ export interface RuntimeLocalSkillsResult { } export interface RuntimeLocalSkillImportResult { - skill: Skill; + status: "created" | "updated" | "conflict"; + skill?: Skill; + conflict?: RuntimeLocalSkillImportConflict; } diff --git a/packages/core/types/index.ts b/packages/core/types/index.ts index e1f8cc893..adae860c0 100644 --- a/packages/core/types/index.ts +++ b/packages/core/types/index.ts @@ -44,6 +44,8 @@ export type { RuntimeModelListStatus, RuntimeModelsResult, RuntimeLocalSkillStatus, + RuntimeLocalSkillImportAction, + RuntimeLocalSkillImportConflict, RuntimeLocalSkillSummary, RuntimeLocalSkillListRequest, CreateRuntimeLocalSkillImportRequest, diff --git a/packages/views/locales/en/skills.json b/packages/views/locales/en/skills.json index de6a32b0b..628790141 100644 --- a/packages/views/locales/en/skills.json +++ b/packages/views/locales/en/skills.json @@ -231,9 +231,27 @@ "bulk_complete_hint": "Import complete.", "bulk_cancelled_hint": "Import cancelled.", "bulk_done_button": "Done", - "bulk_summary_imported": "Imported", + "bulk_summary_created": "Created", + "bulk_summary_updated": "Updated", + "bulk_summary_conflicts": "Conflicts", "bulk_summary_skipped": "Skipped", - "bulk_summary_failed": "Failed" + "bulk_summary_failed": "Failed", + "conflict_single_title": "A skill with this name already exists", + "conflict_bulk_title": "{{count}} skills need a decision", + "conflict_hint": "Choose whether to overwrite the existing workspace skill, import with a new name, or skip it.", + "conflict_locked_creator": "This skill was created by {{creator}}. Only the creator can overwrite it from local import; edit it from the Skill detail page if you need to change it.", + "conflict_locked": "Only the creator can overwrite this skill from local import; edit it from the Skill detail page if you need to change it.", + "conflict_overwrite": "Overwrite", + "conflict_rename": "Rename", + "conflict_skip": "Skip", + "conflict_cancel": "Cancel", + "conflict_overwrite_all": "Overwrite all", + "conflict_skip_all": "Skip all", + "conflict_rename_label": "New skill name", + "conflict_footer": "{{count}} conflict decisions pending.", + "conflict_apply_button": "Apply decisions", + "conflict_missing_resolution": "Choose how to resolve this conflict.", + "conflict_name_still_exists": "That name already exists. Choose another name or skip." }, "file_tree": { "no_files": "No files" diff --git a/packages/views/locales/zh-Hans/skills.json b/packages/views/locales/zh-Hans/skills.json index 7beacc57b..484cb4be6 100644 --- a/packages/views/locales/zh-Hans/skills.json +++ b/packages/views/locales/zh-Hans/skills.json @@ -243,9 +243,27 @@ "bulk_complete_hint": "导入完成。", "bulk_cancelled_hint": "导入已取消。", "bulk_done_button": "完成", - "bulk_summary_imported": "已导入", + "bulk_summary_created": "已创建", + "bulk_summary_updated": "已更新", + "bulk_summary_conflicts": "冲突", "bulk_summary_skipped": "已跳过", - "bulk_summary_failed": "失败" + "bulk_summary_failed": "失败", + "conflict_single_title": "工作区里已存在同名 skill", + "conflict_bulk_title": "{{count}} 个 skill 需要处理冲突", + "conflict_hint": "选择覆盖现有工作区 skill、改名导入，或跳过。", + "conflict_locked_creator": "该 skill 由 {{creator}} 创建，只能由创建者通过本地导入覆盖；如需修改请到 Skill 详情页编辑。", + "conflict_locked": "该 skill 只能由创建者通过本地导入覆盖；如需修改请到 Skill 详情页编辑。", + "conflict_overwrite": "覆盖", + "conflict_rename": "改名", + "conflict_skip": "跳过", + "conflict_cancel": "取消", + "conflict_overwrite_all": "全部覆盖", + "conflict_skip_all": "全部跳过", + "conflict_rename_label": "新的 skill 名称", + "conflict_footer": "还有 {{count}} 个冲突决策待处理。", + "conflict_apply_button": "应用决策", + "conflict_missing_resolution": "请选择如何处理这个冲突。", + "conflict_name_still_exists": "这个名称仍然已存在。请换一个名称或跳过。" }, "file_tree": { "no_files": "无文件" diff --git a/packages/views/skills/components/runtime-local-skill-import-panel.test.tsx b/packages/views/skills/components/runtime-local-skill-import-panel.test.tsx index bff9953fe..b4a3994eb 100644 --- a/packages/views/skills/components/runtime-local-skill-import-panel.test.tsx +++ b/packages/views/skills/components/runtime-local-skill-import-panel.test.tsx @@ -148,6 +148,7 @@ describe("RuntimeLocalSkillImportPanel", () => { }), }); mockResolveRuntimeLocalSkillImport.mockResolvedValue({ + status: "created", skill: MOCK_IMPORTED_SKILL_A, }); }); @@ -183,6 +184,7 @@ describe("RuntimeLocalSkillImportPanel", () => { skill_key: "review-helper", name: "Review Helper", description: "Review pull requests", + supports_conflict: true, }, ); }, @@ -200,8 +202,8 @@ describe("RuntimeLocalSkillImportPanel", () => { }), }); mockResolveRuntimeLocalSkillImport - .mockResolvedValueOnce({ skill: MOCK_IMPORTED_SKILL_A }) - .mockResolvedValueOnce({ skill: MOCK_IMPORTED_SKILL_B }); + .mockResolvedValueOnce({ status: "created", skill: MOCK_IMPORTED_SKILL_A }) + .mockResolvedValueOnce({ status: "created", skill: MOCK_IMPORTED_SKILL_B }); renderPanel(); @@ -240,8 +242,8 @@ describe("RuntimeLocalSkillImportPanel", () => { expect(mockResolveRuntimeLocalSkillImport).toHaveBeenCalledTimes(2); - // Verify summary shows both as imported - expect(screen.getByText("Imported")).toBeInTheDocument(); + // Verify summary shows both as created + expect(screen.getByText("Created")).toBeInTheDocument(); }); it("handles partial failures gracefully", async () => { @@ -254,7 +256,7 @@ describe("RuntimeLocalSkillImportPanel", () => { }), }); mockResolveRuntimeLocalSkillImport - .mockResolvedValueOnce({ skill: MOCK_IMPORTED_SKILL_A }) + .mockResolvedValueOnce({ status: "created", skill: MOCK_IMPORTED_SKILL_A }) .mockRejectedValueOnce(new Error("409 conflict: already exists")); renderPanel(); @@ -291,8 +293,8 @@ describe("RuntimeLocalSkillImportPanel", () => { { timeout: 10000 }, ); - // Summary should show imported and skipped - expect(screen.getByText("Imported")).toBeInTheDocument(); + // Summary should show created and skipped + expect(screen.getByText("Created")).toBeInTheDocument(); expect(screen.getByText("Skipped")).toBeInTheDocument(); }); @@ -344,8 +346,8 @@ describe("RuntimeLocalSkillImportPanel", () => { }), }); mockResolveRuntimeLocalSkillImport - .mockResolvedValueOnce({ skill: MOCK_IMPORTED_SKILL_A }) - .mockResolvedValueOnce({ skill: MOCK_IMPORTED_SKILL_B }); + .mockResolvedValueOnce({ status: "created", skill: MOCK_IMPORTED_SKILL_A }) + .mockResolvedValueOnce({ status: "created", skill: MOCK_IMPORTED_SKILL_B }); const onImported = vi.fn(); const onBulkDone = vi.fn(); @@ -385,4 +387,70 @@ describe("RuntimeLocalSkillImportPanel", () => { expect(onBulkDone).toHaveBeenCalledTimes(1); expect(onImported).not.toHaveBeenCalled(); }); + + it("resolves a creator conflict by overwriting the existing skill", async () => { + mockResolveRuntimeLocalSkillImport + .mockResolvedValueOnce({ + status: "conflict", + conflict: { + existing_skill_id: "existing-skill-1", + existing_created_by: "user-1", + can_overwrite: true, + }, + }) + .mockResolvedValueOnce({ + status: "updated", + skill: { + ...MOCK_IMPORTED_SKILL_A, + id: "existing-skill-1", + }, + }); + + renderPanel(); + + expect( + await screen.findByText("Review Helper", {}, { timeout: 5000 }), + ).toBeInTheDocument(); + + fireEvent.click(screen.getByRole("button", { name: /Review Helper/i })); + + const importButton = screen.getByRole("button", { + name: /Import to Workspace/i, + }); + await waitFor(() => expect(importButton).not.toBeDisabled(), { + timeout: 5000, + }); + fireEvent.click(importButton); + + expect( + await screen.findByText(/A skill with this name already exists/i), + ).toBeInTheDocument(); + + const applyButton = screen.getByRole("button", { + name: /Apply decisions/i, + }); + await waitFor(() => expect(applyButton).not.toBeDisabled(), { + timeout: 5000, + }); + fireEvent.click(applyButton); + + await waitFor( + () => { + expect(mockResolveRuntimeLocalSkillImport).toHaveBeenLastCalledWith( + "runtime-1", + { + skill_key: "review-helper", + name: "Review Helper", + description: "Review pull requests", + supports_conflict: true, + action: "overwrite", + target_skill_id: "existing-skill-1", + }, + ); + }, + { timeout: 5000 }, + ); + + expect(await screen.findByText("Updated")).toBeInTheDocument(); + }); }); diff --git a/packages/views/skills/components/runtime-local-skill-import-panel.tsx b/packages/views/skills/components/runtime-local-skill-import-panel.tsx index 88ea8bca1..13216441c 100644 --- a/packages/views/skills/components/runtime-local-skill-import-panel.tsx +++ b/packages/views/skills/components/runtime-local-skill-import-panel.tsx @@ -4,14 +4,18 @@ import { useEffect, useMemo, useRef, useState } from "react"; import { useQuery, useQueryClient } from "@tanstack/react-query"; import { AlertCircle, + AlertTriangle, CheckCircle2, Download, HardDrive, Loader2, + RefreshCw, SkipForward, + XCircle, } from "lucide-react"; import type { AgentRuntime, + RuntimeLocalSkillImportConflict, RuntimeLocalSkillSummary, Skill, } from "@multica/core/types"; @@ -53,18 +57,27 @@ import { isNameConflictError } from "../lib/utils"; type BulkImportResult = { key: string; name: string; - status: "success" | "skipped" | "failed"; + description?: string; + status: "created" | "updated" | "conflict" | "skipped" | "failed"; + conflict?: RuntimeLocalSkillImportConflict; error?: string; skill?: Skill; }; type BulkImportState = { - phase: "idle" | "importing" | "done" | "cancelled"; + phase: "idle" | "importing" | "resolving" | "done" | "cancelled"; total: number; completed: number; results: BulkImportResult[]; }; +type ConflictResolutionAction = "overwrite" | "rename" | "skip"; + +type ConflictResolutionState = { + action: ConflictResolutionAction; + renameName: string; +}; + const INITIAL_BULK_STATE: BulkImportState = { phase: "idle", total: 0, @@ -90,6 +103,25 @@ function runtimeLabel(runtime: AgentRuntime): string { return `${runtime.name} (${runtime.provider})`; } +function defaultRenameName(name: string): string { + return `${name} copy`; +} + +function ResultIcon({ status }: { status: BulkImportResult["status"] }) { + switch (status) { + case "created": + return ; + case "updated": + return ; + case "conflict": + return ; + case "skipped": + return ; + case "failed": + return ; + } +} + // --------------------------------------------------------------------------- // Skill row with checkbox // --------------------------------------------------------------------------- @@ -195,20 +227,38 @@ function SkillItem({ function BulkImportSummary({ results }: { results: BulkImportResult[] }) { const { t } = useT("skills"); - const succeeded = results.filter((r) => r.status === "success"); + const created = results.filter((r) => r.status === "created"); + const updated = results.filter((r) => r.status === "updated"); + const conflicts = results.filter((r) => r.status === "conflict"); const skipped = results.filter((r) => r.status === "skipped"); const failed = results.filter((r) => r.status === "failed"); return (
{/* Summary counts */} -
+
- {succeeded.length} + {created.length}
- {t(($) => $.runtime_import.bulk_summary_imported)} + {t(($) => $.runtime_import.bulk_summary_created)} +
+
+
+
+ {updated.length} +
+
+ {t(($) => $.runtime_import.bulk_summary_updated)} +
+
+
+
+ {conflicts.length} +
+
+ {t(($) => $.runtime_import.bulk_summary_conflicts)}
@@ -236,15 +286,7 @@ function BulkImportSummary({ results }: { results: BulkImportResult[] }) { key={r.key} className="flex items-center gap-2 rounded px-2 py-1.5 text-xs" > - {r.status === "success" && ( - - )} - {r.status === "skipped" && ( - - )} - {r.status === "failed" && ( - - )} + {r.name} {r.error && ( @@ -258,6 +300,166 @@ function BulkImportSummary({ results }: { results: BulkImportResult[] }) { ); } +function ConflictResolutionPanel({ + conflicts, + resolutions, + onChange, + onOverwriteAll, + onSkipAll, +}: { + conflicts: BulkImportResult[]; + resolutions: Record; + onChange: (key: string, next: ConflictResolutionState) => void; + onOverwriteAll: () => void; + onSkipAll: () => void; +}) { + const { t } = useT("skills"); + const single = conflicts.length === 1; + const canOverwriteAny = conflicts.some((r) => r.conflict?.can_overwrite); + + return ( +
+
+
+ +
+

+ {single + ? t(($) => $.runtime_import.conflict_single_title) + : t(($) => $.runtime_import.conflict_bulk_title, { + count: conflicts.length, + })} +

+

+ {t(($) => $.runtime_import.conflict_hint)} +

+
+
+
+ + {!single && ( +
+ + +
+ )} + +
+ {conflicts.map((r) => { + const resolution = + resolutions[r.key] ?? + ({ + action: r.conflict?.can_overwrite ? "overwrite" : "rename", + renameName: defaultRenameName(r.name), + } satisfies ConflictResolutionState); + const creator = r.conflict?.existing_created_by; + return ( +
+
+ +
+
{r.name}
+ {r.error && ( +

{r.error}

+ )} + {!r.conflict?.can_overwrite && ( +

+ {creator + ? t(($) => $.runtime_import.conflict_locked_creator, { + creator, + }) + : t(($) => $.runtime_import.conflict_locked)} +

+ )} +
+
+ +
+ + + +
+ + {resolution.action === "rename" && ( +
+ + + onChange(r.key, { + action: "rename", + renameName: e.target.value, + }) + } + className="h-8 text-sm" + /> +
+ )} +
+ ); + })} +
+
+ ); +} + // --------------------------------------------------------------------------- // Panel // --------------------------------------------------------------------------- @@ -288,12 +490,17 @@ export function RuntimeLocalSkillImportPanel({ const [selectedRuntimeId, setSelectedRuntimeId] = useState(""); const [selectedKeys, setSelectedKeys] = useState>(new Set()); const [bulkState, setBulkState] = useState(INITIAL_BULK_STATE); + const [conflictResolutions, setConflictResolutions] = useState< + Record + >({}); const cancelRef = useRef(false); // Single-select inline edit fields (shown when exactly 1 skill is checked) const [editName, setEditName] = useState(""); const [editDescription, setEditDescription] = useState(""); const importing = bulkState.phase === "importing"; + const resolvingConflicts = bulkState.phase === "resolving"; + const busy = importing || resolvingConflicts; useEffect(() => { setSelectedRuntimeId((prev) => prev || localRuntimes[0]?.id || ""); @@ -302,6 +509,7 @@ export function RuntimeLocalSkillImportPanel({ useEffect(() => { setSelectedKeys(new Set()); setBulkState(INITIAL_BULK_STATE); + setConflictResolutions({}); setEditName(""); setEditDescription(""); }, [selectedRuntimeId]); @@ -354,6 +562,56 @@ export function RuntimeLocalSkillImportPanel({ const allSelected = runtimeSkills.length > 0 && selectedKeys.size === runtimeSkills.length; const someSelected = selectedKeys.size > 0 && !allSelected; + const pendingConflicts = bulkState.results.filter( + (r) => r.status === "conflict" && r.conflict, + ); + const canApplyConflictResolutions = + pendingConflicts.length > 0 && + pendingConflicts.every((r) => { + const resolution = conflictResolutions[r.key]; + if (!resolution) return false; + if (resolution.action === "overwrite") return !!r.conflict?.can_overwrite; + if (resolution.action === "rename") return !!resolution.renameName.trim(); + return true; + }); + + const setConflictResolution = ( + key: string, + next: ConflictResolutionState, + ) => { + setConflictResolutions((prev) => ({ ...prev, [key]: next })); + }; + + const seedConflictResolutions = (results: BulkImportResult[]) => { + const next: Record = {}; + for (const r of results) { + if (r.status !== "conflict" || !r.conflict) continue; + next[r.key] = { + action: r.conflict.can_overwrite ? "overwrite" : "rename", + renameName: defaultRenameName(r.name), + }; + } + setConflictResolutions(next); + }; + + const refreshImportedSkills = async (results: BulkImportResult[]) => { + await Promise.all([ + qc.invalidateQueries({ + queryKey: runtimeLocalSkillsKeys.forRuntime(selectedRuntimeId), + }), + qc.invalidateQueries({ queryKey: workspaceKeys.skills(wsId) }), + qc.invalidateQueries({ queryKey: workspaceKeys.agents(wsId) }), + ]); + + for (const r of results) { + if ((r.status === "created" || r.status === "updated") && r.skill) { + qc.setQueryData( + skillDetailOptions(wsId, r.skill.id).queryKey, + r.skill, + ); + } + } + }; // -- Bulk import handler -- @@ -381,19 +639,32 @@ export function RuntimeLocalSkillImportPanel({ skill_key: skill.key, name: importName, description: importDescription, + supports_conflict: true, }); - results.push({ - key: skill.key, - name: importName, - status: "success", - skill: result.skill, - }); + if (result.status === "conflict") { + results.push({ + key: skill.key, + name: importName, + description: importDescription, + status: "conflict", + conflict: result.conflict, + }); + } else { + results.push({ + key: skill.key, + name: result.skill?.name ?? importName, + description: importDescription, + status: result.status, + skill: result.skill, + }); + } } catch (error) { const msg = error instanceof Error ? error.message : ""; const isSkipped = isNameConflictError(msg); results.push({ key: skill.key, name: skill.name, + description: importDescription, status: isSkipped ? "skipped" : "failed", error: msg || t(($) => $.runtime_import.toast_import_failed), }); @@ -420,23 +691,16 @@ export function RuntimeLocalSkillImportPanel({ } await Promise.all(executing); - // Invalidate queries ONCE at the end - await Promise.all([ - qc.invalidateQueries({ - queryKey: runtimeLocalSkillsKeys.forRuntime(selectedRuntimeId), - }), - qc.invalidateQueries({ queryKey: workspaceKeys.skills(wsId) }), - qc.invalidateQueries({ queryKey: workspaceKeys.agents(wsId) }), - ]); + await refreshImportedSkills(results); - // Seed query cache for each successfully imported skill - for (const r of results) { - if (r.status === "success" && r.skill) { - qc.setQueryData( - skillDetailOptions(wsId, r.skill.id).queryKey, - r.skill, - ); - } + const conflicts = results.filter((r) => r.status === "conflict"); + if (!cancelRef.current && conflicts.length > 0) { + seedConflictResolutions(results); + setBulkState((prev) => ({ + ...prev, + phase: "resolving", + })); + return; } setBulkState((prev) => ({ @@ -445,13 +709,120 @@ export function RuntimeLocalSkillImportPanel({ })); }; + const handleApplyConflictResolutions = async () => { + if (!selectedRuntimeId || pendingConflicts.length === 0) return; + + const conflicts = [...pendingConflicts]; + let nextResults = bulkState.results; + const applyResult = (key: string, next: Partial) => { + nextResults = nextResults.map((r) => + r.key === key ? { ...r, ...next } : r, + ); + setBulkState((prev) => ({ + ...prev, + results: prev.results.map((r) => + r.key === key ? { ...r, ...next } : r, + ), + })); + }; + + setBulkState((prev) => ({ + ...prev, + phase: "importing", + total: conflicts.length, + completed: 0, + })); + + for (const r of conflicts) { + const resolution = conflictResolutions[r.key]; + if (!resolution) { + applyResult(r.key, { + status: "failed", + error: t(($) => $.runtime_import.conflict_missing_resolution), + }); + setBulkState((prev) => ({ ...prev, completed: prev.completed + 1 })); + continue; + } + + if (resolution.action === "skip") { + applyResult(r.key, { status: "skipped", error: undefined }); + setBulkState((prev) => ({ ...prev, completed: prev.completed + 1 })); + continue; + } + + try { + const result = await resolveRuntimeLocalSkillImport(selectedRuntimeId, { + skill_key: r.key, + name: + resolution.action === "rename" + ? resolution.renameName.trim() + : r.name, + description: r.description, + supports_conflict: true, + ...(resolution.action === "overwrite" && r.conflict + ? { + action: "overwrite" as const, + target_skill_id: r.conflict.existing_skill_id, + } + : {}), + }); + + if (result.status === "conflict") { + applyResult(r.key, { + name: + resolution.action === "rename" + ? resolution.renameName.trim() + : r.name, + status: "conflict", + conflict: result.conflict, + error: t(($) => $.runtime_import.conflict_name_still_exists), + }); + if (result.conflict) { + setConflictResolution(r.key, { + action: result.conflict.can_overwrite ? "overwrite" : "rename", + renameName: defaultRenameName( + resolution.action === "rename" + ? resolution.renameName.trim() + : r.name, + ), + }); + } + } else { + applyResult(r.key, { + name: result.skill?.name ?? r.name, + status: result.status, + skill: result.skill, + conflict: undefined, + error: undefined, + }); + } + } catch (error) { + const msg = error instanceof Error ? error.message : ""; + applyResult(r.key, { + status: "failed", + error: msg || t(($) => $.runtime_import.toast_import_failed), + }); + } + + setBulkState((prev) => ({ ...prev, completed: prev.completed + 1 })); + } + + await refreshImportedSkills(nextResults); + const unresolved = nextResults.some((r) => r.status === "conflict"); + setBulkState((prev) => ({ + ...prev, + results: nextResults, + phase: unresolved ? "resolving" : "done", + })); + }; + const canImport = !!selectedRuntime && selectedRuntime.status === "online" && selectedKeys.size > 0 && // Single-select requires a non-empty name (user may be renaming) (selectedKeys.size > 1 || !!editName.trim()) && - !importing; + !busy; const handleCancel = () => { cancelRef.current = true; @@ -488,15 +859,7 @@ export function RuntimeLocalSkillImportPanel({ key={r.key} className="flex items-center gap-2 rounded px-2 py-1 text-xs" > - {r.status === "success" && ( - - )} - {r.status === "skipped" && ( - - )} - {r.status === "failed" && ( - - )} + {r.name}
))} @@ -510,6 +873,41 @@ export function RuntimeLocalSkillImportPanel({ return ; } + if (bulkState.phase === "resolving") { + return ( + { + setConflictResolutions((prev) => { + const next = { ...prev }; + for (const r of pendingConflicts) { + if (!r.conflict?.can_overwrite) continue; + next[r.key] = { + action: "overwrite", + renameName: prev[r.key]?.renameName ?? defaultRenameName(r.name), + }; + } + return next; + }); + }} + onSkipAll={() => { + setConflictResolutions((prev) => { + const next = { ...prev }; + for (const r of pendingConflicts) { + next[r.key] = { + action: "skip", + renameName: prev[r.key]?.renameName ?? defaultRenameName(r.name), + }; + } + return next; + }); + }} + /> + ); + } + // --- Idle phase: skill selection list --- if (localRuntimes.length === 0) { @@ -626,7 +1024,9 @@ export function RuntimeLocalSkillImportPanel({ // -- Handle "Done" button after import -- const handleDone = () => { - const succeeded = bulkState.results.filter((r) => r.status === "success"); + const succeeded = bulkState.results.filter( + (r) => r.status === "created" || r.status === "updated", + ); // Single-import flow: navigate to the imported skill detail page. // Multi-import flow: close the dialog even if only one succeeded. if (bulkState.total === 1 && succeeded.length === 1 && succeeded[0]!.skill) { @@ -640,9 +1040,9 @@ export function RuntimeLocalSkillImportPanel({
{/* Sticky top: runtime picker + status */}
@@ -691,9 +1091,7 @@ export function RuntimeLocalSkillImportPanel({ style={fadeStyle} aria-disabled={importing || undefined} className={`min-h-0 flex-1 overflow-y-auto px-5 py-3 ${ - importing && bulkState.phase !== "importing" - ? "pointer-events-none opacity-60" - : "" + importing && bulkState.phase !== "importing" ? "pointer-events-none opacity-60" : "" }`} > {middle} @@ -717,6 +1115,22 @@ export function RuntimeLocalSkillImportPanel({ {t(($) => $.runtime_import.bulk_done_button)} + ) : resolvingConflicts ? ( + <> +
+ {t(($) => $.runtime_import.conflict_footer, { + count: pendingConflicts.length, + })} +
+ + ) : importing ? ( <>
From f810e9bc2bbd93f2556404b65f0e299eb89ea3c7 Mon Sep 17 00:00:00 2001 From: J Date: Wed, 10 Jun 2026 15:24:48 +0800 Subject: [PATCH 4/4] fix(skills): preserve bulk flow after conflict resolution Co-authored-by: multica-agent --- .../runtime-local-skill-import-panel.test.tsx | 75 +++++++++++++++++++ .../runtime-local-skill-import-panel.tsx | 16 +++- 2 files changed, 89 insertions(+), 2 deletions(-) diff --git a/packages/views/skills/components/runtime-local-skill-import-panel.test.tsx b/packages/views/skills/components/runtime-local-skill-import-panel.test.tsx index b4a3994eb..dab605bdf 100644 --- a/packages/views/skills/components/runtime-local-skill-import-panel.test.tsx +++ b/packages/views/skills/components/runtime-local-skill-import-panel.test.tsx @@ -453,4 +453,79 @@ describe("RuntimeLocalSkillImportPanel", () => { expect(await screen.findByText("Updated")).toBeInTheDocument(); }); + + it("keeps bulk completion behavior when conflict resolution leaves one success", async () => { + mockRuntimeLocalSkillsOptions.mockReturnValue({ + queryKey: ["runtimes", "local-skills", "runtime-1"], + queryFn: () => + Promise.resolve({ + supported: true, + skills: [MOCK_SKILL_A, MOCK_SKILL_B], + }), + }); + mockResolveRuntimeLocalSkillImport + .mockResolvedValueOnce({ + status: "conflict", + conflict: { + existing_skill_id: "existing-skill-1", + existing_created_by: "user-1", + can_overwrite: true, + }, + }) + .mockRejectedValueOnce(new Error("daemon failed")) + .mockResolvedValueOnce({ + status: "updated", + skill: { + ...MOCK_IMPORTED_SKILL_A, + id: "existing-skill-1", + }, + }); + + const onImported = vi.fn(); + const onBulkDone = vi.fn(); + renderPanel({ onImported, onBulkDone }); + + expect( + await screen.findByText("Review Helper", {}, { timeout: 5000 }), + ).toBeInTheDocument(); + + const selectAllLabel = screen.getByText(/Select all/i); + const selectAllCheckbox = selectAllLabel + .closest("label")! + .querySelector("input[type='checkbox']")!; + fireEvent.click(selectAllCheckbox); + + const importButton = screen.getByRole("button", { + name: /Import 2 Skills/i, + }); + await waitFor(() => expect(importButton).not.toBeDisabled(), { + timeout: 5000, + }); + fireEvent.click(importButton); + + expect( + await screen.findByText(/A skill with this name already exists/i), + ).toBeInTheDocument(); + + const applyButton = screen.getByRole("button", { + name: /Apply decisions/i, + }); + await waitFor(() => expect(applyButton).not.toBeDisabled(), { + timeout: 5000, + }); + fireEvent.click(applyButton); + + await waitFor( + () => { + expect( + screen.getByRole("button", { name: /Done/i }), + ).toBeInTheDocument(); + }, + { timeout: 10000 }, + ); + + fireEvent.click(screen.getByRole("button", { name: /Done/i })); + expect(onBulkDone).toHaveBeenCalledTimes(1); + expect(onImported).not.toHaveBeenCalled(); + }); }); diff --git a/packages/views/skills/components/runtime-local-skill-import-panel.tsx b/packages/views/skills/components/runtime-local-skill-import-panel.tsx index 13216441c..7a902a349 100644 --- a/packages/views/skills/components/runtime-local-skill-import-panel.tsx +++ b/packages/views/skills/components/runtime-local-skill-import-panel.tsx @@ -68,6 +68,7 @@ type BulkImportState = { phase: "idle" | "importing" | "resolving" | "done" | "cancelled"; total: number; completed: number; + selectedCount: number; results: BulkImportResult[]; }; @@ -82,6 +83,7 @@ const INITIAL_BULK_STATE: BulkImportState = { phase: "idle", total: 0, completed: 0, + selectedCount: 0, results: [], }; @@ -622,7 +624,13 @@ export function RuntimeLocalSkillImportPanel({ const total = skillsToImport.length; cancelRef.current = false; - setBulkState({ phase: "importing", total, completed: 0, results: [] }); + setBulkState({ + phase: "importing", + total, + completed: 0, + selectedCount: total, + results: [], + }); const results: BulkImportResult[] = []; @@ -1029,7 +1037,11 @@ export function RuntimeLocalSkillImportPanel({ ); // Single-import flow: navigate to the imported skill detail page. // Multi-import flow: close the dialog even if only one succeeded. - if (bulkState.total === 1 && succeeded.length === 1 && succeeded[0]!.skill) { + if ( + bulkState.selectedCount === 1 && + succeeded.length === 1 && + succeeded[0]!.skill + ) { onImported?.(succeeded[0]!.skill); } else { onBulkDone?.();