Conversation
Add GitHub URL parser supporting shorthand (user/repo), HTTPS, SSH, and tree-path formats. Add skills manifest for tracking installed skills at SHAKA_HOME/skills.json with load, save, add, and remove operations. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add pluggable SkillSourceProvider interface with auto-detection registry. GitHub provider supports git clone with marketplace.json fallback for multi-skill repos. Clawdhub provider fetches from clawhub.ai registry with version resolution and ZIP extraction. Shared pipeline utilities handle temp directory management and SKILL.md validation. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add install, remove, update, and linker services that orchestrate the full skill lifecycle. Install service delegates fetching to source providers, then runs validate-scan-deploy-persist pipeline. Update service re-fetches from the original provider. Linker manages symlinks between SHAKA_HOME/skills/ and provider config directories. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Extend asset-installer with per-skill symlink functions for installed third-party skills. System skills keep the original directory symlink (~/.claude/skills/shaka/), while installed skills get individual symlinks (~/.claude/skills/<name>/). Add cross-platform path.sep handling. Update both Claude and OpenCode configurers. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Format-reminder hook now scans system/agents/ and customizations/agents/ separately (with customization agents extending system capabilities). Skills are scanned from system/skills/ and installed skills/ with per-key deduplication via Map. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add `shaka skill` command group with install, remove, update, and list subcommands. Install supports --github/--clawdhub flags for explicit provider selection and --force/--safe-only for security scan control. Doctor command now reports installed skill health with orphan detection. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Update README with three-tier skill architecture, directory tree, CLI commands, and installation examples. Add CHANGELOG entries for multi-provider skill installation, Clawdhub provider, marketplace fallback, and SkillSourceProvider abstraction. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
a-mpch
force-pushed
the
2026-02-add-skill-management
branch
from
05f3c64 to
07244af
Compare
a-mpch
force-pushed
the
2026-02-add-skill-management
branch
from
df57efb to
cfec053
Compare
| # Security options | ||
| shaka skill install user/repo --force # Skip security scan prompt | ||
| shaka skill install user/repo --safe-only # Abort if non-text files found | ||
| ``` |
There was a problem hiding this comment.
I think these are wrong, wasn't it --yolo? we can rename to --force if you prefer but let's have implementation and docs match
There was a problem hiding this comment.
mb, did --force first and then moved to --yolo
| // GitHub first (matches `/`, `https://`, `git@`), Clawdhub catches bare words. | ||
| registerProvider(createGitHubProvider()); | ||
| registerProvider(createClawdhubProvider()); | ||
|
|
There was a problem hiding this comment.
Design: Module-level side effect in skill-source/index.ts
These calls execute at import time, mutating the global providers array in registry.ts. This is a coupling hazard:
-
Any import of this module triggers registration, even transitive imports from test files. The registry test (
test/unit/services/skill-source/registry.test.ts) correctly callsclearProviders()inbeforeEach, but this is a footgun — a test that imports from./index.tsinstead of./registry.tswill get extra providers silently. -
Multiple imports don't re-register (modules are cached), but if someone calls
clearProviders()and then expects re-importing to restore defaults, it won't work. -
Providers with default dependencies (real git, real HTTP) are registered globally. This means the Clawdhub provider created here uses
fetch()against the realclawhub.airegistry. If any code path in tests reachesdetectProvider("sonoscli")without mocking, it'll hit the network.
Suggestion: Consider lazy initialization. Replace the eager calls with a getDefaultProviders() function, or register in the CLI entry point (src/commands/skill.ts) rather than at module scope.
| // Fetch latest from provider (passing stored subdirectory for update flow) | ||
| const fetchResult = await provider.fetch(skill.source, { subdirectory: skill.subdirectory }); | ||
| if (!fetchResult.ok) return fetchResult; | ||
|
|
||
| try { | ||
| // Check if already up to date | ||
| if (fetchResult.value.version === skill.version) { | ||
| return ok({ | ||
| name, | ||
| previousVersion: skill.version, | ||
| newVersion: fetchResult.value.version, | ||
| upToDate: true, | ||
| }); | ||
| } | ||
|
|
||
| // Deploy and persist | ||
| await installSkillFiles(fetchResult.value.skillDir, shakaHome, name); |
There was a problem hiding this comment.
Design: updateSkill skips security checks entirely
The update path fetches the new version and deploys it directly — no validateSkillStructure, no runSecurityChecks, no enforceSecurityChecks. A malicious actor could push a benign first version (passes install checks) and then push a version containing executables, invisible characters, or prompt injection via HTML comments. The update would silently deploy it.
// Fetch latest from provider (passing stored subdirectory for update flow)
const fetchResult = await provider.fetch(skill.source, { subdirectory: skill.subdirectory });
if (!fetchResult.ok) return fetchResult;
// ...version check...
// Deploy and persist — NO validation or security scan
await installSkillFiles(fetchResult.value.skillDir, shakaHome, name);Suggestion: At minimum, call validateSkillStructure to ensure the updated content still has a valid SKILL.md. Ideally, re-run the security checks too (especially since the whole point of the security system is to catch non-text content). The user already trusts the source, so you could use a lighter touch (warn rather than block), but doing nothing is a gap.
| for (const name of Object.keys(manifestResult.value.skills)) { | ||
| const result = await updateSkill(shakaHome, name, options); | ||
| if (!result.ok) return result; | ||
| results.push(result.value); |
There was a problem hiding this comment.
Design: updateAllSkills fails fast, losing partial results
If skill 3 of 5 fails to update (e.g., network error for one repo), the function returns only the error. The caller (CLI at src/commands/skill.ts:131-142) will print only the error, with no indication that skills 1 and 2 were already updated.
Suggestion: Collect all results (success and failure) and return them together, or at minimum log partial progress. The current behavior means a transient network error for one skill kills the entire batch with no visibility into what did succeed.
| async resolveLatestVersion(skill: InstalledSkill): Promise<Result<string, Error>> { | ||
| const parsed = parseGitHubUrl(skill.source); | ||
| if (!parsed.ok) { | ||
| return err( | ||
| new Error(`Invalid stored source for "${skill.source}": ${parsed.error.message}`), | ||
| ); | ||
| } | ||
|
|
||
| const tempDir = join(tmpdir(), `shaka-version-${Date.now()}`); | ||
| await mkdir(tempDir, { recursive: true }); | ||
|
|
||
| try { | ||
| const cloneFn = options.gitClone ?? defaultGitClone; | ||
| const cloneResult = await cloneFn(parsed.value.cloneUrl, tempDir, parsed.value.ref); | ||
| if (!cloneResult.ok) return cloneResult; | ||
|
|
||
| const revParseFn = options.gitRevParse ?? defaultGitRevParse; | ||
| return await revParseFn(tempDir); | ||
| } finally { | ||
| await cleanupTempDir(tempDir); | ||
| } | ||
| }, |
There was a problem hiding this comment.
Bug: resolveLatestVersion clones the full repo just to check the version
This performs a full git clone --depth 1 just to get the HEAD commit SHA. For version checking, git ls-remote would be much cheaper (no file transfer, no disk I/O). This matters because updateAllSkills calls resolveLatestVersion implicitly through fetch for every installed skill — each one clones the repo.
Note: resolveLatestVersion isn't actually called anywhere in the current code — updateSkill calls provider.fetch() directly and compares versions from the fetch result. This method appears to be dead code at the moment. If it's intended for a future "check for updates without fetching" feature, the clone-to-check approach will be noticeably slow.
Suggestion: Either implement with git ls-remote --heads (no clone needed), or remove the method until it's needed. Dead code is a maintenance burden.
| /** Zero-width and bidirectional override characters that can hide content. */ | ||
| const INVISIBLE_CHARS = | ||
| /[\u200B\u200E\u200F\u2028\u2029\u2060\u2066\u2067\u2068\u2069\u206A-\u206F\uFEFF\u00AD]|\u200C|\u200D/; | ||
|
|
||
| async function checkInvisibleChars(skillPath: string): Promise<SecurityCheckEntry> { | ||
| const files = await collectFiles(skillPath); | ||
| const mdFiles = files.filter((f) => f.toLowerCase().endsWith(".md")); | ||
| const flagged: string[] = []; | ||
| for (const file of mdFiles) { | ||
| const content = await Bun.file(join(skillPath, file)).text(); | ||
| if (INVISIBLE_CHARS.test(content)) { | ||
| flagged.push(file); | ||
| } | ||
| } | ||
| return { | ||
| emoji: "\u{1F47B}", | ||
| label: "No invisible chars", | ||
| passed: flagged.length === 0, | ||
| details: flagged, | ||
| failureMessage: "Skill contains invisible unicode characters, make sure to review it properly.", | ||
| }; | ||
| } |
There was a problem hiding this comment.
Nit: INVISIBLE_CHARS regex includes \u200C and \u200D which have legitimate uses
const INVISIBLE_CHARS =
/[\u200B\u200E\u200F\u2028\u2029\u2060\u2066\u2067\u2068\u2069\u206A-\u206F\uFEFF\u00AD]|\u200C|\u200D/;\u200C (ZWNJ) and \u200D (ZWJ) are required for correct rendering in many non-Latin scripts (Arabic, Hindi, etc.). Flagging these could produce false positives for skills that include text in those languages.
This is minor since the check only flags (doesn't block when onSecurityCheck is provided), but worth noting in a comment.
Suggestion: Add a comment noting the false-positive risk, or separate the "suspicious" characters (ZWSP, bidi overrides) from the "linguistically legitimate" ones (ZWNJ, ZWJ).
| async function readLine(): Promise<string> { | ||
| return new Promise((resolve) => { | ||
| process.stdin.setEncoding("utf-8"); | ||
| const timeout = setTimeout(() => { | ||
| process.stdin.pause(); | ||
| resolve(""); | ||
| }, 30000); | ||
| process.stdin.once("data", (chunk) => { | ||
| clearTimeout(timeout); | ||
| process.stdin.pause(); | ||
| resolve(chunk.toString().trim()); | ||
| }); | ||
| }); | ||
| } |
There was a problem hiding this comment.
Nit: readLine in skill.ts doesn't resume stdin properly
async function readLine(): Promise<string> {
return new Promise((resolve) => {
process.stdin.setEncoding("utf-8");
const timeout = setTimeout(() => {
process.stdin.pause();
resolve("");
}, 30000);
process.stdin.once("data", (chunk) => {
clearTimeout(timeout);
process.stdin.pause();
resolve(chunk.toString().trim());
});
});
}process.stdin.setEncoding is called every time but never unset. The pause() is called but there's no matching resume() before the listener is set. This works in practice because Node auto-resumes readable streams when a data listener is added, but the intent isn't clear from reading the code.
More importantly: the 30-second timeout silently resolves with "", which the caller at line 93 treats as "y" (confirmation):
const answer = await readLine();
return answer === "" || answer.toLowerCase() === "y";So if the user walks away from the terminal for 30 seconds during a security prompt, the skill gets installed. This seems unintentional — a timeout should probably mean "no" (abort), not "yes" (install).
Suggestion: On timeout, resolve with something that triggers abort:
const timeout = setTimeout(() => {
process.stdin.pause();
resolve("n"); // Timeout = abort, not confirm
}, 30000);| function extname(path: string): string { | ||
| const dot = path.lastIndexOf("."); | ||
| if (dot <= 0 || dot === path.length - 1) return ""; | ||
| return path.slice(dot).toLowerCase(); | ||
| } |
There was a problem hiding this comment.
Nit: extname reimplemented when path.extname exists
function extname(path: string): string {
const dot = path.lastIndexOf(".");
if (dot <= 0 || dot === path.length - 1) return "";
return path.slice(dot).toLowerCase();
}Node's path.extname does the same thing (minus the .toLowerCase() call). Using the stdlib would be one less thing to maintain.
Suggestion: import { extname } from "node:path" and call .toLowerCase() on the result.
| function parseFrontmatter(content: string): Record<string, string> | null { | ||
| const match = content.match(/^---\n([\s\S]*?)\n---/); | ||
| if (!match) return null; | ||
|
|
||
| const result: Record<string, string> = {}; | ||
| const lines = (match[1] as string).split("\n"); | ||
| for (const line of lines) { | ||
| const colonIdx = line.indexOf(":"); | ||
| if (colonIdx > 0) { | ||
| const key = line.slice(0, colonIdx).trim(); | ||
| const value = line.slice(colonIdx + 1).trim(); | ||
| result[key] = value; | ||
| } | ||
| } | ||
| return result; | ||
| } |
There was a problem hiding this comment.
Nit: parseFrontmatter in skill-pipeline.ts is fragile
function parseFrontmatter(content: string): Record<string, string> | null {
const match = content.match(/^---\n([\s\S]*?)\n---/);
if (!match) return null;
const result: Record<string, string> = {};
const lines = (match[1] as string).split("\n");
for (const line of lines) {
const colonIdx = line.indexOf(":");
if (colonIdx > 0) {
const key = line.slice(0, colonIdx).trim();
const value = line.slice(colonIdx + 1).trim();
result[key] = value;
}
}
return result;
}This hand-rolled YAML parser only handles key: value on a single line. If a SKILL.md has multi-line values, quoted values, or nested YAML, it will silently drop or mangle them. The format-reminder.ts hook already imports yaml (import { parse as parseYaml } from "yaml"), so the dependency exists in the project.
This is fine for now since only name is needed, but it's the kind of parser that quietly breaks when someone adds a description: "A skill: with colons" to their SKILL.md (the value would be "A skill instead of A skill: with colons).
Suggestion: Either document the limitations ("only supports simple key: value, no YAML features") or use the real YAML parser that's already a dependency.
There was a problem hiding this comment.
We should use clawhub instead of clawdhub everywhere
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
📝 WalkthroughWalkthroughThis pull request introduces a comprehensive multi-provider skill management system for Shaka, including CLI commands for install/remove/update/list operations, pluggable skill source providers (GitHub and Clawdhub), security scanning, manifest-based tracking, and cross-platform improvements with extensive test coverage. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as CLI<br/>skill install
participant Service as Install<br/>Service
participant Provider as Provider<br/>(GitHub/<br/>Clawdhub)
participant Security as Security<br/>Checks
participant Linker as Skill<br/>Linker
participant Configurer as Provider<br/>Configurer
participant Manifest as Manifest<br/>Manager
User->>CLI: shaka skill install user/repo
CLI->>Service: installSkill(input, options)
Service->>Provider: detectProvider(input)
Provider-->>Service: SkillSourceProvider
Service->>Provider: fetch(input)
Provider-->>Service: FetchResult{skillDir}
Service->>Security: runSecurityChecks(skillDir)
Security-->>Service: SecurityReport
Service->>Security: onSecurityCheck callback
Security-->>Service: approved
Service->>Service: validateSkillStructure(skillDir)
Service->>Service: checkNameCollision(name)
Service->>Service: installSkillFiles(skillDir, shakaHome)
Service->>Manifest: persistToManifest(skill)
Manifest-->>Service: InstalledSkill
Service->>Linker: linkSkillToProviders(skillName)
Linker->>Configurer: installPerSkillSymlinks
Configurer-->>Linker: symlinks created
Service-->>CLI: {name, skill}
CLI-->>User: ✓ Skill installed
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@src/commands/skill.ts`:
- Around line 214-227: The readLine function may never receive input if
process.stdin is paused; modify readLine to call process.stdin.resume() before
attaching the once("data") listener and before setting the timeout, ensure you
still call process.stdin.pause() after resolving, and keep the existing
encoding, timeout, and clearTimeout logic so readLine behaves correctly even
when stdin starts paused.
In `@src/services/skill-install-service.ts`:
- Around line 264-276: checkNameCollision currently ignores loadManifest
failures which allows partial installs; update the function so that after
calling loadManifest(shakaHome) you return the manifest error when manifest.ok
is false (e.g., return err(manifest.error)) instead of proceeding, and only
check manifest.value.skills when manifest.ok is true; ensure you reference the
existing function name checkNameCollision and the loadManifest result variable
manifest so the manifest failure is propagated to the caller (preventing later
persistToManifest failures).
- Around line 136-153: In scanForExecutableContent, stop classifying
extensionless files (ext === "") automatically as safe; instead introduce a
SAFE_BASENAMES allowlist (e.g., SAFE_BASENAMES set like ".gitignore", "README"
or other approved names) and use path.basename(relativePath) to check it—if
basename is in SAFE_BASENAMES treat as safe, otherwise treat extensionless files
as unknown; update the conditional that currently checks
SAFE_EXTENSIONS.has(ext) || ext === "" to first check SAFE_EXTENSIONS.has(ext),
then if ext === "" consult SAFE_BASENAMES.contains(basename) to decide between
result.safe and result.unknown, keeping EXECUTABLE_EXTENSIONS handling
unchanged.
- Around line 79-127: The install flow in installSkill can throw from
installSkillFiles or linkSkillToProviders (and persistToManifest may fail),
violating the Result contract and leaving partial installs; wrap the
deployment/persist steps in a try/catch, on any exception or non-ok Result
perform a rollback (e.g., call a new helper like rollbackInstalledSkill or
removeInstalledFiles that uses shakaHome and
validation.value.name/fetchResult.value.skillDir to unlink and delete installed
files and undo provider links), and return an err(...) Result instead of
throwing; ensure persistToManifest is only called after successful install/link
and that all early exits still return Result values.
In `@src/services/skill-source/clawdhub.ts`:
- Around line 143-149: The current use of Bun.$`unzip` is not cross-platform;
replace the unzip call with a platform-aware extraction routine that uses
zipPath and destDir: detect Windows via process.platform === "win32" and on
Windows invoke PowerShell's Expand-Archive (e.g., spawn "powershell" with
-NoProfile -Command Expand-Archive -Path <zipPath> -DestinationPath <destDir>
-Force) capturing errors, while on non-Windows use a Unix-safe extractor (either
Bun.$`unzip` or tar) or, preferably, use a pure-JS library compatible with Bun
(e.g., zip-bun) to extract zipPath into destDir; keep the existing error
handling (return err(new Error(...))) and ensure the finally block still removes
zipPath with rm(zipPath, { force: true }) .catch(() => {}) so cleanup always
runs.
🧹 Nitpick comments (9)
defaults/system/hooks/format-reminder.ts (1)
91-118: Align override wording with actual merge behavior.The comment says later scans “override,” but the code appends agents to an existing capability. Either update the comment or change the logic to replace entries.
✏️ Suggested comment fix
- * Later calls override earlier entries with the same capability key. + * Later calls extend earlier entries with the same capability key.src/services/skill-remove-service.ts (1)
37-38: Consider wrappingunlinkSkillFromProvidersin a try-catch for consistent error handling.If
unlinkSkillFromProvidersthrows (e.g., due to filesystem errors), the exception will propagate uncaught instead of being returned aserr(). This inconsistency could surprise callers expecting aResulttype.♻️ Proposed fix for consistent error handling
// Unlink from providers before removing source directory - await unlinkSkillFromProviders(shakaHome, name); + try { + await unlinkSkillFromProviders(shakaHome, name); + } catch (e) { + return err(new Error(`Failed to unlink skill: ${e instanceof Error ? e.message : String(e)}`)); + }test/unit/services/skill-linker.test.ts (1)
7-57: Test coverage could be expanded to include the happy path.The test suite covers edge cases (missing config, no providers enabled) but doesn't test the primary functionality—actually creating symlinks when providers are enabled. Consider adding a test that:
- Sets up a config with
claude: { enabled: true }- Calls
linkSkillToProviders- Verifies a symlink is created in
testClaudeHome/skills/trelloAlso,
testClaudeHome(Line 9) is created and cleaned up but never used in the current tests, suggesting incomplete test implementation.src/commands/doctor.ts (1)
229-236: Simplify directory existence check.Using
readdirto check directory existence is unconventional and the conditionentries.length >= 0is always true (arrays have non-negative length). Consider usingstatoraccessfor clarity:♻️ Proposed simplification
async function dirExistsOnDisk(path: string): Promise<boolean> { try { - const entries = await readdir(path); - return entries.length >= 0; + const { stat } = await import("node:fs/promises"); + const s = await stat(path); + return s.isDirectory(); } catch { return false; } }Or import
statat the top alongsidereaddirif you want to keep imports consolidated.test/unit/services/skill-source/clawdhub.test.ts (1)
28-35: Remove unused helper function.
fakeFetchSkillWithVersionis defined but never called in the test suite. It appears to be dead code.🗑️ Proposed removal
-/** Fake fetchSkill that returns a specific version (version param or default). */ -function fakeFetchSkillWithVersion(resolvedVersion: string) { - return async (_slug: string, version: string | undefined, destDir: string) => { - await mkdir(destDir, { recursive: true }); - await writeFile(join(destDir, "SKILL.md"), VALID_SKILL_MD); - return ok({ version: version ?? resolvedVersion }); - }; -}src/services/skill-source/registry.ts (1)
44-47: Consider returning a defensive copy or usingas constassertion.
getAllSourceProviders()returns the internal mutable array directly. While the return type isreadonly SkillSourceProvider[], callers could cast and mutate it.♻️ Optional: Return a shallow copy for safety
/** List all registered providers. */ export function getAllSourceProviders(): readonly SkillSourceProvider[] { - return providers; + return [...providers]; }src/services/skill-source/clawdhub.ts (1)
124-130: Consider adding basic response shape validation.The JSON response is cast directly to the expected shape without validation. A malformed registry response could cause runtime errors when accessing
body.latestVersion.version.♻️ Optional: Add minimal validation
const body = (await metaRes.value.json()) as { latestVersion: { version: string } | null; }; - if (!body.latestVersion) { + if (!body.latestVersion?.version) { return err(new Error(`No published version found for "${slug}"`)); }src/commands/skill.ts (1)
146-173: Silent manifest errors inhandleListmay hide issues.If
loadManifestfails (e.g., corrupted JSON), the function silently shows an empty installed skills list without informing the user. Consider logging a warning whenmanifest.okis false.💡 Suggested improvement
const manifest = await loadManifest(shakaHome); - const installedSkills = manifest.ok ? Object.entries(manifest.value.skills) : []; + if (!manifest.ok) { + console.warn(`⚠ Could not load manifest: ${manifest.error.message}`); + } + const installedSkills = manifest.ok ? Object.entries(manifest.value.skills) : [];src/services/skill-update-service.ts (1)
85-100: Consider passing loaded manifest to avoid redundant reads.
updateAllSkillsloads the manifest, then eachupdateSkillcall loads it again. This works correctly but performs redundant I/O. Consider refactoring to share the loaded manifest if performance becomes a concern.
| async function readLine(): Promise<string> { | ||
| return new Promise((resolve) => { | ||
| process.stdin.setEncoding("utf-8"); | ||
| const timeout = setTimeout(() => { | ||
| process.stdin.pause(); | ||
| resolve(""); | ||
| }, 30000); | ||
| process.stdin.once("data", (chunk) => { | ||
| clearTimeout(timeout); | ||
| process.stdin.pause(); | ||
| resolve(chunk.toString().trim()); | ||
| }); | ||
| }); | ||
| } |
There was a problem hiding this comment.
Stdin may need resume() before waiting for data.
If process.stdin is paused (common in Node.js by default), the once("data") listener may never fire. Consider calling process.stdin.resume() before listening.
🔧 Suggested fix
async function readLine(): Promise<string> {
return new Promise((resolve) => {
process.stdin.setEncoding("utf-8");
+ process.stdin.resume();
const timeout = setTimeout(() => {
process.stdin.pause();
resolve("");
}, 30000);
process.stdin.once("data", (chunk) => {
clearTimeout(timeout);
process.stdin.pause();
resolve(chunk.toString().trim());
});
});
}📝 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.
| async function readLine(): Promise<string> { | |
| return new Promise((resolve) => { | |
| process.stdin.setEncoding("utf-8"); | |
| const timeout = setTimeout(() => { | |
| process.stdin.pause(); | |
| resolve(""); | |
| }, 30000); | |
| process.stdin.once("data", (chunk) => { | |
| clearTimeout(timeout); | |
| process.stdin.pause(); | |
| resolve(chunk.toString().trim()); | |
| }); | |
| }); | |
| } | |
| async function readLine(): Promise<string> { | |
| return new Promise((resolve) => { | |
| process.stdin.setEncoding("utf-8"); | |
| process.stdin.resume(); | |
| const timeout = setTimeout(() => { | |
| process.stdin.pause(); | |
| resolve(""); | |
| }, 30000); | |
| process.stdin.once("data", (chunk) => { | |
| clearTimeout(timeout); | |
| process.stdin.pause(); | |
| resolve(chunk.toString().trim()); | |
| }); | |
| }); | |
| } |
🤖 Prompt for AI Agents
In `@src/commands/skill.ts` around lines 214 - 227, The readLine function may
never receive input if process.stdin is paused; modify readLine to call
process.stdin.resume() before attaching the once("data") listener and before
setting the timeout, ensure you still call process.stdin.pause() after
resolving, and keep the existing encoding, timeout, and clearTimeout logic so
readLine behaves correctly even when stdin starts paused.
| export async function installSkill( | ||
| shakaHome: string, | ||
| input: string, | ||
| options: InstallOptions = {}, | ||
| ): Promise<Result<{ name: string; skill: InstalledSkill }, Error>> { | ||
| // Detect or use provided provider | ||
| let provider: SkillSourceProvider; | ||
| if (options.provider) { | ||
| provider = options.provider; | ||
| } else { | ||
| const detected = detectProvider(input); | ||
| if (!detected.ok) return detected; | ||
| provider = detected.value; | ||
| } | ||
|
|
||
| // Fetch from provider | ||
| const fetchResult = await provider.fetch(input, { selectSkill: options.selectSkill }); | ||
| if (!fetchResult.ok) return fetchResult; | ||
|
|
||
| try { | ||
| // Validate SKILL.md | ||
| const validation = await validateSkillStructure(fetchResult.value.skillDir); | ||
| if (!validation.ok) return validation; | ||
|
|
||
| // Security checks (unless --yolo) | ||
| if (!options.yolo) { | ||
| const securityResult = await enforceSecurityChecks( | ||
| fetchResult.value.skillDir, | ||
| validation.value.name, | ||
| options, | ||
| ); | ||
| if (!securityResult.ok) return securityResult; | ||
| } | ||
|
|
||
| // Check for name collision | ||
| const collisionResult = await checkNameCollision(shakaHome, validation.value.name); | ||
| if (!collisionResult.ok) return collisionResult; | ||
|
|
||
| // Deploy, persist, and link to providers | ||
| await installSkillFiles(fetchResult.value.skillDir, shakaHome, validation.value.name); | ||
| await linkSkillToProviders(shakaHome, validation.value.name); | ||
|
|
||
| return await persistToManifest(shakaHome, validation.value.name, { | ||
| source: fetchResult.value.source, | ||
| provider: provider.name, | ||
| version: fetchResult.value.version, | ||
| subdirectory: fetchResult.value.subdirectory, | ||
| installedAt: new Date().toISOString(), | ||
| }); |
There was a problem hiding this comment.
Ensure install stays atomic and always returns Result on failures.
installSkillFiles / linkSkillToProviders can throw, which breaks the Result contract and can leave partially installed files if persistToManifest fails. Consider catching exceptions, returning err(...), and rolling back installed files on failure.
🔧 Suggested fix with rollback and Result consistency
-import { readdir } from "node:fs/promises";
+import { readdir, rm } from "node:fs/promises";
@@
- // Deploy, persist, and link to providers
- await installSkillFiles(fetchResult.value.skillDir, shakaHome, validation.value.name);
- await linkSkillToProviders(shakaHome, validation.value.name);
-
- return await persistToManifest(shakaHome, validation.value.name, {
- source: fetchResult.value.source,
- provider: provider.name,
- version: fetchResult.value.version,
- subdirectory: fetchResult.value.subdirectory,
- installedAt: new Date().toISOString(),
- });
+ // Deploy, persist, and link to providers
+ try {
+ await installSkillFiles(fetchResult.value.skillDir, shakaHome, validation.value.name);
+ await linkSkillToProviders(shakaHome, validation.value.name);
+ } catch (e) {
+ await rm(join(shakaHome, "skills", validation.value.name), {
+ recursive: true,
+ force: true,
+ }).catch(() => {});
+ return err(e instanceof Error ? e : new Error(String(e)));
+ }
+
+ const persistResult = await persistToManifest(shakaHome, validation.value.name, {
+ source: fetchResult.value.source,
+ provider: provider.name,
+ version: fetchResult.value.version,
+ subdirectory: fetchResult.value.subdirectory,
+ installedAt: new Date().toISOString(),
+ });
+ if (!persistResult.ok) {
+ await rm(join(shakaHome, "skills", validation.value.name), {
+ recursive: true,
+ force: true,
+ }).catch(() => {});
+ }
+ return persistResult;🤖 Prompt for AI Agents
In `@src/services/skill-install-service.ts` around lines 79 - 127, The install
flow in installSkill can throw from installSkillFiles or linkSkillToProviders
(and persistToManifest may fail), violating the Result contract and leaving
partial installs; wrap the deployment/persist steps in a try/catch, on any
exception or non-ok Result perform a rollback (e.g., call a new helper like
rollbackInstalledSkill or removeInstalledFiles that uses shakaHome and
validation.value.name/fetchResult.value.skillDir to unlink and delete installed
files and undo provider links), and return an err(...) Result instead of
throwing; ensure persistToManifest is only called after successful install/link
and that all early exits still return Result values.
| export async function scanForExecutableContent(skillPath: string): Promise<ScanResult> { | ||
| const result: ScanResult = { safe: [], executable: [], unknown: [] }; | ||
|
|
||
| const entries = await collectFiles(skillPath); | ||
| for (const relativePath of entries) { | ||
| const ext = extname(relativePath); | ||
|
|
||
| if (SAFE_EXTENSIONS.has(ext) || ext === "") { | ||
| result.safe.push(relativePath); | ||
| } else if (EXECUTABLE_EXTENSIONS.has(ext)) { | ||
| result.executable.push(relativePath); | ||
| } else { | ||
| result.unknown.push(relativePath); | ||
| } | ||
| } | ||
|
|
||
| return result; | ||
| } |
There was a problem hiding this comment.
Extensionless files are treated as safe and can hide executables.
Currently, ext === "" is classified as safe. This can hide scripts like install, .bashrc, or other extensionless executables. Consider treating extensionless files as unknown unless they are on a safe basename allowlist.
🛡️ Safer handling for extensionless files
const SAFE_EXTENSIONS = new Set([".md", ".txt", ".yaml", ".yml", ".json", ".xml", ".csv", ".toml"]);
+const SAFE_BASENAMES = new Set(["README", "LICENSE", "SKILL"]);
@@
for (const relativePath of entries) {
const ext = extname(relativePath);
+ const base = relativePath.split("/").pop() ?? "";
+ const isSafeBasename = SAFE_BASENAMES.has(base.toUpperCase());
- if (SAFE_EXTENSIONS.has(ext) || ext === "") {
+ if (SAFE_EXTENSIONS.has(ext) || isSafeBasename) {
result.safe.push(relativePath);
} else if (EXECUTABLE_EXTENSIONS.has(ext)) {
result.executable.push(relativePath);
} else {
result.unknown.push(relativePath);
}
}🤖 Prompt for AI Agents
In `@src/services/skill-install-service.ts` around lines 136 - 153, In
scanForExecutableContent, stop classifying extensionless files (ext === "")
automatically as safe; instead introduce a SAFE_BASENAMES allowlist (e.g.,
SAFE_BASENAMES set like ".gitignore", "README" or other approved names) and use
path.basename(relativePath) to check it—if basename is in SAFE_BASENAMES treat
as safe, otherwise treat extensionless files as unknown; update the conditional
that currently checks SAFE_EXTENSIONS.has(ext) || ext === "" to first check
SAFE_EXTENSIONS.has(ext), then if ext === "" consult
SAFE_BASENAMES.contains(basename) to decide between result.safe and
result.unknown, keeping EXECUTABLE_EXTENSIONS handling unchanged.
| async function checkNameCollision(shakaHome: string, name: string): Promise<Result<void, Error>> { | ||
| const systemSkillPath = join(shakaHome, "system", "skills", name); | ||
| if (await Bun.file(join(systemSkillPath, "SKILL.md")).exists()) { | ||
| return err(new Error(`Skill "${name}" conflicts with a built-in system skill.`)); | ||
| } | ||
|
|
||
| const manifest = await loadManifest(shakaHome); | ||
| if (manifest.ok && manifest.value.skills[name]) { | ||
| return err(new Error(`Skill "${name}" is already installed. Remove it first or use update.`)); | ||
| } | ||
|
|
||
| return ok(undefined); | ||
| } |
There was a problem hiding this comment.
Handle manifest load failures instead of ignoring them.
If loadManifest fails, the install continues and later persistToManifest is likely to fail after files are already installed. Return the manifest error here to prevent partial installs.
🔧 Suggested fix
const manifest = await loadManifest(shakaHome);
- if (manifest.ok && manifest.value.skills[name]) {
+ if (!manifest.ok) {
+ return err(manifest.error);
+ }
+ if (manifest.value.skills[name]) {
return err(new Error(`Skill "${name}" is already installed. Remove it first or use update.`));
}🤖 Prompt for AI Agents
In `@src/services/skill-install-service.ts` around lines 264 - 276,
checkNameCollision currently ignores loadManifest failures which allows partial
installs; update the function so that after calling loadManifest(shakaHome) you
return the manifest error when manifest.ok is false (e.g., return
err(manifest.error)) instead of proceeding, and only check manifest.value.skills
when manifest.ok is true; ensure you reference the existing function name
checkNameCollision and the loadManifest result variable manifest so the manifest
failure is propagated to the caller (preventing later persistToManifest
failures).
| try { | ||
| await Bun.$`unzip -o ${zipPath} -d ${destDir}`.quiet(); | ||
| } catch (e) { | ||
| return err(new Error(`Failed to extract ZIP: ${e instanceof Error ? e.message : String(e)}`)); | ||
| } finally { | ||
| await rm(zipPath, { force: true }).catch(() => {}); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Windows native unzip command availability default installation
💡 Result:
Windows doesn’t include a standalone unzip command by default, but it does include built-in ways to unzip from the command line on modern Windows:
-
PowerShell:
Expand-Archive(ZIP only)
Expand-Archiveis part of the Microsoft.PowerShell.Archive module, which shipped as a built-in component with PowerShell 5.0 (Windows 10 and WMF 5.0). [1]
Example:Expand-Archive -Path .\file.zip -DestinationPath .\out(also noted as a PS 5.0 feature) [2] -
CMD or PowerShell:
tar(can extract ZIP too, on supported builds)
Windows 10 Insider build 17063 and later includetar.exe(bsdtar/libarchive) that you can run directly from Cmd or PowerShell. [3]
Example:tar -xf file.zip
Practical takeaway: On Windows 10/11, you can normally unzip via Expand-Archive or tar -xf without installing anything extra; on older systems you may need WMF 5.x (for Expand-Archive) or a third-party tool. [1][3]
Sources:
[1] PowerShell Team blog (Archive module shipping with PowerShell 5.0 / Windows 10 / WMF 5.0) (devblogs.microsoft.com)
[2] Richard Siddaway blog (PowerShell 5 zip/unzip cmdlets) (richardspowershellblog.wordpress.com)
[3] Windows Command Line blog (tar included starting Windows 10 Insider build 17063+) (devblogs.microsoft.com)
Citations:
- 1: https://devblogs.microsoft.com/powershell/the-archive-module-is-now-open-source/?utm_source=openai
- 2: https://richardspowershellblog.wordpress.com/2014/10/25/powershell-5-zip-and-unzip/?utm_source=openai
- 3: https://devblogs.microsoft.com/commandline/tar-and-curl-come-to-windows/?utm_source=openai
🌐 Web query:
Bun cross-platform ZIP extraction native support Bunfile API
💡 Result:
Native ZIP extraction in Bun (cross‑platform)
- Bun does not currently ship a built-in, cross-platform ZIP extract API (there’s no
Bun.Zip/Bun.Archive-for-zip equivalent in the official runtime docs). - Bun’s native archive API (
Bun.Archive) is for TAR (including reading/extracting tarballs), not ZIP. (bun.sh)
Practical cross‑platform approaches
- Use a JS library that works in Bun (true cross-platform, no external OS tools):
- Example:
zip-bun(native C bindings; create/read/extract; claims macOS/Linux/Windows support). (socket.dev) - Another option:
@scroogieboy/zip-to-object(works with Node/Deno/Bun; focused on loading ZIP contents into memory). (jsr.io)
- Example:
- Call OS tools via Bun Shell (
$), but this is only “cross-platform” if you branch per OS (e.g.,unzipon macOS/Linux vs PowerShellExpand-Archiveon Windows). Bun Shell itself is cross-platform. (bun.com)
“Bunfile API” clarification
If you mean Bun.file() / BunFile:
Bun.file(path)returns aBunFile, a Bun-onlyBlobsubclass representing a lazily-loaded file on disk. (bun.sh)
If you meant a task/build “Bunfile” (like a Makefile equivalent), tell me what filename/feature you’re referring to (e.g., bunfig.toml, bun run, Bun Shell scripts), and I’ll map it to the right Bun API/docs. (bun.com)
Citations:
- 1: https://bun.sh/docs/runtime/archive?utm_source=openai
- 2: https://socket.dev/npm/package/zip-bun?utm_source=openai
- 3: https://jsr.io/%40scroogieboy/zip-to-object?utm_source=openai
- 4: https://bun.com/docs/runtime/shell?utm_source=openai
- 5: https://bun.sh/docs/api/file-io?utm_source=openai
- 6: https://bun.com/docs/runtime/shell?utm_source=openai
🌐 Web query:
Bun shell unzip compatibility Windows WSL alternative
💡 Result:
unzip isn’t a Windows-standard CLI tool, so a Bun Shell script that runs unzip ... will often work in Linux/macOS/WSL but fail on native Windows unless the user installed an unzip.exe (Info-ZIP, Git for Windows, MSYS2, etc.). Bun Shell is “cross-platform” in syntax, but external commands still need to exist on the host OS. [1]
Windows-native alternatives (no WSL)
1) Use PowerShell’s built-in ZIP extraction
import { $ } from "bun";
await $`powershell -NoProfile -Command Expand-Archive -LiteralPath app.zip -DestinationPath out -Force`;PowerShell’s Expand-Archive is the standard built-in option for .zip. [2]
2) Use tar on Windows (often available)
Many Windows setups include tar.exe (bsdtar/libarchive). If present:
await $`tar -xf app.zip -C out`;3) Use 7-Zip (most compatible for “weird” ZIPs)
If 7-Zip is installed:
await $`7z x app.zip -oout -y`;If you want Linux tool compatibility
Use WSL and install unzip there (e.g., apt install unzip), then run the script inside WSL.
Sources: Bun Shell docs (cross-platform behavior) [1]; PowerShell Expand-Archive usage [2].
[1] (bun.sh)
[2] (howtogeek.com)
Citations:
- 1: https://bun.sh/docs/runtime/shell?utm_source=openai
- 2: https://www.howtogeek.com/670314/how-to-zip-and-unzip-files-using-powershell/?utm_source=openai
Replace unzip with a cross-platform approach; native Windows lacks this command.
Windows does not include unzip by default (nor do Expand-Archive or tar substitute for all ZIP formats). The code will fail on native Windows unless users install unzip via Git Bash, WSL, or MSYS2. Bun does not provide a built-in cross-platform ZIP extraction API. Use either:
- PowerShell's built-in
Expand-Archivefor Windows and a platform-specific fallback for Unix systems, or - A cross-platform JavaScript library (e.g.,
zip-bun) that works in Bun without depending on OS-level tools.
🤖 Prompt for AI Agents
In `@src/services/skill-source/clawdhub.ts` around lines 143 - 149, The current
use of Bun.$`unzip` is not cross-platform; replace the unzip call with a
platform-aware extraction routine that uses zipPath and destDir: detect Windows
via process.platform === "win32" and on Windows invoke PowerShell's
Expand-Archive (e.g., spawn "powershell" with -NoProfile -Command Expand-Archive
-Path <zipPath> -DestinationPath <destDir> -Force) capturing errors, while on
non-Windows use a Unix-safe extractor (either Bun.$`unzip` or tar) or,
preferably, use a pure-JS library compatible with Bun (e.g., zip-bun) to extract
zipPath into destDir; keep the existing error handling (return err(new
Error(...))) and ensure the finally block still removes zipPath with rm(zipPath,
{ force: true }) .catch(() => {}) so cleanup always runs.
2 participants
This PR adds a skill management that symlink to providers enabled.
Currently fetches root SKILL.md, marketplace ".claude/skills//SKILL.md" install with
shaka skill install <repo>Or you can use ClawHub
shaka skill install <name>you can force also with--githubor--clawhubwhats the source of the skill.Summary by CodeRabbit
New Features
Improvements
Documentation