From 36a4ff9a2eacbf244f992c916cf28d28cabffac8 Mon Sep 17 00:00:00 2001 From: fromsko <1614355756@qq.com> Date: Wed, 22 Jul 2026 15:48:50 +0800 Subject: [PATCH] fix(agent-core): surface skill parse failures as session warnings A skill file that fails to parse (malformed YAML frontmatter, missing required fields, unsupported type) was silently dropped during session start with no user-visible feedback. The SessionSkillRegistry did not wire its onWarning callback, and getSessionWarnings only checked the AGENTS.md size warning, so skill load failures never reached the TUI. SessionSkillRegistry now collects parse failures into a warnings list alongside the existing onWarning callback, and getSessionWarnings surfaces them with code 'skill-load-failed' so the TUI status line names the offending file. Fixes #1972 --- .changeset/skill-load-warning.md | 6 ++ packages/agent-core/src/session/index.ts | 8 ++ packages/agent-core/src/skill/registry.ts | 14 ++- .../agent-core/test/skill/registry.test.ts | 100 +++++++++++++++++- 4 files changed, 126 insertions(+), 2 deletions(-) create mode 100644 .changeset/skill-load-warning.md diff --git a/.changeset/skill-load-warning.md b/.changeset/skill-load-warning.md new file mode 100644 index 0000000000..4dafada185 --- /dev/null +++ b/.changeset/skill-load-warning.md @@ -0,0 +1,6 @@ +--- +"@moonshot-ai/agent-core": patch +"@moonshot-ai/kimi-code": patch +--- + +Surface a warning when a skill file fails to parse at session start, instead of dropping it silently. The warning names the offending file and appears in the TUI status line. diff --git a/packages/agent-core/src/session/index.ts b/packages/agent-core/src/session/index.ts index 63f8d6152e..d9d32fbbc7 100644 --- a/packages/agent-core/src/session/index.ts +++ b/packages/agent-core/src/session/index.ts @@ -694,6 +694,14 @@ export class Session { severity: 'warning', }); } + await this.skillsReady; + for (const warning of this.skills.getLoadWarnings()) { + warnings.push({ + code: 'skill-load-failed', + message: warning.message, + severity: 'warning', + }); + } return warnings; } diff --git a/packages/agent-core/src/skill/registry.ts b/packages/agent-core/src/skill/registry.ts index 65b207e274..6a655c35b8 100644 --- a/packages/agent-core/src/skill/registry.ts +++ b/packages/agent-core/src/skill/registry.ts @@ -23,11 +23,16 @@ export interface SkillRegistryOptions { readonly sessionId?: string; } +export interface SkillLoadWarning { + readonly message: string; +} + export class SessionSkillRegistry implements AgentSkillRegistry { private readonly byName = new Map(); private readonly byPluginAndName = new Map(); private readonly roots: string[] = []; private readonly skipped: SkippedSkill[] = []; + private readonly warnings: SkillLoadWarning[] = []; private readonly discoverImpl: typeof discoverSkills; private readonly onWarning: (message: string, cause?: unknown) => void; readonly sessionId?: string; @@ -45,7 +50,10 @@ export class SessionSkillRegistry implements AgentSkillRegistry { const skills = await this.discoverImpl({ roots, - onWarning: this.onWarning, + onWarning: (message, cause) => { + this.warnings.push({ message }); + this.onWarning(message, cause); + }, onSkippedByPolicy: (skill) => this.skipped.push(skill), onDiscoveredSkill: (skill) => { this.indexPluginSkill(skill); @@ -125,6 +133,10 @@ export class SessionSkillRegistry implements AgentSkillRegistry { return [...this.skipped]; } + getLoadWarnings(): readonly SkillLoadWarning[] { + return [...this.warnings]; + } + getKimiSkillsDescription(): string { const rendered = renderGroupedSkills(this.listSkills(), formatFullSkill); return rendered.length === 0 ? 'No skills' : rendered; diff --git a/packages/agent-core/test/skill/registry.test.ts b/packages/agent-core/test/skill/registry.test.ts index 7e9e26056b..cd494f76b6 100644 --- a/packages/agent-core/test/skill/registry.test.ts +++ b/packages/agent-core/test/skill/registry.test.ts @@ -1,4 +1,8 @@ -import { describe, expect, it } from 'vitest'; +import { mkdtemp, mkdir, rm, writeFile } from 'node:fs/promises'; +import { tmpdir } from 'node:os'; +import path from 'pathe'; + +import { afterEach, describe, expect, it } from 'vitest'; import { SessionSkillRegistry } from '../../src/skill'; import type { SkillDefinition, SkillSource } from '../../src/skill'; @@ -161,3 +165,97 @@ function sectionFor(rendered: string, header: string): string { const next = rendered.indexOf('### ', start + header.length); return next === -1 ? rendered.slice(start) : rendered.slice(start, next); } + +const tempDirs: string[] = []; + +afterEach(async () => { + for (const dir of tempDirs.splice(0)) { + await rm(dir, { recursive: true, force: true }); + } +}); + +describe('SessionSkillRegistry load warnings', () => { + it('collects a warning when a skill file fails to parse', async () => { + const root = await mkdtemp(path.join(tmpdir(), 'kimi-skill-registry-')); + tempDirs.push(root); + // Directory-form SKILL.md with a missing required "name" field triggers a + // SkillParseError (not an unsupported-type skip), which must surface as a + // load warning instead of being silently dropped. + await writeSkill(root, path.join('broken', 'SKILL.md'), [ + '---', + 'description: missing the name field', + '---', + 'body', + ]); + + const registry = new SessionSkillRegistry(); + await registry.loadRoots([{ path: root, source: 'project' }]); + + expect(registry.listSkills()).toEqual([]); + const warnings = registry.getLoadWarnings(); + expect(warnings.length).toBeGreaterThan(0); + expect(warnings.some((w) => w.message.includes('broken'))).toBe(true); + }); + + it('collects a warning for malformed YAML frontmatter', async () => { + const root = await mkdtemp(path.join(tmpdir(), 'kimi-skill-registry-')); + tempDirs.push(root); + // "description: foo: bar" is not a valid YAML plain scalar — the second + // ": " turns the line into an ambiguous mapping, which js-yaml rejects. + await writeSkill(root, path.join('bad-yaml', 'SKILL.md'), [ + '---', + 'name: bad-yaml', + 'description: foo: bar', + '---', + 'body', + ]); + + const registry = new SessionSkillRegistry(); + await registry.loadRoots([{ path: root, source: 'user' }]); + + expect(registry.listSkills()).toEqual([]); + expect(registry.getLoadWarnings().length).toBeGreaterThan(0); + }); + + it('produces no warning when all skills parse cleanly', async () => { + const root = await mkdtemp(path.join(tmpdir(), 'kimi-skill-registry-')); + tempDirs.push(root); + await writeSkill(root, path.join('good', 'SKILL.md'), [ + '---', + 'name: good', + 'description: A fine skill', + '---', + 'body', + ]); + + const registry = new SessionSkillRegistry(); + await registry.loadRoots([{ path: root, source: 'project' }]); + + expect(registry.listSkills().map((s) => s.name)).toEqual(['good']); + expect(registry.getLoadWarnings()).toEqual([]); + }); + + it('still forwards warnings to the onWarning callback', async () => { + const root = await mkdtemp(path.join(tmpdir(), 'kimi-skill-registry-')); + tempDirs.push(root); + await writeSkill(root, path.join('broken', 'SKILL.md'), [ + '---', + 'description: missing the name field', + '---', + 'body', + ]); + + const received: string[] = []; + const registry = new SessionSkillRegistry({ onWarning: (m) => received.push(m) }); + await registry.loadRoots([{ path: root, source: 'project' }]); + + expect(registry.getLoadWarnings().length).toBeGreaterThan(0); + expect(received.length).toBeGreaterThan(0); + }); +}); + +async function writeSkill(root: string, relativePath: string, lines: readonly string[]): Promise { + const target = path.join(root, relativePath); + await mkdir(path.dirname(target), { recursive: true }); + await writeFile(target, lines.join('\n')); +}