diff --git a/.changeset/skill-load-failed-warning.md b/.changeset/skill-load-failed-warning.md new file mode 100644 index 0000000000..a80ffbb938 --- /dev/null +++ b/.changeset/skill-load-failed-warning.md @@ -0,0 +1,8 @@ +--- +"@moonshot-ai/kimi-code": patch +"@moonshot-ai/agent-core": patch +"@moonshot-ai/agent-core-v2": patch +"@moonshot-ai/kap-server": patch +--- + +Fix a skill with a `SKILL.md` that fails to parse (e.g. invalid frontmatter) being dropped from the skill list with no signal. A session-start warning now names the skill file and the reason it was not loaded, in both the CLI/TUI engine and `kimi web`/print mode. diff --git a/packages/agent-core-v2/src/app/skillCatalog/fileSkillDiscovery.ts b/packages/agent-core-v2/src/app/skillCatalog/fileSkillDiscovery.ts index 5d40eca819..8de89b01d5 100644 --- a/packages/agent-core-v2/src/app/skillCatalog/fileSkillDiscovery.ts +++ b/packages/agent-core-v2/src/app/skillCatalog/fileSkillDiscovery.ts @@ -183,6 +183,13 @@ async function parseAndRegister(input: { }); } else if (error instanceof SkillParseError) { input.warn?.(`Skipping invalid skill at ${input.skillMdPath}: ${error.message}`, error); + // Also record it as skipped (not just logged) so it surfaces as a + // session warning instead of vanishing silently. + input.skipped.push({ + path: input.skillMdPath, + type: 'invalid-frontmatter', + reason: error.message, + }); } else { input.warn?.(`Skipping skill at ${input.skillMdPath} due to unexpected error`, error); } diff --git a/packages/agent-core-v2/test/app/skillCatalog/fileSkillDiscovery.test.ts b/packages/agent-core-v2/test/app/skillCatalog/fileSkillDiscovery.test.ts index e82d2be20c..8bbed95ae6 100644 --- a/packages/agent-core-v2/test/app/skillCatalog/fileSkillDiscovery.test.ts +++ b/packages/agent-core-v2/test/app/skillCatalog/fileSkillDiscovery.test.ts @@ -217,6 +217,13 @@ describe('FileSkillDiscovery', () => { const result = await discover([skillRoot('skills')]); expect(result.skills).toEqual([]); + expect(result.skipped).toEqual([ + { + path: skillMdPath, + type: 'invalid-frontmatter', + reason: `Missing frontmatter in ${skillMdPath}`, + }, + ]); expect(warnings).toEqual([ { message: `Skipping invalid skill at ${skillMdPath}: Missing frontmatter in ${skillMdPath}`, @@ -225,6 +232,23 @@ describe('FileSkillDiscovery', () => { ]); }); + it('skips (rather than silently drops) a description with an unquoted colon that breaks YAML', async () => { + // Regression for #1972: a `description` containing a bare "word: word" + // (e.g. "Triggers on: ...") is invalid as a plain YAML scalar, so the + // frontmatter fails to parse. The skill used to vanish with no signal + // beyond a log line nobody sees; it must now show up in `skipped`. + await writeSkill( + 'skills/codebase-memory/SKILL.md', + 'name: codebase-memory\ndescription: Explore the codebase. Triggers on: find callers of, trace the call chain.', + ); + + const result = await discover([skillRoot('skills')]); + + expect(result.skills).toEqual([]); + expect(result.skipped).toHaveLength(1); + expect(result.skipped[0]?.type).toBe('invalid-frontmatter'); + }); + it('records a skill with an unsupported type as skipped instead of warning', async () => { await writeSkill('skills/legacy/SKILL.md', 'name: legacy\ndescription: old\ntype: nope'); diff --git a/packages/agent-core/src/session/index.ts b/packages/agent-core/src/session/index.ts index 50762884f8..9f0bedb20f 100644 --- a/packages/agent-core/src/session/index.ts +++ b/packages/agent-core/src/session/index.ts @@ -802,6 +802,17 @@ export class Session { }); } warnings.push(...this.computeSecondaryModelWarnings()); + // Surface any skill that got dropped during catalog load (invalid + // frontmatter, unsupported `type`, …) instead of leaving it silently + // missing from the skill list. + await this.skillsReady; + for (const skipped of this.skills.getSkippedByPolicy()) { + warnings.push({ + code: 'skill-load-failed', + message: `Skill at ${skipped.path} was not loaded: ${skipped.reason}`, + severity: 'warning', + }); + } return warnings; } diff --git a/packages/agent-core/src/skill/scanner.ts b/packages/agent-core/src/skill/scanner.ts index 6c1ddb462c..53481c5e1c 100644 --- a/packages/agent-core/src/skill/scanner.ts +++ b/packages/agent-core/src/skill/scanner.ts @@ -407,6 +407,13 @@ async function parseAndRegister(input: { }); } else if (error instanceof SkillParseError) { input.warn(`Skipping invalid skill at ${input.skillMdPath}: ${error.message}`, error); + // Also record it as skipped (not just logged) so it surfaces as a + // session warning instead of vanishing silently. + input.skip({ + path: input.skillMdPath, + type: 'invalid-frontmatter', + reason: error.message, + }); } else { input.warn(`Skipping skill at ${input.skillMdPath} due to unexpected error`, error); } diff --git a/packages/agent-core/test/harness/skill-session.test.ts b/packages/agent-core/test/harness/skill-session.test.ts index 987380961c..e12eb39c22 100644 --- a/packages/agent-core/test/harness/skill-session.test.ts +++ b/packages/agent-core/test/harness/skill-session.test.ts @@ -611,6 +611,29 @@ describe('HarnessAPI session skills', () => { }); }); + it('reports a skill-load-failed session warning for a SKILL.md that fails to parse', async () => { + // Regression for #1972: an unquoted colon in `description` (e.g. + // "Triggers on: ...") makes the frontmatter invalid YAML, and the skill + // used to be dropped with no signal reaching the TUI, which calls this + // same `getSessionWarnings` RPC at session start. + await writeSkill('broken', [ + '---', + 'name: broken', + 'description: Explore the codebase. Triggers on: find callers of.', + '---', + '', + 'Body.', + ]); + const { rpc } = await createTestRpc(); + const created = await rpc.createSession({ id: 'ses_skill_warning', workDir }); + + const warnings = await rpc.getSessionWarnings({ sessionId: created.id }); + + const skillWarning = warnings.find((warning) => warning.code === 'skill-load-failed'); + expect(skillWarning?.message).toContain('broken/SKILL.md'); + expect(skillWarning?.severity).toBe('warning'); + }); + it('rejects missing and non-inline skills with structured errors', async () => { const { core, rpc } = await createTestRpc(); const created = await rpc.createSession({ id: 'ses_skill_errors', workDir }); diff --git a/packages/agent-core/test/skill/scanner.test.ts b/packages/agent-core/test/skill/scanner.test.ts index 38377338ea..21bc2bce66 100644 --- a/packages/agent-core/test/skill/scanner.test.ts +++ b/packages/agent-core/test/skill/scanner.test.ts @@ -196,6 +196,34 @@ describe('skill discovery', () => { expect(warnings.some((message) => message.includes('"name"'))).toBe(true); expect(warnings.some((message) => message.includes('"description"'))).toBe(true); }); + + it('reports a skill with unparseable frontmatter as skipped, not just warned', async () => { + // Regression for #1972: a `description` containing a bare "word: word" + // (e.g. "Triggers on: ...") is invalid as a plain YAML scalar, so the + // frontmatter fails to parse. The skill used to vanish with only a log + // line nobody sees; it must now show up in the skipped list too. + const { repoDir } = await makeWorkspace(); + const projectRoot = path.join(repoDir, '.kimi-code', 'skills'); + await writeSkill(projectRoot, path.join('codebase-memory', 'SKILL.md'), [ + '---', + 'name: codebase-memory', + 'description: Explore the codebase. Triggers on: find callers of.', + '---', + '', + 'Body.', + ]); + + const skipped: { path: string; type: string; reason: string }[] = []; + const skills = await discoverSkills({ + roots: [{ path: projectRoot, source: 'project' }], + onSkippedByPolicy: (skill) => skipped.push(skill), + }); + + expect(skills).toEqual([]); + expect(skipped).toHaveLength(1); + expect(skipped[0]?.type).toBe('invalid-frontmatter'); + expect(skipped[0]?.path).toContain('codebase-memory/SKILL.md'); + }); }); describe('discoverSkills shape and ordering', () => { diff --git a/packages/kap-server/src/routes/sessions.ts b/packages/kap-server/src/routes/sessions.ts index 1623004e09..9f034a82db 100644 --- a/packages/kap-server/src/routes/sessions.ts +++ b/packages/kap-server/src/routes/sessions.ts @@ -38,11 +38,15 @@ * `GET /sessions/{id}/warnings` surfaces session-level notices in the v1 * `{ code, message, severity }` wire shape: the `agents-md-oversized` warning * (projected from the main agent's `IAgentProfileService.getAgentsMdWarning()` - * — computed and cached when the agent binds a profile) and the - * secondary-model early-validation warning (projected from the Session-scope + * — computed and cached when the agent binds a profile), the secondary-model + * early-validation warning (projected from the Session-scope * `ISessionSecondaryModelWarningService` — computed and cached when the main - * agent is created). An unbound main agent or a valid/unset secondary model - * yields an empty list, matching v1's "no warning" case. + * agent is created), and one `skill-load-failed` entry per + * `ISessionSkillCatalog` `getSkippedByPolicy()` result whose `SKILL.md` failed + * to parse (invalid frontmatter, unsupported `type`, …), so a skill dropped + * during load is reported instead of vanishing silently. An unbound main agent, + * a valid/unset secondary model, and a clean catalog together yield an empty + * list, matching v1's "no warning" case. * * **Wire fidelity**: mirrors v1's `toProtocolSession` * (`packages/agent-core/src/services/session/session.ts`), which populates @@ -89,6 +93,7 @@ import { ISessionMetadata, ISessionLegacyService, ISessionSecondaryModelWarningService, + ISessionSkillCatalog, IEventService, IWorkspaceAliases, IWorkspaceService, @@ -1011,6 +1016,18 @@ export function registerSessionsRoutes(app: SessionRouteHost, core: Scope): void const secondaryModelWarning = session.accessor .get(ISessionSecondaryModelWarningService) .getSecondaryModelWarning(); + + // Surface any skill that got dropped during catalog load (invalid + // frontmatter, unsupported `type`, …) instead of leaving it silently + // missing from the skill list. + const skillCatalog = session.accessor.get(ISessionSkillCatalog); + await skillCatalog.ready; + const skillWarnings = skillCatalog.catalog.getSkippedByPolicy().map((skipped) => ({ + code: 'skill-load-failed', + message: `Skill at ${skipped.path} was not loaded: ${skipped.reason}`, + severity: 'warning' as const, + })); + const warnings = [ ...(agentsMdWarning === undefined ? [] @@ -1030,6 +1047,7 @@ export function registerSessionsRoutes(app: SessionRouteHost, core: Scope): void severity: 'warning' as const, }, ]), + ...skillWarnings, ]; reply.send(okEnvelope({ warnings }, req.id)); } catch (error) { diff --git a/packages/kap-server/test/sessions.test.ts b/packages/kap-server/test/sessions.test.ts index b6a2db27d9..5147f054ea 100644 --- a/packages/kap-server/test/sessions.test.ts +++ b/packages/kap-server/test/sessions.test.ts @@ -792,6 +792,30 @@ describe('server-v2 /api/v1/sessions', () => { expect(sessionWarningsResponseSchema.parse(body.data)).toEqual({ warnings: [] }); }); + it('reports a skill-load-failed warning for a SKILL.md that fails to parse', async () => { + // Regression for #1972: an unquoted colon in `description` (e.g. + // "Triggers on: ...") makes the frontmatter invalid YAML, and the skill + // used to be dropped with no signal reaching the user. + const cwd = home as string; + const skillDir = join(cwd, '.kimi-code', 'skills', 'broken'); + await mkdir(skillDir, { recursive: true }); + await writeFile( + join(skillDir, 'SKILL.md'), + '---\nname: broken\ndescription: Explore the codebase. Triggers on: find callers of.\n---\n\nbody\n', + ); + + const created = await postJson('/api/v1/sessions', { metadata: { cwd } }); + const { status, body } = await getJson<{ + warnings: { code: string; message: string; severity: string }[]; + }>(`/api/v1/sessions/${created.body.data.id}/warnings`); + + expect(status).toBe(200); + expect(body.code).toBe(0); + const skillWarning = body.data.warnings.find((w) => w.code === 'skill-load-failed'); + expect(skillWarning?.message).toContain(join(skillDir, 'SKILL.md')); + expect(sessionWarningsResponseSchema.parse(body.data).warnings).toContainEqual(skillWarning); + }); + it('returns 40401 for warnings of a missing session', async () => { const { body } = await getJson('/api/v1/sessions/sess_missing_warnings/warnings'); expect(body.code).toBe(40401);