From 33f39ab44828dd553e5b4d80c8dc2dde93cf1fd5 Mon Sep 17 00:00:00 2001 From: Suleiman Shahbari Date: Fri, 26 Jun 2026 22:41:07 +0300 Subject: [PATCH] refactor(ai-skills): harden discovery + validation, dedupe fs helpers, sharpen docs Code quality + docs pass for @gemstack/ai-skills: - discover() now skips an unreadable or malformed SKILL.md instead of failing the entire scan, with an optional onError hook to observe what was skipped. This restores the documented "index hundreds of skills safely, run no skill code" contract that a single bad bundle could break. - Skill name is validated at parse time against [a-zA-Z0-9_-] rather than being silently mangled later at compose time, so bad names fail fast. - The undiscovered-name error now lists available skills / hints to call discover() first; loadSkill() separates a missing SKILL.md from an unreadable one (ENOENT vs other errors). - Deduplicated fileExists/isDirectory into a shared fs-utils module. - README: tools module is loaded compiled (tools.js/.mjs/.cjs, not tools.ts); added a direct loadSkill() example and the onError note. Added tests for discovery resilience and name validation. 37 tests pass. --- .changeset/ai-skills-quality-pass.md | 12 ++++++++ packages/ai-skills/README.md | 23 ++++++++++++-- packages/ai-skills/src/fs-utils.ts | 11 +++++++ packages/ai-skills/src/index.ts | 2 +- packages/ai-skills/src/loader.ts | 17 ++++------ packages/ai-skills/src/manifest.test.ts | 12 ++++++++ packages/ai-skills/src/manifest.ts | 4 ++- packages/ai-skills/src/registry.test.ts | 28 +++++++++++++++++ packages/ai-skills/src/registry.ts | 41 ++++++++++++++++++++----- 9 files changed, 127 insertions(+), 23 deletions(-) create mode 100644 .changeset/ai-skills-quality-pass.md create mode 100644 packages/ai-skills/src/fs-utils.ts 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 } -}