skills.sh implementation

[cole-ramer]

PR: Skills.sh Catalog Integration

Branch: feak/skills.sh → staging

Summary

Integrates the full skills.sh catalog (~50,000 skills) into the platform. Users can browse, search, and add skills to their harnesses. Selected skills are passed to the LLM during chat as a manifest, and a custom get_skill_content tool lets the model retrieve full skill markdown on demand.

What Changed

New: Skills Catalog Proxy (FastAPI)

• packages/fastapi/app/routes/skills.py — Three new endpoints (/api/skills, /api/skills/search, /api/skills/detail) that proxy the HuggingFace Dataset Server API. List and search responses strip the detail field to keep payloads small for the browser.
• Registered in main.py under /api/skills.

New: Convex Skills Backend

• packages/convex-backend/convex/skills.ts — Queries (getByName, getByNames) and an ensureSkillDetails action that lazily fetches and stores full skill markdown from HuggingFace into a new skillDetails table. Skill detail is only persisted when a harness containing that skill is saved, avoiding unnecessary storage.
• packages/convex-backend/convex/schema.ts — Added skillDetails table (name, skillName, description, detail, code) with a by_name index. Changed harnesses.skills from v.array(v.string()) to v.array(v.object({ name, description })) so descriptions travel with the harness and don't require extra queries at chat time.

Updated: Harness Mutations

• packages/convex-backend/convex/harnesses.ts — create, update, and duplicate mutations updated for the new skills shape. Includes a one-time migrateSkillsToObjects internal mutation that was used to migrate existing documents from the old string[] format.
• packages/convex-backend/convex/seed.ts — Seed data updated to use { name, description } skill objects.

New: Frontend Skills Components

• apps/web/src/components/skills-browser.tsx — Paginated, searchable skill catalog browser using TanStack Query. Checkbox-style selection matching the MCP preset grid pattern.
• apps/web/src/components/recommended-skills-grid.tsx — Curated grid of 9 recommended skills (vercel-react-best-practices, brainstorming, browser-automation, pdf-vision-reader, systematic-debugging, xlsx, code-review, database-schema-design, git-best-practices).
• apps/web/src/lib/skills.ts — Shared SkillEntry type and RECOMMENDED_SKILLS data.
• Each skill entry displays the skill name, repo path (e.g. 0xbigboss/claude-code), and description.

Updated: Harness Editor ($harnessId.tsx)

• Added "Recommended Skills" grid and "Added Skills" checklist above a "Browse All Skills" dialog.
• Skill add/remove is managed in local React state (like name, model, MCPs) and only persisted on save.
• After save, ensureSkillDetails fires in the background to cache markdown for newly added skills.
• Removed the separate skillDetails Convex query for display — descriptions now come directly from the stored skill objects, eliminating flicker.

Updated: Onboarding (onboarding.tsx)

• Skills step shows the recommended grid and full catalog browser.
• Selected skills saved as { name, description } objects on harness creation.
• ensureSkillDetails called after harness creation to backfill markdown.

Updated: Chat Page (chat/index.tsx)

• New SkillsStatus component in the chat header showing "X Skills" next to the MCP status indicator. Clicking it reveals a dropdown listing all skills in the active harness with their display name and description.
• Chat request payload now sends skills: Array<{ name, description }> to FastAPI.

Updated: Chat Loop (FastAPI chat.py)

• When skills are present, a system message is prepended with the skills manifest (name + description for each skill).
• A get_skill_content tool definition is added to the tools list sent to the LLM.
• In the agentic tool execution loop, calls to get_skill_content are intercepted and handled locally — the backend queries Convex for the cached markdown, falling back to HuggingFace if not yet cached.
• No extra Convex query at chat time for descriptions — they come directly from the request payload.

Updated: FastAPI Models (models.py)

• Added SkillRef model (name + description).
• HarnessConfig.skills changed from list[str] to list[SkillRef].

Updated: Convex Service (services/convex.py)

• Added query_convex helper to execute Convex queries via the HTTP API using the deploy key. Used by the chat route to fetch skill detail markdown.

Minor Fixes

• message-attachments.tsx — Replaced ESLint ignore comment with biome-ignore for useMediaCaption.
• chat/index.tsx — Fixed forEach return value lint (useIterableCallbackReturn) by switching to for...of. Added biome-ignore for noStaticElementInteractions on the drag-and-drop zone.
• Import ordering fixed across all modified files to satisfy biome's organizeImports rule.

Files Changed (22 files, +1204 / -136)

Area	Files
FastAPI	routes/skills.py (new), routes/chat.py, models.py, services/convex.py, main.py
Convex	schema.ts, harnesses.ts, skills.ts (new), seed.ts
Frontend Components	skills-browser.tsx (new), recommended-skills-grid.tsx (new)
Frontend Lib	skills.ts (new), use-chat-stream.ts, models.ts, multimodal.ts
Frontend Routes	chat/index.tsx, harnesses/$harnessId.tsx, onboarding.tsx
Other	attachment-chip.tsx, message-attachments.tsx, use-file-attachments.ts

[DIodide]

@claude review this, propose change of skills routes from FastAPI to TanStack server funcs

[claude]

Claude finished @DIodide's task in 2m 17s —— View job

---

PR Review + TanStack Migration Proposal

• Gather context from changed files
• Review PR code
• Propose TanStack server function migration

