Files
multica/server/internal/handler/skill_create.go
Bohan Jiang e4ec9dc425 MUL-2802: add skill import conflict strategies (#3997)
* feat(skills): structured conflict + overwrite path for local skill re-import

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 <github@multica.ai>

* fix(skills): gate structured conflict behind client opt-in; guard overwrite target name

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 <github@multica.ai>

* feat(skills): resolve local import conflicts in desktop

Co-authored-by: multica-agent <github@multica.ai>

* fix(skills): preserve bulk flow after conflict resolution

Co-authored-by: multica-agent <github@multica.ai>

* feat(cli): add skill import conflict strategies

Co-authored-by: multica-agent <github@multica.ai>

* fix(i18n): sync skill import locale keys

Co-authored-by: multica-agent <github@multica.ai>

* docs: explain skill import conflict handling

Co-authored-by: multica-agent <github@multica.ai>

* docs: refresh skill import source map anchors

Co-authored-by: multica-agent <github@multica.ai>

---------

Co-authored-by: J <j@multica.ai>
Co-authored-by: multica-agent <github@multica.ai>
2026-06-11 13:00:56 +08:00

217 lines
7.5 KiB
Go

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"
)
type skillCreateInput struct {
WorkspaceID pgtype.UUID
CreatorID pgtype.UUID
Name string
Description string
Content string
Config any
Files []CreateSkillFileRequest
}
// createSkillWithFilesInTx writes a skill plus its supporting files using the
// provided sqlc Queries handle, which must already be bound to an open
// transaction. Callers compose skill creation with other writes (e.g. agent
// template materialization) inside one outer transaction. For standalone
// skill creation, prefer createSkillWithFiles, which manages its own tx.
func createSkillWithFilesInTx(ctx context.Context, qtx *db.Queries, input skillCreateInput) (SkillWithFilesResponse, error) {
config, err := json.Marshal(input.Config)
if err != nil {
return SkillWithFilesResponse{}, err
}
if input.Config == nil {
config = []byte("{}")
}
skill, err := qtx.CreateSkill(ctx, db.CreateSkillParams{
WorkspaceID: input.WorkspaceID,
Name: sanitizeNullBytes(input.Name),
Description: sanitizeNullBytes(input.Description),
Content: sanitizeNullBytes(input.Content),
Config: config,
CreatedBy: input.CreatorID,
})
if err != nil {
return SkillWithFilesResponse{}, err
}
fileResps := make([]SkillFileResponse, 0, len(input.Files))
for _, f := range input.Files {
// SKILL.md is reserved for the primary skill content (skill.Content).
// Supporting files must carry additional assets, not duplicate the main file.
if skillpkg.IsReservedContentPath(f.Path) {
continue
}
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))
}
return SkillWithFilesResponse{
SkillResponse: skillToResponse(skill),
Files: fileResps,
}, nil
}
func (h *Handler) createSkillWithFiles(ctx context.Context, input skillCreateInput) (SkillWithFilesResponse, error) {
tx, err := h.TxStarter.Begin(ctx)
if err != nil {
return SkillWithFilesResponse{}, err
}
defer tx.Rollback(ctx)
qtx := h.Queries.WithTx(tx)
result, err := createSkillWithFilesInTx(ctx, qtx, input)
if err != nil {
return SkillWithFilesResponse{}, err
}
if err := tx.Commit(ctx); err != nil {
return SkillWithFilesResponse{}, err
}
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
}