fix: address code review findings for external skill discovery

Security fixes:
- Reject symlinked skill files (lstat check) — prevents arbitrary file read
- Add prototype pollution guards (__proto__, constructor, prototype)
- Reject skill names containing .. segments (path traversal)
- Add duplicate name warnings in external discovery path

Bug fixes:
- Gemini TOML nested commands now preserve directory path (team/review
  instead of just review) — parity with markdown command handling
- TOML parse errors logged at warn level instead of debug
- setSkillDiscoveryResult clears state on empty input (prevents stale data)

Test improvements:
- Isolated home directory (hermetic tests, no real ~ leakage)
- Added adversarial tests: prototype pollution, path traversal, symlinks
- Added discovery result lifecycle tests (set/consume/clear)
- Added nested TOML path preservation test
- Added home directory isolation test

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Author: kulvirgit

packages/opencode/src/skill/discover-external.ts

@@ -1,5 +1,6 @@
 // altimate_change start — auto-discover skills/commands from external AI tool configs
 import path from "path"
+import fs from "fs/promises"
 import { pathToFileURL } from "url"
 import { Log } from "../util/log"
 import { Filesystem } from "../util/filesystem"
@@ -19,13 +20,18 @@ interface ExternalSkillSource {
   format: "skill-md" | "command-md" | "command-toml"
 }
 
+// Discovery priority: Claude Code → Codex → Gemini (skills) → Gemini (commands).
+// Within each source: project-deep → project-shallow → home. First skill with a given name wins.
 const SOURCES: ExternalSkillSource[] = [
   { tool: "claude-code", dir: ".claude", pattern: "commands/**/*.md", scope: "both", format: "command-md" },
   { tool: "codex", dir: ".codex", pattern: "skills/**/SKILL.md", scope: "both", format: "skill-md" },
   { tool: "gemini", dir: ".gemini", pattern: "skills/**/SKILL.md", scope: "both", format: "skill-md" },
   { tool: "gemini", dir: ".gemini", pattern: "commands/**/*.toml", scope: "both", format: "command-toml" },
 ]
 
+// Names that would pollute Object.prototype — must never be used as skill keys
+const POISONED_NAMES = new Set(["__proto__", "constructor", "prototype"])
+
 /**
  * Parse a standard SKILL.md file (Codex, Gemini) using ConfigMarkdown.parse().
  * Returns a Skill.Info or undefined if the file is malformed.
@@ -87,17 +93,20 @@ async function transformCommandMd(filePath: string, commandsRoot: string): Promi
  * Expects `prompt` field for content, optional `description`.
  * Converts `{{args}}` / `{{ args }}` → `$ARGUMENTS`.
  */
-async function transformCommandToml(filePath: string): Promise<Skill.Info | undefined> {
+async function transformCommandToml(filePath: string, commandsRoot: string): Promise<Skill.Info | undefined> {
   try {
+    // Bun-specific: native TOML import support via import attributes (not available in Node.js)
     const mod = await import(pathToFileURL(filePath).href, { with: { type: "toml" } })
     const data = (mod.default || mod) as Record<string, unknown>
 
     if (typeof data.prompt !== "string" || !data.prompt.trim()) {
-      log.debug("TOML command missing prompt field", { path: filePath })
+      log.warn("TOML command missing prompt field", { path: filePath })
       return undefined
     }
 
-    const name = path.basename(filePath, ".toml")
+    // Derive name from relative path (preserving nested directories), matching transformCommandMd
+    const rel = path.relative(commandsRoot, filePath)
+    const name = rel.replace(/\.toml$/i, "").replace(/\\/g, "/")
     const description = typeof data.description === "string" ? data.description : ""
     // Convert Gemini's {{args}} / {{ args }} placeholder to $ARGUMENTS
     const content = data.prompt.replace(/\{\{\s*args\s*\}\}/g, "$ARGUMENTS")
@@ -109,7 +118,7 @@ async function transformCommandToml(filePath: string): Promise<Skill.Info | unde
       content,
     }
   } catch (err) {
-    log.debug("failed to parse TOML command", { path: filePath, err })
+    log.warn("failed to parse TOML command", { path: filePath, err })
     return undefined
   }
 }
