feat(core): add symlink support for skill manager#1690
Conversation
Add support for loading skills from symlinked directories in the skill manager. This allows users to organize and share skills more flexibly by using symbolic links. Changes: - Modified skill discovery logic to detect and follow symlinks - Added validation to ensure symlink targets point to valid directories - Skip broken or invalid symlinks with appropriate warnings - Added comprehensive test coverage for symlink scenarios
📋 Review SummaryThis PR adds symlink support to the Skill Manager, allowing skills to be loaded from symbolic links. The implementation is well-designed with proper error handling for broken symlinks and non-directory targets. The changes include comprehensive tests covering various symlink scenarios. 🔍 General Feedback
🎯 Specific Feedback🟡 High
🟢 Medium
🔵 Low
✅ Highlights
|
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
|
@DragonnZhang please see my comment in #1695. I believe this doesn't solve the issue and in fact the issue has a wrong description of the proposed standard. |
|
@douglascamata opened a new issue for it here: |
TLDR
Added symlink support to Skill Manager, allowing skill directories to be symbolic links. This enables users to share skills across multiple projects or reference skills from external paths.
Dive Deeper
Background
The previous Skill Manager implementation only recognized real directories and did not support symbolic links. This limited users' ability to flexibly manage skills, such as:
Main Changes
Symlink Detection (
packages/core/src/skills/skill-manager.ts:414-417)isDirectory()andisSymbolicLink()fs.stat()to verify the target is a directorySymlink Validation (
packages/core/src/skills/skill-manager.ts:420-435)Comprehensive Test Coverage (
packages/core/src/skills/skill-manager.test.ts)isSymbolicLink()methodTechnical Details
fs.stat()instead offs.lstat()to get information about the symlink targetReviewer Test Plan
Test Environment Setup
Verification Steps
Load symlinked skill
/skillscommandlinked-skill(test-symlink-skill) appears in the skill listTest broken symlink
/skillscommandTest symlink pointing to file
/skillscommandRun test suite
npm run test -- packages/core/src/skills/skill-manager.test.tsTesting Matrix
Tested on macOS (🍏) environment with local and unit tests.
Linked issues / bugs
This PR enhances the flexibility of skill management to support more use cases. No associated issues, this is a feature enhancement.