From 32af6e66183e3a997f843fa7f5907c111da49196 Mon Sep 17 00:00:00 2001 From: Hylouis233 Date: Sat, 10 Oct 2026 21:46:27 +0800 Subject: [PATCH 1/2] fix(skills): emit readable single-line diagnostics for SKILL.md frontmatter errors --- apps/cli/src/ui/interactive-mode.ts | 15 ++- packages/agent-core/src/harness/skills.ts | 127 ++++++++++++++++-- .../agent-core/test/harness/skills.test.ts | 42 +++++- packages/coding-agent/src/core/skills.ts | 18 ++- .../coding-agent/src/utils/frontmatter.ts | 96 +++++++++++++ .../fixtures/skills/quoted-colon/SKILL.md | 8 ++ .../fixtures/skills/unquoted-colon/SKILL.md | 8 ++ .../coding-agent/test/frontmatter.test.ts | 89 +++++++++++- packages/coding-agent/test/skills.test.ts | 48 ++++++- 9 files changed, 423 insertions(+), 28 deletions(-) create mode 100644 packages/coding-agent/test/fixtures/skills/quoted-colon/SKILL.md create mode 100644 packages/coding-agent/test/fixtures/skills/unquoted-colon/SKILL.md 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/packages/agent-core/src/harness/skills.ts b/packages/agent-core/src/harness/skills.ts index 3cb89857..35b6dc97 100644 --- a/packages/agent-core/src/harness/skills.ts +++ b/packages/agent-core/src/harness/skills.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,131 @@ 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"); + 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, + }; +} + +/** A plain scalar cannot contain ": ", so its presence means the value needs quoting. */ +const hasUnquotedColonSeparator = (value: string): boolean => /:(\s|$)/.test(value); + +/** Drop a trailing `#` comment; `#` only starts a comment after whitespace. */ +const stripTrailingComment = (value: string): string => value.replace(/\s+#.*$/, ""); + +/** + * Detect the classic frontmatter mistake of an unquoted `": "` inside the description scalar, + * which `yaml` rejects as a nested mapping. + */ +function hasUnquotedColonInDescription(content: string): boolean { + const { yamlString } = extractFrontmatterBlock(content); + if (yamlString === null) return false; + + const lines = yamlString.split("\n"); + for (let index = 0; index < lines.length; index++) { + const key = /^([ \t]*)description[ \t]*:[ \t]*(.*)$/.exec(lines[index]); + if (!key) continue; + + const value = stripTrailingComment(key[2]); + // Quoted, flow and block scalars may legally contain ": ". + if (value === "" || /^[|>&*"'[\]{}]/.test(value)) continue; + if (hasUnquotedColonSeparator(value)) return true; + + // A plain scalar may continue on more deeply indented lines. + const indent = key[1].length; + for (let next = index + 1; next < lines.length; next++) { + const line = lines[next]; + if (line.trim() === "") continue; + const lineIndent = /^[ \t]*/.exec(line)?.[0].length ?? 0; + if (lineIndent <= indent) break; + if (hasUnquotedColonSeparator(stripTrailingComment(line))) return true; + } + } + + return false; +} + +/** + * 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..ab934ce1 100644 --- a/packages/agent-core/test/harness/skills.test.ts +++ b/packages/agent-core/test/harness/skills.test.ts @@ -94,13 +94,53 @@ 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("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..fb1b4803 100644 --- a/packages/coding-agent/src/utils/frontmatter.ts +++ b/packages/coding-agent/src/utils/frontmatter.ts @@ -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,89 @@ 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"); + 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, + }; +} + +/** A plain scalar cannot contain ": ", so its presence means the value needs quoting. */ +const hasUnquotedColonSeparator = (value: string): boolean => /:(\s|$)/.test(value); + +/** Drop a trailing `#` comment; `#` only starts a comment after whitespace. */ +const stripTrailingComment = (value: string): string => value.replace(/\s+#.*$/, ""); + +/** + * Detect the classic frontmatter mistake of an unquoted `": "` inside the description scalar, + * which `yaml` rejects as a nested mapping. + */ +export function hasUnquotedColonInDescription(content: string): boolean { + const { yamlString } = extractFrontmatter(content); + if (!yamlString) return false; + + const lines = yamlString.split("\n"); + for (let index = 0; index < lines.length; index++) { + const key = /^([ \t]*)description[ \t]*:[ \t]*(.*)$/.exec(lines[index]); + if (!key) continue; + + const value = stripTrailingComment(key[2]); + // Quoted, flow and block scalars may legally contain ": ". + if (value === "" || /^[|>&*"'[\]{}]/.test(value)) continue; + if (hasUnquotedColonSeparator(value)) return true; + + // A plain scalar may continue on more deeply indented lines. + const indent = key[1].length; + for (let next = index + 1; next < lines.length; next++) { + const line = lines[next]; + if (line.trim() === "") continue; + const lineIndent = /^[ \t]*/.exec(line)?.[0].length ?? 0; + if (lineIndent <= indent) break; + if (hasUnquotedColonSeparator(stripTrailingComment(line))) return true; + } + } + + return false; +} + +/** + * 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..9f3c2792 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,87 @@ 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', + ); + }); + + 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); }); From ede9279b1b2fab4037c533e3f8d7837338fbd90b Mon Sep 17 00:00:00 2001 From: Hong Liu <88263959+Hylouis233@users.noreply.github.com> Date: Sun, 11 Oct 2026 20:19:48 -0700 Subject: [PATCH 2/2] fix(skills): make frontmatter diagnostic hints syntax-aware Inspect recovered YAML structure to avoid quoting hints for comments and block scalars. Escape parser control characters and tolerate absent error values. Cover both skill loaders and CLI diagnostic rendering. --- .../test/interactive-mode-diagnostics.test.ts | 42 +++++++++++++ packages/agent-core/src/harness/skills.ts | 59 +++++++------------ .../agent-core/test/harness/skills.test.ts | 25 ++++++++ .../coding-agent/src/utils/frontmatter.ts | 55 +++++++---------- .../coding-agent/test/frontmatter.test.ts | 28 +++++++++ 5 files changed, 137 insertions(+), 72 deletions(-) create mode 100644 apps/cli/test/interactive-mode-diagnostics.test.ts 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 35b6dc97..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"; @@ -347,9 +347,12 @@ const YAML_TO_FILE_LINE_OFFSET = 1; */ function describeFrontmatterError(error: unknown): { line?: number; column?: number; reason: string } { const message = error instanceof Error ? error.message : String(error); - const [headline] = message.split("\n"); + 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 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"; @@ -361,42 +364,24 @@ function describeFrontmatterError(error: unknown): { line?: number; column?: num }; } -/** A plain scalar cannot contain ": ", so its presence means the value needs quoting. */ -const hasUnquotedColonSeparator = (value: string): boolean => /:(\s|$)/.test(value); - -/** Drop a trailing `#` comment; `#` only starts a comment after whitespace. */ -const stripTrailingComment = (value: string): string => value.replace(/\s+#.*$/, ""); - -/** - * Detect the classic frontmatter mistake of an unquoted `": "` inside the description scalar, - * which `yaml` rejects as a nested mapping. - */ +/** Detect a rejected compact mapping in the top-level description value. */ function hasUnquotedColonInDescription(content: string): boolean { const { yamlString } = extractFrontmatterBlock(content); - if (yamlString === null) return false; - - const lines = yamlString.split("\n"); - for (let index = 0; index < lines.length; index++) { - const key = /^([ \t]*)description[ \t]*:[ \t]*(.*)$/.exec(lines[index]); - if (!key) continue; - - const value = stripTrailingComment(key[2]); - // Quoted, flow and block scalars may legally contain ": ". - if (value === "" || /^[|>&*"'[\]{}]/.test(value)) continue; - if (hasUnquotedColonSeparator(value)) return true; - - // A plain scalar may continue on more deeply indented lines. - const indent = key[1].length; - for (let next = index + 1; next < lines.length; next++) { - const line = lines[next]; - if (line.trim() === "") continue; - const lineIndent = /^[ \t]*/.exec(line)?.[0].length ?? 0; - if (lineIndent <= indent) break; - if (hasUnquotedColonSeparator(stripTrailingComment(line))) return true; - } - } - - return false; + 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); + }); } /** diff --git a/packages/agent-core/test/harness/skills.test.ts b/packages/agent-core/test/harness/skills.test.ts index ab934ce1..412a7970 100644 --- a/packages/agent-core/test/harness/skills.test.ts +++ b/packages/agent-core/test/harness/skills.test.ts @@ -141,6 +141,31 @@ Use this skill. 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/utils/frontmatter.ts b/packages/coding-agent/src/utils/frontmatter.ts index fb1b4803..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> = { @@ -63,9 +63,12 @@ const YAML_TO_FILE_LINE_OFFSET = 1; */ export function describeFrontmatterError(error: unknown): FrontmatterErrorDetails { const message = error instanceof Error ? error.message : String(error); - const [headline] = message.split("\n"); + 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 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"; @@ -77,42 +80,24 @@ export function describeFrontmatterError(error: unknown): FrontmatterErrorDetail }; } -/** A plain scalar cannot contain ": ", so its presence means the value needs quoting. */ -const hasUnquotedColonSeparator = (value: string): boolean => /:(\s|$)/.test(value); - -/** Drop a trailing `#` comment; `#` only starts a comment after whitespace. */ -const stripTrailingComment = (value: string): string => value.replace(/\s+#.*$/, ""); - -/** - * Detect the classic frontmatter mistake of an unquoted `": "` inside the description scalar, - * which `yaml` rejects as a nested mapping. - */ +/** 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; - const lines = yamlString.split("\n"); - for (let index = 0; index < lines.length; index++) { - const key = /^([ \t]*)description[ \t]*:[ \t]*(.*)$/.exec(lines[index]); - if (!key) continue; - - const value = stripTrailingComment(key[2]); - // Quoted, flow and block scalars may legally contain ": ". - if (value === "" || /^[|>&*"'[\]{}]/.test(value)) continue; - if (hasUnquotedColonSeparator(value)) return true; - - // A plain scalar may continue on more deeply indented lines. - const indent = key[1].length; - for (let next = index + 1; next < lines.length; next++) { - const line = lines[next]; - if (line.trim() === "") continue; - const lineIndent = /^[ \t]*/.exec(line)?.[0].length ?? 0; - if (lineIndent <= indent) break; - if (hasUnquotedColonSeparator(stripTrailingComment(line))) return true; - } - } - - 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); + }); } /** diff --git a/packages/coding-agent/test/frontmatter.test.ts b/packages/coding-agent/test/frontmatter.test.ts index 9f3c2792..4512b133 100644 --- a/packages/coding-agent/test/frontmatter.test.ts +++ b/packages/coding-agent/test/frontmatter.test.ts @@ -108,6 +108,34 @@ describe("frontmatter error diagnostics", () => { ); }); + 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);