diff --git a/.changeset/ai-skills-quality-pass.md b/.changeset/ai-skills-quality-pass.md new file mode 100644 index 0000000..a099b86 --- /dev/null +++ b/.changeset/ai-skills-quality-pass.md @@ -0,0 +1,12 @@ +--- +"@gemstack/ai-skills": minor +--- + +Quality + docs pass for ai-skills: + +- `SkillRegistry.discover()` no longer aborts the whole scan when a single `SKILL.md` is unreadable or malformed; the bad bundle is skipped and the rest are still indexed. Pass `discover(root, { onError })` to observe what was skipped. This restores the documented "index hundreds of skills safely" contract. +- Skill `name` is now validated at parse time against `[a-zA-Z0-9_-]` instead of being silently mangled later at compose time, so invalid names fail fast with a clear message. +- `load()` on an undiscovered name now lists the available skill names (or hints to call `discover()` first). +- `loadSkill()` distinguishes a missing `SKILL.md` from an unreadable one. +- Deduplicated the internal `fileExists`/`isDirectory` helpers into a shared module. +- README: clarified that the tools module is loaded compiled (`tools.js`/`.mjs`/`.cjs`, not `tools.ts`), and added a direct `loadSkill()` usage example. diff --git a/packages/ai-skills/README.md b/packages/ai-skills/README.md index f4d2856..47eca4e 100644 --- a/packages/ai-skills/README.md +++ b/packages/ai-skills/README.md @@ -5,10 +5,16 @@ Portable capability bundles for [`@gemstack/ai-sdk`](https://github.com/gemstack ``` my-skill/ SKILL.md # YAML frontmatter (name, description, trigger, ...) + markdown instructions - tools.ts # optional: exports ai-sdk tool() objects + tools.ts # optional: exports ai-sdk tool() objects (loaded compiled, see note below) resources/ # optional: reference files ``` +> The loader imports the skill's tools module at runtime, so it resolves the +> *compiled* output (`tools.js` / `tools.mjs` / `tools.cjs`), not `tools.ts`. +> Author in TypeScript and build the skill folder, or ship the compiled file +> alongside `SKILL.md`. The `SKILL.md` instructions and `resources/` stay +> portable as-is; only the typed tools module needs a build step. + ## Installation ```bash @@ -98,7 +104,20 @@ const registry = new SkillRegistry() await registry.discover('./skills') // reads frontmatter only, runs no skill code registry.list() // [{ manifest, dir }, ...] -const refunds = await registry.load('refunds') // now imports tools.ts +const refunds = await registry.load('refunds') // now imports the compiled tools module +``` + +A malformed or unreadable `SKILL.md` is skipped rather than failing the whole scan; pass `discover(root, { onError })` to observe what was skipped. + +If you only need a skill's parts (not a full agent), `loadSkill` returns them directly: + +```ts +import { loadSkill } from '@gemstack/ai-skills' + +const refunds = await loadSkill('./skills/refunds') +refunds.instructions // markdown body (string) +refunds.tools // ai-sdk tool() objects +refunds.resources // [{ name, path }, ...] ``` ## Trust model diff --git a/packages/ai-skills/src/fs-utils.ts b/packages/ai-skills/src/fs-utils.ts new file mode 100644 index 0000000..300db86 --- /dev/null +++ b/packages/ai-skills/src/fs-utils.ts @@ -0,0 +1,11 @@ +import { stat } from 'node:fs/promises' + +/** True if `path` exists and is a regular file. Never throws. */ +export async function fileExists(path: string): Promise { + try { return (await stat(path)).isFile() } catch { return false } +} + +/** True if `path` exists and is a directory. Never throws. */ +export async function isDirectory(path: string): Promise { + try { return (await stat(path)).isDirectory() } catch { return false } +} diff --git a/packages/ai-skills/src/index.ts b/packages/ai-skills/src/index.ts index 11dd938..545d500 100644 --- a/packages/ai-skills/src/index.ts +++ b/packages/ai-skills/src/index.ts @@ -12,7 +12,7 @@ */ export { parseSkillManifest, SkillManifestError } from './manifest.js' export { loadSkill, loadSkills, type LoadSkillOptions } from './loader.js' -export { SkillRegistry, type SkillIndexEntry } from './registry.js' +export { SkillRegistry, type SkillIndexEntry, type DiscoverOptions } from './registry.js' export { composeInstructions, composeTools, diff --git a/packages/ai-skills/src/loader.ts b/packages/ai-skills/src/loader.ts index 3b87fb0..915bb5f 100644 --- a/packages/ai-skills/src/loader.ts +++ b/packages/ai-skills/src/loader.ts @@ -1,8 +1,9 @@ -import { readFile, readdir, stat } from 'node:fs/promises' +import { readFile, readdir } from 'node:fs/promises' import { join, basename } from 'node:path' import { pathToFileURL } from 'node:url' import type { AnyTool } from '@gemstack/ai-sdk' import { parseSkillManifest, SkillManifestError } from './manifest.js' +import { fileExists, isDirectory } from './fs-utils.js' import type { LoadedSkill, SkillResource } from './types.js' /** Candidate filenames for a skill's co-located tools module, in priority order. */ @@ -33,8 +34,10 @@ export async function loadSkill(dir: string, opts: LoadSkillOptions = {}): Promi let markdown: string try { markdown = await readFile(skillPath, 'utf8') - } catch { - throw new SkillManifestError(`no SKILL.md found in ${dir}`, dir) + } catch (err) { + const code = (err as NodeJS.ErrnoException).code + const reason = code === 'ENOENT' ? `no SKILL.md found in ${dir}` : `could not read ${skillPath}: ${(err as Error).message}` + throw new SkillManifestError(reason, dir) } const { manifest, instructions } = parseSkillManifest(markdown, skillPath) @@ -104,11 +107,3 @@ async function loadSkillResources(dir: string): Promise { .map(e => ({ name: basename(e.name), path: join(resourceDir, e.name) })) .sort((a, b) => a.name.localeCompare(b.name)) } - -async function fileExists(path: string): Promise { - try { return (await stat(path)).isFile() } catch { return false } -} - -async function isDirectory(path: string): Promise { - try { return (await stat(path)).isDirectory() } catch { return false } -} diff --git a/packages/ai-skills/src/manifest.test.ts b/packages/ai-skills/src/manifest.test.ts index cb7588c..72ebd39 100644 --- a/packages/ai-skills/src/manifest.test.ts +++ b/packages/ai-skills/src/manifest.test.ts @@ -50,6 +50,18 @@ describe('parseSkillManifest', () => { assert.throws(() => parseSkillManifest(md), SkillManifestError) }) + it('rejects a name with characters that would be silently mangled at compose time', () => { + const md = `---\nname: foo.bar\ndescription: y\n---\nbody` + assert.throws( + () => parseSkillManifest(md), + (e: unknown) => e instanceof SkillManifestError && /letters, numbers, hyphens/.test((e as Error).message), + ) + }) + + it('accepts kebab-case and underscored names', () => { + assert.equal(parseSkillManifest(`---\nname: foo-bar_baz\ndescription: y\n---\n`).manifest.name, 'foo-bar_baz') + }) + it('tolerates and drops unknown frontmatter keys', () => { const md = `---\nname: x\ndescription: y\nfutureField: ignored\n---\nbody` const { manifest } = parseSkillManifest(md) diff --git a/packages/ai-skills/src/manifest.ts b/packages/ai-skills/src/manifest.ts index 4358c06..6acaabd 100644 --- a/packages/ai-skills/src/manifest.ts +++ b/packages/ai-skills/src/manifest.ts @@ -8,7 +8,9 @@ import type { ParsedSkill, SkillManifest } from './types.js' * escape hatch for author-defined fields that should survive. */ const manifestSchema = z.object({ - name: z.string().min(1, 'skill name is required'), + name: z.string() + .min(1, 'skill name is required') + .regex(/^[a-zA-Z0-9_-]+$/, 'skill name may only contain letters, numbers, hyphens, and underscores'), description: z.string().min(1, 'skill description is required'), license: z.string().optional(), appliesTo: z.array(z.string()).optional(), diff --git a/packages/ai-skills/src/registry.test.ts b/packages/ai-skills/src/registry.test.ts index fd7a6f6..d7202d8 100644 --- a/packages/ai-skills/src/registry.test.ts +++ b/packages/ai-skills/src/registry.test.ts @@ -75,4 +75,32 @@ describe('SkillRegistry', () => { const skills = await registry.loadAll(['beta', 'alpha']) assert.deepEqual(skills.map(s => s.manifest.name), ['beta', 'alpha']) }) + + it('skips a malformed SKILL.md instead of failing the whole scan', async () => { + const dir = await mkdtemp(join(tmpdir(), 'ai-skills-bad-')) + try { + await writeSkill(join(dir, 'good'), 'good') + // a SKILL.md with no frontmatter fence — parsing it throws + await mkdir(join(dir, 'broken'), { recursive: true }) + await writeFile(join(dir, 'broken', 'SKILL.md'), 'no frontmatter here') + + const registry = new SkillRegistry() + const errors: string[] = [] + const found = await registry.discover(dir, { + onError: (_err, path) => errors.push(path), + }) + + assert.deepEqual(found.map(e => e.manifest.name), ['good']) + assert.equal(errors.length, 1) + assert.ok(errors[0]!.endsWith(join('broken', 'SKILL.md'))) + } finally { + await rm(dir, { recursive: true, force: true }) + } + }) + + it('lists available skill names in the undiscovered-name error', async () => { + const registry = new SkillRegistry() + await registry.discover(root) + await assert.rejects(() => registry.load('ghost'), /available: .*alpha/) + }) }) diff --git a/packages/ai-skills/src/registry.ts b/packages/ai-skills/src/registry.ts index 3691950..067fa3a 100644 --- a/packages/ai-skills/src/registry.ts +++ b/packages/ai-skills/src/registry.ts @@ -1,7 +1,8 @@ -import { readFile, readdir, stat } from 'node:fs/promises' +import { readFile, readdir } from 'node:fs/promises' import { join } from 'node:path' import { parseSkillManifest } from './manifest.js' import { loadSkill, type LoadSkillOptions } from './loader.js' +import { fileExists } from './fs-utils.js' import type { LoadedSkill, SkillManifest } from './types.js' /** A discovered-but-not-yet-loaded skill: its manifest + where it lives. */ @@ -10,6 +11,16 @@ export interface SkillIndexEntry { dir: string } +/** Options for {@link SkillRegistry.discover}. */ +export interface DiscoverOptions { + /** + * Invoked when a candidate `SKILL.md` cannot be read or parsed. Discovery + * always skips the offending entry and continues scanning the rest; this hook + * lets you observe (log, collect) the skipped entries. Omit to skip silently. + */ + onError?: (error: unknown, skillPath: string) => void +} + /** * Discovers `SKILL.md` bundles under a set of root directories and loads them * on demand. Discovery parses only the cheap frontmatter (the manifest), so @@ -30,8 +41,14 @@ export class SkillRegistry { * indexing each by manifest `name`. Returns the entries found in this scan. * A later scan with a duplicate name overrides the earlier entry (last wins), * mirroring how registered/allowlisted sources layer. + * + * A single unreadable or malformed `SKILL.md` is skipped (not fatal) so one + * bad bundle in a tree cannot break discovery of the rest; pass + * {@link DiscoverOptions.onError} to observe what was skipped. This preserves + * the "index hundreds of skills cheaply" contract: discovery only reads + * frontmatter and never executes skill code. */ - async discover(root: string): Promise { + async discover(root: string, opts: DiscoverOptions = {}): Promise { const found: SkillIndexEntry[] = [] let subdirs: string[] try { @@ -44,7 +61,13 @@ export class SkillRegistry { for (const dir of subdirs) { const skillPath = join(dir, 'SKILL.md') if (!(await fileExists(skillPath))) continue - const { manifest } = parseSkillManifest(await readFile(skillPath, 'utf8'), skillPath) + let manifest: SkillManifest + try { + ;({ manifest } = parseSkillManifest(await readFile(skillPath, 'utf8'), skillPath)) + } catch (err) { + opts.onError?.(err, skillPath) + continue + } const entry: SkillIndexEntry = { manifest, dir } this.entries.set(manifest.name, entry) found.push(entry) @@ -72,7 +95,13 @@ export class SkillRegistry { if (cached && !opts.force) return cached const entry = this.entries.get(name) - if (!entry) throw new Error(`[ai-skills] no skill named "${name}" has been discovered`) + if (!entry) { + const available = [...this.entries.keys()] + const hint = available.length + ? `available: ${available.join(', ')}` + : 'no skills indexed yet — call discover() first' + throw new Error(`[ai-skills] no skill named "${name}" has been discovered (${hint})`) + } const skill = await loadSkill(entry.dir, opts) this.loaded.set(name, skill) @@ -84,7 +113,3 @@ export class SkillRegistry { return Promise.all(names.map(name => this.load(name, opts))) } } - -async function fileExists(path: string): Promise { - try { return (await stat(path)).isFile() } catch { return false } -}