Skip to content

fix(skills): toggling disable-model-invocation on an explicit false value corrupts SKILL.md - #519

Open
hobostay wants to merge 1 commit into
agegr:mainfrom
hobostay:fix/skills-toggle-duplicate-frontmatter-key
Open

fix(skills): toggling disable-model-invocation on an explicit false value corrupts SKILL.md#519
hobostay wants to merge 1 commit into
agegr:mainfrom
hobostay:fix/skills-toggle-duplicate-frontmatter-key

Conversation

@hobostay

Copy link
Copy Markdown

Problem

PATCH /api/skills decides whether the disable-model-invocation key already exists with Boolean(frontmatter[key]) (app/api/skills/route.ts). A SKILL.md that explicitly declares the key as false:

---
name: my-skill
disable-model-invocation: false
---

is treated as having no key at all. Clicking "disable model invocation" in the UI then prepends a second disable-model-invocation: true line, and things go from bad to worse:

  1. The yaml parser rejects duplicate keys outright (Map keys must be unique), so parseFrontmatter now throws for the whole file.
  2. The pi skill loader catches that throw and returns skill: nullthe skill disappears from the agent's skill list (and from the skills panel) after a single UI click.
  3. The PATCH route itself calls parseFrontmatter before editing, so every further toggle attempt on that file returns 500 — the corruption cannot be undone from the UI, only by hand-editing the file.

Repro:

echo '---
name: t
description: t
disable-model-invocation: false
---
body' > SKILL.md
# toggle disable-model-invocation ON via the skills panel
# → file now starts with a duplicate key and no longer parses

Fix

  • Extract the edit into lib/skill-frontmatter.ts (setDisableModelInvocation) with unit tests, and detect the key by presence (hasOwnProperty) instead of truthiness, updating the existing line in place.
  • Restrict the edit to the frontmatter block, so a body line that merely documents the key is never touched.
  • On removal, drop the line together with its preceding newline so no stray blank line is left inside the frontmatter.

Tests: node --experimental-strip-types --test lib/skill-frontmatter.test.mjs (8 cases, including the explicit-false regression and duplicate-key guards). Full suite 595/595, tsc --noEmit and eslint clean.

🤖 Generated with Claude Code

The disable-model-invocation toggle detected the frontmatter key with
Boolean(frontmatter[key]), so a skill that explicitly declared
disable-model-invocation: false was treated as having no key. Disabling
it prepended a second disable-model-invocation: true line, and the yaml
parser rejects duplicate keys outright — after that one click the file
no longer parsed, the skill loader dropped the skill entirely, and the
PATCH route itself 500ed, so the damage could not be undone from the UI.

Detect the key by presence and update the existing line in place, keep
the edit inside the frontmatter block so body lines documenting the key
are untouched, and drop the removed line together with its newline.

Co-Authored-By: Claude <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant