refactor(ai-skills): harden discovery + validation, dedupe fs helpers, docs

.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.

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

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<boolean> {
+  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<boolean> {
+  try { return (await stat(path)).isDirectory() } catch { return false }
+}

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,

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<SkillResource[]> {
     .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<boolean> {
-  try { return (await stat(path)).isFile() } catch { return false }
-}
-
-async function isDirectory(path: string): Promise<boolean> {
-  try { return (await stat(path)).isDirectory() } catch { return false }
-}

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)

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(),

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/)
+  })
 })

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<SkillIndexEntry[]> {
+  async discover(root: string, opts: DiscoverOptions = {}): Promise<SkillIndexEntry[]> {
     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<boolean> {
-  try { return (await stat(path)).isFile() } catch { return false }
-}