feat(agent-skills): sync skill catalog from disk so authored skills appear

[DeJeune]

🚨 Branch strategy — read before opening this PR

The v2 refactor has merged into main, so main is the default branch for active development (v1 and v2 code currently coexist there — expect large, breaking changes).

• Active development (features, refactors, optimizations, fixes for the current codebase) → target main (the default base).
• v1 maintenance (hotfixes and subsequent v1 releases) → branch from and target v1, not main.

What this PR does

Before this PR: skills an agent created via the bundled skill-creator / find-skills skills never appeared in the app — skill-creator called a skills MCP tool that was never wired into a session, find-skills installed globally (npx skills add -g) outside Cherry's library, and reconcileSkills only pushed the DB out to the mirror, so on-disk skills never reached the agent_global_skill catalog the UI reads.

After this PR: the filesystem is the source of truth and the catalog is reconciled from it. reconcileSkills now absorbs agent-authored skills dropped under CLAUDE_CONFIG_DIR/skills into the managed library, adopts/refreshes library skills into the DB, and prunes non-builtin rows whose files were removed (never on a transient scan failure); the dead skills MCP server is deleted; skill-creator/find-skills are retargeted to write into CLAUDE_CONFIG_DIR/skills; and a new skill.reconcile IpcApi route runs when the Skills library opens so authored skills show up without an app restart.

Fixes #

Why we need it and why it was done in this way

Skill visibility depended on an agent voluntarily calling a register tool, which the bundled skills and any out-of-band file edit routinely bypassed — an inherently unreliable sync. Reconciling the catalog from disk removes that dependency entirely.

The following tradeoffs were made: agent_global_skill is kept as a filesystem-reconciled cache rather than dropped, because per-agent enablement still needs a DB home and dropping the table would not remove the need to handle files disappearing; pruning is guarded so a transient scan failure can never wipe the catalog and its enablement.

The following alternatives were considered: (1) wiring the skills MCP server so the agent registers explicitly — rejected as unreliable (depends on the model remembering); (2) dropping the catalog table and reading skills purely from disk — deferred, since it forces provenance metadata into sidecar files and is a larger surgery.

Links to places where the discussion took place: N/A

Breaking changes

None. Existing catalog rows, builtin skills, and per-agent enablement are untouched; reconcile only adds/refreshes/prunes to match what is on disk.

Special notes for your reviewer

• Not yet wired: a live main→renderer push (broadcast('skill.changed')) for skills authored while the Skills page is already open — reconcile currently runs on library open, not on every mid-session change.
• The find-skills install path assumes the skills CLI prints its install location (the agent then copies the folder into CLAUDE_CONFIG_DIR/skills); the exact CLI flag behavior was not verified against a live run.

Checklist

• Branch: This PR targets the correct branch — main for active development, v1 for v1 maintenance fixes
• PR: The PR description is expressive enough and will help future contributors
• Code: Write code that humans can understand and Keep it simple
• Refactor: You have left the code cleaner than you found it (Boy Scout Rule)
• Upgrade: Impact of this change on upgrade flows was considered and addressed if required
• Documentation: A user-guide update was considered and is present (link) or not required. Check this only when the PR introduces or changes a user-facing feature or behavior.
• Self-review: I have reviewed my own code (e.g., via /gh-pr-review, gh pr diff, or GitHub UI) before requesting review from others

Release note

Skills created or installed by an agent (via the skill-creator / find-skills built-in skills) now appear in the app's skill library automatically, without an app restart.

[kangfenmao]

This review was translated automatically.

Overall direction is correct: making the filesystem the source of truth for Skill content can avoid relying on the model to actively call register. However, the current implementation still has risks of the main path being unusable, enabled relationships being accidentally deleted, and user files being overwritten/lost. Recommend fixing before merging.

Critical - Must Fix Before Merge

1. resources/skills/skill-creator/SKILL.md:15-21, resources/skills/find-skills/SKILL.md:145-152: CLAUDE_CONFIG_DIR is not a stable Cherry write path.

   Impact: settingsBuilder.ts:611-621 deletes this variable in macOS Claude Code login mode, commands may fall to /skills; other platforms rewrite it to the user's actual ~/.claude, while reconcile scans Cherry's feature.agents.claude.skills. Additionally, Windows mirror is a real copy; existing Skills modified in the mirror will be lost because absorbDroppedSkills() skips the tracked folder, then linkMirror() recopies with canonical old content.

   Recommendation: Add CHERRY_STUDIO_SKILLS_DIR that cannot be overwritten by users (points to Cherry canonical or dedicated staging entry); the two built-in Skills should only use it; .claude/skills should only be a one-way, rebuildable projection, no longer used as a write API. Add macOS/non-macOS tests for external-provider, and tests for Windows where existing Skills are modified without content loss.

2. src/main/ai/skills/SkillService.ts:915-973,1011-1020: A single SKILL.md read failure is treated as Skill deletion.

   Impact: readSkillMd() swallows all errors including EACCES, EIO, and transient ENOENT and returns null; the directory therefore doesn't enter onDisk, then deleteById() hard deletes the catalog row. agent_skill.skill_id is ON DELETE CASCADE, so all Agents' enable/disable status for that Skill is permanently lost. This is inconsistent with the PR description of "never on a transient scan failure"; existing tests only cover the entire root directory not existing, not single file failure.

   Recommendation: Separate presentFolders from readableSkills, giving read results three states: found / missing / error; when a directory has been enumerated but the descriptor is unreadable, only warn, never prune. External deletion should have at least double confirmation/grace period, or only explicit uninstall does hard-delete. Add tests for EACCES, EIO, and atomic replacement short window, and assert original agent_global_skill.id and agent_skill are preserved.

3. src/main/ai/skills/SkillService.ts:837-846,899-974: Reconcile and install/update/builtin copy are not unified and serialized.

   Impact: Reconcile can be triggered at startup and when the page opens. It can first get an old disk snapshot, then after installation completes read the new DB row, and finally delete the just-installed row based on the old snapshot. When updating an existing Skill, the foo.bak temporarily generated by SkillInstaller.ts:31-47 may be registered as a new local Skill, while the original row and enablement are pruned; builtins may also be preemptively adopted as local between copying and registration, and no longer enabled by default as builtins.

   Recommendation: Use the repository's existing async-mutex to make installSkillDir, update, uninstall, full reconcile, and builtin's "copy file + sync DB" share the same global mutation mutex; just doing single-flight for reconcile itself is not enough. Put backup/staging in a hidden directory explicitly ignored by the scanner; form the change plan inside the lock, then submit the DB batch with a single synchronous withWriteTx. Use deferred barrier tests to force concurrency, assert final row ID, source, provenance, and agent_skill remain unchanged.

4. src/main/ai/skills/SkillService.ts:857-890: Absorb's cp(...force) -> rm is not atomic, failure may delete the only complete original.

   Impact: If fs.cp has already copied SKILL.md, then fails on a supporting file due to disk full or I/O error, canonical will be left with a half-finished product; the same reconcile round will still register this half-finished product, then the mirror phase deletes the original complete directory. When target already exists, it will also silently merge two Skills with the same name. If the mirror root itself is a symlink/junction, rm(src) may also go beyond Cherry's management scope.

   Recommendation: Explicitly report conflict when target exists, prohibit force merge; copy to a unique staging, fully parse/hash/security check before atomic publish, keep source directory before DB success, clear staging on failure and prevent that folder from participating in subsequent stages of this round. Validate root's lstat + realpath before destructive absorb, reject root symlink/junction, and reuse installer's safe copy strategy. Add regression tests for "EIO after copying SKILL.md", "same name conflict", and "root pointing to external directory".

Important - Should Fix In This PR

5. resources/skills/find-skills/SKILL.md:139-146: npx skills add ... -y defaults to project scope within a project, first leaving a project-level Skill in the user's workspace, then copying to Cherry.

   Recommendation: Best to directly reuse SkillService's installation logic; if CLI must be called, run --agent claude-code --copy -y in mktemp staging, import from known staging path after, then clean up, must not modify user's project.

6. src/main/ai/skills/SkillService.ts:777-781,1011-1019: Scanner and parser accept lowercase skill.md, but linkMirror() only checks uppercase SKILL.md, resulting in "UI has record, Agent cannot use".

   Recommendation: Unify all stages to reuse findSkillMdPath(); if SDK only accepts uppercase, then atomically normalize filename at staging publish. Add lowercase-only adopt + mirror test.

7. src/renderer/hooks/resourceCatalog/skillAdapter.ts:33-49: Reconcile is only attached to global Skill library. Agent edit page and creation wizard use useInstalledSkills() which only queries DB, so after user creates a Skill and goes directly to Agent's Skills page, they still can't see it; must first open global library or restart; when request fails, ref is already set to true, same page has no retry entry.

   Recommendation: Extract shared ensureSkillsReconciled(), cover all Skill list entries, let main do single-flight/mutex; allow retry on failure with visible feedback.

Additionally, this PR deletes the Skills MCP server, but src/shared/ai/claudecode/toolRegistry.ts:39,356-363 and related architecture docs still retain skills server/tool descriptions. Recommend cleaning up these dead registrations and outdated docs in the same PR.

Recommend converging the final data flow to: Agent writes to staging -> validates -> atomically publishes to Data/Skills -> conservatively syncs DB -> unidirectionally generates .claude/skills mirror. This can simultaneously solve path drift, Windows overwrite, half-finished publish, and concurrent race conditions.

---

Original Content

整体方向是对的：让文件系统成为 Skill 内容事实源，可以避免依赖模型主动调用 register。但当前实现仍有主路径不可用、启用关系被误删以及用户文件被覆盖/丢失的风险，建议修复后再合并。

Critical - Must Fix Before Merge

1. resources/skills/skill-creator/SKILL.md:15-21、resources/skills/find-skills/SKILL.md:145-152：CLAUDE_CONFIG_DIR 不是稳定的 Cherry 写入路径。

   影响： settingsBuilder.ts:611-621 在 macOS Claude Code 登录模式下会删除该变量，命令可能落到 /skills；其他平台会把它改成用户真实的 ~/.claude，而 reconcile 扫描的是 Cherry 的 feature.agents.claude.skills。此外 Windows mirror 是真实副本，已有 Skill 在 mirror 中被修改后，会因 absorbDroppedSkills() 跳过 tracked folder、随后 linkMirror() 用 canonical 旧内容重拷贝而丢失修改。

   建议： 增加不可被用户覆盖的 CHERRY_STUDIO_SKILLS_DIR（指向 Cherry canonical 或专用 staging 入口），两个内置 Skill 只使用它；.claude/skills 只作为单向、可重建的投影，不再作为写入 API。补 external-provider 的 macOS/非 macOS 测试，以及 Windows 修改已有 Skill 后内容不丢失的测试。