---

Code Review

Overall this is a solid integration. The pattern of storing { name, description } on the harness (so descriptions don't require extra queries at chat time) is a good design decision. A few issues worth addressing:

Bugs / Correctness

1. /api/skills/detail — unreliable exact-match lookup (routes/skills.py:89–108)
   The endpoint searches by name and then filters the top-5 results for an exact match. If the exact skill name isn't in the top 5 HuggingFace search results (e.g. due to fuzzy ranking), the endpoint silently returns 404. The HuggingFace /rows endpoint supports filtering by field value which would be more reliable for exact lookups.

2. /api/skills/detail appears to have no callers — ensureSkillDetails in convex/skills.ts fetches HuggingFace directly (line 84), and _handle_get_skill_content in chat.py also calls HuggingFace directly (line 81). This route is added but unused, creating maintenance surface for nothing. Fix this →

3. migrateSkillsToObjects left in harnesses.ts — one-time migration mutations should be removed once confirmed applied, or they become confusing dead code that someone might accidentally re-run. Fix this →

Architecture

4. Circular type dependency — apps/web/src/lib/skills.ts:1 imports SkillRow from ../components/skills-browser. Lib files should not import from component files. SkillRow (and SkillsResponse) should live in skills.ts and be imported by the component. Fix this →

5. No auth on /api/skills routes — The list/search endpoints proxy public HuggingFace data with no authentication check. Anyone can use your FastAPI server as a free HuggingFace proxy. Worth adding the same get_current_user dependency the chat route uses, or at minimum a simple API key check.

Minor

6. RECOMMENDED_SKILLS descriptions are hardcoded (skills.ts:14–104) and can drift from the actual HuggingFace dataset descriptions. Consider pulling them from the catalog on first load or at build time.

7. Hardcoded ngrok URL in main.py:49 — "https://aab7-140-180-240-231.ngrok-free.app" should not be committed; it should be in env config.

---

Proposed Migration: Skills Routes → TanStack Server Functions

Why migrate?
The /api/skills and /api/skills/search routes are pure HuggingFace proxies with no business logic. Moving them to TanStack server functions:

• Removes a cross-origin round-trip (browser → FastAPI → HuggingFace → FastAPI → browser)
• Eliminates the CORS dependency for this feature
• Removes VITE_FASTAPI_URL coupling from the skills browser (catalog browsing doesn't need the AI backend running)
• Adds full TypeScript type safety end-to-end
• Keeps the pattern consistent with fetchClerkAuth in __root.tsx

What stays in FastAPI: The /api/skills/detail route can be deleted entirely (see issue #2 above). The _handle_get_skill_content function in chat.py already calls HuggingFace directly.

---

Step 1 — Create apps/web/src/lib/skills-api.ts

import { createServerFn } from "@tanstack/react-start";

const HF_BASE = "https://datasets-server.huggingface.co";
const HF_COMMON = "dataset=tickleliu/all-skills-from-skills-sh&config=default&split=train";

function stripDetail(entry: Record<string, unknown>) {
  const r = (entry.row ?? entry) as Record<string, unknown>;
  return {
    name: (r.name as string) ?? "",
    skill_name: (r.skill_name as string) ?? "",
    description: (r.description as string) ?? "",
    code: (r.code as string) ?? "",
  };
}

export const listSkillsFn = createServerFn({ method: "GET" })
  .validator((data: { offset: number; limit: number }) => data)
  .handler(async ({ data }) => {
    const { offset, limit } = data;
    const resp = await fetch(
      `${HF_BASE}/rows?${HF_COMMON}&offset=${offset}&length=${limit}`,
    );
    if (!resp.ok) throw new Error("Failed to fetch skills from catalog");
    const json = await resp.json();
    return {
      rows: (json.rows as Record<string, unknown>[]).map(stripDetail),
      total: (json.num_rows_total as number) ?? 0,
      offset,
      limit,
    };
  });

export const searchSkillsFn = createServerFn({ method: "GET" })
  .validator((data: { q: string; offset: number; limit: number }) => data)
  .handler(async ({ data }) => {
    const { q, offset, limit } = data;
    const encoded = encodeURIComponent(q);
    const resp = await fetch(
      `${HF_BASE}/search?${HF_COMMON}&query=${encoded}&offset=${offset}&length=${limit}`,
    );
    if (!resp.ok) throw new Error("Failed to search skills catalog");
    const json = await resp.json();
    return {
      rows: (json.rows as Record<string, unknown>[]).map(stripDetail),
      total: (json.num_rows_total as number) ?? 0,
      offset,
      limit,
    };
  });

Step 2 — Update skills-browser.tsx queryFn

Replace the direct fetch call with the server functions:

// Before (skills-browser.tsx:57-63)
queryFn: async () => {
  const endpoint = debouncedSearch
    ? `${API_URL}/api/skills/search?q=${encodeURIComponent(debouncedSearch)}&offset=${offset}&limit=${PAGE_SIZE}`
    : `${API_URL}/api/skills?offset=${offset}&limit=${PAGE_SIZE}`;
  const res = await fetch(endpoint);
  if (!res.ok) throw new Error("Failed to fetch skills");
  return res.json();
},

// After
queryFn: () =>
  debouncedSearch
    ? searchSkillsFn({ data: { q: debouncedSearch, offset, limit: PAGE_SIZE } })
    : listSkillsFn({ data: { offset, limit: PAGE_SIZE } }),

Then remove the API_URL constant and env import from skills-browser.tsx.

Step 3 — Remove the FastAPI skills route

# packages/fastapi/app/main.py
-from app.routes import chat, health, mcp_health, mcp_oauth, skills
+from app.routes import chat, health, mcp_health, mcp_oauth
 
-app.include_router(skills.router, prefix="/api/skills", tags=["skills"])

Delete packages/fastapi/app/routes/skills.py.

---

Trade-off to consider: TanStack server functions run on the Node.js SSR server, so catalog fetches will originate from your server IP rather than the browser. If HuggingFace has per-IP rate limits this concentrates requests. At 50k skills with pagination this is unlikely to be an issue in practice, but worth keeping in mind. If rate limiting becomes a concern, a light caching layer (e.g. a 60-second in-memory cache keyed on offset+limit+query) in the server function would address it cleanly without needing Redis.

[DIodide]

@claude review this

[claude]

Claude finished @DIodide's task in 2m 46s —— View job

---

PR Review: skills.sh Catalog Integration

• Gather context from changed files
• Review FastAPI routes & chat integration
• Review Convex backend
• Review frontend components
• Review types & architecture

---

Note: The TanStack migration proposed in the previous review is already applied in commit 0ed7542 — skills-api.ts uses createServerFn, skills-browser.tsx consumes those functions, and the FastAPI skills.py route was never committed. The migrateSkillsToObjects mutation and the circular type dependency are also resolved. The review below focuses on what remains.

---

Bugs / Correctness

1. ensureSkillDetails writes empty detail on HuggingFace miss, then _handle_get_skill_content re-fetches HuggingFace on every chat request (skills.ts:99–110, chat.py:75–76)

When the HuggingFace top-5 search doesn't contain an exact name match, ensureSkillDetails stores detail = "". On the next chat, _handle_get_skill_content calls query_convex which returns the stored row, but result.get("detail") is falsy for "" → falls through to the HuggingFace fallback again on every single request. checkExists in ensureSkillDetails prevents re-fetching background fills, but the chat-time path has no such guard.

Fix: either skip insertion when detail is empty (so chat falls back every time but the Convex cache only stores real data), or use a sentinel like detail = "__not_found__" to distinguish "tried but absent" from "not yet cached". Fix this →

2. Chat-time HuggingFace fallback has the same exact-match fragility (chat.py:88–94)

_handle_get_skill_content searches HuggingFace for the skill name and then filters for an exact match in the top-5 results — the same logic the previous review flagged for the removed /api/skills/detail endpoint. If the skill name isn't in the top 5 ranked results, this silently returns "Could not retrieve content". Fix this →

3. Hardcoded ngrok URL committed in main.py:50

"https://aab7-140-180-240-231.ngrok-free.app",

This URL will expire and is a dev artifact. It should either be in env config or removed. Also 127.0.0.1:57177 and 127.0.0.1:57609 (lines 49, 51) look like ephemeral dev ports — fine locally but shouldn't be hardcoded. Fix this →

---

Architecture

4. insertSkillDetail never updates stale data (skills.ts:46–48)

if (existing) return existing._id;  // always skips if any record exists

If a skill's detail or description changes in the HuggingFace dataset, the cached version is never refreshed. This is probably fine for now, but worth a comment noting it's an intentional write-once cache.

5. SkillsStatus re-implements click-outside instead of using DropdownMenu (chat/index.tsx:1622–1661)

A custom useRef + document.addEventListener("mousedown") + AnimatePresence panel is built, while the file already imports DropdownMenu, DropdownMenuContent, DropdownMenuItem, DropdownMenuTrigger. This duplicates work, misses keyboard navigation (Escape to close), and breaks the app's component consistency. Fix this →

6. SkillEntry type inlined in SkillsStatus signature (chat/index.tsx:1620)

skills: Array<{ name: string; description: string }>;

The shared SkillEntry type from lib/skills should be used here instead of the inline shape. Minor but keeps types in sync if SkillEntry ever grows.

---

Minor

7. getByNames does N separate DB queries (skills.ts:21–29) — Promise.all over individual ctx.db.query calls for each name. For large skill sets this adds N round-trips. Not a practical concern now, but consider a single query with a filter if this ever serves large harnesses.

8. ensureSkillDetails processes skills sequentially (skills.ts:73) — the for...of loop awaits each HuggingFace call one by one. Using Promise.all would parallelize the fetches. Not a hot path since it's fire-and-forget, but mentioning for completeness.

---

What's Good

• The TanStack migration is clean — skills-api.ts server functions replace the FastAPI proxy entirely, skills-browser.tsx has zero FastAPI coupling, and main.py has no skills router.
• Storing { name, description } on the harness eliminates the extra Convex query at chat time — good design.
• allowed_skill_names guard in _handle_get_skill_content (chat.py:68–69) correctly prevents prompt injection via crafted skill names.
• Types are properly layered: SkillRow/SkillsResponse in lib/skills.ts, SkillEntry for harness storage, SkillRef in Python models — all consistent.
• migrateSkillsToObjects removed, circular lib→component type dependency fixed from the previous review.

[DIodide]

@cole-ramer status?

[cole-ramer]

I haven't really had a chance to work on it again. I expect to work on it more tomorrow or Tuesday. I can easily switch to skills. When they search for skills, but I still don't know how to get the list of skills for the browser. I have looked at the npx skills repo and had Claude look at it, and I haven't found a way that we can get the list of all the skills.

[cole-ramer]

Abandoned the hugging face approach; now there is a script that searches the skills.sh api with a bunch of keywords and letter combinations and adds them to a skillsIndex table in convex, which contains the necessary metadata. Skills browser now sorts by installs descending, and the search feature directly calls the skill. The search feature. If there is a new skill that wasn't caught before, found in the search, it is added to skillsIndex. When a skill is added for the first time, it gets the .md file directly from GitHub. Also, with skills.sh api there is no accessible description of each skill, so I just dropped it. A future fix could be allowing users to open the MD file before adding it to their harness.

[cole-ramer]

@claude review this

[claude]

Claude finished @cole-ramer's task in 2m 28s —— View job

---

PR Review: skills.sh Catalog Integration (Revised Approach)

• Gather context from changed files
• Review scraper script (scripts/scrape-skills.ts)
• Review Convex backend (skills.ts, schema.ts)
• Review frontend components (skills-browser.tsx, recommended-skills-grid.tsx)
• Review FastAPI chat integration (chat.py, convex.py)
• Review harness editor & onboarding changes
• Summarize findings

---

The pivot from HuggingFace to the skills.sh API + GitHub SKILL.md fetch is a solid direction — the scraper approach makes sense for bootstrap, and live search-and-discover keeps the index fresh. A few significant issues to address before merge:

Bugs / Correctness

1. getSkillsIndexCount is an O(N) full table scan (skills.ts:93–99)

const all = await ctx.db.query("skillsIndex").collect();
return all.length;

With ~50k skills, this loads every document to count them on every paginated browse. Convex will hit document read limits here. The browseSkills query can return a hasMore boolean instead, or you can track count as a separate counter document. Fix this →

2. browseSkills pagination degrades to O(offset+limit) (skills.ts:69–79)

const rows = await ctx.db.query("skillsIndex")
    .withIndex("by_installs").order("desc")
    .take(args.offset + args.limit);
return { rows: rows.slice(args.offset), ... };

.take(N) reads N documents from the start of the index every time. Page 100 reads 2,000+ documents to return 20. Use Convex's cursor-based pagination (paginate()) instead of offset/limit. Fix this →

3. checkExistingSkills does N sequential DB queries (skills.ts:137–149)

A for loop of individual .query() calls for each fullId — for a 50-skill batch that's 50 sequential round-trips inside a single Convex query. Consider a single .collect() filtered by q.gte("fullId", ...).lte(...) or just accept the N queries knowing the use case is bounded. Fix this →

4. upsertSkillsIndexBatch is a public mutation (skills.ts:102)

Any authenticated user can call api.skills.upsertSkillsIndexBatch directly from the browser to inject arbitrary entries into the catalog. Since this is only legitimately called from the scraper (via HTTP API with deploy key) and from the discoverSkillsFromSearch action, it should be internalMutation. Fix this →

Architecture

5. Search index is over description but most entries have description: ""

searchSkillsIndex uses .withSearchIndex("search_skills", q => q.search("description", ...)) but skills.sh doesn't return descriptions, so ~all scraped entries have empty descriptions. The Convex full-text search will return almost nothing, meaning search always falls through to the live skills.sh API. This isn't a bug per se but it means the Convex search step is mostly wasted work. Consider searching skillId instead (or adding a skillId search index) since that's the field that actually has data. Fix this →

6. discoverSkillsFromSearch fires on every debounced search (skills-browser.tsx:79–90)

The useEffect triggers a Convex action on every non-empty search result — including repeated queries for the same terms the user already typed before. Once the catalog is populated by the scraper, this fires a checkExistingSkills round-trip on every keystroke even when nothing new is found. Consider adding a client-side Set of already-attempted fullIds to skip the Convex action when all results have been seen before in the session.

7. SkillsStatus still uses custom click-outside dismiss (chat/index.tsx:1617–1682)

Still unfixed from the previous review — useRef + document.addEventListener("mousedown") + AnimatePresence instead of the DropdownMenu components already imported in the file. No keyboard navigation (Escape to close). Fix this →

Minor

8. Hardcoded ngrok URL still in main.py:50 — previously flagged, still present. Fix this →

9. RECOMMENDED_SKILLS install counts are hardcoded (skills.ts:34–120) — these numbers will drift from reality. Since these skills will be in the skillsIndex table after the scraper runs, RecommendedSkillsGrid could query their live counts via convexQuery. For now the counts are display-only so it's low priority.

10. SkillsStatus uses inline type instead of SkillEntry (chat/index.tsx:1620) — Array<{ name: string; description: string }> should use the shared SkillEntry type from lib/skills.

What's Good

• The scraper design is clean: alphabet + keyword sweep, 100ms delays, batched Convex upserts in chunks of 50, graceful error handling throughout.
• Live search merge logic in skills-browser.tsx (Convex results first, then skills.sh results not already in the index) is correct.
• allowed_skill_names guard in _handle_get_skill_content correctly prevents prompt injection via crafted skill names.
• discoverSkillsFromSearch fire-and-forget with .catch(() => {}) is the right pattern for a background catalog update.
• Dropping the description requirement (since skills.sh doesn't expose them) is the right call — better to show skillId + source + install count than fake descriptions.
• The ensureSkillDetails action correctly skips fetching if existing?.detail is already populated.

---

[cole-ramer]

@claude review this

[claude]

Claude finished @cole-ramer's task in 6m 40s —— View job

---

PR Review: skills.sh Catalog Integration (Latest State)

• Gather context from changed files
• Review Convex backend (skills.ts, schema.ts)
• Review frontend components (skills-browser.tsx, recommended-skills-grid.tsx)
• Review FastAPI chat integration (chat.py, convex.py)
• Review scraper script
• Check status of previously flagged issues
• Summarize findings

---

What's Fixed Since Last Review

All major issues from the previous round are resolved:

• ✅ getSkillsIndexCount O(N) table scan — removed; cursor pagination replaces it
• ✅ browseSkills — now uses .paginate() cursor properly (skills.ts:65–77)
• ✅ upsertSkillsIndexBatch — now internalMutation
• ✅ Search index — now on skillId instead of empty description
• ✅ discoverSkillsFromSearch — has seenFullIdsRef session cache to avoid re-sending known fullIds
• ✅ SkillsStatus — uses DropdownMenu / DropdownMenuContent with keyboard nav
• ✅ ngrok URL — removed from main.py

This is a significant improvement. The remaining issues are narrower in scope.

---

Bugs / Correctness

1. _search_skills_sh in chat.py doesn't URL-encode skill_id (chat.py:112)

resp = await http_client.get(
    f"https://skills.sh/api/search?q={skill_id}&limit=20",

If skill_id contains : (common in some skill IDs) or spaces, the URL is malformed. The TypeScript equivalent in skills-api.ts correctly uses encodeURIComponent. Fix this →

2. discoverSkillsFromSearch fetches SKILL.md for every new skill, sequentially (skills.ts:436–446)

When a search returns 20 results not yet in the index, the action calls fetchSkillMd for each — which itself can make up to ~8 GitHub requests per skill (2 branches × 3 base paths + root + tree API). That's potentially 160 unauthenticated GitHub API requests from one user search. The loop is sequential (for...of + await), so it runs for many seconds per search.

The discoverSkillsFromSearch action's purpose is to keep the index fresh — it doesn't need to fetch SKILL.md eagerly. The description field is "" for scraper-sourced entries too. Consider deferring SKILL.md fetching to ensureSkillDetails (which already fires after harness save) rather than doing it at search-discovery time. Fix this →

3. No GitHub API authentication (skills.ts:162–172, chat.py:130–226)

Both the Convex action and FastAPI fallback make unauthenticated GitHub API calls. The unauthenticated rate limit is 60 requests/hour per IP. The scraper, discoverSkillsFromSearch, and _handle_get_skill_content all share this quota. Under normal use with a populated index this won't matter, but during initial scraping or heavy skill-content fetching it could cause silent 403 failures. A GITHUB_TOKEN env var read as a Bearer header would increase the limit to 5,000/hour. Fix this →

---

Architecture

4. checkExistingSkills is still sequential (skills.ts:128–138)

for (const fullId of args.fullIds) {
    const doc = await ctx.db.query("skillsIndex") ...
    if (doc) existing.add(fullId);
}

The previous review flagged this. Promise.all would parallelize the lookups. With up to 20 skills per batch it's not catastrophic, but it's an easy fix. Fix this →

5. ensureSkillDetails still processes sequentially (skills.ts:350)

Each skill name goes through a full fetchSkillMd (up to ~8 HTTP calls) sequentially. Since this fires after harness save (background, fire-and-forget), user experience isn't blocked — but for a harness with 9 recommended skills this could take a while. Promise.all or a bounded concurrency pool (e.g. 3 at a time) would help. Low priority given the fire-and-forget nature.

---

Minor

6. Ephemeral dev ports still in CORS (main.py:49–50)

"http://127.0.0.1:57177",
"http://127.0.0.1:57609",

The ngrok URL is gone (good), but these two ephemeral dev ports remain. They won't cause bugs but are meaningless in any environment except the specific dev machine where they were added. Worth removing. Fix this →

7. SkillsResponse interface is unused dead code (skills.ts:11–16)

export interface SkillsResponse {
    rows: SkillRow[];
    total: number;
    offset: number;
    limit: number;
}

No file imports SkillsResponse — it's a leftover from the HuggingFace offset/limit approach. Safe to delete. Fix this →

8. ChatHeader prop uses inline type instead of SkillEntry (chat/index.tsx:1678)

skills: Array<{ name: string; description: string }>;

The shared SkillEntry type from lib/skills should be used here. Already flagged in the previous review. Fix this →

---

Overall

This is substantially cleaner than the previous revision. The pagination fix, security fix on upsertSkillsIndexBatch, and SkillsStatus component refactor are all solid. The main actionable items before merge are #1 (URL encoding bug), #2 (GitHub API request storm per search), and #3 (rate limiting with a GitHub token). Items #4–#8 are polish.

[cole-ramer]

@claude review this

[cole-ramer]

@claude review this

[claude]

Claude finished @cole-ramer's task in 5m 56s —— View job

---

PR Review: skills.sh Catalog Integration (Latest State)

• Gather context from changed files
• Check status of previously flagged issues
• Review Convex backend (skills.ts, schema.ts)
• Review FastAPI chat integration (chat.py, convex.py)
• Review frontend components
• Summarize findings

---

What's Fixed Since Last Review

All major items from the previous round are resolved:

• ✅ URL encoding — _search_skills_sh now uses quote(skill_id, safe='') (chat.py:113)
• ✅ Non-eager SKILL.md fetching — discoverSkillsFromSearch no longer hits GitHub at search time; just upserts empty-description index entries (skills.ts:437–443)
• ✅ checkExistingSkills — uses Promise.all (skills.ts:129)
• ✅ ensureSkillDetails — bounded concurrency of 3 (skills.ts:351)
• ✅ SkillsResponse dead interface — removed from skills.ts
• ✅ SkillEntry type — used throughout chat/index.tsx instead of inline shape
• ✅ upsertSkillDetail — now patches existing records (no longer write-once)

---

Bugs / Correctness

1. Skill summary query_convex call blocks chat start (chat.py:350–365)

Every chat request that has skills now fires a synchronous Convex query (skills:getByNames) before the first token streams. If Convex is slow (cold start, deploys), this adds latency to every chat response. The summaries are a nice UX improvement but optional — if the getByNames query returns nothing, the system prompt just lacks summaries. Consider running this with a short timeout (e.g. 3s) and falling back gracefully on timeout, rather than awaiting it unconditionally on the critical path. Currently query_convex has a fixed 10-second timeout (convex.py:32). Fix this →

---

Architecture

2. Ephemeral dev ports still in CORS (main.py:49–50)

"http://127.0.0.1:57177",
"http://127.0.0.1:57609",

Flagged in the previous two reviews. Still present. These are meaningless outside the specific machine where they were added. Remove them (or replace with an EXTRA_CORS_ORIGINS env var). Fix this →

3. No GitHub API authentication (skills.ts:162–172, chat.py:88–102)

Both the Convex action and FastAPI fallback still make unauthenticated GitHub API requests. The unauthenticated rate limit is 60 requests/hour per IP. The scraper, discoverSkillsFromSearch (when it fetches skill details via ensureSkillDetails), and _handle_get_skill_content all share this quota from your server's IP. Under normal steady-state use with a warm cache this is fine, but during initial scraping or cache misses it will fail silently. Adding a GITHUB_TOKEN env var would raise the limit to 5,000/hour with minimal effort. Fix this →

4. Non-standard Radix Tooltip/DropdownMenu nesting (chat/index.tsx:1622–1636)

<DropdownMenu>
  <Tooltip>
    <TooltipTrigger asChild>
      <DropdownMenuTrigger asChild>   {/* ← non-standard */}
        <button ...>

The standard Radix pattern for a tooltip-wrapped dropdown trigger is the reverse:

<DropdownMenu>
  <DropdownMenuTrigger asChild>
    <Tooltip>
      <TooltipTrigger asChild>
        <button ...>

The current nesting works because both use asChild and collapse onto the same <button>, but it places Tooltip as the controller of DropdownMenuTrigger — meaning Tooltip's open/close state can interfere with the dropdown opening in some Radix versions. The standard order avoids this. Fix this →

---

Minor

5. RECOMMENDED_SKILLS install counts are hardcoded (skills.ts:29–110) — The counts will drift from reality as skills gain/lose installs. Since these skills will be in the skillsIndex after the scraper runs, RecommendedSkillsGrid could query their live counts. Low priority since counts are display-only.

---

What's Good in This Revision

• The URL encoding fix is correct and clean (PR title is accurate — this was the key bug from last review).
• Non-eager discoverSkillsFromSearch is the right call — background catalog growth without hammering GitHub on every keystroke.
• Skill summaries extracted from cached SKILL.md (chat.py:45–65) give the model better context about each skill without requiring a tool call — good system prompt design.
• upsertSkillDetail now patches existing records — stale SKILL.md content can be updated.
• All type usage is consistent: SkillEntry[] throughout frontend, SkillRef in Python models.
• The fallback chain in _handle_get_skill_content (Convex → original source → GitHub resolution → skills.sh search → resolved skills.sh source) is comprehensive and well-structured.

---

Summary: Two items from previous reviews remain unaddressed (ephemeral dev ports, GitHub auth). One new latency concern with the getByNames query on the chat critical path. One nesting issue in SkillsStatus. Items #2 and #3 are the most important to address before merge.

[cole-ramer]

@claude review this

[claude]

Claude finished @cole-ramer's task in 2m 0s —— View job

---

Claude finished @cole-ramer's task in 5m —

---

PR Review: skills.sh Catalog Integration (Latest State)

• Gather context from changed files
• Check status of previously flagged issues
• Review Convex backend (skills.ts, schema.ts)
• Review FastAPI chat integration (chat.py, convex.py, main.py)
• Review frontend components
• Summarize findings

---

What's Fixed Since Last Review

Significant progress — all but two items from the previous round are resolved:

• ✅ Ephemeral dev ports (127.0.0.1:57177, 127.0.0.1:57609) — removed from main.py; CORS origins are now clean
• ✅ SkillsStatus Tooltip/DropdownMenu nesting — now correctly ordered: DropdownMenuTrigger > Tooltip > TooltipTrigger (chat/index.tsx:1622–1636)
• ✅ URL encoding (quote(skill_id, safe='')) — already fixed previously, still correct
• ✅ checkExistingSkills — uses Promise.all (skills.ts:129)
• ✅ ensureSkillDetails — bounded concurrency of 3 (skills.ts:351)
• ✅ upsertSkillsIndexBatch — internalMutation; discoverSkillsFromSearch defers SKILL.md fetching
• ✅ SkillEntry type used in SkillsStatus and ChatHeader (chat/index.tsx:1618, 1678)

---

Remaining Issues

1. query_convex for skill summaries still blocks chat start (chat.py:350–354)

skill_details_list = await query_convex(
    http_client,
    "skills:getByNames",
    {"names": [s.name for s in skill_refs]},
)

This was flagged in the last two reviews and is still unaddressed. The Convex query has a 10-second timeout (convex.py:32). If Convex is slow during cold start or a deploy, every chat request with skills incurs that latency on the critical path before the first token streams. The summaries enhance system prompt context but are non-essential — the model can still use get_skill_content to retrieve full content. Wrapping with asyncio.wait_for and falling back on timeout would eliminate the latency risk with minimal code. [Fix this →](https://claude.ai/code?q=In%20%60packages%2Ffastapi%2Fapp%2Froutes%2Fchat.py%60%20on%20branch%20feak%2Fskills.sh%2C%20wrap%20the%20query_convex%20call%20at%20line%20350%20(skills%3AgetByNames)%20in%20%60asyncio.wait_for(query_convex(...%2C%20timeout%3D3.0)%60%20and%20catch%20asyncio.TimeoutError%2C%20falling%20back%20to%20an%20empty%20%60details_by_name%60%20dict%20so%20slow%20Convex%20responses%20don%27t%20delay%20chat%20streaming.&repo=DIodide/Harness)

2. No GitHub API authentication (skills.ts:162–172, chat.py:88–102)

Both the Convex fetchSkillMdFromRepo/resolveGitHubRepo functions and the FastAPI _fetch_skill_md_from_repo/_resolve_github_repo functions make unauthenticated GitHub API calls (gh_headers has no Authorization header). The unauthenticated rate limit is 60 requests/hour per server IP, shared across all users. The scraper script, ensureSkillDetails, and _handle_get_skill_content all draw from this same quota. Under normal steady-state use with a warm cache this won't matter, but a cache miss for a popular skill could quickly exhaust the limit during peak usage.

Adding a GITHUB_TOKEN env var raises the limit to 5,000/hour with minimal change:

# In chat.py _fetch_skill_md_from_repo:
gh_headers = {"Accept": "application/vnd.github.v3+json"}
if settings.github_token:
    gh_headers["Authorization"] = f"Bearer {settings.github_token}"

// In skills.ts fetchSkillMdFromRepo:
const token = process.env.GITHUB_TOKEN;
const ghHeaders: Record<string, string> = { Accept: "application/vnd.github.v3+json" };
if (token) ghHeaders["Authorization"] = `Bearer ${token}`;

Fix this →

---

Minor

3. inputValidator vs validator in skills-api.ts (skills-api.ts:21)

.inputValidator((data: { q: string; limit: number }) => data)

The TanStack Start docs use .validator() (not .inputValidator()). This is an API difference that may work or silently fail depending on the version — worth verifying it matches the TanStack Start version in use. The listSkillsFn from the previous design used .validator() consistently.

4. RECOMMENDED_SKILLS install counts are hardcoded (skills.ts:29–110)

The counts will drift over time. Since these skills will be in skillsIndex after the scraper runs, RecommendedSkillsGrid could query their live counts via Convex. Low priority since counts are display-only context, not functional.

5. Browse-mode cursor stack grows unboundedly (skills-browser.tsx:168–177)

The cursor stack is trimmed on search-term changes (setCursorStack([null])) but not on unmount or when the user navigates far forward and back. With 50k skills and 20/page that's 2,500 potential cursor entries in memory for a single session. Not a practical issue currently, but worth noting.

---

What's Good

• Ephemeral port CORS cleanup is done — the allow_origins list is now clean and config-driven.
• SkillsStatus dropdown nesting is correct and uses standard Radix primitives with keyboard nav.
• discoverSkillsFromSearch fire-and-forget pattern with session-level seenFullIdsRef deduplication is clean.
• checkExistingSkills is now parallelized with Promise.all.
• The ensureSkillDetails bounded concurrency (3 at a time) is the right trade-off for a background, fire-and-forget action.
• Type consistency is solid: SkillRow in lib, SkillEntry on harness/chat, SkillRef in Python models.
• upsertSkillsIndexBatch as internalMutation prevents client-side catalog injection.

Summary: Two substantive items remain — the Convex query latency on the chat critical path (#1) and GitHub API rate limiting (#2). Both are low-effort fixes with meaningful reliability impact. Items #3–#5 are polish/low priority.

[cole-ramer]

@claude review this

[claude]

Claude finished @cole-ramer's task in 2m 13s —— View job

---

PR Review: skills.sh Catalog Integration (Latest State)

• Gather context from changed files
• Check status of previously flagged issues
• Review FastAPI chat integration
• Review Convex backend
• Review frontend components
• Summarize findings

---

What's Fixed Since Last Review

All items from the previous round are resolved except GitHub auth:

• ✅ asyncio.wait_for wrapping query_convex with 3-second timeout + graceful fallback on error (chat.py:354–367)
• ✅ Ephemeral dev ports (127.0.0.1:57177, 127.0.0.1:57609) — removed from main.py; CORS list is now clean
• ✅ SkillsStatus Tooltip/DropdownMenu nesting — now correctly DropdownMenuTrigger > Tooltip > TooltipTrigger (chat/index.tsx:1622–1636)
• ✅ SkillEntry type used in ChatHeader props (chat/index.tsx:1678)
• ✅ inputValidator → .validator() in skills-api.ts — already correct

---

Remaining Issue

1. No GitHub API authentication (skills.ts:162, chat.py:139)

Both the Convex fetchSkillMdFromRepo/resolveGitHubRepo and FastAPI _fetch_skill_md_from_repo/_resolve_github_repo make unauthenticated GitHub API calls:

# chat.py:139
gh_headers = {"Accept": "application/vnd.github.v3+json"}
# no Authorization header

// skills.ts:211
const ghHeaders = { Accept: "application/vnd.github.v3+json" };
// no Authorization header

This has been flagged in the last three reviews. The unauthenticated rate limit is 60 requests/hour per server IP — shared across all users, the scraper, ensureSkillDetails, and _handle_get_skill_content. Under normal steady-state use with a warm cache this won't matter, but any cache miss storm or initial scraper run can hit this limit causing silent failures. Adding a GITHUB_TOKEN env var raises the limit to 5,000/hour with minimal effort.

Fix this →

---

Minor (Low Priority)

2. RECOMMENDED_SKILLS install counts are hardcoded (skills.ts:29–111)

Install counts like 263727, 142765, etc. will drift from reality over time. Since these skills are in skillsIndex after the scraper runs, RecommendedSkillsGrid could query their live counts from Convex. Low priority — counts are display-only context.

3. Browse-mode cursor stack grows unboundedly (skills-browser.tsx:34)

The cursorStack state is trimmed on search-term changes (setCursorStack([null])) but not bounded during forward navigation. With 50k skills at 20/page that's up to 2,500 entries in memory. Not a practical problem at current scale, but worth noting.

---

What's Good

This PR is in solid shape. Everything from the previous four review rounds has been addressed except the GitHub token. The quality of recent fixes has been high:

• asyncio.wait_for fallback is clean — the catch-all except (asyncio.TimeoutError, Exception) is the right pattern here
• CORS list is now clean and config-driven via settings.frontend_url
• SkillsStatus uses standard Radix primitives with correct nesting and keyboard nav
• discoverSkillsFromSearch fire-and-forget with seenFullIdsRef session deduplication prevents redundant Convex roundtrips
• checkExistingSkills parallelized with Promise.all
• upsertSkillsIndexBatch is internalMutation — client injection is blocked
• Type consistency solid throughout: SkillRow in lib, SkillEntry on harness/chat, SkillRef in Python models

Summary: One substantive item remains — GitHub API auth (#1). Items #2 and #3 are polish. This is ready to merge once #1 is addressed.

[cole-ramer]

@claude this commit should hopefully be the result of the merged staging and feak/skills.sh (that way it is super easy to merge into staging) make sure nothing broke

[claude]

Claude finished @cole-ramer's task in 3m 48s —— View job

---

Merge Verification

• Gather context — check git log and diff vs staging
• Check for merge conflict markers
• Review merged files for correctness
• Check key integration points (FastAPI, Convex, frontend)
• Summarize findings

---

The merge looks clean overall. No leftover conflict markers, and the two feature sets (skills + sandbox) integrate without stepping on each other.

What Was Merged

Commit b71ac2a merged staging (65891c8) into the skills branch. Staging brought in the sandbox feature (Daytona integration, new routes, schema tables, frontend components), deployment configs, and some frontend changes to chat/index.tsx, $harnessId.tsx, and onboarding.tsx — all of which also had skills changes on this branch.

Merge Result: Looks Good

FastAPI chat.py — Skills and sandbox integrate correctly and in the right order: MCP tools → skills manifest/tool injection → sandbox tool injection → agentic loop. No conflicts in logic.

FastAPI convex.py — Both query_convex (needed by skills) and create_sandbox_record (needed by sandbox) are present.

FastAPI models.py — SkillRef, SandboxConfig, and HarnessConfig all present and correctly composed.

Convex schema.ts — Both skillDetails/skillsIndex (from this branch) and sandboxes (from staging) are in the schema with proper indexes.

Convex _generated/api.d.ts — Both skills and sandboxes modules are registered.

Frontend chat/index.tsx — Imports look correct. The last commit (00c6527) cleaned up a duplicate import block (multimodal/models imports appeared twice after the merge — once in the original position, once re-inserted by the merge at a different location). The cleanup was correct, both sets referenced the same symbols, and the originals at lines 107–113 remain.

---

One Regression from the Merge

http://127.0.0.1:57177 reappeared in main.py:53 — This ephemeral dev port was removed from the skills branch in a previous commit, but it was still present on staging's main.py, so the merge pulled it back in. It was flagged in the last three review rounds.

# packages/fastapi/app/main.py:53 — should be removed
"http://127.0.0.1:57177",

Fix this →

---

Summary

The merge is functionally sound — no conflicts, no broken imports, no missing symbols. The only issue is the dev port CORS origin re-introduced from staging. Everything else that was previously fixed in the skills review cycle remains fixed.