From 4719929f6a18ec897b6c0af00c51177df2efc9cb Mon Sep 17 00:00:00 2001 From: Mario Zechner Date: Sat, 24 Jan 2026 00:45:01 +0100 Subject: [PATCH] refactor(coding-agent): unify SkillWarning and ResourceDiagnostic - Create shared diagnostics.ts with ResourceCollision and ResourceDiagnostic types - Update loadSkills() to return diagnostics instead of warnings - Remove SkillWarning type and skillWarningsToDiagnostics() conversion - Update skills.test.ts to use diagnostics --- packages/coding-agent/src/core/diagnostics.ts | 15 ++++ .../coding-agent/src/core/resource-loader.ts | 49 ++---------- packages/coding-agent/src/core/skills.ts | 76 +++++++++---------- packages/coding-agent/src/index.ts | 1 - packages/coding-agent/test/skills.test.ts | 65 +++++++++------- 5 files changed, 95 insertions(+), 111 deletions(-) create mode 100644 packages/coding-agent/src/core/diagnostics.ts diff --git a/packages/coding-agent/src/core/diagnostics.ts b/packages/coding-agent/src/core/diagnostics.ts new file mode 100644 index 00000000..20fb8024 --- /dev/null +++ b/packages/coding-agent/src/core/diagnostics.ts @@ -0,0 +1,15 @@ +export interface ResourceCollision { + resourceType: "extension" | "skill" | "prompt" | "theme"; + name: string; // skill name, command/tool/flag name, prompt name, theme name + winnerPath: string; + loserPath: string; + winnerSource?: string; // e.g., "npm:foo", "git:...", "local" + loserSource?: string; +} + +export interface ResourceDiagnostic { + type: "warning" | "error" | "collision"; + message: string; + path?: string; + collision?: ResourceCollision; +} diff --git a/packages/coding-agent/src/core/resource-loader.ts b/packages/coding-agent/src/core/resource-loader.ts index 54a18ba5..1fd8b642 100644 --- a/packages/coding-agent/src/core/resource-loader.ts +++ b/packages/coding-agent/src/core/resource-loader.ts @@ -4,6 +4,10 @@ import { join, resolve } from "node:path"; import chalk from "chalk"; import { CONFIG_DIR_NAME, getAgentDir } from "../config.js"; import { loadThemeFromPath, type Theme } from "../modes/interactive/theme/theme.js"; +import type { ResourceDiagnostic } from "./diagnostics.js"; + +export type { ResourceCollision, ResourceDiagnostic } from "./diagnostics.js"; + import { createEventBus, type EventBus } from "./event-bus.js"; import { createExtensionRuntime, @@ -16,25 +20,9 @@ import { DefaultPackageManager, type PathMetadata } from "./package-manager.js"; import type { PromptTemplate } from "./prompt-templates.js"; import { loadPromptTemplates } from "./prompt-templates.js"; import { SettingsManager } from "./settings-manager.js"; -import type { Skill, SkillWarning } from "./skills.js"; +import type { Skill } from "./skills.js"; import { loadSkills } from "./skills.js"; -export interface ResourceCollision { - resourceType: "extension" | "skill" | "prompt" | "theme"; - name: string; // skill name, command/tool/flag name, prompt name, theme name - winnerPath: string; - loserPath: string; - winnerSource?: string; // e.g., "npm:foo", "git:...", "local" - loserSource?: string; -} - -export interface ResourceDiagnostic { - type: "warning" | "error" | "collision"; - message: string; - path?: string; - collision?: ResourceCollision; -} - export interface ResourceLoader { getExtensions(): LoadExtensionsResult; getSkills(): { skills: Skill[]; diagnostics: ResourceDiagnostic[] }; @@ -332,12 +320,11 @@ export class DefaultResourceLoader implements ResourceLoader { if (this.noSkills && skillPaths.length === 0) { skillsResult = { skills: [], diagnostics: [] }; } else { - const result = loadSkills({ + skillsResult = loadSkills({ cwd: this.cwd, agentDir: this.agentDir, skillPaths, }); - skillsResult = { skills: result.skills, diagnostics: this.skillWarningsToDiagnostics(result.warnings) }; } const resolvedSkills = this.skillsOverride ? this.skillsOverride(skillsResult) : skillsResult; this.skills = resolvedSkills.skills; @@ -520,30 +507,6 @@ export class DefaultResourceLoader implements ResourceLoader { return { extensions, errors }; } - private skillWarningsToDiagnostics(warnings: SkillWarning[]): ResourceDiagnostic[] { - return warnings.map((w) => { - // If it's a name collision, create proper collision structure - if (w.collisionName && w.collisionWinner) { - return { - type: "collision" as const, - message: w.message, - path: w.skillPath, - collision: { - resourceType: "skill" as const, - name: w.collisionName, - winnerPath: w.collisionWinner, - loserPath: w.skillPath, - }, - }; - } - return { - type: "warning" as const, - message: w.message, - path: w.skillPath, - }; - }); - } - private dedupePrompts(prompts: PromptTemplate[]): { prompts: PromptTemplate[]; diagnostics: ResourceDiagnostic[] } { const seen = new Map(); const diagnostics: ResourceDiagnostic[] = []; diff --git a/packages/coding-agent/src/core/skills.ts b/packages/coding-agent/src/core/skills.ts index eb5d1b62..5d4bd160 100644 --- a/packages/coding-agent/src/core/skills.ts +++ b/packages/coding-agent/src/core/skills.ts @@ -3,6 +3,7 @@ import { homedir } from "os"; import { basename, dirname, isAbsolute, join, resolve } from "path"; import { CONFIG_DIR_NAME, getAgentDir } from "../config.js"; import { parseFrontmatter } from "../utils/frontmatter.js"; +import type { ResourceDiagnostic } from "./diagnostics.js"; /** * Standard frontmatter fields per Agent Skills spec. @@ -37,18 +38,9 @@ export interface Skill { source: string; } -export interface SkillWarning { - skillPath: string; - message: string; - /** For name collisions, the name that collided */ - collisionName?: string; - /** For name collisions, the path of the skill that was loaded (winner) */ - collisionWinner?: string; -} - export interface LoadSkillsResult { skills: Skill[]; - warnings: SkillWarning[]; + diagnostics: ResourceDiagnostic[]; } /** @@ -130,10 +122,10 @@ export function loadSkillsFromDir(options: LoadSkillsFromDirOptions): LoadSkills function loadSkillsFromDirInternal(dir: string, source: string, includeRootFiles: boolean): LoadSkillsResult { const skills: Skill[] = []; - const warnings: SkillWarning[] = []; + const diagnostics: ResourceDiagnostic[] = []; if (!existsSync(dir)) { - return { skills, warnings }; + return { skills, diagnostics }; } try { @@ -168,7 +160,7 @@ function loadSkillsFromDirInternal(dir: string, source: string, includeRootFiles if (isDirectory) { const subResult = loadSkillsFromDirInternal(fullPath, source, false); skills.push(...subResult.skills); - warnings.push(...subResult.warnings); + diagnostics.push(...subResult.diagnostics); continue; } @@ -186,15 +178,18 @@ function loadSkillsFromDirInternal(dir: string, source: string, includeRootFiles if (result.skill) { skills.push(result.skill); } - warnings.push(...result.warnings); + diagnostics.push(...result.diagnostics); } } catch {} - return { skills, warnings }; + return { skills, diagnostics }; } -function loadSkillFromFile(filePath: string, source: string): { skill: Skill | null; warnings: SkillWarning[] } { - const warnings: SkillWarning[] = []; +function loadSkillFromFile( + filePath: string, + source: string, +): { skill: Skill | null; diagnostics: ResourceDiagnostic[] } { + const diagnostics: ResourceDiagnostic[] = []; try { const rawContent = readFileSync(filePath, "utf-8"); @@ -206,13 +201,13 @@ function loadSkillFromFile(filePath: string, source: string): { skill: Skill | n // Validate frontmatter fields const fieldErrors = validateFrontmatterFields(allKeys); for (const error of fieldErrors) { - warnings.push({ skillPath: filePath, message: error }); + diagnostics.push({ type: "warning", message: error, path: filePath }); } // Validate description const descErrors = validateDescription(frontmatter.description); for (const error of descErrors) { - warnings.push({ skillPath: filePath, message: error }); + diagnostics.push({ type: "warning", message: error, path: filePath }); } // Use name from frontmatter, or fall back to parent directory name @@ -221,12 +216,12 @@ function loadSkillFromFile(filePath: string, source: string): { skill: Skill | n // Validate name const nameErrors = validateName(name, parentDirName); for (const error of nameErrors) { - warnings.push({ skillPath: filePath, message: error }); + diagnostics.push({ type: "warning", message: error, path: filePath }); } // Still load the skill even with warnings (unless description is completely missing) if (!frontmatter.description || frontmatter.description.trim() === "") { - return { skill: null, warnings }; + return { skill: null, diagnostics }; } return { @@ -237,12 +232,12 @@ function loadSkillFromFile(filePath: string, source: string): { skill: Skill | n baseDir: skillDir, source, }, - warnings, + diagnostics, }; } catch (error) { const message = error instanceof Error ? error.message : "failed to parse skill file"; - warnings.push({ skillPath: filePath, message }); - return { skill: null, warnings }; + diagnostics.push({ type: "warning", message, path: filePath }); + return { skill: null, diagnostics }; } } @@ -309,7 +304,7 @@ function resolveSkillPath(p: string, cwd: string): string { /** * Load skills from all configured locations. - * Returns skills and any validation warnings. + * Returns skills and any validation diagnostics. */ export function loadSkills(options: LoadSkillsOptions = {}): LoadSkillsResult { const { cwd = process.cwd(), agentDir, skillPaths = [] } = options; @@ -319,11 +314,11 @@ export function loadSkills(options: LoadSkillsOptions = {}): LoadSkillsResult { const skillMap = new Map(); const realPathSet = new Set(); - const allWarnings: SkillWarning[] = []; - const collisionWarnings: SkillWarning[] = []; + const allDiagnostics: ResourceDiagnostic[] = []; + const collisionDiagnostics: ResourceDiagnostic[] = []; function addSkills(result: LoadSkillsResult) { - allWarnings.push(...result.warnings); + allDiagnostics.push(...result.diagnostics); for (const skill of result.skills) { // Resolve symlinks to detect duplicate files let realPath: string; @@ -340,11 +335,16 @@ export function loadSkills(options: LoadSkillsOptions = {}): LoadSkillsResult { const existing = skillMap.get(skill.name); if (existing) { - collisionWarnings.push({ - skillPath: skill.filePath, + collisionDiagnostics.push({ + type: "collision", message: `name "${skill.name}" collision`, - collisionName: skill.name, - collisionWinner: existing.filePath, + path: skill.filePath, + collision: { + resourceType: "skill", + name: skill.name, + winnerPath: existing.filePath, + loserPath: skill.filePath, + }, }); } else { skillMap.set(skill.name, skill); @@ -359,7 +359,7 @@ export function loadSkills(options: LoadSkillsOptions = {}): LoadSkillsResult { for (const rawPath of skillPaths) { const resolvedPath = resolveSkillPath(rawPath, cwd); if (!existsSync(resolvedPath)) { - allWarnings.push({ skillPath: resolvedPath, message: "skill path does not exist" }); + allDiagnostics.push({ type: "warning", message: "skill path does not exist", path: resolvedPath }); continue; } @@ -370,21 +370,21 @@ export function loadSkills(options: LoadSkillsOptions = {}): LoadSkillsResult { } else if (stats.isFile() && resolvedPath.endsWith(".md")) { const result = loadSkillFromFile(resolvedPath, "custom"); if (result.skill) { - addSkills({ skills: [result.skill], warnings: result.warnings }); + addSkills({ skills: [result.skill], diagnostics: result.diagnostics }); } else { - allWarnings.push(...result.warnings); + allDiagnostics.push(...result.diagnostics); } } else { - allWarnings.push({ skillPath: resolvedPath, message: "skill path is not a markdown file" }); + allDiagnostics.push({ type: "warning", message: "skill path is not a markdown file", path: resolvedPath }); } } catch (error) { const message = error instanceof Error ? error.message : "failed to read skill path"; - allWarnings.push({ skillPath: resolvedPath, message }); + allDiagnostics.push({ type: "warning", message, path: resolvedPath }); } } return { skills: Array.from(skillMap.values()), - warnings: [...allWarnings, ...collisionWarnings], + diagnostics: [...allDiagnostics, ...collisionDiagnostics], }; } diff --git a/packages/coding-agent/src/index.ts b/packages/coding-agent/src/index.ts index 3e8296a2..f6509551 100644 --- a/packages/coding-agent/src/index.ts +++ b/packages/coding-agent/src/index.ts @@ -187,7 +187,6 @@ export { loadSkillsFromDir, type Skill, type SkillFrontmatter, - type SkillWarning, } from "./core/skills.js"; // Tools export { diff --git a/packages/coding-agent/test/skills.test.ts b/packages/coding-agent/test/skills.test.ts index 74e7d855..6dec6f4b 100644 --- a/packages/coding-agent/test/skills.test.ts +++ b/packages/coding-agent/test/skills.test.ts @@ -1,6 +1,7 @@ import { homedir } from "os"; import { join, resolve } from "path"; import { describe, expect, it } from "vitest"; +import type { ResourceDiagnostic } from "../src/core/diagnostics.js"; import { formatSkillsForPrompt, loadSkills, loadSkillsFromDir, type Skill } from "../src/core/skills.js"; const fixturesDir = resolve(__dirname, "fixtures/skills"); @@ -9,7 +10,7 @@ const collisionFixturesDir = resolve(__dirname, "fixtures/skills-collision"); describe("skills", () => { describe("loadSkillsFromDir", () => { it("should load a valid skill", () => { - const { skills, warnings } = loadSkillsFromDir({ + const { skills, diagnostics } = loadSkillsFromDir({ dir: join(fixturesDir, "valid-skill"), source: "test", }); @@ -18,95 +19,101 @@ describe("skills", () => { expect(skills[0].name).toBe("valid-skill"); expect(skills[0].description).toBe("A valid skill for testing purposes."); expect(skills[0].source).toBe("test"); - expect(warnings).toHaveLength(0); + expect(diagnostics).toHaveLength(0); }); it("should warn when name doesn't match parent directory", () => { - const { skills, warnings } = loadSkillsFromDir({ + const { skills, diagnostics } = loadSkillsFromDir({ dir: join(fixturesDir, "name-mismatch"), source: "test", }); expect(skills).toHaveLength(1); expect(skills[0].name).toBe("different-name"); - expect(warnings.some((w) => w.message.includes("does not match parent directory"))).toBe(true); + expect( + diagnostics.some((d: ResourceDiagnostic) => d.message.includes("does not match parent directory")), + ).toBe(true); }); it("should warn when name contains invalid characters", () => { - const { skills, warnings } = loadSkillsFromDir({ + const { skills, diagnostics } = loadSkillsFromDir({ dir: join(fixturesDir, "invalid-name-chars"), source: "test", }); expect(skills).toHaveLength(1); - expect(warnings.some((w) => w.message.includes("invalid characters"))).toBe(true); + expect(diagnostics.some((d: ResourceDiagnostic) => d.message.includes("invalid characters"))).toBe(true); }); it("should warn when name exceeds 64 characters", () => { - const { skills, warnings } = loadSkillsFromDir({ + const { skills, diagnostics } = loadSkillsFromDir({ dir: join(fixturesDir, "long-name"), source: "test", }); expect(skills).toHaveLength(1); - expect(warnings.some((w) => w.message.includes("exceeds 64 characters"))).toBe(true); + expect(diagnostics.some((d: ResourceDiagnostic) => d.message.includes("exceeds 64 characters"))).toBe(true); }); it("should warn and skip skill when description is missing", () => { - const { skills, warnings } = loadSkillsFromDir({ + const { skills, diagnostics } = loadSkillsFromDir({ dir: join(fixturesDir, "missing-description"), source: "test", }); expect(skills).toHaveLength(0); - expect(warnings.some((w) => w.message.includes("description is required"))).toBe(true); + expect(diagnostics.some((d: ResourceDiagnostic) => d.message.includes("description is required"))).toBe(true); }); it("should warn when unknown frontmatter fields are present", () => { - const { skills, warnings } = loadSkillsFromDir({ + const { skills, diagnostics } = loadSkillsFromDir({ dir: join(fixturesDir, "unknown-field"), source: "test", }); expect(skills).toHaveLength(1); - expect(warnings.some((w) => w.message.includes('unknown frontmatter field "author"'))).toBe(true); - expect(warnings.some((w) => w.message.includes('unknown frontmatter field "version"'))).toBe(true); + expect( + diagnostics.some((d: ResourceDiagnostic) => d.message.includes('unknown frontmatter field "author"')), + ).toBe(true); + expect( + diagnostics.some((d: ResourceDiagnostic) => d.message.includes('unknown frontmatter field "version"')), + ).toBe(true); }); it("should load nested skills recursively", () => { - const { skills, warnings } = loadSkillsFromDir({ + const { skills, diagnostics } = loadSkillsFromDir({ dir: join(fixturesDir, "nested"), source: "test", }); expect(skills).toHaveLength(1); expect(skills[0].name).toBe("child-skill"); - expect(warnings).toHaveLength(0); + expect(diagnostics).toHaveLength(0); }); it("should skip files without frontmatter", () => { - const { skills, warnings } = loadSkillsFromDir({ + const { skills, diagnostics } = loadSkillsFromDir({ dir: join(fixturesDir, "no-frontmatter"), source: "test", }); // no-frontmatter has no description, so it should be skipped expect(skills).toHaveLength(0); - expect(warnings.some((w) => w.message.includes("description is required"))).toBe(true); + expect(diagnostics.some((d: ResourceDiagnostic) => d.message.includes("description is required"))).toBe(true); }); it("should warn and skip skill when YAML frontmatter is invalid", () => { - const { skills, warnings } = loadSkillsFromDir({ + const { skills, diagnostics } = loadSkillsFromDir({ dir: join(fixturesDir, "invalid-yaml"), source: "test", }); expect(skills).toHaveLength(0); - expect(warnings.some((w) => w.message.includes("at line"))).toBe(true); + expect(diagnostics.some((d: ResourceDiagnostic) => d.message.includes("at line"))).toBe(true); }); it("should preserve multiline descriptions from YAML", () => { - const { skills, warnings } = loadSkillsFromDir({ + const { skills, diagnostics } = loadSkillsFromDir({ dir: join(fixturesDir, "multiline-description"), source: "test", }); @@ -114,17 +121,17 @@ describe("skills", () => { expect(skills).toHaveLength(1); expect(skills[0].description).toContain("\n"); expect(skills[0].description).toContain("This is a multiline description."); - expect(warnings).toHaveLength(0); + expect(diagnostics).toHaveLength(0); }); it("should warn when name contains consecutive hyphens", () => { - const { skills, warnings } = loadSkillsFromDir({ + const { skills, diagnostics } = loadSkillsFromDir({ dir: join(fixturesDir, "consecutive-hyphens"), source: "test", }); expect(skills).toHaveLength(1); - expect(warnings.some((w) => w.message.includes("consecutive hyphens"))).toBe(true); + expect(diagnostics.some((d: ResourceDiagnostic) => d.message.includes("consecutive hyphens"))).toBe(true); }); it("should load all skills from fixture directory", () => { @@ -140,13 +147,13 @@ describe("skills", () => { }); it("should return empty for non-existent directory", () => { - const { skills, warnings } = loadSkillsFromDir({ + const { skills, diagnostics } = loadSkillsFromDir({ dir: "/non/existent/path", source: "test", }); expect(skills).toHaveLength(0); - expect(warnings).toHaveLength(0); + expect(diagnostics).toHaveLength(0); }); it("should use parent directory name when name not in frontmatter", () => { @@ -258,24 +265,24 @@ describe("skills", () => { const emptyCwd = resolve(__dirname, "fixtures/empty-cwd"); it("should load from explicit skillPaths", () => { - const { skills, warnings } = loadSkills({ + const { skills, diagnostics } = loadSkills({ agentDir: emptyAgentDir, cwd: emptyCwd, skillPaths: [join(fixturesDir, "valid-skill")], }); expect(skills).toHaveLength(1); expect(skills[0].source).toBe("custom"); - expect(warnings).toHaveLength(0); + expect(diagnostics).toHaveLength(0); }); it("should warn when skill path does not exist", () => { - const { skills, warnings } = loadSkills({ + const { skills, diagnostics } = loadSkills({ agentDir: emptyAgentDir, cwd: emptyCwd, skillPaths: ["/non/existent/path"], }); expect(skills).toHaveLength(0); - expect(warnings.some((w) => w.message.includes("does not exist"))).toBe(true); + expect(diagnostics.some((d: ResourceDiagnostic) => d.message.includes("does not exist"))).toBe(true); }); it("should expand ~ in skillPaths", () => {