Skip to content

fix(registry): extract skill content from ZIP archive#22

Merged
mgoldsborough merged 2 commits into
mainfrom
fix/skill-content-extraction
Mar 4, 2026
Merged

fix(registry): extract skill content from ZIP archive#22
mgoldsborough merged 2 commits into
mainfrom
fix/skill-content-extraction

Conversation

@mgoldsborough
Copy link
Copy Markdown
Contributor

Summary

  • Skill .skill files are ZIP archives containing SKILL.md, but the announce endpoint was reading the ZIP binary as raw UTF-8 text and regex-matching for YAML frontmatter — which always failed silently, storing content as null
  • Skill detail pages (e.g., /skills/@nimblebraininc/upjack-app-builder) only showed tags/metadata with no skill body content
  • Now uses AdmZip (already a dependency) to extract SKILL.md from the archive before parsing frontmatter

Test plan

  • Verify pnpm typecheck and pnpm test pass (65/65 tests passing locally)
  • Re-announce an existing skill (e.g., upjack-app-builder) and verify the API returns non-null content
  • Confirm the skill detail page renders the markdown body on the Overview tab

…file

The .skill file is a ZIP archive containing SKILL.md, but the announce
endpoint was reading the ZIP binary as UTF-8 text and regex-matching
for YAML frontmatter. This always failed, storing content as null.

Now uses AdmZip to extract SKILL.md from the archive before parsing.
@mgoldsborough mgoldsborough requested a review from a team February 26, 2026 03:06
Move ZIP extraction + frontmatter body parsing from inline route handler
into extractSkillContent() utility with 9 unit tests covering nested/root
SKILL.md, edge cases (CRLF, no frontmatter, empty body), and error
resilience (corrupted ZIP).
@mgoldsborough mgoldsborough added qa-reviewed QA review completed with no critical issues pkg/registry Registry API server labels Feb 26, 2026
Copy link
Copy Markdown
Contributor

@JoeCardoso13 JoeCardoso13 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clean fix — correct root cause, minimal diff, good test coverage.

Minor nitpicks (non-blocking):

  1. Observability — When extractSkillContent() returns undefined, the route handler proceeds silently. A log.warn would help debug content extraction failures in production.
  2. Robustness — The frontmatter regex ([\s\S]*? non-greedy) could mismatch if YAML content contains a literal \n---\n line. Unlikely in practice; same regex as before, so not a regression.
  3. Test coverage — No test for deeply nested paths (e.g., a/b/c/SKILL.md). The endsWith logic handles it, but it's undocumented by tests.
  4. Error handling — The try/catch in extractSkillContent only wraps the AdmZip constructor, but getData() can also throw. Consider widening it to cover the full function body so the string | undefined return contract holds. The route handler's outer try/catch prevents any real damage.

@mgoldsborough mgoldsborough merged commit 291753b into main Mar 4, 2026
2 checks passed
@mgoldsborough mgoldsborough deleted the fix/skill-content-extraction branch March 4, 2026 19:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pkg/registry Registry API server qa-reviewed QA review completed with no critical issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants