fix(agent-skills): address review safety gaps

Signed-off-by: Pleasurecruise <3196812536@qq.com>

Author: Pleasurecruise

resources/skills/find-skills/SKILL.md

@@ -27,8 +27,8 @@ Cherry gives you two built-in tools for skills — use them, and do NOT shell ou
   opaque `install_source` value.
 - **`install_skill(install_source)`** — install ONE skill into Cherry's managed
   library and enable it for this agent. Cherry clones the repo, installs just that one skill,
-  and registers it in a single deterministic step. Permission handling follows the active Claude
-  permission mode (Step 6).
+  and registers it in a single deterministic step. Cherry requires explicit user approval
+  independently of the active Claude permission mode (Step 6).
 
 **Browse skills at:** https://skills.sh/
 
@@ -58,8 +58,9 @@ If the leaderboard doesn't cover the user's need, call the `search_skills` tool:
 - User asks "can you help me with PR reviews?" → `search_skills("pr review")`
 - User asks "I need to create a changelog" → `search_skills("changelog")`
 
-Each result includes an opaque `install_source` — pass that exact value to
-`install_skill`.
+Each result includes an opaque `install_source`, source registry, review URL,
+install count, and available star count. Pass the exact `install_source` value
+to `install_skill`.
 
 ### Step 4: Verify Quality Before Recommending
 
@@ -90,7 +91,7 @@ I can install it into Cherry's skill library for you — want me to go ahead?
 Learn more: https://skills.sh/vercel-labs/agent-skills/react-best-practices
 ```
 
-### Step 6: Install (Uses the Active Permission Mode)
+### Step 6: Install (Requires Explicit Confirmation)
 
 **⚠️ Security:** Skills are third-party code that runs with full agent
 permissions. A malicious skill could read, modify, or delete files on your
@@ -102,13 +103,12 @@ Before installing any skill:
    code and will run with full agent permissions.
 2. **Provide a review link** — the skills.sh page (or source repository) so
    the user can review the skill's SKILL.md and any scripts it contains.
-3. **Require install intent** — call `install_skill` only when the user asked to install the skill
-   or accepted a presented option. A search-only request must not mutate the skill library.
+3. **Ask for explicit confirmation** — call `install_skill` only after the user accepts the
+   security warning and review link. A search-only request must not mutate the skill library.
 
-Once the user has expressed install intent, call `install_skill` with the exact `install_source`
-from the search result. Do not add another model-level confirmation step: Claude's active permission
-mode is the authority. Default and accept-edits modes may prompt through the SDK; bypass-permissions
-mode runs directly.
+Once the user confirms, call `install_skill` with the exact `install_source` from the search result.
+Cherry enforces the approval boundary even when the agent otherwise runs in bypass-permissions mode;
+channel and scheduled turns cannot install third-party skills.
 
 - `install_skill("claude-plugins:vercel-labs/agent-skills/skills/react-best-practices")`
 

resources/skills/skill-creator/SKILL.md

@@ -10,16 +10,17 @@ keeps in sync with the filesystem automatically — there is **no** install or r
 tool to call, and you should **ignore** any `package_skill.py` / `.skill` packaging
 steps mentioned later in this file (they apply to Claude Code / Claude.ai, not here).
 
-**To create a new skill, write it straight into Cherry's managed skills directory:**
+**To create a new skill, write it into Cherry's authoring inbox:**
 
 1. Resolve the directory once by running `echo "$CHERRY_STUDIO_SKILLS_DIR"` in Bash.
-   That folder is Cherry's managed skill library.
+   That folder is Cherry's staging inbox, not the executable skill library.
 2. Create `$CHERRY_STUDIO_SKILLS_DIR/<skill-folder-name>/` and write `SKILL.md` plus any
    supporting files (`scripts/`, `references/`, `assets/`) into it with your normal file
    tools.
-3. That's it. Cherry's skill sync detects the new directory, registers it in the catalog,
-   and lists it in the app — no register step. You can re-edit the files in place at any
-   time and the changes are picked up on the next sync.
+3. That's it. Cherry's skill sync validates and atomically publishes the staged directory,
+   registers it in the catalog, and lists it in the app — no register step. If the folder
+   collides with a builtin, marketplace, or differently-authored skill, Cherry keeps the
+   draft in staging and refuses to overwrite the installed content.
 
 Use a lowercase, hyphenated `<skill-folder-name>` (e.g. `my-cool-skill`). The `name:`
 field inside your `SKILL.md` frontmatter is the display name and may differ from the

src/main/ai/mcp/servers/__tests__/skills.test.ts

@@ -28,7 +28,16 @@ async function callTool(server: SkillsServerInstance, name: string, args: Record
 }
 
 function mockMarketplace(skills: unknown[]) {
-  fetchMock.mockResolvedValue({ ok: true, json: async () => ({ skills }) })
+  fetchMock.mockImplementation(async (url: string) => ({
+    ok: true,
+    status: 200,
+    statusText: 'OK',
+    json: async () => {
+      if (url.startsWith('https://skills.sh/')) return { skills: [] }
+      if (url.startsWith('https://clawhub.ai/')) return { results: [] }
+      return { skills }
+    }
+  }))
 }
 
 describe('SkillsServer', () => {
@@ -50,15 +59,85 @@ describe('SkillsServer', () => {
           namespace: 'vercel-labs',
           description: 'React perf',
           author: 'vercel',
+          stars: 42,
           installs: 100,
-          metadata: { repoOwner: 'vercel-labs', repoName: 'agent-skills', directoryPath: 'skills/react-best-practices' }
+          sourceUrl: 'https://github.com/vercel-labs/agent-skills/tree/main/skills/react-best-practices',
+          metadata: {
+            repoOwner: 'vercel-labs',
+            repoName: 'agent-skills',
+            directoryPath: 'skills/react-best-practices'
+          }
         }
       ])
 
       const result = await callTool(createServer(), 'search_skills', { query: 'react perf' })
+      const payload = JSON.parse(
+        result.content[0].text.slice(result.content[0].text.indexOf('['), result.content[0].text.lastIndexOf(']') + 1)
+      )
 
       expect(result.isError).toBeFalsy()
-      expect(result.content[0].text).toContain('claude-plugins:vercel-labs/agent-skills/skills/react-best-practices')
+      expect(payload).toEqual([
+        expect.objectContaining({
+          stars: 42,
+          source_registry: 'claude-plugins.dev',
+          source_url: 'https://github.com/vercel-labs/agent-skills/tree/main/skills/react-best-practices',
+          install_source: 'claude-plugins:vercel-labs/agent-skills/skills/react-best-practices'
+        })
+      ])
+    })
+
+    it('searches every marketplace supported by the renderer', async () => {
+      fetchMock.mockImplementation(async (url: string) => ({
+        ok: true,
+        status: 200,
+        statusText: 'OK',
+        json: async () => {
+          if (url.startsWith('https://skills.sh/')) {
+            return {
+              query: 'developer tools',
+              count: 1,
+              skills: [
+                {
+                  id: 'owner/repo/web-search',
+                  skillId: 'web-search',
+                  name: 'Web Search',
+                  source: 'owner/repo',
+                  installs: 12
+                }
+              ]
+            }
+          }
+          if (url.startsWith('https://clawhub.ai/')) {
+            return {
+              results: [
+                {
+                  score: 1,
+                  slug: 'code-review',
+                  displayName: 'Code Review',
+                  summary: 'Review code',
+                  version: '1.0.0',
+                  updatedAt: 1,
+                  ownerHandle: 'owner'
+                }
+              ]
+            }
+          }
+          return { skills: [] }
+        }
+      }))
+
+      const result = await callTool(createServer(), 'search_skills', { query: 'developer tools' })
+      const payload = JSON.parse(
+        result.content[0].text.slice(result.content[0].text.indexOf('['), result.content[0].text.lastIndexOf(']') + 1)
+      )
+
+      expect(fetchMock).toHaveBeenCalledTimes(3)
+      expect(payload).toEqual(
+        expect.arrayContaining([
+          expect.objectContaining({ source_registry: 'skills.sh', install_source: 'skills.sh:owner/repo/web-search' }),
+          expect.objectContaining({ source_registry: 'clawhub.ai', install_source: 'clawhub:code-review' })
+        ])
+      )
     })
 
     it('builds install_source from directoryPath, not the display name (regression)', async () => {

src/main/ai/mcp/servers/skills.ts

@@ -3,12 +3,12 @@ import { skillService } from '@main/ai/skills/SkillService'
 import { McpServer } from '@modelcontextprotocol/sdk/server/mcp.js'
 import type { Tool } from '@modelcontextprotocol/sdk/types.js'
 import { CallToolRequestSchema, ErrorCode, ListToolsRequestSchema, McpError } from '@modelcontextprotocol/sdk/types.js'
-import { normalizeClaudePlugins } from '@shared/utils/skillMarketplace'
+import { searchSkillMarketplaces } from '@shared/utils/skillMarketplace'
 import { net } from 'electron'
 
 const logger = loggerService.withContext('McpServer:Skills')
 
-const MARKETPLACE_BASE_URL = 'https://claude-plugins.dev'
+const REQUEST_TIMEOUT_MS = 15_000
 
 const SEARCH_TOOL: Tool = {
   name: 'search_skills',
@@ -29,7 +29,7 @@ const SEARCH_TOOL: Tool = {
 const INSTALL_TOOL: Tool = {
   name: 'install_skill',
   description:
-    "Install ONE marketplace skill into Cherry Studio's managed library and enable it for the current agent. Pass the exact `install_source` string from a search_skills result — do NOT construct it yourself, and do NOT run `npx skills add`, `git clone`, or any shell command. Cherry clones the repo, installs just that single skill, and registers it. Call this only when the user intends to install the skill; the active Claude permission mode controls whether execution prompts or runs directly.",
+    "Install ONE marketplace skill into Cherry Studio's managed library and enable it for the current agent. Pass the exact `install_source` string from a search_skills result — do NOT construct it yourself, and do NOT run `npx skills add`, `git clone`, or any shell command. Cherry clones the repo, installs just that single skill, and registers it. Call this only after the user explicitly confirms the installation; Cherry enforces that confirmation independently of the agent permission mode.",
   inputSchema: {
     type: 'object',
     properties: {
@@ -107,19 +107,17 @@ class SkillsServer {
     const query = args.query
     if (!query) throw new McpError(ErrorCode.InvalidParams, "'query' is required for search_skills")
 
-    const url = new URL(`${MARKETPLACE_BASE_URL}/api/skills`)
-    url.searchParams.set('q', query.replace(/[-_]+/g, ' ').trim())
-    url.searchParams.set('limit', '20')
-    url.searchParams.set('offset', '0')
-
-    const response = await net.fetch(url.toString(), { method: 'GET' })
-    if (!response.ok) {
-      throw new Error(`Marketplace API returned ${response.status}: ${response.statusText}`)
-    }
-
-    // Shared normalizer: builds install_source from the real directoryPath and drops entries whose
-    // install target can't be resolved reliably (so we never hand back an ambiguous one).
-    const results = normalizeClaudePlugins(await response.json())
+    const results = await searchSkillMarketplaces(
+      query.replace(/[-_]+/g, ' ').trim(),
+      (url) => this.fetchMarketplaceJson(url),
+      (source, error) => {
+        logger.warn('Skill marketplace search source failed', {
+          agentId: this.agentId,
+          source,
+          error: error instanceof Error ? error.message : String(error)
+        })
+      }
+    )
 
     if (results.length === 0) {
       return { content: [{ type: 'text' as const, text: `No installable skills found for "${query}".` }] }
@@ -129,7 +127,10 @@ class SkillsServer {
       name: r.name,
       description: r.description,
       author: r.author,
+      stars: r.stars,
       installs: r.downloads,
+      source_registry: r.sourceRegistry,
+      source_url: r.sourceUrl,
       install_source: r.installSource
     }))
 
@@ -144,6 +145,20 @@ class SkillsServer {
     }
   }
 
+  private async fetchMarketplaceJson(url: string): Promise<unknown> {
+    const controller = new AbortController()
+    const timer = setTimeout(() => controller.abort(), REQUEST_TIMEOUT_MS)
+    try {
+      const response = await net.fetch(url, { method: 'GET', signal: controller.signal })
+      if (!response.ok) {
+        throw new Error(`Marketplace API returned ${response.status}: ${response.statusText}`)
+      }
+      return response.json()
+    } finally {
+      clearTimeout(timer)
+    }
+  }
+
   private async installSkill(args: Record<string, string | undefined>) {
     const installSource = args.install_source
     if (!installSource) {

src/main/ai/runtime/claudeCode/__tests__/settingsBuilder.test.ts

@@ -681,15 +681,15 @@ describe('buildClaudeCodeSessionSettings', () => {
   })
 
   it.each([
-    { permissionMode: 'default', headless: false, shouldDeny: false },
-    { permissionMode: 'acceptEdits', headless: false, shouldDeny: false },
-    { permissionMode: 'bypassPermissions', headless: false, shouldDeny: false },
-    { permissionMode: 'default', headless: true, shouldDeny: true },
-    { permissionMode: 'acceptEdits', headless: true, shouldDeny: true },
-    { permissionMode: 'bypassPermissions', headless: true, shouldDeny: false }
+    { permissionMode: 'default', headless: false, expectedDecision: 'ask' },
+    { permissionMode: 'acceptEdits', headless: false, expectedDecision: 'ask' },
+    { permissionMode: 'bypassPermissions', headless: false, expectedDecision: 'ask' },
+    { permissionMode: 'default', headless: true, expectedDecision: 'deny' },
+    { permissionMode: 'acceptEdits', headless: true, expectedDecision: 'deny' },
+    { permissionMode: 'bypassPermissions', headless: true, expectedDecision: 'deny' }
   ])(
-    'applies SDK permission semantics to skill install ($permissionMode, headless=$headless)',
-    async ({ permissionMode, headless, shouldDeny }) => {
+    'requires an explicit skill-install decision ($permissionMode, headless=$headless)',
+    async ({ permissionMode, headless, expectedDecision }) => {
       const isCurrentTurnHeadless = vi.fn(() => headless)
       mocks.applicationGet.mockImplementation((name: string) => {
         if (name === 'PreferenceService') return { get: vi.fn(() => undefined) }
@@ -724,16 +724,15 @@ describe('buildClaudeCodeSessionSettings', () => {
           )
         )
       )
-      const denial = expect.objectContaining({
-        hookSpecificOutput: expect.objectContaining({ permissionDecision: 'deny' })
+      const decision = expect.objectContaining({
+        hookSpecificOutput: expect.objectContaining({ permissionDecision: expectedDecision })
       })
 
-      if (shouldDeny) expect(results).toContainEqual(denial)
-      else expect(results).not.toContainEqual(denial)
+      expect(results).toContainEqual(decision)
     }
   )
 
-  it('uses the live permission mode when a warm session switches to bypassPermissions', async () => {
+  it('still denies headless installation when a warm session switches to bypassPermissions', async () => {
     let permissionMode = 'default'
     const isCurrentTurnHeadless = vi.fn(() => true)
     mocks.applicationGet.mockImplementation((name: string) => {
@@ -771,7 +770,7 @@ describe('buildClaudeCodeSessionSettings', () => {
       )
     )
 
-    expect(results).not.toContainEqual(
+    expect(results).toContainEqual(
       expect.objectContaining({ hookSpecificOutput: expect.objectContaining({ permissionDecision: 'deny' }) })
     )
   })
@@ -1294,9 +1293,8 @@ describe('buildClaudeCodeSessionSettings', () => {
       expect(settings.env!.CLAUDE_CODE_USE_VERTEX).toBe('0')
       // Non-mac (platform mock has no isMac): reuse the user's real config dir from the login shell.
       expect(settings.env!.CLAUDE_CONFIG_DIR).toBe('/home/me/.claude')
-      // The Cherry skill library path is injected unconditionally, so it survives external-CLI
-      // stripping — skill authoring keeps a stable target even when CLAUDE_CONFIG_DIR is redirected.
-      expect(settings.env!.CHERRY_STUDIO_SKILLS_DIR).toBe('/app/feature.agents.skills')
+      // The authoring inbox is injected unconditionally, so it survives external-CLI stripping.
+      expect(settings.env!.CHERRY_STUDIO_SKILLS_DIR).toBe('/app/feature.agents.skills.authoring')
     })
 
     it('falls back CLAUDE_CONFIG_DIR to ~/.claude when the shell does not set it', async () => {
@@ -1335,9 +1333,8 @@ describe('buildClaudeCodeSessionSettings', () => {
       )
 
       expect(settings.env).not.toHaveProperty('CLAUDE_CONFIG_DIR')
-      // CLAUDE_CONFIG_DIR is dropped on macOS login, but the Cherry skill library path stays
-      // injected, so skill authoring still resolves to a stable, Cherry-owned directory.
-      expect(settings.env!.CHERRY_STUDIO_SKILLS_DIR).toBe('/app/feature.agents.skills')
+      // CLAUDE_CONFIG_DIR is dropped on macOS login, but the Cherry authoring inbox stays injected.
+      expect(settings.env!.CHERRY_STUDIO_SKILLS_DIR).toBe('/app/feature.agents.skills.authoring')
     })
 
     it('blocks a reserved agent env_var override but passes through non-reserved keys', async () => {

src/main/ai/runtime/claudeCode/settingsBuilder.ts

@@ -582,7 +582,7 @@ async function buildEnvironment(provider: Provider, agent: AgentEntity): Promise
     ENABLE_TOOL_SEARCH: 'auto',
     CLAUDE_CODE_EXPERIMENTAL_AGENT_TEAMS: '1',
     CHERRY_STUDIO_BUN_PATH: bunPath,
-    CHERRY_STUDIO_SKILLS_DIR: application.getPath('feature.agents.skills'),
+    CHERRY_STUDIO_SKILLS_DIR: application.getPath('feature.agents.skills.authoring'),
     ...(customGitBashPath ? { CLAUDE_CODE_GIT_BASH_PATH: customGitBashPath } : {})
   }
 
@@ -878,23 +878,21 @@ async function buildToolPermissions(
     }
   }
 
-  // Installing a skill requires the same permission handling as any other mutating tool. Interactive
-  // turns defer to the SDK: default / acceptEdits prompt through canUseTool, while bypassPermissions
-  // runs directly. A headless turn has no responder, so deny only when its live permission mode still
-  // requires approval. Resolve the mode from the session snapshot so a warm connection observes a
-  // live permission-mode update instead of the agent config captured when these hooks were built.
+  // Third-party skills execute with the agent's full permissions, so installation always crosses an
+  // explicit user-approval boundary independent of the agent's general permission mode. Headless
+  // turns cannot answer that prompt and therefore fail closed.
   const headlessSkillInstallHook: HookCallback = async (input): Promise<HookJSONOutput> => {
     if (!input || input.hook_event_name !== 'PreToolUse') return {}
     const toolName = String((input as Record<string, unknown>).tool_name ?? '')
     if (toolName !== 'mcp__skills__install_skill') return {}
-    if (getToolPolicySnapshot(session.id)?.getPermissionMode() === 'bypassPermissions') return {}
-    if (!application.get('AgentSessionRuntimeService').isCurrentTurnHeadless(session.id)) return {}
+    const headless = application.get('AgentSessionRuntimeService').isCurrentTurnHeadless(session.id)
     return {
       hookSpecificOutput: {
         hookEventName: 'PreToolUse',
-        permissionDecision: 'deny',
-        permissionDecisionReason:
-          'This channel or scheduled turn cannot approve a skill installation. Use bypassPermissions for unattended installation, or install it from an interactive turn.'
+        permissionDecision: headless ? 'deny' : 'ask',
+        permissionDecisionReason: headless
+          ? 'Channel and scheduled turns cannot approve third-party skill installation. Install it from an interactive turn.'
+          : 'Installing this third-party skill will let it run with the agent permissions. Confirm the installation.'
       }
     }
   }