@@ -691,9 +1103,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 +1127,22 @@ export function RuntimeLocalSkillImportPanel({
{t(($) => $.runtime_import.bulk_done_button)}
+ ) : resolvingConflicts ? (
+ <>
+
+ {t(($) => $.runtime_import.conflict_footer, {
+ count: pendingConflicts.length,
+ })}
+
+
+
) : importing ? (
<>
diff --git a/server/internal/handler/runtime_local_skills.go b/server/internal/handler/runtime_local_skills.go
index ef7357d5a..6aa6bc727 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
@@ -62,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) (*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)
@@ -75,6 +122,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 +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"`
- 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"`
+ // 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
@@ -277,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) (*RuntimeLocalSkillImportRequest, error) {
+func (s *InMemoryLocalSkillImportStore) Create(_ context.Context, input LocalSkillImportRequestInput) (*RuntimeLocalSkillImportRequest, error) {
s.mu.Lock()
defer s.mu.Unlock()
@@ -288,15 +345,18 @@ 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: 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
@@ -396,6 +456,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 +485,14 @@ 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"`
+ // 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 {
@@ -435,7 +516,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,14 +624,36 @@ 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),
- )
+ 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(), 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
@@ -671,21 +775,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 +809,103 @@ 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,
+ ExpectedName: sanitizeNullBytes(name),
+ 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"
+ case errors.Is(oerr, errSkillOverwriteNameMismatch):
+ failMsg = "target skill name no longer matches the imported 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. 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.resolveLocalSkillConflict(w, r, req, 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 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")
+ if existing, found, lerr := h.lookupSkillByName(r.Context(), rt.WorkspaceID, sanitizeNullBytes(name)); lerr == nil && found {
+ 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
}
- writeJSON(w, http.StatusOK, map[string]string{"status": "ok"})
+ h.failLocalSkillImport(w, r, requestID, err.Error())
return
}
@@ -766,3 +930,64 @@ 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"})
+}
+
+// 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.
+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..bf467e207
--- /dev/null
+++ b/server/internal/handler/runtime_local_skills_overwrite_test.go
@@ -0,0 +1,434 @@
+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", "supports_conflict": true},
+ 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", "supports_conflict": true},
+ 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)
+ }
+}
+
+// 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 61657f194..b67c4eaaf 100644
--- a/server/internal/handler/runtime_local_skills_redis_store.go
+++ b/server/internal/handler/runtime_local_skills_redis_store.go
@@ -262,18 +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) (*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,
- 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 {
@@ -282,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)
}
@@ -494,6 +497,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..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)
+ 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)
}
@@ -249,6 +257,47 @@ 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, LocalSkillImportRequestInput{
+ RuntimeID: "runtime-1",
+ CreatorID: "user-1",
+ SkillKey: "review-helper",
+ })
+ 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 +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)
+ req, err := nodeA.Create(ctx, LocalSkillImportRequestInput{
+ RuntimeID: "runtime-import",
+ CreatorID: "user-1",
+ SkillKey: "review-helper",
+ })
if err != nil {
t.Fatalf("create: %v", err)
}
@@ -365,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)
+ 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 cd13cce70..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)
+ 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)
+ 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,14 +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")),
- )
+ 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)
}
@@ -534,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)
+ 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.go b/server/internal/handler/skill.go
index ecf5efa2e..9db861057 100644
--- a/server/internal/handler/skill.go
+++ b/server/internal/handler/skill.go
@@ -390,6 +390,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 12b10ffbd..ab0c46786 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"
skillpkg "github.com/multica-ai/multica/server/internal/skill"
db "github.com/multica-ai/multica/server/pkg/db/generated"
@@ -89,3 +91,126 @@ 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")
+ 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
+ // 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
+// 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
+ }
+ // 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
+ // 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
+}