diff --git a/cli/commands/skills/validate.test.ts b/cli/commands/skills/validate.test.ts index 68adcc4f6c..f49acd2933 100644 --- a/cli/commands/skills/validate.test.ts +++ b/cli/commands/skills/validate.test.ts @@ -7,9 +7,12 @@ import { validateSkillDirectory } from "./validate.ts"; async function withTempSkill( files: Record, fn: (dir: string) => Promise, + skillDirName = "test-skill", ): Promise { - const dir = await Deno.makeTempDir({ prefix: "vf-skill-validate-" }); + const rootDir = await Deno.makeTempDir({ prefix: "vf-skill-validate-" }); + const dir = join(rootDir, skillDirName); try { + await Deno.mkdir(dir, { recursive: true }); for (const [path, content] of Object.entries(files)) { const target = join(dir, path); await Deno.mkdir(join(target, ".."), { recursive: true }); @@ -17,7 +20,7 @@ async function withTempSkill( } await fn(dir); } finally { - await Deno.remove(dir, { recursive: true }); + await Deno.remove(rootDir, { recursive: true }); } } @@ -37,7 +40,7 @@ Review the submitted changes. }, async (dir) => { const issues = await validateSkillDirectory(dir); assertEquals(issues, []); - }); + }, "code-review"); }); it("reports a missing SKILL.md", async () => { @@ -60,8 +63,26 @@ description: Invalid name. const issues = await validateSkillDirectory(dir); assertEquals(issues.length, 1); assertEquals(issues[0]?.severity, "error"); - assertEquals(issues[0]?.message.includes("Invalid skill name"), true); - }); + assertEquals(issues[0]?.message.includes('Invalid skill name "BadName"'), true); + }, "bad-name"); + }); + + it("reports SKILL.md frontmatter name mismatch with directory", async () => { + await withTempSkill({ + "SKILL.md": `--- +name: email +description: Mismatched name. +--- + +# Email +`, + }, async (dir) => { + const issues = await validateSkillDirectory(dir); + assertEquals(issues, [{ + severity: "error", + message: 'Skill name "email" does not match directory name "process-email"', + }]); + }, "process-email"); }); it("warns when SKILL.md has no instruction body", async () => { @@ -74,6 +95,6 @@ description: Empty instruction body. }, async (dir) => { const issues = await validateSkillDirectory(dir); assertEquals(issues, [{ severity: "warning", message: "SKILL.md body is empty" }]); - }); + }, "empty-body"); }); }); diff --git a/cli/commands/skills/validate.ts b/cli/commands/skills/validate.ts index 8491345fc3..ed937fbcd2 100644 --- a/cli/commands/skills/validate.ts +++ b/cli/commands/skills/validate.ts @@ -9,7 +9,7 @@ import { createSuccessEnvelope, isJsonMode, outputJson } from "../../shared/json import { exitProcess, logError, logSuccess, logWarning } from "#cli/utils"; import { createFileSystem } from "veryfront/platform"; import { basename } from "#std/path.ts"; -import { parseSkillFrontmatter, validateSkillMetadata } from "veryfront/skill"; +import { parseSkillFrontmatter, SKILL_NAME_REGEX, validateSkillMetadata } from "veryfront/skill"; interface ValidationIssue { severity: "error" | "warning"; @@ -36,7 +36,9 @@ export async function validateSkillDirectory(dir: string): Promise, + directoryName: string, +): void { + if (typeof frontmatter.name !== "string") return; + + const name = frontmatter.name.trim(); + if (!SKILL_NAME_REGEX.test(name)) { + throw new Error( + `Invalid skill name "${name}": must be lowercase alphanumeric with hyphens, 1-64 characters`, + ); + } + + if (name !== directoryName) { + throw new Error(`Skill name "${name}" does not match directory name "${directoryName}"`); + } +} + async function outputResults( dir: string, issues: ValidationIssue[], diff --git a/scripts/lint/test-typecheck-baseline.json b/scripts/lint/test-typecheck-baseline.json index c02df5a2ba..616ee192d8 100644 --- a/scripts/lint/test-typecheck-baseline.json +++ b/scripts/lint/test-typecheck-baseline.json @@ -32,7 +32,6 @@ "src/cache/registry.test.ts", "src/chat/upload-handler.test.ts", "src/config/env.test.ts", - "src/discovery/agent-scoped-capabilities.test.ts", "src/embedding/chunk.test.ts", "src/embedding/rag-store.test.ts", "src/errors/middleware/wrap-unknown.test.ts", diff --git a/src/agent/runtime/project-skill-catalog.test.ts b/src/agent/runtime/project-skill-catalog.test.ts index 946fb086e4..92826b1fb2 100644 --- a/src/agent/runtime/project-skill-catalog.test.ts +++ b/src/agent/runtime/project-skill-catalog.test.ts @@ -241,6 +241,36 @@ Deno.test("catalog includes colocated skills with owner metadata and source path assertEquals(own?.sourcePath, "agents/researcher/SKILL.md"); }); +Deno.test("catalog accepts provider-safe colocated skill ids for dotted agent ids", async () => { + const { catalog } = createSkillCatalog({ + paths: [ + "agents/a.b/AGENT.md", + "agents/a.b/skills/x_y/SKILL.md", + "agents/a.b/skills/x_y/references/styles.md", + ], + contentsByPath: { + "agents/a.b/skills/x_y/SKILL.md": `--- +name: X Y +description: Owned underscore helper +metadata: + display_name: X Y +--- +Use X Y. +`, + }, + }); + + const skills = await catalog(); + assertEquals(skills.map((skill) => skill.id), ["a_b--x_y"]); + const nested = skills[0]; + assertEquals(nested?.name, "a_b--x_y"); + assertEquals(nested?.displayName, "X Y"); + assertEquals(nested?.ownerAgentId, "a.b"); + assertEquals(nested?.shortName, "x_y"); + assertEquals(nested?.sourcePath, "agents/a.b/skills/x_y/SKILL.md"); + assertEquals(nested?.references, ["references/styles.md"]); +}); + Deno.test("catalog keeps global skills unowned and carries their source paths", async () => { const { catalog } = createSkillCatalog({ paths: ["skills/gmail/SKILL.md"], diff --git a/src/agent/runtime/skill-metadata.test.ts b/src/agent/runtime/skill-metadata.test.ts index 8642181571..c0a54f7b70 100644 --- a/src/agent/runtime/skill-metadata.test.ts +++ b/src/agent/runtime/skill-metadata.test.ts @@ -12,11 +12,15 @@ Deno.test("parseRuntimeSkillMetadata parses valid frontmatter", () => { const content = `--- name: My Skill description: A useful skill +metadata: + display_name: My Display Skill + tier: project --- Body content here`; const metadata = parseRuntimeSkillMetadata(content); assertExists(metadata); assertEquals(metadata.name, "My Skill"); + assertEquals(metadata.metadata, { display_name: "My Display Skill", tier: "project" }); assertEquals(metadata.description, "A useful skill"); }); @@ -33,10 +37,12 @@ Deno.test("parseRuntimeSkillMetadata returns empty metadata for empty content", assertEquals(metadata.name, undefined); }); -Deno.test("buildRuntimeSkillDefinition builds a skill definition from valid content", () => { +Deno.test("buildRuntimeSkillDefinition builds a canonical skill definition from valid content", () => { const content = `--- -name: Code Review +name: code-review description: Reviews code quality +metadata: + display_name: Code Review --- # Code Review Skill Review the code for quality issues.`; @@ -44,11 +50,66 @@ Review the code for quality issues.`; const skill = buildRuntimeSkillDefinition({ id: "code-review", content }); assertExists(skill); assertEquals(skill.id, "code-review"); - assertEquals(skill.name, "Code Review"); + assertEquals(skill.name, "code-review"); + assertEquals(skill.displayName, "Code Review"); assertEquals(skill.description, "Reviews code quality"); + assertEquals(skill.metadata, { display_name: "Code Review" }); assertEquals(skill.instructions, content); }); +Deno.test("buildRuntimeSkillDefinition recovers a legacy display-style frontmatter name", () => { + const content = `--- +name: Process Email +description: Process email +--- +Body`; + const skill = buildRuntimeSkillDefinition({ id: "process-email", content }); + assertExists(skill); + assertEquals(skill.id, "process-email"); + assertEquals(skill.name, "process-email"); + assertEquals(skill.displayName, "Process Email"); +}); + +Deno.test("buildRuntimeSkillDefinition rejects invalid canonical ids", () => { + const errors: Array | undefined> = []; + const skill = buildRuntimeSkillDefinition({ + id: "Process Email", + content: `--- +description: Process email +--- +Body`, + logger: { + error: (_message, metadata) => errors.push(metadata), + }, + }); + + assertEquals(skill, null); + assertEquals(errors[0]?.id, "Process Email"); +}); + +Deno.test("buildRuntimeSkillDefinition accepts provider-safe owned namespaced ids", () => { + const content = `--- +name: x_y +description: Owned helper +metadata: + display_name: Owned Helper +--- +Body`; + const skill = buildRuntimeSkillDefinition({ + id: "a_b--x_y", + content, + ownerAgentId: "a.b", + shortName: "x_y", + }); + + assertExists(skill); + assertEquals(skill.id, "a_b--x_y"); + assertEquals(skill.name, "a_b--x_y"); + assertEquals(skill.displayName, "Owned Helper"); + assertEquals(skill.ownerAgentId, "a.b"); + assertEquals(skill.shortName, "x_y"); +}); + Deno.test("buildRuntimeSkillDefinition uses id as fallback name", () => { const content = `--- description: A skill @@ -65,6 +126,8 @@ name: Test # This is the heading Some body text`; const skill = buildRuntimeSkillDefinition({ id: "test", content }); + assertEquals(skill?.name, "test"); + assertEquals(skill?.displayName, "Test"); assertEquals(skill?.description, "This is the heading"); }); diff --git a/src/agent/runtime/skill-metadata.ts b/src/agent/runtime/skill-metadata.ts index 04985302e3..9c6b571249 100644 --- a/src/agent/runtime/skill-metadata.ts +++ b/src/agent/runtime/skill-metadata.ts @@ -1,5 +1,6 @@ import { extract } from "#std/front-matter/yaml.ts"; import { defineSchema, lazySchema } from "#veryfront/schemas/index.ts"; +import { SKILL_NAME_REGEX, SKILL_PROVIDER_SAFE_ID_REGEX } from "#veryfront/skill/types.ts"; function normalizeAllowedTools(value: string | string[] | undefined): string[] { if (value === undefined) { @@ -22,6 +23,7 @@ export interface RuntimeSkillFrontmatter { name: string | undefined; description: string | undefined; allowedTools: string[]; + metadata: Record | undefined; model: string | undefined; thinking: false | number | undefined; maxSteps: number | undefined; @@ -35,19 +37,22 @@ export const getRuntimeSkillFrontmatterSchema = defineSchema((v) => "allowed-tools": v.union([v.string(), v.array(v.string())]).optional(), allowed_tools: v.union([v.string(), v.array(v.string())]).optional(), model: v.string().optional(), + metadata: v.record(v.string(), v.unknown()).optional(), thinking: v.union([v.literal(false), v.coerce.number().int().positive()]).optional(), "max-steps": v.coerce.number().int().positive().optional(), }) .passthrough() .transform((data): RuntimeSkillFrontmatter => { const d = data as Record; + const metadata = normalizeMetadata(d.metadata); return { - name: (typeof d.name === "string" ? d.name.trim() : undefined) || undefined, + name: normalizeOptionalString(d.name), description: (typeof d.description === "string" ? d.description.trim() : undefined) || undefined, allowedTools: normalizeAllowedTools( (d["allowed-tools"] ?? d.allowed_tools) as string | string[] | undefined, ), + metadata, model: (typeof d.model === "string" ? d.model.trim() : undefined) || undefined, thinking: d.thinking as false | number | undefined, maxSteps: d["max-steps"] as number | undefined, @@ -55,6 +60,27 @@ export const getRuntimeSkillFrontmatterSchema = defineSchema((v) => }) ); +function normalizeOptionalString(value: unknown): string | undefined { + return (typeof value === "string" ? value.trim() : undefined) || undefined; +} + +function normalizeMetadata(value: unknown): Record | undefined { + if (!value || typeof value !== "object" || Array.isArray(value)) { + return undefined; + } + + const entries = Object.entries(value as Record); + if (entries.length === 0) { + return undefined; + } + + const metadata: Record = {}; + for (const [key, rawValue] of entries) { + metadata[key] = String(rawValue); + } + return metadata; +} + /** Schema for runtime skill frontmatter. * @deprecated Use getRuntimeSkillFrontmatterSchema() */ @@ -64,9 +90,11 @@ export const RuntimeSkillFrontmatterSchema = lazySchema(getRuntimeSkillFrontmatt export type RuntimeSkillDefinition = { id: string; name: string; + displayName?: string; description: string; instructions: string; allowedTools: string[]; + metadata?: Record; model?: string; thinking?: false | number; maxSteps?: number; @@ -261,19 +289,37 @@ export function buildRuntimeSkillDefinition(input: { sourcePath?: string; logger?: RuntimeSkillMetadataLogger; }): RuntimeSkillDefinition | null { + if (!isRuntimeSkillIdValid(input)) { + input.logger?.error?.("Invalid skill id; skipping skill", { + id: input.id, + error: input.ownerAgentId === undefined && input.shortName === undefined + ? "must be lowercase alphanumeric with hyphens, 1-64 characters" + : "must be provider-safe letters, numbers, underscores, or hyphens, 1-64 characters", + }); + return null; + } + const document = parseRuntimeSkillDocument(input.content, { logger: input.logger }); if (!document) { return null; } const { metadata, body } = document; + const canonicalName = input.id; + const explicitDisplayName = metadata.metadata?.display_name?.trim() || undefined; + const legacyDisplayName = metadata.name && metadata.name !== canonicalName + ? metadata.name + : undefined; + const displayName = explicitDisplayName ?? legacyDisplayName; return { id: input.id, - name: metadata.name ?? input.id, + name: canonicalName, + ...(displayName ? { displayName } : {}), description: metadata.description ?? extractDescriptionFromMarkdown(body, input.id), instructions: input.content, allowedTools: metadata.allowedTools, + ...(metadata.metadata ? { metadata: metadata.metadata } : {}), ...(metadata.model ? { model: metadata.model } : {}), ...(metadata.thinking !== undefined ? { thinking: metadata.thinking } : {}), ...(metadata.maxSteps !== undefined ? { maxSteps: metadata.maxSteps } : {}), @@ -286,6 +332,18 @@ export function buildRuntimeSkillDefinition(input: { }; } +function isRuntimeSkillIdValid(input: { + id: string; + ownerAgentId?: string; + shortName?: string; +}): boolean { + if (input.ownerAgentId !== undefined || input.shortName !== undefined) { + return SKILL_PROVIDER_SAFE_ID_REGEX.test(input.id); + } + + return SKILL_NAME_REGEX.test(input.id); +} + /** Normalizes runtime skill reference path. */ export function normalizeRuntimeSkillReferencePath(path: string): string | null { const normalized = path.trim().replaceAll("\\", "/"); diff --git a/src/agent/runtime/skill-prompt.test.ts b/src/agent/runtime/skill-prompt.test.ts index 497653bcdb..95e3bc2203 100644 --- a/src/agent/runtime/skill-prompt.test.ts +++ b/src/agent/runtime/skill-prompt.test.ts @@ -48,7 +48,8 @@ Deno.test("buildRuntimeAvailableSkillsPromptBlock renders skills and delegation const block = buildRuntimeAvailableSkillsPromptBlock([ createSkill({ id: "build-ui", - name: "Build UI guidance", + name: "build-ui", + displayName: "Build UI guidance", description: "Build UI", allowedTools: ["bash", "writeFile"], }), @@ -113,6 +114,20 @@ Deno.test("buildRuntimeAvailableSkillsPromptBlock does not repeat an id-only nam assertEquals(block.includes("code-review (`code-review`)"), false); }); +Deno.test("buildRuntimeAvailableSkillsPromptBlock keeps canonical name out of display labels", () => { + const block = buildRuntimeAvailableSkillsPromptBlock([ + createSkill({ + id: "process-email", + name: "process-email", + displayName: "Process Email", + description: "Process email", + }), + ]); + + assertStringIncludes(block, "- Process Email (`process-email`): Process email"); + assertEquals(block.includes("- process-email (`process-email`)"), false); +}); + Deno.test("buildRuntimeAvailableSkillsPromptBlock truncates long skill lists", () => { const skills = Array.from( { length: MAX_RUNTIME_SKILL_PROMPT_ENTRIES + 2 }, diff --git a/src/agent/runtime/skill-prompt.ts b/src/agent/runtime/skill-prompt.ts index b101168e28..cb5c269f09 100644 --- a/src/agent/runtime/skill-prompt.ts +++ b/src/agent/runtime/skill-prompt.ts @@ -62,7 +62,7 @@ export function formatRuntimeSkillMetadata(skill: RuntimeSkillDefinition): strin } function formatRuntimeSkillLabel(skill: RuntimeSkillDefinition): string { - return skill.name === skill.id ? skill.id : `${skill.name} (\`${skill.id}\`)`; + return skill.displayName ? `${skill.displayName} (\`${skill.id}\`)` : skill.id; } /** Builds runtime available skills prompt block. */ diff --git a/src/discovery/agent-scoped-capabilities.test.ts b/src/discovery/agent-scoped-capabilities.test.ts index 4ada1969d3..60fde51c10 100644 --- a/src/discovery/agent-scoped-capabilities.test.ts +++ b/src/discovery/agent-scoped-capabilities.test.ts @@ -25,6 +25,9 @@ function emptyResult(): DiscoveryResult { prompts: new Map(), workflows: new Map(), tasks: new Map(), + schedules: new Map(), + webhooks: new Map(), + evals: new Map(), errors: [], }; } @@ -101,6 +104,38 @@ Deno.test("directory and flat agents discover side by side with owned skills reg } }); +Deno.test("directory agents with dotted ids register provider-safe owned skill ids", async () => { + const root = await Deno.makeTempDir(); + skillRegistry.clearAll(); + try { + const agentsDir = `${root}/agents`; + await Deno.mkdir(`${agentsDir}/a.b/skills/x_y`, { recursive: true }); + await Deno.writeTextFile( + `${agentsDir}/a.b/AGENT.md`, + `---\nname: Dotted Agent\nskills: [x_y]\n---\nHandle dotted ids.\n`, + ); + await Deno.writeTextFile( + `${agentsDir}/a.b/skills/x_y/SKILL.md`, + `---\nname: X Y\ndescription: Owned underscore helper\nmetadata:\n display_name: X Y\n---\nUse X Y.\n`, + ); + + const result = emptyResult(); + await discoverRuntimeAgentMarkdownDefinitions(agentsDir, result, context); + + assertEquals(result.errors, []); + const skill = skillRegistry.get("a_b--x_y"); + assertEquals(skill?.id, "a_b--x_y"); + assertEquals(skill?.metadata.name, "a_b--x_y"); + assertEquals(skill?.metadata.displayName, "X Y"); + assertEquals(skill?.ownerAgentId, "a.b"); + assertEquals(skill?.shortName, "x_y"); + } finally { + skillRegistry.clearAll(); + cleanupAgents(["a.b"]); + await Deno.remove(root, { recursive: true }); + } +}); + Deno.test("a coordinator's skills: true does not include its delegate's owned skills", async () => { const root = await Deno.makeTempDir(); skillRegistry.clearAll(); diff --git a/src/discovery/agent-scoped-capabilities.ts b/src/discovery/agent-scoped-capabilities.ts index 73d9bb7bf2..706521576f 100644 --- a/src/discovery/agent-scoped-capabilities.ts +++ b/src/discovery/agent-scoped-capabilities.ts @@ -215,7 +215,9 @@ async function buildSkillFromDir(input: { const content = await readDiscoveryTextFile(skillMdPath, input.context); const parsed = await parseSkillFrontmatter(content); - const metadata = validateSkillMetadata(parsed.frontmatter, input.id); + const metadata = validateSkillMetadata(parsed.frontmatter, input.id, { + providerSafeName: true, + }); return { id: input.id, diff --git a/src/discovery/handlers/skill-handler.ts b/src/discovery/handlers/skill-handler.ts index 42ec752de9..ef5ce65cd8 100644 --- a/src/discovery/handlers/skill-handler.ts +++ b/src/discovery/handlers/skill-handler.ts @@ -84,13 +84,9 @@ export async function discoverSkills( // Validate metadata (directory name as fallback for skill name) const metadata = validateSkillMetadata(parsed.frontmatter, entry.name); - // Warn if metadata name differs from directory name, use directory name as ID + // The directory is the canonical identity; legacy/display-style names + // are preserved only as display metadata. const skillId = entry.name; - if (metadata.name !== entry.name) { - logger.warn( - `Skill "${metadata.name}" in directory "${entry.name}" — using directory name as ID`, - ); - } // Check for duplicate IDs (first wins) if (skills.has(skillId)) { diff --git a/src/discovery/skill-discovery.test.ts b/src/discovery/skill-discovery.test.ts index 9924928bee..cde7fc647a 100644 --- a/src/discovery/skill-discovery.test.ts +++ b/src/discovery/skill-discovery.test.ts @@ -52,4 +52,45 @@ Other instructions.`, assertEquals(result.skills.has("other"), true); }); + + it("discovers legacy display-style names by canonical directory id", async () => { + const files = { + "/project/skills/process-email/SKILL.md": `--- +name: Process Email +description: Processes support emails. +metadata: + display_name: Support Email Processor + team: support +--- +Use this skill for support email workflows.`, + }; + + const result = await discoverAll({ + baseDir: "/project", + toolDirs: [], + agentDirs: [], + resourceDirs: [], + promptDirs: [], + workflowDirs: [], + taskDirs: [], + skillDirs: ["skills"], + fsAdapter: createSkillTestAdapter(files), + verbose: false, + }); + + const skill = result.skills.get("process-email"); + assertExists(skill); + assertEquals(skill.id, "process-email"); + assertEquals(skill.metadata.name, "process-email"); + assertEquals(skill.metadata.displayName, "Support Email Processor"); + assertEquals(skill.metadata.metadata, { + display_name: "Support Email Processor", + team: "support", + }); + + const registrySkill = skillRegistry.get("process-email"); + assertExists(registrySkill); + assertEquals(registrySkill.metadata.displayName, "Support Email Processor"); + assertEquals(result.skills.has("Process Email"), false); + }); }); diff --git a/src/skill/parser.test.ts b/src/skill/parser.test.ts index bdcb32b13e..f64e15d6fe 100644 --- a/src/skill/parser.test.ts +++ b/src/skill/parser.test.ts @@ -56,6 +56,7 @@ Body text.`; "my-skill", ); assertEquals(result.name, "my-skill"); + assertEquals(result.displayName, undefined); assertEquals(result.description, "A skill"); }); @@ -76,11 +77,55 @@ Body text.`; } }); - it("should throw on invalid name (uppercase)", () => { + it("should preserve a legacy display-style frontmatter name as displayName", () => { + const result = validateSkillMetadata( + { name: "Process Email", description: "desc" }, + "process-email", + ); + assertEquals(result.name, "process-email"); + assertEquals(result.displayName, "Process Email"); + }); + + it("should preserve a mismatched frontmatter name as displayName", () => { + const result = validateSkillMetadata( + { name: "email", description: "desc" }, + "process-email", + ); + assertEquals(result.name, "process-email"); + assertEquals(result.displayName, "email"); + }); + + it("should prefer metadata.display_name over a legacy frontmatter display name", () => { + const result = validateSkillMetadata( + { + name: "Process Email", + description: "desc", + metadata: { display_name: "Email Processor", owner: "ops" }, + }, + "process-email", + ); + assertEquals(result.name, "process-email"); + assertEquals(result.displayName, "Email Processor"); + assertEquals(result.metadata, { display_name: "Email Processor", owner: "ops" }); + }); + + it("should throw on invalid directory/canonical name", () => { + try { + validateSkillMetadata( + { name: "process-email", description: "desc" }, + "Process Email", + ); + throw new Error("Should have thrown"); + } catch (e) { + assertEquals((e as Error).message.includes("Invalid skill name"), true); + } + }); + + it("should reject whitespace around the directory canonical name", () => { try { validateSkillMetadata( - { name: "MySkill", description: "desc" }, - "MySkill", + { description: "desc" }, + " process-email ", ); throw new Error("Should have thrown"); } catch (e) { diff --git a/src/skill/parser.ts b/src/skill/parser.ts index 8be66da7e2..a7be73a2a6 100644 --- a/src/skill/parser.ts +++ b/src/skill/parser.ts @@ -10,7 +10,12 @@ import { createError, toError } from "#veryfront/errors"; import { validateAllowedToolPatterns } from "./allowed-tools.ts"; -import { SKILL_DESCRIPTION_MAX_LENGTH, SKILL_NAME_REGEX, type SkillMetadata } from "./types.ts"; +import { + SKILL_DESCRIPTION_MAX_LENGTH, + SKILL_NAME_REGEX, + SKILL_PROVIDER_SAFE_ID_REGEX, + type SkillMetadata, +} from "./types.ts"; /** Result of parsing a SKILL.md file */ interface ParsedSkillContent { @@ -76,27 +81,32 @@ function parseFrontmatterFallback(content: string): ParsedSkillContent { export function validateSkillMetadata( frontmatter: Record, directoryName: string, + options: { providerSafeName?: boolean } = {}, ): SkillMetadata { - // Name: from frontmatter or directory name - const rawName = typeof frontmatter.name === "string" ? frontmatter.name.trim() : directoryName; + const canonicalName = directoryName; + const nameRegex = options.providerSafeName ? SKILL_PROVIDER_SAFE_ID_REGEX : SKILL_NAME_REGEX; + const nameExpectation = options.providerSafeName + ? "must be provider-safe letters, numbers, underscores, or hyphens, 1-64 characters" + : "must be lowercase alphanumeric with hyphens, 1-64 characters"; - if (!SKILL_NAME_REGEX.test(rawName)) { + if (!nameRegex.test(canonicalName)) { throw toError( createError({ type: "agent", - message: - `Invalid skill name "${rawName}": must be lowercase alphanumeric with hyphens, 1-64 characters`, + message: `Invalid skill name "${canonicalName}": ${nameExpectation}`, }), ); } + const rawName = typeof frontmatter.name === "string" ? frontmatter.name.trim() : undefined; + // Description: required const rawDescription = frontmatter.description; if (!rawDescription || typeof rawDescription !== "string" || !rawDescription.trim()) { throw toError( createError({ type: "agent", - message: `Skill "${rawName}" is missing a required "description" field`, + message: `Skill "${canonicalName}" is missing a required "description" field`, }), ); } @@ -105,7 +115,7 @@ export function validateSkillMetadata( // Allowed-tools: parse from space-delimited string or array const allowedToolPatterns = frontmatter["allowed-tools"] ?? frontmatter.allowed_tools; - const allowedTools = parseAllowedTools(allowedToolPatterns, rawName); + const allowedTools = parseAllowedTools(allowedToolPatterns, canonicalName); // License: optional string passthrough const license = typeof frontmatter.license === "string" ? frontmatter.license.trim() : undefined; @@ -117,9 +127,13 @@ export function validateSkillMetadata( // Metadata: convert nested object values to strings const metadata = parseMetadata(frontmatter.metadata); + const explicitDisplayName = getMetadataDisplayName(metadata); + const legacyDisplayName = rawName && rawName !== canonicalName ? rawName : undefined; + const displayName = explicitDisplayName ?? legacyDisplayName; return { - name: rawName, + name: canonicalName, + ...(displayName && { displayName }), description, ...(allowedTools && { allowedTools }), ...(license && { license }), @@ -128,6 +142,11 @@ export function validateSkillMetadata( }; } +function getMetadataDisplayName(metadata: Record | undefined): string | undefined { + const displayName = metadata?.display_name?.trim(); + return displayName ? displayName : undefined; +} + /** * Parse `allowed-tools` from frontmatter. * Accepts a space-delimited string or an array of strings. diff --git a/src/skill/prompt-augmentation.ts b/src/skill/prompt-augmentation.ts index af32f37156..27d006cffa 100644 --- a/src/skill/prompt-augmentation.ts +++ b/src/skill/prompt-augmentation.ts @@ -32,7 +32,8 @@ export function buildSkillManifestPrompt(skills: Map): string { let displayedSkillCount = 0; for (const [id, skill] of skills) { - lines.push(`- **${id}**: ${skill.metadata.description}`); + const label = skill.metadata.displayName ? `${skill.metadata.displayName} (\`${id}\`)` : id; + lines.push(`- **${label}**: ${skill.metadata.description}`); displayedSkillCount += 1; if (displayedSkillCount === MAX_SKILL_MANIFEST_PROMPT_ENTRIES) { break; diff --git a/src/skill/skill-owner-scope.test.ts b/src/skill/skill-owner-scope.test.ts index 47addbcfd9..05490e1aa8 100644 --- a/src/skill/skill-owner-scope.test.ts +++ b/src/skill/skill-owner-scope.test.ts @@ -238,3 +238,36 @@ Deno.test("load_skill loads the caller's own skill via its short name", async () await Deno.remove(tempDir, { recursive: true }); } }); + +Deno.test("load_skill resolves provider-safe owned short names before plain-id validation", async () => { + const tempDir = await Deno.makeTempDir(); + try { + await Deno.writeTextFile( + `${tempDir}/SKILL.md`, + `---\nname: X Y\ndescription: Owned helper\n---\n\nUse the owned helper.\n`, + ); + + skillRegistry.clearAll(); + registerSkill( + "a_b--x_y", + makeSkill({ + id: "a_b--x_y", + rootPath: tempDir, + ownerAgentId: "a.b", + shortName: "x_y", + }), + ); + + const loadSkill = createLoadSkillTool(); + const content = await loadSkill.execute( + { skillId: "x_y" }, + { agentId: "a.b" }, + ) as { instructions: string; skillId: string }; + + assertEquals(content.skillId, "a_b--x_y"); + assertEquals(content.instructions.trim(), "Use the owned helper."); + } finally { + skillRegistry.clearAll(); + await Deno.remove(tempDir, { recursive: true }); + } +}); diff --git a/src/skill/tools.ts b/src/skill/tools.ts index 659957f75e..bcd8a9414f 100644 --- a/src/skill/tools.ts +++ b/src/skill/tools.ts @@ -23,6 +23,8 @@ import type { Skill, SkillContent, SkillScriptExecutor } from "./types.ts"; import { SKILL_ASSETS_DIR, SKILL_MD_FILENAME, + SKILL_NAME_REGEX, + SKILL_PROVIDER_SAFE_ID_REGEX, SKILL_REFERENCES_DIR, SKILL_RESOURCES_DIR, SKILL_SCRIPTS_DIR, @@ -92,16 +94,35 @@ function resolveVisibleSkillOrThrow( ): Skill { const scope = { agentId: context?.agentId }; const skill = skillRegistry.resolveVisibleSkill(skillId, scope); - if (!skill) { - const visible = skillRegistry.getVisibleSkillIds(scope).join(", "); + if (skill) { + return skill; + } + + if (!isUnresolvedSkillSelectorValid(skillId)) { throw toError( createError({ type: "agent", - message: `Skill "${skillId}" not found. Available skills: ${visible || "none"}`, + message: + `Invalid skill id "${skillId}": must be lowercase alphanumeric with hyphens, 1-64 characters`, }), ); } - return skill; + + const visible = skillRegistry.getVisibleSkillIds(scope).join(", "); + throw toError( + createError({ + type: "agent", + message: `Skill "${skillId}" not found. Available skills: ${visible || "none"}`, + }), + ); +} + +function isUnresolvedSkillSelectorValid(skillId: string): boolean { + if (SKILL_NAME_REGEX.test(skillId)) { + return true; + } + + return skillId.includes("--") && SKILL_PROVIDER_SAFE_ID_REGEX.test(skillId); } function hasRuntimeSkillBoundary( diff --git a/src/skill/types.ts b/src/skill/types.ts index e20c138de5..498abb75b0 100644 --- a/src/skill/types.ts +++ b/src/skill/types.ts @@ -14,6 +14,9 @@ import type { FileSystemAdapter } from "#veryfront/platform/adapters/base.ts"; /** Valid skill name: lowercase alphanumeric + hyphens, 1-64 chars */ export const SKILL_NAME_REGEX = /^[a-z0-9][a-z0-9-]{0,63}$/; +/** Provider-safe owned skill id: sanitized namespace + short name, max 64 chars */ +export const SKILL_PROVIDER_SAFE_ID_REGEX = /^[A-Za-z0-9_-]{1,64}$/; + /** Valid allowed-tool pattern: exact ID or prefix wildcard (e.g. "api:*") */ export const SKILL_ALLOWED_TOOL_PATTERN_REGEX = /^[A-Za-z][A-Za-z0-9._-]*(:[A-Za-z][A-Za-z0-9._-]*)*(:\*)?$/; @@ -43,6 +46,8 @@ export const SKILL_ASSETS_DIR = "assets"; export interface SkillMetadata { /** Skill identifier (lowercase, hyphenated) */ name: string; + /** Optional human-readable label; never used for skill lookup/reference. */ + displayName?: string; /** Human-readable description */ description: string; /** Tool access restrictions (space-delimited in YAML, parsed to array) */ diff --git a/tests/e2e/features/skill-capabilities.test.ts b/tests/e2e/features/skill-capabilities.test.ts index 89cdf6f880..8ce3b67666 100644 --- a/tests/e2e/features/skill-capabilities.test.ts +++ b/tests/e2e/features/skill-capabilities.test.ts @@ -411,6 +411,137 @@ export const POST = createAgUiHandler("researcher"); }); }); + it("loads skills by canonical id while preserving explicit and legacy display names", async () => { + const projectDir = await createSkillProject("skill-display-names", { + "app/page.tsx": ` +export default function Home() { + return
Skill display metadata smoke test
; +} +`, + "agents/email-assistant.ts": ` +import { agent } from "veryfront/agent"; + +export default agent({ + id: "email-assistant", + system: "You route support emails.", + skills: ["process-email", "legacy-helper"], +}); +`, + "skills/process-email/SKILL.md": ` +--- +name: process-email +description: Processes inbound support emails. +metadata: + display_name: Process Email +--- +Use this skill to classify and draft support email replies. +`, + "skills/process-email/references/checklist.md": ` +Check sender, urgency, and requested action. +`, + "skills/legacy-helper/SKILL.md": ` +--- +name: Legacy Helper +description: Legacy display-style skill name. +--- +Use this legacy helper by canonical id. +`, + "app/api/chat/route.ts": ` +import { createAgUiHandler } from "veryfront/agent"; + +export const POST = createAgUiHandler("email-assistant"); +`, + }); + + await withServer(projectDir, async (server) => { + const { response: pageResponse, html } = await fetchPage(server, "/"); + expectPage(html, pageResponse) + .toRender() + .withElement("skill-display-page") + .withText("Skill display metadata smoke test") + .withoutErrors(); + + const { response: canonicalResponse, json: canonicalJson } = await postJson<{ + success: boolean; + toolId: string; + result: { + skillId: string; + instructions: string; + references?: string[]; + }; + }>(server, "/_dev/api/execute-tool", { + body: { + toolId: "load_skill", + args: { skillId: "process-email" }, + }, + }); + assertEquals(canonicalResponse.status, 200); + assertEquals(canonicalJson.success, true); + assertEquals(canonicalJson.toolId, "load_skill"); + assertEquals(canonicalJson.result.skillId, "process-email"); + assertStringIncludes(canonicalJson.result.instructions, "classify and draft"); + assertEquals(canonicalJson.result.references, ["references/checklist.md"]); + + const { response: referenceResponse, json: referenceJson } = await postJson<{ + success: boolean; + toolId: string; + result: { + path: string; + content: string; + }; + }>(server, "/_dev/api/execute-tool", { + body: { + toolId: "load_skill_reference", + args: { skillId: "process-email", reference: "references/checklist.md" }, + }, + }); + assertEquals(referenceResponse.status, 200); + assertEquals(referenceJson.success, true); + assertEquals(referenceJson.toolId, "load_skill_reference"); + assertEquals(referenceJson.result.path, "references/checklist.md"); + assertStringIncludes(referenceJson.result.content, "Check sender"); + + const { response: legacyResponse, json: legacyJson } = await postJson<{ + success: boolean; + toolId: string; + result: { + skillId: string; + instructions: string; + }; + }>(server, "/_dev/api/execute-tool", { + body: { + toolId: "load_skill", + args: { skillId: "legacy-helper" }, + }, + }); + assertEquals(legacyResponse.status, 200); + assertEquals(legacyJson.success, true); + assertEquals(legacyJson.result.skillId, "legacy-helper"); + assertStringIncludes(legacyJson.result.instructions, "legacy helper by canonical id"); + + const { response: displayNameResponse, json: displayNameJson } = await postJson<{ + error: string; + }>(server, "/_dev/api/execute-tool", { + body: { + toolId: "load_skill", + args: { skillId: "Process Email" }, + }, + }); + assertEquals(displayNameResponse.status, 500); + assertStringIncludes(displayNameJson.error, "Invalid skill id"); + + expectServer(server).withoutErrors(); + }, { + timeout: 60_000, + env: { + OPENAI_API_KEY: "", + ANTHROPIC_API_KEY: "", + GOOGLE_API_KEY: "", + VERYFRONT_DISABLE_LOCAL_AI: "1", + }, + }); + }); + it("runs the 3-minute AI chatbot pattern documented by the skill", async () => { const projectDir = await createSkillProject("chatbot", { "app/page.tsx": `