2. src/main/ai/skills/SkillService.ts:915-973,1011-1020：单个 SKILL.md 暂时读失败会被当成 Skill 已删除。

   影响： readSkillMd() 吞掉 EACCES、EIO 和瞬时 ENOENT 等所有错误并返回 null；该目录因此不进入 onDisk，随后 deleteById() 硬删 catalog row。agent_skill.skill_id 是 ON DELETE CASCADE，所有 Agent 对该 Skill 的启用/禁用状态也会永久丢失。这与 PR 描述的“never on a transient scan failure”不一致；现有测试只覆盖整个根目录不存在，没有覆盖单文件失败。

   建议： 分开维护 presentFolders 与 readableSkills，让读取结果具有 found / missing / error 三态；目录已被枚举到但 descriptor 不可读时只告警、绝不 prune。外部删除至少做二次确认/宽限期，或仅显式 uninstall 才 hard-delete。补 EACCES、EIO、原子替换短窗口的测试，并断言原 agent_global_skill.id 与 agent_skill 均保留。

3. src/main/ai/skills/SkillService.ts:837-846,899-974：reconcile 与 install/update/builtin copy 没有统一串行化。

   影响： 启动时和页面打开时都能触发 reconcile。它可以先取得旧磁盘快照，安装完成后再读到新 DB row，最后依据旧快照删掉刚安装的 row。更新已有 Skill 时，SkillInstaller.ts:31-47 暂时生成的 foo.bak 还可能被注册成新 local Skill，同时原 row 与 enablement 被 prune；builtin 在复制与登记之间也可能被抢先收养为 local，以后不再按 builtin 默认启用。

   建议： 使用仓库已有的 async-mutex，让 installSkillDir、update、uninstall、完整 reconcile，以及 builtin 的“复制文件 + sync DB”共用同一把全局 mutation mutex；只给 reconcile 本身做 single-flight 不够。把 backup/staging 放入扫描器明确忽略的隐藏目录；锁内先形成变更计划，再用一次同步 withWriteTx 提交 DB batch。用 deferred barrier 测试强制制造并发，断言最终 row ID、source、provenance 和 agent_skill 不变。

4. src/main/ai/skills/SkillService.ts:857-890：absorb 的 cp(...force) -> rm 不是原子操作，失败时可能删除唯一完整原件。

   影响： 如果 fs.cp 已复制 SKILL.md，随后在 supporting file 上因磁盘满或 I/O 错误失败，canonical 会留下半成品；同一轮 reconcile 仍会登记这个半成品，mirror 阶段再删除原始完整目录。目标已存在时还会静默合并两份同名 Skill。若 mirror root 本身是 symlink/junction，rm(src) 还可能越过 Cherry 管理范围。

   建议： 目标存在时明确报冲突，禁止 force merge；复制到唯一 staging，完整 parse/hash/安全校验后再原子 publish，DB 成功前保留源目录，失败时清 staging 并阻止该 folder 参与本轮后续阶段。destructive absorb 前校验 root 的 lstat + realpath，拒绝 root symlink/junction，并复用 installer 的安全复制策略。补“复制出 SKILL.md 后抛 EIO”“同名冲突”“root 指向外部目录”的回归测试。

Important - Should Fix In This PR

5. resources/skills/find-skills/SKILL.md:139-146：npx skills add ... -y 在项目内默认使用 project scope，先在用户工作区留下项目级 Skill，再复制给 Cherry。

   建议： 最好直接复用 SkillService 的安装逻辑；如果必须调用 CLI，在 mktemp staging 中运行 --agent claude-code --copy -y，从已知 staging 路径导入后清理，不得修改用户项目。

6. src/main/ai/skills/SkillService.ts:777-781,1011-1019：扫描器和 parser 接受小写 skill.md，但 linkMirror() 只检查大写 SKILL.md，会出现“UI 有记录、Agent 无法使用”。

   建议： 所有阶段统一复用 findSkillMdPath()；如果 SDK 只接受大写，则在 staging publish 时原子规范化文件名。补 lowercase-only 的 adopt + mirror 测试。

7. src/renderer/hooks/resourceCatalog/skillAdapter.ts:33-49：reconcile 只挂在全局 Skill library。Agent 编辑页和创建向导使用的 useInstalledSkills() 只查询 DB，因此用户创建 Skill 后直接去 Agent 的 Skills 页仍看不到，必须先打开全局库或重启；请求失败时 ref 已被置为 true，同一页面也没有重试入口。

   建议： 抽出共享 ensureSkillsReconciled()，覆盖所有 Skill 列表入口，由 main 做 single-flight/mutex；失败时允许重试并提供可见反馈。

另外，本 PR 删除了 Skills MCP server，但 src/shared/ai/claudecode/toolRegistry.ts:39,356-363 和相关架构文档仍保留 skills server/tool 描述，建议同 PR 清掉这些死注册和过时文档。

推荐把最终数据流收敛为：Agent 写 staging -> 校验后原子发布到 Data/Skills -> 保守同步 DB -> 单向生成 .claude/skills mirror。这样可以同时解决路径漂移、Windows 覆盖、半成品发布和并发竞态。

