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

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<Record<string, unknown>>(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) {

lib/skill-frontmatter.test.mjs

@@ -0,0 +1,104 @@
+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("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);
+    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.");
+  });
+});

lib/skill-frontmatter.ts

@@ -0,0 +1,45 @@
+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
+ * 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<Record<string, unknown>>(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) {
+      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)/, `---$1${KEY}: true$1`);
+    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.
+  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;
+}