fix: parse skill frontmatter as YAML (#25610)

coderd/x/skills/skills_test.go

@@ -26,6 +26,33 @@ func TestParsePersonalSkillMarkdown(t *testing.T) {
 		require.Equal(t, "Use this skill.", content.Body)
 	})
 
+	t.Run("ValidWithFoldedDescription", func(t *testing.T) {
+		t.Parallel()
+
+		content, err := skills.ParsePersonalSkillMarkdown([]byte(strings.Join([]string{
+			"---",
+			"name: brainstorming",
+			"description: >",
+			"  Use before any creative work: features, components, functionality changes,",
+			"  or behavior modifications. Turns ideas into approved designs through",
+			"  collaborative dialog. Hard gate: no implementation action until the",
+			"  design is presented and approved.",
+			"---",
+			"Use this skill.",
+		}, "\n")))
+
+		require.NoError(t, err)
+		require.Equal(t, "brainstorming", content.Name)
+		require.Equal(t, strings.Join([]string{
+			"Use before any creative work: features, components, functionality changes,",
+			"or behavior modifications. Turns ideas into approved designs through",
+			"collaborative dialog. Hard gate: no implementation action until the",
+			"design is presented and approved.",
+		}, " "), content.Description)
+		require.Equal(t, skills.SourcePersonal, content.Source)
+		require.Equal(t, "Use this skill.", content.Body)
+	})
+
 	t.Run("ValidWithoutDescription", func(t *testing.T) {
 		t.Parallel()
 
@@ -67,6 +94,16 @@ func TestParsePersonalSkillMarkdown(t *testing.T) {
 		require.ErrorContains(t, err, "frontmatter must contain a 'name' field")
 	})
 
+	t.Run("NonStringName", func(t *testing.T) {
+		t.Parallel()
+
+		_, err := skills.ParsePersonalSkillMarkdown([]byte(
+			"---\nname: null\n---\nBody.\n",
+		))
+
+		require.ErrorIs(t, err, skills.ErrInvalidSkillName)
+	})
+
 	t.Run("NonKebabCaseName", func(t *testing.T) {
 		t.Parallel()
 

codersdk/workspacesdk/frontmatter.go

@@ -5,6 +5,7 @@ import (
 	"strings"
 
 	"golang.org/x/xerrors"
+	"gopkg.in/yaml.v3"
 )
 
 // SkillNameRegex is the regular expression used to validate kebab-case skill names.
@@ -24,32 +25,21 @@ var markdownCommentRe = regexp.MustCompile(`<!--[\s\S]*?-->`)
 // the frontmatter is missing a required name field.
 var ErrFrontmatterNameRequired = xerrors.New("frontmatter missing required 'name' field")
 
-func unquoteFrontmatterScalar(value string) string {
-	if len(value) < 2 {
-		return value
+func frontmatterStringField(frontmatter map[string]any, key string) (string, bool, error) {
+	value, ok := frontmatter[key]
+	if !ok {
+		return "", false, nil
 	}
-
-	quote := value[0]
-	if quote != value[len(value)-1] {
-		return value
-	}
-
-	inner := value[1 : len(value)-1]
-	switch quote {
-	case '"':
-		// This parser supports a small SKILL.md scalar subset, not full
-		// YAML. Double quotes only unescape quoted text and Windows paths.
-		return strings.NewReplacer(`\"`, `"`, `\\`, `\`).Replace(inner)
-	case '\'':
-		return inner
-	default:
-		return value
+	stringValue, ok := value.(string)
+	if !ok {
+		return "", true, xerrors.Errorf("frontmatter field %q must be a string", key)
 	}
+	return strings.TrimRight(stringValue, "\r\n"), true, nil
 }
 
 // ParseSkillFrontmatter extracts name, description, and the
 // remaining body from a skill meta file. The expected format is
-// YAML-ish frontmatter delimited by "---" lines:
+// YAML frontmatter delimited by "---" lines:
 //
 //	---
 //	name: my-skill
@@ -78,25 +68,23 @@ func ParseSkillFrontmatter(content string) (name, description, body string, err
 		)
 	}
 
-	for _, line := range lines[1:closingIdx] {
-		key, value, ok := strings.Cut(line, ":")
-		if !ok {
-			continue
-		}
-		key = strings.TrimSpace(key)
-		value = strings.TrimSpace(value)
-		value = unquoteFrontmatterScalar(value)
-		switch strings.ToLower(key) {
-		case "name":
-			name = value
-		case "description":
-			description = value
-		}
+	frontmatterContent := strings.Join(lines[1:closingIdx], "\n")
+	var frontmatter map[string]any
+	if err := yaml.Unmarshal([]byte(frontmatterContent), &frontmatter); err != nil {
+		return "", "", "", xerrors.Errorf("parse frontmatter YAML: %w", err)
 	}
 
-	if name == "" {
+	name, ok, err := frontmatterStringField(frontmatter, "name")
+	if err != nil {
+		return "", "", "", xerrors.Errorf("%w: %v", ErrFrontmatterNameRequired, err)
+	}
+	if !ok || name == "" {
 		return "", "", "", xerrors.Errorf("%w", ErrFrontmatterNameRequired)
 	}
+	description, _, err = frontmatterStringField(frontmatter, "description")
+	if err != nil {
+		return "", "", "", err
+	}
 
 	// Everything after the closing delimiter is the body.
 	body = strings.Join(lines[closingIdx+1:], "\n")

codersdk/workspacesdk/frontmatter_test.go

@@ -1,6 +1,7 @@
 package workspacesdk_test
 
 import (
+	"strings"
 	"testing"
 
 	"github.com/stretchr/testify/require"
@@ -41,13 +42,48 @@ func TestParseSkillFrontmatter(t *testing.T) {
 		require.Equal(t, "Review \"critical\" C:\\paths.", desc)
 	})
 
-	t.Run("PlainHashValue", func(t *testing.T) {
+	t.Run("FoldedDescription", func(t *testing.T) {
+		t.Parallel()
+		name, desc, body, err := workspacesdk.ParseSkillFrontmatter(
+			strings.Join([]string{
+				"---",
+				"name: brainstorming",
+				"description: >",
+				"  Use before any creative work: features, components, functionality changes,",
+				"  or behavior modifications. Turns ideas into approved designs through",
+				"  collaborative dialog. Hard gate: no implementation action until the",
+				"  design is presented and approved.",
+				"",
+				"---",
+				"Use this skill.",
+			}, "\n"),
+		)
+		require.NoError(t, err)
+		require.Equal(t, "brainstorming", name)
+		require.Equal(t, strings.Join([]string{
+			"Use before any creative work: features, components, functionality changes,",
+			"or behavior modifications. Turns ideas into approved designs through",
+			"collaborative dialog. Hard gate: no implementation action until the",
+			"design is presented and approved.",
+		}, " "), desc)
+		require.Equal(t, "Use this skill.", body)
+	})
+
+	t.Run("YAMLComments", func(t *testing.T) {
 		t.Parallel()
 		_, desc, _, err := workspacesdk.ParseSkillFrontmatter(
 			"---\nname: plain-hash\ndescription: Build # test\n---\nBody\n",
 		)
 		require.NoError(t, err)
-		require.Equal(t, "Build # test", desc)
+		require.Equal(t, "Build", desc)
+	})
+
+	t.Run("ErrorNullDescription", func(t *testing.T) {
+		t.Parallel()
+		_, _, _, err := workspacesdk.ParseSkillFrontmatter(
+			"---\nname: null-description\ndescription: null\n---\nBody\n",
+		)
+		require.ErrorContains(t, err, `frontmatter field "description" must be a string`)
 	})
 
 	t.Run("NoDescription", func(t *testing.T) {
@@ -99,14 +135,12 @@ func TestParseSkillFrontmatter(t *testing.T) {
 		require.Empty(t, body)
 	})
 
-	t.Run("CaseInsensitiveKeys", func(t *testing.T) {
+	t.Run("YAMLKeysAreCaseSensitive", func(t *testing.T) {
 		t.Parallel()
-		name, desc, _, err := workspacesdk.ParseSkillFrontmatter(
+		_, _, _, err := workspacesdk.ParseSkillFrontmatter(
 			"---\nName: upper\nDescription: Also upper\n---\n",
 		)
-		require.NoError(t, err)
-		require.Equal(t, "upper", name)
-		require.Equal(t, "Also upper", desc)
+		require.ErrorIs(t, err, workspacesdk.ErrFrontmatterNameRequired)
 	})
 
 	t.Run("UnknownKeysIgnored", func(t *testing.T) {
@@ -139,6 +173,15 @@ func TestParseSkillFrontmatter(t *testing.T) {
 		require.ErrorContains(t, err, "frontmatter missing required 'name' field")
 	})
 
+	t.Run("ErrorNullName", func(t *testing.T) {
+		t.Parallel()
+		_, _, _, err := workspacesdk.ParseSkillFrontmatter(
+			"---\nname: null\n---\nBody\n",
+		)
+		require.ErrorIs(t, err, workspacesdk.ErrFrontmatterNameRequired)
+		require.ErrorContains(t, err, `frontmatter field "name" must be a string`)
+	})
+
 	t.Run("WhitespaceAroundDelimiters", func(t *testing.T) {
 		t.Parallel()
 		name, _, _, err := workspacesdk.ParseSkillFrontmatter(

site/src/pages/AgentsPage/utils/personalSkills.test.ts

@@ -98,7 +98,7 @@ describe("parsePersonalSkillMarkdown", () => {
 	it("parses SKILL.md frontmatter and body", () => {
 		expect(
 			parsePersonalSkillMarkdown(
-				'---\nname: test-skill\ndescription: "Does a thing"\n---\n\nUse this skill.',
+				'---\nname: "test-skill"\ndescription: "Does a thing"\n---\n\nUse this skill.',
 			),
 		).toEqual({
 			name: "test-skill",
@@ -107,14 +107,62 @@ describe("parsePersonalSkillMarkdown", () => {
 		});
 	});
 
-	it("uses backend-compatible parsing for YAML-comment-sensitive values", () => {
+	it("parses folded YAML description values", () => {
+		expect(
+			parsePersonalSkillMarkdown(
+				[
+					"---",
+					"name: brainstorming",
+					"description: >",
+					"  Use before any creative work: features, components, functionality changes,",
+					"  or behavior modifications. Turns ideas into approved designs through",
+					"  collaborative dialog. Hard gate: no implementation action until the",
+					"  design is presented and approved.",
+					"---",
+					"Use this skill.",
+				].join("\n"),
+			),
+		).toEqual({
+			name: "brainstorming",
+			description: [
+				"Use before any creative work: features, components, functionality changes,",
+				"or behavior modifications. Turns ideas into approved designs through",
+				"collaborative dialog. Hard gate: no implementation action until the",
+				"design is presented and approved.",
+			].join(" "),
+			body: "Use this skill.",
+		});
+	});
+
+	it("uses YAML comment semantics in frontmatter", () => {
 		expect(
 			parsePersonalSkillMarkdown(
 				"---\nname: test-skill\ndescription: Build # test\n---\nBody",
 			),
 		).toEqual({
 			name: "test-skill",
-			description: "Build # test",
+			description: "Build",
+			body: "Body",
+		});
+	});
+
+	it("rejects non-string frontmatter fields", () => {
+		expect(() =>
+			parsePersonalSkillMarkdown("---\nname: null\n---\nBody"),
+		).toThrow("Skill name must be a string.");
+		expect(() =>
+			parsePersonalSkillMarkdown(
+				"---\nname: test-skill\ndescription: null\n---\nBody",
+			),
+		).toThrow("Skill description must be a string.");
+	});
+
+	it("allows whitespace around frontmatter delimiters", () => {
+		expect(
+			parsePersonalSkillMarkdown("  ---  \nname: test-skill\n  ---  \nBody"),
+		).toEqual({
+			name: "test-skill",
+			description: "",
 			body: "Body",
 		});
 	});
@@ -207,6 +255,29 @@ describe("buildPersonalSkillMarkdown", () => {
 		).toBe("---\nname: test-skill\n---\nUse this skill.\n");
 	});
 
+	it("quotes skill names that YAML would otherwise coerce", () => {
+		const content = buildPersonalSkillMarkdown({
+			name: "true",
+			description: "",
+			body: "Use this skill.",
+		});
+
+		expect(content).toContain('name: "true"');
+		expect(parsePersonalSkillMarkdown(content)).toMatchObject({
+			name: "true",
+		});
+
+		const numericNameContent = buildPersonalSkillMarkdown({
+			name: "123",
+			description: "",
+			body: "Use this skill.",
+		});
+		expect(numericNameContent).toContain('name: "123"');
+		expect(parsePersonalSkillMarkdown(numericNameContent)).toMatchObject({
+			name: "123",
+		});
+	});
+
 	it("escapes quoted description values", () => {
 		const content = buildPersonalSkillMarkdown({
 			name: "test-skill",

site/src/pages/AgentsPage/utils/personalSkills.ts

@@ -1,3 +1,4 @@
+import frontMatter from "front-matter";
 import type * as TypesGen from "#/api/typesGenerated";
 
 export const PERSONAL_SKILL_MAX_SIZE_BYTES = 64 * 1024;
@@ -87,34 +88,26 @@ export const filterPersonalSkills = (
 
 class PersonalSkillMarkdownError extends Error {}
 
-const unquoteFrontmatterScalar = (value: string): string => {
-	if (value.length < 2) {
-		return value;
+const frontmatterStringField = (
+	attributes: Record<string, unknown>,
+	key: "name" | "description",
+): string => {
+	const value = attributes[key];
+	if (value === undefined) {
+		return "";
 	}
-
-	const first = value[0];
-	const last = value[value.length - 1];
-	if (first !== last) {
-		return value;
+	if (typeof value !== "string") {
+		throw new PersonalSkillMarkdownError(`Skill ${key} must be a string.`);
 	}
-
-	const inner = value.slice(1, -1);
-	if (first === '"') {
-		return inner.replaceAll('\\"', '"').replaceAll("\\\\", "\\");
-	}
-	if (first === "'") {
-		return inner;
-	}
-	return value;
+	return value.replace(/[\r\n]+$/, "");
 };
 
-// This parser is only for projecting SKILL.md content into form fields.
-// The API reparses and validates saved content on submit, so this mirrors the
-// backend scalar subset instead of accepting full YAML semantics.
+// The API re-validates on submit; this only projects content into form fields.
 export const parsePersonalSkillMarkdown = (
 	content: string,
 ): PersonalSkillFormValues => {
-	const lines = content.replace(/^\uFEFF/, "").split("\n");
+	const normalizedContent = content.replace(/^\uFEFF/, "");
+	const lines = normalizedContent.split("\n");
 	if (lines[0]?.trim() !== "---") {
 		throw new PersonalSkillMarkdownError(
 			"Missing opening frontmatter delimiter.",
@@ -130,28 +123,24 @@ export const parsePersonalSkillMarkdown = (
 		);
 	}
 
-	let name = "";
-	let description = "";
-	for (const line of lines.slice(1, closingIndex)) {
-		const separatorIndex = line.indexOf(":");
-		if (separatorIndex < 0) {
-			continue;
+	const parseableContent = [
+		"---",
+		...lines.slice(1, closingIndex),
+		"---",
+		...lines.slice(closingIndex + 1),
+	].join("\n");
+	const parsed = (() => {
+		try {
+			return frontMatter<Record<string, unknown>>(parseableContent);
+		} catch (error) {
+			const message = error instanceof Error ? error.message : "unknown error";
+			throw new PersonalSkillMarkdownError(`Invalid frontmatter: ${message}`);
 		}
-		const key = line.slice(0, separatorIndex).trim().toLowerCase();
-		const value = unquoteFrontmatterScalar(
-			line.slice(separatorIndex + 1).trim(),
-		);
-		if (key === "name") {
-			name = value;
-		} else if (key === "description") {
-			description = value;
-		}
-	}
+	})();
 
-	const body = lines
-		.slice(closingIndex + 1)
-		.join("\n")
-		.trim();
+	const name = frontmatterStringField(parsed.attributes, "name");
+	const description = frontmatterStringField(parsed.attributes, "description");
+	const body = parsed.body.trim();
 
 	if (!name) {
 		throw new PersonalSkillMarkdownError("Skill name is required.");
@@ -185,6 +174,14 @@ const frontmatterLineValue = (value: string): string =>
 const frontmatterStringValue = (value: string): string =>
 	`"${frontmatterLineValue(value).replace(/\\/g, "\\\\").replace(/"/g, '\\"')}"`;
 
+const frontmatterNameValue = (value: string): string => {
+	const lineValue = frontmatterLineValue(value);
+	if (/^(?:true|false|null)$/.test(lineValue) || /^[0-9]/.test(lineValue)) {
+		return frontmatterStringValue(lineValue);
+	}
+	return lineValue;
+};
+
 export const isValidPersonalSkillDescription = (description: string): boolean =>
 	getPersonalSkillContentSizeBytes(description) <=
 	PERSONAL_SKILL_MAX_DESCRIPTION_BYTES;
@@ -192,7 +189,7 @@ export const isValidPersonalSkillDescription = (description: string): boolean =>
 export const buildPersonalSkillMarkdown = (
 	values: PersonalSkillFormValues,
 ): string => {
-	const name = frontmatterLineValue(values.name);
+	const name = frontmatterNameValue(values.name);
 	const description = frontmatterLineValue(values.description);
 	const body = values.body.trim();
 	const frontmatter = ["---", `name: ${name}`];