[kangfenmao]

This review was translated automatically.

Reviewing the latest head 3c3b13e: most issues raised in the previous round have been fixed, including the stable CHERRY_STUDIO_SKILLS_DIR, removal of destructive absorb, unified mutation lock, support for lowercase skill.md, reconciling/retrying both UI entry points, and replacing npx skills with an internal MCP. However, the current implementation still has 3 blocking issues that could install the wrong Skill, overwrite existing content, or lose Agent enablement relationships. It is recommended to fix these before merging.

Critical - Must Fix Before Merge

1. src/main/ai/mcp/servers/skills.ts:24-43,151-164: Search results use the display name name to construct the install identifier, discarding the real metadata.directoryPath returned by the marketplace API.

   Impact: Real data already contains results where the display name differs entirely from the directory, e.g. Agent Development has directory plugins/plugin-dev/skills/agent-development. The current code would generate anthropics/claude-code/Agent Development; after direct directory, exact name, and hyphen fuzzy match all fail, SkillService.resolveSkillDirectory() falls back to the first candidate found in the repo scan, so the user confirms installing A but may end up installing B. The existing src/renderer/utils/skillSearch.ts:64-89 already explicitly uses directoryPath and filters ambiguous results; the new MCP should not reimplement a weaker set of rules.

   Suggestion: Extract Claude Plugins response validation and normalization into shared code, used by both UI and MCP; the identifier must be owner/repo/directoryPath, and results without a reliable directory should not be returned; when explicit directory resolution fails, throw an error directly — do not fall back to the first candidate. Add a regression test for name: "Agent Development", directoryPath: "plugins/plugin-dev/skills/agent-development", and verify the full path actually passed to SkillService.install().

2. src/main/ai/mcp/servers/skills.ts:177-199, src/main/ai/skills/SkillService.ts:544-579: install_skill silently overwrites builtin, local, or other-source Skills with the same folder name.

   Impact: The generic install logic currently only protects source === 'system'. If installing another skill-creator / find-skills, it will directly replace Cherry's built-in files, but the database row still keeps source: 'builtin'; and builtin is enabled by default for all Agents when there is no join row, so this third-party content would apply to all Agents, not just the "current Agent only" as the tool promises. The same conflict would also irrecoverably overwrite the user's own local Skills.

   Suggestion: builtin and system must always be protected from being overwritten by a normal marketplace install; if the existing directory provenance or sourceUrl differs, return an explicit conflict instead of silently replacing. When the exact same source is already installed, only enable it for the current Agent — no need to re-copy; if the product wants to allow same-name coexistence, a non-conflicting managed folder should be generated. Add a builtin/local same-name install test, asserting that the original files, source, row id, and per-Agent enablement all remain unchanged.

3. src/main/ai/skills/SkillService.ts:903-928,975-982,1031-1042: The previous round's cascade deletion issue still does not cover the case where the directory exists but SKILL.md is temporarily ENOENT.

   Impact: The current three-state read only records non-ENOENT as error. If an editor or external tool moves away the old descriptor before writing the new file during save, both casings would briefly return ENOENT, so the directory ends up in neither onDisk nor unreadable, and deleteById() would still delete the catalog row and permanently delete all agent_skill enablement relationships via FK cascade. The newly added test creates SKILL.md as a directory to produce EISDIR, which does not cover this actual ENOENT window.

   Suggestion: Independently record presentFolders during scanning; only prune when the entire Skill folder was not successfully enumerated. When the folder exists but the descriptor is missing, unreadable, or fails to parse, keep the original row and enablement, only log a warning. Add a test for "existing row + agent_skill, directory exists but descriptor ENOENT", and keep the control test for "entire folder missing triggers deletion".

Important

v2-refactor-temp/docs/ai/declarative-tool-registry.md:5-9,72-76,146-152 still states in multiple places that the Skills MCP is "defined but unmounted", which contradicts the current implementation in this PR; please update the server wiring, approval, and security boundary descriptions accordingly.

CI is currently all green, but the above are runtime data and installation correctness issues that existing happy-path tests cannot cover. Recommended fix order: first unify the marketplace normalizer and make directory resolution fail-closed, then add install source conflict protection, and finally refine the present-folder prune semantics and regression tests.

---

Original Content

复审最新 head 3c3b13e：上一轮提出的大部分问题已经修复，包括稳定的 CHERRY_STUDIO_SKILLS_DIR、移除破坏性的 absorb、统一 mutation lock、支持小写 skill.md、补齐两个 UI 入口的 reconcile/retry，以及用内部 MCP 取代 npx skills。不过当前实现仍有 3 个会导致装错 Skill、覆盖已有内容或丢失 Agent 启用关系的阻塞问题，建议修复后再合并。

Critical - Must Fix Before Merge

1. src/main/ai/mcp/servers/skills.ts:24-43,151-164：搜索结果使用展示名称 name 拼安装 identifier，丢掉了市场接口返回的真实 metadata.directoryPath。

   影响： 真实数据中已经存在展示名与目录完全不同的结果，例如 Agent Development 的目录是 plugins/plugin-dev/skills/agent-development。当前代码会生成 anthropics/claude-code/Agent Development；直接目录、精确名称和连字符 fuzzy match 都找不到后，SkillService.resolveSkillDirectory() 会 fallback 到仓库扫描到的第一个候选，因此用户确认安装 A，最终可能安装 B。仓库现有 src/renderer/utils/skillSearch.ts:64-89 已明确使用 directoryPath 并过滤歧义结果，新 MCP 不应重新实现一套更弱的规则。

   建议： 把 Claude Plugins 响应校验和 normalizer 提取到 shared，UI 与 MCP 共用；identifier 必须是 owner/repo/directoryPath，缺少可靠目录的结果不要返回；显式目录解析失败时直接报错，禁止 fallback 到第一个候选。补 name: "Agent Development"、directoryPath: "plugins/plugin-dev/skills/agent-development" 的回归测试，并验证实际传给 SkillService.install() 的完整路径。

2. src/main/ai/mcp/servers/skills.ts:177-199、src/main/ai/skills/SkillService.ts:544-579：install_skill 会静默覆盖同文件夹名的 builtin、local 或其他来源 Skill。

   影响： 通用安装逻辑目前只保护 source === 'system'。如果安装另一个 skill-creator / find-skills，会直接替换 Cherry 内置文件，但数据库 row 仍保持 source: 'builtin'；而 builtin 在没有 join row 时默认对所有 Agent 启用，所以这份第三方内容会作用于全部 Agent，不是工具承诺的“只启用给当前 Agent”。同样的冲突也会不可恢复地覆盖用户自己创作的 local Skill。

   建议： builtin 与 system 一律禁止被普通 marketplace 安装覆盖；已有目录 provenance 或 sourceUrl 不同则返回明确冲突，不能静默替换。完全相同来源已安装时只为当前 Agent enable，不需要重新复制；若产品希望允许同名共存，应生成不冲突的 managed folder。补 builtin/local 同名安装测试，断言原文件、source、row id 和各 Agent 的 enablement 均不变。

3. src/main/ai/skills/SkillService.ts:903-928,975-982,1031-1042：上一轮的级联删除问题仍未覆盖“目录存在，但 SKILL.md 暂时 ENOENT”的情况。

   影响： 当前三态读取只把非 ENOENT 记为 error。如果编辑器或外部工具保存时先移走旧 descriptor、再写入新文件，两种大小写都会短暂返回 ENOENT，于是该目录既不进入 onDisk 也不进入 unreadable，随后 deleteById() 仍会删除 catalog row，并通过 FK cascade 永久删除所有 agent_skill 启用关系。新增测试把 SKILL.md 建成目录来制造 EISDIR，没有覆盖这个实际的 ENOENT 窗口。

   建议： 扫描时独立记录 presentFolders；只有整个 Skill 文件夹没有被成功枚举到时才 prune。文件夹存在但 descriptor 缺失、不可读或解析失败时保留原 row 与 enablement，仅记录告警。补“已有 row + agent_skill，目录存在但 descriptor ENOENT”的测试，并保留“整个文件夹消失才删除”的对照测试。

Important

v2-refactor-temp/docs/ai/declarative-tool-registry.md:5-9,72-76,146-152 仍多处写着 Skills MCP “defined but unmounted”，已与本 PR 当前实现相反；请同步更新 server wiring、approval 和安全边界说明。

CI 当前全绿，但以上属于运行时数据与安装正确性问题，现有 happy-path 测试无法覆盖。推荐修复顺序：先统一 marketplace normalizer 并让目录解析 fail closed，再加入安装来源冲突保护，最后完善 present-folder prune 语义及回归测试。

[zhangjiadi225]

This review was translated automatically.

Additionally confirmed two blocking security issues: install_skill lacks a mandatory authorization gate independent of SDK permission mode in bypassPermissions/headless scenarios; meanwhile, the unvalidated installation identifier can escape the clone root directory via path traversal and copy local directories. The previously identified issues in existing reviews — wrong Skill installation, source override, and enablement cascade loss — also remain unresolved. It is recommended to fix all of them before merging.

---

Original Content

补充确认了两项阻塞安全问题：install_skill 在 bypassPermissions/headless 场景下缺少独立于 SDK permission mode 的强制授权门禁；同时未经校验的安装 identifier 可以通过路径穿越逃逸 clone 根目录并复制本地目录。现有 review 已指出的装错 Skill、来源覆盖和 enablement 级联丢失问题也仍未解决，建议全部修复后再合并。

[zhangjiadi225]

This review comment was translated automatically.

[A4] Here it claims per-use authorization by "not adding install_skill to allowedTools", but this only holds true when the SDK actually calls canUseTool; the same file already explains that under bypassPermissions, the SDK skips this gate. Currently, there is no PreToolUse/runtime mandatory gate covering mcp__skills__install_skill, and since the server is injected into every session, a bypass/headless Agent can install and execute third-party Skills without explicit user confirmation. Please add a mandatory authorization boundary independent of the permission mode (require confirmation in interactive scenarios, reject in headless scenarios), and add regression tests for bypassPermissions and headless modes.

---

Original Content