@@ -129,11 +138,21 @@ async function scanSource(
     absolute: true,
     include: "file",
     dot: true,
-    symlink: true,
+    symlink: false, // Security: don't follow symlinks — prevents reading arbitrary files via crafted repos
   }).catch(() => [] as string[])
 
   const results: Skill.Info[] = []
   for (const match of matches) {
+    // Security: reject symlinks — prevents reading arbitrary files via crafted repos
+    try {
+      const stat = await fs.lstat(match)
+      if (stat.isSymbolicLink()) {
+        log.warn("skipping symlinked skill file", { path: match })
+        continue
+      }
+    } catch {
+      continue
+    }
     let skill: Skill.Info | undefined
     switch (source.format) {
       case "skill-md":
@@ -143,7 +162,7 @@ async function scanSource(
         skill = await transformCommandMd(match, path.join(baseDir, "commands"))
         break
       case "command-toml":
-        skill = await transformCommandToml(match)
+        skill = await transformCommandToml(match, path.join(baseDir, "commands"))
         break
     }
     if (skill) results.push(skill)
@@ -158,20 +177,33 @@ async function scanSource(
  * Searches both home directory and project directory (walking up from CWD to worktree root).
  * Returns discovered skills and contributing source labels.
  */
-export async function discoverExternalSkills(worktree: string): Promise<{
+export async function discoverExternalSkills(worktree: string, homeDir?: string): Promise<{
   skills: Skill.Info[]
   sources: string[]
 }> {
   log.info("Discovering skills/commands from external AI tool configs...")
   const allSkills: Skill.Info[] = []
   const sources: string[] = []
   const seen = new Set<string>()
-  const homedir = Global.Path.home
+  const homedir = homeDir ?? Global.Path.home
 
   const addSkills = (skills: Skill.Info[], sourceLabel: string) => {
     let added = 0
     for (const skill of skills) {
-      if (seen.has(skill.name)) continue
+      // Guard against prototype pollution
+      if (POISONED_NAMES.has(skill.name)) {
+        log.warn("rejecting skill with reserved name", { name: skill.name, source: sourceLabel })
+        continue
+      }
+      // Reject path traversal in derived names
+      if (skill.name.includes("..")) {
+        log.warn("rejecting skill with path traversal in name", { name: skill.name, source: sourceLabel })
+        continue
+      }
+      if (seen.has(skill.name)) {
+        log.warn("duplicate external skill name, skipping", { name: skill.name, source: sourceLabel, existing: allSkills.find((s) => s.name === skill.name)?.location })
+        continue
+      }
       seen.add(skill.name)
       allSkills.push(skill)
       added++
@@ -214,9 +246,7 @@ let _lastDiscovery: { skillNames: string[]; sources: string[] } | null = null
 
 /** Called from skill.ts after merge with only the names that were actually added. */
 export function setSkillDiscoveryResult(skillNames: string[], sources: string[]) {
-  if (skillNames.length > 0) {
-    _lastDiscovery = { skillNames, sources }
-  }
+  _lastDiscovery = skillNames.length > 0 ? { skillNames, sources } : null
 }
 
 /** Returns and clears the last discovery result (for one-time notification). */

packages/opencode/test/skill/discover-external.test.ts

@@ -1,26 +1,34 @@
 import { describe, test, expect, beforeEach, afterEach } from "bun:test"
-import { mkdtemp, rm, mkdir, writeFile } from "fs/promises"
+import { mkdtemp, rm, mkdir, writeFile, symlink } from "fs/promises"
 import { tmpdir } from "os"
 import path from "path"
-import { discoverExternalSkills } from "../../src/skill/discover-external"
+import {
+  discoverExternalSkills,
+  setSkillDiscoveryResult,
+  consumeSkillDiscoveryResult,
+} from "../../src/skill/discover-external"
 import { Instance } from "../../src/project/instance"
 
 let tempDir: string
+let homeDir: string
 
 beforeEach(async () => {
   tempDir = await mkdtemp(path.join(tmpdir(), "skill-discover-"))
+  homeDir = await mkdtemp(path.join(tmpdir(), "skill-discover-home-"))
 })
 
 afterEach(async () => {
   await rm(tempDir, { recursive: true, force: true })
+  await rm(homeDir, { recursive: true, force: true })
 })
 
 describe("discoverExternalSkills", () => {
-  // Helper to run discovery with tempDir as both worktree and Instance.directory
+  // Helper to run discovery with tempDir as both worktree and Instance.directory,
+  // and an isolated homeDir to prevent real home directory from leaking into results
   async function discover(worktree?: string) {
     return Instance.provide({
       directory: worktree ?? tempDir,
-      fn: () => discoverExternalSkills(worktree ?? tempDir),
+      fn: () => discoverExternalSkills(worktree ?? tempDir, homeDir),
     })
   }
 
@@ -146,6 +154,21 @@ prompt = "Deploy the app to {{ args }} environment"
     expect(skill!.content).not.toContain("{{")
   })
 
+  test("preserves nested TOML command path as name", async () => {
+    await mkdir(path.join(tempDir, ".gemini", "commands", "team"), { recursive: true })
+    await writeFile(
+      path.join(tempDir, ".gemini", "commands", "team", "deploy.toml"),
+      `description = "Team deploy"
+prompt = "Deploy for team"
+`,
+    )
+
+    const { skills } = await discover()
+    const skill = skills.find((s) => s.name === "team/deploy")
+    expect(skill).toBeDefined()
+    expect(skill!.description).toBe("Team deploy")
+  })
+
   test("skips Gemini TOML command without prompt field", async () => {
     await mkdir(path.join(tempDir, ".gemini", "commands"), { recursive: true })
     await writeFile(
@@ -295,10 +318,9 @@ prompt = "Run {{args}}"
   // --- Worktree edge cases ---
 
   test("skips project scan when worktree is /", async () => {
-    // Should not throw and should return empty (no dirs at /)
     const result = await Instance.provide({
       directory: tempDir,
-      fn: () => discoverExternalSkills("/"),
+      fn: () => discoverExternalSkills("/", homeDir),
     })
     expect(result.skills).toEqual([])
   })
@@ -324,4 +346,149 @@ Content here
     expect(skill).toBeDefined()
     expect(skill!.location).toBe(cmdPath)
   })
+
+  // --- Security: Prototype pollution ---
+
+  test("rejects skills named __proto__", async () => {
+    await mkdir(path.join(tempDir, ".claude", "commands"), { recursive: true })
+    await writeFile(
+      path.join(tempDir, ".claude", "commands", "__proto__.md"),
+      `---
+name: __proto__
+description: Malicious skill
+---
+
+Exploit content
+`,
+    )
+
+    const { skills } = await discover()
+    expect(skills.find((s) => s.name === "__proto__")).toBeUndefined()
+  })
+
+  test("rejects skills named constructor", async () => {
+    await mkdir(path.join(tempDir, ".claude", "commands"), { recursive: true })
+    await writeFile(
+      path.join(tempDir, ".claude", "commands", "constructor.md"),
+      `---
+name: constructor
+description: Malicious skill
+---
+
+Exploit
+`,
+    )
+
+    const { skills } = await discover()
+    expect(skills.find((s) => s.name === "constructor")).toBeUndefined()
+  })
+
+  test("rejects skills named prototype", async () => {
+    await mkdir(path.join(tempDir, ".codex", "skills", "proto"), { recursive: true })
+    await writeFile(
+      path.join(tempDir, ".codex", "skills", "proto", "SKILL.md"),
+      `---
+name: prototype
+description: Malicious
+---
+
+Content
+`,
+    )
+
+    const { skills } = await discover()
+    expect(skills.find((s) => s.name === "prototype")).toBeUndefined()
+  })
+
+  // --- Security: Path traversal ---
+
+  test("rejects skill names containing .. segments", async () => {
+    await mkdir(path.join(tempDir, ".claude", "commands"), { recursive: true })
+    await writeFile(
+      path.join(tempDir, ".claude", "commands", "legit.md"),
+      `---
+name: ../../etc/passwd
+description: Path traversal attempt
+---
+
+Content
+`,
+    )
+
+    const { skills } = await discover()
+    expect(skills.find((s) => s.name === "../../etc/passwd")).toBeUndefined()
+  })
+
+  // --- Security: Symlinks not followed ---
+
+  test("does not follow symlinks to files outside the directory", async () => {
+    // Create a sensitive file outside the project
+    const sensitiveDir = await mkdtemp(path.join(tmpdir(), "sensitive-"))
+    await writeFile(path.join(sensitiveDir, "secret.txt"), "TOP SECRET DATA")
+
+    // Create a symlink inside .claude/commands/ pointing to the sensitive file
+    await mkdir(path.join(tempDir, ".claude", "commands"), { recursive: true })
+    try {
+      await symlink(
+        path.join(sensitiveDir, "secret.txt"),
+        path.join(tempDir, ".claude", "commands", "steal.md"),
+      )
+    } catch {
+      // symlink may fail on some platforms — skip test
+      await rm(sensitiveDir, { recursive: true, force: true })
+      return
+    }
+
+    const { skills } = await discover()
+    // The symlinked file should NOT be discovered (symlink: false)
+    expect(skills.find((s) => s.name === "steal")).toBeUndefined()
+
+    await rm(sensitiveDir, { recursive: true, force: true })
+  })
+
+  // --- Discovery result lifecycle ---
+
+  test("consumeSkillDiscoveryResult returns null when nothing set", () => {
+    // Clear any prior state
+    setSkillDiscoveryResult([], [])
+    const result = consumeSkillDiscoveryResult()
+    expect(result).toBeNull()
+  })
+
+  test("consumeSkillDiscoveryResult returns result once then null", () => {
+    setSkillDiscoveryResult(["my-skill"], [".claude/commands"])
+    const first = consumeSkillDiscoveryResult()
+    expect(first).toEqual({ skillNames: ["my-skill"], sources: [".claude/commands"] })
+    const second = consumeSkillDiscoveryResult()
+    expect(second).toBeNull()
+  })
+
+  test("setSkillDiscoveryResult with empty array clears previous result", () => {
+    setSkillDiscoveryResult(["old-skill"], [".claude/commands"])
+    setSkillDiscoveryResult([], [])
+    const result = consumeSkillDiscoveryResult()
+    expect(result).toBeNull()
+  })
+
+  // --- Home directory isolation ---
+
+  test("discovers skills from home directory separately", async () => {
+    // Put a skill in the isolated home directory
+    await mkdir(path.join(homeDir, ".claude", "commands"), { recursive: true })
+    await writeFile(
+      path.join(homeDir, ".claude", "commands", "home-cmd.md"),
+      `---
+name: home-cmd
+description: Home command
+---
+
+Home content
+`,
+    )
+
+    const { skills } = await discover()
+    const skill = skills.find((s) => s.name === "home-cmd")
+    expect(skill).toBeDefined()
+    expect(skill!.location).toContain(homeDir)
+  })
 })