From 2943d0303ce1da92b7d954cfd09c7769178a0d7e Mon Sep 17 00:00:00 2001 From: Yichen Jiang Date: Wed, 1 Jul 2026 19:57:52 +0800 Subject: [PATCH] Address skill review findings --- docs/cordis-catalog/events-and-services.md | 2 +- packages/core/skill/src/index.ts | 19 +++++-- packages/core/skill/tests/skill.spec.ts | 66 ++++++++++++++++++++++ 3 files changed, 82 insertions(+), 5 deletions(-) diff --git a/docs/cordis-catalog/events-and-services.md b/docs/cordis-catalog/events-and-services.md index 6fc641db67..0247aaf945 100644 --- a/docs/cordis-catalog/events-and-services.md +++ b/docs/cordis-catalog/events-and-services.md @@ -441,7 +441,7 @@ async get(name: string, options: SkillLookupOptions = {}): Promise ``` -Source: [`packages/core/skill/src/index.ts:106`](../../packages/core/skill/src/index.ts) +Source: [`packages/core/skill/src/index.ts:107`](../../packages/core/skill/src/index.ts) ### `ctx.subagents` — `SubagentService` diff --git a/packages/core/skill/src/index.ts b/packages/core/skill/src/index.ts index 631d84a415..4f6fc4129b 100644 --- a/packages/core/skill/src/index.ts +++ b/packages/core/skill/src/index.ts @@ -18,6 +18,7 @@ import type {} from '@deepseek-ai/dsh-agent' const SKILL_NAME = /^[a-z0-9]+(?:-[a-z0-9]+)*$/ const MAX_PROMPT_FIELD_LENGTH = 500 +const MAX_COLLECT_CACHE_ENTRIES = 128 export function isSkillName(name: string): boolean { return SKILL_NAME.test(name) @@ -189,6 +190,10 @@ export class SkillService extends Service { const collected = this.collectFresh(roots) this.collectCache.set(key, collected) + if (this.collectCache.size > MAX_COLLECT_CACHE_ENTRIES) { + const oldest = this.collectCache.keys().next().value + if (oldest !== undefined) this.collectCache.delete(oldest) + } return collected } @@ -334,10 +339,10 @@ function parseFrontmatter(raw: string): { data: Record; body: s const end = raw.indexOf('\n---', 4) if (end < 0) return undefined const yaml = raw.slice(4, end) - const bodyStart = raw.indexOf('\n', end + 4) const parsed = parseYaml(yaml) as unknown if (typeof parsed !== 'object' || parsed === null || Array.isArray(parsed)) return undefined - return { data: parsed as Record, body: bodyStart < 0 ? '' : raw.slice(bodyStart + 1) } + const body = raw.slice(end + 4) + return { data: parsed as Record, body: body.startsWith('\n') ? body.slice(1) : body } } async function findProjectRoot(cwd: string): Promise { @@ -379,8 +384,10 @@ function compareSummary(left: SkillSummary, right: SkillSummary): number { function promptLine(value: string): string { const normalized = value.replaceAll(/\s+/g, ' ').trim() - if (normalized.length <= MAX_PROMPT_FIELD_LENGTH) return normalized - return `${normalized.slice(0, MAX_PROMPT_FIELD_LENGTH - 3)}...` + const truncated = normalized.length <= MAX_PROMPT_FIELD_LENGTH + ? normalized + : `${normalized.slice(0, MAX_PROMPT_FIELD_LENGTH - 3)}...` + return escapeText(truncated) } function stringField(data: Record, key: string): string | undefined { @@ -410,6 +417,10 @@ function escapeAttr(value: string): string { return value.replaceAll('&', '&').replaceAll('"', '"').replaceAll('<', '<') } +function escapeText(value: string): string { + return value.replaceAll('&', '&').replaceAll('<', '<').replaceAll('>', '>') +} + function collectCacheKey(roots: { project: SkillRoot[]; shared: SkillRoot[] }, runtimeRevision: number): string { return JSON.stringify({ runtimeRevision, roots }) } diff --git a/packages/core/skill/tests/skill.spec.ts b/packages/core/skill/tests/skill.spec.ts index 659e4c8b74..e9d558c6de 100644 --- a/packages/core/skill/tests/skill.spec.ts +++ b/packages/core/skill/tests/skill.spec.ts @@ -128,6 +128,24 @@ describe('SkillService', () => { expect(await ctx.skills.get('Bad_Name')).toBeUndefined() }) + it('keeps skill body text that begins immediately after the closing frontmatter delimiter', async () => { + const home = await tempDir('skill-frontmatter-body') + const root = join(home, '.dsh/skills') + await mkdir(root, { recursive: true }) + await writeFile(join(root, 'tight-body.md'), [ + '---', + 'name: tight-body', + 'description: Tight body', + '---First line must survive.', + 'Second line.', + ].join('\n')) + + const ctx = new Context() + await ctx.plugin(SkillService, { dshHome: join(home, '.dsh'), agentsHome: join(home, '.agents'), installSystemSkills: false }) + + expect((await ctx.skills.get('tight-body'))?.content).toBe('First line must survive.\nSecond line.') + }) + it('renders no model listing when no model-invocable skills exist', async () => { const home = await tempDir('skill-empty-listing') const ctx = new Context() @@ -243,6 +261,29 @@ describe('SkillService', () => { expect((await ctx.skills.get('long-skill'))?.description).toBe(longDescription) }) + it('escapes prompt listing text fields without changing stored skill content', async () => { + const home = await tempDir('skill-prompt-escape') + const root = join(home, '.dsh/skills') + await mkdir(root, { recursive: true }) + await writeFile(join(root, 'escaped-skill.md'), [ + '---', + 'name: escaped-skill', + 'description: Use safely', + 'whenToUse: Handle & marker', + '---', + 'Full body.', + ].join('\n')) + + const ctx = new Context() + await ctx.plugin(SkillService, { dshHome: join(home, '.dsh'), agentsHome: join(home, '.agents'), installSystemSkills: false }) + + const listing = await ctx.skills.renderModelListing() + expect(listing).toContain('description: Use </available_skills><oops> safely') + expect(listing).toContain('whenToUse: Handle <tag> & marker') + expect(listing).not.toContain('description: Use safely') + expect((await ctx.skills.get('escaped-skill'))?.description).toBe('Use safely') + }) + it('adds skill guidance through the agent/request waterfall without including bodies', async () => { const home = await tempDir('skill-guidance') await writeSkill(join(home, '.dsh/skills'), 'research-helper', 'Research helper', 'Long body that must not be listed.') @@ -301,6 +342,31 @@ describe('SkillService', () => { expect(await ctx.skills.list()).toEqual([]) }) + it('bounds discovery cache entries across many project roots', async () => { + const home = await tempDir('skill-cache-bound-home') + const projects = await Promise.all(Array.from({ length: 129 }, async (_, index) => { + const project = await tempDir(`skill-cache-bound-project-${index}`) + await mkdir(join(project, '.git'), { recursive: true }) + await writeSkill(join(project, '.dsh/skills'), `project-${index}`, `Project ${index}`) + return project + })) + + const ctx = new Context() + await ctx.plugin(SkillService, { dshHome: join(home, '.dsh'), agentsHome: join(home, '.agents'), installSystemSkills: false }) + + const firstProject = projects[0] + if (firstProject === undefined) throw new Error('expected at least one project') + expect((await ctx.skills.list({ cwd: firstProject })).map(skill => skill.name)).toEqual(['project-0']) + await writeSkill(join(firstProject, '.dsh/skills'), 'late-project-0', 'Late project 0') + expect((await ctx.skills.list({ cwd: firstProject })).map(skill => skill.name)).toEqual(['project-0']) + + for (const project of projects.slice(1)) { + await ctx.skills.list({ cwd: project }) + } + + expect((await ctx.skills.list({ cwd: firstProject })).map(skill => skill.name)).toEqual(['late-project-0', 'project-0']) + }) + it('removes runtime registered skills when the returned disposer is called', async () => { const home = await tempDir('skill-runtime-disposer') const ctx = new Context()