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 0f7ec8d235..3833d71a3d 100644 --- a/packages/agent-core/src/session/index.ts +++ b/packages/agent-core/src/session/index.ts @@ -694,6 +694,17 @@ export class Session { severity: 'warning', }); } + // 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 223fe64dcc..abe10c3fcd 100644 --- a/packages/agent-core/test/harness/skill-session.test.ts +++ b/packages/agent-core/test/harness/skill-session.test.ts @@ -608,6 +608,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 e47a0f7637..6f918ed91e 100644 --- a/packages/kap-server/src/routes/sessions.ts +++ b/packages/kap-server/src/routes/sessions.ts @@ -14,7 +14,7 @@ * POST /sessions/{session_id}/children create child session (fork+tag) * GET /sessions/{session_id}/status best-effort * GET /sessions/{session_id}/goal current goal (null when none) - * GET /sessions/{session_id}/warnings agents-md-oversized notice + * GET /sessions/{session_id}/warnings agents-md-oversized + skill-load-failed notices * * The `POST /sessions/{tail}` actions split into two groups. The thin * pass-throughs — `fork` / `compact` / `abort` / `archive` / `restore` — call @@ -35,12 +35,16 @@ * `create`, `fork`, and child creation publish `event.session.created` on the * core event bus, matching v1. * - * `GET /sessions/{id}/warnings` surfaces the only v1 warning - * (`agents-md-oversized`) by projecting the main agent's - * `IAgentProfileService.getAgentsMdWarning()` — computed and cached when the - * agent binds a profile (via `prepareSystemPromptContext`) — into the v1 - * `{ code, message, severity }` wire shape. An unbound main agent yields an - * empty list, matching v1's "no warning" case. + * `GET /sessions/{id}/warnings` surfaces two notices in the v1 + * `{ code, message, severity }` wire shape: + * - `agents-md-oversized` — the main agent's + * `IAgentProfileService.getAgentsMdWarning()`, computed and cached when the + * agent binds a profile (via `prepareSystemPromptContext`). An unbound main + * agent yields none of these, matching v1's "no warning" case. + * - `skill-load-failed` — one entry per `ISessionSkillCatalog` + * `getSkippedByPolicy()` result whose `SKILL.md` failed to parse (invalid + * frontmatter, unsupported `type`, …), so a skill that got dropped during + * load is reported instead of vanishing silently. * * **Wire fidelity**: mirrors v1's `toProtocolSession` * (`packages/agent-core/src/services/session/session.ts`), which populates @@ -88,6 +92,7 @@ import { ISessionLifecycleService, ISessionMetadata, ISessionLegacyService, + ISessionSkillCatalog, IEventService, IWorkspaceRegistry, isError2, @@ -1004,7 +1009,7 @@ export function registerSessionsRoutes(app: SessionRouteHost, core: Scope): void // matching v1's "no warning" case. const agent = await ensureMainAgent(session); const agentsMdWarning = agent.accessor.get(IAgentProfileService).getAgentsMdWarning(); - const warnings = + const agentsMdWarnings = agentsMdWarning === undefined ? [] : [ @@ -1014,7 +1019,19 @@ export function registerSessionsRoutes(app: SessionRouteHost, core: Scope): void severity: 'warning' as const, }, ]; - reply.send(okEnvelope({ warnings }, req.id)); + + // 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, + })); + + reply.send(okEnvelope({ warnings: [...agentsMdWarnings, ...skillWarnings] }, req.id)); } catch (error) { sendMappedError(reply, req, error); } diff --git a/packages/kap-server/test/sessions.test.ts b/packages/kap-server/test/sessions.test.ts index 69cda99b17..1086eaeaef 100644 --- a/packages/kap-server/test/sessions.test.ts +++ b/packages/kap-server/test/sessions.test.ts @@ -719,6 +719,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);