getcompanion-ai/co-mono
1
0
Fork 0
mirror of https://github.com/getcompanion-ai/co-mono.git synced 2026-04-15 09:01:14 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions

refactor(coding-agent): unify SkillWarning and ResourceDiagnostic

Browse source 
- 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
This commit is contained in:
Mario Zechner 2026-01-24 00:45:01 +01:00
parent 6d0d434d8c
commit 4719929f6a
5 changed files with 95 additions and 111 deletions
Download patch file Download diff file Expand all files Collapse all files

15
packages/coding-agent/src/core/diagnostics.ts Normal file
View file

@ -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;
}

49
packages/coding-agent/src/core/resource-loader.ts
View file

@ -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<string, PromptTemplate>();
const diagnostics: ResourceDiagnostic[] = [];

76
packages/coding-agent/src/core/skills.ts
View file

@ -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<string, Skill>();
const realPathSet = new Set<string>();
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],
};
}

1
packages/coding-agent/src/index.ts
View file

@ -187,7 +187,6 @@ export {
loadSkillsFromDir,
type Skill,
type SkillFrontmatter,
type SkillWarning,
} from "./core/skills.js";
// Tools
export {

65
packages/coding-agent/test/skills.test.ts
View file

@ -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", () => {