-
Notifications
You must be signed in to change notification settings - Fork 2.3k
fix(skill-loader): discover skills from .agents/skills/ directory #1710
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: dev
Are you sure you want to change the base?
Conversation
Plugin's skill tool was overriding OpenCode's native SkillTool, but only loaded skills from .claude/skills/ and .opencode/skills/. Skills placed in .agents/skills/ (supported by OpenCode natively) were silently dropped. Add discoverProjectAgentsSkills() and discoverGlobalAgentsSkills() to match OpenCode's EXTERNAL_DIRS = [".claude", ".agents"] behavior.
|
Thank you for your contribution! Before we can merge this PR, we need you to sign our Contributor License Agreement (CLA). To sign the CLA, please comment on this PR with: This is a one-time requirement. Once signed, all your future contributions will be automatically accepted. I have read the CLA Document and I hereby sign the CLA You can retrigger this bot by commenting recheck in this Pull Request. Posted by the CLA Assistant Lite bot. |
…code.skills flag .agents/skills/ is an OpenCode-native path, not a Claude Code compat feature. It should always be loaded like .opencode/skills/, not gated behind claude_code.skills config.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1 issue found across 3 files
Confidence score: 4/5
- This PR looks safe to merge overall, with only a low-severity test isolation concern.
mock.restore()insrc/features/opencode-skill-loader/loader.test.tsdoesn’t revertmock.module()in Bun, so a mockedos.homedir()can leak to later tests and cause intermittent failures.- Pay close attention to
src/features/opencode-skill-loader/loader.test.ts- ensure mocked module state doesn’t persist across tests.
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="src/features/opencode-skill-loader/loader.test.ts">
<violation number="1" location="src/features/opencode-skill-loader/loader.test.ts:629">
P2: `mock.restore()` does not undo `mock.module()` overrides in Bun, so the mocked `os` module remains in the module cache after this test. This can leak the fake `homedir()` into subsequent tests and cause flakiness. Restore the module mock explicitly (e.g., re-mock `os` with the original exports) instead of relying on `mock.restore()`.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| expect(skill?.scope).toBe("user") | ||
| expect(skill?.definition.description).toContain("A skill from global .agents/skills directory") | ||
| } finally { | ||
| mock.restore() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P2: mock.restore() does not undo mock.module() overrides in Bun, so the mocked os module remains in the module cache after this test. This can leak the fake homedir() into subsequent tests and cause flakiness. Restore the module mock explicitly (e.g., re-mock os with the original exports) instead of relying on mock.restore().
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/features/opencode-skill-loader/loader.test.ts, line 629:
<comment>`mock.restore()` does not undo `mock.module()` overrides in Bun, so the mocked `os` module remains in the module cache after this test. This can leak the fake `homedir()` into subsequent tests and cause flakiness. Restore the module mock explicitly (e.g., re-mock `os` with the original exports) instead of relying on `mock.restore()`.</comment>
<file context>
@@ -558,6 +559,120 @@ Skill body.
+ expect(skill?.scope).toBe("user")
+ expect(skill?.definition.description).toContain("A skill from global .agents/skills directory")
+ } finally {
+ mock.restore()
+ }
+ })
</file context>
Summary
skilltool overrides OpenCode's nativeSkillTool, but was missing.agents/skills/directory support.agents/skills/(project-level) and~/.agents/skills/(global) were silently dropped when the plugin was activeEXTERNAL_DIRS = [".claude", ".agents"], but the plugin only covered.claude/skills/Root Cause
OpenCode's tool registry (
prompt.ts) builds tools astools[item.id] = .... Since plugin custom tools come after built-in tools, the plugin'sskilltool (id:"skill") overwrites OpenCode's nativeSkillTool. The plugin's skill discovery only searched.claude/skills/and.opencode/skills/, missing.agents/skills/entirely.Changes
loader.ts: AdddiscoverProjectAgentsSkills()(.agents/skills/, scope:"project") anddiscoverGlobalAgentsSkills()(~/.agents/skills/, scope:"user")skill-context.ts: Wire new discovery functions into the skill context pipelineloader.test.ts: Add 3 test cases covering project-level, global, and integration discoveryTesting
All 17 loader tests pass. No regressions.
Summary by cubic
Fixes skill discovery so the plugin always loads skills from .agents/skills at project and user scopes, matching OpenCode’s behavior. Prevents .agents skills from being dropped or gated behind the claude_code.skills flag.
Bug Fixes
Tests
Written for commit e4ed668. Summary will update on new commits.