[A4] 这里通过“不把 install_skill 加入 allowedTools”来声称逐次授权，但这只在 SDK 实际调用 canUseTool 时成立；同一文件已经说明 bypassPermissions 下 SDK 会跳过该门禁。当前也没有覆盖 mcp__skills__install_skill 的 PreToolUse/运行时强制门禁，而 server 被注入每个 session，因此 bypass/headless Agent 可以在没有用户明确确认的情况下安装并执行第三方 Skill。请增加独立于 permission mode 的强制授权边界（交互场景要求确认、headless 场景拒绝），并补 bypassPermissions 与 headless 回归测试。

[DeJeune]

Thanks for the thorough reviews @kangfenmao @zhangjiadi225 — all blocking issues are addressed in 2b837db6. Point-by-point:

Critical

C1 — wrong skill installed (identifier built from the display name).
Extracted the claude-plugins normalizer into @shared/utils/skillMarketplace (normalizeClaudePlugins), now shared by the renderer search UI and the skills MCP. search_skills returns the validated install_source (owner/repo/directoryPath, built from the real directoryPath and dropping entries without a resolvable directory); the model passes it back verbatim and never assembles the identifier from the name. resolveSkillDirectory now fails closed when an explicit directory has no SKILL.md, instead of falling back to the first repo candidate. Regression test with name: "Agent Development" / directoryPath: "plugins/plugin-dev/skills/agent-development" asserts the exact path handed to SkillService.install.

C2 — silent overwrite of builtin/local/other-source skills.
installSkillDir now overwrites in place only for an exact same-source re-install (source + sourceUrl match). A marketplace/local install colliding with a builtin/system/local or different-origin skill of the same folder name throws a conflict — third-party content can no longer replace builtin files while the row keeps source: 'builtin' (and stays enabled for all agents). Test asserts original files/source/row-id/enablement remain unchanged.

C3 — cascade delete on a transient descriptor ENOENT.
Reconcile now prunes only when the whole folder is gone from disk (presentFolders), so an atomic-save window where both SKILL.md casings are briefly ENOENT no longer deletes the catalog row and cascades away agent_skill. Test: folder present + descriptor ENOENT → row and enablement survive; the "whole folder gone → pruned" control remains.

Security

S1 — install_skill authorization gate independent of the SDK permission mode.
Added a skillInstallApprovalHook PreToolUse hook (fires in every permission mode): it denies install_skill in headless / bypassPermissions / acceptEdits turns; normal interactive modes go through per-call approval (it is not on the auto-approve list). search_skills stays read-only and auto-approved.

S2 — path traversal via the install identifier.
resolveSkillDirectory rejects a directoryPath that resolves outside the clone root.

Important

v2-refactor-temp/docs/ai/declarative-tool-registry.md updated — skills is now wired (search + install) with the approval and security-boundary description; the "defined but unmounted" notes are marked historical.

---

All of the above is in 2b837db6. 153 skill-related tests pass and node/web typecheck is clean. Recommended review order was followed: shared marketplace normalizer + fail-closed resolution first, then install-source conflict protection, then present-folder prune semantics and regression tests.

[zhangjiadi225]

LGTM

[zhangjiadi225]

Requesting changes based on the eight newly confirmed issues noted inline. The filesystem-as-source-of-truth direction is sound, but the current write, recovery, and cross-platform boundaries can still overwrite or orphan skills, lose per-agent enablement, execute stale Windows mirror content, and miss the main same-session Composer visibility path. The marketplace MCP also does not yet satisfy the bundled discovery and pre-install review contract.

For de-duplication, I did not repeat existing review threads. The following earlier concerns still apply at ffd68480: the mandatory authorization thread remains unresolved because headless bypassPermissions is explicitly allowed; repository containment is still lexical rather than realpath-based for symlink escape; unrelated local/ZIP installs still compare as the same origin because both persist sourceUrl: null; and builtin copy/version writes remain outside the shared mutation lock even though the DB sync is locked.

Reviewed against exact head ffd68480e74aa29b253682650521d662310c0238. The new CI run was still pending when this review was submitted.

[zhangjiadi225]

[B4] The bundled find-skills contract requires checking GitHub stars/source reputation and presenting an exact review link before installation, but this response drops stars, sourceUrl, and sourceRegistry even though the normalized result already contains them. The model cannot comply deterministically and may have to guess the link or security facts. Please include those fields in the returned view and assert them in the server test.

[zhangjiadi225]

[A1/B8] This reconciles only on the first hook mount. AgentComposer stays mounted while an agent creates a skill; opening its Skills panel calls useAvailableSkills.refresh, which only refetches DataApi/local skills and never calls skill.reconcile. The authored directory is therefore still absent from the DB and the panel misses it until another view triggers reconcile, the component remounts, or the app restarts. Please have the explicit refresh/panel-open event reconcile before refetching, and add a sequence test for create-after-initial-reconcile.

[zhangjiadi225]

[A1/B7] Folder collision lookup remains case-sensitive, while default macOS/Windows filesystems are case-insensitive and the SQLite text unique index uses binary collation. With an existing Foo, installing foo misses this row yet targets the same physical directory; the installer can replace its files and the DB can hold both names, so later uninstall/prune can affect the wrong row. Normalize folder keys consistently (for example to lowercase), or enforce a case-insensitive conflict check and uniqueness constraint before any filesystem mutation.

[DeJeune]

Thanks for the detailed review. I re-evaluated the findings against the goal and architecture of this PR, and I am not claiming that every item is resolved.

Addressed in 4b1894d:

