diff --git a/apps/cli/src/ui/interactive-mode.ts b/apps/cli/src/ui/interactive-mode.ts index b0d7f919..13fe39b1 100644 --- a/apps/cli/src/ui/interactive-mode.ts +++ b/apps/cli/src/ui/interactive-mode.ts @@ -1933,8 +1933,19 @@ export class InteractiveMode { for (const d of otherDiagnostics) { if (d.path) { const formattedPath = this.formatPathWithSource(d.path, this.findSourceInfoForPath(d.path, sourceInfos)); - lines.push(theme.fg(d.type === "error" ? "error" : "warning", ` ${formattedPath}`)); - lines.push(theme.fg(d.type === "error" ? "error" : "warning", ` ${d.message}`)); + // Some diagnostics (e.g. skill frontmatter errors) already name their file so they stay + // readable outside this list; don't print the path twice. + if (d.message.startsWith(d.path)) { + lines.push( + theme.fg( + d.type === "error" ? "error" : "warning", + ` ${formattedPath}${d.message.slice(d.path.length)}`, + ), + ); + } else { + lines.push(theme.fg(d.type === "error" ? "error" : "warning", ` ${formattedPath}`)); + lines.push(theme.fg(d.type === "error" ? "error" : "warning", ` ${d.message}`)); + } } else { lines.push(theme.fg(d.type === "error" ? "error" : "warning", ` ${d.message}`)); } diff --git a/apps/cli/test/interactive-mode-diagnostics.test.ts b/apps/cli/test/interactive-mode-diagnostics.test.ts new file mode 100644 index 00000000..58dda21c --- /dev/null +++ b/apps/cli/test/interactive-mode-diagnostics.test.ts @@ -0,0 +1,42 @@ +import { stripVTControlCharacters } from "node:util"; +import { beforeAll, describe, expect, it } from "vitest"; +import type { ResourceDiagnostic } from "../../../packages/coding-agent/src/core/diagnostics.ts"; +import type { SourceInfo } from "../../../packages/coding-agent/src/core/source-info.ts"; +import { initTheme } from "../../../packages/coding-agent/src/theme/theme.ts"; +import { InteractiveMode } from "../src/ui/interactive-mode.ts"; + +type DiagnosticFormatter = { + formatDiagnostics(diagnostics: ResourceDiagnostic[], sources: Map): string; + formatPathWithSource(path: string): string; + findSourceInfoForPath(): undefined; +}; + +function format(diagnostics: ResourceDiagnostic[]): string { + const mode = Object.create(InteractiveMode.prototype) as DiagnosticFormatter; + mode.formatPathWithSource = () => "[project] example/SKILL.md"; + mode.findSourceInfoForPath = () => undefined; + return stripVTControlCharacters(mode.formatDiagnostics(diagnostics, new Map())); +} + +describe("resource diagnostics", () => { + beforeAll(() => initTheme("dark")); + + it.each([ + "invalid YAML frontmatter at line 3, column 14: invalid YAML", + 'frontmatter is missing "description" — add e.g. description: "..."', + ])("prints a self-contained frontmatter diagnostic on one line: %s", (reason) => { + const path = "/skills/example/SKILL.md"; + const output = format([{ type: "warning", path, message: `${path}: ${reason}` }]); + expect(output).toBe(` [project] example/SKILL.md: ${reason}`); + }); + + it("preserves the separate path line for other diagnostics", () => { + expect(format([{ type: "error", path: "/skills/example/SKILL.md", message: "cannot read file" }])).toBe( + " [project] example/SKILL.md\n cannot read file", + ); + }); + + it("renders diagnostics without paths", () => { + expect(format([{ type: "warning", message: "no skill directories found" }])).toBe(" no skill directories found"); + }); +}); diff --git a/packages/agent-core/src/harness/skills.ts b/packages/agent-core/src/harness/skills.ts index 3cb89857..9072d1c7 100644 --- a/packages/agent-core/src/harness/skills.ts +++ b/packages/agent-core/src/harness/skills.ts @@ -1,4 +1,4 @@ -import { parse } from "yaml"; +import { isMap, isScalar, parse, parseDocument } from "yaml"; import { SkillIgnoreMatcher } from "./skill-discovery.ts"; import { type ExecutionEnv, type FileInfo, type Result, type Skill, toError } from "./types.ts"; @@ -240,7 +240,12 @@ async function loadSkillFromFile( const parsed = parseFrontmatter(rawContent.value); if (!parsed.ok) { if (isDeclaredSkill) { - diagnostics.push({ type: "warning", code: "parse_failed", message: parsed.error.message, path: filePath }); + diagnostics.push({ + type: "warning", + code: "parse_failed", + message: formatFrontmatterError(filePath, parsed.error, rawContent.value), + path: filePath, + }); } return { skill: null, diagnostics }; } @@ -251,7 +256,7 @@ async function loadSkillFromFile( return { skill: null, diagnostics }; } - for (const error of validateDescription(description)) { + for (const error of validateDescription(description, filePath)) { diagnostics.push({ type: "warning", code: "invalid_metadata", message: error, path: filePath }); } @@ -289,35 +294,116 @@ function validateName(name: string, parentDirName: string): string[] { return errors; } -function validateDescription(description: string | undefined): string[] { +/** + * Validate description per Agent Skills spec. + * + * `filePath` keeps the "missing description" diagnostic self-contained so it stays actionable + * wherever diagnostics are surfaced. + */ +function validateDescription(description: string | undefined, filePath: string): string[] { const errors: string[] = []; if (!description || description.trim() === "") { - errors.push("description is required"); + errors.push(`${filePath}: frontmatter is missing "description" — add e.g. description: "..."`); } else if (description.length > MAX_DESCRIPTION_LENGTH) { errors.push(`description exceeds ${MAX_DESCRIPTION_LENGTH} characters (${description.length})`); } return errors; } +function extractFrontmatterBlock(content: string): { yamlString: string | null; body: string } { + const normalized = content + .replace(/^\uFEFF/, "") + .replace(/\r\n/g, "\n") + .replace(/\r/g, "\n"); + if (!normalized.startsWith("---")) return { yamlString: null, body: normalized }; + const endIndex = normalized.indexOf("\n---", 3); + if (endIndex === -1) return { yamlString: null, body: normalized }; + return { yamlString: normalized.slice(4, endIndex), body: normalized.slice(endIndex + 4).trim() }; +} + function parseFrontmatter>( content: string, ): Result<{ frontmatter: T; body: string }, Error> { try { - const normalized = content - .replace(/^\uFEFF/, "") - .replace(/\r\n/g, "\n") - .replace(/\r/g, "\n"); - if (!normalized.startsWith("---")) return { ok: true, value: { frontmatter: {} as T, body: normalized } }; - const endIndex = normalized.indexOf("\n---", 3); - if (endIndex === -1) return { ok: true, value: { frontmatter: {} as T, body: normalized } }; - const yamlString = normalized.slice(4, endIndex); - const body = normalized.slice(endIndex + 4).trim(); + const { yamlString, body } = extractFrontmatterBlock(content); + if (yamlString === null) return { ok: true, value: { frontmatter: {} as T, body } }; return { ok: true, value: { frontmatter: (parse(yamlString) ?? {}) as T, body } }; } catch (error) { return { ok: false, error: toError(error) }; } } +/** + * The YAML block always starts on file line 2 (right after the opening `---`), so parser line + * numbers are shifted to point at the line a user actually edits. + */ +const YAML_TO_FILE_LINE_OFFSET = 1; + +/** + * Flatten a `yaml` parse error into a single line. + * + * The `yaml` package appends a quoted (and possibly truncated) source line plus a caret to its + * messages, which wraps into an unreadable block inside the diagnostics list. + */ +function describeFrontmatterError(error: unknown): { line?: number; column?: number; reason: string } { + const message = error instanceof Error ? error.message : String(error); + const headline = (message.split("\n")[0] ?? "").replace( + /[\u0000-\u001f\u007f-\u009f\u2028\u2029]/g, + (character) => `\\u${character.charCodeAt(0).toString(16).padStart(4, "0")}`, + ); + const location = /^(.*?)\s+at line (\d+), column (\d+)/.exec(headline ?? ""); + const position = (error as { linePos?: Array<{ line?: number; col?: number }> })?.linePos?.[0]; + const line = position?.line ?? (location ? Number(location[2]) : undefined); + const column = position?.col ?? (location ? Number(location[3]) : undefined); + const reason = (location?.[1] ?? headline ?? "").trim() || "invalid YAML"; + + return { + line: line === undefined ? undefined : line + YAML_TO_FILE_LINE_OFFSET, + column, + reason, + }; +} + +/** Detect a rejected compact mapping in the top-level description value. */ +function hasUnquotedColonInDescription(content: string): boolean { + const { yamlString } = extractFrontmatterBlock(content); + if (!yamlString) return false; + + // Inspect the parser's recovered tree so comments, quoted strings and block scalar + // contents cannot be mistaken for description keys. + const document = parseDocument(yamlString); + if (!isMap(document.contents)) return false; + return document.contents.items.some(({ key, value }) => { + if (!isScalar(key) || key.value !== "description" || !isMap(value)) return false; + const start = value.range?.[0]; + const keyEnd = key.range?.[1]; + if (start === undefined || keyEnd === undefined) return false; + // A normal nested mapping on the next line is not an unquoted description. + if (yamlString.slice(keyEnd, start).includes("\n")) return false; + return document.errors.some((error) => error.code === "BLOCK_AS_IMPLICIT_KEY" && error.pos[0] === start); + }); +} + +/** + * Build a single-line, actionable diagnostic for a frontmatter syntax error, e.g. + * `/path/SKILL.md: invalid YAML frontmatter at line 3, column 14: Nested mappings are not allowed + * in compact mappings — description contains an unquoted ":" — wrap the value in double quotes`. + */ +function formatFrontmatterError(filePath: string, error: unknown, content: string): string { + const { line, column, reason } = describeFrontmatterError(error); + const at = + line !== undefined && column !== undefined + ? ` at line ${line}, column ${column}` + : line !== undefined + ? ` at line ${line}` + : ""; + const hint = hasUnquotedColonInDescription(content) + ? ' — description contains an unquoted ":" — wrap the value in double quotes' + : ""; + + return `${filePath}: invalid YAML frontmatter${at}: ${reason}${hint}`; +} + async function resolveKind( env: ExecutionEnv, info: FileInfo, diff --git a/packages/agent-core/test/harness/skills.test.ts b/packages/agent-core/test/harness/skills.test.ts index 88c15594..412a7970 100644 --- a/packages/agent-core/test/harness/skills.test.ts +++ b/packages/agent-core/test/harness/skills.test.ts @@ -94,13 +94,78 @@ Use this skill. { type: "warning", code: "invalid_metadata", - message: "description is required", + message: `${join(root, "user/broken/SKILL.md")}: frontmatter is missing "description" — add e.g. description: "..."`, path: join(root, "user/broken/SKILL.md"), source: { type: "user" }, }, ]); }); + it("reports a single-line diagnostic for an unquoted colon in the description", async () => { + const root = createTempDir(); + const env = new NodeExecutionEnv({ cwd: root }); + await env.createDir("user/search-first", { recursive: true }); + await env.writeFile( + "user/search-first/SKILL.md", + "---\nname: search-first\ndescription: Search the web: before guessing\n---\nUse this skill.", + ); + + const { skills, diagnostics } = await loadSkills(env, "user"); + + expect(skills).toEqual([]); + expect(diagnostics).toEqual([ + { + type: "warning", + code: "parse_failed", + message: + `${join(root, "user/search-first/SKILL.md")}: invalid YAML frontmatter at line 3, column 14: ` + + 'Nested mappings are not allowed in compact mappings — description contains an unquoted ":" — ' + + "wrap the value in double quotes", + path: join(root, "user/search-first/SKILL.md"), + }, + ]); + }); + + it("loads a description whose colon is quoted", async () => { + const root = createTempDir(); + const env = new NodeExecutionEnv({ cwd: root }); + await env.createDir("user/search-first", { recursive: true }); + await env.writeFile( + "user/search-first/SKILL.md", + '---\nname: search-first\ndescription: "Search the web: before guessing"\n---\nUse this skill.', + ); + + const { skills, diagnostics } = await loadSkills(env, "user"); + + expect(diagnostics).toEqual([]); + expect(skills.map((skill) => skill.description)).toEqual(["Search the web: before guessing"]); + }); + + it.each([ + "description: # note: later\nother: [unclosed", + "description: |\n description: harmless: text\nother: [unclosed", + ])("omits quoting hints for valid description syntax: %s", async (yaml) => { + const root = createTempDir(); + const env = new NodeExecutionEnv({ cwd: root }); + await env.createDir("user/example", { recursive: true }); + await env.writeFile("user/example/SKILL.md", `---\nname: example\n${yaml}\n---`); + const { diagnostics } = await loadSkills(env, "user"); + expect(diagnostics).toHaveLength(1); + expect(diagnostics[0].code).toBe("parse_failed"); + expect(diagnostics[0].message).not.toContain("wrap the value"); + }); + + it("escapes terminal control characters in parser reasons", async () => { + const root = createTempDir(); + const env = new NodeExecutionEnv({ cwd: root }); + await env.createDir("user/example", { recursive: true }); + await env.writeFile("user/example/SKILL.md", '---\nname: example\ndescription: "bad\\\u001b[31m"\n---'); + const { diagnostics } = await loadSkills(env, "user"); + expect(diagnostics).toHaveLength(1); + expect(diagnostics[0].message).not.toContain("\u001b"); + expect(diagnostics[0].message).toContain("\\u001b"); + }); + it("loads direct markdown children only from the root directory", async () => { const root = createTempDir(); const env = new NodeExecutionEnv({ cwd: root }); diff --git a/packages/coding-agent/src/core/skills.ts b/packages/coding-agent/src/core/skills.ts index 31da06bc..007fe030 100644 --- a/packages/coding-agent/src/core/skills.ts +++ b/packages/coding-agent/src/core/skills.ts @@ -2,7 +2,7 @@ import { SkillIgnoreMatcher } from "@step-harness/agent-core"; import { existsSync, readdirSync, readFileSync, statSync } from "fs"; import { basename, dirname, join, relative, resolve, sep } from "path"; import { CONFIG_DIR_NAME, getAgentDir } from "../config.ts"; -import { parseFrontmatter } from "../utils/frontmatter.ts"; +import { formatFrontmatterError, parseFrontmatter } from "../utils/frontmatter.ts"; import { canonicalizePath, resolvePath } from "../utils/paths.ts"; import type { ResourceDiagnostic } from "./diagnostics.ts"; import { createSyntheticSourceInfo, type SourceInfo } from "./source-info.ts"; @@ -76,12 +76,15 @@ function validateName(name: string): string[] { /** * Validate description per Agent Skills spec. + * + * `filePath` keeps the "missing description" diagnostic self-contained so it stays actionable + * wherever diagnostics are surfaced. */ -function validateDescription(description: unknown): string[] { +function validateDescription(description: unknown, filePath: string): string[] { const errors: string[] = []; if (typeof description !== "string" || description.trim() === "") { - errors.push("description is required"); + errors.push(`${filePath}: frontmatter is missing "description" — add e.g. description: "..."`); } else if (description.length > MAX_DESCRIPTION_LENGTH) { errors.push(`description exceeds ${MAX_DESCRIPTION_LENGTH} characters (${description.length})`); } @@ -263,8 +266,11 @@ function loadSkillFromFile( ({ frontmatter } = parseFrontmatter(rawContent)); } catch (error) { if (isDeclaredSkill) { - const message = error instanceof Error ? error.message : "failed to parse skill file"; - diagnostics.push({ type: "warning", message, path: filePath }); + diagnostics.push({ + type: "warning", + message: formatFrontmatterError(filePath, error, rawContent), + path: filePath, + }); } return { skill: null, diagnostics }; } @@ -279,7 +285,7 @@ function loadSkillFromFile( const parentDirName = basename(skillDir); // Validate description - const descErrors = validateDescription(description); + const descErrors = validateDescription(description, filePath); for (const error of descErrors) { diagnostics.push({ type: "warning", message: error, path: filePath }); } diff --git a/packages/coding-agent/src/utils/frontmatter.ts b/packages/coding-agent/src/utils/frontmatter.ts index 54481073..85df5c64 100644 --- a/packages/coding-agent/src/utils/frontmatter.ts +++ b/packages/coding-agent/src/utils/frontmatter.ts @@ -1,4 +1,4 @@ -import { parse } from "yaml"; +import { isMap, isScalar, parse, parseDocument } from "yaml"; import { stripBom } from "./text.ts"; type ParsedFrontmatter> = { @@ -6,6 +6,16 @@ type ParsedFrontmatter> = { body: string; }; +/** Location and reason of a frontmatter syntax error, flattened onto a single line. */ +export interface FrontmatterErrorDetails { + /** 1-based line in the file, when the parser reports one. */ + line?: number; + /** 1-based column in the file, when the parser reports one. */ + column?: number; + /** Parser reason, without the quoted source line or caret. */ + reason: string; +} + const normalizeNewlines = (value: string): string => value.replace(/\r\n/g, "\n").replace(/\r/g, "\n"); const extractFrontmatter = (content: string): { yamlString: string | null; body: string } => { @@ -38,3 +48,74 @@ export const parseFrontmatter = = Record parseFrontmatter(content).body; + +/** + * The YAML block always starts on file line 2 (right after the opening `---`), so parser line + * numbers are shifted to point at the line a user actually edits. + */ +const YAML_TO_FILE_LINE_OFFSET = 1; + +/** + * Flatten a `yaml` parse error into a single line. + * + * The `yaml` package appends a quoted (and possibly truncated) source line plus a caret to its + * messages, which wraps into an unreadable block inside the diagnostics list. + */ +export function describeFrontmatterError(error: unknown): FrontmatterErrorDetails { + const message = error instanceof Error ? error.message : String(error); + const headline = (message.split("\n")[0] ?? "").replace( + /[\u0000-\u001f\u007f-\u009f\u2028\u2029]/g, + (character) => `\\u${character.charCodeAt(0).toString(16).padStart(4, "0")}`, + ); + const location = /^(.*?)\s+at line (\d+), column (\d+)/.exec(headline ?? ""); + const position = (error as { linePos?: Array<{ line?: number; col?: number }> })?.linePos?.[0]; + const line = position?.line ?? (location ? Number(location[2]) : undefined); + const column = position?.col ?? (location ? Number(location[3]) : undefined); + const reason = (location?.[1] ?? headline ?? "").trim() || "invalid YAML"; + + return { + line: line === undefined ? undefined : line + YAML_TO_FILE_LINE_OFFSET, + column, + reason, + }; +} + +/** Detect a rejected compact mapping in the top-level description value. */ +export function hasUnquotedColonInDescription(content: string): boolean { + const { yamlString } = extractFrontmatter(content); + if (!yamlString) return false; + + // Inspect the parser's recovered tree so comments, quoted strings and block scalar + // contents cannot be mistaken for description keys. + const document = parseDocument(yamlString); + if (!isMap(document.contents)) return false; + return document.contents.items.some(({ key, value }) => { + if (!isScalar(key) || key.value !== "description" || !isMap(value)) return false; + const start = value.range?.[0]; + const keyEnd = key.range?.[1]; + if (start === undefined || keyEnd === undefined) return false; + // A normal nested mapping on the next line is not an unquoted description. + if (yamlString.slice(keyEnd, start).includes("\n")) return false; + return document.errors.some((error) => error.code === "BLOCK_AS_IMPLICIT_KEY" && error.pos[0] === start); + }); +} + +/** + * Build a single-line, actionable diagnostic for a frontmatter syntax error, e.g. + * `/path/SKILL.md: invalid YAML frontmatter at line 3, column 14: Nested mappings are not allowed + * in compact mappings — description contains an unquoted ":" — wrap the value in double quotes`. + */ +export function formatFrontmatterError(filePath: string, error: unknown, content: string): string { + const { line, column, reason } = describeFrontmatterError(error); + const at = + line !== undefined && column !== undefined + ? ` at line ${line}, column ${column}` + : line !== undefined + ? ` at line ${line}` + : ""; + const hint = hasUnquotedColonInDescription(content) + ? ' — description contains an unquoted ":" — wrap the value in double quotes' + : ""; + + return `${filePath}: invalid YAML frontmatter${at}: ${reason}${hint}`; +} diff --git a/packages/coding-agent/test/fixtures/skills/quoted-colon/SKILL.md b/packages/coding-agent/test/fixtures/skills/quoted-colon/SKILL.md new file mode 100644 index 00000000..4d9fae32 --- /dev/null +++ b/packages/coding-agent/test/fixtures/skills/quoted-colon/SKILL.md @@ -0,0 +1,8 @@ +--- +name: quoted-colon +description: "Enforce a claim-to-proof workflow: define the evidence, then require it" +--- + +# Quoted Colon + +This skill has a correctly quoted colon in its description. diff --git a/packages/coding-agent/test/fixtures/skills/unquoted-colon/SKILL.md b/packages/coding-agent/test/fixtures/skills/unquoted-colon/SKILL.md new file mode 100644 index 00000000..0e835a35 --- /dev/null +++ b/packages/coding-agent/test/fixtures/skills/unquoted-colon/SKILL.md @@ -0,0 +1,8 @@ +--- +name: unquoted-colon +description: Enforce a claim-to-proof workflow: define the evidence, then require it +--- + +# Unquoted Colon + +This skill has an unquoted colon in its description. diff --git a/packages/coding-agent/test/frontmatter.test.ts b/packages/coding-agent/test/frontmatter.test.ts index 43a92985..4512b133 100644 --- a/packages/coding-agent/test/frontmatter.test.ts +++ b/packages/coding-agent/test/frontmatter.test.ts @@ -1,5 +1,11 @@ import { describe, expect, it } from "vitest"; -import { parseFrontmatter, stripFrontmatter } from "../src/utils/frontmatter.ts"; +import { + describeFrontmatterError, + formatFrontmatterError, + hasUnquotedColonInDescription, + parseFrontmatter, + stripFrontmatter, +} from "../src/utils/frontmatter.ts"; describe("parseFrontmatter", () => { it("parses keys, strips quotes, and returns body", () => { @@ -47,6 +53,115 @@ describe("parseFrontmatter", () => { }); }); +describe("frontmatter error diagnostics", () => { + const unquotedColon = "---\nname: search-first\ndescription: Search the web: before guessing\n---\nBody"; + + const captureError = (content: string): unknown => { + try { + parseFrontmatter(content); + } catch (error) { + return error; + } + throw new Error("expected parseFrontmatter to throw"); + }; + + it("flattens a YAML error onto a single line with file-relative coordinates", () => { + const details = describeFrontmatterError(captureError(unquotedColon)); + + expect(details).toEqual({ + line: 3, + column: 14, + reason: "Nested mappings are not allowed in compact mappings", + }); + expect(JSON.stringify(details)).not.toContain("\\n"); + }); + + it("keeps the parser reason for errors without a source snippet", () => { + expect(describeFrontmatterError(new Error("boom"))).toEqual({ + line: undefined, + column: undefined, + reason: "boom", + }); + expect(describeFrontmatterError("nope")).toEqual({ line: undefined, column: undefined, reason: "nope" }); + }); + + it("formats a single-line message that names the file, location, reason and fix", () => { + expect(formatFrontmatterError("/skills/search-first/SKILL.md", captureError(unquotedColon), unquotedColon)).toBe( + '/skills/search-first/SKILL.md: invalid YAML frontmatter at line 3, column 14: Nested mappings are not allowed in compact mappings — description contains an unquoted ":" — wrap the value in double quotes', + ); + }); + + it("omits the colon hint for unrelated YAML errors", () => { + const broken = "---\nname: broken\ndescription: [unclosed\n---\nBody"; + const message = formatFrontmatterError("/skills/broken/SKILL.md", captureError(broken), broken); + + expect(message).toBe( + "/skills/broken/SKILL.md: invalid YAML frontmatter at line 3, column 23: " + + "Flow sequence in block collection must be sufficiently indented and end with a ]", + ); + }); + + it("omits the location when the parser does not report one", () => { + expect(formatFrontmatterError("/skills/broken/SKILL.md", new Error("boom"), unquotedColon)).toBe( + "/skills/broken/SKILL.md: invalid YAML frontmatter: boom — " + + 'description contains an unquoted ":" — wrap the value in double quotes', + ); + }); + + it.each([null, undefined])("handles an absent error (%s)", (error) => { + expect(() => describeFrontmatterError(error)).not.toThrow(); + }); + + it("escapes control characters in parser reasons", () => { + const broken = '---\ndescription: "bad\\\u001b[31m"\n---'; + const message = formatFrontmatterError("/skills/broken/SKILL.md", captureError(broken), broken); + expect(message).not.toContain("\u001b"); + expect(message).toContain("\\u001b"); + }); + + it.each([ + "description: # note: later\nother: [unclosed", + "description: |\n description: harmless: text\nother: [unclosed", + "notes: |\n description: harmless: text\ndescription: valid\nother: [unclosed", + ])("does not recommend quoting valid descriptions: %s", (yaml) => { + const broken = `---\n${yaml}\n---`; + expect(hasUnquotedColonInDescription(broken)).toBe(false); + expect(formatFrontmatterError("/skills/broken/SKILL.md", captureError(broken), broken)).not.toContain( + "wrap the value", + ); + }); + + it.each(["\n", "\r\n", "\r"])("preserves file-relative coordinates with %j line endings", (newline) => { + const broken = `\uFEFF${unquotedColon.replaceAll("\n", newline)}`; + expect(describeFrontmatterError(captureError(broken))).toMatchObject({ line: 3, column: 14 }); + }); + + describe("hasUnquotedColonInDescription", () => { + it("detects an unquoted colon in the description value", () => { + expect(hasUnquotedColonInDescription(unquotedColon)).toBe(true); + }); + + it("detects an unquoted colon on a continued description line", () => { + const content = + "---\nname: x\ndescription: Enforce a claim-to-proof workflow\n define the evidence: then require it\n---\nBody"; + expect(hasUnquotedColonInDescription(content)).toBe(true); + }); + + it("accepts quoted, block and flow-safe descriptions", () => { + expect(hasUnquotedColonInDescription('---\ndescription: "Enforce a claim: to proof"\n---\n')).toBe(false); + expect(hasUnquotedColonInDescription("---\ndescription: 'Enforce a claim: to proof'\n---\n")).toBe(false); + expect(hasUnquotedColonInDescription("---\ndescription: |\n Enforce a claim: to proof\n---\n")).toBe(false); + expect(hasUnquotedColonInDescription("---\ndescription: Enforce a claim-to-proof workflow\n---\n")).toBe( + false, + ); + expect(hasUnquotedColonInDescription("---\ndescription: Runs at 10:30 daily\n---\n")).toBe(false); + expect(hasUnquotedColonInDescription("---\ndescription: See https://example.com/x\n---\n")).toBe(false); + expect(hasUnquotedColonInDescription("---\ndescription: Enforce a claim # note: later\n---\n")).toBe(false); + expect(hasUnquotedColonInDescription("# no frontmatter: at all")).toBe(false); + }); + }); +}); + describe("stripFrontmatter", () => { it("removes frontmatter and trims body", () => { const input = "---\nkey: value\n---\n\nBody\n"; diff --git a/packages/coding-agent/test/skills.test.ts b/packages/coding-agent/test/skills.test.ts index 03980199..1e234381 100644 --- a/packages/coding-agent/test/skills.test.ts +++ b/packages/coding-agent/test/skills.test.ts @@ -81,7 +81,10 @@ describe("skills", () => { }); expect(skills).toHaveLength(0); - expect(diagnostics.some((d: ResourceDiagnostic) => d.message.includes("description is required"))).toBe(true); + const messages = diagnostics.map((d: ResourceDiagnostic) => d.message); + expect(messages).toContain( + `${join(fixturesDir, "missing-description", "SKILL.md")}: frontmatter is missing "description" — add e.g. description: "..."`, + ); }); it("should ignore unknown frontmatter fields", () => { @@ -125,7 +128,10 @@ describe("skills", () => { // no-frontmatter has no description, so it should be skipped expect(skills).toHaveLength(0); - expect(diagnostics.some((d: ResourceDiagnostic) => d.message.includes("description is required"))).toBe(true); + const messages = diagnostics.map((d: ResourceDiagnostic) => d.message); + expect(messages).toContain( + `${join(fixturesDir, "no-frontmatter", "SKILL.md")}: frontmatter is missing "description" — add e.g. description: "..."`, + ); }); it("should warn and skip skill when YAML frontmatter is invalid", () => { @@ -135,7 +141,38 @@ describe("skills", () => { }); expect(skills).toHaveLength(0); - expect(diagnostics.some((d: ResourceDiagnostic) => d.message.includes("at line"))).toBe(true); + const messages = diagnostics.map((d: ResourceDiagnostic) => d.message); + expect(messages).toHaveLength(1); + expect(messages[0]).toBe( + `${join(fixturesDir, "invalid-yaml", "SKILL.md")}: invalid YAML frontmatter at line 3, column 31: ` + + "Flow sequence in block collection must be sufficiently indented and end with a ]", + ); + }); + + it("should hint at the unquoted colon in a description", () => { + const { skills, diagnostics } = loadSkillsFromDir({ + dir: join(fixturesDir, "unquoted-colon"), + source: "test", + }); + + expect(skills).toHaveLength(0); + const messages = diagnostics.map((d: ResourceDiagnostic) => d.message); + expect(messages).toEqual([ + `${join(fixturesDir, "unquoted-colon", "SKILL.md")}: invalid YAML frontmatter at line 3, column 14: ` + + 'Nested mappings are not allowed in compact mappings — description contains an unquoted ":" — ' + + "wrap the value in double quotes", + ]); + }); + + it("should still load a description whose colon is quoted", () => { + const { skills, diagnostics } = loadSkillsFromDir({ + dir: join(fixturesDir, "quoted-colon"), + source: "test", + }); + + expect(diagnostics).toHaveLength(0); + expect(skills).toHaveLength(1); + expect(skills[0].description).toBe("Enforce a claim-to-proof workflow: define the evidence, then require it"); }); it("should preserve multiline descriptions from YAML", () => { @@ -167,8 +204,9 @@ describe("skills", () => { }); // Should load all skills that have descriptions (even with warnings) - // valid-skill, name-mismatch, invalid-name-chars, long-name, unknown-field, nested/child-skill, consecutive-hyphens - // NOT: missing-description, no-frontmatter (both missing descriptions) + // valid-skill, name-mismatch, invalid-name-chars, long-name, unknown-field, nested/child-skill, + // consecutive-hyphens, quoted-colon + // NOT: missing-description, no-frontmatter (both missing descriptions), unquoted-colon (invalid YAML) expect(skills.length).toBeGreaterThanOrEqual(6); });