Files
multica/server/internal/handler/runtime_local_skills_test.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

588 lines
18 KiB
Go

package handler
import (
"bytes"
"context"
"encoding/json"
"fmt"
"net/http"
"net/http/httptest"
"testing"
"time"
)
func newRequestAsUser(userID, method, path string, body any) *http.Request {
var buf bytes.Buffer
if body != nil {
json.NewEncoder(&buf).Encode(body)
}
req := httptest.NewRequest(method, path, &buf)
req.Header.Set("Content-Type", "application/json")
req.Header.Set("X-User-ID", userID)
req.Header.Set("X-Workspace-ID", testWorkspaceID)
return req
}
func createRuntimeLocalSkillTestRuntime(t *testing.T, ownerID string) string {
t.Helper()
runtimeName := fmt.Sprintf("runtime-local-skill-%d", time.Now().UnixNano())
daemonID := fmt.Sprintf("runtime-local-skill-daemon-%d", time.Now().UnixNano())
var runtimeID string
if err := testPool.QueryRow(context.Background(), `
INSERT INTO agent_runtime (
workspace_id, daemon_id, name, runtime_mode, provider, status, device_info, metadata, owner_id, last_seen_at
)
VALUES ($1, $2, $3, 'local', 'claude', 'online', 'Runtime Local Skills Test', '{}'::jsonb, $4, now())
RETURNING id
`, testWorkspaceID, daemonID, runtimeName, ownerID).Scan(&runtimeID); err != nil {
t.Fatalf("create local runtime: %v", err)
}
t.Cleanup(func() {
testPool.Exec(context.Background(), `DELETE FROM agent_runtime WHERE id = $1`, runtimeID)
})
return runtimeID
}
func createRuntimeLocalSkillTestMember(t *testing.T, role string) string {
t.Helper()
email := fmt.Sprintf("runtime-local-skills-%d@multica.ai", time.Now().UnixNano())
name := fmt.Sprintf("Runtime Local Skills %s", role)
var userID string
if err := testPool.QueryRow(context.Background(), `
INSERT INTO "user" (name, email)
VALUES ($1, $2)
RETURNING id
`, name, email).Scan(&userID); err != nil {
t.Fatalf("create user: %v", err)
}
if _, err := testPool.Exec(context.Background(), `
INSERT INTO member (workspace_id, user_id, role)
VALUES ($1, $2, $3)
`, testWorkspaceID, userID, role); err != nil {
t.Fatalf("create member: %v", err)
}
t.Cleanup(func() {
testPool.Exec(context.Background(), `DELETE FROM "user" WHERE id = $1`, userID)
})
return userID
}
func countSkillsByName(t *testing.T, name string) int {
t.Helper()
var count int
if err := testPool.QueryRow(context.Background(), `
SELECT count(*)
FROM skill
WHERE workspace_id = $1 AND name = $2
`, testWorkspaceID, name).Scan(&count); err != nil {
t.Fatalf("count skills: %v", err)
}
return count
}
func countSkillFiles(t *testing.T, skillID string) int {
t.Helper()
var count int
if err := testPool.QueryRow(context.Background(), `
SELECT count(*)
FROM skill_file
WHERE skill_id = $1
`, skillID).Scan(&count); err != nil {
t.Fatalf("count skill files: %v", err)
}
return count
}
func TestInMemoryLocalSkillListStore_PreservesSummaries(t *testing.T) {
ctx := context.Background()
store := NewInMemoryLocalSkillListStore()
req, err := store.Create(ctx, "runtime-xyz")
if err != nil {
t.Fatalf("create: %v", err)
}
body := map[string]any{
"status": "completed",
"supported": true,
"skills": []map[string]any{
{
"key": "review-helper",
"name": "Review Helper",
"description": "Review PRs",
"source_path": "~/.claude/skills/review-helper",
"provider": "claude",
"file_count": 2,
},
},
}
raw, _ := json.Marshal(body)
var parsed struct {
Skills []RuntimeLocalSkillSummary `json:"skills"`
}
if err := json.Unmarshal(raw, &parsed); err != nil {
t.Fatalf("unmarshal report body: %v", err)
}
if err := store.Complete(ctx, req.ID, parsed.Skills, true); err != nil {
t.Fatalf("complete: %v", err)
}
got, err := store.Get(ctx, req.ID)
if err != nil {
t.Fatalf("get: %v", err)
}
if got == nil {
t.Fatal("expected stored result")
}
if len(got.Skills) != 1 {
t.Fatalf("expected 1 skill, got %d", len(got.Skills))
}
if got.Skills[0].SourcePath != "~/.claude/skills/review-helper" {
t.Fatalf("source_path = %q", got.Skills[0].SourcePath)
}
if got.Skills[0].FileCount != 2 {
t.Fatalf("file_count = %d", got.Skills[0].FileCount)
}
}
func TestInMemoryLocalSkillListStore_TimesOutRunningRequests(t *testing.T) {
ctx := context.Background()
store := NewInMemoryLocalSkillListStore()
req, err := store.Create(ctx, "runtime-xyz")
if err != nil {
t.Fatalf("create: %v", err)
}
req.Status = RuntimeLocalSkillRunning
startedAt := time.Now().Add(-61 * time.Second)
req.RunStartedAt = &startedAt
got, err := store.Get(ctx, req.ID)
if err != nil {
t.Fatalf("get: %v", err)
}
if got == nil {
t.Fatal("expected stored request")
}
if got.Status != RuntimeLocalSkillTimeout {
t.Fatalf("expected timeout, got %s", got.Status)
}
if got.Error == "" {
t.Fatal("expected timeout error")
}
}
func TestInMemoryLocalSkillImportStore_TimesOutRunningRequests(t *testing.T) {
ctx := context.Background()
store := NewInMemoryLocalSkillImportStore()
req, err := store.Create(ctx, LocalSkillImportRequestInput{
RuntimeID: "runtime-xyz",
CreatorID: "user-1",
SkillKey: "review-helper",
})
if err != nil {
t.Fatalf("create: %v", err)
}
req.Status = RuntimeLocalSkillRunning
startedAt := time.Now().Add(-61 * time.Second)
req.RunStartedAt = &startedAt
got, err := store.Get(ctx, req.ID)
if err != nil {
t.Fatalf("get: %v", err)
}
if got == nil {
t.Fatal("expected stored request")
}
if got.Status != RuntimeLocalSkillTimeout {
t.Fatalf("expected timeout, got %s", got.Status)
}
if got.Error == "" {
t.Fatal("expected timeout error")
}
}
func TestInitiateListLocalSkills_RequiresRuntimeOwner(t *testing.T) {
if testHandler == nil {
t.Skip("database not available")
}
runtimeID := createRuntimeLocalSkillTestRuntime(t, testUserID)
adminUserID := createRuntimeLocalSkillTestMember(t, "admin")
w := httptest.NewRecorder()
req := withURLParams(
newRequestAsUser(adminUserID, http.MethodPost, "/api/runtimes/"+runtimeID+"/local-skills", nil),
"runtimeId", runtimeID,
)
testHandler.InitiateListLocalSkills(w, req)
if w.Code != http.StatusForbidden {
t.Fatalf("expected 403, got %d: %s", w.Code, w.Body.String())
}
}
func TestGetLocalSkillImportRequest_RequiresRuntimeOwner(t *testing.T) {
if testHandler == nil {
t.Skip("database not available")
}
runtimeID := createRuntimeLocalSkillTestRuntime(t, testUserID)
adminUserID := createRuntimeLocalSkillTestMember(t, "admin")
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)
}
w := httptest.NewRecorder()
req := withURLParams(
newRequestAsUser(adminUserID, http.MethodGet, "/api/runtimes/"+runtimeID+"/local-skills/import/"+importReq.ID, nil),
"runtimeId", runtimeID,
"requestId", importReq.ID,
)
testHandler.GetLocalSkillImportRequest(w, req)
if w.Code != http.StatusForbidden {
t.Fatalf("expected 403, got %d: %s", w.Code, w.Body.String())
}
}
func TestRuntimeLocalSkillImportFlow_EndToEnd(t *testing.T) {
if testHandler == nil {
t.Skip("database not available")
}
runtimeID := createRuntimeLocalSkillTestRuntime(t, testUserID)
w := httptest.NewRecorder()
initReq := withURLParams(
newRequestAsUser(testUserID, http.MethodPost, "/api/runtimes/"+runtimeID+"/local-skills/import", map[string]any{
"skill_key": "review-helper",
"name": "Imported Review Helper",
"description": "Imported description",
}),
"runtimeId", runtimeID,
)
testHandler.InitiateImportLocalSkill(w, initReq)
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)
}
w = httptest.NewRecorder()
heartbeatReq := newDaemonTokenRequest(http.MethodPost, "/api/daemon/heartbeat", map[string]any{
"runtime_id": runtimeID,
}, testWorkspaceID, "runtime-local-skills-daemon")
testHandler.DaemonHeartbeat(w, heartbeatReq)
if w.Code != http.StatusOK {
t.Fatalf("DaemonHeartbeat: expected 200, got %d: %s", w.Code, w.Body.String())
}
var heartbeatResp map[string]any
if err := json.NewDecoder(w.Body).Decode(&heartbeatResp); err != nil {
t.Fatalf("decode heartbeat response: %v", err)
}
pending, ok := heartbeatResp["pending_local_skill_import"].(map[string]any)
if !ok {
t.Fatalf("expected pending_local_skill_import, got %v", heartbeatResp)
}
if pending["id"] != importReq.ID {
t.Fatalf("pending id = %v, want %s", pending["id"], importReq.ID)
}
if pending["skill_key"] != "review-helper" {
t.Fatalf("pending skill_key = %v", pending["skill_key"])
}
if _, ok := pending["name"]; ok {
t.Fatalf("heartbeat payload should not include name: %v", pending)
}
if _, ok := pending["description"]; ok {
t.Fatalf("heartbeat payload should not include description: %v", pending)
}
w = httptest.NewRecorder()
reportReq := withURLParams(
newDaemonTokenRequest(http.MethodPost, "/api/daemon/runtimes/"+runtimeID+"/local-skills/import/"+importReq.ID+"/result", map[string]any{
"status": "completed",
"skill": map[string]any{
"name": "Original Review Helper",
"description": "Original description",
"content": "# Review Helper",
"source_path": "~/.claude/skills/review-helper",
"provider": "claude",
"files": []map[string]any{
{
"path": "templates/check.md",
"content": "body",
},
},
},
}, testWorkspaceID, "runtime-local-skills-daemon"),
"runtimeId", runtimeID,
"requestId", importReq.ID,
)
testHandler.ReportLocalSkillImportResult(w, reportReq)
if w.Code != http.StatusOK {
t.Fatalf("ReportLocalSkillImportResult: expected 200, got %d: %s", w.Code, w.Body.String())
}
w = httptest.NewRecorder()
pollReq := withURLParams(
newRequestAsUser(testUserID, http.MethodGet, "/api/runtimes/"+runtimeID+"/local-skills/import/"+importReq.ID, nil),
"runtimeId", runtimeID,
"requestId", importReq.ID,
)
testHandler.GetLocalSkillImportRequest(w, pollReq)
if w.Code != http.StatusOK {
t.Fatalf("GetLocalSkillImportRequest: expected 200, got %d: %s", w.Code, w.Body.String())
}
var completed RuntimeLocalSkillImportRequest
if err := json.NewDecoder(w.Body).Decode(&completed); err != nil {
t.Fatalf("decode poll response: %v", err)
}
if completed.Status != RuntimeLocalSkillCompleted {
t.Fatalf("expected completed status, got %s", completed.Status)
}
if completed.Skill == nil {
t.Fatal("expected imported skill")
}
if completed.Skill.Name != "Imported Review Helper" {
t.Fatalf("imported name = %q", completed.Skill.Name)
}
if completed.Skill.Description != "Imported description" {
t.Fatalf("imported description = %q", completed.Skill.Description)
}
if got := countSkillFiles(t, completed.Skill.ID); got != 1 {
t.Fatalf("expected 1 imported file, got %d", got)
}
}
func TestBatchImportViaHeartbeat(t *testing.T) {
if testHandler == nil {
t.Skip("database not available")
}
runtimeID := createRuntimeLocalSkillTestRuntime(t, testUserID)
// Create 5 import requests.
skillKeys := []string{"skill-a", "skill-b", "skill-c", "skill-d", "skill-e"}
importIDs := make([]string, 0, len(skillKeys))
for _, key := range skillKeys {
w := httptest.NewRecorder()
req := withURLParams(
newRequestAsUser(testUserID, http.MethodPost, "/api/runtimes/"+runtimeID+"/local-skills/import", map[string]any{
"skill_key": key,
}),
"runtimeId", runtimeID,
)
testHandler.InitiateImportLocalSkill(w, req)
if w.Code != http.StatusOK {
t.Fatalf("InitiateImportLocalSkill(%s): expected 200, got %d: %s", key, 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)
}
importIDs = append(importIDs, importReq.ID)
}
// Single heartbeat should return all 5 via the plural field.
w := httptest.NewRecorder()
heartbeatReq := newDaemonTokenRequest(http.MethodPost, "/api/daemon/heartbeat", map[string]any{
"runtime_id": runtimeID,
"supports_batch_import": true,
}, testWorkspaceID, "runtime-local-skills-daemon")
testHandler.DaemonHeartbeat(w, heartbeatReq)
if w.Code != http.StatusOK {
t.Fatalf("DaemonHeartbeat: expected 200, got %d: %s", w.Code, w.Body.String())
}
var heartbeatResp map[string]any
if err := json.NewDecoder(w.Body).Decode(&heartbeatResp); err != nil {
t.Fatalf("decode heartbeat response: %v", err)
}
// Singular field (backwards compat) should contain the first item.
singular, ok := heartbeatResp["pending_local_skill_import"].(map[string]any)
if !ok {
t.Fatalf("expected pending_local_skill_import, got %v", heartbeatResp)
}
if singular["id"] != importIDs[0] {
t.Fatalf("singular id = %v, want %s", singular["id"], importIDs[0])
}
// Plural field should contain all 5.
pluralRaw, ok := heartbeatResp["pending_local_skill_imports"].([]any)
if !ok {
t.Fatalf("expected pending_local_skill_imports array, got %T", heartbeatResp["pending_local_skill_imports"])
}
if len(pluralRaw) != len(skillKeys) {
t.Fatalf("expected %d pending imports, got %d", len(skillKeys), len(pluralRaw))
}
// Verify IDs match.
gotIDs := make(map[string]bool)
for _, item := range pluralRaw {
m, ok := item.(map[string]any)
if !ok {
t.Fatalf("expected map, got %T", item)
}
gotIDs[m["id"].(string)] = true
}
for _, id := range importIDs {
if !gotIDs[id] {
t.Fatalf("missing import ID %s in plural field", id)
}
}
// Second heartbeat should return nothing (all were claimed).
w = httptest.NewRecorder()
heartbeatReq2 := newDaemonTokenRequest(http.MethodPost, "/api/daemon/heartbeat", map[string]any{
"runtime_id": runtimeID,
"supports_batch_import": true,
}, testWorkspaceID, "runtime-local-skills-daemon")
testHandler.DaemonHeartbeat(w, heartbeatReq2)
if w.Code != http.StatusOK {
t.Fatalf("DaemonHeartbeat: expected 200, got %d: %s", w.Code, w.Body.String())
}
var heartbeatResp2 map[string]any
if err := json.NewDecoder(w.Body).Decode(&heartbeatResp2); err != nil {
t.Fatalf("decode heartbeat response: %v", err)
}
if _, ok := heartbeatResp2["pending_local_skill_import"]; ok {
t.Fatalf("second heartbeat should have no pending imports, got %v", heartbeatResp2)
}
if _, ok := heartbeatResp2["pending_local_skill_imports"]; ok {
t.Fatalf("second heartbeat should have no pending imports plural, got %v", heartbeatResp2)
}
}
func TestReportLocalSkillImportResult_IgnoresTimedOutRequests(t *testing.T) {
if testHandler == nil {
t.Skip("database not available")
}
runtimeID := createRuntimeLocalSkillTestRuntime(t, testUserID)
ctx := context.Background()
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)
}
importReq.Status = RuntimeLocalSkillRunning
startedAt := time.Now().Add(-61 * time.Second)
importReq.RunStartedAt = &startedAt
timedOut, err := testHandler.LocalSkillImportStore.Get(ctx, importReq.ID)
if err != nil {
t.Fatalf("get import request: %v", err)
}
if timedOut == nil || timedOut.Status != RuntimeLocalSkillTimeout {
t.Fatalf("expected timed out request, got %#v", timedOut)
}
beforeCount := countSkillsByName(t, "Timed Out Import")
w := httptest.NewRecorder()
reportReq := withURLParams(
newDaemonTokenRequest(http.MethodPost, "/api/daemon/runtimes/"+runtimeID+"/local-skills/import/"+importReq.ID+"/result", map[string]any{
"status": "completed",
"skill": map[string]any{
"name": "Original Review Helper",
"description": "Original description",
"content": "# Review Helper",
"source_path": "~/.claude/skills/review-helper",
"provider": "claude",
},
}, testWorkspaceID, "runtime-local-skills-daemon"),
"runtimeId", runtimeID,
"requestId", importReq.ID,
)
testHandler.ReportLocalSkillImportResult(w, reportReq)
if w.Code != http.StatusOK {
t.Fatalf("ReportLocalSkillImportResult: expected 200, got %d: %s", w.Code, w.Body.String())
}
afterCount := countSkillsByName(t, "Timed Out Import")
if afterCount != beforeCount {
t.Fatalf("expected timed out report to be ignored, count before=%d after=%d", beforeCount, afterCount)
}
}
func TestReportLocalSkillImportResult_RejectsCrossWorkspaceDaemonToken(t *testing.T) {
if testHandler == nil {
t.Skip("database not available")
}
runtimeID := createRuntimeLocalSkillTestRuntime(t, testUserID)
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)
}
w := httptest.NewRecorder()
reportReq := withURLParams(
newDaemonTokenRequest(http.MethodPost, "/api/daemon/runtimes/"+runtimeID+"/local-skills/import/"+importReq.ID+"/result", map[string]any{
"status": "failed",
"error": "forbidden",
}, "00000000-0000-0000-0000-000000000000", "attacker-daemon"),
"runtimeId", runtimeID,
"requestId", importReq.ID,
)
testHandler.ReportLocalSkillImportResult(w, reportReq)
if w.Code != http.StatusNotFound {
t.Fatalf("expected 404, got %d: %s", w.Code, w.Body.String())
}
}
func TestCleanOptionalString(t *testing.T) {
if got := cleanOptionalString(nil); got != nil {
t.Fatalf("expected nil, got %q", *got)
}
raw := " "
if got := cleanOptionalString(&raw); got != nil {
t.Fatalf("expected nil for whitespace-only value, got %q", *got)
}
value := " Review Helper "
got := cleanOptionalString(&value)
if got == nil || *got != "Review Helper" {
t.Fatalf("expected trimmed value, got %#v", got)
}
}
func ptr[T any](value T) *T {
return &value
}