feat: runtime skill discovery — REST API, MCP server, cache, ranking, Python client #42
Conversation
… Python client (#42) Add runtime skill discovery capabilities enabling agents to autonomously search and retrieve skills on demand, inspired by skyll.app's approach of making skills queryable by intent rather than requiring pre-installation. Core infrastructure: - Pluggable cache backend with LRU + TTL (MemoryCache) - Multi-signal relevance ranker (content 40pts, query 30pts, popularity 15pts, refs 15pts) - Enhanced SKILL.md parser with frontmatter extraction and reference discovery - Runtime skill injector fetching SKILL.md from GitHub with caching REST API server (@skillkit/api): - Hono-based HTTP server on port 3737 - GET/POST /search with ranked results and filters - GET /skills/:source/:id, /trending, /categories, /health, /cache/stats - Sliding-window rate limiting (60 req/min/IP) MCP server (@skillkit/mcp): - 4 tools: search_skills, get_skill, recommend_skills, list_categories - 2 resources: skills://trending, skills://categories - Compatible with Claude Desktop, Cursor, any MCP client CLI serve command: - `skillkit serve` starts the API server - Options: --port, --host, --cors, --cache-ttl Community registry: - registry/SKILLS.md with 26 curated skills across 12 categories - CommunityRegistry class integrated into FederatedSearch Python client (skillkit-client): - Async httpx + pydantic client for the REST API - search, get_skill, trending, categories, cache_stats, health All 856+ tests pass across 24 workspace tasks.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughAdds a new TypeScript REST API package, an async Python client with Pydantic models and tests, MCP integration, core subsystems (LRU MemoryCache, relevance ranker, parser, runtime injector, community registry), a Serve CLI command, tests across packages, and workspace/package configuration. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User/CLI
participant API as REST API Server
participant Cache as MemoryCache
participant Ranker as RelevanceRanker
participant FS as Filesystem
User->>API: GET /search?q=react
activate API
API->>Cache: get("search:q=react|limit=20|inc=false")
activate Cache
Cache-->>API: miss
deactivate Cache
API->>FS: read skills data
activate FS
FS-->>API: skills[]
deactivate FS
API->>Ranker: rank(skills, "react")
activate Ranker
Ranker-->>API: RankedSkill[]
deactivate Ranker
API->>Cache: set("search:...", SearchResponse)
activate Cache
Cache-->>API: ok
deactivate Cache
API-->>User: 200 SearchResponse
deactivate API
sequenceDiagram
participant Client as Python Client
participant Network as Network/HTTP
participant API as REST API Server
participant GitHub as GitHub API
Client->>Client: __aenter__ (init AsyncClient)
Client->>Network: GET /health
Network->>API: /health
API-->>Network: HealthResponse
Network-->>Client: response
Client->>Network: POST /search (filters)
Network->>API: /search
API->>API: apply filters, rank
API-->>Network: SearchResponse
Network-->>Client: response
Client->>Network: GET /skills/{source}/{id}
Network->>API: /skills/{source}/{id}
API-->>Network: Skill
Network-->>Client: response
Client->>Client: __aexit__ (close AsyncClient)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~70 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 |
Add explicit return type to rate limiter middleware and remove stale CategoryCount type annotation in categories route.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
🤖 Fix all issues with AI agents
In `@clients/python/pyproject.toml`:
- Around line 39-42: The Documentation URL under the [project.urls] table
currently points to "agenstskills.com" which appears misspelled; update the
Documentation key in pyproject.toml (under [project.urls]) to the correct domain
(e.g., "agentskills.com" or the intended documentation URL) so the
Repository/Documentation metadata are accurate and not broken.
In `@clients/python/skillkit/__init__.py`:
- Around line 4-11: The __all__ list in clients/python/skillkit/__init__.py is
unsorted and triggers Ruff RUF022; reorder the __all__ entries alphabetically
(e.g., CacheStats, Category, HealthResponse, SearchResponse, Skill,
SkillKitClient) so the module-level __all__ is sorted, then run the linter to
confirm the warning is resolved.
In `@clients/python/skillkit/client.py`:
- Around line 24-36: The __aenter__ method currently unconditionally creates a
new httpx.AsyncClient and can overwrite an existing client created by
_get_client, leaking the original; change __aenter__ to only create a new
AsyncClient if self._client is None (mirror _get_client's logic) so the existing
client is reused, while leaving __aexit__'s close behavior (await
self._client.aclose(); self._client = None) intact to properly release
resources.
In `@packages/api/src/middleware/rate-limit.ts`:
- Around line 20-41: The middleware handler currently awaits next() but does not
return its result, which violates noImplicitReturns and prevents proper
propagation; update the async handler returned by the top-level function (the
anonymous async (c: Context, next: Next) => { ... } that uses windows, windowMs,
maxRequests) to return the next() call (e.g., replace the final await next()
with return await next() or return next()) so both the 429 branch and the
allowed-request path always return a value.
In `@packages/api/src/routes/categories.ts`:
- Around line 10-20: The loop in categories.ts currently mixes skill.tags and
skill.category into one map (tagCounts) and prefixes categories with
"category:", which leaks internal format; change the aggregation to maintain two
separate collections (e.g., tagCounts for skill.tags and categoryCounts for
skill.category) while iterating over skills (referencing skill.tags,
skill.category and tagCounts), increment counts into the appropriate map without
adding a "category:" prefix, and adjust the endpoint response shape to return
tags and categories as distinct fields so clients receive raw category names
rather than prefixed values.
- Line 22: The code uses the TypeScript type CategoryCount when creating the
categories array (const categories: CategoryCount[] =
Array.from(tagCounts.entries())) but never imports it; add an import for
CategoryCount from the module that defines your shared types (e.g., the file
that exports other route types) so the type resolves. Locate the categories.ts
file and add a named import like import { CategoryCount } from
'<your-types-module>'; ensure the import path matches where CategoryCount is
declared and keep the existing usage of tagCounts and categories unchanged.
In `@packages/api/src/routes/search.ts`:
- Around line 23-41: The ranking code currently maps ApiSkill to a reduced
object then looks up originals by name causing incorrect matches and O(n)
lookups; instead call ranker.rank<ApiSkill>() with the full skills array (pass
skills directly) so the returned ranked items reference the original ApiSkill
objects, then build results from the ranked slice (respect includeContent logic
on each ranked item) rather than using skills.find by name; update the usages
around ranker.rank, the ranked variable, and the results construction (also
apply same change to the analogous block handling lines 83-101).
In `@packages/api/src/routes/skills.ts`:
- Around line 11-13: The current lookup uses a loose substring match
(s.source.includes(source)) which can produce false positives; change the
matching in the skills.find call to a stricter check such as exact equality
(s.source === source) or a pattern-aware check (e.g., s.source.startsWith(...)
or s.source.endsWith(...)) depending on source shape, so update the predicate
that creates skill (the skills.find lambda) to use the chosen stricter
comparator while keeping the name check (s.name === id) intact.
In `@packages/api/src/routes/trending.ts`:
- Around line 12-27: The mapping from ranked results back to original skills
uses name-only lookup and can return the wrong entry when duplicate names exist
across sources; update the mapping in the results computation to use both source
and name: ensure the objects passed into ranker.rank include source, ensure you
read r.skill.source (in addition to r.skill.name) from the ranked entry, and
change the lookup from skills.find((s) => s.name === r.skill.name) to a dual-key
lookup like skills.find((s) => s.source === r.skill.source && s.name ===
r.skill.name) so the returned "rest" is taken from the correct skill.
In `@packages/cli/src/commands/serve.ts`:
- Around line 57-58: The current fallbacks use `parseInt(this.port, 10) || 3737`
and `parseInt(this.cacheTtl, 10) || 86_400_000` which treat 0 as falsy; change
to parse the values into numbers (e.g. const p = parseInt(this.port, 10); const
t = parseInt(this.cacheTtl, 10)) and then set portNum = Number.isFinite(p) ? p :
3737 and cacheTtlMs = Number.isFinite(t) ? t : 86_400_000, optionally adding
range checks (e.g. port >= 0 && port <= 65535) to validate `portNum` and
`cacheTtlMs`; update references to `portNum`, `cacheTtlMs`, `this.port`, and
`this.cacheTtl`.
In `@packages/core/src/parser/references.ts`:
- Around line 81-90: The naive line-by-line frontmatter parser (fmBlock ->
frontmatter) fails on values with colons, quoted/complex YAML, arrays, and
multiline entries; replace the manual parsing loop with a proper YAML parse
using the workspace yaml package (import { parse } from 'yaml') and set
frontmatter = parse(fmBlock) ?? {} cast to Record<string, unknown>, and remove
the loop that slices on ':'; alternatively, if you must keep the simple parser,
document the constraint near fmBlock/frontmatter and add a unit test covering
colon-containing values like "url: https://example.com".
In `@packages/core/src/runtime/injector.ts`:
- Around line 35-52: The cache key uses the raw source string so different URL
forms bypass cache; normalize the key using parseSource before caching and
lookup: in fetch(), call parseSource(source) first (or construct cacheKey from
parsed.owner and parsed.repo) and replace the existing cacheKey =
`${source}:${skillId}` with a normalized key like `${owner}/${repo}:${skillId}`
so both cache.get and cache.set use the same normalized identifier (references:
fetch, parseSource, cacheKey, cache.get, cache.set).
- Around line 77-87: The fetch loop in the function that iterates over paths
(the code using `for (const path of paths)` and the `fetch(url, { headers })`
call) should use an AbortController to enforce a timeout (e.g., 10–30s) so
network stalls don't hang the process; create an AbortController per request,
pass controller.signal to fetch, schedule a setTimeout to call
controller.abort() after the chosen timeout, and clear that timer after the
fetch completes (both on success and in the catch path) to avoid leaks; ensure
the catch properly handles AbortError and continues to the next path if aborted
or failed.
In `@packages/mcp/src/index.ts`:
- Around line 69-99: The tool schemas advertise include_references but the code
never loads or returns references; remove the misleading parameter and related
types until implemented: delete include_references from the search_skills and
get_skill tool definitions (symbols: search_skills, get_skill) in index.ts,
remove include_references properties from the SearchSkillsInputSchema and
GetSkillInputSchema in types.ts, and update any call sites or handler signatures
for handleSearchSkills and handleGetSkill so they no longer accept or reference
include_references; also remove any references in SkillEntry and from loadSkills
that relate to references so types and loading logic match the advertised
contract.
In `@packages/mcp/src/resources.ts`:
- Around line 6-24: getTrendingResource uses ranker.rank on only skill
names/descriptions then re-resolves each ranked item via skills.find, which can
return the wrong SkillEntry when names aren’t unique; change getTrendingResource
to pass the full SkillEntry objects into ranker.rank and then map the ranker
result directly (destructure the SkillEntry from the rank result instead of
calling skills.find) so you retain original source/tags reliably (refer to
getTrendingResource and ranker.rank), and apply the same refactor to the
duplicated patterns in tools.ts (handleSearchSkills, handleRecommendSkills).
In `@packages/mcp/src/tools.ts`:
- Around line 8-16: Add an optional references?: string[] field to the
SkillEntry interface and make sure any public-facing functions that accept an
include_references flag (e.g., get_skill and search / searchSkills) respect it
by omitting the references property from returned SkillEntry objects when
include_references is false; update the code paths that build/return skill
objects to conditionally include references (do not return the full object
unconditionally) to avoid leaking references.
- Around line 141-154: The function handleListCategories currently iterates
skill.tags and counts tags; change it to read and count skill.category (treating
it as a string or undefined) so the Map tallies categories not tags, e.g., use
if (skill.category) { tagCounts.set(skill.category,
(tagCounts.get(skill.category) || 0) + 1) } and rename tagCounts/related
variables to categoryCounts or similar for clarity; apply the identical change
to the analogous counting logic in resources.ts so both functions count
s.category rather than s.tags.
- Around line 38-57: The current mapping passes reduced objects to ranker.rank
and later re-finds originals by name (using filtered.find) which causes
name-collision bugs and O(n²) overhead; instead pass the full SkillEntry objects
(the elements of filtered) directly into ranker.rank, then when building results
read fields from r.skill (e.g., r.skill.name, r.skill.description,
r.skill.source, r.skill.tags, r.skill.content) and remove the filtered.find
lookup; apply the same change in the other block referenced (lines ~111–129) so
both uses of ranker.rank operate on and return full SkillEntry objects rather
than name-only shapes.
In `@pnpm-workspace.yaml`:
- Line 4: Remove the 'clients/*' entry from the pnpm workspace packages list in
pnpm-workspace.yaml because it points to a directory that only contains a Python
project (clients/python/ with pyproject.toml) and no Node.js packages; edit the
packages array to delete the 'clients/*' item so pnpm won't search for
non-existent package.json files.
🧹 Nitpick comments (10)
clients/python/tests/test_client.py (1)
29-58: Strengthen tests by asserting request params/body.The mocks only check the path, so mismatched query keys or filter payloads won’t fail tests. Consider matching params/json (or asserting route calls) to catch regressions.
✅ Example assertions
@@ - mock_api.get("/search").respond( + route = mock_api.get("/search", params={"q": "react", "limit": "20"}).respond( json={ "skills": [{"name": "react-perf", "source": "owner/repo"}], "total": 1, "query": "react", "limit": 20, } ) async with SkillKitClient(BASE_URL) as client: result = await client.search("react") assert result.total == 1 assert result.skills[0].name == "react-perf" + assert route.called @@ - mock_api.post("/search").respond( + route = mock_api.post( + "/search", + json={ + "query": "auth", + "limit": 20, + "include_content": False, + "filters": {"tags": ["nextjs"]}, + }, + ).respond( json={ "skills": [{"name": "nextjs-auth", "source": "other/repo"}], "total": 1, "query": "auth", "limit": 20, } ) async with SkillKitClient(BASE_URL) as client: result = await client.search_with_filters("auth", tags=["nextjs"]) assert result.total == 1 + assert route.calledpackages/core/src/cache/memory.ts (1)
86-100: Consider evicting expired entries before LRU.The current
evictLRUimplementation scans for the least recently accessed entry but doesn't prioritize already-expired entries. When eviction is needed, removing an expired entry (if any exist) would be more efficient than evicting a potentially still-valid LRU entry.This is a minor optimization and not blocking.
💡 Optional enhancement
private evictLRU(): void { let oldestKey: string | null = null; let oldestTime = Infinity; + const now = Date.now(); for (const [key, entry] of this.store) { + // Prioritize expired entries + if (entry.expiresAt <= now) { + this.store.delete(key); + return; + } if (entry.lastAccessed < oldestTime) { oldestTime = entry.lastAccessed; oldestKey = key; } } if (oldestKey) { this.store.delete(oldestKey); } }packages/core/src/parser/__tests__/references.test.ts (2)
48-60: Consider adding test coverage forparseSkillMdwith askillDirparameter.The current test only covers parsing without a
skillDir. Adding a test case that provides askillDirwould exercise thediscoverReferencesintegration path and ensure full coverage of the function's branching logic.
67-87: Test only validates theexamplesdirectory; other reference directories are untested.Per the implementation in
references.ts,discoverReferencesscans six reference directories (REFERENCE_DIRS). This test only mocks theexamplespath. Consider adding test cases for other directories (e.g.,prompts,templates) to ensure complete coverage.packages/core/src/runtime/injector.ts (1)
84-86: Empty catch block silently swallows all errors.The empty catch block masks potentially important errors (e.g., network failures, DNS issues, rate limiting). Consider logging or differentiating between recoverable errors (404) and unexpected failures.
packages/api/src/routes/health.ts (2)
13-13: Hardcoded version string will drift from actual package version.The version
'1.11.0'is hardcoded and will require manual updates on each release, risking inconsistency with the actual package version. Consider reading the version frompackage.jsonor injecting it as a parameter.
5-5: Module-levelstartTimemay not reflect actual server start time.The
startTimeis captured when the module is first loaded, which may occur before the server actually starts listening. For accurate uptime reporting, consider passing the start timestamp as a parameter tohealthRoutes().packages/mcp/src/resources.ts (1)
29-44: Consider sharing category aggregation with tools to prevent drift.
getCategoriesResourceduplicates the same tag-count logic inhandleListCategories. A small shared helper would reduce future divergence.packages/mcp/src/types.ts (1)
3-30: Tighten Zod constraints for query and limits.Consider
.trim().min(1)forqueryand.int()for numeric limits to avoid empty/float inputs slipping through.♻️ Suggested tweak
- query: z.string().describe('Search query for skills'), - limit: z.number().min(1).max(50).default(10).describe('Maximum results'), + query: z.string().trim().min(1).describe('Search query for skills'), + limit: z.number().int().min(1).max(50).default(10).describe('Maximum results'), @@ - limit: z.number().min(1).max(20).default(5).describe('Maximum results'), + limit: z.number().int().min(1).max(20).default(5).describe('Maximum results'),packages/core/src/ranking/relevance.ts (1)
62-79: Guard against whitespace-only queries in scoring.Trimming the query and returning 0 for empty strings avoids giving queryMatch points for empty input.
♻️ Suggested tweak
- const q = query.toLowerCase(); + const q = query.trim().toLowerCase(); + if (!q) return 0; const name = skill.name.toLowerCase(); @@ - const words = q.split(/\s+/); + const words = q.split(/\s+/);
| [project.urls] | ||
| Homepage = "https://github.com/rohitg00/skillkit" | ||
| Documentation = "https://agenstskills.com" | ||
| Repository = "https://github.com/rohitg00/skillkit" |
There was a problem hiding this comment.
Documentation URL looks misspelled.
The "Documentation" URL uses agenstskills.com; please confirm the intended domain so package metadata doesn’t point to a broken link.
🤖 Prompt for AI Agents
In `@clients/python/pyproject.toml` around lines 39 - 42, The Documentation URL
under the [project.urls] table currently points to "agenstskills.com" which
appears misspelled; update the Documentation key in pyproject.toml (under
[project.urls]) to the correct domain (e.g., "agentskills.com" or the intended
documentation URL) so the Repository/Documentation metadata are accurate and not
broken.
| __all__ = [ | ||
| "SkillKitClient", | ||
| "Skill", | ||
| "SearchResponse", | ||
| "HealthResponse", | ||
| "CacheStats", | ||
| "Category", | ||
| ] |
There was a problem hiding this comment.
Sort __all__ to satisfy Ruff (RUF022).
This keeps linting clean and avoids CI warnings.
🔧 Proposed fix
__all__ = [
- "SkillKitClient",
- "Skill",
- "SearchResponse",
- "HealthResponse",
- "CacheStats",
- "Category",
+ "CacheStats",
+ "Category",
+ "HealthResponse",
+ "SearchResponse",
+ "Skill",
+ "SkillKitClient",
]📝 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.
| __all__ = [ | |
| "SkillKitClient", | |
| "Skill", | |
| "SearchResponse", | |
| "HealthResponse", | |
| "CacheStats", | |
| "Category", | |
| ] | |
| __all__ = [ | |
| "CacheStats", | |
| "Category", | |
| "HealthResponse", | |
| "SearchResponse", | |
| "Skill", | |
| "SkillKitClient", | |
| ] |
🧰 Tools
🪛 Ruff (0.14.14)
[warning] 4-11: __all__ is not sorted
Apply an isort-style sorting to __all__
(RUF022)
🤖 Prompt for AI Agents
In `@clients/python/skillkit/__init__.py` around lines 4 - 11, The __all__ list in
clients/python/skillkit/__init__.py is unsorted and triggers Ruff RUF022;
reorder the __all__ entries alphabetically (e.g., CacheStats, Category,
HealthResponse, SearchResponse, Skill, SkillKitClient) so the module-level
__all__ is sorted, then run the linter to confirm the warning is resolved.
| async def __aenter__(self) -> "SkillKitClient": | ||
| self._client = httpx.AsyncClient(base_url=self.base_url, timeout=self.timeout) | ||
| return self | ||
|
|
||
| async def __aexit__(self, *args: object) -> None: | ||
| if self._client: | ||
| await self._client.aclose() | ||
| self._client = None | ||
|
|
||
| def _get_client(self) -> httpx.AsyncClient: | ||
| if self._client is None: | ||
| self._client = httpx.AsyncClient(base_url=self.base_url, timeout=self.timeout) | ||
| return self._client |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n clients/python/skillkit/client.py | head -100Repository: rohitg00/skillkit
Length of output: 3849
🏁 Script executed:
find . -name "*.py" -type f | xargs grep -l "SkillKitClient" | head -10Repository: rohitg00/skillkit
Length of output: 172
🏁 Script executed:
rg "SkillKitClient" -A 5 -B 5 --type py | head -200Repository: rohitg00/skillkit
Length of output: 7146
Prevent resource leak when reusing client after direct method calls.
__aenter__ unconditionally creates a new AsyncClient without checking if one already exists. This causes a leak if _get_client() was called first (e.g., await client.health()), then the context manager is entered—the original client is overwritten without being closed.
Reuse the existing client instead by checking if self._client is None: before creating a new one, mirroring the logic in _get_client().
Proposed fix
async def __aenter__(self) -> "SkillKitClient":
- self._client = httpx.AsyncClient(base_url=self.base_url, timeout=self.timeout)
+ if self._client is None:
+ self._client = httpx.AsyncClient(base_url=self.base_url, timeout=self.timeout)
return self🤖 Prompt for AI Agents
In `@clients/python/skillkit/client.py` around lines 24 - 36, The __aenter__
method currently unconditionally creates a new httpx.AsyncClient and can
overwrite an existing client created by _get_client, leaking the original;
change __aenter__ to only create a new AsyncClient if self._client is None
(mirror _get_client's logic) so the existing client is reused, while leaving
__aexit__'s close behavior (await self._client.aclose(); self._client = None)
intact to properly release resources.
| return async (c: Context, next: Next) => { | ||
| const ip = c.req.header('x-forwarded-for') || c.req.header('x-real-ip') || 'unknown'; | ||
| const now = Date.now(); | ||
|
|
||
| let entry = windows.get(ip); | ||
| if (!entry || now > entry.resetAt) { | ||
| entry = { count: 0, resetAt: now + windowMs }; | ||
| windows.set(ip, entry); | ||
| } | ||
|
|
||
| entry.count++; | ||
|
|
||
| c.header('X-RateLimit-Limit', String(maxRequests)); | ||
| c.header('X-RateLimit-Remaining', String(Math.max(0, maxRequests - entry.count))); | ||
| c.header('X-RateLimit-Reset', String(Math.ceil(entry.resetAt / 1000))); | ||
|
|
||
| if (entry.count > maxRequests) { | ||
| return c.json({ error: 'Too many requests', retryAfter: Math.ceil((entry.resetAt - now) / 1000) }, 429); | ||
| } | ||
|
|
||
| await next(); | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check TS config for noImplicitReturns
find . -name 'tsconfig*.json' -type f | head -5
rg "noImplicitReturns" -g 'tsconfig*.json'Repository: rohitg00/skillkit
Length of output: 297
🏁 Script executed:
#!/bin/bash
# Examine the actual rate-limit.ts file to confirm the code
cat -n packages/api/src/middleware/rate-limit.tsRepository: rohitg00/skillkit
Length of output: 1526
🏁 Script executed:
#!/bin/bash
# Look for how next() is used elsewhere to understand return type expectations
rg -n "await next\(\)" packages/api --type=ts -A 2 -B 2 | head -40Repository: rohitg00/skillkit
Length of output: 317
Return the result of next() to satisfy noImplicitReturns.
With noImplicitReturns: true enabled, the middleware handler must return a value on all code paths. Currently, the 429 branch returns via c.json(...) but the allow-request path calls await next() without returning. In Hono, middleware must return the result of next() to propagate responses through the chain.
Fix
- await next();
+ return await next();📝 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.
| return async (c: Context, next: Next) => { | |
| const ip = c.req.header('x-forwarded-for') || c.req.header('x-real-ip') || 'unknown'; | |
| const now = Date.now(); | |
| let entry = windows.get(ip); | |
| if (!entry || now > entry.resetAt) { | |
| entry = { count: 0, resetAt: now + windowMs }; | |
| windows.set(ip, entry); | |
| } | |
| entry.count++; | |
| c.header('X-RateLimit-Limit', String(maxRequests)); | |
| c.header('X-RateLimit-Remaining', String(Math.max(0, maxRequests - entry.count))); | |
| c.header('X-RateLimit-Reset', String(Math.ceil(entry.resetAt / 1000))); | |
| if (entry.count > maxRequests) { | |
| return c.json({ error: 'Too many requests', retryAfter: Math.ceil((entry.resetAt - now) / 1000) }, 429); | |
| } | |
| await next(); | |
| }; | |
| return async (c: Context, next: Next) => { | |
| const ip = c.req.header('x-forwarded-for') || c.req.header('x-real-ip') || 'unknown'; | |
| const now = Date.now(); | |
| let entry = windows.get(ip); | |
| if (!entry || now > entry.resetAt) { | |
| entry = { count: 0, resetAt: now + windowMs }; | |
| windows.set(ip, entry); | |
| } | |
| entry.count++; | |
| c.header('X-RateLimit-Limit', String(maxRequests)); | |
| c.header('X-RateLimit-Remaining', String(Math.max(0, maxRequests - entry.count))); | |
| c.header('X-RateLimit-Reset', String(Math.ceil(entry.resetAt / 1000))); | |
| if (entry.count > maxRequests) { | |
| return c.json({ error: 'Too many requests', retryAfter: Math.ceil((entry.resetAt - now) / 1000) }, 429); | |
| } | |
| return await next(); | |
| }; |
🧰 Tools
🪛 GitHub Actions: CI
[error] 20-20: TypeScript error: Not all code paths return a value. (tsc --noEmit)
🤖 Prompt for AI Agents
In `@packages/api/src/middleware/rate-limit.ts` around lines 20 - 41, The
middleware handler currently awaits next() but does not return its result, which
violates noImplicitReturns and prevents proper propagation; update the async
handler returned by the top-level function (the anonymous async (c: Context,
next: Next) => { ... } that uses windows, windowMs, maxRequests) to return the
next() call (e.g., replace the final await next() with return await next() or
return next()) so both the 429 branch and the allowed-request path always return
a value.
| for (const skill of skills) { | ||
| if (skill.tags) { | ||
| for (const tag of skill.tags) { | ||
| tagCounts.set(tag, (tagCounts.get(tag) || 0) + 1); | ||
| } | ||
| } | ||
| if (skill.category) { | ||
| const key = `category:${skill.category}`; | ||
| tagCounts.set(key, (tagCounts.get(key) || 0) + 1); | ||
| } | ||
| } |
There was a problem hiding this comment.
Endpoint mixes tags and categories with a category: prefix leaking into response.
The implementation aggregates both skill.tags and skill.category into a single map, with categories prefixed by "category:". This prefix will appear in API responses (e.g., "category:automation"), which:
- Mixes two conceptually different things (tags vs categories)
- Leaks internal naming convention to consumers
- May confuse clients expecting only category names
Consider separating these or clarifying the API contract.
🤖 Prompt for AI Agents
In `@packages/api/src/routes/categories.ts` around lines 10 - 20, The loop in
categories.ts currently mixes skill.tags and skill.category into one map
(tagCounts) and prefixes categories with "category:", which leaks internal
format; change the aggregation to maintain two separate collections (e.g.,
tagCounts for skill.tags and categoryCounts for skill.category) while iterating
over skills (referencing skill.tags, skill.category and tagCounts), increment
counts into the appropriate map without adding a "category:" prefix, and adjust
the endpoint response shape to return tags and categories as distinct fields so
clients receive raw category names rather than prefixed values.
| export interface SkillEntry { | ||
| name: string; | ||
| description?: string; | ||
| source: string; | ||
| repo?: string; | ||
| tags?: string[]; | ||
| category?: string; | ||
| content?: string; | ||
| } |
There was a problem hiding this comment.
include_references is ignored (and potentially leaked).
Search never returns references even when requested, and get_skill returns the full object regardless of the flag. Add references? to SkillEntry and gate output on include_references.
🧩 Suggested fix
export interface SkillEntry {
name: string;
description?: string;
source: string;
repo?: string;
tags?: string[];
category?: string;
content?: string;
+ references?: string[];
}
@@
- if (input.include_content) result.content = r.skill.content;
+ if (input.include_content) result.content = r.skill.content;
+ if (input.include_references) result.references = r.skill.references;
@@
- return {
- content: [
- {
- type: 'text' as const,
- text: JSON.stringify(skill, null, 2),
- },
- ],
- };
+ const payload: Record<string, unknown> = { ...skill };
+ if (!input.include_references) delete payload.references;
+ return {
+ content: [
+ {
+ type: 'text' as const,
+ text: JSON.stringify(payload, null, 2),
+ },
+ ],
+ };Also applies to: 56-57, 70-90
🤖 Prompt for AI Agents
In `@packages/mcp/src/tools.ts` around lines 8 - 16, Add an optional references?:
string[] field to the SkillEntry interface and make sure any public-facing
functions that accept an include_references flag (e.g., get_skill and search /
searchSkills) respect it by omitting the references property from returned
SkillEntry objects when include_references is false; update the code paths that
build/return skill objects to conditionally include references (do not return
the full object unconditionally) to avoid leaking references.
| const ranked = ranker.rank( | ||
| filtered.map((s) => ({ | ||
| name: s.name, | ||
| description: s.description, | ||
| content: s.content, | ||
| })), | ||
| input.query, | ||
| ); | ||
|
|
||
| const results = ranked.slice(0, input.limit).map((r) => { | ||
| const original = filtered.find((s) => s.name === r.skill.name)!; | ||
| const result: Record<string, unknown> = { | ||
| name: original.name, | ||
| description: original.description, | ||
| source: original.source, | ||
| tags: original.tags, | ||
| score: r.score, | ||
| }; | ||
| if (input.include_content) result.content = original.content; | ||
| return result; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "tools.ts" -path "*/mcp/*" | head -5Repository: rohitg00/skillkit
Length of output: 88
🏁 Script executed:
cat -n packages/mcp/src/tools.ts | head -150Repository: rohitg00/skillkit
Length of output: 4918
🏁 Script executed:
rg "RelevanceRanker\|ranker\.rank" packages/mcp/src/ -A 2 -B 2Repository: rohitg00/skillkit
Length of output: 43
🏁 Script executed:
fd "ranker\|Ranker" packages/mcp/src --type fRepository: rohitg00/skillkit
Length of output: 43
🏁 Script executed:
rg "class RelevanceRanker\|export.*RelevanceRanker" --type ts -A 10Repository: rohitg00/skillkit
Length of output: 43
🏁 Script executed:
fd -e ts -e js . | xargs grep -l "RelevanceRanker" | head -10Repository: rohitg00/skillkit
Length of output: 279
🏁 Script executed:
find . -path "*/core/*" -name "*.ts" | head -20Repository: rohitg00/skillkit
Length of output: 919
🏁 Script executed:
find packages/core -type f -name "*.ts" 2>/dev/null | head -20Repository: rohitg00/skillkit
Length of output: 879
🏁 Script executed:
cat -n packages/core/src/ranking/relevance.tsRepository: rohitg00/skillkit
Length of output: 3396
🏁 Script executed:
cat -n packages/core/src/ranking/__tests__/relevance.test.tsRepository: rohitg00/skillkit
Length of output: 3728
🏁 Script executed:
cat -n packages/mcp/src/tools.ts | head -20Repository: rohitg00/skillkit
Length of output: 642
Fix name-based lookups that can cause collisions and O(n²) overhead.
The code maps SkillEntry objects to reduced shapes for ranking, then re-finds originals by name only. With multiple skills sharing the same name across sources, find() returns the first match, not the correct entry. Additionally, this adds O(n²) lookups. Since SkillEntry satisfies RankableSkill requirements, pass full objects directly to ranker.rank() and access fields from r.skill:
Suggested fix
- const ranked = ranker.rank(
- filtered.map((s) => ({
- name: s.name,
- description: s.description,
- content: s.content,
- })),
- input.query,
- );
+ const ranked = ranker.rank(filtered, input.query);
@@
- const results = ranked.slice(0, input.limit).map((r) => {
- const original = filtered.find((s) => s.name === r.skill.name)!;
- const result: Record<string, unknown> = {
- name: original.name,
- description: original.description,
- source: original.source,
- tags: original.tags,
- score: r.score,
- };
- if (input.include_content) result.content = original.content;
- return result;
- });
+ const results = ranked.slice(0, input.limit).map((r) => {
+ const result: Record<string, unknown> = {
+ name: r.skill.name,
+ description: r.skill.description,
+ source: r.skill.source,
+ tags: r.skill.tags,
+ score: r.score,
+ };
+ if (input.include_content) result.content = r.skill.content;
+ return result;
+ });Also applies to lines 111–129.
🤖 Prompt for AI Agents
In `@packages/mcp/src/tools.ts` around lines 38 - 57, The current mapping passes
reduced objects to ranker.rank and later re-finds originals by name (using
filtered.find) which causes name-collision bugs and O(n²) overhead; instead pass
the full SkillEntry objects (the elements of filtered) directly into
ranker.rank, then when building results read fields from r.skill (e.g.,
r.skill.name, r.skill.description, r.skill.source, r.skill.tags,
r.skill.content) and remove the filtered.find lookup; apply the same change in
the other block referenced (lines ~111–129) so both uses of ranker.rank operate
on and return full SkillEntry objects rather than name-only shapes.
| export function handleListCategories(skills: SkillEntry[]) { | ||
| const tagCounts = new Map<string, number>(); | ||
|
|
||
| for (const skill of skills) { | ||
| if (skill.tags) { | ||
| for (const tag of skill.tags) { | ||
| tagCounts.set(tag, (tagCounts.get(tag) || 0) + 1); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| const categories = Array.from(tagCounts.entries()) | ||
| .map(([name, count]) => ({ name, count })) | ||
| .sort((a, b) => b.count - a.count); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the SkillEntry type definition
rg -n "interface SkillEntry|type SkillEntry" packages/mcp/src -A 10Repository: rohitg00/skillkit
Length of output: 583
🏁 Script executed:
#!/bin/bash
# Check how handleListCategories is used
rg -n "handleListCategories" packages/mcp/srcRepository: rohitg00/skillkit
Length of output: 528
🏁 Script executed:
#!/bin/bash
# Look for search filter logic
rg -n "category|categories" packages/mcp/src -B 2 -A 2Repository: rohitg00/skillkit
Length of output: 5305
🏁 Script executed:
#!/bin/bash
# Check the skill data structure
fd -n "skills\|skill" -e json | head -5Repository: rohitg00/skillkit
Length of output: 291
🏁 Script executed:
#!/bin/bash
# View the exact implementation of handleListCategories
sed -n '141,154p' packages/mcp/src/tools.tsRepository: rohitg00/skillkit
Length of output: 479
🏁 Script executed:
#!/bin/bash
# Check resources.ts for similar logic
sed -n '30,45p' packages/mcp/src/resources.tsRepository: rohitg00/skillkit
Length of output: 497
🏁 Script executed:
#!/bin/bash
# Check if there are other places counting categories from tags
rg -n "tagCounts\|categoryCounts" packages/mcp/srcRepository: rohitg00/skillkit
Length of output: 43
Fix handleListCategories to count category field instead of tags.
The function counts skill.tags but should count skill.category to align with the search filter (line 30-31) which filters by s.category. This mismatch causes the function to return tag counts instead of category counts. The same issue exists in packages/mcp/src/resources.ts (lines 30-45).
Suggested fix for tools.ts
- const tagCounts = new Map<string, number>();
+ const categoryCounts = new Map<string, number>();
for (const skill of skills) {
- if (skill.tags) {
- for (const tag of skill.tags) {
- tagCounts.set(tag, (tagCounts.get(tag) || 0) + 1);
- }
- }
+ if (skill.category) {
+ categoryCounts.set(skill.category, (categoryCounts.get(skill.category) || 0) + 1);
+ }
}
- const categories = Array.from(tagCounts.entries())
+ const categories = Array.from(categoryCounts.entries())
.map(([name, count]) => ({ name, count }))
.sort((a, b) => b.count - a.count);Apply the same fix to resources.ts.
🤖 Prompt for AI Agents
In `@packages/mcp/src/tools.ts` around lines 141 - 154, The function
handleListCategories currently iterates skill.tags and counts tags; change it to
read and count skill.category (treating it as a string or undefined) so the Map
tallies categories not tags, e.g., use if (skill.category) {
tagCounts.set(skill.category, (tagCounts.get(skill.category) || 0) + 1) } and
rename tagCounts/related variables to categoryCounts or similar for clarity;
apply the identical change to the analogous counting logic in resources.ts so
both functions count s.category rather than s.tags.
| packages: | ||
| - 'packages/*' | ||
| - 'apps/*' | ||
| - 'clients/*' |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd -t f '^package\.json$' clients/Repository: rohitg00/skillkit
Length of output: 43
🏁 Script executed:
fd -t d '^clients$' . && echo "=== Clients directory exists ===" && ls -la clients/ || echo "=== Clients directory not found ==="Repository: rohitg00/skillkit
Length of output: 271
🏁 Script executed:
find clients/ -type f -name "*.json" | head -20 && echo "=== Directory structure ===" && ls -la clients/python/Repository: rohitg00/skillkit
Length of output: 376
Remove 'clients/*' from the workspace configuration.
The clients/ directory contains only a Python project (clients/python/ with pyproject.toml). pnpm workspaces manage Node.js packages with package.json files. Since there are no Node.js packages in clients/, this workspace entry will not discover any packages and should be removed from pnpm-workspace.yaml.
🤖 Prompt for AI Agents
In `@pnpm-workspace.yaml` at line 4, Remove the 'clients/*' entry from the pnpm
workspace packages list in pnpm-workspace.yaml because it points to a directory
that only contains a Python project (clients/python/ with pyproject.toml) and no
Node.js packages; edit the packages array to delete the 'clients/*' item so pnpm
won't search for non-existent package.json files.
- Fix version-check: bump docs packages to 1.11.0 - Parse first IP from X-Forwarded-For header in rate limiter - Add try/catch for malformed JSON in POST /search - Use exact source match in skill routes (owner/repo/id pattern) - Fix NaN fallback for invalid limit params in trending/search - Filter empty strings from query words in relevance ranker - Use Map lookups instead of O(n^2) find in MCP resources/tools - URL-encode path params in Python client - Fix -h flag conflict with Clipanion help in serve command - Fix typo: remoti-dev -> remotion-dev in registry SKILLS.md
- Use source+name composite key Map lookups in search GET/POST routes - Use source+name composite key Map lookup in trending route - Fix Python client get_skill to use /skills/:owner/:repo/:id URL pattern - Update Python test to match new 3-segment skill URL
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| const recommendMap = new Map(skills.map((s) => [s.name, s])); | ||
| const results = ranked.slice(0, input.limit).map((r) => { | ||
| const original = recommendMap.get(r.skill.name)!; |
There was a problem hiding this comment.
🟡 Potential null dereference in handleRecommendSkills when skills have duplicate names
In handleRecommendSkills, the code creates a map keyed only by skill name (skills.map((s) => [s.name, s])), but skills can have the same name from different sources. When the ranker returns a skill, the lookup uses only the name, which may return a different skill than the one that was ranked.
Click to expand
Issue Details
At packages/mcp/src/tools.ts:124, the map is created with just the skill name as key:
const recommendMap = new Map(skills.map((s) => [s.name, s]));However, the ranker at line 115-122 receives skills that include a source property, but the lookup at line 126 only uses r.skill.name:
const original = recommendMap.get(r.skill.name)!;If two skills have the same name but different sources, the map will only contain the last one (due to Map overwriting), and the lookup may return the wrong skill or potentially undefined (though the ! assertion assumes it exists).
This is inconsistent with handleSearchSkills which correctly uses ${s.source}:${s.name} as the key.
Impact
Wrong skill metadata could be returned in recommendations, or if the skill name doesn't exist in the map (edge case with filtering), it could cause a runtime error.
Recommendation: Use a composite key like ${s.source}:${s.name} for the map, similar to how handleSearchSkills does it, and include source in the ranked skill lookup.
Was this helpful? React with 👍 or 👎 to provide feedback.
| const skillMap = new Map(skills.map((s) => [s.name, s])); | ||
|
|
||
| const ranked = ranker.rank( | ||
| skills.map((s) => ({ | ||
| name: s.name, | ||
| description: s.description, | ||
| content: s.content, | ||
| })), | ||
| ); | ||
|
|
||
| const trending = ranked.slice(0, 20).map((r) => { | ||
| const original = skillMap.get(r.skill.name); |
There was a problem hiding this comment.
🟡 Potential null dereference in getTrendingResource when skills have duplicate names
In getTrendingResource, the code creates a map keyed only by skill name, but skills can have the same name from different sources. This can cause incorrect skill data to be returned.
Click to expand
Issue Details
At packages/mcp/src/resources.ts:7, the map is created with just the skill name as key:
const skillMap = new Map(skills.map((s) => [s.name, s]));The lookup at line 18 uses only r.skill.name:
const original = skillMap.get(r.skill.name);If two skills have the same name but different sources, the map will only contain the last one, and the lookup may return the wrong skill's metadata (description, source, tags).
This is inconsistent with the API routes which correctly use ${s.source}:${s.name} as the key.
Impact
Wrong skill metadata could be returned in the trending resource, potentially showing incorrect source/tags for a skill.
Recommendation: Use a composite key like ${s.source}:${s.name} for the map, and include source in the ranked skill data passed to the ranker so it can be used for lookup.
Was this helpful? React with 👍 or 👎 to provide feedback.
| setInterval(() => { | ||
| const now = Date.now(); | ||
| for (const [key, entry] of windows) { | ||
| if (now > entry.resetAt) { | ||
| windows.delete(key); | ||
| } | ||
| } | ||
| }, windowMs).unref(); |
There was a problem hiding this comment.
🟡 Memory leak from uncleared interval in rate limiter
The rate limiter creates a setInterval that runs indefinitely. While .unref() is called to prevent it from keeping the process alive, the interval and its closure (holding the windows Map) are never cleared, causing a memory leak if rateLimiter() is called multiple times.
Click to expand
Issue Details
At packages/api/src/middleware/rate-limit.ts:11-18:
setInterval(() => {
const now = Date.now();
for (const [key, entry] of windows) {
if (now > entry.resetAt) {
windows.delete(key);
}
}
}, windowMs).unref();Each call to rateLimiter() creates a new interval that is never cleared. In testing scenarios or if the server is restarted programmatically, this could lead to multiple intervals running and holding references to old windows Maps.
Impact
In typical production use (single server instance), this is not a significant issue. However, in testing or when creating multiple app instances, this could cause memory leaks.
Recommendation: Return a cleanup function or store the interval reference so it can be cleared when the server shuts down. Alternatively, document that rateLimiter() should only be called once per application lifecycle.
Was this helpful? React with 👍 or 👎 to provide feedback.
| def _get_client(self) -> httpx.AsyncClient: | ||
| if self._client is None: | ||
| self._client = httpx.AsyncClient(base_url=self.base_url, timeout=self.timeout) | ||
| return self._client |
There was a problem hiding this comment.
🟡 Python client creates unclosed httpx.AsyncClient when used without context manager
The Python client's _get_client() method creates an AsyncClient that is never closed if the client is used without the async context manager (async with), leading to resource leaks.
Click to expand
Issue Details
At clients/python/skillkit/client.py:34-37:
def _get_client(self) -> httpx.AsyncClient:
if self._client is None:
self._client = httpx.AsyncClient(base_url=self.base_url, timeout=self.timeout)
return self._clientIf a user creates a SkillKitClient and calls methods directly without using async with, the client is created lazily but never closed:
client = SkillKitClient()
await client.health() # Creates client internally
# Client is never closed - resource leakWhile there is a close() method at line 137-140, users may not know to call it.
Impact
Resource leak (unclosed HTTP connections) when the client is used without the context manager pattern.
Recommendation: Add a __del__ method that warns if the client wasn't closed, or document clearly that users must either use async with or call close() explicitly. Alternatively, consider using a synchronous client or requiring context manager usage.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@clients/python/skillkit/client.py`:
- Around line 101-106: The get_skill method currently accepts a single source
string and silently treats a value without a "/" as owner==repo, which can
misroute requests; update async def get_skill(self, source: str, skill_id: str)
to validate source contains exactly an "owner/repo" form (e.g., check
source.split("/", 1) or "if '/' not in source") and if missing raise a
ValueError with a clear message, then proceed to quote parts and call client.get
as before; reference the get_skill function, the source variable, and owner/repo
split logic to locate where to add the validation and error raise.
In `@packages/mcp/src/tools.ts`:
- Around line 33-35: The source filter currently uses a partial match
(s.source.includes(input.filters!.source!)); change it to an exact equality
check (s.source === input.filters!.source) to enforce exact source matching
consistent with the category filter; update the filter expression in tools.ts
where input.filters.source is used (the filtered.filter callback) and ensure you
still guard for input.filters and input.filters.source before applying the
filter.
🧹 Nitpick comments (4)
packages/api/src/middleware/rate-limit.ts (1)
11-18: Guardunref()for non‑Node runtimes.
unref()doesn’t exist in some Hono runtimes (e.g., Workers), so this can throw. Use a safe guard to keep the middleware portable.♻️ Proposed fix
- setInterval(() => { + const cleanupTimer = setInterval(() => { const now = Date.now(); for (const [key, entry] of windows) { if (now > entry.resetAt) { windows.delete(key); } } - }, windowMs).unref(); + }, windowMs); + cleanupTimer.unref?.();packages/cli/src/commands/serve.ts (2)
46-53: Consider validating required fields during normalization.The
namefield is cast without validation. If any skill in the JSON lacks aname, it will silently becomeundefined, which could cause issues downstream in search/display logic.💡 Optional: Add filtering for valid skills
- const skills = (skillsData.skills || []).map((skill: Record<string, unknown>) => ({ - name: skill.name as string, - description: skill.description as string | undefined, - source: (skill.source as string) || '', - repo: skill.repo as string | undefined, - tags: skill.tags as string[] | undefined, - category: skill.category as string | undefined, - })); + const skills = (skillsData.skills || []) + .filter((skill: Record<string, unknown>) => typeof skill.name === 'string' && skill.name) + .map((skill: Record<string, unknown>) => ({ + name: skill.name as string, + description: skill.description as string | undefined, + source: (skill.source as string) || '', + repo: skill.repo as string | undefined, + tags: skill.tags as string[] | undefined, + category: skill.category as string | undefined, + }));
84-92: Unreachablereturn 0statement.The
await new Promise(() => {})on line 84 never resolves, making thereturn 0on line 91 dead code. While this doesn't cause runtime issues (the server runs until terminated), it's misleading.💡 Optional: Remove unreachable code or add comment
Option 1 — Remove unreachable return:
console.log(colors.muted('Press Ctrl+C to stop')); await new Promise(() => {}); + return 0; // Never reached; server runs until SIGINT } catch (err) { const message = err instanceof Error ? err.message : String(err); console.error(colors.error(`Failed to start server: ${message}`)); return 1; } - - return 0; }Option 2 — Move return inside try block with explanatory comment (shown above).
packages/api/src/__tests__/skills.test.ts (1)
11-12: Align test description with the actual route shape.The route is
/skills/:owner/:repo/:id, but the test name says/skills/:source/:id. This can mislead future readers.💡 Proposed tweak
-it('GET /skills/:source/:id returns a skill', async () => { +it('GET /skills/:owner/:repo/:id returns a skill', async () => {
| async def get_skill(self, source: str, skill_id: str) -> Skill: | ||
| client = self._get_client() | ||
| parts = source.split("/", 1) | ||
| owner = quote(parts[0], safe="") | ||
| repo = quote(parts[1], safe="") if len(parts) > 1 else owner | ||
| response = await client.get(f"/skills/{owner}/{repo}/{quote(skill_id, safe='')}") |
There was a problem hiding this comment.
Reject non-owner/repo inputs to avoid accidental misrouting.
When source has no /, the fallback repo = owner silently forms /skills/{owner}/{owner}/..., which is likely unintended and hides caller mistakes. Consider enforcing an explicit owner/repo format (or split parameters) and raise a ValueError otherwise.
Suggested fix
- parts = source.split("/", 1)
- owner = quote(parts[0], safe="")
- repo = quote(parts[1], safe="") if len(parts) > 1 else owner
+ parts = source.split("/", 1)
+ if len(parts) != 2:
+ raise ValueError("source must be in the form 'owner/repo'")
+ owner = quote(parts[0], safe="")
+ repo = quote(parts[1], safe="")🤖 Prompt for AI Agents
In `@clients/python/skillkit/client.py` around lines 101 - 106, The get_skill
method currently accepts a single source string and silently treats a value
without a "/" as owner==repo, which can misroute requests; update async def
get_skill(self, source: str, skill_id: str) to validate source contains exactly
an "owner/repo" form (e.g., check source.split("/", 1) or "if '/' not in
source") and if missing raise a ValueError with a clear message, then proceed to
quote parts and call client.get as before; reference the get_skill function, the
source variable, and owner/repo split logic to locate where to add the
validation and error raise.
| if (input.filters.source) { | ||
| filtered = filtered.filter((s) => s.source.includes(input.filters!.source!)); | ||
| } |
There was a problem hiding this comment.
Source filter uses partial match instead of exact match.
The category filter uses exact equality (===) but the source filter uses includes(), which is inconsistent and can cause false positives (e.g., filtering by "github" would also match "github-enterprise"). The PR objectives state "enforce exact source/owner/repo/id matching".
🔧 Suggested fix
if (input.filters.source) {
- filtered = filtered.filter((s) => s.source.includes(input.filters!.source!));
+ filtered = filtered.filter((s) => s.source === input.filters!.source);
}🤖 Prompt for AI Agents
In `@packages/mcp/src/tools.ts` around lines 33 - 35, The source filter currently
uses a partial match (s.source.includes(input.filters!.source!)); change it to
an exact equality check (s.source === input.filters!.source) to enforce exact
source matching consistent with the category filter; update the filter
expression in tools.ts where input.filters.source is used (the filtered.filter
callback) and ensure you still guard for input.filters and input.filters.source
before applying the filter.
Summary
Adds runtime skill discovery capabilities enabling agents to autonomously search and retrieve skills on demand — similar to how skyll.app approaches agent skill discovery by making skills queryable by intent rather than requiring pre-installation.
New Packages
@skillkit/api— Hono-based REST API server (port 3737) with ranked search, trending, categories, skill lookup, rate limiting, and response caching@skillkit/mcp— MCP server with 4 tools (search_skills,get_skill,recommend_skills,list_categories) + 2 resources (skills://trending,skills://categories) — works with Claude Desktop, Cursor, and any MCP clientNew Core Modules
cache/— PluggableCacheBackend<V>interface withMemoryCache(LRU + TTL, default 1000 entries / 24h)ranking/— Multi-signalRelevanceRankerscoring 0-100: content availability (40pts), query match (30pts), popularity (15pts), references (15pts)parser/— Enhanced SKILL.md parser:stripFrontmatter(),parseSkillMd(),discoverReferences()scanning 6 reference directoriesruntime/—SkillInjectorclass: fetches SKILL.md from GitHub (tries 3 paths), parses, caches,inject()returns body without frontmatterCLI
skillkit serve/skillkit server— starts the REST API with--port,--host,--cors,--cache-ttloptionsCommunity Registry
registry/SKILLS.md— 26 curated skills across 12 categoriesCommunityRegistryclass integrated intoFederatedSearchPython Client
clients/python/—skillkit-clientpackage (httpx + pydantic):search(),get_skill(),trending(),categories(),cache_stats(),health()MCP Usage
{ "mcpServers": { "skillkit": { "command": "npx", "args": ["@skillkit/mcp"] } } }Test plan
pnpm build— all 12 packages build (+ 2 new: api, mcp)pnpm test— all 24 workspace tasks pass (856+ tests)skillkit serve→curl http://localhost:3737/healthcurl "http://localhost:3737/search?q=react"Summary by CodeRabbit
New Features
Infrastructure
Documentation