diff --git a/coderd/x/skills/skills_test.go b/coderd/x/skills/skills_test.go index 6c40dad601..5cb53de2ea 100644 --- a/coderd/x/skills/skills_test.go +++ b/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() diff --git a/codersdk/workspacesdk/frontmatter.go b/codersdk/workspacesdk/frontmatter.go index c70be57d74..74c67adaa9 100644 --- a/codersdk/workspacesdk/frontmatter.go +++ b/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(``) // 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") diff --git a/codersdk/workspacesdk/frontmatter_test.go b/codersdk/workspacesdk/frontmatter_test.go index c1155a641f..0f76d68490 100644 --- a/codersdk/workspacesdk/frontmatter_test.go +++ b/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( diff --git a/site/src/pages/AgentsPage/utils/personalSkills.test.ts b/site/src/pages/AgentsPage/utils/personalSkills.test.ts index 5c40910b2d..881362f0db 100644 --- a/site/src/pages/AgentsPage/utils/personalSkills.test.ts +++ b/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", diff --git a/site/src/pages/AgentsPage/utils/personalSkills.ts b/site/src/pages/AgentsPage/utils/personalSkills.ts index 8f3baa3f48..c7f38b97bb 100644 --- a/site/src/pages/AgentsPage/utils/personalSkills.ts +++ b/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, + 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>(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}`];