From 5480c69c9e7e2ef7d5a4d554f0d4b19b602f97d8 Mon Sep 17 00:00:00 2001 From: Antoine GIRARD Date: Thu, 11 Jun 2026 06:18:04 +0200 Subject: [PATCH 01/20] fix: sort execution log past runs by timestamp (newest first) (#4018) MUL-3217 --- .../app/(app)/[workspace]/issue/[id]/runs.tsx | 8 ++++---- .../components/execution-log-section.tsx | 20 +++++++++---------- 2 files changed, 13 insertions(+), 15 deletions(-) diff --git a/apps/mobile/app/(app)/[workspace]/issue/[id]/runs.tsx b/apps/mobile/app/(app)/[workspace]/issue/[id]/runs.tsx index e6e6d4251a..5cbcd58aa5 100644 --- a/apps/mobile/app/(app)/[workspace]/issue/[id]/runs.tsx +++ b/apps/mobile/app/(app)/[workspace]/issue/[id]/runs.tsx @@ -1,7 +1,7 @@ /** * Agent Runs sheet — presented as a formSheet by the parent Stack. Two * sections: Active (queued/dispatched/running, created_at desc) and Past - * (failed → cancelled → completed, completed_at desc within each). Empty + * (completed_at desc, status rank as tiebreaker). Empty * sections hide entirely. * * Both entry points (the in-card AgentActivityRow and the Stack-header @@ -58,9 +58,9 @@ export default function IssueRunsRoute() { t.status === "cancelled", ); return filtered.sort((a, b) => { - const ord = PAST_STATUS_ORDER[a.status] - PAST_STATUS_ORDER[b.status]; - if (ord !== 0) return ord; - return (b.completed_at ?? "").localeCompare(a.completed_at ?? ""); + const timeDiff = (b.completed_at ?? "").localeCompare(a.completed_at ?? ""); + if (timeDiff !== 0) return timeDiff; + return PAST_STATUS_ORDER[a.status] - PAST_STATUS_ORDER[b.status]; }); }, [allTasks]); diff --git a/packages/views/issues/components/execution-log-section.tsx b/packages/views/issues/components/execution-log-section.tsx index 91f21e5541..513c1770bf 100644 --- a/packages/views/issues/components/execution-log-section.tsx +++ b/packages/views/issues/components/execution-log-section.tsx @@ -48,9 +48,9 @@ interface ExecutionLogSectionProps { issueId: string; } -// Past-runs sort priority: failed first (needs attention), then -// cancelled (procedural noise), then completed (the boring 'done' -// case sinks to the bottom). Within each group, newest first. +// Past-runs sort priority: newest first by timestamp. When two runs +// share the same timestamp, failed ranks above cancelled, which ranks +// above completed. const PAST_STATUS_RANK: Record = { failed: 0, cancelled: 1, @@ -96,17 +96,15 @@ export function ExecutionLogSection({ issueId }: ExecutionLogSectionProps) { t.status === "failed" || t.status === "cancelled", ); - // Stable sort: failed first, cancelled second, completed last. - // Within group: newest completed_at first (fall back to created_at - // for malformed rows missing completed_at). return past.toSorted((a, b) => { - const rankDiff = - (PAST_STATUS_RANK[a.status] ?? 99) - - (PAST_STATUS_RANK[b.status] ?? 99); - if (rankDiff !== 0) return rankDiff; const at = a.completed_at ?? a.created_at; const bt = b.completed_at ?? b.created_at; - return new Date(bt).getTime() - new Date(at).getTime(); + const timeDiff = new Date(bt).getTime() - new Date(at).getTime(); + if (timeDiff !== 0) return timeDiff; + return ( + (PAST_STATUS_RANK[a.status] ?? 99) - + (PAST_STATUS_RANK[b.status] ?? 99) + ); }); }, [tasks]); From e4ec9dc425b96b70d2685695268c714f2f759f18 Mon Sep 17 00:00:00 2001 From: Bohan Jiang <52446949+Bohan-J@users.noreply.github.com> Date: Thu, 11 Jun 2026 13:00:56 +0800 Subject: [PATCH 02/20] MUL-2802: add skill import conflict strategies (#3997) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 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 * 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 * feat(skills): resolve local import conflicts in desktop Co-authored-by: multica-agent * fix(skills): preserve bulk flow after conflict resolution Co-authored-by: multica-agent * feat(cli): add skill import conflict strategies Co-authored-by: multica-agent * fix(i18n): sync skill import locale keys Co-authored-by: multica-agent * docs: explain skill import conflict handling Co-authored-by: multica-agent * docs: refresh skill import source map anchors Co-authored-by: multica-agent --------- Co-authored-by: J Co-authored-by: multica-agent --- apps/docs/content/docs/cli.ja.mdx | 13 + apps/docs/content/docs/cli.ko.mdx | 13 + apps/docs/content/docs/cli.mdx | 19 + apps/docs/content/docs/cli.zh.mdx | 13 + 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/ja/skills.json | 22 +- packages/views/locales/ko/skills.json | 22 +- packages/views/locales/zh-Hans/skills.json | 22 +- .../runtime-local-skill-import-panel.test.tsx | 161 +++++- .../runtime-local-skill-import-panel.tsx | 536 ++++++++++++++++-- server/cmd/multica/cmd_skill.go | 110 +++- server/cmd/multica/cmd_skill_test.go | 73 ++- .../internal/handler/runtime_local_skills.go | 345 +++++++++-- .../runtime_local_skills_overwrite_test.go | 434 ++++++++++++++ .../runtime_local_skills_redis_store.go | 42 +- .../runtime_local_skills_redis_store_test.go | 63 +- .../handler/runtime_local_skills_test.go | 33 +- server/internal/handler/skill.go | 211 ++++++- server/internal/handler/skill_create.go | 125 ++++ .../handler/skill_import_duplicate_test.go | 101 ++++ .../multica-skill-importing/SKILL.md | 94 ++- .../references/skill-importing-source-map.md | 91 +-- .../internal/service/builtin_skills_test.go | 7 + 26 files changed, 2342 insertions(+), 267 deletions(-) create mode 100644 server/internal/handler/runtime_local_skills_overwrite_test.go diff --git a/apps/docs/content/docs/cli.ja.mdx b/apps/docs/content/docs/cli.ja.mdx index 83f35fdf30..0e753e96fe 100644 --- a/apps/docs/content/docs/cli.ja.mdx +++ b/apps/docs/content/docs/cli.ja.mdx @@ -79,6 +79,19 @@ CI やヘッドレス環境では、ブラウザフローをスキップでき | `multica skill import ...` | GitHub、ClawHub、またはローカルマシンからスキルをインポート | | `multica skill files ...` | ネスト: スキルのファイルを管理 | +### スキルインポートの競合 + +`multica skill import --url ` の既定値は `--on-conflict fail` です。同じ名前のスキルがすでに存在する場合、コマンドは構造化された `conflict` 結果で終了し、ワークスペースは変更されません。 + +既存スキルの作成者で、スキル ID とエージェントの紐付けを維持したまま内容を置き換える場合は `--on-conflict overwrite` を使います。既存スキルを残してコピーを取り込む場合は `--on-conflict rename` を使うと、`-2` のような接尾辞が自動で付きます。同名の項目を単に飛ばす場合は `--on-conflict skip` を使います。 + +```bash +multica skill import --url https://skills.sh/acme/repo/review-helper +multica skill import --url https://skills.sh/acme/repo/review-helper --on-conflict overwrite +multica skill import --url https://skills.sh/acme/repo/review-helper --on-conflict rename +multica skill import --url https://skills.sh/acme/repo/review-helper --on-conflict skip +``` + ## スクワッド | コマンド | 用途 | diff --git a/apps/docs/content/docs/cli.ko.mdx b/apps/docs/content/docs/cli.ko.mdx index 6f32b59887..24b255f5a6 100644 --- a/apps/docs/content/docs/cli.ko.mdx +++ b/apps/docs/content/docs/cli.ko.mdx @@ -79,6 +79,19 @@ CI나 headless 환경에서는 브라우저 플로우를 건너뛰세요. 웹 | `multica skill import ...` | GitHub, ClawHub, 또는 로컬 기기에서 스킬 가져오기 | | `multica skill files ...` | 중첩: 스킬의 파일 관리 | +### 스킬 가져오기 충돌 + +`multica skill import --url `의 기본값은 `--on-conflict fail`입니다. 같은 이름의 스킬이 이미 있으면 명령은 구조화된 `conflict` 결과로 종료되며 워크스페이스를 변경하지 않습니다. + +기존 스킬을 만든 사용자이고, 스킬 ID와 에이전트 연결은 유지한 채 내용을 바꾸려면 `--on-conflict overwrite`를 사용하세요. 기존 스킬을 그대로 두고 복사본을 가져오려면 `--on-conflict rename`을 사용하면 `-2` 같은 접미사가 자동으로 붙습니다. 같은 이름의 항목을 그냥 건너뛰려면 `--on-conflict skip`을 사용하세요. + +```bash +multica skill import --url https://skills.sh/acme/repo/review-helper +multica skill import --url https://skills.sh/acme/repo/review-helper --on-conflict overwrite +multica skill import --url https://skills.sh/acme/repo/review-helper --on-conflict rename +multica skill import --url https://skills.sh/acme/repo/review-helper --on-conflict skip +``` + ## 스쿼드 | 명령어 | 용도 | diff --git a/apps/docs/content/docs/cli.mdx b/apps/docs/content/docs/cli.mdx index 2536fb7773..f51db7ddf2 100644 --- a/apps/docs/content/docs/cli.mdx +++ b/apps/docs/content/docs/cli.mdx @@ -79,6 +79,25 @@ For the difference between token types, see [Authentication and tokens](/auth-to | `multica skill import ...` | Import a skill from GitHub, ClawHub, or the local machine | | `multica skill files ...` | Nested: manage a skill's files | +### Skill import conflicts + +`multica skill import --url ` defaults to `--on-conflict fail`. If a skill +with the same name already exists, the command exits with a structured +`conflict` result and does not change the workspace. + +Use `--on-conflict overwrite` when you created the existing skill and want to +replace its content while preserving its ID and agent bindings. Use +`--on-conflict rename` to import a copy with an automatic suffix such as `-2`. +Use `--on-conflict skip` to leave the existing skill untouched and report +`skipped`. + +```bash +multica skill import --url https://skills.sh/acme/repo/review-helper +multica skill import --url https://skills.sh/acme/repo/review-helper --on-conflict overwrite +multica skill import --url https://skills.sh/acme/repo/review-helper --on-conflict rename +multica skill import --url https://skills.sh/acme/repo/review-helper --on-conflict skip +``` + ## Squads | Command | Purpose | diff --git a/apps/docs/content/docs/cli.zh.mdx b/apps/docs/content/docs/cli.zh.mdx index 1f8b0e6607..0b5555057c 100644 --- a/apps/docs/content/docs/cli.zh.mdx +++ b/apps/docs/content/docs/cli.zh.mdx @@ -79,6 +79,19 @@ Token 类型的详细区分见 [认证与令牌](/auth-tokens)。 | `multica skill import ...` | 从 GitHub / ClawHub / 本机导入 Skill | | `multica skill files ...` | 嵌套：管理 Skill 的文件 | +### Skill 导入冲突 + +`multica skill import --url ` 默认等同于 `--on-conflict fail`。如果工作区里已经有同名 Skill，命令会返回结构化 `conflict` 结果并退出，不会修改工作区。 + +如果你是已有 Skill 的 creator，并且想用新导入内容覆盖它，同时保留原 Skill 的 ID 和 agent 绑定，用 `--on-conflict overwrite`。如果想保留已有 Skill、另存一份，用 `--on-conflict rename`，系统会自动加 `-2` 这类后缀。如果只是批量导入时遇到同名项就跳过，用 `--on-conflict skip`。 + +```bash +multica skill import --url https://skills.sh/acme/repo/review-helper +multica skill import --url https://skills.sh/acme/repo/review-helper --on-conflict overwrite +multica skill import --url https://skills.sh/acme/repo/review-helper --on-conflict rename +multica skill import --url https://skills.sh/acme/repo/review-helper --on-conflict skip +``` + ## 小队 | 命令 | 用途 | diff --git a/packages/core/runtimes/local-skills.ts b/packages/core/runtimes/local-skills.ts index 20f7fe0b36..f0c711f842 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 c015b46854..8e43d60237 100644 --- a/packages/core/types/agent.ts +++ b/packages/core/types/agent.ts @@ -617,9 +617,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; @@ -644,6 +653,9 @@ export interface CreateRuntimeLocalSkillImportRequest { skill_key: string; name?: string; description?: string; + action?: RuntimeLocalSkillImportAction; + target_skill_id?: string; + supports_conflict?: boolean; } export interface RuntimeLocalSkillImportRequest { @@ -652,8 +664,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; @@ -665,5 +681,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 7da9709fec..6bae1cf62c 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 de6a32b0b2..6287901417 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/ja/skills.json b/packages/views/locales/ja/skills.json index 8ac5001276..ae72a940a7 100644 --- a/packages/views/locales/ja/skills.json +++ b/packages/views/locales/ja/skills.json @@ -228,9 +228,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": "同じ名前のスキルがすでに存在します", + "conflict_bulk_title": "{{count}} 個のスキルに判断が必要です", + "conflict_hint": "既存のワークスペーススキルを上書きするか、新しい名前でインポートするか、スキップするかを選択してください。", + "conflict_locked_creator": "このスキルは {{creator}} が作成しました。ローカルインポートで上書きできるのは作成者だけです。変更が必要な場合は、スキル詳細ページで編集してください。", + "conflict_locked": "ローカルインポートでこのスキルを上書きできるのは作成者だけです。変更が必要な場合は、スキル詳細ページで編集してください。", + "conflict_overwrite": "上書き", + "conflict_rename": "名前を変更", + "conflict_skip": "スキップ", + "conflict_cancel": "キャンセル", + "conflict_overwrite_all": "すべて上書き", + "conflict_skip_all": "すべてスキップ", + "conflict_rename_label": "新しいスキル名", + "conflict_footer": "{{count}} 件の競合判断が残っています。", + "conflict_apply_button": "決定を適用", + "conflict_missing_resolution": "この競合の処理方法を選択してください。", + "conflict_name_still_exists": "その名前はすでに存在します。別の名前を選択するか、スキップしてください。" }, "file_tree": { "no_files": "ファイルなし" diff --git a/packages/views/locales/ko/skills.json b/packages/views/locales/ko/skills.json index dce5b2cf56..562bcdf6f1 100644 --- a/packages/views/locales/ko/skills.json +++ b/packages/views/locales/ko/skills.json @@ -231,9 +231,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": "이 이름의 스킬이 이미 있습니다", + "conflict_bulk_title": "{{count}}개의 스킬에 결정이 필요합니다", + "conflict_hint": "기존 워크스페이스 스킬을 덮어쓸지, 새 이름으로 가져올지, 건너뛸지 선택하세요.", + "conflict_locked_creator": "이 스킬은 {{creator}}님이 만들었습니다. 로컬 가져오기에서 덮어쓰기는 생성자만 할 수 있습니다. 변경이 필요하면 스킬 상세 페이지에서 편집하세요.", + "conflict_locked": "로컬 가져오기에서 이 스킬은 생성자만 덮어쓸 수 있습니다. 변경이 필요하면 스킬 상세 페이지에서 편집하세요.", + "conflict_overwrite": "덮어쓰기", + "conflict_rename": "이름 변경", + "conflict_skip": "건너뛰기", + "conflict_cancel": "취소", + "conflict_overwrite_all": "모두 덮어쓰기", + "conflict_skip_all": "모두 건너뛰기", + "conflict_rename_label": "새 스킬 이름", + "conflict_footer": "{{count}}개의 충돌 결정이 남아 있습니다.", + "conflict_apply_button": "결정 적용", + "conflict_missing_resolution": "이 충돌을 처리할 방법을 선택하세요.", + "conflict_name_still_exists": "해당 이름이 이미 있습니다. 다른 이름을 선택하거나 건너뛰세요." }, "file_tree": { "no_files": "파일 없음" diff --git a/packages/views/locales/zh-Hans/skills.json b/packages/views/locales/zh-Hans/skills.json index 7beacc57be..484cb4be64 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 bff9953fef..dab605bdfb 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,145 @@ 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(); + }); + + 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 88ea8bca12..7a902a3494 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,22 +57,33 @@ 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; + selectedCount: number; results: BulkImportResult[]; }; +type ConflictResolutionAction = "overwrite" | "rename" | "skip"; + +type ConflictResolutionState = { + action: ConflictResolutionAction; + renameName: string; +}; + const INITIAL_BULK_STATE: BulkImportState = { phase: "idle", total: 0, completed: 0, + selectedCount: 0, results: [], }; @@ -90,6 +105,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 +229,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 +288,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 +302,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 +492,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 +511,7 @@ export function RuntimeLocalSkillImportPanel({ useEffect(() => { setSelectedKeys(new Set()); setBulkState(INITIAL_BULK_STATE); + setConflictResolutions({}); setEditName(""); setEditDescription(""); }, [selectedRuntimeId]); @@ -354,6 +564,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 -- @@ -364,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[] = []; @@ -381,19 +647,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 +699,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 +717,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 +867,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 +881,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,10 +1032,16 @@ 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) { + if ( + bulkState.selectedCount === 1 && + succeeded.length === 1 && + succeeded[0]!.skill + ) { onImported?.(succeeded[0]!.skill); } else { onBulkDone?.(); @@ -640,9 +1052,9 @@ export function RuntimeLocalSkillImportPanel({
{/* Sticky top: runtime picker + status */}
@@ -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/cmd/multica/cmd_skill.go b/server/cmd/multica/cmd_skill.go index c6b08fcf50..23da0a03e4 100644 --- a/server/cmd/multica/cmd_skill.go +++ b/server/cmd/multica/cmd_skill.go @@ -7,7 +7,6 @@ import ( "errors" "fmt" "io" - "net/http" "net/url" "os" "strings" @@ -141,6 +140,7 @@ func init() { // skill import skillImportCmd.Flags().String("url", "", "URL to import from (required)") + skillImportCmd.Flags().String("on-conflict", "fail", "Conflict strategy when a skill with the same name exists: fail, overwrite, rename, or skip") skillImportCmd.Flags().String("output", "json", "Output format: table or json") // skill search @@ -419,9 +419,14 @@ func runSkillImport(cmd *cobra.Command, _ []string) error { if importURL == "" { return fmt.Errorf("--url is required") } + onConflict, _ := cmd.Flags().GetString("on-conflict") + if !validSkillImportConflictStrategy(onConflict) { + return fmt.Errorf("--on-conflict must be one of: fail, overwrite, rename, skip") + } body := map[string]any{ - "url": importURL, + "url": importURL, + "on_conflict": onConflict, } ctx, cancel := context.WithTimeout(context.Background(), cli.AtLeastAPITimeout(60*time.Second)) @@ -429,44 +434,107 @@ func runSkillImport(cmd *cobra.Command, _ []string) error { var result map[string]any if err := client.PostJSON(ctx, "/api/skills/import", body, &result); err != nil { - if handleSkillImportConflict(cmd, err) { - return nil + if handledErr := handleSkillImportError(cmd, err); handledErr != nil { + return handledErr } return fmt.Errorf("import skill: %w", err) } - output, _ := cmd.Flags().GetString("output") - if output == "json" { - return cli.PrintJSON(os.Stdout, result) - } + return printSkillImportResult(cmd, result) +} - fmt.Printf("Skill imported: %s (%s)\n", strVal(result, "name"), strVal(result, "id")) - return nil +func validSkillImportConflictStrategy(strategy string) bool { + switch strategy { + case "fail", "overwrite", "rename", "skip": + return true + } + return false } -func handleSkillImportConflict(cmd *cobra.Command, err error) bool { +func handleSkillImportError(cmd *cobra.Command, err error) error { var httpErr *cli.HTTPError - if !errors.As(err, &httpErr) || httpErr.StatusCode != http.StatusConflict || strings.TrimSpace(httpErr.Body) == "" { - return false + if !errors.As(err, &httpErr) || strings.TrimSpace(httpErr.Body) == "" { + return nil } var body map[string]any if json.Unmarshal([]byte(httpErr.Body), &body) != nil { - return false + return nil } - if _, ok := body["existing_skill"]; !ok { - return false + if _, ok := body["status"]; !ok { + if _, hasExisting := body["existing_skill"]; !hasExisting { + return nil + } + body = normalizeLegacySkillImportConflict(body) } + if err := printSkillImportResult(cmd, body); err != nil { + return err + } + reason := strVal(body, "reason") + if reason == "" { + reason = strVal(body, "error") + } + if reason == "" { + reason = "skill import conflict" + } + return errors.New(reason) +} + +func normalizeLegacySkillImportConflict(body map[string]any) map[string]any { + reason := strVal(body, "error") + if reason == "" { + reason = "a skill with this name already exists" + } + reason += "; use --on-conflict overwrite to replace it or --on-conflict rename to import a copy" + return map[string]any{ + "status": "conflict", + "reason": reason, + "existing_skill": body["existing_skill"], + } +} + +func printSkillImportResult(cmd *cobra.Command, result map[string]any) error { output, _ := cmd.Flags().GetString("output") if output == "json" { - _ = cli.PrintJSON(os.Stdout, body) - return true + return cli.PrintJSON(os.Stdout, result) } - existing, _ := body["existing_skill"].(map[string]any) - fmt.Printf("Skill already exists: %s (%s)\n", strVal(existing, "name"), strVal(existing, "id")) - return true + status := strVal(result, "status") + if status == "" { + fmt.Printf("Skill imported: %s (%s)\n", strVal(result, "name"), strVal(result, "id")) + return nil + } + + skill := nestedMap(result, "skill") + existing := nestedMap(result, "existing_skill") + reason := strVal(result, "reason") + switch status { + case "created": + fmt.Printf("Skill imported: %s (%s)\n", strVal(skill, "name"), strVal(skill, "id")) + case "updated": + fmt.Printf("Skill updated: %s (%s)\n", strVal(skill, "name"), strVal(skill, "id")) + case "skipped": + fmt.Printf("Skill skipped: %s (%s)\n", strVal(existing, "name"), strVal(existing, "id")) + case "conflict": + fmt.Printf("Skill import conflict: %s (%s)\n", strVal(existing, "name"), strVal(existing, "id")) + case "failed": + fmt.Printf("Skill import failed: %s\n", reason) + default: + fmt.Printf("Skill import %s\n", status) + } + if reason != "" && status != "failed" { + fmt.Printf("Reason: %s\n", reason) + } + return nil +} + +func nestedMap(m map[string]any, key string) map[string]any { + nested, _ := m[key].(map[string]any) + if nested == nil { + return map[string]any{} + } + return nested } func runSkillSearch(cmd *cobra.Command, args []string) error { diff --git a/server/cmd/multica/cmd_skill_test.go b/server/cmd/multica/cmd_skill_test.go index 1efbb41521..aedd39a74a 100644 --- a/server/cmd/multica/cmd_skill_test.go +++ b/server/cmd/multica/cmd_skill_test.go @@ -18,6 +18,7 @@ func newSkillImportTestCmd() *cobra.Command { cmd.Flags().String("workspace-id", "", "") cmd.Flags().String("profile", "", "") cmd.Flags().String("url", "", "") + cmd.Flags().String("on-conflict", "fail", "") cmd.Flags().String("output", "json", "") return cmd } @@ -43,7 +44,7 @@ func captureStdout(t *testing.T, fn func() error) (string, error) { return string(out), runErr } -func TestRunSkillImportJsonTreatsDuplicateAsStructuredResult(t *testing.T) { +func TestRunSkillImportJsonTreatsDuplicateAsConflictResult(t *testing.T) { t.Setenv("HOME", t.TempDir()) t.Setenv("MULTICA_TOKEN", "test-token") t.Setenv("MULTICA_WORKSPACE_ID", "workspace-123") @@ -58,10 +59,21 @@ func TestRunSkillImportJsonTreatsDuplicateAsStructuredResult(t *testing.T) { if r.Header.Get("X-Workspace-ID") != "workspace-123" { t.Fatalf("X-Workspace-ID = %q, want workspace-123", r.Header.Get("X-Workspace-ID")) } + var body map[string]any + if err := json.NewDecoder(r.Body).Decode(&body); err != nil { + t.Fatalf("decode request body: %v", err) + } + if body["url"] != "https://skills.sh/acme/review-helper" { + t.Fatalf("url = %v", body["url"]) + } + if body["on_conflict"] != "fail" { + t.Fatalf("on_conflict = %v, want fail", body["on_conflict"]) + } w.Header().Set("Content-Type", "application/json") w.WriteHeader(http.StatusConflict) _ = json.NewEncoder(w).Encode(map[string]any{ - "error": "a skill with this name already exists", + "status": "conflict", + "reason": "a skill with this name already exists; use --on-conflict overwrite to replace it or --on-conflict rename to import a copy", "existing_skill": map[string]any{ "id": "skill-123", "name": "review-helper", @@ -78,16 +90,19 @@ func TestRunSkillImportJsonTreatsDuplicateAsStructuredResult(t *testing.T) { out, err := captureStdout(t, func() error { return runSkillImport(cmd, nil) }) - if err != nil { - t.Fatalf("runSkillImport returned error for duplicate import: %v", err) + if err == nil { + t.Fatal("expected duplicate import to return an error") } var got map[string]any if err := json.Unmarshal([]byte(out), &got); err != nil { t.Fatalf("decode stdout JSON %q: %v", out, err) } - if got["error"] != "a skill with this name already exists" { - t.Fatalf("error = %v", got["error"]) + if got["status"] != "conflict" { + t.Fatalf("status = %v", got["status"]) + } + if !strings.Contains(strVal(got, "reason"), "--on-conflict overwrite") { + t.Fatalf("reason = %v", got["reason"]) } existing, ok := got["existing_skill"].(map[string]any) if !ok { @@ -98,6 +113,52 @@ func TestRunSkillImportJsonTreatsDuplicateAsStructuredResult(t *testing.T) { } } +func TestRunSkillImportSendsOnConflictAndPrintsStructuredResult(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + t.Setenv("MULTICA_TOKEN", "test-token") + t.Setenv("MULTICA_WORKSPACE_ID", "workspace-123") + + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + var body map[string]any + if err := json.NewDecoder(r.Body).Decode(&body); err != nil { + t.Fatalf("decode request body: %v", err) + } + if body["on_conflict"] != "overwrite" { + t.Fatalf("on_conflict = %v, want overwrite", body["on_conflict"]) + } + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusOK) + _ = json.NewEncoder(w).Encode(map[string]any{ + "status": "updated", + "skill": map[string]any{ + "id": "skill-123", + "name": "review-helper", + }, + }) + })) + defer srv.Close() + t.Setenv("MULTICA_SERVER_URL", srv.URL) + + cmd := newSkillImportTestCmd() + _ = cmd.Flags().Set("url", "https://skills.sh/acme/review-helper") + _ = cmd.Flags().Set("on-conflict", "overwrite") + _ = cmd.Flags().Set("output", "json") + + out, err := captureStdout(t, func() error { + return runSkillImport(cmd, nil) + }) + if err != nil { + t.Fatalf("runSkillImport returned error: %v", err) + } + var got map[string]any + if err := json.Unmarshal([]byte(out), &got); err != nil { + t.Fatalf("decode stdout JSON %q: %v", out, err) + } + if got["status"] != "updated" { + t.Fatalf("status = %v", got["status"]) + } +} + func TestRunSkillSearchRequestsSearchEndpoint(t *testing.T) { var gotPath string srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { diff --git a/server/internal/handler/runtime_local_skills.go b/server/internal/handler/runtime_local_skills.go index ef7357d5a0..6aa6bc7279 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 0000000000..bf467e207e --- /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 61657f1948..b67c4eaaf2 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 4c782c9698..2faaea1306 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 cd13cce70f..d92afe6342 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 ecf5efa2ef..cd045426dd 100644 --- a/server/internal/handler/skill.go +++ b/server/internal/handler/skill.go @@ -101,9 +101,18 @@ type SkillWithFilesResponse struct { Files []SkillFileResponse `json:"files"` } +type SkillImportResult struct { + Status string `json:"status"` + Reason string `json:"reason,omitempty"` + Skill *SkillWithFilesResponse `json:"skill,omitempty"` + ExistingSkill *ExistingSkillIdentity `json:"existing_skill,omitempty"` +} + type ExistingSkillIdentity struct { - ID string `json:"id"` - Name string `json:"name"` + ID string `json:"id"` + Name string `json:"name"` + CreatedBy string `json:"created_by,omitempty"` + CanOverwrite bool `json:"can_overwrite,omitempty"` } func writeSkillImportDuplicateConflict(w http.ResponseWriter, existing ExistingSkillIdentity) { @@ -138,7 +147,19 @@ func (h *Handler) existingSkillIdentityByName(ctx context.Context, workspaceID p } return ExistingSkillIdentity{}, false, err } - return ExistingSkillIdentity{ID: uuidToString(skill.ID), Name: skill.Name}, true, nil + return existingSkillIdentity(skill, ""), true, nil +} + +func existingSkillIdentity(skill db.Skill, userID string) ExistingSkillIdentity { + identity := ExistingSkillIdentity{ + ID: uuidToString(skill.ID), + Name: skill.Name, + CanOverwrite: canOverwriteSkillByLocalImport(userID, skill), + } + if skill.CreatedBy.Valid { + identity.CreatedBy = uuidToString(skill.CreatedBy) + } + return identity } // decodeSkillConfig decodes a JSONB skill.config blob, defaulting to {} when @@ -390,6 +411,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) @@ -521,7 +551,25 @@ func (h *Handler) DeleteSkill(w http.ResponseWriter, r *http.Request) { // --- Skill import --- type ImportSkillRequest struct { - URL string `json:"url"` + URL string `json:"url"` + OnConflict string `json:"on_conflict,omitempty"` +} + +const ( + importOnConflictFail = "fail" + importOnConflictOverwrite = "overwrite" + importOnConflictRename = "rename" + importOnConflictSkip = "skip" +) + +const maxImportRenameAttempts = 50 + +func validImportOnConflict(strategy string) bool { + switch strategy { + case "", importOnConflictFail, importOnConflictOverwrite, importOnConflictRename, importOnConflictSkip: + return true + } + return false } // Per-import bundle limits. These mirror the local-runtime importer so that @@ -1719,6 +1767,116 @@ func skillMdNotFoundError(owner, repo, skillName string) error { return fmt.Errorf("SKILL.md not found in repository %s/%s for skill %s", owner, repo, skillName) } +func skillImportConflictReason() string { + return "a skill with this name already exists; use --on-conflict overwrite to replace it or --on-conflict rename to import a copy" +} + +func (h *Handler) createImportedSkillWithName(ctx context.Context, workspaceID, creatorID pgtype.UUID, name string, imported *importedSkill, config map[string]any, files []CreateSkillFileRequest) (SkillWithFilesResponse, error) { + return h.createSkillWithFiles(ctx, skillCreateInput{ + WorkspaceID: workspaceID, + CreatorID: creatorID, + Name: name, + Description: imported.description, + Content: imported.content, + Config: config, + Files: files, + }) +} + +func (h *Handler) createRenamedImportedSkill(ctx context.Context, workspaceID, creatorID pgtype.UUID, baseName string, imported *importedSkill, config map[string]any, files []CreateSkillFileRequest) (SkillWithFilesResponse, error) { + for suffix := 2; suffix < maxImportRenameAttempts+2; suffix++ { + candidate := fmt.Sprintf("%s-%d", baseName, suffix) + resp, err := h.createImportedSkillWithName(ctx, workspaceID, creatorID, candidate, imported, config, files) + if err == nil { + return resp, nil + } + if !isUniqueViolation(err) { + return SkillWithFilesResponse{}, err + } + } + return SkillWithFilesResponse{}, fmt.Errorf("failed to find an available renamed skill name after %d attempts", maxImportRenameAttempts) +} + +func skillImportOverwriteFailure(err error) (int, string) { + switch { + case errors.Is(err, errSkillOverwriteNotFound): + return http.StatusConflict, "target skill no longer exists" + case errors.Is(err, errSkillOverwriteForbidden): + return http.StatusForbidden, "only the skill creator can overwrite this skill" + case errors.Is(err, errSkillOverwriteNameMismatch): + return http.StatusConflict, "target skill name no longer matches the imported skill" + default: + return http.StatusInternalServerError, "failed to overwrite skill: " + err.Error() + } +} + +func (h *Handler) resolveImportSkillConflict(w http.ResponseWriter, r *http.Request, strategy string, workspaceID string, workspaceUUID, creatorUUID pgtype.UUID, creatorID string, name string, imported *importedSkill, config map[string]any, files []CreateSkillFileRequest, existing db.Skill) { + existingInfo := existingSkillIdentity(existing, creatorID) + switch strategy { + case importOnConflictSkip: + writeJSON(w, http.StatusOK, SkillImportResult{ + Status: "skipped", + Reason: "a skill with this name already exists", + ExistingSkill: &existingInfo, + }) + case importOnConflictOverwrite: + if !canOverwriteSkillByLocalImport(creatorID, existing) { + writeJSON(w, http.StatusForbidden, SkillImportResult{ + Status: "failed", + Reason: "only the skill creator can overwrite this skill", + ExistingSkill: &existingInfo, + }) + return + } + resp, err := h.overwriteSkillWithFiles(r.Context(), skillOverwriteInput{ + WorkspaceID: workspaceUUID, + TargetSkillID: existing.ID, + UserID: creatorID, + ExpectedName: name, + Description: imported.description, + Content: imported.content, + Config: config, + Files: files, + }) + if err != nil { + status, reason := skillImportOverwriteFailure(err) + writeJSON(w, status, SkillImportResult{ + Status: "failed", + Reason: reason, + ExistingSkill: &existingInfo, + }) + return + } + actorType, actorID := h.resolveActor(r, creatorID, workspaceID) + h.publish(protocol.EventSkillUpdated, workspaceID, actorType, actorID, map[string]any{"skill": resp}) + writeJSON(w, http.StatusOK, SkillImportResult{Status: "updated", Skill: &resp}) + case importOnConflictRename: + resp, err := h.createRenamedImportedSkill(r.Context(), workspaceUUID, creatorUUID, name, imported, config, files) + if err != nil { + writeJSON(w, http.StatusInternalServerError, SkillImportResult{ + Status: "failed", + Reason: "failed to create renamed skill: " + err.Error(), + ExistingSkill: &existingInfo, + }) + return + } + actorType, actorID := h.resolveActor(r, creatorID, workspaceID) + h.publish(protocol.EventSkillCreated, workspaceID, actorType, actorID, map[string]any{"skill": resp}) + writeJSON(w, http.StatusCreated, SkillImportResult{ + Status: "created", + Reason: "renamed to avoid an existing skill", + Skill: &resp, + ExistingSkill: &existingInfo, + }) + default: + writeJSON(w, http.StatusConflict, SkillImportResult{ + Status: "conflict", + Reason: skillImportConflictReason(), + ExistingSkill: &existingInfo, + }) + } +} + // --- Import handler --- func (h *Handler) ImportSkill(w http.ResponseWriter, r *http.Request) { @@ -1739,6 +1897,15 @@ func (h *Handler) ImportSkill(w http.ResponseWriter, r *http.Request) { writeError(w, http.StatusBadRequest, "invalid request body") return } + if !validImportOnConflict(req.OnConflict) { + writeError(w, http.StatusBadRequest, "on_conflict must be one of: fail, overwrite, rename, skip") + return + } + structuredResult := req.OnConflict != "" + strategy := req.OnConflict + if strategy == "" { + strategy = importOnConflictFail + } source, normalized, err := detectImportSource(req.URL) if err != nil { @@ -1779,19 +1946,31 @@ func (h *Handler) ImportSkill(w http.ResponseWriter, r *http.Request) { if imported.origin != nil { config["origin"] = imported.origin } + name := sanitizeNullBytes(imported.name) - resp, err := h.createSkillWithFiles(r.Context(), skillCreateInput{ - WorkspaceID: workspaceUUID, - CreatorID: creatorUUID, - Name: imported.name, - Description: imported.description, - Content: imported.content, - Config: config, - Files: files, - }) + if structuredResult { + if existing, found, lerr := h.lookupSkillByName(r.Context(), workspaceUUID, name); lerr != nil { + writeJSON(w, http.StatusInternalServerError, SkillImportResult{ + Status: "failed", + Reason: "failed to check for existing skill: " + lerr.Error(), + }) + return + } else if found { + h.resolveImportSkillConflict(w, r, strategy, workspaceID, workspaceUUID, creatorUUID, creatorID, name, imported, config, files, existing) + return + } + } + + resp, err := h.createImportedSkillWithName(r.Context(), workspaceUUID, creatorUUID, name, imported, config, files) if err != nil { if isUniqueViolation(err) { - if existing, found, findErr := h.existingSkillIdentityByName(r.Context(), workspaceUUID, imported.name); findErr == nil && found { + if structuredResult { + if existing, found, lerr := h.lookupSkillByName(r.Context(), workspaceUUID, name); lerr == nil && found { + h.resolveImportSkillConflict(w, r, strategy, workspaceID, workspaceUUID, creatorUUID, creatorID, name, imported, config, files, existing) + return + } + } + if existing, found, findErr := h.existingSkillIdentityByName(r.Context(), workspaceUUID, name); findErr == nil && found { writeSkillImportDuplicateConflict(w, existing) } else { writeError(w, http.StatusConflict, "a skill with this name already exists") @@ -1803,6 +1982,10 @@ func (h *Handler) ImportSkill(w http.ResponseWriter, r *http.Request) { } actorType, actorID := h.resolveActor(r, creatorID, workspaceID) h.publish(protocol.EventSkillCreated, workspaceID, actorType, actorID, map[string]any{"skill": resp}) + if structuredResult { + writeJSON(w, http.StatusCreated, SkillImportResult{Status: "created", Skill: &resp}) + return + } writeJSON(w, http.StatusCreated, resp) } diff --git a/server/internal/handler/skill_create.go b/server/internal/handler/skill_create.go index 12b10ffbde..ab0c467869 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 +} diff --git a/server/internal/handler/skill_import_duplicate_test.go b/server/internal/handler/skill_import_duplicate_test.go index 1c4312ebb0..f2e3e8e4cd 100644 --- a/server/internal/handler/skill_import_duplicate_test.go +++ b/server/internal/handler/skill_import_duplicate_test.go @@ -3,6 +3,7 @@ package handler import ( "context" "encoding/json" + "net/http" "net/http/httptest" "testing" ) @@ -46,3 +47,103 @@ func TestWriteSkillImportDuplicateConflictIncludesExistingSkill(t *testing.T) { t.Fatalf("existing_skill = %#v", existing) } } + +func withMockClawHubImport(t *testing.T, skillName string) string { + t.Helper() + slug := "review-helper" + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case "/api/v1/skills/" + slug: + _ = json.NewEncoder(w).Encode(map[string]any{ + "skill": map[string]any{ + "slug": slug, + "displayName": skillName, + "summary": "Imported test skill", + "tags": map[string]string{"latest": "1.0.0"}, + }, + }) + case "/api/v1/skills/" + slug + "/versions/1.0.0": + _ = json.NewEncoder(w).Encode(map[string]any{ + "version": map[string]any{ + "version": "1.0.0", + "files": []map[string]any{ + {"path": "SKILL.md", "size": 16}, + }, + }, + }) + case "/api/v1/skills/" + slug + "/file": + _, _ = w.Write([]byte("# Imported\n")) + default: + t.Fatalf("unexpected ClawHub path: %s", r.URL.String()) + } + })) + prev := clawHubAPIBase + clawHubAPIBase = srv.URL + "/api/v1" + t.Cleanup(func() { + clawHubAPIBase = prev + srv.Close() + }) + return "https://clawhub.ai/acme/" + slug +} + +func TestImportSkillOnConflictSkipReturnsStructuredResult(t *testing.T) { + if testHandler == nil || testPool == nil { + t.Skip("handler test DB not configured") + } + namePrefix := "url-import-skip" + skillName := namePrefix + "-" + t.Name() + existingID := insertHandlerTestSkill(t, namePrefix, "# Existing") + importURL := withMockClawHubImport(t, skillName) + + w := httptest.NewRecorder() + req := newRequestAsUser(testUserID, http.MethodPost, "/api/skills/import", map[string]any{ + "url": importURL, + "on_conflict": "skip", + }) + testHandler.ImportSkill(w, req) + + if w.Code != http.StatusOK { + t.Fatalf("status = %d, want 200: %s", w.Code, w.Body.String()) + } + var body SkillImportResult + if err := json.Unmarshal(w.Body.Bytes(), &body); err != nil { + t.Fatalf("decode body: %v", err) + } + if body.Status != "skipped" { + t.Fatalf("status = %q", body.Status) + } + if body.ExistingSkill == nil || body.ExistingSkill.ID != existingID || body.ExistingSkill.Name != skillName { + t.Fatalf("existing_skill = %#v", body.ExistingSkill) + } +} + +func TestImportSkillOnConflictRenameCreatesSuffixedSkill(t *testing.T) { + if testHandler == nil || testPool == nil { + t.Skip("handler test DB not configured") + } + namePrefix := "url-import-rename" + skillName := namePrefix + "-" + t.Name() + insertHandlerTestSkill(t, namePrefix, "# Existing") + importURL := withMockClawHubImport(t, skillName) + + w := httptest.NewRecorder() + req := newRequestAsUser(testUserID, http.MethodPost, "/api/skills/import", map[string]any{ + "url": importURL, + "on_conflict": "rename", + }) + testHandler.ImportSkill(w, req) + + if w.Code != http.StatusCreated { + t.Fatalf("status = %d, want 201: %s", w.Code, w.Body.String()) + } + var body SkillImportResult + if err := json.Unmarshal(w.Body.Bytes(), &body); err != nil { + t.Fatalf("decode body: %v", err) + } + if body.Status != "created" || body.Skill == nil { + t.Fatalf("body = %#v", body) + } + if body.Skill.Name != skillName+"-2" { + t.Fatalf("created skill name = %q, want %q", body.Skill.Name, skillName+"-2") + } +} diff --git a/server/internal/service/builtin_skills/multica-skill-importing/SKILL.md b/server/internal/service/builtin_skills/multica-skill-importing/SKILL.md index ce3a4887b2..c38c8bcaa8 100644 --- a/server/internal/service/builtin_skills/multica-skill-importing/SKILL.md +++ b/server/internal/service/builtin_skills/multica-skill-importing/SKILL.md @@ -1,6 +1,6 @@ --- name: multica-skill-importing -description: "Use when a user provides a skill URL, slug, or clear intent to import/install a specific skill into the current Multica workspace. Teaches the workspace import API/CLI path (POST /api/skills/import), the supported URL source families, the SkillWithFilesResponse shape returned on success, duplicate 409 handling with the existing_skill body, additive agent binding vs replace-all, and the reserved SKILL.md supporting-file rule. Do not use it to decide which skill the user needs, and never treat an external local installer like npx skills add as the final Multica install." +description: "Use when a user provides a skill URL, slug, or clear intent to import/install a specific skill into the current Multica workspace. Teaches the workspace import API/CLI path (POST /api/skills/import), the supported URL source families, --on-conflict fail|overwrite|rename|skip behavior and structured import results, additive agent binding vs replace-all, and the reserved SKILL.md supporting-file rule. Do not use it to decide which skill the user needs, and never treat an external local installer like npx skills add as the final Multica install." user-invocable: false allowed-tools: Bash(multica *) --- @@ -28,11 +28,11 @@ import endpoint, driven by this CLI: multica skill import --url --output json ``` -That CLI sends: +The CLI defaults to `--on-conflict fail`. Current CLIs send: ```text POST /api/skills/import -body: { "url": "" } +body: { "url": "", "on_conflict": "fail" } ``` Do not finish with `npx skills add`. That installs into an external/local skill @@ -66,17 +66,30 @@ directly; search is not required by the API: multica skill import --url --output json ``` -2. Treat a successful response as the source of truth. The body is a workspace -`SkillWithFilesResponse` — it embeds the standard `SkillResponse` and adds the -supporting `files` array. Report the relevant fields: +2. Treat the response as the source of truth. Current CLI imports use the +structured import result envelope: -- `id` -- `name` -- `description` -- `config.origin` (provenance: which source the skill was imported from — set - only when the source supplied an origin, so treat it as possibly absent) -- `files` / files count -- `created_at` / `updated_at` +```json +{ + "status": "created|updated|conflict|skipped|failed", + "reason": "...", + "skill": { "...": "SkillWithFilesResponse when created/updated" }, + "existing_skill": { "id": "...", "name": "...", "can_overwrite": true } +} +``` + +For `created` / `updated`, `skill` is a workspace `SkillWithFilesResponse`: it +embeds the standard `SkillResponse` and adds the supporting `files` array. Report +the relevant fields: + +- `status` and `reason` when present. +- `skill.id` / `skill.name` / `skill.description`. +- `skill.config.origin` (provenance: which source the skill was imported from — + set only when the source supplied an origin, so treat it as possibly absent). +- `skill.files` / files count. +- `skill.created_at` / `skill.updated_at`. +- `existing_skill.id` / `existing_skill.name` when status is `conflict`, + `skipped`, or `failed` due to an existing skill. Because the response is structured, read these returned fields instead of guessing whether the import succeeded. @@ -118,9 +131,46 @@ rename it to a non-reserved path. (The hard `400` rejection — "SKILL.md is res for the primary skill content" — only fires on the dedicated single-file endpoint `PUT /api/skills/{id}/files`, not on import.) -## Duplicate imports (409) +## Same-name conflicts: `--on-conflict` + +Default behavior is safe: `multica skill import --url ` is equivalent to +`--on-conflict fail`. If the imported skill name already exists, the command +prints a structured `conflict` result and exits non-zero; no skill is created or +updated. + +Choose an explicit strategy only when the user asked for it or the intent is +clear: + +- `--on-conflict fail` (default): do nothing on conflict; report `status: + conflict` with a reason that suggests overwrite or rename. +- `--on-conflict overwrite`: update the existing same-name skill in place, but + only if the current user is the skill's original creator. This preserves the + skill ID, `created_by`, `created_at`, and agent-skill bindings; it replaces + description, content, provenance config, and supporting files. Non-creators get + `status: failed`. +- `--on-conflict rename`: create a new skill with an automatic suffix such as + `-2` / `-3`; the existing skill is untouched. +- `--on-conflict skip`: leave the existing skill untouched and report `status: + skipped`. + +Concrete examples: + +```bash +# Safe default. Fails with status=conflict if review-helper already exists. +multica skill import --url https://skills.sh/acme/repo/review-helper --output json + +# Replace the existing same-name skill, preserving its ID and agent bindings. +multica skill import --url https://skills.sh/acme/repo/review-helper --on-conflict overwrite --output json + +# Keep the existing skill and import a copy such as review-helper-2. +multica skill import --url https://skills.sh/acme/repo/review-helper --on-conflict rename --output json + +# Batch-friendly behavior: leave the existing skill alone and mark it skipped. +multica skill import --url https://skills.sh/acme/repo/review-helper --on-conflict skip --output json +``` -A duplicate import returns `409`. On current servers the body carries the existing +Legacy compatibility: clients that do not send `on_conflict` keep the old +contract. A duplicate import returns `409` and the body carries the existing workspace skill identity: ```json @@ -133,19 +183,17 @@ workspace skill identity: } ``` -`multica skill import --url --output json` prints that structured conflict -body and exits successfully (exit 0) for this duplicate case. Treat -`existing_skill.id` and `existing_skill.name` as the source of truth, then fetch -details if needed: +Current CLI normalizes that legacy shape into `status: conflict` and exits +non-zero for the default `fail` strategy. Treat `existing_skill.id` and +`existing_skill.name` as the source of truth, then fetch details if needed: ```bash multica skill get --output json ``` -Legacy fallback: a legacy server or old CLI may return a `409` whose body is only a -string like `a skill with this name already exists`, with no `existing_skill` key. -The CLI cannot recognize that as the duplicate case, so it exits non-zero. Recover -by finding the existing workspace skill yourself: +Older servers may return a `409` whose body is only a string like `a skill with +this name already exists`, with no `existing_skill` key. Recover by finding the +existing workspace skill yourself: ```bash multica skill list --output json diff --git a/server/internal/service/builtin_skills/multica-skill-importing/references/skill-importing-source-map.md b/server/internal/service/builtin_skills/multica-skill-importing/references/skill-importing-source-map.md index 5df7eeed18..b24ab662d8 100644 --- a/server/internal/service/builtin_skills/multica-skill-importing/references/skill-importing-source-map.md +++ b/server/internal/service/builtin_skills/multica-skill-importing/references/skill-importing-source-map.md @@ -17,44 +17,48 @@ grep -n "func IsReservedContentPath" server/internal/skill/reserved.go | Behavior | File:line | |---|---| -| `ImportSkill` handler (`POST /api/skills/import`) | `server/internal/handler/skill.go:1724` | -| Decodes `ImportSkillRequest` (`{ "url": ... }`) | `server/internal/handler/skill.go:1737-1741`, struct at `:523` | -| Detects source family + normalizes URL | `server/internal/handler/skill.go:1743` (calls `detectImportSource`) | -| Persists provenance into `config.origin` | `server/internal/handler/skill.go:1776-1781` — set at `:1780` **only when** `imported.origin != nil` (`:1779`); otherwise `config` stays `{}` and `origin` is absent | -| Builds skill + files via `createSkillWithFiles` (def `server/internal/handler/skill_create.go:72`, tx body `:27`) | call site `server/internal/handler/skill.go:1783` | -| Success: `201 Created` with the response body | `server/internal/handler/skill.go:1806` | -| Route registration `r.Post("/import", h.ImportSkill)` | `server/cmd/server/router.go:621` (under `/api/skills`, `:617`) | +| `ImportSkill` handler (`POST /api/skills/import`) | `server/internal/handler/skill.go:1882` | +| Decodes `ImportSkillRequest` (`{ "url": ..., "on_conflict": ... }`) | `server/internal/handler/skill.go:1895-1899`, struct at `:553` | +| Validates `on_conflict` (`fail`, `overwrite`, `rename`, `skip`) | `server/internal/handler/skill.go:1900-1908`, helper `validImportOnConflict` at `:566` | +| Detects source family + normalizes URL | `server/internal/handler/skill.go:1910` (calls `detectImportSource`) | +| Persists provenance into `config.origin` | `server/internal/handler/skill.go:1944-1948` — set only when `imported.origin != nil`; otherwise `config` stays `{}` and `origin` is absent | +| Structured conflict dispatcher | `server/internal/handler/skill.go:1813-1878` | +| Builds skill + files via `createSkillWithFiles` (def `server/internal/handler/skill_create.go:77`, tx body `:29`) | wrapped by `createImportedSkillWithName` at `server/internal/handler/skill.go:1774` | +| Structured success: `201 Created` with `{status:"created", skill}` when `on_conflict` was sent | `server/internal/handler/skill.go:1985-1988` | +| Legacy success: `201 Created` with bare `SkillWithFilesResponse` when `on_conflict` was omitted | `server/internal/handler/skill.go:1990` | +| Route registration `r.Post("/import", h.ImportSkill)` | `server/cmd/server/router.go:874` | ## CLI: `multica skill import --url` | Behavior | File:line | |---|---| | `skill import` command def | `server/cmd/multica/cmd_skill.go:60-64` | -| `--url` flag | `server/cmd/multica/cmd_skill.go:143` | +| `--url` flag | `server/cmd/multica/cmd_skill.go:142` | +| `--on-conflict` flag (default `fail`) | `server/cmd/multica/cmd_skill.go:143` | | `--output` flag (default `json`) | `server/cmd/multica/cmd_skill.go:144` | | `runSkillImport` | `server/cmd/multica/cmd_skill.go:412` | | Requires `--url` | `server/cmd/multica/cmd_skill.go:418-421` | -| `POST /api/skills/import` | `server/cmd/multica/cmd_skill.go:431` | -| On error, tries conflict handler; returns `nil` (exit 0) when handled | `server/cmd/multica/cmd_skill.go:432-434` | +| Reads and validates `--on-conflict` | `server/cmd/multica/cmd_skill.go:422-425` | +| Sends `on_conflict` in the request body | `server/cmd/multica/cmd_skill.go:428-431` | +| `POST /api/skills/import` | `server/cmd/multica/cmd_skill.go:436` | +| Structured HTTP error body handling | `server/cmd/multica/cmd_skill.go:437-440`, `handleSkillImportError` at `:454` | +| Prints structured result (`json` or table) | `server/cmd/multica/cmd_skill.go:443`, helper at `:497` | -## Duplicate 409 handling +## Same-name conflict handling | Behavior | File:line | |---|---| -| `ImportSkill` unique-violation branch | `server/internal/handler/skill.go:1793-1800` | -| Structured 409 via `writeSkillImportDuplicateConflict` (looks up existing skill) | `server/internal/handler/skill.go:1794-1795` | -| `writeSkillImportDuplicateConflict` writes `{error, existing_skill}` at status 409 | `server/internal/handler/skill.go:109-114` | -| `ExistingSkillIdentity` (`{id,name}`) | `server/internal/handler/skill.go:104-107` | -| `existingSkillIdentityByName` (GetSkillByWorkspaceAndName) | `server/internal/handler/skill.go:130-142` | -| Defensive fallback: plain-string 409 when lookup misses (delete-race) | `server/internal/handler/skill.go:1796-1798` | -| CLI `handleSkillImportConflict` | `server/cmd/multica/cmd_skill.go:447` | -| Requires HTTP 409 + non-empty body | `server/cmd/multica/cmd_skill.go:449-451` | -| Requires `existing_skill` key (else `false` → non-zero exit) | `server/cmd/multica/cmd_skill.go:457-459` | -| Under `--output json`: prints body, returns `true` (caller exits 0) | `server/cmd/multica/cmd_skill.go:461-465` | - -A legacy plain-string 409 (no `existing_skill` key) fails the `:457-459` guard, -so `handleSkillImportConflict` returns `false`, `runSkillImport` falls through to -`return fmt.Errorf("import skill: ...")` at `:435` → non-zero exit. +| `SkillImportResult` (`status`, `reason`, `skill`, `existing_skill`) | `server/internal/handler/skill.go:104-109` | +| `ExistingSkillIdentity` (`id`, `name`, `created_by`, `can_overwrite`) | `server/internal/handler/skill.go:112-117` | +| Pre-create lookup for structured conflict flow | `server/internal/handler/skill.go:1951-1962` | +| Race-safe unique-violation fallback into structured conflict flow | `server/internal/handler/skill.go:1966-1971` | +| Default `fail`: `status:"conflict"` and HTTP 409 | `server/internal/handler/skill.go:1872-1877` | +| `overwrite`: creator-only update, preserves skill identity/bindings via `overwriteSkillWithFiles` | `server/internal/handler/skill.go:1823-1852`, tx helper at `server/internal/handler/skill_create.go:133` | +| `rename`: creates suffixed name with bounded attempts | `server/internal/handler/skill.go:1854-1870`, helper at `:1786` | +| `skip`: returns `status:"skipped"` and leaves existing skill untouched | `server/internal/handler/skill.go:1816-1821` | +| Legacy duplicate branch when `on_conflict` was omitted | `server/internal/handler/skill.go:1973-1978` | +| Legacy duplicate response `{error, existing_skill}` | `server/internal/handler/skill.go:118-123` | +| CLI normalizes legacy `{existing_skill}` body into `status:"conflict"` | `server/cmd/multica/cmd_skill.go:454-482`, helper at `:484` | ## Response shape: `SkillWithFilesResponse` @@ -64,35 +68,36 @@ so `handleSkillImportConflict` returns `false`, `runSkillImport` falls through t | `SkillResponse` fields (`id, workspace_id, name, description, content, config, created_by, created_at, updated_at`) | `server/internal/handler/skill.go:41-51` | | `SkillFileResponse` fields | `server/internal/handler/skill.go:80-87` | | `createSkillWithFilesInTx` returns `SkillWithFilesResponse{SkillResponse, Files}` | `server/internal/handler/skill_create.go:66-69` | -| `config.origin` set on import | `server/internal/handler/skill.go:1780` | +| `config.origin` set on import | `server/internal/handler/skill.go:1947` | -The import response is a `SkillWithFilesResponse` (not a bare `SkillResponse`): -it carries every `SkillResponse` field plus the `files` array. +For current CLI imports, `SkillWithFilesResponse` appears under +`SkillImportResult.skill` when status is `created` or `updated`. Legacy clients +that omit `on_conflict` still receive a bare `SkillWithFilesResponse`. ## URL source families (`detectImportSource`) | Behavior | File:line | |---|---| -| `detectImportSource` | `server/internal/handler/skill.go:723-756` | -| `skills.sh` / `www.skills.sh` | `server/internal/handler/skill.go:743-744` | -| `clawhub.ai` / `www.clawhub.ai` | `server/internal/handler/skill.go:745-746` | -| `github.com` / `www.github.com` | `server/internal/handler/skill.go:747-748` | -| Bare slug (no host) defaults to ClawHub | `server/internal/handler/skill.go:750-753` | -| `parseGitHubURL` handles `/tree/{ref}/...` and `/blob/{ref}/.../SKILL.md` | `server/internal/handler/skill.go:1402-1455` (tree/blob check `:1415-1432`) | +| `detectImportSource` | `server/internal/handler/skill.go:773-804` | +| `skills.sh` / `www.skills.sh` | `server/internal/handler/skill.go:791-792` | +| `clawhub.ai` / `www.clawhub.ai` | `server/internal/handler/skill.go:793-794` | +| `github.com` / `www.github.com` | `server/internal/handler/skill.go:795-796` | +| Bare slug (no host) defaults to ClawHub | `server/internal/handler/skill.go:798-800` | +| `parseGitHubURL` handles `/tree/{ref}/...` and `/blob/{ref}/.../SKILL.md` | `server/internal/handler/skill.go:1450-1503` (tree/blob check `:1463-1480`) | ## Additive add vs replace-all set | Behavior | File:line | |---|---| -| `AddAgentSkills` (additive: AddAgentSkill loop, no RemoveAll) | `server/internal/handler/skill.go:1978`; loop `:2009-2017` | -| Route `POST /api/agents/{id}/skills/add` | `server/cmd/server/router.go:598` | -| `SetAgentSkills` (replace-all: RemoveAllAgentSkills then re-add) | `server/internal/handler/skill.go:1923`; `RemoveAllAgentSkills` `:1955`; re-add `:1960-1968` | -| Route `PUT /api/agents/{id}/skills` | `server/cmd/server/router.go:597` | +| `AddAgentSkills` (additive: AddAgentSkill loop, no RemoveAll) | `server/internal/handler/skill.go:2161`; loop `:2192-2200` | +| Route `POST /api/agents/{id}/skills/add` | `server/cmd/server/router.go:851` | +| `SetAgentSkills` (replace-all: RemoveAllAgentSkills then re-add) | `server/internal/handler/skill.go:2106`; `RemoveAllAgentSkills` `:2138`; re-add `:2143-2151` | +| Route `PUT /api/agents/{id}/skills` | `server/cmd/server/router.go:850` | | CLI `agent skills add` def ("without replacing existing assignments") | `server/cmd/multica/cmd_agent.go:125-130` | -| `runAgentSkillsAdd` → `POST .../skills/add` | `server/cmd/multica/cmd_agent.go:839`; POST `:860` | +| `runAgentSkillsAdd` → `POST .../skills/add` | `server/cmd/multica/cmd_agent.go:797`; POST `:818` | | CLI `agent skills set` def ("replaces all current assignments") | `server/cmd/multica/cmd_agent.go:118-123` | -| `runAgentSkillsSet` → `PUT .../skills` | `server/cmd/multica/cmd_agent.go:814`; PUT `:832` | -| CLI `agent skills list` | `server/cmd/multica/cmd_agent.go:782`; GET `:792` | +| `runAgentSkillsSet` → `PUT .../skills` | `server/cmd/multica/cmd_agent.go:772`; PUT `:790` | +| CLI `agent skills list` | `server/cmd/multica/cmd_agent.go:740`; GET `:750` | ## Reserved primary-content filename (`SKILL.md`) @@ -101,8 +106,8 @@ it carries every `SkillResponse` field plus the `files` array. | `ContentFilename = "SKILL.md"` | `server/internal/skill/reserved.go:12` | | `IsReservedContentPath` (cleans path, case-insensitive compare) | `server/internal/skill/reserved.go:25-27` | | Import/create path: reserved supporting file is **silently skipped** (`continue`) | `server/internal/handler/skill_create.go:50-54` | -| `UpdateSkill` (PUT `/api/skills/{id}`) replace-files path: also silently skips | `server/internal/handler/skill.go:461-464` | -| `UpsertSkillFile` (PUT `/api/skills/{id}/files`): **rejects 400** "SKILL.md is reserved for the primary skill content" | `server/internal/handler/skill.go:1851-1854` | +| `UpdateSkill` (PUT `/api/skills/{id}`) replace-files path: also silently skips | `server/internal/handler/skill.go:490-494` | +| `UpsertSkillFile` (PUT `/api/skills/{id}/files`): **rejects 400** "SKILL.md is reserved for the primary skill content" | `server/internal/handler/skill.go:2014`; reserved check `:2034-2036` | Reason `SKILL.md` is reserved: the daemon writes the skill's `Content` to that path itself when preparing the execution environment, so a supporting file may not also diff --git a/server/internal/service/builtin_skills_test.go b/server/internal/service/builtin_skills_test.go index 48e9f05225..a72972d862 100644 --- a/server/internal/service/builtin_skills_test.go +++ b/server/internal/service/builtin_skills_test.go @@ -276,6 +276,13 @@ func TestSkillImportingSkillCoversWorkspaceImportContracts(t *testing.T) { "skills.sh", "github.com", "config.origin", + "--on-conflict fail", + "--on-conflict overwrite", + "--on-conflict rename", + "--on-conflict skip", + "status", + "conflict", + "skipped", "409", "existing_skill", "id", From 8151f60c6cbe40b763145f4e01a0832af7bde92b Mon Sep 17 00:00:00 2001 From: Bohan Jiang <52446949+Bohan-J@users.noreply.github.com> Date: Thu, 11 Jun 2026 13:07:44 +0800 Subject: [PATCH 03/20] fix(daemon): drop stale resume session when workdir is not reused (#4027) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CLI backends key their session stores to the cwd (Claude Code looks sessions up under ~/.claude/projects//), so a prior session id can only resolve when the task runs in the exact workdir the session was recorded against. When the prior workdir no longer exists (GC'd after the issue went done, daemon reinstall, manual cleanup), execenv.Reuse falls back to a fresh Prepare but the stale session id was still passed to the backend: claude exited within a second and the run failed before doing any work — permanently, because the failed run records no session_id and the next claim serves the same stale pointer again. Gate ResumeSessionID on the workdir actually being reused, and correct PriorSessionResumed so the runtime brief uses the cold-path wording when the session is dropped. Fixes multica-ai/multica#3854 (MUL-3221) Co-authored-by: J Co-authored-by: multica-agent --- server/internal/daemon/daemon.go | 27 ++++++++++- server/internal/daemon/daemon_test.go | 66 +++++++++++++++++++++++++++ 2 files changed, 92 insertions(+), 1 deletion(-) diff --git a/server/internal/daemon/daemon.go b/server/internal/daemon/daemon.go index af87269c0f..2e04ae1041 100644 --- a/server/internal/daemon/daemon.go +++ b/server/internal/daemon/daemon.go @@ -2579,6 +2579,30 @@ func providerNeedsInlineSystemPrompt(provider string) bool { } } +// gateResumeToReusedWorkdir clears the task's prior session unless the task +// runs in the exact workdir the session was recorded against, and reports +// whether that workdir was reused. CLI backends key their session stores to +// the cwd (Claude Code looks sessions up under ~/.claude/projects//), +// so a session id from a different workdir can never resolve: the CLI exits +// within a second and the run fails before doing any work — permanently, +// because the failed run records no session and the next claim serves the +// same stale pointer again. This fires whenever the prior workdir no longer +// exists (GC'd after the issue went done, daemon reinstall, manual cleanup) +// and execenv.Reuse fell back to a fresh Prepare (GitHub #3854). +func gateResumeToReusedWorkdir(task *Task, taskCtx *execenv.TaskContextForEnv, envWorkDir string, taskLog *slog.Logger) bool { + reused := task.PriorWorkDir != "" && envWorkDir == task.PriorWorkDir + if !reused && task.PriorSessionID != "" { + taskLog.Info("dropping prior session: workdir not reused, per-cwd session cannot resolve", + "session_id", task.PriorSessionID, + "prior_workdir", task.PriorWorkDir, + "workdir", envWorkDir, + ) + task.PriorSessionID = "" + taskCtx.PriorSessionResumed = false + } + return reused +} + func (d *Daemon) runTask(ctx context.Context, task Task, provider string, slot int, taskLog *slog.Logger) (TaskResult, error) { // Refuse to spawn an agent without a workspace. An empty workspace_id // here would make MULTICA_WORKSPACE_ID empty in the agent env, and the @@ -2721,6 +2745,8 @@ func (d *Daemon) runTask(ctx context.Context, task Task, provider string, slot i defer d.unmarkActiveEnvRoot(env.RootDir) } + reused := gateResumeToReusedWorkdir(&task, &taskCtx, env.WorkDir, taskLog) + // Inject runtime-specific config (meta skill) so the agent discovers .agent_context/. runtimeBrief, err := execenv.InjectRuntimeConfig(env.WorkDir, provider, taskCtx) if err != nil { @@ -2853,7 +2879,6 @@ func (d *Daemon) runTask(ctx context.Context, task Task, provider string, slot i return TaskResult{}, fmt.Errorf("create agent backend: %w", err) } - reused := task.PriorWorkDir != "" && env.WorkDir == task.PriorWorkDir taskLog.Info("starting agent", "provider", provider, "workdir", env.WorkDir, diff --git a/server/internal/daemon/daemon_test.go b/server/internal/daemon/daemon_test.go index de1d462dea..fa748e5f9c 100644 --- a/server/internal/daemon/daemon_test.go +++ b/server/internal/daemon/daemon_test.go @@ -18,6 +18,7 @@ import ( "testing" "time" + "github.com/multica-ai/multica/server/internal/daemon/execenv" "github.com/multica-ai/multica/server/internal/daemon/repocache" "github.com/multica-ai/multica/server/pkg/agent" ) @@ -896,6 +897,71 @@ func newRepoReadyTestDaemon(t *testing.T, handler http.HandlerFunc) *Daemon { return d } +func TestGateResumeToReusedWorkdir(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + sessionID string + priorDir string + envDir string + wantSession string + wantReused bool + }{ + { + name: "same workdir keeps session", + sessionID: "sess-1", + priorDir: "/ws/task-a/workdir", + envDir: "/ws/task-a/workdir", + wantSession: "sess-1", + wantReused: true, + }, + { + name: "fresh workdir drops session", + sessionID: "sess-1", + priorDir: "/ws/task-a/workdir", + envDir: "/ws/task-b/workdir", + wantSession: "", + wantReused: false, + }, + { + name: "session without recorded workdir drops session", + sessionID: "sess-1", + priorDir: "", + envDir: "/ws/task-b/workdir", + wantSession: "", + wantReused: false, + }, + { + name: "no prior session is a no-op", + sessionID: "", + priorDir: "/ws/task-a/workdir", + envDir: "/ws/task-b/workdir", + wantSession: "", + wantReused: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + task := Task{PriorSessionID: tt.sessionID, PriorWorkDir: tt.priorDir} + taskCtx := execenv.TaskContextForEnv{PriorSessionResumed: tt.sessionID != ""} + + reused := gateResumeToReusedWorkdir(&task, &taskCtx, tt.envDir, slog.Default()) + + if reused != tt.wantReused { + t.Fatalf("reused = %v, want %v", reused, tt.wantReused) + } + if task.PriorSessionID != tt.wantSession { + t.Fatalf("PriorSessionID = %q, want %q", task.PriorSessionID, tt.wantSession) + } + if taskCtx.PriorSessionResumed != (tt.wantSession != "") { + t.Fatalf("PriorSessionResumed = %v, want %v", taskCtx.PriorSessionResumed, tt.wantSession != "") + } + }) + } +} + func TestExecuteAndDrain_ResumeFailureFallback(t *testing.T) { t.Parallel() From 0cbb834f9684252a858e5ff8cb76806cd2ec7edd Mon Sep 17 00:00:00 2001 From: Bohan Jiang <52446949+Bohan-J@users.noreply.github.com> Date: Thu, 11 Jun 2026 14:35:51 +0800 Subject: [PATCH 04/20] docs: merge latest changelog entries (#4030) Co-authored-by: J Co-authored-by: multica-agent --- apps/web/features/landing/i18n/en.ts | 28 ++++++++-------------------- apps/web/features/landing/i18n/ja.ts | 28 ++++++++-------------------- apps/web/features/landing/i18n/ko.ts | 28 ++++++++-------------------- apps/web/features/landing/i18n/zh.ts | 28 ++++++++-------------------- 4 files changed, 32 insertions(+), 80 deletions(-) diff --git a/apps/web/features/landing/i18n/en.ts b/apps/web/features/landing/i18n/en.ts index 541b3f8e0f..2b151d40c5 100644 --- a/apps/web/features/landing/i18n/en.ts +++ b/apps/web/features/landing/i18n/en.ts @@ -293,19 +293,25 @@ export function createEnDict(allowSignup: boolean): LandingDict { }, entries: [ { - version: "0.3.20", + version: "0.3.19", date: "2026-06-10", - title: "Safer Comment Triggers and More Reliable Attachments", + title: "Safer Comment Triggers, Reliable Agents, and Attachments", changes: [], features: [ "Comment boxes now show which agents or squads will start work before you send, with controls to avoid accidental runs", "Run transcripts now include timestamps, making agent progress and handoffs easier to review", "Autopilot detail pages now show who created each autopilot", "Claude Fable 5 is now available in Multica's supported model and pricing list", + "Issue conversations can now resolve a specific reply, making long threads easier to close while keeping the final answer visible", + "Lark and Feishu conversations now show a typing reaction while Multica is preparing a reply, then clear it before the answer is sent", + "Agent runs now know who started each task, making handoffs, audit trails, and privacy-aware behavior more accurate", + "OpenClaw users can point Multica at a custom app location and data folder from their local configuration", ], improvements: [ "Comment trigger indicators are quieter, clearer, and less likely to crowd long agent names", "Desktop now disables daemon start and stop controls when the daemon is managed outside Multica, such as in WSL2", + "The active agent indicator in an Issue header is easier to read, with motion only while work is running and clearer queued wording otherwise", + "The CLI now gives clearer guidance around common errors, sign-in problems, and project setup values", ], fixes: [ "Inline images and files in Issue descriptions now stay visible across web and desktop after reloads", @@ -313,24 +319,6 @@ export function createEnDict(allowSignup: boolean): LandingDict { "Issue pages refresh their data after realtime reconnects, avoiding stale timelines after a connection drop", "Agent task initiator history now works more reliably for older task records", "Sticky Issue comments keep a cleaner visual edge while scrolling", - ], - }, - { - version: "0.3.19", - date: "2026-06-09", - title: "More Reliable Agents, Attachments, and Issue Threads", - changes: [], - features: [ - "Issue conversations can now resolve a specific reply, making long threads easier to close while keeping the final answer visible", - "Lark and Feishu conversations now show a typing reaction while Multica is preparing a reply, then clear it before the answer is sent", - "Agent runs now know who started each task, making handoffs, audit trails, and privacy-aware behavior more accurate", - "OpenClaw users can point Multica at a custom app location and data folder from their local configuration", - ], - improvements: [ - "The active agent indicator in an Issue header is easier to read, with motion only while work is running and clearer queued wording otherwise", - "The CLI now gives clearer guidance around common errors, sign-in problems, and project setup values", - ], - fixes: [ "Newly posted attachments now use stable private download links, so images and files stay visible after temporary upload links expire", "Autopilot runs started from newly created Issues now fail cleanly when the assigned task cannot complete, instead of staying stuck", "Inbox deep links now scroll inside the Issue timeline without pushing the desktop window out of place", diff --git a/apps/web/features/landing/i18n/ja.ts b/apps/web/features/landing/i18n/ja.ts index 4eab8b9c9e..9b18baf14e 100644 --- a/apps/web/features/landing/i18n/ja.ts +++ b/apps/web/features/landing/i18n/ja.ts @@ -269,19 +269,25 @@ export function createJaDict(allowSignup: boolean): LandingDict { }, entries: [ { - version: "0.3.20", + version: "0.3.19", date: "2026-06-10", - title: "より安全なコメントトリガーと安定した添付ファイル", + title: "より安全なコメントトリガー、安定したエージェントと添付ファイル", changes: [], features: [ "コメント入力欄では、送信前にどのエージェントやスクワッドが動き始めるかを確認でき、誤って実行することを避けられます。", "実行記録に時刻が表示されるようになり、エージェントの進捗や引き継ぎを振り返りやすくなりました。", "オートパイロット詳細ページで、誰が作成したかを確認できるようになりました。", "Claude Fable 5 が Multica の対応モデルと料金一覧に加わりました。", + "イシューの会話で特定の返信を解決として残せるようになり、長いスレッドを閉じても結論を確認しやすくなりました。", + "Lark と Feishu の会話では、Multica が返信を準備している間に入力中のリアクションを表示し、返信前に自動で消します。", + "エージェント実行は、誰がそのタスクを始めたかを把握できるようになり、引き継ぎ、監査、プライバシーに配慮した動作がより正確になります。", + "OpenClaw ユーザーは、ローカル設定から独自のアプリ場所とデータフォルダーを指定できます。", ], improvements: [ "コメントトリガーの表示はより控えめで読みやすく、長いエージェント名でも混み合いにくくなりました。", "WSL2 など Multica の外でデーモンが管理されている場合、デスクトップは開始と停止の操作を無効にします。", + "イシュー上部のアクティブなエージェント表示は、実行中だけ動き、待機中は待機状態を明確に示すため、読み取りやすくなりました。", + "CLI は、よくあるエラー、サインインの問題、プロジェクト設定の値について、よりわかりやすく案内します。", ], fixes: [ "イシュー説明内の画像とファイルは、Web とデスクトップのどちらでも再読み込み後に表示され続けます。", @@ -289,24 +295,6 @@ export function createJaDict(allowSignup: boolean): LandingDict { "リアルタイム接続が復帰したあと、イシュー画面はデータを更新し、古いタイムラインが残りにくくなりました。", "エージェントタスクの開始者履歴が、古いタスク記録でもより信頼できるようになりました。", "スクロール中の固定イシューコメントの境界がよりきれいに表示されます。", - ], - }, - { - version: "0.3.19", - date: "2026-06-09", - title: "より安定したエージェント、添付ファイル、イシューの会話", - changes: [], - features: [ - "イシューの会話で特定の返信を解決として残せるようになり、長いスレッドを閉じても結論を確認しやすくなりました。", - "Lark と Feishu の会話では、Multica が返信を準備している間に入力中のリアクションを表示し、返信前に自動で消します。", - "エージェント実行は、誰がそのタスクを始めたかを把握できるようになり、引き継ぎ、監査、プライバシーに配慮した動作がより正確になります。", - "OpenClaw ユーザーは、ローカル設定から独自のアプリ場所とデータフォルダーを指定できます。", - ], - improvements: [ - "イシュー上部のアクティブなエージェント表示は、実行中だけ動き、待機中は待機状態を明確に示すため、読み取りやすくなりました。", - "CLI は、よくあるエラー、サインインの問題、プロジェクト設定の値について、よりわかりやすく案内します。", - ], - fixes: [ "新しく投稿された添付ファイルは安定した非公開ダウンロードリンクを使うため、一時的なアップロードリンクが期限切れになっても画像やファイルを表示できます。", "新規イシューから始まったオートパイロット実行は、割り当てられたタスクが完了できない場合に正しく失敗し、進行中のまま残りません。", "受信箱からコメントリンクを開いたとき、デスクトップ画面全体ではなくイシューのタイムラインだけがスクロールします。", diff --git a/apps/web/features/landing/i18n/ko.ts b/apps/web/features/landing/i18n/ko.ts index ec6bb1e7eb..d0172dc9de 100644 --- a/apps/web/features/landing/i18n/ko.ts +++ b/apps/web/features/landing/i18n/ko.ts @@ -268,19 +268,25 @@ export function createKoDict(allowSignup: boolean): LandingDict { }, entries: [ { - version: "0.3.20", + version: "0.3.19", date: "2026-06-10", - title: "더 안전한 댓글 트리거와 더 안정적인 첨부 파일", + title: "더 안전한 댓글 트리거, 안정적인 에이전트와 첨부 파일", changes: [], features: [ "댓글 입력창에서 보내기 전에 어떤 에이전트나 스쿼드가 작업을 시작할지 확인하고, 실수로 실행되는 일을 줄일 수 있습니다.", "실행 기록에 시간이 표시되어 에이전트 진행 상황과 인계를 더 쉽게 검토할 수 있습니다.", "오토파일럿 상세 페이지에서 누가 만들었는지 확인할 수 있습니다.", "Claude Fable 5가 Multica의 지원 모델과 가격 목록에 추가되었습니다.", + "이슈 대화에서 특정 답글을 해결 답변으로 남길 수 있어, 긴 스레드를 접어도 결론을 더 쉽게 확인할 수 있습니다.", + "Lark와 Feishu 대화는 Multica가 답변을 준비하는 동안 입력 중 반응을 표시하고, 답변을 보내기 전에 자동으로 지웁니다.", + "에이전트 실행은 각 작업을 누가 시작했는지 알 수 있어 인계, 감사, 개인정보를 고려한 동작이 더 정확해집니다.", + "OpenClaw 사용자는 로컬 설정에서 사용자 지정 앱 위치와 데이터 폴더를 지정할 수 있습니다.", ], improvements: [ "댓글 트리거 표시가 더 조용하고 명확해졌으며, 긴 에이전트 이름도 덜 비좁게 보입니다.", "WSL2처럼 Multica 밖에서 데몬을 관리하는 경우 데스크톱은 시작과 중지 조작을 비활성화합니다.", + "이슈 헤더의 활성 에이전트 표시가 더 읽기 쉬워졌으며, 실제 실행 중일 때만 움직이고 대기 중일 때는 대기 상태를 명확히 보여 줍니다.", + "CLI는 흔한 오류, 로그인 문제, 프로젝트 설정 값에 대해 더 명확하게 안내합니다.", ], fixes: [ "이슈 설명의 이미지와 파일은 웹과 데스크톱에서 다시 열어도 계속 표시됩니다.", @@ -288,24 +294,6 @@ export function createKoDict(allowSignup: boolean): LandingDict { "실시간 연결이 복구된 뒤 이슈 화면이 데이터를 새로고침해 오래된 타임라인이 남지 않습니다.", "에이전트 작업을 시작한 사람의 기록이 오래된 작업에서도 더 안정적으로 유지됩니다.", "스크롤 중 고정된 이슈 댓글의 가장자리가 더 깔끔하게 보입니다.", - ], - }, - { - version: "0.3.19", - date: "2026-06-09", - title: "더 안정적인 에이전트, 첨부 파일, 이슈 대화", - changes: [], - features: [ - "이슈 대화에서 특정 답글을 해결 답변으로 남길 수 있어, 긴 스레드를 접어도 결론을 더 쉽게 확인할 수 있습니다.", - "Lark와 Feishu 대화는 Multica가 답변을 준비하는 동안 입력 중 반응을 표시하고, 답변을 보내기 전에 자동으로 지웁니다.", - "에이전트 실행은 각 작업을 누가 시작했는지 알 수 있어 인계, 감사, 개인정보를 고려한 동작이 더 정확해집니다.", - "OpenClaw 사용자는 로컬 설정에서 사용자 지정 앱 위치와 데이터 폴더를 지정할 수 있습니다.", - ], - improvements: [ - "이슈 헤더의 활성 에이전트 표시가 더 읽기 쉬워졌으며, 실제 실행 중일 때만 움직이고 대기 중일 때는 대기 상태를 명확히 보여 줍니다.", - "CLI는 흔한 오류, 로그인 문제, 프로젝트 설정 값에 대해 더 명확하게 안내합니다.", - ], - fixes: [ "새로 올린 첨부 파일은 안정적인 비공개 다운로드 링크를 사용해 임시 업로드 링크가 만료된 뒤에도 이미지와 파일이 계속 표시됩니다.", "새 이슈에서 시작된 오토파일럿 실행은 배정된 작업이 완료되지 못하면 올바르게 실패 처리되어 진행 중에 멈춰 있지 않습니다.", "받은함에서 댓글 링크를 열 때 데스크톱 화면 전체가 밀리지 않고 이슈 타임라인 안에서만 스크롤됩니다.", diff --git a/apps/web/features/landing/i18n/zh.ts b/apps/web/features/landing/i18n/zh.ts index 70975423bf..d2b83eb109 100644 --- a/apps/web/features/landing/i18n/zh.ts +++ b/apps/web/features/landing/i18n/zh.ts @@ -293,19 +293,25 @@ export function createZhDict(allowSignup: boolean): LandingDict { }, entries: [ { - version: "0.3.20", + version: "0.3.19", date: "2026-06-10", - title: "更安全的评论触发和更稳定的附件", + title: "更安全的评论触发、更稳定的智能体和附件", changes: [], features: [ "评论输入框现在会在发送前显示哪些智能体或小队会开始工作，也可以避免误触发运行", "智能体运行记录现在会显示时间点，回看进度和交接信息更清楚", "自动任务详情页现在会显示创建人", "Claude Fable 5 现在已加入 Multica 支持的模型和价格列表", + "Issue 讨论可以把某一条回复设为解决结论，长讨论收起后也能直接看到最终答案", + "在 Lark 和飞书里和 Multica 对话时，会显示等待中的输入状态，回复发出后自动清除", + "每次智能体任务都会带上真实发起人信息，交接、审计和权限判断更准确", + "OpenClaw 可以从本地配置中读取自定义程序位置和数据目录", ], improvements: [ "评论触发提示更安静、更清楚，遇到较长的智能体名称时也不容易拥挤", "桌面端在守护进程由 Multica 之外的环境管理时，会禁用启动和停止控制，例如 WSL2 场景", + "Issue 顶部的智能体状态更容易区分：运行中才显示动效，等待中会明确显示排队状态", + "命令行会直接说明常见错误、登录问题和项目配置问题的处理方式", ], fixes: [ "Issue 描述里的图片和文件在网页端和桌面端重新打开后都会保持可见", @@ -313,24 +319,6 @@ export function createZhDict(allowSignup: boolean): LandingDict { "实时连接断开并恢复后，Issue 页面会刷新数据，避免时间线停留在旧状态", "智能体任务的发起人历史在较早任务记录上也会更可靠", "滚动时置顶的 Issue 评论边缘显示更干净", - ], - }, - { - version: "0.3.19", - date: "2026-06-09", - title: "身份上下文优化、附件稳定性和 Issue 讨论升级", - changes: [], - features: [ - "Issue 讨论可以把某一条回复设为解决结论，长讨论收起后也能直接看到最终答案", - "在 Lark 和飞书里和 Multica 对话时，会显示等待中的输入状态，回复发出后自动清除", - "每次智能体任务都会带上真实发起人信息，交接、审计和权限判断更准确", - "OpenClaw 可以从本地配置中读取自定义程序位置和数据目录", - ], - improvements: [ - "Issue 顶部的智能体状态更容易区分：运行中才显示动效，等待中会明确显示排队状态", - "命令行会直接说明常见错误、登录问题和项目配置问题的处理方式", - ], - fixes: [ "新上传的附件会使用稳定的私有下载链接，临时上传链接过期后图片和文件仍能正常显示", "自动任务通过新建 Issue 启动后，如果对应的智能体任务失败，会同步标记为失败，不会一直卡在进行中", "从收件箱打开评论链接时，只会滚动 Issue 时间线，不会把桌面窗口内容顶出可见区域", From 6acca84c28f292d3d1a7160ce437eee404b0f8c6 Mon Sep 17 00:00:00 2001 From: Truffle Date: Wed, 10 Jun 2026 23:54:56 -0700 Subject: [PATCH 05/20] fix(agent): clear stale session id when a resumed ACP session is gone [MUL-3216] (#4015) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(agent): clear stale session id when a resumed ACP session is gone When an agent's stored ACP session no longer exists on the runtime side, session/resume still succeeds — hermes echoes the requested sessionId back — so the failure only surfaces when session/prompt returns JSON-RPC -32603 "Session not found". The backend then reported Status=failed with the stale SessionID still set, which kept the daemon's resume-failure fallback (gated on SessionID == "") from ever firing. The failed task never updates the stored session, so every future mention on the same (agent, issue) dispatched against the same dead id, forever (#4010). handleResponse now returns a structured acpRPCError instead of a flat string (rendered text unchanged), and the hermes/kimi/kiro prompt-error paths clear the session id when the error is session-not-found class on a resumed session. The daemon's existing retry then re-executes with a fresh session and stores the replacement id, healing the mapping. * fix(agent): clear stale session id when set_model hits a dead resumed session With a model override, session/set_model runs before session/prompt, so a resumed session that is gone on the agent side surfaces there instead of at the prompt — and the error branch returned the stale SessionID, so the daemon's fresh-session retry (gated on SessionID == "") never fired. Apply the same clear-the-id fix in the set_model error branch of all three backends. Also relax isACPSessionNotFound to accept -32602: kimi-cli raises RequestError.invalid_params({"session_id": "Session not found"}) for every unknown-session path (src/kimi_cli/acp/server.py), so pinning -32603 made the fix dead code for kimi. The wording gate keeps unrelated invalid_params errors (e.g. "model not available") on the preserve-the-id path. Regression tests for all three backends: resumed session + model override + set_model failing with each runtime's observed session-not-found shape must yield status=failed with an empty SessionID. --- server/pkg/agent/hermes.go | 75 ++++++++++- server/pkg/agent/hermes_test.go | 226 ++++++++++++++++++++++++++++++++ server/pkg/agent/kimi.go | 24 ++++ server/pkg/agent/kimi_test.go | 79 +++++++++++ server/pkg/agent/kiro.go | 24 ++++ server/pkg/agent/kiro_test.go | 76 +++++++++++ 6 files changed, 499 insertions(+), 5 deletions(-) diff --git a/server/pkg/agent/hermes.go b/server/pkg/agent/hermes.go index ad03246a48..930e575308 100644 --- a/server/pkg/agent/hermes.go +++ b/server/pkg/agent/hermes.go @@ -5,6 +5,7 @@ import ( "bytes" "context" "encoding/json" + "errors" "fmt" "io" "log/slog" @@ -298,6 +299,17 @@ func (b *hermesBackend) Execute(ctx context.Context, prompt string, opts ExecOpt b.cfg.Logger.Warn("hermes set_session_model failed", "error", err, "requested_model", opts.Model) finalStatus = "failed" finalError = fmt.Sprintf("hermes could not switch to model %q: %v", opts.Model, err) + if opts.ResumeSessionID != "" && isACPSessionNotFound(err) { + // On a resumed session with a model override, the dead + // session surfaces here instead of at session/prompt. + // Same fix as the prompt path below: clear the id so + // the daemon's resume-failure fallback retries fresh. + b.cfg.Logger.Warn("resumed session not found at set_model time; clearing session id so the daemon retries fresh", + "backend", "hermes", + "session_id", sessionID, + ) + sessionID = "" + } resCh <- Result{ Status: finalStatus, Error: finalError, @@ -338,6 +350,23 @@ func (b *hermesBackend) Execute(ctx context.Context, prompt string, opts ExecOpt } else { finalStatus = "failed" finalError = fmt.Sprintf("hermes session/prompt failed: %v", err) + if opts.ResumeSessionID != "" && isACPSessionNotFound(err) { + // The agent no longer knows the session we resumed. + // Hermes echoes the requested id back from + // session/resume even when the session is gone, so + // resolveResumedSessionID can't catch this — it only + // surfaces here, at prompt time. Return an empty + // SessionID so the daemon's resume-failure fallback + // retries with a fresh session and stores the + // replacement id; keeping the stale id makes every + // future dispatch on this (agent, issue) fail the + // same way. + b.cfg.Logger.Warn("resumed session not found at prompt time; clearing session id so the daemon retries fresh", + "backend", "hermes", + "session_id", sessionID, + ) + sessionID = "" + } } } else { // The prompt completed. Check if we got a promptDone result @@ -607,6 +636,46 @@ func (c *hermesClient) handleAgentRequest(raw map[string]json.RawMessage) { } } +// acpRPCError is a JSON-RPC error frame returned by the agent process. +// It renders exactly like the flat string handleResponse used to build +// with fmt.Errorf, so logs and surfaced task errors are unchanged, but +// keeps the code and message structured so callers can branch on the +// error class (see isACPSessionNotFound) instead of parsing text. +type acpRPCError struct { + Method string + Code int + Message string + Data string +} + +func (e *acpRPCError) Error() string { + if e.Data != "" { + return fmt.Sprintf("%s: %s (code=%d, data=%s)", e.Method, e.Message, e.Code, e.Data) + } + return fmt.Sprintf("%s: %s (code=%d)", e.Method, e.Message, e.Code) +} + +// isACPSessionNotFound reports whether err is the agent rejecting a +// session id it no longer knows. Runtimes signal this with codes and +// wording that vary — Hermes says "Session not found" under -32603 +// (Internal error), Kiro puts "No session found with id ..." in +// `data` under -32603, and kimi-cli raises invalid_params (-32602) +// with {"session_id": "Session not found"} in `data` for every +// unknown-session path (src/kimi_cli/acp/server.py) — so neither the +// code nor the text alone is discriminating and both are matched. +func isACPSessionNotFound(err error) bool { + var rpcErr *acpRPCError + if !errors.As(err, &rpcErr) { + return false + } + if rpcErr.Code != -32603 && rpcErr.Code != -32602 { + return false + } + text := strings.ToLower(rpcErr.Message + " " + rpcErr.Data) + return strings.Contains(text, "session not found") || + strings.Contains(text, "no session found") +} + func (c *hermesClient) handleResponse(raw map[string]json.RawMessage) { var id int if err := json.Unmarshal(raw["id"], &id); err != nil { @@ -651,11 +720,7 @@ func (c *hermesClient) handleResponse(raw map[string]json.RawMessage) { detail = string(rpcErr.Data) } } - if detail != "" { - pr.ch <- rpcResult{err: fmt.Errorf("%s: %s (code=%d, data=%s)", pr.method, rpcErr.Message, rpcErr.Code, detail)} - } else { - pr.ch <- rpcResult{err: fmt.Errorf("%s: %s (code=%d)", pr.method, rpcErr.Message, rpcErr.Code)} - } + pr.ch <- rpcResult{err: &acpRPCError{Method: pr.method, Code: rpcErr.Code, Message: rpcErr.Message, Data: detail}} } else { // If this is a prompt response, extract usage and stop reason. if pr.method == "session/prompt" { diff --git a/server/pkg/agent/hermes_test.go b/server/pkg/agent/hermes_test.go index bc2ec9f1e9..c3d052e37d 100644 --- a/server/pkg/agent/hermes_test.go +++ b/server/pkg/agent/hermes_test.go @@ -3,6 +3,7 @@ package agent import ( "context" "encoding/json" + "fmt" "log/slog" "os" "path/filepath" @@ -1512,6 +1513,231 @@ func TestHermesBackendPromotesProviderErrorWithNonEmptyOutput(t *testing.T) { } } +// TestIsACPSessionNotFound pins the discrimination the resumed-session +// recovery relies on: only a JSON-RPC -32603 whose text names a missing +// session counts. Provider errors (429s, auth failures) and plain +// transport errors must NOT match — those failures happen on sessions +// that still exist, and clearing the id for them would discard a +// healthy session. +func TestIsACPSessionNotFound(t *testing.T) { + t.Parallel() + + cases := []struct { + name string + err error + want bool + }{ + { + name: "hermes session not found in message", + err: &acpRPCError{Method: "session/prompt", Code: -32603, Message: "Session not found"}, + want: true, + }, + { + name: "kiro no session found in data", + err: &acpRPCError{Method: "session/prompt", Code: -32603, Message: "Internal error", Data: "No session found with id ses_abc"}, + want: true, + }, + { + name: "internal error without session wording", + err: &acpRPCError{Method: "session/prompt", Code: -32603, Message: "Internal error", Data: "upstream provider returned HTTP 429"}, + want: false, + }, + { + name: "kimi session not found as invalid_params data", + // kimi-cli raises RequestError.invalid_params({"session_id": + // "Session not found"}) for every unknown-session path + // (src/kimi_cli/acp/server.py), so -32602 must match too. + err: &acpRPCError{Method: "session/set_model", Code: -32602, Message: "Invalid params", Data: `{"session_id": "Session not found"}`}, + want: true, + }, + { + name: "invalid params without session wording", + err: &acpRPCError{Method: "session/set_model", Code: -32602, Message: "model not available: bogus-model"}, + want: false, + }, + { + name: "session wording under an unrelated code", + err: &acpRPCError{Method: "session/prompt", Code: -32601, Message: "Session not found"}, + want: false, + }, + { + name: "plain error", + err: fmt.Errorf("session/prompt: Session not found (code=-32603)"), + want: false, + }, + { + name: "wrapped rpc error", + err: fmt.Errorf("request failed: %w", &acpRPCError{Method: "session/prompt", Code: -32603, Message: "Session not found"}), + want: true, + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + if got := isACPSessionNotFound(tc.err); got != tc.want { + t.Errorf("isACPSessionNotFound(%v) = %v, want %v", tc.err, got, tc.want) + } + }) + } +} + +// fakeHermesACPStaleResumeScript impersonates the failure shape from +// GitHub multica#4010: session/resume succeeds and echoes back the +// requested sessionId (hermes' observed behavior even when it no longer +// knows the session), and the subsequent session/prompt then fails with +// JSON-RPC -32603 "Session not found". +func fakeHermesACPStaleResumeScript() string { + return `#!/bin/sh +while IFS= read -r line; do + id=$(printf '%s' "$line" | sed -n 's/.*"id":\([0-9]*\).*/\1/p') + case "$line" in + *'"method":"initialize"'*) + printf '{"jsonrpc":"2.0","id":%s,"result":{"protocolVersion":1,"agentCapabilities":{}}}\n' "$id" + ;; + *'"method":"session/resume"'*) + sid=$(printf '%s' "$line" | sed -n 's/.*"sessionId":"\([^"]*\)".*/\1/p') + printf '{"jsonrpc":"2.0","id":%s,"result":{"sessionId":"%s"}}\n' "$id" "$sid" + ;; + *'"method":"session/prompt"'*) + printf '{"jsonrpc":"2.0","id":%s,"error":{"code":-32603,"message":"Session not found"}}\n' "$id" + exit 0 + ;; + esac +done +` +} + +// TestHermesBackendClearsSessionIDWhenResumedSessionNotFound pins the +// fix for GitHub multica#4010: when a resumed session turns out to be +// gone on the agent side (resume echoes the requested id, prompt then +// fails -32603 "Session not found"), the Result must carry an empty +// SessionID. The daemon's resume-failure fallback keys on +// `SessionID == ""` — with the stale id still in the Result, the retry +// never fires and every future dispatch on the same (agent, issue) +// loops on the dead session. +func TestHermesBackendClearsSessionIDWhenResumedSessionNotFound(t *testing.T) { + t.Parallel() + + fakePath := filepath.Join(t.TempDir(), "hermes") + writeTestExecutable(t, fakePath, []byte(fakeHermesACPStaleResumeScript())) + + backend, err := New("hermes", Config{ExecutablePath: fakePath, Logger: slog.Default()}) + if err != nil { + t.Fatalf("new hermes backend: %v", err) + } + + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + + session, err := backend.Execute(ctx, "prompt-ignored", ExecOptions{ + Timeout: 5 * time.Second, + ResumeSessionID: "ses_stale", + }) + if err != nil { + t.Fatalf("execute: %v", err) + } + go func() { + for range session.Messages { + } + }() + + select { + case result, ok := <-session.Result: + if !ok { + t.Fatal("result channel closed without a value") + } + if result.Status != "failed" { + t.Fatalf("expected status=failed, got %q (error=%q)", result.Status, result.Error) + } + if !strings.Contains(result.Error, "Session not found") { + t.Errorf("expected error to surface the session-not-found message, got %q", result.Error) + } + if result.SessionID != "" { + t.Errorf("expected empty session id so the daemon's fresh-session retry fires, got %q", result.SessionID) + } + case <-time.After(10 * time.Second): + t.Fatal("timeout waiting for result") + } +} + +// fakeHermesACPStaleResumeSetModelScript is the model-override variant +// of fakeHermesACPStaleResumeScript: session/resume echoes the requested +// sessionId back, and the dead session then surfaces at +// session/set_model (which runs before session/prompt whenever the +// caller picked a model) with the same -32603 "Session not found". +func fakeHermesACPStaleResumeSetModelScript() string { + return `#!/bin/sh +while IFS= read -r line; do + id=$(printf '%s' "$line" | sed -n 's/.*"id":\([0-9]*\).*/\1/p') + case "$line" in + *'"method":"initialize"'*) + printf '{"jsonrpc":"2.0","id":%s,"result":{"protocolVersion":1,"agentCapabilities":{}}}\n' "$id" + ;; + *'"method":"session/resume"'*) + sid=$(printf '%s' "$line" | sed -n 's/.*"sessionId":"\([^"]*\)".*/\1/p') + printf '{"jsonrpc":"2.0","id":%s,"result":{"sessionId":"%s"}}\n' "$id" "$sid" + ;; + *'"method":"session/set_model"'*) + printf '{"jsonrpc":"2.0","id":%s,"error":{"code":-32603,"message":"Session not found"}}\n' "$id" + exit 0 + ;; + esac +done +` +} + +// TestHermesBackendClearsSessionIDWhenSetModelSessionNotFound pins the +// set_model sibling of the prompt-path fix above: with a model +// override, session/set_model runs before session/prompt, so a dead +// resumed session surfaces there instead. The Result must carry an +// empty SessionID here too, or the daemon's fresh-session retry never +// fires for any agent configured with a model. +func TestHermesBackendClearsSessionIDWhenSetModelSessionNotFound(t *testing.T) { + t.Parallel() + + fakePath := filepath.Join(t.TempDir(), "hermes") + writeTestExecutable(t, fakePath, []byte(fakeHermesACPStaleResumeSetModelScript())) + + backend, err := New("hermes", Config{ExecutablePath: fakePath, Logger: slog.Default()}) + if err != nil { + t.Fatalf("new hermes backend: %v", err) + } + + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + + session, err := backend.Execute(ctx, "prompt-ignored", ExecOptions{ + Timeout: 5 * time.Second, + ResumeSessionID: "ses_stale", + Model: "some-model", + }) + if err != nil { + t.Fatalf("execute: %v", err) + } + go func() { + for range session.Messages { + } + }() + + select { + case result, ok := <-session.Result: + if !ok { + t.Fatal("result channel closed without a value") + } + if result.Status != "failed" { + t.Fatalf("expected status=failed, got %q (error=%q)", result.Status, result.Error) + } + if !strings.Contains(result.Error, `could not switch to model "some-model"`) { + t.Errorf("expected error to name the requested model, got %q", result.Error) + } + if result.SessionID != "" { + t.Errorf("expected empty session id so the daemon's fresh-session retry fires, got %q", result.SessionID) + } + case <-time.After(10 * time.Second): + t.Fatal("timeout waiting for result") + } +} + // fakeHermesACPTransientRetryScript emits a single retryable per- // attempt warning to stderr and then completes with a normal agent // text turn — the situation where the upstream LLM blipped on diff --git a/server/pkg/agent/kimi.go b/server/pkg/agent/kimi.go index 896b276e79..db266e4f5b 100644 --- a/server/pkg/agent/kimi.go +++ b/server/pkg/agent/kimi.go @@ -273,6 +273,17 @@ func (b *kimiBackend) Execute(ctx context.Context, prompt string, opts ExecOptio b.cfg.Logger.Warn("kimi set_session_model failed", "error", err, "requested_model", opts.Model) finalStatus = "failed" finalError = fmt.Sprintf("kimi could not switch to model %q: %v", opts.Model, err) + if opts.ResumeSessionID != "" && isACPSessionNotFound(err) { + // On a resumed session with a model override, the dead + // session surfaces here instead of at session/prompt. + // Same fix as the prompt path below: clear the id so + // the daemon's resume-failure fallback retries fresh. + b.cfg.Logger.Warn("resumed session not found at set_model time; clearing session id so the daemon retries fresh", + "backend", "kimi", + "session_id", sessionID, + ) + sessionID = "" + } resCh <- Result{ Status: finalStatus, Error: finalError, @@ -307,6 +318,19 @@ func (b *kimiBackend) Execute(ctx context.Context, prompt string, opts ExecOptio } else { finalStatus = "failed" finalError = fmt.Sprintf("kimi session/prompt failed: %v", err) + if opts.ResumeSessionID != "" && isACPSessionNotFound(err) { + // See the hermes backend: the runtime echoes the + // requested id back from session/resume even when + // the session is gone, so the stale id only fails + // here, at prompt time. Empty SessionID lets the + // daemon's resume-failure fallback retry fresh and + // store the replacement id. + b.cfg.Logger.Warn("resumed session not found at prompt time; clearing session id so the daemon retries fresh", + "backend", "kimi", + "session_id", sessionID, + ) + sessionID = "" + } } } else { select { diff --git a/server/pkg/agent/kimi_test.go b/server/pkg/agent/kimi_test.go index b4df8124dd..5734908130 100644 --- a/server/pkg/agent/kimi_test.go +++ b/server/pkg/agent/kimi_test.go @@ -149,6 +149,85 @@ func TestKimiBackendSetModelFailureFailsTask(t *testing.T) { } } +// fakeKimiACPStaleResumeSetModelScript impersonates kimi-cli when a +// resumed session is gone and the caller picked a model: +// session/resume echoes the requested sessionId back, then +// session/set_model rejects the unknown session the way kimi-cli +// actually does — RequestError.invalid_params (-32602) with +// {"session_id": "Session not found"} in data +// (src/kimi_cli/acp/server.py, set_session_model). +func fakeKimiACPStaleResumeSetModelScript() string { + return `#!/bin/sh +while IFS= read -r line; do + id=$(printf '%s' "$line" | sed -n 's/.*"id":\([0-9]*\).*/\1/p') + case "$line" in + *'"method":"initialize"'*) + printf '{"jsonrpc":"2.0","id":%s,"result":{"protocolVersion":1,"agentCapabilities":{}}}\n' "$id" + ;; + *'"method":"session/resume"'*) + sid=$(printf '%s' "$line" | sed -n 's/.*"sessionId":"\([^"]*\)".*/\1/p') + printf '{"jsonrpc":"2.0","id":%s,"result":{"sessionId":"%s"}}\n' "$id" "$sid" + ;; + *'"method":"session/set_model"'*) + printf '{"jsonrpc":"2.0","id":%s,"error":{"code":-32602,"message":"Invalid params","data":{"session_id":"Session not found"}}}\n' "$id" + exit 0 + ;; + esac +done +` +} + +// TestKimiBackendClearsSessionIDWhenSetModelSessionNotFound pins the +// set_model sibling of the resumed-session fix: with a model override, +// session/set_model runs before session/prompt, so a dead resumed +// session surfaces there. The Result must carry an empty SessionID so +// the daemon's fresh-session retry (gated on SessionID == "") fires. +func TestKimiBackendClearsSessionIDWhenSetModelSessionNotFound(t *testing.T) { + t.Parallel() + + fakePath := filepath.Join(t.TempDir(), "kimi") + writeTestExecutable(t, fakePath, []byte(fakeKimiACPStaleResumeSetModelScript())) + + backend, err := New("kimi", Config{ExecutablePath: fakePath, Logger: slog.Default()}) + if err != nil { + t.Fatalf("new kimi backend: %v", err) + } + + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + + session, err := backend.Execute(ctx, "prompt-ignored", ExecOptions{ + Timeout: 5 * time.Second, + ResumeSessionID: "ses_stale", + Model: "kimi-for-coding", + }) + if err != nil { + t.Fatalf("execute: %v", err) + } + go func() { + for range session.Messages { + } + }() + + select { + case result, ok := <-session.Result: + if !ok { + t.Fatal("result channel closed without a value") + } + if result.Status != "failed" { + t.Fatalf("expected status=failed, got %q (error=%q)", result.Status, result.Error) + } + if !strings.Contains(result.Error, `could not switch to model "kimi-for-coding"`) { + t.Errorf("expected error to name the requested model, got %q", result.Error) + } + if result.SessionID != "" { + t.Errorf("expected empty session id so the daemon's fresh-session retry fires, got %q", result.SessionID) + } + case <-time.After(10 * time.Second): + t.Fatal("timeout waiting for result") + } +} + // TestKimiBackendInvokesACPSubcommand pins the argv for `kimi`. An // earlier fix tried passing `--yolo` to bypass per-tool approval // prompts, but the `acp` subcommand in kimi-cli takes no options diff --git a/server/pkg/agent/kiro.go b/server/pkg/agent/kiro.go index d85f2fea23..f6d79473d0 100644 --- a/server/pkg/agent/kiro.go +++ b/server/pkg/agent/kiro.go @@ -257,6 +257,17 @@ func (b *kiroBackend) Execute(ctx context.Context, prompt string, opts ExecOptio b.cfg.Logger.Warn("kiro set_session_model failed", "error", err, "requested_model", opts.Model) finalStatus = "failed" finalError = fmt.Sprintf("kiro could not switch to model %q: %v", opts.Model, err) + if opts.ResumeSessionID != "" && isACPSessionNotFound(err) { + // On a resumed session with a model override, the dead + // session surfaces here instead of at session/prompt. + // Same fix as the prompt path below: clear the id so + // the daemon's resume-failure fallback retries fresh. + b.cfg.Logger.Warn("resumed session not found at set_model time; clearing session id so the daemon retries fresh", + "backend", "kiro", + "session_id", sessionID, + ) + sessionID = "" + } resCh <- Result{ Status: finalStatus, Error: finalError, @@ -296,6 +307,19 @@ func (b *kiroBackend) Execute(ctx context.Context, prompt string, opts ExecOptio } else { finalStatus = "failed" finalError = fmt.Sprintf("kiro session/prompt failed: %v", err) + if opts.ResumeSessionID != "" && isACPSessionNotFound(err) { + // See the hermes backend: the runtime echoes the + // requested id back from session/resume even when + // the session is gone, so the stale id only fails + // here, at prompt time. Empty SessionID lets the + // daemon's resume-failure fallback retry fresh and + // store the replacement id. + b.cfg.Logger.Warn("resumed session not found at prompt time; clearing session id so the daemon retries fresh", + "backend", "kiro", + "session_id", sessionID, + ) + sessionID = "" + } } } else { select { diff --git a/server/pkg/agent/kiro_test.go b/server/pkg/agent/kiro_test.go index 5ac23f34f8..7175f0440f 100644 --- a/server/pkg/agent/kiro_test.go +++ b/server/pkg/agent/kiro_test.go @@ -162,6 +162,82 @@ func TestKiroBackendSetModelFailureFailsTask(t *testing.T) { } } +// fakeKiroACPStaleLoadSetModelScript impersonates kiro when a resumed +// session is gone and the caller picked a model: session/load returns +// an empty result (so the requested id is kept), then +// session/set_model rejects the unknown session with kiro's observed +// wording — -32603 with "No session found with id ..." in data. +func fakeKiroACPStaleLoadSetModelScript() string { + return `#!/bin/sh +while IFS= read -r line; do + id=$(printf '%s' "$line" | sed -n 's/.*"id":\([0-9]*\).*/\1/p') + case "$line" in + *'"method":"initialize"'*) + printf '{"jsonrpc":"2.0","id":%s,"result":{"protocolVersion":1,"agentCapabilities":{"loadSession":true}}}\n' "$id" + ;; + *'"method":"session/load"'*) + printf '{"jsonrpc":"2.0","id":%s,"result":{}}\n' "$id" + ;; + *'"method":"session/set_model"'*) + printf '{"jsonrpc":"2.0","id":%s,"error":{"code":-32603,"message":"Internal error","data":"No session found with id ses_stale"}}\n' "$id" + exit 0 + ;; + esac +done +` +} + +// TestKiroBackendClearsSessionIDWhenSetModelSessionNotFound pins the +// set_model sibling of the resumed-session fix: with a model override, +// session/set_model runs before session/prompt, so a dead resumed +// session surfaces there. The Result must carry an empty SessionID so +// the daemon's fresh-session retry (gated on SessionID == "") fires. +func TestKiroBackendClearsSessionIDWhenSetModelSessionNotFound(t *testing.T) { + t.Parallel() + + fakePath := filepath.Join(t.TempDir(), "kiro-cli") + writeTestExecutable(t, fakePath, []byte(fakeKiroACPStaleLoadSetModelScript())) + + backend, err := New("kiro", Config{ExecutablePath: fakePath, Logger: slog.Default()}) + if err != nil { + t.Fatalf("new kiro backend: %v", err) + } + + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + + session, err := backend.Execute(ctx, "prompt-ignored", ExecOptions{ + Timeout: 5 * time.Second, + ResumeSessionID: "ses_stale", + Model: "auto", + }) + if err != nil { + t.Fatalf("execute: %v", err) + } + go func() { + for range session.Messages { + } + }() + + select { + case result, ok := <-session.Result: + if !ok { + t.Fatal("result channel closed without a value") + } + if result.Status != "failed" { + t.Fatalf("expected status=failed, got %q (error=%q)", result.Status, result.Error) + } + if !strings.Contains(result.Error, `could not switch to model "auto"`) { + t.Errorf("expected error to name the requested model, got %q", result.Error) + } + if result.SessionID != "" { + t.Errorf("expected empty session id so the daemon's fresh-session retry fires, got %q", result.SessionID) + } + case <-time.After(10 * time.Second): + t.Fatal("timeout waiting for result") + } +} + func TestKiroBackendInvokesACPWithTrustAllTools(t *testing.T) { t.Parallel() From 0985bad9fd3038e7790d1089a0e445b6b5dcfb7d Mon Sep 17 00:00:00 2001 From: Naiyuan Qing <145280634+NevilleQingNY@users.noreply.github.com> Date: Thu, 11 Jun 2026 15:45:53 +0800 Subject: [PATCH 06/20] fix(issues): render thread replies in chronological order (#3691) (#4033) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit collectThreadReplies walked the parent_id tree depth-first, so an agent reply forced to nest under its trigger comment rendered before earlier sibling replies (A-D-B-C instead of A-B-C-D) whenever the agent returned late. Sort the collected subtree by created_at (id tie-break) so the thread reads in arrival order — the same order the server already feeds agents via `comment list --thread` (ListThreadCommentsForIssue). All other consumers of the array (resolution derivation, fold bars, counts, deep-link) are order-independent. Co-authored-by: Claude Fable 5 --- .../issues/components/thread-utils.test.ts | 63 +++++++++++++++++++ .../views/issues/components/thread-utils.ts | 18 ++++-- 2 files changed, 76 insertions(+), 5 deletions(-) create mode 100644 packages/views/issues/components/thread-utils.test.ts diff --git a/packages/views/issues/components/thread-utils.test.ts b/packages/views/issues/components/thread-utils.test.ts new file mode 100644 index 0000000000..2c1f9c76ce --- /dev/null +++ b/packages/views/issues/components/thread-utils.test.ts @@ -0,0 +1,63 @@ +import { describe, expect, it } from "vitest"; +import type { TimelineEntry } from "@multica/core/types"; +import { collectThreadReplies } from "./thread-utils"; + +function comment(id: string, createdAt: string, parentId: string | null): TimelineEntry { + return { + type: "comment", + id, + actor_type: "member", + actor_id: "user-1", + content: id, + parent_id: parentId, + created_at: createdAt, + updated_at: createdAt, + comment_type: "comment", + } as TimelineEntry; +} + +function bucketByParent(entries: TimelineEntry[]): Map { + const map = new Map(); + for (const e of entries) { + if (!e.parent_id) continue; + const list = map.get(e.parent_id) ?? []; + list.push(e); + map.set(e.parent_id, list); + } + return map; +} + +describe("collectThreadReplies", () => { + it("orders a late nested reply after earlier sibling replies (#3691)", () => { + // R1 (50m ago) triggered a slow agent; R2 (30m) and R3 (10m) arrived while + // it ran; D (3m ago) is the agent's reply, forced to nest under R1. A + // depth-first walk yields R1-D-R2-R3; the thread must read R1-R2-R3-D. + const r1 = comment("r1", "2026-06-11T10:00:00Z", "root"); + const r2 = comment("r2", "2026-06-11T10:20:00Z", "root"); + const r3 = comment("r3", "2026-06-11T10:40:00Z", "root"); + const d = comment("d", "2026-06-11T10:47:00Z", "r1"); + + const out = collectThreadReplies("root", bucketByParent([r1, r2, r3, d])); + + expect(out.map((e) => e.id)).toEqual(["r1", "r2", "r3", "d"]); + }); + + it("still returns every descendant across nesting levels", () => { + const r1 = comment("r1", "2026-06-11T10:00:00Z", "root"); + const d1 = comment("d1", "2026-06-11T10:05:00Z", "r1"); + const d2 = comment("d2", "2026-06-11T10:10:00Z", "d1"); + + const out = collectThreadReplies("root", bucketByParent([r1, d1, d2])); + + expect(out.map((e) => e.id)).toEqual(["r1", "d1", "d2"]); + }); + + it("breaks created_at ties by id so the order is deterministic", () => { + const b = comment("b", "2026-06-11T10:00:00Z", "root"); + const a = comment("a", "2026-06-11T10:00:00Z", "b"); + + const out = collectThreadReplies("root", bucketByParent([b, a])); + + expect(out.map((e) => e.id)).toEqual(["a", "b"]); + }); +}); diff --git a/packages/views/issues/components/thread-utils.ts b/packages/views/issues/components/thread-utils.ts index ab821e8b81..a47074fb91 100644 --- a/packages/views/issues/components/thread-utils.ts +++ b/packages/views/issues/components/thread-utils.ts @@ -1,11 +1,19 @@ import type { TimelineEntry } from "@multica/core/types"; +import { sortTimelineEntriesAsc } from "@multica/core/issues/timeline-sort"; /** * Walks the parent_id graph rooted at `rootId` and returns every descendant in - * traversal order. Shared between CommentCard (which renders the expanded - * thread) and ResolvedThreadBar (which displays the collapsed count + author - * list) so the two views stay in sync — direct-children-only counts diverge - * once nested replies exist (see Emacs review on PR #2300). + * CHRONOLOGICAL order (created_at ASC, id tie-break). Shared between + * CommentCard (which renders the expanded thread) and ResolvedThreadBar + * (which displays the collapsed count + author list) so the two views stay in + * sync — direct-children-only counts diverge once nested replies exist (see + * Emacs review on PR #2300). + * + * Chronological, not depth-first: agent replies are forced to nest under the + * comment that triggered them, so a depth-first walk lets a slow agent's late + * reply render BEFORE earlier sibling replies (#3691). The server's --thread + * output the agent reads is already chronological (ListThreadCommentsForIssue + * in comment.sql); this keeps the UI on the same order. */ export function collectThreadReplies( rootId: string, @@ -20,7 +28,7 @@ export function collectThreadReplies( } }; walk(rootId); - return out; + return sortTimelineEntriesAsc(out); } /** From 5957454dd96578aa3458f61e7393de0855d94eed Mon Sep 17 00:00:00 2001 From: Multica Eve Date: Thu, 11 Jun 2026 17:46:36 +0800 Subject: [PATCH 07/20] docs: add June 11 changelog entry (#4037) Co-authored-by: Eve Co-authored-by: multica-agent --- apps/web/features/landing/i18n/en.ts | 19 +++++++++++++++++++ apps/web/features/landing/i18n/ja.ts | 19 +++++++++++++++++++ apps/web/features/landing/i18n/ko.ts | 19 +++++++++++++++++++ apps/web/features/landing/i18n/zh.ts | 19 +++++++++++++++++++ 4 files changed, 76 insertions(+) diff --git a/apps/web/features/landing/i18n/en.ts b/apps/web/features/landing/i18n/en.ts index 2b151d40c5..4b3b7a1534 100644 --- a/apps/web/features/landing/i18n/en.ts +++ b/apps/web/features/landing/i18n/en.ts @@ -292,6 +292,25 @@ export function createEnDict(allowSignup: boolean): LandingDict { fixes: "Bug Fixes", }, entries: [ + { + version: "0.3.20", + date: "2026-06-11", + title: "Skill Imports, Cleaner Run History, and Resilient Agents", + changes: [], + features: [ + "Skill imports now let you choose what happens when a skill already exists: stop, replace it, save a renamed copy, or skip it", + "Import results now clearly show which skills were added, updated, skipped, blocked by a conflict, or could not be imported", + ], + improvements: [ + "Execution logs now show the newest past runs first on web and mobile, so recent progress is easier to scan", + "Changelog content was cleaned up so the latest release notes stay grouped under the right release", + ], + fixes: [ + "Issue thread replies now stay in the order they arrived, even when a slower agent reply lands later", + "Agents can recover when a saved session disappears, starting fresh instead of failing again on every mention", + "Reviving an Issue from a new workspace folder now starts a fresh session instead of retrying one that only existed in the old folder", + ], + }, { version: "0.3.19", date: "2026-06-10", diff --git a/apps/web/features/landing/i18n/ja.ts b/apps/web/features/landing/i18n/ja.ts index 9b18baf14e..d1bb9d8f52 100644 --- a/apps/web/features/landing/i18n/ja.ts +++ b/apps/web/features/landing/i18n/ja.ts @@ -268,6 +268,25 @@ export function createJaDict(allowSignup: boolean): LandingDict { fixes: "バグ修正", }, entries: [ + { + version: "0.3.20", + date: "2026-06-11", + title: "スキルのインポート、実行履歴、より安定したエージェント", + changes: [], + features: [ + "スキルのインポート時に同じスキルがすでにある場合、停止、置き換え、別名で保存、スキップを選べるようになりました。", + "インポート結果では、追加、更新、スキップ、競合、失敗したスキルがわかりやすく表示されます。", + ], + improvements: [ + "Web とモバイルの実行履歴は新しい過去実行を先に表示するため、最近の進捗を追いやすくなりました。", + "変更履歴の内容を整理し、最新のリリースノートが正しいバージョンにまとまるようにしました。", + ], + fixes: [ + "イシューの返信は到着した順番のまま表示され、遅れて届いたエージェント返信が途中に割り込まなくなりました。", + "保存済みセッションが失われた場合でも、エージェントは新しく開始して復旧でき、以後のメンションで失敗し続けません。", + "新しい作業フォルダーからイシューを再開すると、古いフォルダーにだけ存在したセッションではなく新しいセッションで始まります。", + ], + }, { version: "0.3.19", date: "2026-06-10", diff --git a/apps/web/features/landing/i18n/ko.ts b/apps/web/features/landing/i18n/ko.ts index d0172dc9de..46fbfed9c2 100644 --- a/apps/web/features/landing/i18n/ko.ts +++ b/apps/web/features/landing/i18n/ko.ts @@ -267,6 +267,25 @@ export function createKoDict(allowSignup: boolean): LandingDict { fixes: "버그 수정", }, entries: [ + { + version: "0.3.20", + date: "2026-06-11", + title: "스킬 가져오기, 실행 기록, 더 안정적인 에이전트", + changes: [], + features: [ + "스킬을 가져올 때 같은 스킬이 이미 있으면 중단, 교체, 이름을 바꿔 저장, 건너뛰기 중에서 선택할 수 있습니다.", + "가져오기 결과에서 추가, 업데이트, 건너뜀, 충돌, 실패한 스킬을 더 명확하게 확인할 수 있습니다.", + ], + improvements: [ + "웹과 모바일 실행 기록은 최신 과거 실행을 먼저 보여 주어 최근 진행 상황을 더 쉽게 확인할 수 있습니다.", + "변경 로그 콘텐츠를 정리해 최신 릴리스 노트가 올바른 버전에 묶이도록 했습니다.", + ], + fixes: [ + "이슈 스레드 답글은 도착한 순서대로 표시되어, 늦게 도착한 에이전트 답글이 중간에 끼어들지 않습니다.", + "저장된 세션이 사라져도 에이전트가 새로 시작해 복구할 수 있어, 이후 멘션마다 계속 실패하지 않습니다.", + "새 작업 폴더에서 이슈를 다시 시작할 때 이전 폴더에만 있던 세션을 재시도하지 않고 새 세션으로 시작합니다.", + ], + }, { version: "0.3.19", date: "2026-06-10", diff --git a/apps/web/features/landing/i18n/zh.ts b/apps/web/features/landing/i18n/zh.ts index d2b83eb109..e61cc2e18f 100644 --- a/apps/web/features/landing/i18n/zh.ts +++ b/apps/web/features/landing/i18n/zh.ts @@ -292,6 +292,25 @@ export function createZhDict(allowSignup: boolean): LandingDict { fixes: "问题修复", }, entries: [ + { + version: "0.3.20", + date: "2026-06-11", + title: "技能导入、运行记录和更稳定的智能体", + changes: [], + features: [ + "导入技能时，如果同名技能已存在，现在可以选择停止、替换、另存为新名称或跳过", + "导入结果会清楚显示哪些技能已新增、已更新、已跳过、发生冲突或导入失败", + ], + improvements: [ + "网页端和移动端的执行记录现在会优先显示最新的历史运行，更容易看清最近进展", + "更新日志内容已整理，最新发布内容会归在正确的版本下", + ], + fixes: [ + "Issue 讨论里的回复现在会按到达顺序显示，即使较慢的智能体回复稍后才出现，也不会插到前面", + "当已保存的会话失效时，智能体可以自动重新开始，不会在后续每次提及时反复失败", + "从新的工作目录重新唤起 Issue 时，现在会开始新会话，不会继续尝试只存在于旧目录里的会话", + ], + }, { version: "0.3.19", date: "2026-06-10", From 5c136f8557af1dae71d43e00c4135c3c7140c993 Mon Sep 17 00:00:00 2001 From: yuhaowin <34699768+yuhaowin@users.noreply.github.com> Date: Fri, 12 Jun 2026 12:59:54 +0800 Subject: [PATCH 08/20] fix(lark): fix auth race and redirect param in LarkBindPage (#4047) Two bugs prevented the Lark binding flow from completing for already-logged-in users: 1. The useEffect ran before AuthInitializer's getMe() returned, setting state to needs-auth; the guard then blocked re-entry once auth loaded. 2. The sign-in redirect used ?redirect= but the login page reads ?next=. Co-authored-by: Claude Sonnet 4.6 Co-authored-by: multica-agent --- packages/views/lark/bind-page.test.tsx | 120 +++++++++++++++++++++++++ packages/views/lark/bind-page.tsx | 8 +- 2 files changed, 125 insertions(+), 3 deletions(-) create mode 100644 packages/views/lark/bind-page.test.tsx diff --git a/packages/views/lark/bind-page.test.tsx b/packages/views/lark/bind-page.test.tsx new file mode 100644 index 0000000000..7352d00f03 --- /dev/null +++ b/packages/views/lark/bind-page.test.tsx @@ -0,0 +1,120 @@ +import { describe, it, expect, vi, beforeEach } from "vitest"; +import { render, screen, waitFor, fireEvent } from "@testing-library/react"; +import type { ReactNode } from "react"; +import { I18nProvider } from "@multica/core/i18n/react"; +import enCommon from "../locales/en/common.json"; + +const TEST_RESOURCES = { en: { common: enCommon } }; + +// --------------------------------------------------------------------------- +// Hoisted mutable auth state — lets individual tests set different scenarios +// --------------------------------------------------------------------------- +const mockAuthState = vi.hoisted(() => ({ + user: null as { id: string; email: string } | null, + isLoading: false, +})); + +const mockNavigatePush = vi.hoisted(() => vi.fn()); +const mockRedeemToken = vi.hoisted(() => vi.fn()); + +vi.mock("@multica/core/auth", () => { + const useAuthStore = Object.assign( + (sel?: (s: typeof mockAuthState) => unknown) => + sel ? sel(mockAuthState) : mockAuthState, + { getState: () => mockAuthState }, + ); + return { useAuthStore }; +}); + +vi.mock("../navigation", () => ({ + useNavigation: () => ({ push: mockNavigatePush }), +})); + +vi.mock("@multica/core/api", () => ({ + api: { redeemLarkBindingToken: mockRedeemToken }, +})); + +import { LarkBindPage } from "./bind-page"; + +function I18nWrapper({ children }: { children: ReactNode }) { + return ( + + {children} + + ); +} + +function renderPage(token: string | null) { + return render(, { wrapper: I18nWrapper }); +} + +describe("LarkBindPage", () => { + beforeEach(() => { + mockAuthState.user = null; + mockAuthState.isLoading = false; + mockNavigatePush.mockReset(); + mockRedeemToken.mockReset(); + }); + + it("shows redeeming text while auth is still loading (not needs-auth)", () => { + mockAuthState.isLoading = true; + mockAuthState.user = null; + renderPage("tok123"); + expect(screen.getByText(/redeeming binding token/i)).toBeInTheDocument(); + expect(screen.queryByRole("button", { name: /sign in/i })).toBeNull(); + }); + + it("shows needs-auth UI when auth finishes loading and user is null", () => { + mockAuthState.isLoading = false; + mockAuthState.user = null; + renderPage("tok123"); + expect( + screen.getByRole("button", { name: /sign in/i }), + ).toBeInTheDocument(); + }); + + it("starts redemption immediately when user is already logged in", async () => { + mockAuthState.isLoading = false; + mockAuthState.user = { id: "u1", email: "u@example.com" }; + mockRedeemToken.mockResolvedValue({ + workspace_id: "ws1", + installation_id: "inst1", + }); + renderPage("tok123"); + await waitFor(() => { + expect(mockRedeemToken).toHaveBeenCalledWith("tok123"); + }); + }); + + it("shows success state after successful redemption", async () => { + mockAuthState.isLoading = false; + mockAuthState.user = { id: "u1", email: "u@example.com" }; + mockRedeemToken.mockResolvedValue({ + workspace_id: "ws1", + installation_id: "inst1", + }); + renderPage("tok123"); + await waitFor(() => { + expect(screen.getByText(/you're bound/i)).toBeInTheDocument(); + }); + }); + + it("sign-in button navigates with ?next= parameter (not ?redirect=)", () => { + mockAuthState.isLoading = false; + mockAuthState.user = null; + renderPage("mytoken"); + fireEvent.click(screen.getByRole("button", { name: /sign in/i })); + expect(mockNavigatePush).toHaveBeenCalledTimes(1); + const url: string = mockNavigatePush.mock.calls[0]?.[0] as string; + expect(url).toContain("?next="); + expect(url).not.toContain("?redirect="); + expect(url).toContain(encodeURIComponent("mytoken")); + }); + + it("shows missing token error when token is null", () => { + renderPage(null); + expect( + screen.getByText(/missing its binding token/i), + ).toBeInTheDocument(); + }); +}); diff --git a/packages/views/lark/bind-page.tsx b/packages/views/lark/bind-page.tsx index d1e9e4973e..8c13223618 100644 --- a/packages/views/lark/bind-page.tsx +++ b/packages/views/lark/bind-page.tsx @@ -29,6 +29,7 @@ type RedeemState = export function LarkBindPage({ token }: { token: string | null }) { const { t } = useT("common"); const user = useAuthStore((s) => s.user); + const isAuthLoading = useAuthStore((s) => s.isLoading); const navigation = useNavigation(); const [state, setState] = useState({ kind: "idle" }); @@ -37,11 +38,12 @@ export function LarkBindPage({ token }: { token: string | null }) { setState({ kind: "error", reason: "missing_token" }); return; } + if (isAuthLoading) return; if (!user) { setState({ kind: "needs-auth" }); return; } - if (state.kind !== "idle") return; + if (state.kind !== "idle" && state.kind !== "needs-auth") return; setState({ kind: "redeeming" }); (async () => { try { @@ -58,7 +60,7 @@ export function LarkBindPage({ token }: { token: string | null }) { }); } })(); - }, [token, user, state.kind]); + }, [token, user, isAuthLoading, state.kind]); return (
@@ -76,7 +78,7 @@ export function LarkBindPage({ token }: { token: string | null }) { size="sm" onClick={() => navigation.push( - `/login?redirect=${encodeURIComponent( + `/login?next=${encodeURIComponent( `/lark/bind?token=${encodeURIComponent(token ?? "")}`, )}`, ) From 21ff178ac0c323a6f0ca93e775588cf3c948365f Mon Sep 17 00:00:00 2001 From: Bohan Jiang <52446949+Bohan-J@users.noreply.github.com> Date: Fri, 12 Jun 2026 13:09:28 +0800 Subject: [PATCH 09/20] MUL-2701: hide raw creator UUID in skill import conflict UI (#3498) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 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 * 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 * feat(skills): resolve local import conflicts in desktop Co-authored-by: multica-agent * fix(skills): preserve bulk flow after conflict resolution Co-authored-by: multica-agent * fix(skills): show creator name instead of UUID in import conflict UI When a local skill import hits a name conflict with a skill owned by another user, the locked-creator message rendered the raw existing_created_by UUID via the {{creator}} placeholder, which is unreadable. Resolve the UUID against the workspace member list and render the display name instead. When the creator has left the workspace (or the member list hasn't loaded), fall back to the unbranded conflict_locked message rather than leak the UUID. Adds two test cases covering both branches. MUL-2701 Co-authored-by: multica-agent --------- Co-authored-by: J Co-authored-by: multica-agent Co-authored-by: Eve --- .../runtime-local-skill-import-panel.test.tsx | 81 +++++++++++++++++++ .../runtime-local-skill-import-panel.tsx | 12 ++- 2 files changed, 90 insertions(+), 3 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 dab605bdfb..b6c7c2dbb4 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 @@ -15,6 +15,13 @@ const TEST_RESOURCES = { const mockResolveRuntimeLocalSkillImport = vi.hoisted(() => vi.fn()); const mockRuntimeListOptions = vi.hoisted(() => vi.fn()); const mockRuntimeLocalSkillsOptions = vi.hoisted(() => vi.fn()); +const mockListMembers = vi.hoisted(() => vi.fn()); + +vi.mock("@multica/core/api", () => ({ + api: { + listMembers: (...args: unknown[]) => mockListMembers(...args), + }, +})); vi.mock("@multica/core/hooks", () => ({ useWorkspaceId: () => "ws-1", @@ -151,6 +158,10 @@ describe("RuntimeLocalSkillImportPanel", () => { status: "created", skill: MOCK_IMPORTED_SKILL_A, }); + mockListMembers.mockResolvedValue([ + { user_id: "user-1", name: "Alice", email: "alice@example.com" }, + { user_id: "user-2", name: "Bob", email: "bob@example.com" }, + ]); }); it("imports a single skill when selected via checkbox", async () => { @@ -528,4 +539,74 @@ describe("RuntimeLocalSkillImportPanel", () => { expect(onBulkDone).toHaveBeenCalledTimes(1); expect(onImported).not.toHaveBeenCalled(); }); + + it("renders the creator's display name for non-overwritable conflicts", async () => { + mockResolveRuntimeLocalSkillImport.mockResolvedValueOnce({ + status: "conflict", + conflict: { + existing_skill_id: "existing-skill-1", + existing_created_by: "user-2", + can_overwrite: false, + }, + }); + + 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); + + // Bob is user-2 in the mocked member list. The locked message must show + // the resolved name, never the raw UUID. + expect( + await screen.findByText(/created by Bob/i, {}, { timeout: 5000 }), + ).toBeInTheDocument(); + expect(screen.queryByText(/user-2/)).not.toBeInTheDocument(); + }); + + it("falls back to the unbranded locked message when the creator left the workspace", async () => { + mockResolveRuntimeLocalSkillImport.mockResolvedValueOnce({ + status: "conflict", + conflict: { + existing_skill_id: "existing-skill-1", + existing_created_by: "user-gone", + can_overwrite: false, + }, + }); + + 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); + + // user-gone is not in the workspace; the UI must not leak the UUID and + // should render the no-creator variant of the message. + expect( + await screen.findByText( + /Only the creator can overwrite this skill/i, + {}, + { timeout: 5000 }, + ), + ).toBeInTheDocument(); + expect(screen.queryByText(/user-gone/)).not.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 7a902a3494..799b290796 100644 --- a/packages/views/skills/components/runtime-local-skill-import-panel.tsx +++ b/packages/views/skills/components/runtime-local-skill-import-panel.tsx @@ -28,6 +28,7 @@ import { resolveRuntimeLocalSkillImport, } from "@multica/core/runtimes"; import { + memberListOptions, skillDetailOptions, workspaceKeys, } from "@multica/core/workspace/queries"; @@ -316,6 +317,8 @@ function ConflictResolutionPanel({ onSkipAll: () => void; }) { const { t } = useT("skills"); + const wsId = useWorkspaceId(); + const { data: members = [] } = useQuery(memberListOptions(wsId)); const single = conflicts.length === 1; const canOverwriteAny = conflicts.some((r) => r.conflict?.can_overwrite); @@ -366,7 +369,10 @@ function ConflictResolutionPanel({ action: r.conflict?.can_overwrite ? "overwrite" : "rename", renameName: defaultRenameName(r.name), } satisfies ConflictResolutionState); - const creator = r.conflict?.existing_created_by; + const creatorId = r.conflict?.existing_created_by; + const creatorName = creatorId + ? members.find((m) => m.user_id === creatorId)?.name + : undefined; return (
@@ -378,9 +384,9 @@ function ConflictResolutionPanel({ )} {!r.conflict?.can_overwrite && (

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

From c510515da7a76486195a69894503033ebf5033cf Mon Sep 17 00:00:00 2001 From: Bohan Jiang <52446949+Bohan-J@users.noreply.github.com> Date: Fri, 12 Jun 2026 13:37:35 +0800 Subject: [PATCH 10/20] fix: suggest daemon profiles for empty disk usage - suggest other profile workspace roots when disk-usage sees an empty selected root - include the default profile in reverse suggestions and shell-quote profile arguments - keep JSON output and explicit --workspaces-root behavior unchanged MUL-3232 --- server/cmd/multica/cmd_daemon.go | 150 ++++++++++++++++++++++++++ server/cmd/multica/cmd_daemon_test.go | 113 +++++++++++++++++++ 2 files changed, 263 insertions(+) diff --git a/server/cmd/multica/cmd_daemon.go b/server/cmd/multica/cmd_daemon.go index 8d46aca0ee..15498ea4a1 100644 --- a/server/cmd/multica/cmd_daemon.go +++ b/server/cmd/multica/cmd_daemon.go @@ -10,6 +10,7 @@ import ( "os" "os/exec" "path/filepath" + "sort" "strconv" "strings" "time" @@ -735,9 +736,11 @@ func runDaemonDiskUsage(cmd *cobra.Command, _ []string) error { if byWorkspace { printDiskUsageWorkspaceTable(os.Stdout, report) + printDiskUsageEmptyHint(os.Stdout, report, profile, rootOverride) return nil } printDiskUsageTaskTable(os.Stdout, report) + printDiskUsageEmptyHint(os.Stdout, report, profile, rootOverride) return nil } @@ -814,6 +817,153 @@ func printDiskUsageWorkspaceTable(w io.Writer, report daemon.DiskUsageReport) { formatBytes(report.TotalArtifactSizeBytes), report.TotalArtifactRatio*100) } +func printDiskUsageEmptyHint(w io.Writer, report daemon.DiskUsageReport, profile, rootOverride string) { + if report.TotalTaskCount != 0 || rootOverride != "" { + return + } + suggestions := diskUsageProfileSuggestions(profile, report.WorkspacesRoot) + if len(suggestions) == 0 { + return + } + fmt.Fprintln(w) + fmt.Fprintln(w, "Other workspace roots contain task directories:") + for _, s := range suggestions { + fmt.Fprintf(w, " %s # %s (%d task%s)\n", + s.Command, s.Root, s.TaskCount, pluralS(s.TaskCount)) + } +} + +type diskUsageProfileSuggestion struct { + Profile string + Command string + Root string + TaskCount int +} + +func diskUsageProfileSuggestions(currentProfile, currentRoot string) []diskUsageProfileSuggestion { + out := make([]diskUsageProfileSuggestion, 0) + if currentProfile != "" { + if root, err := daemon.ResolveWorkspacesRoot("", ""); err == nil && !samePath(root, currentRoot) { + if taskCount := countDiskUsageTaskDirs(root); taskCount > 0 { + out = append(out, diskUsageProfileSuggestion{ + Profile: "", + Command: "multica daemon disk-usage", + Root: root, + TaskCount: taskCount, + }) + } + } + } + + profilesRoot, err := profilesRootDir() + if err != nil { + return out + } + entries, err := os.ReadDir(profilesRoot) + if err != nil { + return out + } + for _, entry := range entries { + if !entry.IsDir() { + continue + } + profile := entry.Name() + if profile == currentProfile { + continue + } + root, err := daemon.ResolveWorkspacesRoot(profile, "") + if err != nil || samePath(root, currentRoot) { + continue + } + taskCount := countDiskUsageTaskDirs(root) + if taskCount == 0 { + continue + } + out = append(out, diskUsageProfileSuggestion{ + Profile: profile, + Command: "multica --profile " + shellQuoteArg(profile) + " daemon disk-usage", + Root: root, + TaskCount: taskCount, + }) + } + sort.Slice(out, func(i, j int) bool { + if out[i].TaskCount == out[j].TaskCount { + return out[i].Profile < out[j].Profile + } + return out[i].TaskCount > out[j].TaskCount + }) + const maxSuggestions = 5 + if len(out) > maxSuggestions { + out = out[:maxSuggestions] + } + return out +} + +func shellQuoteArg(s string) string { + if s == "" { + return "''" + } + if strings.IndexFunc(s, func(r rune) bool { + return !(r == '-' || r == '_' || r == '.' || r == '/' || + r >= '0' && r <= '9' || + r >= 'A' && r <= 'Z' || + r >= 'a' && r <= 'z') + }) == -1 { + return s + } + return "'" + strings.ReplaceAll(s, "'", "'\\''") + "'" +} + +func countDiskUsageTaskDirs(root string) int { + wsEntries, err := os.ReadDir(root) + if err != nil { + return 0 + } + count := 0 + for _, wsEntry := range wsEntries { + if !wsEntry.IsDir() || wsEntry.Name() == ".repos" { + continue + } + taskEntries, err := os.ReadDir(filepath.Join(root, wsEntry.Name())) + if err != nil { + continue + } + for _, taskEntry := range taskEntries { + if taskEntry.IsDir() { + count++ + } + } + } + return count +} + +func profilesRootDir() (string, error) { + home, err := os.UserHomeDir() + if err != nil { + return "", err + } + return filepath.Join(home, ".multica", "profiles"), nil +} + +func samePath(a, b string) bool { + if a == "" || b == "" { + return false + } + aa, errA := filepath.Abs(a) + bb, errB := filepath.Abs(b) + if errA != nil || errB != nil { + return a == b + } + return aa == bb +} + +func pluralS(n int) string { + if n == 1 { + return "" + } + return "s" +} + // formatRatio renders a 0..1 fraction as a percentage to one decimal. A // non-finite or negative input collapses to "0.0%" — total=0 workspaces // shouldn't surface "NaN%". diff --git a/server/cmd/multica/cmd_daemon_test.go b/server/cmd/multica/cmd_daemon_test.go index 5db31fd8ae..4e412e3440 100644 --- a/server/cmd/multica/cmd_daemon_test.go +++ b/server/cmd/multica/cmd_daemon_test.go @@ -2,8 +2,12 @@ package main import ( "bytes" + "os" + "path/filepath" "strings" "testing" + + "github.com/multica-ai/multica/server/internal/daemon" ) // TestDaemonAlive locks in the liveness predicate the lifecycle commands rely @@ -127,6 +131,86 @@ func TestPrintDaemonStatusAlignsValuesWithProfileLabel(t *testing.T) { } } +func TestPrintDiskUsageEmptyHintSuggestsProfilesWithTasks(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + t.Setenv("MULTICA_WORKSPACES_ROOT", "") + + mkdirProfile(t, home, "empty") + mkdirProfile(t, home, "one-task") + mkdirProfile(t, home, "space profile") + mkdirProfile(t, home, "two-tasks") + + writeDiskUsageTaskFile(t, home, "one-task", "ws1", "task1", "workdir/main.go") + writeDiskUsageTaskFile(t, home, "space profile", "ws3", "task1", "workdir/main.go") + writeDiskUsageTaskFile(t, home, "two-tasks", "ws2", "task1", "workdir/main.go") + writeDiskUsageTaskFile(t, home, "two-tasks", "ws2", "task2", "workdir/main.go") + + var out bytes.Buffer + printDiskUsageEmptyHint(&out, daemon.DiskUsageReport{ + WorkspacesRoot: filepath.Join(home, "multica_workspaces"), + }, "", "") + + got := out.String() + if !strings.Contains(got, "Other workspace roots contain task directories:") { + t.Fatalf("hint output = %q, want profile suggestion header", got) + } + if !strings.Contains(got, "multica --profile two-tasks daemon disk-usage") { + t.Fatalf("hint output = %q, want two-tasks profile command", got) + } + if !strings.Contains(got, "multica --profile one-task daemon disk-usage") { + t.Fatalf("hint output = %q, want one-task profile command", got) + } + if !strings.Contains(got, "multica --profile 'space profile' daemon disk-usage") { + t.Fatalf("hint output = %q, want shell-quoted profile command", got) + } + if strings.Contains(got, "empty") { + t.Fatalf("hint output = %q, want empty profile omitted", got) + } + if strings.Index(got, "two-tasks") > strings.Index(got, "one-task") { + t.Fatalf("hint output = %q, want larger profile first", got) + } +} + +func TestPrintDiskUsageEmptyHintSuggestsDefaultFromNamedProfile(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + t.Setenv("MULTICA_WORKSPACES_ROOT", "") + + writeDefaultDiskUsageTaskFile(t, home, "ws0", "task0", "workdir/main.go") + + var out bytes.Buffer + printDiskUsageEmptyHint(&out, daemon.DiskUsageReport{ + WorkspacesRoot: filepath.Join(home, "multica_workspaces_named"), + }, "named", "") + + got := out.String() + if !strings.Contains(got, "multica daemon disk-usage") { + t.Fatalf("hint output = %q, want default profile command", got) + } + if strings.Contains(got, "--profile") { + t.Fatalf("hint output = %q, want default profile command without --profile", got) + } +} + +func TestPrintDiskUsageEmptyHintSkipsExplicitRootOverride(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + t.Setenv("MULTICA_WORKSPACES_ROOT", "") + + mkdirProfile(t, home, "has-task") + writeDiskUsageTaskFile(t, home, "has-task", "ws1", "task1", "workdir/main.go") + + var out bytes.Buffer + printDiskUsageEmptyHint(&out, daemon.DiskUsageReport{ + WorkspacesRoot: filepath.Join(home, "custom-root"), + }, "", filepath.Join(home, "custom-root")) + + if got := out.String(); got != "" { + t.Fatalf("hint output = %q, want no hint for explicit root override", got) + } +} + func valueColumn(t *testing.T, line string) int { t.Helper() colon := strings.Index(line, ":") @@ -141,3 +225,32 @@ func valueColumn(t *testing.T, line string) int { t.Fatalf("line missing value: %q", line) return 0 } + +func mkdirProfile(t *testing.T, home, profile string) { + t.Helper() + if err := os.MkdirAll(filepath.Join(home, ".multica", "profiles", profile), 0o755); err != nil { + t.Fatal(err) + } +} + +func writeDiskUsageTaskFile(t *testing.T, home, profile, workspaceID, taskID, rel string) { + t.Helper() + path := filepath.Join(home, "multica_workspaces_"+profile, workspaceID, taskID, rel) + writeDiskUsageFile(t, path) +} + +func writeDefaultDiskUsageTaskFile(t *testing.T, home, workspaceID, taskID, rel string) { + t.Helper() + path := filepath.Join(home, "multica_workspaces", workspaceID, taskID, rel) + writeDiskUsageFile(t, path) +} + +func writeDiskUsageFile(t *testing.T, path string) { + t.Helper() + if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(path, []byte("x"), 0o644); err != nil { + t.Fatal(err) + } +} From 9f720a401ce20dd6869b7948116105c162914c48 Mon Sep 17 00:00:00 2001 From: Bohan Jiang <52446949+Bohan-J@users.noreply.github.com> Date: Fri, 12 Jun 2026 14:21:59 +0800 Subject: [PATCH 11/20] fix(desktop): improve renderer recovery prompt (#4056) Co-authored-by: J Co-authored-by: multica-agent --- .../src/main/renderer-recovery.test.ts | 28 ++++++++++++++++++- apps/desktop/src/main/renderer-recovery.ts | 22 +++++++++++---- 2 files changed, 44 insertions(+), 6 deletions(-) diff --git a/apps/desktop/src/main/renderer-recovery.test.ts b/apps/desktop/src/main/renderer-recovery.test.ts index afacaaeb4d..9704b163a9 100644 --- a/apps/desktop/src/main/renderer-recovery.test.ts +++ b/apps/desktop/src/main/renderer-recovery.test.ts @@ -1,5 +1,5 @@ import { describe, expect, it, vi, beforeEach, afterEach } from "vitest"; -import { installRendererRecoveryHandlers } from "./renderer-recovery"; +import { createElectronReloadPrompt, installRendererRecoveryHandlers } from "./renderer-recovery"; type Handler = (...args: unknown[]) => void; @@ -109,4 +109,30 @@ describe("installRendererRecoveryHandlers", () => { expect(showReloadPrompt).not.toHaveBeenCalled(); expect(fixture.reload).not.toHaveBeenCalled(); }); + + it("shows actionable recovery guidance before diagnostic details", async () => { + let detail = ""; + const showMessageBox = vi.fn( + async (options: { title: string; message: string; detail: string }) => { + detail = options.detail; + return { response: 1 }; + }, + ); + const showReloadPrompt = createElectronReloadPrompt(showMessageBox); + + await showReloadPrompt({ kind: "unresponsive", context: {} }); + + expect(showMessageBox).toHaveBeenCalledWith( + expect.objectContaining({ + title: "Multica needs to reload", + message: "The desktop window has been stuck for a few seconds.", + detail: expect.stringContaining( + "Click Reload to refresh this window and keep using Multica.", + ), + }), + ); + expect(detail).toContain("what you were doing right before this message appeared"); + expect(detail).toContain("Activity Monitor sample"); + expect(detail).toContain("Diagnostic details:\nkind: unresponsive\ncontext: {}"); + }); }); diff --git a/apps/desktop/src/main/renderer-recovery.ts b/apps/desktop/src/main/renderer-recovery.ts index e6df0cfb0f..4e26d052e4 100644 --- a/apps/desktop/src/main/renderer-recovery.ts +++ b/apps/desktop/src/main/renderer-recovery.ts @@ -109,18 +109,30 @@ function isRecoverableRendererExit(details: unknown) { function rendererRecoveryMessage(kind: ReloadPromptPayload["kind"]) { switch (kind) { case "render-process-gone": - return "The desktop renderer process stopped responding or crashed."; + return "The desktop window stopped unexpectedly."; case "preload-error": - return "The desktop preload script failed before the app could start."; + return "The desktop window could not finish starting."; case "unresponsive": - return "The desktop window is not responding."; + return "The desktop window has been stuck for a few seconds."; } } function rendererRecoveryDetail(payload: ReloadPromptPayload) { + const guidance = [ + "Click Reload to refresh this window and keep using Multica.", + "If this keeps happening, please tell us what you were doing right before this message appeared and whether Reload recovered the window.", + ]; + + if (payload.kind === "unresponsive") { + guidance.push( + "For macOS reports, an Activity Monitor sample of the Multica Helper (Renderer) process helps us find what blocked the app.", + ); + } + return [ - "Reloading is the safest recovery path for this window.", + ...guidance, "", + "Diagnostic details:", `kind: ${payload.kind}`, `context: ${JSON.stringify(payload.context)}`, ].join("\n"); @@ -132,4 +144,4 @@ function defaultDevLog(tag: string, ...args: unknown[]) { function formatError(error: unknown) { return error instanceof Error ? (error.stack ?? error.message) : String(error); -} \ No newline at end of file +} From f37d71a443be6fd149c03f2000eab229346b62db Mon Sep 17 00:00:00 2001 From: Bohan Jiang <52446949+Bohan-J@users.noreply.github.com> Date: Fri, 12 Jun 2026 14:57:14 +0800 Subject: [PATCH 12/20] fix: apply single skill overwrite immediately (#4057) Co-authored-by: J Co-authored-by: multica-agent --- .../runtime-local-skill-import-panel.test.tsx | 60 +++++++++++++++++++ .../runtime-local-skill-import-panel.tsx | 28 ++++++--- 2 files changed, 81 insertions(+), 7 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 b6c7c2dbb4..5c13d0818c 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 @@ -465,6 +465,66 @@ describe("RuntimeLocalSkillImportPanel", () => { expect(await screen.findByText("Updated")).toBeInTheDocument(); }); + it("applies a single creator conflict when clicking overwrite", 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(); + + fireEvent.click(screen.getByRole("button", { name: /^Overwrite$/i })); + + 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(); + }); + it("keeps bulk completion behavior when conflict resolution leaves one success", async () => { mockRuntimeLocalSkillsOptions.mockReturnValue({ queryKey: ["runtimes", "local-skills", "runtime-1"], 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 799b290796..9e2deb5d9b 100644 --- a/packages/views/skills/components/runtime-local-skill-import-panel.tsx +++ b/packages/views/skills/components/runtime-local-skill-import-panel.tsx @@ -307,12 +307,14 @@ function ConflictResolutionPanel({ conflicts, resolutions, onChange, + onResolveNow, onOverwriteAll, onSkipAll, }: { conflicts: BulkImportResult[]; resolutions: Record; onChange: (key: string, next: ConflictResolutionState) => void; + onResolveNow?: (key: string, next: ConflictResolutionState) => void; onOverwriteAll: () => void; onSkipAll: () => void; }) { @@ -401,12 +403,17 @@ function ConflictResolutionPanel({ variant={ resolution.action === "overwrite" ? "default" : "outline" } - onClick={() => - onChange(r.key, { + onClick={() => { + const next = { action: "overwrite", renameName: resolution.renameName, - }) - } + } satisfies ConflictResolutionState; + if (single && r.conflict?.can_overwrite && onResolveNow) { + onResolveNow(r.key, next); + } else { + onChange(r.key, next); + } + }} disabled={!r.conflict?.can_overwrite} > @@ -723,7 +730,9 @@ export function RuntimeLocalSkillImportPanel({ })); }; - const handleApplyConflictResolutions = async () => { + const handleApplyConflictResolutions = async ( + resolutionOverrides: Record = {}, + ) => { if (!selectedRuntimeId || pendingConflicts.length === 0) return; const conflicts = [...pendingConflicts]; @@ -748,7 +757,8 @@ export function RuntimeLocalSkillImportPanel({ })); for (const r of conflicts) { - const resolution = conflictResolutions[r.key]; + const resolution = + resolutionOverrides[r.key] ?? conflictResolutions[r.key]; if (!resolution) { applyResult(r.key, { status: "failed", @@ -893,6 +903,10 @@ export function RuntimeLocalSkillImportPanel({ conflicts={pendingConflicts} resolutions={conflictResolutions} onChange={setConflictResolution} + onResolveNow={(key, next) => { + setConflictResolution(key, next); + void handleApplyConflictResolutions({ [key]: next }); + }} onOverwriteAll={() => { setConflictResolutions((prev) => { const next = { ...prev }; @@ -1143,7 +1157,7 @@ export function RuntimeLocalSkillImportPanel({