From 2b0fff32ca169053aafca6d1ea58216c71304972 Mon Sep 17 00:00:00 2001 From: Qiaochu Hu <110803307+hobostay@users.noreply.github.com> Date: Mon, 17 Aug 2026 13:39:19 +0800 Subject: [PATCH 1/2] fix(skills): don't corrupt SKILL.md when toggling an explicit false key MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- app/api/skills/route.ts | 22 ++-------- lib/skill-frontmatter.test.mjs | 75 ++++++++++++++++++++++++++++++++++ lib/skill-frontmatter.ts | 40 ++++++++++++++++++ 3 files changed, 118 insertions(+), 19 deletions(-) create mode 100644 lib/skill-frontmatter.test.mjs create mode 100644 lib/skill-frontmatter.ts diff --git a/app/api/skills/route.ts b/app/api/skills/route.ts index d71fd103d..291be3ba6 100644 --- a/app/api/skills/route.ts +++ b/app/api/skills/route.ts @@ -2,8 +2,9 @@ import { NextResponse } from "next/server"; import { existsSync, readFileSync, writeFileSync } from "fs"; import { homedir } from "os"; import path from "path"; -import { getAgentDir, parseFrontmatter } from "@earendil-works/pi-coding-agent"; +import { getAgentDir } from "@earendil-works/pi-coding-agent"; import { loadSkillsWithInstallInfo } from "@/lib/skills-service"; +import { setDisableModelInvocation } from "@/lib/skill-frontmatter"; import { getAllowedFileRoots, isExistingFilePathAllowed } from "@/lib/file-access"; export const dynamic = "force-dynamic"; @@ -47,24 +48,7 @@ export async function PATCH(req: Request) { } const content = readFileSync(filePath, "utf8"); - const key = "disable-model-invocation"; - - // Use parseFrontmatter to check current value, then do a surgical line edit - // to preserve the original YAML formatting of all other fields. - const { frontmatter } = parseFrontmatter>(content); - const alreadySet = Boolean(frontmatter[key]); - - let updated = content; - if (disableModelInvocation && !alreadySet) { - // Add key after the opening --- line - updated = content.replace(/^---\r?\n/, `---\n${key}: true\n`); - // If no frontmatter exists, create one - if (updated === content) updated = `---\n${key}: true\n---\n${content}`; - } else if (!disableModelInvocation && alreadySet) { - // Remove the key line entirely - updated = content.replace(new RegExp(`^${key}\\s*:.*\\r?\\n`, "m"), ""); - } - + const updated = setDisableModelInvocation(content, disableModelInvocation); writeFileSync(filePath, updated, "utf8"); return NextResponse.json({ success: true }); } catch (e) { diff --git a/lib/skill-frontmatter.test.mjs b/lib/skill-frontmatter.test.mjs new file mode 100644 index 000000000..8880baa26 --- /dev/null +++ b/lib/skill-frontmatter.test.mjs @@ -0,0 +1,75 @@ +import assert from "node:assert/strict"; +import { describe, it } from "node:test"; +import { parseFrontmatter } from "@earendil-works/pi-coding-agent"; + +import { setDisableModelInvocation } from "./skill-frontmatter.ts"; + +describe("setDisableModelInvocation", () => { + const withFrontmatter = "---\nname: my-skill\ndescription: Does things\n---\n\nBody text.\n"; + + it("adds the key after the opening fence when absent", () => { + const updated = setDisableModelInvocation(withFrontmatter, true); + assert.equal( + updated, + "---\ndisable-model-invocation: true\nname: my-skill\ndescription: Does things\n---\n\nBody text.\n", + ); + assert.equal(parseFrontmatter(updated).frontmatter["disable-model-invocation"], true); + }); + + it("creates a frontmatter block when the file has none", () => { + const updated = setDisableModelInvocation("Just a body.\n", true); + const { frontmatter, body } = parseFrontmatter(updated); + assert.equal(frontmatter["disable-model-invocation"], true); + assert.equal(body, "Just a body."); + }); + + it("replaces an explicit false value instead of adding a duplicate key", () => { + const content = "---\nname: my-skill\ndisable-model-invocation: false\ndescription: Does things\n---\n\nBody text.\n"; + const updated = setDisableModelInvocation(content, true); + // A duplicate key would make the whole file unparseable and the loader + // would drop the skill, so this must parse to a single true value. + const { frontmatter } = parseFrontmatter(updated); + assert.equal(frontmatter["disable-model-invocation"], true); + assert.equal( + updated.match(/^disable-model-invocation[^\n]*/gm)?.length, + 1, + "must not emit a duplicate key", + ); + }); + + it("keeps a single key when disabling an already-true skill", () => { + const content = "---\nname: my-skill\ndisable-model-invocation: true\ndescription: Does things\n---\n\nBody text.\n"; + const updated = setDisableModelInvocation(content, true); + assert.equal(parseFrontmatter(updated).frontmatter["disable-model-invocation"], true); + assert.equal(updated.match(/^disable-model-invocation[^\n]*/gm)?.length, 1); + }); + + it("removes the key when disabling is turned off", () => { + const content = "---\nname: my-skill\ndisable-model-invocation: true\ndescription: Does things\n---\n\nBody text.\n"; + const updated = setDisableModelInvocation(content, false); + assert.deepEqual(parseFrontmatter(updated).frontmatter, { + name: "my-skill", + description: "Does things", + }); + }); + + it("removes the key when it is the last frontmatter line before the closing fence", () => { + const content = "---\nname: my-skill\ndescription: Does things\ndisable-model-invocation: true\n---\n\nBody text.\n"; + const updated = setDisableModelInvocation(content, false); + assert.equal(updated, withFrontmatter); + }); + + it("is a no-op when disabling is off and the key is absent", () => { + assert.equal(setDisableModelInvocation(withFrontmatter, false), withFrontmatter); + }); + + it("preserves unrelated frontmatter formatting and body lines that mention the key", () => { + const content = "---\nname: my-skill\n# comment\nallowed-tools: [ read, write ]\ndisable-model-invocation: false\n---\nUse `disable-model-invocation: true` to hide me.\n"; + const updated = setDisableModelInvocation(content, true); + const { frontmatter, body } = parseFrontmatter(updated); + assert.equal(frontmatter["disable-model-invocation"], true); + assert.deepEqual(frontmatter["allowed-tools"], ["read", "write"]); + assert.match(updated, /# comment/); + assert.equal(body, "Use `disable-model-invocation: true` to hide me."); + }); +}); diff --git a/lib/skill-frontmatter.ts b/lib/skill-frontmatter.ts new file mode 100644 index 000000000..dd7f87808 --- /dev/null +++ b/lib/skill-frontmatter.ts @@ -0,0 +1,40 @@ +import { parseFrontmatter } from "@earendil-works/pi-coding-agent"; + +const KEY = "disable-model-invocation"; + +/** + * Toggle the `disable-model-invocation` frontmatter key with a surgical line + * edit that preserves the original YAML formatting of every other field. + * + * The key is detected by presence rather than truthiness: an explicit + * `disable-model-invocation: false` must be updated in place. Prepending a + * second key (as a truthiness check would) creates a duplicate YAML key that + * makes the whole file unparseable, and the skill loader then drops the skill. + */ +export function setDisableModelInvocation(content: string, disable: boolean): string { + const { frontmatter } = parseFrontmatter>(content); + const hasKey = Object.prototype.hasOwnProperty.call(frontmatter, KEY); + if (!disable && !hasKey) return content; + + // Only edit inside the frontmatter block, so a body line that happens to + // document the key is never touched. + const closing = content.startsWith("---") ? content.indexOf("\n---", 3) : -1; + const head = closing === -1 ? content : content.slice(0, closing); + const tail = closing === -1 ? "" : content.slice(closing); + + if (disable) { + if (hasKey) { + return head.replace(new RegExp(`^${KEY}[ \\t]*:[^\\n]*`, "m"), `${KEY}: true`) + tail; + } + const withKey = head.replace(/^---\r?\n/, `---\n${KEY}: true\n`); + if (withKey === head) { + // No frontmatter block at all — create one. + return `---\n${KEY}: true\n---\n${content}`; + } + return withKey + tail; + } + + // Drop the line together with its preceding newline so no blank line is + // left behind; the key is never the first line of the frontmatter block. + return head.replace(new RegExp(`\\n${KEY}[ \\t]*:[^\\n]*`), "") + tail; +} From 1d7e7390b5fe8498663e545d004c801232cd43a4 Mon Sep 17 00:00:00 2001 From: Alex Yang Date: Tue, 25 Aug 2026 17:29:55 +0800 Subject: [PATCH 2/2] fix(skills): preserve frontmatter formatting --- lib/skill-frontmatter.test.mjs | 29 +++++++++++++++++++++++++++++ lib/skill-frontmatter.ts | 11 ++++++++--- 2 files changed, 37 insertions(+), 3 deletions(-) diff --git a/lib/skill-frontmatter.test.mjs b/lib/skill-frontmatter.test.mjs index 8880baa26..ce5f27d99 100644 --- a/lib/skill-frontmatter.test.mjs +++ b/lib/skill-frontmatter.test.mjs @@ -37,6 +37,35 @@ describe("setDisableModelInvocation", () => { ); }); + it("updates and removes indented quoted keys", () => { + for (const quote of ['"', "'"]) { + const content = `---\n name: my-skill\n ${quote}disable-model-invocation${quote}: false\n---\nBody text.\n`; + const updated = setDisableModelInvocation(content, true); + assert.equal(parseFrontmatter(updated).frontmatter["disable-model-invocation"], true); + assert.equal( + parseFrontmatter(setDisableModelInvocation(updated, false)).frontmatter["disable-model-invocation"], + undefined, + ); + } + }); + + it("rejects unsupported key formatting instead of silently succeeding", () => { + const content = "---\n{ disable-model-invocation: false, name: my-skill }\n---\nBody text.\n"; + assert.throws(() => setDisableModelInvocation(content, true), /unsupported frontmatter formatting/); + }); + + it("preserves CRLF line endings when updating or adding the key", () => { + const content = "---\r\nname: my-skill\r\ndisable-model-invocation: false\r\n---\r\nBody text.\r\n"; + assert.equal( + setDisableModelInvocation(content, true), + "---\r\nname: my-skill\r\ndisable-model-invocation: true\r\n---\r\nBody text.\r\n", + ); + assert.equal( + setDisableModelInvocation(content.replace("disable-model-invocation: false\r\n", ""), true), + "---\r\ndisable-model-invocation: true\r\nname: my-skill\r\n---\r\nBody text.\r\n", + ); + }); + it("keeps a single key when disabling an already-true skill", () => { const content = "---\nname: my-skill\ndisable-model-invocation: true\ndescription: Does things\n---\n\nBody text.\n"; const updated = setDisableModelInvocation(content, true); diff --git a/lib/skill-frontmatter.ts b/lib/skill-frontmatter.ts index dd7f87808..7eb025cb4 100644 --- a/lib/skill-frontmatter.ts +++ b/lib/skill-frontmatter.ts @@ -1,6 +1,7 @@ import { parseFrontmatter } from "@earendil-works/pi-coding-agent"; const KEY = "disable-model-invocation"; +const KEY_LINE = `[ \\t]*(?:${KEY}|"${KEY}"|'${KEY}')[ \\t]*:`; /** * Toggle the `disable-model-invocation` frontmatter key with a surgical line @@ -24,9 +25,11 @@ export function setDisableModelInvocation(content: string, disable: boolean): st if (disable) { if (hasKey) { - return head.replace(new RegExp(`^${KEY}[ \\t]*:[^\\n]*`, "m"), `${KEY}: true`) + tail; + const keyLine = new RegExp(`^(${KEY_LINE})[^\\r\\n]*(\\r?)$`, "m"); + if (!keyLine.test(head)) throw new Error(`Cannot edit ${KEY}: unsupported frontmatter formatting`); + return head.replace(keyLine, "$1 true$2") + tail; } - const withKey = head.replace(/^---\r?\n/, `---\n${KEY}: true\n`); + const withKey = head.replace(/^---(\r?\n)/, `---$1${KEY}: true$1`); if (withKey === head) { // No frontmatter block at all — create one. return `---\n${KEY}: true\n---\n${content}`; @@ -36,5 +39,7 @@ export function setDisableModelInvocation(content: string, disable: boolean): st // Drop the line together with its preceding newline so no blank line is // left behind; the key is never the first line of the frontmatter block. - return head.replace(new RegExp(`\\n${KEY}[ \\t]*:[^\\n]*`), "") + tail; + const keyLine = new RegExp(`\\n${KEY_LINE}[^\\n]*`); + if (!keyLine.test(head)) throw new Error(`Cannot edit ${KEY}: unsupported frontmatter formatting`); + return head.replace(keyLine, "") + tail; }