fix(agent-core-v2): surface a warning when a skill fails to load#1987
fix(agent-core-v2): surface a warning when a skill fails to load#1987arimu1 wants to merge 2 commits into
Conversation
A SKILL.md whose frontmatter fails to parse (e.g. a description with an
unquoted colon that breaks YAML) was only logged to the internal log file
and otherwise vanished from the skill list with no visible signal. Record
these as skipped skills (same mechanism already used for unsupported skill
types) and surface them through GET /sessions/{id}/warnings, which the TUI
already shows at session start.
🦋 Changeset detectedLatest commit: 809e770 The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 95df2f4de4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Also record it as skipped (not just logged) so it surfaces as a | ||
| // session warning instead of vanishing silently. |
There was a problem hiding this comment.
packages/agent-core-v2/AGENTS.md requires comments in this package to live only in the top-of-file /** */ block and never beside functions or statements. This new catch-block explanation violates that local review rule; please remove it or fold any necessary rationale into the module header.
Useful? React with 👍 / 👎.
The v2/kap-server fix for MoonshotAI#1972 never reached the interactive TUI: it calls Session.getSessionWarnings() through an in-process KimiCore bound to agent-core (v1), not kap-server. Mirror the same fix there — record a SkillParseError as a skipped skill (byte-identical to the existing UnsupportedSkillTypeError handling) and surface it from getSessionWarnings() via the registry's (previously unused) getSkippedByPolicy().
|
Confirmed the review trace: the interactive TUI never reaches
Test results:
|
1 participant
Related Issue
Resolve #1972
Problem
I dug into the report and the trigger turns out not to be a length limit — there is no length cap on
descriptionanywhere in the skill loader. The actual repro is a YAML gotcha: adescriptionthat contains an unquotedword: wordsequence (e.g."...Triggers on: explore the codebase...") is invalid as a plain YAML scalar, so the frontmatter fails to parse. I confirmed this directly against the parser'sjs-yamlcall — the 453-char example from the issue throwsbad indentation of a mapping entry, while a 453-char description with no colon parses fine, and the "fix" the reporter found (shortening the description) happened to also remove the colon.Regardless of the exact trigger, the reported symptom is real: any
SKILL.mdwhose frontmatter fails to parse is dropped from the skill list, and the only trace is alog.warn()call that goes to the internal rotating log file — nothing a user sees during a normal session.What changed
This repo runs two engines side by side, and the fix has to land in both:
packages/agent-core-v2/src/app/skillCatalog/fileSkillDiscovery.ts: when a skill's frontmatter fails to parse, it's now also recorded as a skipped skill (the same mechanism already used for unsupported skill types), in addition to the existing log line.packages/kap-server/src/routes/sessions.ts:GET /sessions/{id}/warningsnow also includes oneskill-load-failedentry per skipped skill, naming the file and the parse error. This is the path used bykimi weband bykimi -punderKIMI_CODE_EXPERIMENTAL_FLAG.packages/agent-core/src/skill/scanner.ts: the byte-identical fix on the v1 engine — aSkillParseErrornow also records the skipped skill instead of only logging it.packages/agent-core/src/session/index.ts:Session.getSessionWarnings()now includes the sameskill-load-failedentries, sourced from the session's own skill registry (getSkippedByPolicy()), mirroring the kap-server route above.An earlier version of this PR only touched the v2/kap-server side and claimed the TUI required no changes because "it already calls this endpoint." That was wrong: the interactive TUI (
apps/kimi-code/src/tui/kimi-tui.ts) callssession.getSessionWarnings()on the@moonshot-ai/kimi-code-sdkclient, which binds to an in-processKimiCorefrom@moonshot-ai/agent-core(v1), not tokap-server—kap-server/v2 is only reached bykimi weband bykimi -punder the experimental flag. Since the reporter hit this in the plain interactive TUI, only the v2 fix would never have surfaced their warning. Both engines are fixed now.I kept this scoped to
SkillParseError(invalid/missing frontmatter, missing required fields) since that's the class of failure in the issue. I left the catch-all "unexpected error" branch alone in both engines — its message is intentionally vague and not something worth surfacing as a normal warning.Checklist
gen-changesetsskill.gen-docsskill, or this PR needs no doc update — no new user-facing surface was added (the warning endpoint and its wire shape already existed foragents-md-oversized), so I did not touchdocs/.