Browse Source

fix: ensure frontmatter can process same content as other agents (#8719)

Aiden Cline 2 months ago
parent
commit
dc1c25cff5

+ 1 - 1
packages/opencode/src/cli/error.ts

@@ -28,7 +28,7 @@ export function FormatError(input: unknown) {
     return `Directory "${input.data.dir}" in ${input.data.path} is not valid. Rename the directory to "${input.data.suggestion}" or remove it. This is a common typo.`
   }
   if (ConfigMarkdown.FrontmatterError.isInstance(input)) {
-    return `Failed to parse frontmatter in ${input.data.path}:\n${input.data.message}`
+    return input.data.message
   }
   if (Config.InvalidError.isInstance(input))
     return [

+ 3 - 3
packages/opencode/src/config/config.ts

@@ -235,7 +235,7 @@ export namespace Config {
     })) {
       const md = await ConfigMarkdown.parse(item).catch((err) => {
         const message = ConfigMarkdown.FrontmatterError.isInstance(err)
-          ? `${err.data.path}: ${err.data.message}`
+          ? err.data.message
           : `Failed to parse command ${item}`
         Bus.publish(Session.Event.Error, { error: new NamedError.Unknown({ message }).toObject() })
         log.error("failed to load command", { command: item, err })
@@ -274,7 +274,7 @@ export namespace Config {
     })) {
       const md = await ConfigMarkdown.parse(item).catch((err) => {
         const message = ConfigMarkdown.FrontmatterError.isInstance(err)
-          ? `${err.data.path}: ${err.data.message}`
+          ? err.data.message
           : `Failed to parse agent ${item}`
         Bus.publish(Session.Event.Error, { error: new NamedError.Unknown({ message }).toObject() })
         log.error("failed to load agent", { agent: item, err })
@@ -312,7 +312,7 @@ export namespace Config {
     })) {
       const md = await ConfigMarkdown.parse(item).catch((err) => {
         const message = ConfigMarkdown.FrontmatterError.isInstance(err)
-          ? `${err.data.path}: ${err.data.message}`
+          ? err.data.message
           : `Failed to parse mode ${item}`
         Bus.publish(Session.Event.Error, { error: new NamedError.Unknown({ message }).toObject() })
         log.error("failed to load mode", { mode: item, err })

+ 54 - 2
packages/opencode/src/config/markdown.ts

@@ -14,8 +14,60 @@ export namespace ConfigMarkdown {
     return Array.from(template.matchAll(SHELL_REGEX))
   }
 
+  export function preprocessFrontmatter(content: string): string {
+    const match = content.match(/^---\r?\n([\s\S]*?)\r?\n---/)
+    if (!match) return content
+
+    const frontmatter = match[1]
+    const lines = frontmatter.split("\n")
+    const result: string[] = []
+
+    for (const line of lines) {
+      // skip comments and empty lines
+      if (line.trim().startsWith("#") || line.trim() === "") {
+        result.push(line)
+        continue
+      }
+
+      // skip lines that are continuations (indented)
+      if (line.match(/^\s+/)) {
+        result.push(line)
+        continue
+      }
+
+      // match key: value pattern
+      const kvMatch = line.match(/^([a-zA-Z_][a-zA-Z0-9_]*)\s*:\s*(.*)$/)
+      if (!kvMatch) {
+        result.push(line)
+        continue
+      }
+
+      const key = kvMatch[1]
+      const value = kvMatch[2].trim()
+
+      // skip if value is empty, already quoted, or uses block scalar
+      if (value === "" || value === ">" || value === "|" || value.startsWith('"') || value.startsWith("'")) {
+        result.push(line)
+        continue
+      }
+
+      // if value contains a colon, convert to block scalar
+      if (value.includes(":")) {
+        result.push(`${key}: |`)
+        result.push(`  ${value}`)
+        continue
+      }
+
+      result.push(line)
+    }
+
+    const processed = result.join("\n")
+    return content.replace(frontmatter, () => processed)
+  }
+
   export async function parse(filePath: string) {
-    const template = await Bun.file(filePath).text()
+    const raw = await Bun.file(filePath).text()
+    const template = preprocessFrontmatter(raw)
 
     try {
       const md = matter(template)
@@ -24,7 +76,7 @@ export namespace ConfigMarkdown {
       throw new FrontmatterError(
         {
           path: filePath,
-          message: `Failed to parse YAML frontmatter: ${err instanceof Error ? err.message : String(err)}`,
+          message: `${filePath}: Failed to parse YAML frontmatter: ${err instanceof Error ? err.message : String(err)}`,
         },
         { cause: err },
       )

+ 1 - 1
packages/opencode/src/skill/skill.ts

@@ -48,7 +48,7 @@ export namespace Skill {
     const addSkill = async (match: string) => {
       const md = await ConfigMarkdown.parse(match).catch((err) => {
         const message = ConfigMarkdown.FrontmatterError.isInstance(err)
-          ? `${err.data.path}: ${err.data.message}`
+          ? err.data.message
           : `Failed to parse skill ${match}`
         Bus.publish(Session.Event.Error, { error: new NamedError.Unknown({ message }).toObject() })
         log.error("failed to load skill", { skill: match, err })

+ 4 - 0
packages/opencode/test/config/fixtures/empty-frontmatter.md

@@ -0,0 +1,4 @@
+---
+---
+
+Content

+ 28 - 0
packages/opencode/test/config/fixtures/frontmatter.md

@@ -0,0 +1,28 @@
+---
+description: "This is a description wrapped in quotes"
+# field: this is a commented out field that should be ignored
+occupation: This man has the following occupation: Software Engineer
+title: 'Hello World'
+name: John "Doe"
+
+family: He has no 'family'
+summary: >
+  This is a summary
+url: https://example.com:8080/path?query=value
+time: The time is 12:30:00 PM
+nested: First: Second: Third: Fourth
+quoted_colon: "Already quoted: no change needed"
+single_quoted_colon: 'Single quoted: also fine'
+mixed: He said "hello: world" and then left
+empty:
+dollar: Use $' and $& for special patterns
+---
+
+Content that should not be parsed:
+
+fake_field: this is not yaml
+another: neither is this
+time: 10:30:00 AM
+url: https://should-not-be-parsed.com:3000
+
+The above lines look like YAML but are just content.

+ 1 - 0
packages/opencode/test/config/fixtures/no-frontmatter.md

@@ -0,0 +1 @@
+Content

+ 164 - 61
packages/opencode/test/config/markdown.test.ts

@@ -1,89 +1,192 @@
-import { expect, test } from "bun:test"
+import { expect, test, describe } from "bun:test"
 import { ConfigMarkdown } from "../../src/config/markdown"
 
-const template = `This is a @valid/path/to/a/file and it should also match at
-the beginning of a line:
+describe("ConfigMarkdown: normal template", () => {
+  const template = `This is a @valid/path/to/a/file and it should also match at
+  the beginning of a line:
 
-@another-valid/path/to/a/file
+  @another-valid/path/to/a/file
 
-but this is not:
+  but this is not:
 
-   - Adds a "Co-authored-by:" footer which clarifies which AI agent
-     helped create this commit, using an appropriate \`noreply@...\`
-     or \`[email protected]\` email address.
+     - Adds a "Co-authored-by:" footer which clarifies which AI agent
+       helped create this commit, using an appropriate \`noreply@...\`
+       or \`[email protected]\` email address.
 
-We also need to deal with files followed by @commas, ones
-with @file-extensions.md, even @multiple.extensions.bak,
-hidden directories like @.config/ or files like @.bashrc
-and ones at the end of a sentence like @foo.md.
+  We also need to deal with files followed by @commas, ones
+  with @file-extensions.md, even @multiple.extensions.bak,
+  hidden directories like @.config/ or files like @.bashrc
+  and ones at the end of a sentence like @foo.md.
 
-Also shouldn't forget @/absolute/paths.txt with and @/without/extensions,
-as well as @~/home-files and @~/paths/under/home.txt.
+  Also shouldn't forget @/absolute/paths.txt with and @/without/extensions,
+  as well as @~/home-files and @~/paths/under/home.txt.
 
-If the reference is \`@quoted/in/backticks\` then it shouldn't match at all.`
+  If the reference is \`@quoted/in/backticks\` then it shouldn't match at all.`
 
-const matches = ConfigMarkdown.files(template)
+  const matches = ConfigMarkdown.files(template)
 
-test("should extract exactly 12 file references", () => {
-  expect(matches.length).toBe(12)
-})
+  test("should extract exactly 12 file references", () => {
+    expect(matches.length).toBe(12)
+  })
 
-test("should extract valid/path/to/a/file", () => {
-  expect(matches[0][1]).toBe("valid/path/to/a/file")
-})
+  test("should extract valid/path/to/a/file", () => {
+    expect(matches[0][1]).toBe("valid/path/to/a/file")
+  })
 
-test("should extract another-valid/path/to/a/file", () => {
-  expect(matches[1][1]).toBe("another-valid/path/to/a/file")
-})
+  test("should extract another-valid/path/to/a/file", () => {
+    expect(matches[1][1]).toBe("another-valid/path/to/a/file")
+  })
 
-test("should extract paths ignoring comma after", () => {
-  expect(matches[2][1]).toBe("commas")
-})
+  test("should extract paths ignoring comma after", () => {
+    expect(matches[2][1]).toBe("commas")
+  })
 
-test("should extract a path with a file extension and comma after", () => {
-  expect(matches[3][1]).toBe("file-extensions.md")
-})
+  test("should extract a path with a file extension and comma after", () => {
+    expect(matches[3][1]).toBe("file-extensions.md")
+  })
 
-test("should extract a path with multiple dots and comma after", () => {
-  expect(matches[4][1]).toBe("multiple.extensions.bak")
-})
+  test("should extract a path with multiple dots and comma after", () => {
+    expect(matches[4][1]).toBe("multiple.extensions.bak")
+  })
 
-test("should extract hidden directory", () => {
-  expect(matches[5][1]).toBe(".config/")
-})
+  test("should extract hidden directory", () => {
+    expect(matches[5][1]).toBe(".config/")
+  })
 
-test("should extract hidden file", () => {
-  expect(matches[6][1]).toBe(".bashrc")
-})
+  test("should extract hidden file", () => {
+    expect(matches[6][1]).toBe(".bashrc")
+  })
 
-test("should extract a file ignoring period at end of sentence", () => {
-  expect(matches[7][1]).toBe("foo.md")
-})
+  test("should extract a file ignoring period at end of sentence", () => {
+    expect(matches[7][1]).toBe("foo.md")
+  })
 
-test("should extract an absolute path with an extension", () => {
-  expect(matches[8][1]).toBe("/absolute/paths.txt")
-})
+  test("should extract an absolute path with an extension", () => {
+    expect(matches[8][1]).toBe("/absolute/paths.txt")
+  })
 
-test("should extract an absolute path without an extension", () => {
-  expect(matches[9][1]).toBe("/without/extensions")
-})
+  test("should extract an absolute path without an extension", () => {
+    expect(matches[9][1]).toBe("/without/extensions")
+  })
+
+  test("should extract an absolute path in home directory", () => {
+    expect(matches[10][1]).toBe("~/home-files")
+  })
 
-test("should extract an absolute path in home directory", () => {
-  expect(matches[10][1]).toBe("~/home-files")
+  test("should extract an absolute path under home directory", () => {
+    expect(matches[11][1]).toBe("~/paths/under/home.txt")
+  })
+
+  test("should not match when preceded by backtick", () => {
+    const backtickTest = "This `@should/not/match` should be ignored"
+    const backtickMatches = ConfigMarkdown.files(backtickTest)
+    expect(backtickMatches.length).toBe(0)
+  })
+
+  test("should not match email addresses", () => {
+    const emailTest = "Contact [email protected] for help"
+    const emailMatches = ConfigMarkdown.files(emailTest)
+    expect(emailMatches.length).toBe(0)
+  })
 })
 
-test("should extract an absolute path under home directory", () => {
-  expect(matches[11][1]).toBe("~/paths/under/home.txt")
+describe("ConfigMarkdown: frontmatter parsing", async () => {
+  const parsed = await ConfigMarkdown.parse(import.meta.dir + "/fixtures/frontmatter.md")
+
+  test("should parse without throwing", () => {
+    expect(parsed).toBeDefined()
+    expect(parsed.data).toBeDefined()
+    expect(parsed.content).toBeDefined()
+  })
+
+  test("should extract description field", () => {
+    expect(parsed.data.description).toBe("This is a description wrapped in quotes")
+  })
+
+  test("should extract occupation field with colon in value", () => {
+    expect(parsed.data.occupation).toBe("This man has the following occupation: Software Engineer\n")
+  })
+
+  test("should extract title field with single quotes", () => {
+    expect(parsed.data.title).toBe("Hello World")
+  })
+
+  test("should extract name field with embedded quotes", () => {
+    expect(parsed.data.name).toBe('John "Doe"')
+  })
+
+  test("should extract family field with embedded single quotes", () => {
+    expect(parsed.data.family).toBe("He has no 'family'")
+  })
+
+  test("should extract multiline summary field", () => {
+    expect(parsed.data.summary).toBe("This is a summary\n")
+  })
+
+  test("should not include commented fields in data", () => {
+    expect(parsed.data.field).toBeUndefined()
+  })
+
+  test("should extract URL with port", () => {
+    expect(parsed.data.url).toBe("https://example.com:8080/path?query=value\n")
+  })
+
+  test("should extract time with colons", () => {
+    expect(parsed.data.time).toBe("The time is 12:30:00 PM\n")
+  })
+
+  test("should extract value with multiple colons", () => {
+    expect(parsed.data.nested).toBe("First: Second: Third: Fourth\n")
+  })
+
+  test("should preserve already double-quoted values with colons", () => {
+    expect(parsed.data.quoted_colon).toBe("Already quoted: no change needed")
+  })
+
+  test("should preserve already single-quoted values with colons", () => {
+    expect(parsed.data.single_quoted_colon).toBe("Single quoted: also fine")
+  })
+
+  test("should extract value with quotes and colons mixed", () => {
+    expect(parsed.data.mixed).toBe('He said "hello: world" and then left\n')
+  })
+
+  test("should handle empty values", () => {
+    expect(parsed.data.empty).toBeNull()
+  })
+
+  test("should handle dollar sign replacement patterns literally", () => {
+    expect(parsed.data.dollar).toBe("Use $' and $& for special patterns")
+  })
+
+  test("should not parse fake yaml from content", () => {
+    expect(parsed.data.fake_field).toBeUndefined()
+    expect(parsed.data.another).toBeUndefined()
+  })
+
+  test("should extract content after frontmatter without modification", () => {
+    expect(parsed.content).toContain("Content that should not be parsed:")
+    expect(parsed.content).toContain("fake_field: this is not yaml")
+    expect(parsed.content).toContain("url: https://should-not-be-parsed.com:3000")
+  })
 })
 
-test("should not match when preceded by backtick", () => {
-  const backtickTest = "This `@should/not/match` should be ignored"
-  const backtickMatches = ConfigMarkdown.files(backtickTest)
-  expect(backtickMatches.length).toBe(0)
+describe("ConfigMarkdown: frontmatter parsing w/ empty frontmatter", async () => {
+  const result = await ConfigMarkdown.parse(import.meta.dir + "/fixtures/empty-frontmatter.md")
+
+  test("should parse without throwing", () => {
+    expect(result).toBeDefined()
+    expect(result.data).toEqual({})
+    expect(result.content.trim()).toBe("Content")
+  })
 })
 
-test("should not match email addresses", () => {
-  const emailTest = "Contact [email protected] for help"
-  const emailMatches = ConfigMarkdown.files(emailTest)
-  expect(emailMatches.length).toBe(0)
+describe("ConfigMarkdown: frontmatter parsing w/ no frontmatter", async () => {
+  const result = await ConfigMarkdown.parse(import.meta.dir + "/fixtures/no-frontmatter.md")
+
+  test("should parse without throwing", () => {
+    expect(result).toBeDefined()
+    expect(result.data).toEqual({})
+    expect(result.content.trim()).toBe("Content")
+  })
 })