• search_skills now returns stars, source_registry, and source_url, with server-test coverage;
• an explicit Composer Skills refresh now runs skill.reconcile before the DataApi refetch, with a sequence regression test.

Not addressed by that commit:

• multi-registry MCP search parity;
• canonical-library symlink handling;
• case-insensitive folder identity;
• stale Windows mirror cleanup;
• interrupted-install backup recovery;
• realpath containment, local/ZIP provenance, and builtin copy/version locking from the earlier review.

I also did not introduce a SkillsStaging publication layer. Direct writes to the managed canonical library are the selected filesystem-as-source-of-truth design in this PR; adding a second inbox, publish lifecycle, failure-retention policy, and migration boundary would be an architectural change rather than a correction to the permission-mode bug. If authored-skill ownership needs a stronger reservation API, that should be designed explicitly rather than added here as an incidental staging protocol.

The mandatory-confirmation suggestion is intentionally not applied: bypassPermissions is Cherry full-auto mode and must preserve the Claude Agent SDK permission contract. Adding an independent confirmation gate would make that mode no longer full-auto. Headless installation is denied only when the live permission mode still requires approval.

CI for the new head is currently pending. The remaining findings above should be treated as open design/reliability work, not as resolved by this update.

[kangfenmao]

This review was translated automatically.

This round completed a multi-Agent reviewer–verifier review based on the current head 33668a3c. New inline findings cover installation failure causing loss of the original Skill, insufficient copy integrity, builtin cross-source override, historical case conflicts causing reconcile to abort, and install_source not being bound to search results. There are also two blocking issues that cannot be attached to current diff lines: ClawHub installation does not check existing malware/suspicious moderation results; download responses are fully loaded into Electron main process memory via arrayBuffer() before any size limit is applied, which can trigger OOM from oversized responses. The previously raised issues in the thread about headless + bypassPermissions allowing unconfirmed installation, and skill-creator directly writing to the managed library and potentially overwriting existing Skills, also still hold. The above involves user file loss, upgrade compatibility, and third-party code execution boundaries. It is recommended to fix these before merging.

---

Original Content

本轮基于当前 head 33668a3c 完成多 Agent reviewer–verifier 复核。新增 inline findings 覆盖安装失败导致原 Skill 丢失、复制完整性不足、builtin 跨来源覆盖、历史大小写冲突导致 reconcile 中止，以及 install_source 未绑定搜索结果。另有两项无法挂到当前 diff 行的阻塞问题：ClawHub 安装没有检查已有的 malware/suspicious moderation 结果；下载响应在任何体积限制前通过 arrayBuffer() 全量进入 Electron 主进程内存，可被超大响应触发 OOM。此前线程中的 headless + bypassPermissions 无人确认安装，以及 skill-creator 直接写 managed library、可覆盖既有 Skill，也仍然成立。以上涉及用户文件丢失、升级兼容和第三方代码执行边界，建议修复后再合并。

[kangfenmao]

This review comment was translated automatically.

[A1] If rename(destPath, backupPath) fails while the old directory is still intact, hasBackup remains false, but here destPath is deleted unconditionally, and there is no backup to restore afterwards. A failed installation/upgrade will directly lose the original Skill. Please only clean up the target after the new directory has started publishing, or record whether the rename actually succeeded and ensure the old directory is not deleted in this error branch.

---

Original Content

[A1] 如果 rename(destPath, backupPath) 在旧目录仍完整时失败，hasBackup 还保持为 false，这里却无条件删除 destPath，随后也没有备份可恢复。一次安装/升级失败会直接丢失原 Skill。请只在新目录已经开始发布后清理目标，或记录 rename 是否真正成功并确保旧目录不会在该错误分支被删除。

[DeJeune]

Fixed in 8a0294b. Rollback now tracks whether publishing actually started; if moving the original directory to backup fails, the catch path no longer removes destPath. Added a regression test asserting the original is untouched.

[kangfenmao]

This review comment was translated automatically.

[A1] The integrity check here only calculates SKILL.md, but the copy helper function ignores ENOENT that occurs for other files during the copy process. Therefore, when scripts/assets is missed during copying, the two descriptor hashes remain the same, and the code will delete the old backup and publish an incomplete Skill. Please perform a deterministic check on the entire directory, or make the release fail if any source file disappears during the copying process.

---

Original Content

[A1] 这里的完整性校验只计算 SKILL.md，但复制辅助函数会忽略复制途中对其他文件发生的 ENOENT。因此 scripts/assets 漏拷时两个 descriptor hash 仍相同，代码会删除旧备份并发布不完整 Skill。请对完整目录做确定性校验，或让复制过程中任何源文件消失都使发布失败。

[DeJeune]

Fixed in 8a0294b. Publish verification now computes a deterministic hash over the complete copied directory, including regular files and directory structure. A disappeared scripts/assets file now fails the publish and restores the backup. Added coverage where SKILL.md matches but a script differs.

[kangfenmao]

This review comment was translated automatically.

[A1] existing might be a marketplace/local/system Skill with the same name, but here it will overwrite its directory first, and then change the row to builtin. When a new built-in Skill with the same name is added in the next version, it will irreversibly overwrite user content and change the default enabled semantics. Please reject cross-source collisions before any file writes; only allow updating records that have been confirmed to belong to this builtin.

---

Original Content

