From b95595f0c7ed9769bad0fdb1e64727ff0dc34284 Mon Sep 17 00:00:00 2001 From: Yichen Jiang Date: Mon, 6 Jul 2026 10:21:00 +0800 Subject: [PATCH] fix: harden runtime skill registration --- docs/core-data-structures/skills.md | 2 +- packages/core/skill/README.md | 6 ++-- packages/core/skill/src/index.ts | 19 ++++++---- packages/core/skill/tests/skill.spec.ts | 46 ++++++++++++++++++++++++- 4 files changed, 62 insertions(+), 11 deletions(-) diff --git a/docs/core-data-structures/skills.md b/docs/core-data-structures/skills.md index a7eabacf2d..51681bcfe2 100644 --- a/docs/core-data-structures/skills.md +++ b/docs/core-data-structures/skills.md @@ -71,7 +71,7 @@ interface SkillLookupOptions { } ``` -The service can be pointed at alternate user roots in tests or deployments. `installSystemSkills` controls whether bundled system skills are materialized under `/skills/.system` on startup. +The service can be pointed at alternate user roots in tests or deployments. `installSystemSkills` controls whether bundled system skills are materialized under `/skills/.system` on startup. `promptFieldMaxLength` must be at least `3`, matching the `...` truncation suffix reserved in rendered prompt fields. ```ts type-equiv interface Config { diff --git a/packages/core/skill/README.md b/packages/core/skill/README.md index a91c3f116d..84c3e8bba6 100644 --- a/packages/core/skill/README.md +++ b/packages/core/skill/README.md @@ -8,7 +8,7 @@ Agent skill discovery and model-facing skill guidance. - `ctx.skills.list({ cwd? })` Returns model-invocable skill summaries for the current workspace. - `ctx.skills.get(name, { cwd? })` Returns the full skill, including disabled-for-model skills. -- `ctx.skills.register(skill): () => void` Registers a runtime skill, disposed with the calling fiber. +- `ctx.skills.register(skill): () => void` Registers a runtime skill, disposed with the calling fiber. Same-name runtime registrations are first-wins: a duplicate logs a warning and gets a no-op disposer. ### Config @@ -18,7 +18,7 @@ Agent skill discovery and model-facing skill guidance. | `agentsHome` | `$DSH_AGENTS_HOME` or `~/.agents` | Shared agent config root scanned for compatible skills. | | `extraRoots` | `[]` | Additional skill roots scanned after user roots and before system skills. | | `installSystemSkills` | `true` | Whether startup materializes bundled system skills under `dshHome`. | -| `promptFieldMaxLength` | `500` | Maximum rendered `description` / `whenToUse` length in the prompt listing. | +| `promptFieldMaxLength` | `500` | Maximum rendered `description` / `whenToUse` length in the prompt listing; must be at least `3` because truncated fields reserve `...`. | | `collectCacheMaxEntries` | `128` | Maximum cwd/root discovery promises kept in memory. | ### Discovery @@ -39,7 +39,7 @@ The project root is the nearest ancestor containing `.git`; without one, the cur When `ctx.fs` is available, discovery lists roots through `ctx.fs.listDir`, reads skill files through `ctx.fs.readText`, and installs system skills through `ctx.fs.writeText`. Without a filesystem service, the package falls back to Node filesystem I/O so the service can still run in minimal test contexts. Missing, unreadable, or malformed skill files warn and skip instead of failing the whole request. -Discovery is memoized per resolved root set and runtime-skill revision. Runtime `register()` and disposer calls invalidate the cache; disk-only changes are picked up on the next invalidation or process restart. +Discovery is memoized per resolved root set and runtime-skill revision. Runtime `register()` and active disposer calls invalidate the cache; duplicate runtime registrations do not alter the active set. Disk-only changes are picked up on the next invalidation or process restart. ## Skill Format diff --git a/packages/core/skill/src/index.ts b/packages/core/skill/src/index.ts index 503057b8bd..e50f82d488 100644 --- a/packages/core/skill/src/index.ts +++ b/packages/core/skill/src/index.ts @@ -75,7 +75,7 @@ export interface Config { extraRoots?: string[] /** Ensure bundled system skills exist under `/skills/.system`. Defaults true. */ installSystemSkills?: boolean - /** Maximum rendered description/whenToUse length in the prompt listing. */ + /** Maximum rendered description/whenToUse length in the prompt listing; minimum 3. */ promptFieldMaxLength?: number /** Maximum number of cwd/root discovery promises kept in the in-memory cache. */ collectCacheMaxEntries?: number @@ -159,7 +159,7 @@ export class SkillService extends Service { this.installSystemSkills = config.installSystemSkills ?? true this.promptFieldMaxLength = config.promptFieldMaxLength ?? DEFAULT_PROMPT_FIELD_LENGTH this.collectCacheMaxEntries = config.collectCacheMaxEntries ?? DEFAULT_COLLECT_CACHE_ENTRIES - assertPositiveInteger('promptFieldMaxLength', this.promptFieldMaxLength) + assertPositiveInteger('promptFieldMaxLength', this.promptFieldMaxLength, 3) assertPositiveInteger('collectCacheMaxEntries', this.collectCacheMaxEntries) if (this.installSystemSkills) { const systemRoot = join(this.dshHome, 'skills/.system') @@ -178,11 +178,18 @@ export class SkillService extends Service { /** * Register a runtime skill contribution. + * Same-name runtime registrations are first-wins: a duplicate logs a warning + * and returns a no-op disposer so it cannot remove the active contribution. * @param skill - the complete skill definition to expose for discovery. - * @returns a disposer that removes the runtime skill and invalidates caches. + * @returns a disposer that removes this runtime contribution and invalidates caches. */ register(skill: SkillRegistration): () => void { const normalized = normalizeSkill(skill) + const existing = this.runtime.get(normalized.name) + if (existing !== undefined) { + this.ctx.logger.warn(`runtime skill "${normalized.name}" from ${normalized.source} ignored because it is already registered from ${existing.source}`) + return () => {} + } const dispose = this.ctx.effect(function* (this: SkillService) { this.runtime.set(normalized.name, normalized) this.invalidateCache() @@ -587,9 +594,9 @@ function promptLine(value: string, maxLength: number): string { return escapeText(truncated) } -function assertPositiveInteger(name: string, value: number): void { - if (!Number.isInteger(value) || value < 1) { - throw new Error(`skill: ${name} must be a positive integer`) +function assertPositiveInteger(name: string, value: number, minimum = 1): void { + if (!Number.isInteger(value) || value < minimum) { + throw new Error(`skill: ${name} must be an integer greater than or equal to ${minimum}`) } } diff --git a/packages/core/skill/tests/skill.spec.ts b/packages/core/skill/tests/skill.spec.ts index 183954ccdf..aed07a563b 100644 --- a/packages/core/skill/tests/skill.spec.ts +++ b/packages/core/skill/tests/skill.spec.ts @@ -1,4 +1,4 @@ -import { describe, expect, it } from 'vitest' +import { describe, expect, it, vi } from 'vitest' import { mkdir, readdir, readFile, stat, symlink, writeFile } from 'node:fs/promises' import { dirname, join } from 'node:path' import { tmpdir } from 'node:os' @@ -331,6 +331,12 @@ describe('SkillService', () => { installSystemSkills: false, promptFieldMaxLength: 0, })).rejects.toThrow('promptFieldMaxLength') + await expect(ctx.plugin(SkillService, { + dshHome: join(home, '.dsh'), + agentsHome: join(home, '.agents'), + installSystemSkills: false, + promptFieldMaxLength: 2, + })).rejects.toThrow('greater than or equal to 3') await expect(ctx.plugin(SkillService, { dshHome: join(home, '.dsh'), agentsHome: join(home, '.agents'), @@ -661,6 +667,44 @@ describe('SkillService', () => { expect(await ctx.skills.list()).toEqual([]) }) + it('keeps the first runtime skill when a duplicate name is registered', async () => { + const home = await tempDir('skill-runtime-duplicate') + const ctx = new Context() + const warn = vi.spyOn(ctx.logger, 'warn').mockImplementation(() => undefined) + await ctx.plugin(SkillService, { dshHome: join(home, '.dsh'), agentsHome: join(home, '.agents'), installSystemSkills: false }) + + const firstDispose = ctx.skills.register({ + name: 'same-runtime', + description: 'first', + content: 'First body.', + directory: 'memory://first', + source: 'runtime', + }) + const duplicateDispose = ctx.skills.register({ + name: 'same-runtime', + description: 'second', + content: 'Second body.', + directory: 'memory://second', + source: 'runtime', + }) + + await expect(ctx.skills.get('same-runtime')).resolves.toMatchObject({ + description: 'first', + content: 'First body.', + directory: 'memory://first', + }) + expect(warn).toHaveBeenCalledWith(expect.stringContaining('runtime skill "same-runtime"')) + + duplicateDispose() + await expect(ctx.skills.get('same-runtime')).resolves.toMatchObject({ + description: 'first', + content: 'First body.', + }) + + firstDispose() + await expect(ctx.skills.get('same-runtime')).resolves.toBeUndefined() + }) + it('rejects invalid runtime skill registrations', async () => { const home = await tempDir('skill-runtime-invalid') const ctx = new Context()