feat: integrate skills.sh as first-class registry#75
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
|
Warning Rate limit exceeded
⌛ 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. 📝 WalkthroughWalkthroughThis PR introduces Skills.sh as a new integrated provider and registry into the skillkit ecosystem. It adds the Skills.sh source to the marketplace, implements a dedicated SkillsShProvider for cloning skills, and a SkillsShRegistry for federated searching and statistics retrieval. CLI commands are enhanced to leverage this registry in federated searches and display post-install popularity metrics. Changes
Sequence DiagramsequenceDiagram
participant User as User/CLI
participant Find as FindCommand
participant FedSearch as FederatedSearch
participant SkillsReg as SkillsShRegistry
participant GitReg as GitHubRegistry
participant SkillsAPI as skills.sh API
participant GitAPI as GitHub API
User->>Find: find [query]
Find->>FedSearch: search(query, options)
par Parallel Search
FedSearch->>SkillsReg: search(query)
SkillsReg->>SkillsAPI: fetch leaderboard
SkillsAPI-->>SkillsReg: HTML response
SkillsReg->>SkillsReg: parse & cache
SkillsReg-->>FedSearch: ExternalSkill[]
and
FedSearch->>GitReg: search(query)
GitReg->>GitAPI: query SKILL.md files
GitAPI-->>GitReg: search results
GitReg-->>FedSearch: ExternalSkill[]
end
FedSearch->>Find: FederatedResult
Find->>Find: format & display results
Find-->>User: formatted output (Skills.sh + GitHub sections)
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/cli/src/commands/install.ts (1)
303-305:⚠️ Potential issue | 🟡 MinorNo validation that
--agentvalues are validAgentTypeentries.
this.agentvalues are cast directly toAgentType[]without validation. Invalid agent names will pass through silently and fail later whengetAdapter(agentType)is called, producing a confusing error.♻️ Suggested validation
if (this.agent && this.agent.length > 0) { - // Explicitly specified agents - targetAgents = this.agent as AgentType[]; + // Validate explicitly specified agents + const allValid = getAllAdapters().map((a) => a.type); + const invalid = this.agent.filter((a) => !allValid.includes(a as AgentType)); + if (invalid.length > 0) { + error(`Unknown agent(s): ${invalid.join(", ")}`); + console.log(colors.muted(`Available: ${allValid.join(", ")}`)); + return 1; + } + targetAgents = this.agent as AgentType[];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/commands/install.ts` around lines 303 - 305, Validate the user-provided this.agent values before assigning to targetAgents: check each string against the known AgentType set/enum (the same source used by getAdapter(agentType)) and if any value is not a valid AgentType, produce a clear error message (or throw) listing the invalid entries and stop execution; otherwise cast/assign the validated array to targetAgents. Ensure this validation is done where targetAgents is set (the branch that currently does targetAgents = this.agent as AgentType[]) and reference AgentType and getAdapter(agentType) for the canonical list to validate against.
🧹 Nitpick comments (5)
packages/cli/src/commands/find.ts (2)
310-314:formatInstallsis duplicated asformatCountininstall.ts.Both
FindCommand.formatInstallsandInstallCommand.formatCount(install.ts line 695) have identical logic. Extract this into a shared utility (e.g., in a helpers module) to keep them in sync.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/commands/find.ts` around lines 310 - 314, The duplicate formatting logic in FindCommand.formatInstalls and InstallCommand.formatCount should be extracted into a single shared utility function (e.g., export function formatCount in a new helpers/utils module) and both methods should be replaced to call that utility; locate FindCommand.formatInstalls and InstallCommand.formatCount, move the conditional logic (>=1_000_000 => "M", >=1_000 => "K", else string) into the new exported function, import that helper in both files, and remove the duplicated implementations so both classes delegate to the shared formatCount helper.
199-206: Fragile coupling:skill.descriptionused as a path component in the install hint.
installSourcefalls back toskill.description, which currently happens to be"owner/repo"as set bySkillsShRegistry.parseLeaderboard. If the description format ever changes, this install command becomes invalid. Consider using a dedicated field (e.g.,skill.source) parsed to extractowner/repoinstead.♻️ Suggested fix
if (!this.quiet) { - const installSource = - skill.description || - skill.source.replace("https://github.com/", ""); + const installSource = + skill.source.replace("https://github.com/", ""); console.log( ` ${colors.muted(`skillkit install skills.sh/${installSource}/${skill.name}`)}`, );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/commands/find.ts` around lines 199 - 206, Replace the fragile use of skill.description for the install path with a deterministic parse of skill.source: in the block that computes installSource (the variable near the console.log inside the find command), extract "owner/repo" from skill.source by removing the "https://github.com/" prefix or by applying a regex that captures the owner/repo segment; fall back to skill.source unmodified only if parsing fails, and remove reliance on skill.description; update any parsing logic in SkillsShRegistry.parseLeaderboard to ensure skill.source consistently contains the original repo URL or owner/repo string so install hints built from installSource are always valid.packages/core/src/registry/skills-sh.ts (2)
73-101: Timer leak onAbortControllerabort path.When the fetch is aborted by the timer, execution enters the
catchblock whereclearTimeout(timer)is called — that's fine. However, ifresponse.text()(line 93) throws afterclearTimeout(timer)on line 89 already ran, it's also handled. The logic is correct.One minor note: there is no explicit handling to distinguish between a timeout abort and other fetch errors. Consider logging at debug level when
controller.signal.abortedis true so timeout vs network failure can be distinguished by operators.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/registry/skills-sh.ts` around lines 73 - 101, fetchLeaderboard may leak the timeout if an exception is thrown before clearTimeout is reached; refactor to ensure clearTimeout(timer) is always called by moving it into a finally block (so the AbortController timer is cleared on success, abort, or error) and in the catch handler check controller.signal.aborted to log a debug message differentiating timeout aborts from other fetch/network errors; update references to fetchLeaderboard, controller, timer and the catch/finally handling accordingly.
162-177: Extract shared parsing logic fromresolveSkillsShUrlandSkillsShProvider.parseSource.Both functions contain identical parsing logic: stripping the protocol, removing the
skills.sh/prefix, splitting on/, and filtering empty parts. Consider extracting a shared helper function to avoid divergence, or having one delegate to the other. Note thatparseSourcejoins multiple path segments forsubpath, whileresolveSkillsShUrlassigns onlyparts[2]toskillName—align this behavior when refactoring.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/registry/skills-sh.ts` around lines 162 - 177, Both resolveSkillsShUrl and SkillsShProvider.parseSource duplicate the same URL-cleaning and splitting logic; extract a single helper (e.g., parseSkillsShSource) and have both resolveSkillsShUrl and SkillsShProvider.parseSource call it (or have resolveSkillsShUrl delegate to SkillsShProvider.parseSource) so the logic stays in one place. Ensure the helper returns { owner, repo, skillName?, subpath? } where skillName/subpath behavior is consistent with parseSource by joining all remaining path segments (Array.slice(2).join("/") for the subpath/skillName) instead of only taking parts[2], and update resolveSkillsShUrl to use that joined value for skillName to avoid divergence between the two functions.packages/core/src/registry/index.ts (1)
137-160:FederatedSearchno longer forwardstimeoutMsto registries.The
searchmethod'soptionsparameter droppedtimeoutMs, so each registry falls back to its own hardcoded default (15s for SkillsShRegistry, 10s for GitHubSkillRegistry). If this was intentional, no action needed. If callers should be able to override the timeout, propagate it:async search( query: string, - options?: { limit?: number }, + options?: { limit?: number; timeoutMs?: number }, ): Promise<FederatedResult> { const limit = options?.limit ?? 20; const results = await Promise.allSettled( - this.registries.map((r) => r.search(query, { limit })), + this.registries.map((r) => r.search(query, { limit, timeoutMs: options?.timeoutMs })), );Also note: the
RateLimitErrorre-throw on line 158 breaks the resilience ofPromise.allSettled— if any registry is rate-limited, the entire federated search fails rather than returning partial results from other registries.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/registry/index.ts` around lines 137 - 160, The federated search dropped options.timeoutMs and also rethrows RateLimitError which aborts partial results; update the search method signature to accept options?: { limit?: number; timeoutMs?: number }, pass the timeout through when calling each registry's search (i.e. this.registries.map(r => r.search(query, { limit, timeoutMs }))), and change the error handling loop so RateLimitError instances are recorded (e.g. push the registry name into a rateLimitedRegistries array) instead of throwing, allowing all fulfilled results to be returned in the FederatedResult alongside the list of activeRegistries and any rate-limited registries.
🤖 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/cli/src/commands/find.ts`:
- Around line 158-165: The federated search block is only running when
this.query is set, so interactive searches (where the CLI populates a local
search variable via the prompt) skip SkillsShRegistry/GitHubSkillRegistry;
change the condition to check the resolved search term used for the rest of the
command (e.g., use the final search variable instead of only this.query or OR
with it) or move the FederatedSearch initialization to run whenever a search
term exists after the interactive prompt completes, ensuring FederatedSearch(),
addRegistry(new SkillsShRegistry()), and addRegistry(new GitHubSkillRegistry())
are executed for both direct CLI args and interactive input.
In `@packages/cli/src/commands/install.ts`:
- Around line 639-680: The function showSkillsShStats currently always
constructs a SkillsShRegistry and calls getSkillStats even when provider is not
Skills.sh; add an early guard that checks provider.type (e.g., if provider.type
!== 'skills.sh') and return immediately for non-skills.sh providers, then move
creation of new SkillsShRegistry() and the getSkillStats loop inside the guarded
branch so network requests only happen for provider.type === 'skills.sh'. Ensure
you reference showSkillsShStats, provider.type, SkillsShRegistry, and
getSkillStats when making the change.
- Around line 500-503: The npm install call currently uses the deprecated flag
"--production" in the execFileAsync invocation inside the install command;
update the execFileAsync call that runs npm (the call passing
["install","--production"] with cwd: targetPath) to use the modern flag
["install","--omit=dev"] instead so npm 7+ compatibility is ensured and dev deps
are omitted; leave the rest of the options (cwd: targetPath) unchanged.
In `@packages/core/src/providers/skills-sh.ts`:
- Around line 50-104: The clone method returns searchDir computed from
parsed.subpath without validating that it is inside the created tempDir,
allowing path traversal; update clone (function clone) to resolve and validate
subpath before using it: compute an absolute path for the candidate searchDir
(e.g., using path.resolve(tempDir, subpath || ".")) and verify containment
(e.g., with path.relative or the existing isPathInside helper) that the resolved
searchDir is inside tempDir, and if not return { success: false, error: "Invalid
subpath" } (or similar) before calling discoverSkills and before returning
path/tempRoot/discoveredSkills to ensure defense-in-depth.
In `@packages/core/src/registry/skills-sh.ts`:
- Around line 57-68: The loose matching in getSkillStats uses
skill.source.includes(source) which can yield false positives (e.g.,
"owner/repo-extended"); update the check to perform a strict match — compare
skill.source === source (or normalize both sides then compare) when locating the
exact repo for the given skillName, preserving the rest of the returned object
(rank, installs, name, source, skillName) and keeping the loop and return
behavior intact.
- Around line 104-145: The parseLeaderboard function performs fragile HTML
scraping of skills.sh using two regexes and currently can fail silently; add a
focused integration test that includes a real HTML snapshot of skills.sh
(covering both the detailed href pattern and the simpler skills.sh/... fallback)
to assert expected ExternalSkill entries and install parsing via
parseInstallCount, and modify parseLeaderboard to emit a debug-level warning
(using the module's logger) when html is non-empty but the returned skills array
is empty so site changes are visible; also enhance the fallback branch to
attempt extracting install counts (reusing parseInstallCount) when available in
the simpler matches.
---
Outside diff comments:
In `@packages/cli/src/commands/install.ts`:
- Around line 303-305: Validate the user-provided this.agent values before
assigning to targetAgents: check each string against the known AgentType
set/enum (the same source used by getAdapter(agentType)) and if any value is not
a valid AgentType, produce a clear error message (or throw) listing the invalid
entries and stop execution; otherwise cast/assign the validated array to
targetAgents. Ensure this validation is done where targetAgents is set (the
branch that currently does targetAgents = this.agent as AgentType[]) and
reference AgentType and getAdapter(agentType) for the canonical list to validate
against.
---
Duplicate comments:
In `@packages/cli/src/commands/install.ts`:
- Around line 695-699: The formatCount method in install.ts duplicates the logic
in FindCommand.formatInstalls in find.ts; extract this formatting logic into a
shared utility (e.g., create a new exported function formatInstalls or
formatCount in a common utils/module) and replace the local
InstallCommand.formatCount and FindCommand.formatInstalls implementations with
imports that call the shared function; update references in install.ts
(formatCount) and find.ts (FindCommand.formatInstalls) to use the new exported
utility and remove the duplicated code.
---
Nitpick comments:
In `@packages/cli/src/commands/find.ts`:
- Around line 310-314: The duplicate formatting logic in
FindCommand.formatInstalls and InstallCommand.formatCount should be extracted
into a single shared utility function (e.g., export function formatCount in a
new helpers/utils module) and both methods should be replaced to call that
utility; locate FindCommand.formatInstalls and InstallCommand.formatCount, move
the conditional logic (>=1_000_000 => "M", >=1_000 => "K", else string) into the
new exported function, import that helper in both files, and remove the
duplicated implementations so both classes delegate to the shared formatCount
helper.
- Around line 199-206: Replace the fragile use of skill.description for the
install path with a deterministic parse of skill.source: in the block that
computes installSource (the variable near the console.log inside the find
command), extract "owner/repo" from skill.source by removing the
"https://github.com/" prefix or by applying a regex that captures the owner/repo
segment; fall back to skill.source unmodified only if parsing fails, and remove
reliance on skill.description; update any parsing logic in
SkillsShRegistry.parseLeaderboard to ensure skill.source consistently contains
the original repo URL or owner/repo string so install hints built from
installSource are always valid.
In `@packages/core/src/registry/index.ts`:
- Around line 137-160: The federated search dropped options.timeoutMs and also
rethrows RateLimitError which aborts partial results; update the search method
signature to accept options?: { limit?: number; timeoutMs?: number }, pass the
timeout through when calling each registry's search (i.e. this.registries.map(r
=> r.search(query, { limit, timeoutMs }))), and change the error handling loop
so RateLimitError instances are recorded (e.g. push the registry name into a
rateLimitedRegistries array) instead of throwing, allowing all fulfilled results
to be returned in the FederatedResult alongside the list of activeRegistries and
any rate-limited registries.
In `@packages/core/src/registry/skills-sh.ts`:
- Around line 73-101: fetchLeaderboard may leak the timeout if an exception is
thrown before clearTimeout is reached; refactor to ensure clearTimeout(timer) is
always called by moving it into a finally block (so the AbortController timer is
cleared on success, abort, or error) and in the catch handler check
controller.signal.aborted to log a debug message differentiating timeout aborts
from other fetch/network errors; update references to fetchLeaderboard,
controller, timer and the catch/finally handling accordingly.
- Around line 162-177: Both resolveSkillsShUrl and SkillsShProvider.parseSource
duplicate the same URL-cleaning and splitting logic; extract a single helper
(e.g., parseSkillsShSource) and have both resolveSkillsShUrl and
SkillsShProvider.parseSource call it (or have resolveSkillsShUrl delegate to
SkillsShProvider.parseSource) so the logic stays in one place. Ensure the helper
returns { owner, repo, skillName?, subpath? } where skillName/subpath behavior
is consistent with parseSource by joining all remaining path segments
(Array.slice(2).join("/") for the subpath/skillName) instead of only taking
parts[2], and update resolveSkillsShUrl to use that joined value for skillName
to avoid divergence between the two functions.
| private async showSkillsShStats( | ||
| skillNames: string[], | ||
| provider: { | ||
| type: string; | ||
| parseSource: (s: string) => { owner: string; repo: string } | null; | ||
| }, | ||
| ): Promise<void> { | ||
| try { | ||
| const parsed = provider.parseSource(this.source); | ||
| if (!parsed) return; | ||
|
|
||
| const registry = new SkillsShRegistry(); | ||
| const statsResults: SkillsShStats[] = []; | ||
|
|
||
| for (const name of skillNames) { | ||
| const stats = await registry.getSkillStats( | ||
| parsed.owner, | ||
| parsed.repo, | ||
| name, | ||
| ); | ||
| if (stats) statsResults.push(stats); | ||
| } | ||
|
|
||
| if (statsResults.length > 0) { | ||
| console.log(""); | ||
| console.log(colors.bold("Skills.sh Popularity:")); | ||
| for (const s of statsResults) { | ||
| const rank = | ||
| s.rank <= 10 | ||
| ? colors.success(`#${s.rank}`) | ||
| : colors.muted(`#${s.rank}`); | ||
| const installs = | ||
| s.installs > 0 ? this.formatCount(s.installs) : "n/a"; | ||
| console.log( | ||
| ` ${rank} ${colors.primary(s.skillName)} ${colors.muted(`(${installs} installs)`)}`, | ||
| ); | ||
| } | ||
| } | ||
| } catch { | ||
| // Non-critical, silently skip | ||
| } | ||
| } |
There was a problem hiding this comment.
showSkillsShStats fires a network request for every install, even for non-skills.sh sources.
The method creates a SkillsShRegistry and calls getSkillStats (which fetches the leaderboard HTML) regardless of the provider type. For local installs or GitHub-only sources, this is a wasted 15-second-timeout network call. Guard on provider type:
♻️ Suggested guard
private async showSkillsShStats(
skillNames: string[],
provider: {
type: string;
parseSource: (s: string) => { owner: string; repo: string } | null;
},
): Promise<void> {
try {
+ if (provider.type === "local") return;
const parsed = provider.parseSource(this.source);
if (!parsed) return;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private async showSkillsShStats( | |
| skillNames: string[], | |
| provider: { | |
| type: string; | |
| parseSource: (s: string) => { owner: string; repo: string } | null; | |
| }, | |
| ): Promise<void> { | |
| try { | |
| const parsed = provider.parseSource(this.source); | |
| if (!parsed) return; | |
| const registry = new SkillsShRegistry(); | |
| const statsResults: SkillsShStats[] = []; | |
| for (const name of skillNames) { | |
| const stats = await registry.getSkillStats( | |
| parsed.owner, | |
| parsed.repo, | |
| name, | |
| ); | |
| if (stats) statsResults.push(stats); | |
| } | |
| if (statsResults.length > 0) { | |
| console.log(""); | |
| console.log(colors.bold("Skills.sh Popularity:")); | |
| for (const s of statsResults) { | |
| const rank = | |
| s.rank <= 10 | |
| ? colors.success(`#${s.rank}`) | |
| : colors.muted(`#${s.rank}`); | |
| const installs = | |
| s.installs > 0 ? this.formatCount(s.installs) : "n/a"; | |
| console.log( | |
| ` ${rank} ${colors.primary(s.skillName)} ${colors.muted(`(${installs} installs)`)}`, | |
| ); | |
| } | |
| } | |
| } catch { | |
| // Non-critical, silently skip | |
| } | |
| } | |
| private async showSkillsShStats( | |
| skillNames: string[], | |
| provider: { | |
| type: string; | |
| parseSource: (s: string) => { owner: string; repo: string } | null; | |
| }, | |
| ): Promise<void> { | |
| try { | |
| if (provider.type === "local") return; | |
| const parsed = provider.parseSource(this.source); | |
| if (!parsed) return; | |
| const registry = new SkillsShRegistry(); | |
| const statsResults: SkillsShStats[] = []; | |
| for (const name of skillNames) { | |
| const stats = await registry.getSkillStats( | |
| parsed.owner, | |
| parsed.repo, | |
| name, | |
| ); | |
| if (stats) statsResults.push(stats); | |
| } | |
| if (statsResults.length > 0) { | |
| console.log(""); | |
| console.log(colors.bold("Skills.sh Popularity:")); | |
| for (const s of statsResults) { | |
| const rank = | |
| s.rank <= 10 | |
| ? colors.success(`#${s.rank}`) | |
| : colors.muted(`#${s.rank}`); | |
| const installs = | |
| s.installs > 0 ? this.formatCount(s.installs) : "n/a"; | |
| console.log( | |
| ` ${rank} ${colors.primary(s.skillName)} ${colors.muted(`(${installs} installs)`)}`, | |
| ); | |
| } | |
| } | |
| } catch { | |
| // Non-critical, silently skip | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cli/src/commands/install.ts` around lines 639 - 680, The function
showSkillsShStats currently always constructs a SkillsShRegistry and calls
getSkillStats even when provider is not Skills.sh; add an early guard that
checks provider.type (e.g., if provider.type !== 'skills.sh') and return
immediately for non-skills.sh providers, then move creation of new
SkillsShRegistry() and the getSkillStats loop inside the guarded branch so
network requests only happen for provider.type === 'skills.sh'. Ensure you
reference showSkillsShStats, provider.type, SkillsShRegistry, and getSkillStats
when making the change.
| private parseLeaderboard(html: string): ExternalSkill[] { | ||
| const skills: ExternalSkill[] = []; | ||
|
|
||
| const lines = html.split("\n"); | ||
| for (const line of lines) { | ||
| // Match patterns like: [1vercel-labs/skills290.0K] from the page links | ||
| const hrefMatch = line.match( | ||
| /href="\/([^/]+)\/([^/]+)\/([^"]+)".*?(\d[\d,.]*[KMB]?)\s*<\/a>/, | ||
| ); | ||
| if (hrefMatch) { | ||
| const [, owner, repo, skillName, installStr] = hrefMatch; | ||
| const installs = this.parseInstallCount(installStr); | ||
|
|
||
| skills.push({ | ||
| name: skillName, | ||
| description: `${owner}/${repo}`, | ||
| source: `https://github.com/${owner}/${repo}`, | ||
| registry: this.name, | ||
| path: skillName, | ||
| stars: installs, | ||
| }); | ||
| } | ||
| } | ||
|
|
||
| // Fallback: try simpler pattern matching if structured parsing found nothing | ||
| if (skills.length === 0) { | ||
| const simplePattern = /skills\.sh\/([^/]+)\/([^/]+)\/([^/"]+)/g; | ||
| let match; | ||
| while ((match = simplePattern.exec(html)) !== null) { | ||
| const [, owner, repo, skillName] = match; | ||
| skills.push({ | ||
| name: skillName, | ||
| description: `${owner}/${repo}`, | ||
| source: `https://github.com/${owner}/${repo}`, | ||
| registry: this.name, | ||
| path: skillName, | ||
| }); | ||
| } | ||
| } | ||
|
|
||
| return skills; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
skills.sh public API JSON endpoint leaderboard
💡 Result:
Skills.sh does not publish a documented “public API” (or official JSON endpoint) for the leaderboard. The official docs only describe browsing the leaderboard on the website and explain that rankings come from anonymous CLI telemetry—without mentioning any API/JSON endpoints. [1]
If you just need leaderboard data, the site exposes it as HTML pages you can scrape (e.g., the “Hot” leaderboard at https://skills.sh/hot, plus tabs for “All Time” and “Trending (24h)”). [2]
Sources:
[1] Skills.sh Documentation (https://skills.sh/docs)
[2] Skills.sh Hot leaderboard page (https://skills.sh/hot)
🏁 Script executed:
# Examine the code file and related test files
find . -name "*skills*" -type f | grep -E "\.(ts|js|test|spec)" | head -20Repository: rohitg00/skillkit
Length of output: 486
🏁 Script executed:
# Check if there are any tests for the skills-sh registry
find . -path "*/test*" -name "*skills*" -type fRepository: rohitg00/skillkit
Length of output: 83
🏁 Script executed:
# Examine the fetchLeaderboard and parseLeaderboard methods in detail
cat -n packages/core/src/registry/skills-sh.tsRepository: rohitg00/skillkit
Length of output: 6075
🏁 Script executed:
# Check the skills test file
cat -n tests/skills.test.tsRepository: rohitg00/skillkit
Length of output: 16236
🏁 Script executed:
# Also check if there are any snapshots or fixtures for HTML testing
find . -name "*.test.ts" -o -name "*.spec.ts" | xargs grep -l "skills" | head -10Repository: rohitg00/skillkit
Length of output: 430
🏁 Script executed:
# Look for any HTML fixtures or mocks related to skills.sh
find . -name "*skills*" -type f | grep -E "(fixture|mock|snapshot)" | head -20Repository: rohitg00/skillkit
Length of output: 43
🏁 Script executed:
# Check if parseLeaderboard is tested anywhere
rg -A 5 "parseLeaderboard" --type tsRepository: rohitg00/skillkit
Length of output: 957
No public API exists for skills.sh—HTML scraping is the only option, but it has no test coverage and will fail silently on site changes.
skills.sh does not publish a documented public API or JSON endpoint for leaderboard data (verified via official docs). This means the line-by-line regex parsing in parseLeaderboard (lines 107–126) and its fallback regex (lines 129–142) are the only integration point. Any change to skills.sh's HTML structure will silently produce zero results with no warning.
The code has no tests, fixtures, or safeguards for this fragile parsing:
- No integration tests validating the regex against actual skills.sh HTML
- No logging when parsing yields zero results from non-empty HTML
- Fallback regex has no install count extraction
Strongly recommend:
- Add an integration test with an HTML snapshot from skills.sh to validate both regex patterns and alert on changes.
- Log a warning (at debug level) when
parseLeaderboardreturns an empty array from non-empty HTML.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/core/src/registry/skills-sh.ts` around lines 104 - 145, The
parseLeaderboard function performs fragile HTML scraping of skills.sh using two
regexes and currently can fail silently; add a focused integration test that
includes a real HTML snapshot of skills.sh (covering both the detailed href
pattern and the simpler skills.sh/... fallback) to assert expected ExternalSkill
entries and install parsing via parseInstallCount, and modify parseLeaderboard
to emit a debug-level warning (using the module's logger) when html is non-empty
but the returned skills array is empty so site changes are visible; also enhance
the fallback branch to attempt extracting install counts (reusing
parseInstallCount) when available in the simpler matches.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| { "source": "openrouterteam/agent-skills", "name": "OpenRouter SDK" }, | ||
| { "source": "intellectronica/agent-skills", "name": "Context7" } | ||
| { "source": "intellectronica/agent-skills", "name": "Context7" }, | ||
| { "source": "skills.sh", "name": "Skills.sh Leaderboard", "official": true, "registry": "skills-sh" } |
There was a problem hiding this comment.
🟡 sources.json entry violates its own JSON schema pattern
The new skills.sh entry in sources.json has "source": "skills.sh", which violates the schema pattern defined in sources-schema.json:34: "^[a-zA-Z0-9_-]+/[a-zA-Z0-9_-]+$". The pattern requires owner/repo format (two segments separated by / with only alphanumeric, underscore, and hyphen characters). "skills.sh" has a dot and no slash.
Impact and Details
The schema at marketplace/sources-schema.json:34 explicitly defines "pattern": "^[a-zA-Z0-9_-]+/[a-zA-Z0-9_-]+$" for the source field, and the sources.json references this schema via "$schema": "./sources-schema.json". Any tooling (CI, editors, JSON schema validators) that validates sources.json against its schema will report a validation error for this entry.
Additionally, this entry is marked "official": true. While the --top feature in find.ts:93-95 currently takes .slice(0, 5) of official sources (and skills.sh is the 7th official entry so it's excluded), if earlier official entries are removed or reordered, the string "skills.sh" would be used in the skill.source.includes(f.source) check at packages/cli/src/commands/find.ts:100, which could cause false positive matches on unrelated skill sources containing that substring.
Was this helpful? React with 👍 or 👎 to provide feedback.
| if (result.status === "fulfilled" && result.value.length > 0) { | ||
| allSkills.push(...result.value); | ||
| activeRegistries.push(this.registries[i].name); | ||
| } else if (result.status === 'rejected' && result.reason instanceof RateLimitError) { | ||
| throw result.reason; | ||
| } |
There was a problem hiding this comment.
🟡 RateLimitError silently swallowed — users lose actionable GITHUB_TOKEN guidance
The FederatedSearch.search() method previously re-threw RateLimitError from rejected promises, and the find command caught it to show a specific message: "Set GITHUB_TOKEN env var or wait before retrying." Both the re-throw and the specific catch handler were removed. Now when GitHub rate-limits the user (common without a token — only 10 requests/minute), they see only a generic "External search unavailable" message.
Root Cause
In the old FederatedSearch.search(), lines 141-143 of packages/core/src/registry/index.ts contained:
} else if (result.status === 'rejected' && result.reason instanceof RateLimitError) {
throw result.reason;
}This was removed in the new code. The GitHubSkillRegistry.search() still throws RateLimitError at line 71, and still re-throws it in its own catch at line 120. But FederatedSearch wraps calls in Promise.allSettled, and the new code simply ignores all rejected results without checking the reason.
Meanwhile, in packages/cli/src/commands/find.ts, the import of RateLimitError was removed, and the catch block was changed from specific handling (showing GITHUB_TOKEN guidance) to a bare catch { s.stop(colors.warning("External search unavailable")); }.
This means when --federated is used and GitHub rate-limits the request, users get no guidance on how to fix it (by setting GITHUB_TOKEN), making the error harder to resolve.
Was this helpful? React with 👍 or 👎 to provide feedback.
Add SkillsShRegistry and SkillsShProvider so users can install skills directly from skills.sh (e.g. `skillkit install skills.sh/owner/repo/skill`). The find command now searches skills.sh by default alongside the local marketplace. Post-install shows skills.sh popularity stats and pro tips highlighting skillkit's unique features (translate, recommend, evaluate).
…ration - Fix path traversal vulnerability in SkillsShProvider.clone (validate subpath containment) - Fix federated search skipped for interactive queries (use searchTerm instead of this.query) - Guard showSkillsShStats to skip network call for local providers - Replace deprecated npm --production with --omit=dev - Fix loose includes() match in getSkillStats to use strict endsWith() - Fix FederatedSearch resilience: don't re-throw RateLimitError, return partial results - Forward timeoutMs through FederatedSearch to registries - Extract shared formatCount utility to helpers.ts (deduplicate find.ts/install.ts) - Fix install hint to use skill.source instead of fragile skill.description - Fix timer leak in fetchLeaderboard using finally block - Deduplicate URL parsing via shared parseSkillsShParts helper - Validate --agent flag values against known agent types
rohitg00
force-pushed
the
feat/skillssh-integration
branch
from
37fbbd9 to
2b63b37
Compare
1 participant
Add SkillsShRegistry and SkillsShProvider so users can install skills directly from skills.sh (e.g.
skillkit install skills.sh/owner/repo/skill). The find command now searches skills.sh by default alongside the local marketplace. Post-install shows skills.sh popularity stats and pro tips highlighting skillkit's unique features (translate, recommend, evaluate).Summary by CodeRabbit
New Features
Chores