[A1] existing 可能是同名的 marketplace/local/system Skill，但这里会先覆盖其目录，后面再把该行改成 builtin。下个版本新增同名内置 Skill 时会不可逆覆盖用户内容，并改变默认启用语义。请在任何文件写入前拒绝跨来源碰撞；仅允许更新已经确认属于该 builtin 的记录。

[DeJeune]

Fixed in 8a0294b. syncBuiltinSkill now rejects any existing non-builtin catalog owner before filesystem writes, and unregistered storage is reclaimed only when the builtin marker and full bundled content match. It no longer rewrites local or marketplace rows to builtin. Added regression coverage preserving user files and source.

[kangfenmao]

This review comment was translated automatically.

[A1] If the historical database already has two rows Foo/foo, they will be entirely excluded from dbByFolder due to the conflict group; when one of the directories exists on disk, it will still be inserted as a new Skill here and collide with agent_global_skill_folder_name_unique, causing the entire reconcile to abort, and subsequent prune/mirror repairs will not be executed. Please explicitly isolate or fix the conflict group, and ensure that this disk key no longer enters the insert branch.

---

Original Content

[A1] 历史数据库若已有 Foo/foo 两行，它们会因冲突组被整体排除出 dbByFolder；磁盘存在其中一个目录时，这里仍按新 Skill 插入并撞上 agent_global_skill_folder_name_unique，使整个 reconcile 中止，后续 prune/mirror 修复都不执行。请显式隔离或修复冲突组，并保证该磁盘 key 不再进入 insert 分支。

[DeJeune]

Fixed in 8a0294b. Historical case-colliding DB keys are explicitly quarantined before the disk insert loop, and their mirrors are removed; reconciliation continues with later prune and mirror repairs. A regression test seeds Foo/foo plus a later stale row and verifies there is no unique-index abort.

[kangfenmao]

This review comment was translated automatically.

[A4] install_source only validates non-emptiness and is not bound to the actual results returned by search_skills. The caller can construct any supported source themselves. For skills.sh, when the target basename does not match, resolveSkillDirectory will also fall back to the first candidate in the repository, resulting in 'confirming A, but actually installing B'. Please use the server-issued/cached search result identifier, and fail closed when the explicit target is not hit.

---

Original Content

[A4] install_source 只校验非空，没有绑定到 search_skills 实际返回的结果，调用方可自行构造任意受支持来源。对 skills.sh，目标 basename 不匹配时 resolveSkillDirectory 还会回退到仓库第一个候选，形成“确认 A、实际安装 B”。请使用服务端签发/缓存的搜索结果标识，且显式目标未命中时 fail closed。

[DeJeune]

Fixed in 8a0294b. The MCP server now accepts only install_source values issued by search_skills in that live server session. skills.sh identifiers require an explicit owner/repo/skill target, and a missing exact basename now fails closed instead of using fuzzy or first-candidate fallback. Tests cover unissued sources and missing explicit targets.

[vaayne]

The filesystem-as-source-of-truth direction is sound, but the current ownership and reconciliation boundaries can still lose user data or persist modified builtin instructions across agents. Please address the inline findings before merge; the existing unresolved installation-safety threads remain blocking as well.

[vaayne]

[high] This prune relies only on the readdir snapshot captured at the start of reconciliation, but agent file writes to CHERRY_STUDIO_SKILLS_DIR do not take mutationLock. A reachable sequence is: the agent moves the folder away, reconcile snapshots it as absent, the agent recreates it, and this still deletes the catalog row. The FK then cascades away every agent_skill enablement; a later reconcile re-adopts the files under a new, disabled row. Please lstat the canonical path immediately before deleting and prune only if it is still ENOENT, with a race regression test.

[DeJeune]

Fixed in 8a0294b. Before deleting a non-builtin row, reconciliation now lstat checks the canonical path and keeps the row on any result other than confirmed ENOENT. Added a race regression where the initial readdir snapshot is stale but the folder exists at prune time; the row and agent_skill enablement survive.

[vaayne]

[critical] This accepts content changes for an existing builtin row from the same root exposed to agents as CHERRY_STUDIO_SKILLS_DIR. An agent can overwrite (for example) skill-creator/SKILL.md; reconcile updates its hash/metadata while preserving source: builtin, mirrors it, and the builtin default then loads the modified instructions for other agents that have not explicitly disabled it. The earlier collision guard only protects installs, not direct authoring writes. Please separate authored skills into a distinct writable root/namespace, or reject and restore out-of-band changes to builtin content.

[DeJeune]

Fixed in 8a0294b without introducing a second authoring or staging root. Existing builtin rows are no longer refreshed from direct filesystem writes; builtin content uses a trusted full-directory hash, linkMirror refuses mismatches, and builtins are mirrored as verified copies instead of canonical symlinks so an out-of-band write cannot immediately propagate to other agents. Same-version startup sync restores modified builtin files. Added quarantine and restore regressions.

[vaayne]

[low] Moving steerHook to index 7 fixes this test, but the keeps an empty-text steer pending test below still calls preToolUse![6]. Index 6 is now rtkRewriteHook; with no tool_name it returns {} and leaves the pending steer untouched, so that test passes without exercising steerHook. Please update it to the actual hook (preferably without relying on a fragile array index).

[DeJeune]

Fixed in 8a0294b. Both tests now locate steerHook by callback name rather than an array index, so the empty-text case invokes the actual hook.