Feature/install skills#13
Conversation
- Add skills-link add command - Support user/repo and full URL formats - Support --skill flag to install specific skill - Update README with new command
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 40 minutes and 4 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe PR introduces a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as skills-link add
participant Config as Config Loader
participant Git as GitHub
participant FS as File System
participant Inquirer as User Selection
User->>CLI: Run: skills-link add <repo> [--skill X]
CLI->>Config: Load masterDir config
Config-->>CLI: Config data
alt Config invalid
CLI-->>User: Error: Missing/invalid config
else Config valid
CLI->>Git: Clone repo (shallow)
Git-->>FS: Create temp directory
Git-->>FS: Clone into .temp-add/
alt Clone succeeds
CLI->>FS: Find directories with SKILL.md
alt Found skills
alt --skill specified
CLI->>CLI: Use specified skill
else Single skill found
CLI->>CLI: Auto-select
else Multiple skills
CLI->>Inquirer: Prompt skill selection
Inquirer-->>User: Show skill choices
User-->>Inquirer: Select skill
Inquirer-->>CLI: Selected skill
end
CLI->>Config: Check existing skills
Config-->>CLI: Existing skill list
alt Skill exists
CLI->>Inquirer: Prompt overwrite
Inquirer-->>User: Overwrite? (y/n)
User-->>Inquirer: Response
Inquirer-->>CLI: User choice
alt User declines
CLI-->>User: Cancelled
else User confirms
CLI->>FS: Remove old skill
CLI->>FS: Move new skill to masterDir
CLI-->>User: Installation success
end
else New skill
CLI->>FS: Move skill to masterDir
CLI-->>User: Installation success
end
CLI->>FS: Clean up temp directory
else No skills found
CLI-->>User: Error: No SKILL.md found
CLI->>FS: Remove temp directory
end
else Clone fails
CLI-->>User: Error: Clone failed
CLI->>FS: Remove temp directory
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new add command to the skills-link CLI, enabling users to install skills directly from GitHub repositories. The update includes changes to the CLI entry point, documentation, and the addition of a new command module. Review feedback identified several critical issues in the add command implementation: a naming bug when skills are located at the repository root, a recursive search function that misses root-level skills and uses synchronous methods incorrectly, and redundant logic in URL parsing. Furthermore, inconsistencies were found in the localization files, where certain keys are unused or missing across languages.
| } else if (skillDirs.length === 1) { | ||
| // 只有一个，直接使用 | ||
| targetDir = skillDirs[0] | ||
| skillName = path.basename(targetDir) |
There was a problem hiding this comment.
If the skill is found at the root of the repository (i.e., targetDir is the temporary directory), path.basename(targetDir) will return .temp-add instead of the actual skill or repository name. You should handle this case by deriving the name from the repository URL or the user input.
| async function findSkillDirs(dir: string): Promise<string[]> { | ||
| const results: string[] = [] | ||
| const entries = fs.readdirSync(dir, { withFileTypes: true }) | ||
|
|
||
| for (const entry of entries) { | ||
| const fullPath = path.join(dir, entry.name) | ||
|
|
||
| if (entry.isDirectory()) { | ||
| // 检查是否包含 SKILL.md | ||
| const skillFile = path.join(fullPath, 'SKILL.md') | ||
| if (fs.existsSync(skillFile)) { | ||
| results.push(fullPath) | ||
| } else { | ||
| // 递归搜索子目录 | ||
| const subDirs = await findSkillDirs(fullPath) | ||
| results.push(...subDirs) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return results | ||
| } |
There was a problem hiding this comment.
There are two issues here:
- Bug: The function misses
SKILL.mdfiles located at the root of the repository because it only iterates over subdirectories. - Consistency: The function is marked
asyncbut uses synchronous filesystem methods (fs.readdirSync,fs.existsSync). It's better to make it fully synchronous or usefs.promises.
Also, it's good practice to ignore the .git directory during recursion.
function findSkillDirs(dir: string): string[] {
const results: string[] = []
// Check if the directory itself is a skill
if (fs.existsSync(path.join(dir, 'SKILL.md'))) {
return [dir]
}
const entries = fs.readdirSync(dir, { withFileTypes: true })
for (const entry of entries) {
if (entry.isDirectory() && entry.name !== '.git') {
const fullPath = path.join(dir, entry.name)
const skillFile = path.join(fullPath, 'SKILL.md')
if (fs.existsSync(skillFile)) {
results.push(fullPath)
} else {
results.push(...findSkillDirs(fullPath))
}
}
}
return results
}| await git.clone(repoUrl, tempDir, ['--depth', '1']) | ||
|
|
||
| // 查找 SKILL.md 文件 | ||
| const skillDirs = await findSkillDirs(tempDir) |
There was a problem hiding this comment.
If findSkillDirs is changed to a synchronous function (as suggested below), the await keyword here should be removed.
| const skillDirs = await findSkillDirs(tempDir) | |
| const skillDirs = findSkillDirs(tempDir) |
| if (input.startsWith('https://github.com/')) { | ||
| return input.replace('https://github.com/', 'https://github.com/') | ||
| } |
There was a problem hiding this comment.
This replacement is redundant as it replaces the string with itself. You can simply return the input.
| if (input.startsWith('https://github.com/')) { | |
| return input.replace('https://github.com/', 'https://github.com/') | |
| } | |
| if (input.startsWith('https://github.com/')) { | |
| return input | |
| } |
| "linkPrompt": "是否立即创建链接？", | ||
| "linking": "正在创建链接...", | ||
| "linksCreated": "已创建 {count} 个链接", |
There was a problem hiding this comment.
These translation keys (linkPrompt, linking, linksCreated) are present in the Chinese locale but missing in the English locale. Additionally, they do not appear to be used in the current implementation of the add command. If you intended to offer automatic linking after installation, the logic seems to be missing from src/commands/add.ts.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
bin/skills-link.js (1)
97-103: Argument/option descriptions are not localized.The command description uses
t('cli.commands.add'), but the<repo>argument and--skilloption descriptions are hardcoded English. Other commands consistently uset(...)for option help (e.g.,cli.options.message,cli.options.dryRun). Consider addingcli.options.skill(and an arg description key) tosrc/locales/{en,zh}.jsonand usingt(...)here for consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/skills-link.js` around lines 97 - 103, The argument and option descriptions for the "add" command are hardcoded English; update the .argument('<repo>', ...) and .option('-s, --skill <name>', ...) calls in the program.command('add') handler to use localized strings via t(...), e.g., add keys like cli.args.repo and cli.options.skill to your locale files (src/locales/{en,zh}.json) and replace the literal descriptions with t('cli.args.repo') and t('cli.options.skill') so they match other commands that use t(...).src/commands/add.ts (1)
195-216:findSkillDirsrecurses into.gitand other irrelevant directories.The cloned repo always has a
.gitsubtree which can contain thousands of entries; the function recurses into it because it has noSKILL.md. Skipping.git,node_modules, and other dotfile dirs would be a meaningful speedup on real repos and is a one-line guard.for (const entry of entries) { + if (entry.name === '.git' || entry.name === 'node_modules' || entry.name.startsWith('.')) { + continue + } const fullPath = path.join(dir, entry.name)Also, this is declared
asyncbut uses fully synchronousfs.readdirSync/fs.existsSync; consider switching tofs.promises.readdirfor consistency, or drop theasyncand unnecessaryawaitat line 209.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/add.ts` around lines 195 - 216, The findSkillDirs function currently recurses into irrelevant dot dirs (like .git and node_modules) and mixes synchronous fs calls with async/await; fix by skipping dot and known junk dirs and making I/O consistently async: inside findSkillDirs, add a short-circuit guard that ignores directories whose name starts with '.' or are in a SKIP_DIRS set (e.g., const SKIP_DIRS = new Set(['.git','node_modules'])), and convert the body to use fs.promises.readdir(fullPath, { withFileTypes: true }) and fs.promises.access or fs.promises.stat for checking SKILL.md so you can await the recursive call to findSkillDirs(fullPath) rather than calling sync APIs; update the await at the recursive call site accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/commands/add.ts`:
- Around line 168-190: parseRepoUrl currently has a no-op replace and a loose
host/path check; update parseRepoUrl to (1) normalize and validate explicit
GitHub URLs only by checking hostname for https/http via new URL(input).hostname
=== 'github.com' and accept SSH only when it starts with 'git@github.com:'; (2)
remove the useless input.replace(...) branch and instead normalize by stripping
a trailing '.git' and any trailing slash from returned URLs; and (3) tighten the
user/repo fallback by validating input against a safe regex like
^[\\w.-]+/[\\w.-]+$ before returning `https://github.com/${input}` so inputs
like '../foo/bar' are rejected. Ensure these changes are applied inside the
parseRepoUrl function.
- Around line 58-162: The temp clone uses a fixed tempDir (const tempDir =
path.join(masterDir, '.temp-add')) which risks leftover .git and race
conditions; change to create a unique temp directory using os.tmpdir() and
fs.mkdtempSync('skills-link-add-') (or append process.pid/Date.now()) and wrap
the main work in a try { ... } finally { if (fs.existsSync(tempDir))
fs.rmSync(tempDir, { recursive: true, force: true }) } to guarantee cleanup;
also replace fs.renameSync(targetDir, destDir) with a cross-device-safe
copy-then-remove (or use a recursive copy helper) to move from tempDir to
destDir; update references to tempDir, masterDir, and functions
findSkillDirs/listSkills accordingly.
In `@src/locales/en.json`:
- Around line 418-435: The en.json "add" namespace is missing the keys
"linkPrompt", "linking", and "linksCreated" that exist in zh.json; update the
en.json "add" block to include these three keys (with appropriate English text)
if the intended flow will call runAdd's linking step, or remove those keys from
zh.json if no linking step is ever invoked; check the runAdd function to decide
whether to add and wire these messages into the linking flow (use the same key
names: linkPrompt, linking, linksCreated) so both locales remain parity with the
actual runAdd behavior.
---
Nitpick comments:
In `@bin/skills-link.js`:
- Around line 97-103: The argument and option descriptions for the "add" command
are hardcoded English; update the .argument('<repo>', ...) and .option('-s,
--skill <name>', ...) calls in the program.command('add') handler to use
localized strings via t(...), e.g., add keys like cli.args.repo and
cli.options.skill to your locale files (src/locales/{en,zh}.json) and replace
the literal descriptions with t('cli.args.repo') and t('cli.options.skill') so
they match other commands that use t(...).
In `@src/commands/add.ts`:
- Around line 195-216: The findSkillDirs function currently recurses into
irrelevant dot dirs (like .git and node_modules) and mixes synchronous fs calls
with async/await; fix by skipping dot and known junk dirs and making I/O
consistently async: inside findSkillDirs, add a short-circuit guard that ignores
directories whose name starts with '.' or are in a SKIP_DIRS set (e.g., const
SKIP_DIRS = new Set(['.git','node_modules'])), and convert the body to use
fs.promises.readdir(fullPath, { withFileTypes: true }) and fs.promises.access or
fs.promises.stat for checking SKILL.md so you can await the recursive call to
findSkillDirs(fullPath) rather than calling sync APIs; update the await at the
recursive call site accordingly.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: dfe2637b-7505-4115-8694-d41830726ece
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (8)
.claude/settings.local.jsonREADME.mdREADME.zh.mdbin/skills-link.jspackage.jsonsrc/commands/add.tssrc/locales/en.jsonsrc/locales/zh.json
| "add": { | ||
| "noConfig": "Config file not found", | ||
| "runSetupFirst": "Please run skills-link setup first", | ||
| "masterDirNotExist": "Master directory does not exist", | ||
| "usage": "Usage: skills-link add <repo>", | ||
| "usageExample": "Example: skills-link add vercel-labs/agent-skills", | ||
| "invalidUrl": "Invalid GitHub address", | ||
| "cloning": "Cloning {url}...", | ||
| "cloneFailed": "Clone failed: {error}", | ||
| "noSkillFound": "No SKILL.md file found", | ||
| "selectSkill": "Select skill to install:", | ||
| "skillExists": "Skill {name} already exists", | ||
| "overwritePrompt": "Overwrite?", | ||
| "installed": "Installed: {name}", | ||
| "done": "Install complete!", | ||
| "skillNotFound": "Skill not found: {name}", | ||
| "availableSkills": "Available skills:" | ||
| } |
There was a problem hiding this comment.
Locale parity: add namespace differs between en.json and zh.json.
src/locales/zh.json adds three additional keys under add: linkPrompt, linking, linksCreated. None of these exist here in en.json, and src/commands/add.ts does not reference them either. Either drop them from zh.json (current code path doesn't create links after install) or, if a follow-up "create links" step is intended, mirror the keys here and wire them into runAdd. Otherwise users running with --lang zh will see strings that are never displayed and English users won't have parity if they ever are.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/locales/en.json` around lines 418 - 435, The en.json "add" namespace is
missing the keys "linkPrompt", "linking", and "linksCreated" that exist in
zh.json; update the en.json "add" block to include these three keys (with
appropriate English text) if the intended flow will call runAdd's linking step,
or remove those keys from zh.json if no linking step is ever invoked; check the
runAdd function to decide whether to add and wire these messages into the
linking flow (use the same key names: linkPrompt, linking, linksCreated) so both
locales remain parity with the actual runAdd behavior.
- Use unique temp dir with os.tmpdir() and timestamp - Add try/finally for guaranteed cleanup - Use recursive copy instead of rename for cross-device safety - Tighten URL validation with regex for user/repo - Remove unused linking prompt
1 participant
Feature/install skills
Summary by CodeRabbit
New Features
skills-link addcommand to install skills directly from GitHub repositories with optional skill selection for specific installations.Documentation
add,list,sync,app) with practical GitHub repository installation examples in English and Chinese.