feat: auto-discover skills/commands from Claude Code, Codex, and Gemini configs#405
feat: auto-discover skills/commands from Claude Code, Codex, and Gemini configs#405
Conversation
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review.
Tip: disable this comment in your organization's Code Review settings.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (10)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (6)
📝 WalkthroughWalkthroughAdds runtime external AI-tool skill discovery (Claude Code, Codex, Gemini) gated by a new config flag, integrates discovered skills into skill loading when enabled, exposes a discovery tool and template, updates command hint extraction to use templates/skill content, and adds tests and a sanity check. Changes
Sequence DiagramsequenceDiagram
participant Loader as "Skill Loader"
participant Config as "Config"
participant Discover as "discover-external"
participant FS as "File System"
participant Skills as "Runtime Skills Map"
participant Tool as "SkillDiscoverTool"
Loader->>Config: read experimental.auto_skill_discovery
Config-->>Loader: true/false
alt auto discovery enabled
Loader->>Discover: discoverExternalSkills(worktree, home?)
Discover->>FS: glob & read files (md, toml) in project/home
FS-->>Discover: file contents
Discover->>Discover: parse frontmatter/TOML, normalize placeholders
Discover->>Discover: validate names, deduplicate results
Discover-->>Loader: { skills, sources }
Loader->>Skills: merge new skills (skip existing)
Tool->>Discover: discoverExternalSkills(worktree)
Tool-->>Tool: format list/add response (compare with Skills)
else disabled
Loader->>Skills: load builtin skills only
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/opencode/src/config/config.ts (1)
1546-1547:⚠️ Potential issue | 🟡 MinorRemove orphaned statements at end of file.
These two standalone
Filesystem.writeexpressions appear to be leftover artifacts from a refactoring or merge. They have no effect and should be removed.🐛 Proposed fix
-Filesystem.write -Filesystem.write🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opencode/src/config/config.ts` around lines 1546 - 1547, Remove the two orphaned standalone expressions "Filesystem.write" found at the end of the file; these are no-op remnants and should be deleted so only real function definitions/exports remain (search for the symbol "Filesystem.write" near the file tail and remove those two lines).
🧹 Nitpick comments (1)
packages/opencode/test/skill/discover-external.test.ts (1)
8-16: Consider using thetmpdirfixture for consistency.The test uses manual
mkdtemp/rmwithbeforeEach/afterEachinstead of the project'stmpdirfixture. While functional, usingtmpdirfromfixture/fixture.tswithawait usingsyntax would align with project conventions and provide automatic cleanup. As per coding guidelines, use thetmpdirfunction fromfixture/fixture.tsto create temporary directories for tests with automatic cleanup.♻️ Example pattern using tmpdir
import { tmpdir } from "../fixture/fixture" describe("discoverExternalSkills", () => { test("discovers Claude Code command with frontmatter", async () => { await using tmp = await tmpdir({ init: async (dir) => { await mkdir(path.join(dir, ".claude", "commands"), { recursive: true }) await writeFile( path.join(dir, ".claude", "commands", "review.md"), `---\nname: review\ndescription: Review the code changes\n---\n\nPlease review the following code changes: $ARGUMENTS\n`, ) }, }) const { skills } = await Instance.provide({ directory: tmp.path, fn: () => discoverExternalSkills(tmp.path), }) // assertions... }) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opencode/test/skill/discover-external.test.ts` around lines 8 - 16, Replace the manual mkdtemp/rm setup in the test with the project's tmpdir fixture: import tmpdir from fixture/fixture.ts, remove beforeEach/afterEach and tempDir variable, and use "await using tmp = await tmpdir({ init: async (dir) => { ... } })" inside the test to create the directory and seed files; then call discoverExternalSkills(tmp.path) (or pass tmp.path into Instance.provide) so the fixture handles cleanup automatically.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/opencode/src/config/config.ts`:
- Around line 1546-1547: Remove the two orphaned standalone expressions
"Filesystem.write" found at the end of the file; these are no-op remnants and
should be deleted so only real function definitions/exports remain (search for
the symbol "Filesystem.write" near the file tail and remove those two lines).
---
Nitpick comments:
In `@packages/opencode/test/skill/discover-external.test.ts`:
- Around line 8-16: Replace the manual mkdtemp/rm setup in the test with the
project's tmpdir fixture: import tmpdir from fixture/fixture.ts, remove
beforeEach/afterEach and tempDir variable, and use "await using tmp = await
tmpdir({ init: async (dir) => { ... } })" inside the test to create the
directory and seed files; then call discoverExternalSkills(tmp.path) (or pass
tmp.path into Instance.provide) so the fixture handles cleanup automatically.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 1cc2ba19-9156-47a9-8323-9d6249fff3dd
📒 Files selected for processing (6)
packages/opencode/src/command/index.tspackages/opencode/src/config/config.tspackages/opencode/src/skill/discover-external.tspackages/opencode/src/skill/skill.tspackages/opencode/test/skill/discover-external.test.tspackages/opencode/test/tool/skill.test.ts
️✅ There are no secrets present in this pull request anymore.If these secrets were true positive and are still valid, we highly recommend you to revoke them. 🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request. |
0851d05 to
9024913
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/opencode/src/config/config.ts (1)
1549-1550:⚠️ Potential issue | 🟡 MinorRemove extraneous statements at end of file.
These two
Filesystem.writestatements appear to be leftover debug artifacts or accidental insertions. They have no arguments and would cause syntax errors if the file were executed standalone.🧹 Proposed fix
-Filesystem.write -Filesystem.write🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opencode/src/config/config.ts` around lines 1549 - 1550, Remove the two stray, argument-less calls to Filesystem.write at the end of the file (the extraneous "Filesystem.write" statements), which are invalid and likely accidental; delete those lines and scan the tail of the module (e.g., near the end of config.ts) for any other leftover debug/accidental statements and remove them to ensure the file ends with valid exports or declarations only.
🧹 Nitpick comments (3)
packages/opencode/src/skill/discover-external.ts (2)
215-227: Root check is Unix-specific.The
worktree !== "/"guard prevents infinite traversal when worktree is the filesystem root, but this check is Unix-specific. On Windows, the root might beC:\or similar. Consider usingpath.parse(worktree).root === worktreefor cross-platform compatibility.♻️ Cross-platform root check
- if ((source.scope === "project" || source.scope === "both") && worktree !== "/") { + // Cross-platform check: worktree is not a filesystem root + const isRoot = path.parse(worktree).root === worktree + if ((source.scope === "project" || source.scope === "both") && !isRoot) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opencode/src/skill/discover-external.ts` around lines 215 - 227, The guard preventing traversal to filesystem root is Unix-specific (worktree !== "/"); change it to a cross-platform root check using path.parse(worktree).root so the loop skips when worktree is the filesystem root. Update the condition around the Filesystem.up loop (where SOURCES, worktree, Instance.directory, and Filesystem.up are used) to compare path.parse(worktree).root === worktree (or the inverse) instead of string-equality to "/" so Windows roots like "C:\\" are handled correctly.
137-156: Redundant but defensive symlink checks.The
symlink: falseoption inGlob.scanshould already exclude symlinks, but the explicitlstatcheck (lines 148-156) provides defense-in-depth. This is acceptable for security-sensitive code, though you could simplify by removing one check if performance becomes a concern.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opencode/src/skill/discover-external.ts` around lines 137 - 156, The code currently uses Glob.scan(..., symlink: false) and then redundantly calls fs.lstat + stat.isSymbolicLink() for each match; remove the per-file symlink lstat block (the try/catch that calls fs.lstat and checks stat.isSymbolicLink) to avoid duplicate checks and extra I/O, rely on Glob.scan's symlink: false option instead, and update the surrounding comment to note that Glob.scan excludes symlinks; references: Glob.scan, fs.lstat, stat.isSymbolicLink, matches.packages/opencode/src/skill/skill.ts (1)
241-243: Security consideration: discovered skill directories expand permission whitelist.Per
packages/opencode/src/permission/permission.ts:57-62,Skill.dirs()is used to build file-access permission whitelists. Adding directories from external AI tool configs (which could be in arbitrary locations like~/.claude/or project-upward paths) automatically expands the accessible file scope.This is likely intentional to allow skills to reference their local files, but ensure users understand that enabling
auto_skill_discoverymay grant broader file access permissions than expected.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opencode/src/skill/skill.ts` around lines 241 - 243, The current code adds external skill directories into the global whitelist by doing skills[skill.name] = skill; dirs.add(path.dirname(skill.location)); which broadens file-access scope; change this so auto-added directories are either constrained or opt-in: (1) resolve and sanitize skill.location and only add its dirname to dirs if it resides under a permitted root (e.g., project root or configured allowedRoots) using path.resolve and path.relative checks, (2) or gate adding to dirs behind a config flag like auto_skill_discovery_allow_external and log a clear warning when an external path is accepted, and (3) update Skill.dirs() usage in permission building to skip or mark external dirs so they aren’t treated as fully trusted; reference skills, dirs, skill.location, Skill.dirs(), and auto_skill_discovery in your changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/opencode/src/skill/discover-external.ts`:
- Around line 245-258: The one-time read-and-clear pattern (_lastDiscovery and
consumeSkillDiscoveryResult) is unused in production; remove the dead code by
deleting the _lastDiscovery variable and the consumeSkillDiscoveryResult export,
and keep setSkillDiscoveryResult as the single API that records discoveries (or
convert it to a no-op logger if you prefer); update any tests that import
consumeSkillDiscoveryResult to use setSkillDiscoveryResult or a new test helper
instead. Ensure references to _lastDiscovery and consumeSkillDiscoveryResult are
removed so there are no unused exports.
In `@packages/opencode/src/skill/skill.ts`:
- Around line 233-251: The code calls discoverExternalSkills(...) and then
setSkillDiscoveryResult(added, sources) but never invokes
consumeSkillDiscoveryResult(...) in production, so discovery metadata is never
surfaced; update the production path in the experimental.auto_skill_discovery
block to invoke the consumer after setting results (e.g., call
consumeSkillDiscoveryResult(added, sources) or a UI/notification helper) or
remove the set/consume API and directly emit the discovery results where needed;
look for the functions discoverExternalSkills, setSkillDiscoveryResult and
consumeSkillDiscoveryResult to change the flow so discovered skills are
logged/notified to users (or the setter/getter pattern is eliminated).
---
Outside diff comments:
In `@packages/opencode/src/config/config.ts`:
- Around line 1549-1550: Remove the two stray, argument-less calls to
Filesystem.write at the end of the file (the extraneous "Filesystem.write"
statements), which are invalid and likely accidental; delete those lines and
scan the tail of the module (e.g., near the end of config.ts) for any other
leftover debug/accidental statements and remove them to ensure the file ends
with valid exports or declarations only.
---
Nitpick comments:
In `@packages/opencode/src/skill/discover-external.ts`:
- Around line 215-227: The guard preventing traversal to filesystem root is
Unix-specific (worktree !== "/"); change it to a cross-platform root check using
path.parse(worktree).root so the loop skips when worktree is the filesystem
root. Update the condition around the Filesystem.up loop (where SOURCES,
worktree, Instance.directory, and Filesystem.up are used) to compare
path.parse(worktree).root === worktree (or the inverse) instead of
string-equality to "/" so Windows roots like "C:\\" are handled correctly.
- Around line 137-156: The code currently uses Glob.scan(..., symlink: false)
and then redundantly calls fs.lstat + stat.isSymbolicLink() for each match;
remove the per-file symlink lstat block (the try/catch that calls fs.lstat and
checks stat.isSymbolicLink) to avoid duplicate checks and extra I/O, rely on
Glob.scan's symlink: false option instead, and update the surrounding comment to
note that Glob.scan excludes symlinks; references: Glob.scan, fs.lstat,
stat.isSymbolicLink, matches.
In `@packages/opencode/src/skill/skill.ts`:
- Around line 241-243: The current code adds external skill directories into the
global whitelist by doing skills[skill.name] = skill;
dirs.add(path.dirname(skill.location)); which broadens file-access scope; change
this so auto-added directories are either constrained or opt-in: (1) resolve and
sanitize skill.location and only add its dirname to dirs if it resides under a
permitted root (e.g., project root or configured allowedRoots) using
path.resolve and path.relative checks, (2) or gate adding to dirs behind a
config flag like auto_skill_discovery_allow_external and log a clear warning
when an external path is accepted, and (3) update Skill.dirs() usage in
permission building to skip or mark external dirs so they aren’t treated as
fully trusted; reference skills, dirs, skill.location, Skill.dirs(), and
auto_skill_discovery in your changes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 3f667acd-86f0-49b8-a3ad-bffd093e2f83
📒 Files selected for processing (4)
packages/opencode/src/command/index.tspackages/opencode/src/config/config.tspackages/opencode/src/skill/discover-external.tspackages/opencode/src/skill/skill.ts
✅ Files skipped from review due to trivial changes (1)
- packages/opencode/src/command/index.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/sanity/phases/resilience.sh`:
- Around line 214-223: The test currently only prints static "config-schema: ok"
and "module-load: ok" without actually importing the discover-external module;
update the Node one-liner that sets DISCOVER_OUTPUT so it loads the TypeScript
module and asserts its exports: run Node with a TypeScript loader (e.g., require
ts-node/register via -r or NODE_OPTIONS) and require the actual module path
(e.g., './src/skill/discover-external' or the package export that points to it),
then check for expected exports/shape and print "module-load: ok" only on
success and "module-load: error <msg>" on failure; ensure the test targets the
discover-external symbol (the module name) rather than relying on skill/index.ts
re-exports.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 3e8800b8-8c2f-4f47-b665-b06358dbac76
📒 Files selected for processing (1)
test/sanity/phases/resilience.sh
a89aec9 to
26647c9
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/opencode/test/skill/discover-external.test.ts (1)
12-23: Consider using thetmpdirfixture for consistency with project conventions.The coding guidelines recommend using
tmpdirfromfixture/fixture.tswithawait usingsyntax for automatic cleanup. The current implementation uses manualmkdtemp/rmwhich works but is more verbose.This is a minor consistency suggestion — the current approach is functionally correct.
♻️ Example refactor using fixture/tmpdir pattern
-import { mkdtemp, rm, mkdir, writeFile, symlink } from "fs/promises" -import { tmpdir } from "os" +import { mkdir, writeFile, symlink, rm } from "fs/promises" +import { tmpdir } from "../fixture/fixture" import path from "path" ... -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 }) -}) +// In each test: +// await using tmp = await tmpdir() +// await using homeDir = await tmpdir()As per coding guidelines: "Use the
tmpdirfunction fromfixture/fixture.tsto create temporary directories for tests with automatic cleanup" and "Always useawait usingsyntax withtmpdir()for automatic cleanup".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opencode/test/skill/discover-external.test.ts` around lines 12 - 23, Replace the manual mkdtemp/rm pattern in the test (the tempDir/homeDir variables and beforeEach/afterEach hooks) with the project tmpdir fixture usage: import tmpdir from fixture/fixture.ts and create temp dirs with "await using const tempDir = await tmpdir(...)" and "await using const homeDir = await tmpdir(...)" inside the test or a setup block so automatic cleanup is handled; remove the beforeEach/afterEach that call mkdtemp and rm and any top-level tempDir/homeDir variables to follow the fixture/tmpdir convention.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/opencode/src/altimate/tools/skill-discover.ts`:
- Around line 80-106: The "add" path builds toAdd but then only calls
Skill.invalidate(), which doesn't repopulate discovered skills unless
config.experimental?.auto_skill_discovery is true during stateInit; fix by
persisting or registering the selected discoveries directly instead of relying
on a reload—e.g., before calling Skill.invalidate(), take toAdd and add them
into the runtime skill state (the same place stateInit would load into) or save
their names into the persistent config so stateInit will pick them up, or
temporarily set config.experimental.auto_skill_discovery = true for the reload
cycle; update the code around toAdd and Skill.invalidate() to perform one of
these (explicitly add discovered skills to the skill state or enable the flag
during reload) so the "add" action actually makes the skills available.
---
Nitpick comments:
In `@packages/opencode/test/skill/discover-external.test.ts`:
- Around line 12-23: Replace the manual mkdtemp/rm pattern in the test (the
tempDir/homeDir variables and beforeEach/afterEach hooks) with the project
tmpdir fixture usage: import tmpdir from fixture/fixture.ts and create temp dirs
with "await using const tempDir = await tmpdir(...)" and "await using const
homeDir = await tmpdir(...)" inside the test or a setup block so automatic
cleanup is handled; remove the beforeEach/afterEach that call mkdtemp and rm and
any top-level tempDir/homeDir variables to follow the fixture/tmpdir convention.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: b4ebd73e-5b22-498f-a42d-d9337c1aaebf
📒 Files selected for processing (10)
packages/opencode/src/altimate/tools/skill-discover.tspackages/opencode/src/command/index.tspackages/opencode/src/command/template/discover-skills.txtpackages/opencode/src/config/config.tspackages/opencode/src/skill/discover-external.tspackages/opencode/src/skill/skill.tspackages/opencode/src/tool/registry.tspackages/opencode/test/skill/discover-external.test.tspackages/opencode/test/tool/skill.test.tstest/sanity/phases/resilience.sh
✅ Files skipped from review due to trivial changes (2)
- packages/opencode/src/command/template/discover-skills.txt
- packages/opencode/src/config/config.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/opencode/test/tool/skill.test.ts
- test/sanity/phases/resilience.sh
- packages/opencode/src/skill/skill.ts
26647c9 to
d6c62db
Compare
…ni configs
Adds external skill discovery with both auto and manual modes:
Auto mode (opt-in):
Set experimental.auto_skill_discovery: true in config to auto-load
external skills at startup.
Manual mode (always available):
Run /discover-skills to scan, list, and selectively add skills on
demand — works regardless of config setting. Uses the skill_discover
tool which directly registers skills into runtime state (no config
flag dependency).
Sources scanned:
- .claude/commands/**/*.md (Claude Code commands)
- .codex/skills/**/SKILL.md (Codex CLI skills)
- .gemini/skills/**/SKILL.md (Gemini CLI skills)
- .gemini/commands/**/*.toml (Gemini commands, {{args}} → $ARGUMENTS)
Security:
- Rejects symlinks (lstat check)
- Rejects __proto__/constructor/prototype names
- Rejects path traversal (.. in names)
- Existing skills never overwritten
Tests: 21 unit tests + 1 sanity resilience test
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
d6c62db to
da29d4a
Compare
Summary
.claude/commands/*.md, Codex.codex/skills/**/SKILL.md, Gemini.gemini/skills/**/SKILL.mdand.gemini/commands/*.toml)experimental.auto_skill_discoveryconfig toggle (default:true)$ARGUMENTShints viahints(skill.content)Test plan
bun turbo typecheckpassestest/skill/discover-external.test.tspasstest/skill/skill.test.ts(13 tests) — no regressionstest/tool/skill.test.ts(5 tests) — no regressions~/.claude/commands/test-cmd.md→ verify/test-cmdappears~/.gemini/commands/deploy.toml→ verify/deploydiscoveredexperimental.auto_skill_discovery: false→ verify no discovery🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests