feat(publish): add RFC 8615 well-known skills support #36
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughAdds CLI commands (AgentFromSkill, PublishSubmit), implements skill→subagent conversion, introduces a Well-Known provider and RFC‑8615 publish flow (.well-known/skills), reworks publish to generate hosting structures, and adds tests and docs for converters and well‑known utilities. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant CLI as CLI (agent from-skill)
participant Discovery as Skill Discovery
participant Converter as Skill Converter
participant Generator as Markdown Generator
participant FS as File System
User->>CLI: run `agent from-skill` with options
CLI->>Discovery: discover skills at path
Discovery-->>CLI: return skill list
CLI->>Converter: load skill content & parse frontmatter
Converter->>Converter: build canonical agent object
Converter-->>Generator: canonical agent
Generator->>Generator: render YAML frontmatter + body
Generator-->>CLI: markdown content
CLI->>FS: sanitize filename and write file (or dry-run)
FS-->>CLI: success or preview
CLI-->>User: output path / preview / guidance
sequenceDiagram
actor User
participant CLI as CLI (publish)
participant Discovery as Skill Discovery
participant WellKnownGen as Well-Known Generator
participant FS as File System
participant Browser as Browser
User->>CLI: run `publish` (or `publish submit`)
CLI->>Discovery: discover skills under skillPath
Discovery-->>CLI: list of skills + frontmatter
CLI->>WellKnownGen: generate .well-known/skills structure
WellKnownGen->>FS: create directories and write SKILL.md and files
WellKnownGen->>FS: write index.json
FS-->>WellKnownGen: written paths
WellKnownGen-->>CLI: structure metadata (or preview)
alt dry-run
CLI-->>User: display preview of files to be written
else submit
CLI->>FS: read index.json
CLI->>Browser: open issue creation URL with payload
Browser-->>User: issue form with prefilled content
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 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 |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| if (!index || !index.skills || index.skills.length === 0) { | ||
| return { | ||
| success: false, | ||
| error: `No skills found at ${baseUrl}/.well-known/skills/index.json`, | ||
| }; | ||
| } |
There was a problem hiding this comment.
🟡 Temp directory not cleaned up on early return in WellKnownProvider.clone()
When the clone method returns early due to no skills being found in the index, the temp directory that was created is not cleaned up, causing a resource leak.
Click to expand
How the bug is triggered
- The temp directory is created at line 61:
mkdirSync(tempDir, { recursive: true }); - If the fetched index is null, has no skills array, or has an empty skills array (line 85-90), the function returns an error result without cleaning up the temp directory
- The catch block at lines 144-151 only handles thrown exceptions, not early returns
Actual vs Expected
Actual: When no skills are found in the index, the function returns { success: false, error: ... } but the created temp directory at tempDir remains on disk.
Expected: The temp directory should be cleaned up before returning the error, similar to how it's done on line 130 when no valid skills are found after processing.
Impact
Repeated failed attempts to clone from well-known URLs with empty or invalid indexes will leave orphaned temporary directories in the system's temp folder, gradually consuming disk space.
Recommendation: Add rmSync(tempDir, { recursive: true, force: true }); before the return statement at line 86, similar to the cleanup done at line 130.
Was this helpful? React with 👍 or 👎 to provide feedback.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@packages/cli/src/commands/agent.ts`:
- Around line 942-955: The default filename built from skill.name isn't
sanitized, allowing path traversal; apply the same sanitizeFilename(skill.name)
logic used for this.output, reject and return 1 if sanitizeFilename returns
falsy (logging the same error messages), and then set filename =
`${sanitized}.md`; ensure outputPath = join(targetDir, filename) uses this
sanitized filename to prevent writing outside targetDir (refer to
sanitizeFilename, this.output, filename, skill.name, outputPath, targetDir).
In `@packages/cli/src/commands/publish.ts`:
- Around line 64-107: Discovered skill names from frontmatter (skill.name) are
used directly to build output paths (join(..., skill.name)) which allows path
traversal; validate/sanitize skill.name before using it in
wellKnownDir/skillDir/mkdirSync/writeFileSync by rejecting or normalizing names
that contain path separators, “..”, or absolute paths and by mapping to a safe
filename (e.g., slugify or use path.basename and a whitelist/regex like
/^[A-Za-z0-9._-]+$/); update the loop that creates skillDir and writes files
(the code that calls join(wellKnownDir, skill.name), mkdirSync(skillDir, ...),
and writeFileSync(destPath, ...)) to use the sanitized name and error/skip any
invalid skills while logging a clear warning.
In `@packages/core/src/agents/skill-converter.ts`:
- Around line 213-218: The frontmatter extractor extractBodyContent currently
only matches Unix newlines, so update its regex to accept optional CR before LF
(use \r?\n in the pattern) or otherwise normalize line endings before matching;
modify the match in extractBodyContent to handle CRLF (e.g., change the
/^---\s*\n...$/ pattern to allow \r?\n or run content = content.replace(/\r\n/g,
'\n') before matching) so Windows-style frontmatter is stripped in inline mode
and duplicated frontmatter is avoided.
In `@packages/core/src/providers/wellknown.ts`:
- Around line 96-125: The code uses untrusted skill.name when building skillDir
(join(tempDir, skill.name)) and discoveredSkills entries, creating a path
traversal risk; validate or sanitize skill.name before use by either normalizing
and rejecting names containing path separators or ".." (e.g., forbid "/" or "\"
and ".."), or replace it with path.basename(skill.name), then compute skillDir
via path.resolve(tempDir, safeName) and assert the resolved path
startsWith(path.resolve(tempDir)) to guarantee it remains inside tempDir; update
all uses (skillDir, discoveredSkills.name, discoveredSkills.dirName, and fileUrl
construction if it uses skill.name) to use the sanitized safeName.
🧹 Nitpick comments (2)
packages/core/src/providers/__tests__/wellknown.test.ts (1)
1-6: Remove unusedviimport.The
vimock utility is imported but not used in this test file.🧹 Proposed fix
-import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { describe, it, expect, beforeEach, afterEach } from 'vitest';packages/core/src/agents/skill-converter.ts (1)
165-193: Consider escaping more YAML fields for robustness.
name,author, and individualtagscan include characters that break YAML. Escaping them likedescriptionwill avoid malformed frontmatter for non‑slug names.♻️ Suggested hardening
- lines.push(`name: ${canonical.name}`); + lines.push(`name: ${escapeYamlString(canonical.name)}`); ... - if (canonical.author) { - lines.push(`author: ${canonical.author}`); - } + if (canonical.author) { + lines.push(`author: ${escapeYamlString(canonical.author)}`); + } if (canonical.tags && canonical.tags.length > 0) { - lines.push(`tags: [${canonical.tags.join(', ')}]`); + lines.push(`tags: [${canonical.tags.map(t => escapeYamlString(t)).join(', ')}]`); }
| const resolvedSkillDir = resolve(skillDir); | ||
| const resolvedWellKnownDir = resolve(wellKnownDir); | ||
|
|
||
| if (!resolvedSkillDir.startsWith(resolvedWellKnownDir)) { |
There was a problem hiding this comment.
🟡 Incomplete path traversal check allows writing outside target directory
The path traversal check in PublishCommand.execute() uses startsWith without ensuring a trailing path separator, which can be bypassed.
Click to expand
Issue
At line 128, the check is:
if (!resolvedSkillDir.startsWith(resolvedWellKnownDir)) {This check is flawed because startsWith matches prefixes, not path components. For example, if resolvedWellKnownDir is /tmp/output/.well-known/skills, a skill directory like /tmp/output/.well-known/skillsXXX would pass this check even though it's a sibling directory, not a child.
Actual vs Expected
- Actual: A crafted skill name could write files outside the intended
.well-known/skills/directory - Expected: Only paths that are proper children of the well-known directory should be allowed
Comparison
The correct implementation exists in packages/core/src/providers/wellknown.ts:118:
if (!resolvedSkillDir.startsWith(resolvedTempDir + '/') && resolvedSkillDir !== resolvedTempDir) {This correctly appends a path separator before the startsWith check.
Impact
While the sanitizeSkillName function provides some protection by rejecting names with path separators, this is still a defense-in-depth issue that could lead to unexpected file writes if the sanitization is bypassed or modified.
Recommendation: Change line 128 to: if (!resolvedSkillDir.startsWith(resolvedWellKnownDir + '/') && resolvedSkillDir !== resolvedWellKnownDir) {
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 `@packages/core/src/providers/wellknown.ts`:
- Around line 197-229: The function generateWellKnownStructure uses skill.name
directly when building skillDir and writing files; replace that with the
sanitized name via sanitizeSkillName(skill.name), and after computing skillDir
use path.resolve to get the absolute path and validate that it is rooted inside
wellKnownDir (e.g., resolvedSkillDir.startsWith(path.resolve(wellKnownDir) +
path.sep)) before creating directories or writing files; follow the same pattern
as fetchSkills to throw or skip on invalid names so untrusted inputs cannot
perform path traversal.
- Around line 114-120: The containment check using
resolvedSkillDir.startsWith(resolvedTempDir + '/') is platform-unsafe because it
hardcodes '/'; update the check in the block that computes skillDir /
resolvedSkillDir / resolvedTempDir to use platform path separator (path.sep) or
a normalized comparison: import or use path.sep and replace the literal '/' with
path.sep (or normalize both paths and ensure you check for prefix plus
separator) so resolvedSkillDir.startsWith(resolvedTempDir + path.sep) (and keep
the existing resolvedSkillDir !== resolvedTempDir fallback) to make the
containment check work on Windows and POSIX; reference the variables skillDir,
resolvedSkillDir, resolvedTempDir and the startsWith check to locate the change.
🧹 Nitpick comments (1)
packages/core/src/agents/skill-converter.ts (1)
161-199: Escape YAML scalars and list items to avoid invalid frontmatter.
Unescapedname,author, and list items (tools/tags/skills) can break YAML if they contain special characters or spaces. Consider reusingescapeYamlStringfor these fields and list items.💡 Suggested update to harden YAML output
- lines.push(`name: ${canonical.name}`); + lines.push(`name: ${escapeYamlString(canonical.name)}`); lines.push(`description: ${escapeYamlString(canonical.description)}`); @@ - if (canonical.author) { - lines.push(`author: ${canonical.author}`); - } - if (canonical.tags && canonical.tags.length > 0) { - lines.push(`tags: [${canonical.tags.join(', ')}]`); - } + if (canonical.author) { + lines.push(`author: ${escapeYamlString(canonical.author)}`); + } + appendYamlList(lines, 'tags', canonical.tags); @@ function appendYamlList(lines: string[], key: string, items?: string[]): void { if (!items || items.length === 0) return; lines.push(`${key}:`); for (const item of items) { - lines.push(` - ${item}`); + lines.push(` - ${escapeYamlString(item)}`); } }Also applies to: 202-207
Add `skillkit agent from-skill` command to convert SkillKit skills into Claude Code native subagent format (.md files in .claude/agents/). Features: - Reference mode (default): generates subagent with `skills: [skill-name]` - Inline mode (--inline): embeds full skill content in system prompt - Options: --model, --permission, --global, --output, --dry-run New files: - packages/core/src/agents/skill-converter.ts - packages/core/src/agents/__tests__/skill-converter.test.ts (23 tests) Closes #22
Redesign publish workflow to align with industry standard: - Add WellKnownProvider for auto-discovery from any domain - skillkit add https://example.com discovers skills via /.well-known/skills/ - skillkit publish generates well-known hosting structure - skillkit publish submit opens GitHub issue (legacy workflow) Provider discovers skills from: - /.well-known/skills/index.json (manifest) - /.well-known/skills/{skill-name}/SKILL.md (skill files) Includes 16 tests for WellKnownProvider and structure generation.
…lback When using the /.well-known/skills.json fallback URL, the previous logic produced a malformed URL with duplicated .well-known path segment: - Before: https://example.com/.well-known/.well-known/skills - After: https://example.com/.well-known/skills Extract calculateBaseSkillsUrl helper function for better testability and add 4 new unit tests covering both URL formats.
- README.md: Add self-hosting section with well-known URI structure - commands.mdx: Add Publishing Commands section - marketplace.mdx: Expand publish section with self-hosting instructions - skills.mdx: Add self-hosting workflow documentation Documents the new `skillkit publish` workflow that generates RFC 8615 well-known URI structures for decentralized skill hosting.
- agent.ts: Sanitize skill.name when building default filename - publish.ts: Add sanitizeSkillName() to validate skill names before using in file paths, verify resolved paths stay within target dir - skill-converter.ts: Normalize CRLF to LF before extracting body content to handle Windows-style line endings - wellknown.ts: Add sanitizeSkillName() to validate remote skill names, verify resolved paths stay within temp dir, encode URL components
rohitg00
force-pushed
the
feat/well-known-skills
branch
from
242a4df to
821fc1d
Compare
Only escape strings that actually need it for valid YAML parsing:
- Strings containing newlines or colons
- Strings starting with special YAML characters (-, *, &, !, {, [, >, |, @, `)
- Strings starting with quotes
This fixes unnecessary escaping of strings like "code-quality" where
hyphens in the middle don't need escaping.
- Use path.sep instead of hardcoded '/' for cross-platform compatibility - Add sanitization and path containment check in generateWellKnownStructure - Remove unused 'vi' import in wellknown.test.ts
| for (const [filename, content] of Object.entries(skill.additionalFiles)) { | ||
| writeFileSync(join(skillDir, filename), content); |
There was a problem hiding this comment.
🔴 Path traversal vulnerability in generateWellKnownStructure via unsanitized additionalFiles filename
The generateWellKnownStructure function writes files from additionalFiles without sanitizing the filename keys, allowing path traversal attacks.
Click to expand
Vulnerable Code
At packages/core/src/providers/wellknown.ts:229-230:
for (const [filename, content] of Object.entries(skill.additionalFiles)) {
writeFileSync(join(skillDir, filename), content);If filename contains path traversal characters like ../../../etc/passwd, the file will be written outside the intended directory.
Proof of Concept
const skillDir = '/tmp/test/.well-known/skills/my-skill';
const maliciousFilename = '../../../etc/passwd';
const destPath = join(skillDir, maliciousFilename);
// destPath = '/tmp/test/etc/passwd' - outside the skill directory!Impact
If an attacker can control the additionalFiles keys (e.g., through a malicious skill definition passed to this exported function), they could write arbitrary files to the filesystem.
Recommendation: Sanitize the filename using basename() before joining with the skill directory:
for (const [filename, content] of Object.entries(skill.additionalFiles)) {
const safeFilename = basename(filename);
if (!safeFilename || safeFilename.startsWith('.')) continue;
writeFileSync(join(skillDir, safeFilename), content);
Was this helpful? React with 👍 or 👎 to provide feedback.
Summary
Redesigns the publish workflow to align with the industry-standard well-known URI (RFC 8615) approach used by Vercel and others.
Changes
New WellKnownProvider (
packages/core/src/providers/wellknown.ts)/.well-known/skills/index.json/.well-known/skills/{skill-name}/SKILL.mdhttps://example.com(not github/gitlab/bitbucket)Redesigned
skillkit publish/.well-known/skills/index.jsonmanifest/.well-known/skills/{skill-name}/--outputto specify target directory--dry-runto preview without writingLegacy workflow preserved as
skillkit publish submitUsage
Output Structure
Test plan
skillkit publish --dry-runworksSummary by CodeRabbit
New Features
Tests
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.