From 3dd8375a877d41191f0d9429df2ccb542b5b83f0 Mon Sep 17 00:00:00 2001 From: dyoshikawa Date: Mon, 27 Jul 2026 00:13:12 -0700 Subject: [PATCH 1/4] fix(agentsskills): emit spec-conformant SKILL.md and diagnose name/description violations The `agentsskills` target is rulesync's implementation of the Agent Skills standard, but the SKILL.md it generated was not conformant. Export conformance. The spec types `allowed-tools` as "a space-separated string of tools", `compatibility` as a 1-500 character string, and `metadata` as "a map from string keys to string values". `fromRulesyncSkill` copied all three verbatim from the rulesync frontmatter, so a legacy list or object input was written out as a YAML sequence / mapping that the spec's own `skills-ref validate` rejects. They are now normalized on the way out: a list is joined with spaces (mirroring `DeepagentsSkill`, which already did this), an object `compatibility` is flattened to `key: value` pairs, and non-string `metadata` values are stringified. Input stays permissive, so nothing existing breaks. Validation. The normative `name` grammar (1-64 chars, lowercase alphanumerics and single hyphens, must match the parent directory name) and the non-empty `description` requirement were never checked, so rulesync could emit a skill that conformant clients silently skip at the user's runtime. Generation now reports each violation through the logger, plus an over-length `compatibility`. These are warnings, not errors: a client only skips such a skill, import stays lenient per the spec's client-implementation guide, and failing outright would break existing skill directories. Reporting requires a logger, so `ToolSkillFromRulesyncSkillParams` gains an optional `logger` that the skills processor now passes through. Refs https://agentskills.io/specification Closes #2429 Co-Authored-By: Claude Opus 5 (1M context) --- docs/reference/file-formats.md | 2 + skills/rulesync/file-formats.md | 2 + .../skills/agentsskills-skill.test.ts | 128 ++++++++++++++++ src/features/skills/agentsskills-skill.ts | 142 +++++++++++++++++- src/features/skills/skills-processor.ts | 1 + src/features/skills/tool-skill.ts | 7 + 6 files changed, 277 insertions(+), 5 deletions(-) diff --git a/docs/reference/file-formats.md b/docs/reference/file-formats.md index 3c0c4d079..f662431b3 100644 --- a/docs/reference/file-formats.md +++ b/docs/reference/file-formats.md @@ -324,6 +324,8 @@ The command body itself uses a Claude Code-compatible **universal syntax** (e.g. > **Devin note:** Devin's extensibility docs no longer document a standalone workflows/commands component — reusable prompts invoked as slash commands are [Skills](https://docs.devin.ai/cli/extensibility/skills/overview) (`/name`). Rulesync therefore emits each command onto the native skills surface as `.devin/skills//SKILL.md` (project) / `~/.config/devin/skills//SKILL.md` (global, via `--global`), with `name`/`description` frontmatter derived from the command file. The legacy Windsurf/Cascade-era `.devin/workflows/` and `~/.codeium/windsurf/global_workflows/` locations are no longer emitted (stale outputs there stay gitignored but are not cleaned up automatically). Commands import and `--delete` are no-ops for `devin` because the skills feature owns the `.devin/skills/` tree (importing it as commands would double-import every skill). Note that a command and a skill sharing the same name write the same `SKILL.md` path, so keep command and skill names distinct for this target. +> **Agent Skills standard note (`agentsskills`):** Rulesync accepts the legacy rulesync spellings on input but always **emits** the shapes the [specification](https://agentskills.io/specification) requires: `allowed-tools` becomes a space-separated scalar (a YAML list is joined), `compatibility` becomes a string (an object is flattened to `key: value` pairs), and `metadata` values are stringified so the block stays a string→string map. Generation also checks the normative constraints and warns — without failing the run — when `name` is empty, longer than 64 characters, contains anything but lowercase letters, digits and single hyphens, or does not match its parent directory name; when `description` is empty or longer than 1024 characters; or when `compatibility` exceeds 500 characters. These are warnings rather than errors because a conformant client only _skips_ such a skill, and because import stays lenient as the [client-implementation guide](https://agentskills.io/client-implementation/adding-skills-support) advises. Validate the result with the spec's own [`skills-ref validate`](https://github.com/agentskills/agentskills/tree/main/skills-ref). + > **Hermes Agent note:** Commands are global-only and remain distinct from skills. Rulesync writes JSON command specs to `~/.hermes/rulesync/commands/.json`, installs the `rulesync-commands` plugin under `~/.hermes/plugins/`, and enables it in `~/.hermes/config.yaml`. The plugin registers each spec with Hermes's [`ctx.register_command()` plugin API](https://hermes-agent.nousresearch.com/docs/user-guide/features/plugins/) and dispatches the prompt through `delegate_task`; invocation arguments are appended to the prompt. `.rulesync/skills//SKILL.md` still generates a full [Hermes Agent Skill](https://hermes-agent.nousresearch.com/docs/user-guide/features/skills/) under `~/.hermes/skills//SKILL.md`, which Hermes also exposes as a dynamic slash command. Rulesync rejects command/skill names that would collide in Hermes's slash-command namespace, and rejects nested command paths that flatten to the same name. Generate commands with `rulesync generate --targets hermesagent --features commands --global`. > > Releases before this native plugin transport emitted Hermes commands as `~/.hermes/skills//SKILL.md`. Rulesync cannot distinguish those files from real user-authored skills safely, so remove an obsolete legacy file manually after confirming that `.rulesync/skills//SKILL.md` does not own it. diff --git a/skills/rulesync/file-formats.md b/skills/rulesync/file-formats.md index 373543441..acb6ff1ce 100644 --- a/skills/rulesync/file-formats.md +++ b/skills/rulesync/file-formats.md @@ -324,6 +324,8 @@ The command body itself uses a Claude Code-compatible **universal syntax** (e.g. > **Devin note:** Devin's extensibility docs no longer document a standalone workflows/commands component — reusable prompts invoked as slash commands are [Skills](https://docs.devin.ai/cli/extensibility/skills/overview) (`/name`). Rulesync therefore emits each command onto the native skills surface as `.devin/skills//SKILL.md` (project) / `~/.config/devin/skills//SKILL.md` (global, via `--global`), with `name`/`description` frontmatter derived from the command file. The legacy Windsurf/Cascade-era `.devin/workflows/` and `~/.codeium/windsurf/global_workflows/` locations are no longer emitted (stale outputs there stay gitignored but are not cleaned up automatically). Commands import and `--delete` are no-ops for `devin` because the skills feature owns the `.devin/skills/` tree (importing it as commands would double-import every skill). Note that a command and a skill sharing the same name write the same `SKILL.md` path, so keep command and skill names distinct for this target. +> **Agent Skills standard note (`agentsskills`):** Rulesync accepts the legacy rulesync spellings on input but always **emits** the shapes the [specification](https://agentskills.io/specification) requires: `allowed-tools` becomes a space-separated scalar (a YAML list is joined), `compatibility` becomes a string (an object is flattened to `key: value` pairs), and `metadata` values are stringified so the block stays a string→string map. Generation also checks the normative constraints and warns — without failing the run — when `name` is empty, longer than 64 characters, contains anything but lowercase letters, digits and single hyphens, or does not match its parent directory name; when `description` is empty or longer than 1024 characters; or when `compatibility` exceeds 500 characters. These are warnings rather than errors because a conformant client only _skips_ such a skill, and because import stays lenient as the [client-implementation guide](https://agentskills.io/client-implementation/adding-skills-support) advises. Validate the result with the spec's own [`skills-ref validate`](https://github.com/agentskills/agentskills/tree/main/skills-ref). + > **Hermes Agent note:** Commands are global-only and remain distinct from skills. Rulesync writes JSON command specs to `~/.hermes/rulesync/commands/.json`, installs the `rulesync-commands` plugin under `~/.hermes/plugins/`, and enables it in `~/.hermes/config.yaml`. The plugin registers each spec with Hermes's [`ctx.register_command()` plugin API](https://hermes-agent.nousresearch.com/docs/user-guide/features/plugins/) and dispatches the prompt through `delegate_task`; invocation arguments are appended to the prompt. `.rulesync/skills//SKILL.md` still generates a full [Hermes Agent Skill](https://hermes-agent.nousresearch.com/docs/user-guide/features/skills/) under `~/.hermes/skills//SKILL.md`, which Hermes also exposes as a dynamic slash command. Rulesync rejects command/skill names that would collide in Hermes's slash-command namespace, and rejects nested command paths that flatten to the same name. Generate commands with `rulesync generate --targets hermesagent --features commands --global`. > > Releases before this native plugin transport emitted Hermes commands as `~/.hermes/skills//SKILL.md`. Rulesync cannot distinguish those files from real user-authored skills safely, so remove an obsolete legacy file manually after confirming that `.rulesync/skills//SKILL.md` does not own it. diff --git a/src/features/skills/agentsskills-skill.test.ts b/src/features/skills/agentsskills-skill.test.ts index 79351ea64..1154cae2f 100644 --- a/src/features/skills/agentsskills-skill.test.ts +++ b/src/features/skills/agentsskills-skill.test.ts @@ -4,6 +4,7 @@ import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import { SKILL_FILE_NAME } from "../../constants/general.js"; import { RULESYNC_SKILLS_RELATIVE_DIR_PATH } from "../../constants/rulesync-paths.js"; +import { createMockLogger } from "../../test-utils/mock-logger.js"; import { setupTestDirectory } from "../../test-utils/test-directories.js"; import { ensureDir, writeFileContent } from "../../utils/file.js"; import { AgentsSkillsSkill } from "./agentsskills-skill.js"; @@ -198,6 +199,133 @@ Body.`; description: "Test skill description", }); }); + + it("should serialize allowed-tools, compatibility and metadata into the spec's scalar forms", () => { + const rulesyncSkill = new RulesyncSkill({ + outputRoot: testDir, + relativeDirPath: RULESYNC_SKILLS_RELATIVE_DIR_PATH, + dirName: "demo-skill", + frontmatter: { + name: "demo-skill", + description: "Demo skill for conformance check.", + agentsskills: { + "allowed-tools": ["Read", "Bash(git:*)"], + compatibility: { runtime: "node", packages: ["jq"] }, + metadata: { version: 1, author: "example-org", tags: ["a", "b"] }, + }, + }, + body: "Body", + validate: true, + }); + + const agentsSkillsSkill = AgentsSkillsSkill.fromRulesyncSkill({ rulesyncSkill }); + + expect(agentsSkillsSkill.getFrontmatter()).toEqual({ + name: "demo-skill", + description: "Demo skill for conformance check.", + "allowed-tools": "Read Bash(git:*)", + compatibility: 'runtime: node, packages: ["jq"]', + metadata: { version: "1", author: "example-org", tags: '["a","b"]' }, + }); + }); + + it("should leave already-conformant scalar values untouched", () => { + const rulesyncSkill = new RulesyncSkill({ + outputRoot: testDir, + relativeDirPath: RULESYNC_SKILLS_RELATIVE_DIR_PATH, + dirName: "demo-skill", + frontmatter: { + name: "demo-skill", + description: "Demo skill.", + agentsskills: { + "allowed-tools": "Bash(git:*) Read", + compatibility: "Requires Python 3.14+ and uv", + metadata: { version: "1.0" }, + }, + }, + body: "Body", + validate: true, + }); + + expect(AgentsSkillsSkill.fromRulesyncSkill({ rulesyncSkill }).getFrontmatter()).toEqual({ + name: "demo-skill", + description: "Demo skill.", + "allowed-tools": "Bash(git:*) Read", + compatibility: "Requires Python 3.14+ and uv", + metadata: { version: "1.0" }, + }); + }); + + it("should warn about every normative name/description violation without failing", () => { + const logger = createMockLogger(); + const rulesyncSkill = new RulesyncSkill({ + outputRoot: testDir, + relativeDirPath: RULESYNC_SKILLS_RELATIVE_DIR_PATH, + dirName: "My_Bad--Name", + frontmatter: { + name: "Totally-Different-NAME--x", + description: "", + }, + body: "Body", + validate: true, + }); + + const agentsSkillsSkill = AgentsSkillsSkill.fromRulesyncSkill({ rulesyncSkill, logger }); + + // Generation still succeeds — import stays lenient per the spec's client guide. + expect(agentsSkillsSkill).toBeInstanceOf(AgentsSkillsSkill); + + const warnings = logger.warn.mock.calls.map(([message]) => String(message)); + expect(warnings).toHaveLength(3); + expect(warnings[0]).toContain("lowercase letters, digits and single hyphens"); + expect(warnings[1]).toContain('must match its parent directory name "My_Bad--Name"'); + expect(warnings[2]).toContain("`description` is required and must not be empty"); + for (const warning of warnings) { + expect(warning).toContain(join(".agents", "skills", "My_Bad--Name", SKILL_FILE_NAME)); + } + }); + + it("should warn when name, description or compatibility exceed their length limits", () => { + const logger = createMockLogger(); + const rulesyncSkill = new RulesyncSkill({ + outputRoot: testDir, + relativeDirPath: RULESYNC_SKILLS_RELATIVE_DIR_PATH, + dirName: "a".repeat(65), + frontmatter: { + name: "a".repeat(65), + description: "d".repeat(1025), + agentsskills: { compatibility: "c".repeat(501) }, + }, + body: "Body", + validate: true, + }); + + AgentsSkillsSkill.fromRulesyncSkill({ rulesyncSkill, logger }); + + const warnings = logger.warn.mock.calls.map(([message]) => String(message)); + expect(warnings.some((w) => w.includes("`name` is 65 characters"))).toBe(true); + expect(warnings.some((w) => w.includes("`description` is 1025 characters"))).toBe(true); + expect(warnings.some((w) => w.includes("`compatibility` is 501 characters"))).toBe(true); + }); + + it("should not warn for a fully conformant skill", () => { + const logger = createMockLogger(); + const rulesyncSkill = new RulesyncSkill({ + outputRoot: testDir, + relativeDirPath: RULESYNC_SKILLS_RELATIVE_DIR_PATH, + dirName: "pdf-processing", + frontmatter: { + name: "pdf-processing", + description: "Extract PDF text. Use when handling PDFs.", + }, + body: "Body", + validate: true, + }); + + AgentsSkillsSkill.fromRulesyncSkill({ rulesyncSkill, logger }); + + expect(logger.warn).not.toHaveBeenCalled(); + }); }); describe("isTargetedByRulesyncSkill", () => { diff --git a/src/features/skills/agentsskills-skill.ts b/src/features/skills/agentsskills-skill.ts index e0f04331e..76a6bc635 100644 --- a/src/features/skills/agentsskills-skill.ts +++ b/src/features/skills/agentsskills-skill.ts @@ -30,6 +30,119 @@ const AgentsSkillsSkillFrontmatterSchema = z.looseObject({ export type AgentsSkillsSkillFrontmatter = z.infer; +// Normative limits from the Agent Skills specification. +// https://agentskills.io/specification +const NAME_MAX_LENGTH = 64; +const DESCRIPTION_MAX_LENGTH = 1024; +const COMPATIBILITY_MAX_LENGTH = 500; + +// "Unicode lowercase alphanumeric characters (`a-z`, `0-9`) and hyphens (`-`)", +// with no leading/trailing hyphen and no consecutive hyphens — all four rules +// expressed as alphanumeric runs joined by single hyphens. +const NAME_PATTERN = /^[a-z0-9]+(?:-[a-z0-9]+)*$/; + +/** + * Render a non-string YAML value as the string the spec requires. Scalars use + * their natural text form (`1` → `"1"`), containers are JSON-encoded so the + * original structure stays readable rather than collapsing to `[object Object]`. + */ +function stringifyValue(value: unknown): string { + if (typeof value === "string") { + return value; + } + if (typeof value === "object" && value !== null) { + return JSON.stringify(value); + } + return String(value); +} + +/** + * The spec types `allowed-tools` as "a space-separated string of tools", so an + * array from a legacy rulesync input is joined rather than emitted as a YAML + * sequence. Mirrors `DeepagentsSkill`. + */ +function toAllowedToolsString(value: string | string[]): string { + return Array.isArray(value) ? value.join(" ") : value; +} + +/** + * The spec types `compatibility` as a free-form string. An object from a legacy + * rulesync input is flattened to `key: value` pairs instead of being emitted as + * a YAML mapping, which conformant clients reject. + */ +function toCompatibilityString(value: string | Record): string { + if (typeof value === "string") { + return value; + } + return Object.entries(value) + .map(([key, entry]) => `${key}: ${stringifyValue(entry)}`) + .join(", "); +} + +/** + * The spec types `metadata` as "a map from string keys to string values", so + * non-string values (e.g. a YAML number `version: 1`) are stringified. + */ +function toStringMetadata(metadata: Record): Record { + return Object.fromEntries( + Object.entries(metadata).map(([key, value]) => [key, stringifyValue(value)]), + ); +} + +/** + * Collect the normative `name` / `description` violations the Agent Skills spec + * defines. These are reported as warnings rather than errors: import stays + * lenient per the spec's client-implementation guide, and failing generation + * outright would break existing skill directories. What must not happen is + * emitting a skill that conformant clients silently skip without saying so. + * + * @see https://agentskills.io/specification + * @see https://agentskills.io/client-implementation/adding-skills-support + */ +function collectSpecViolations({ + name, + description, + dirName, +}: { + name: string; + description: string; + dirName: string; +}): string[] { + const violations: string[] = []; + + if (name.length === 0) { + violations.push("`name` is required and must not be empty"); + } else { + if (name.length > NAME_MAX_LENGTH) { + violations.push( + `\`name\` is ${name.length} characters; the Agent Skills spec allows at most ${NAME_MAX_LENGTH}`, + ); + } + if (!NAME_PATTERN.test(name)) { + violations.push( + `\`name\` "${name}" must contain only lowercase letters, digits and single hyphens, with no leading, trailing or consecutive hyphens`, + ); + } + if (name !== dirName) { + violations.push( + `\`name\` "${name}" must match its parent directory name "${dirName}"; conformant clients require them to be equal`, + ); + } + } + + if (description.length === 0) { + violations.push( + "`description` is required and must not be empty; conformant clients skip a skill without one", + ); + } else if (description.length > DESCRIPTION_MAX_LENGTH) { + violations.push( + `\`description\` is ${description.length} characters; the Agent Skills spec allows at most ${DESCRIPTION_MAX_LENGTH}`, + ); + } + + return violations; +} + export type AgentsSkillsSkillParams = { outputRoot?: string; relativeDirPath?: string; @@ -152,26 +265,45 @@ export class AgentsSkillsSkill extends ToolSkill { rulesyncSkill, validate = true, global = false, + logger, }: ToolSkillFromRulesyncSkillParams): AgentsSkillsSkill { const settablePaths = AgentsSkillsSkill.getSettablePaths({ global }); const rulesyncFrontmatter = rulesyncSkill.getFrontmatter(); const agentsskillsSection = rulesyncFrontmatter.agentsskills; + const dirName = rulesyncSkill.getDirName(); + const skillPath = join(settablePaths.relativeDirPath, dirName, SKILL_FILE_NAME); + + const compatibility = + agentsskillsSection?.compatibility === undefined + ? undefined + : toCompatibilityString(agentsskillsSection.compatibility); + if (compatibility !== undefined && compatibility.length > COMPATIBILITY_MAX_LENGTH) { + logger?.warn( + `${skillPath}: \`compatibility\` is ${compatibility.length} characters; the Agent Skills spec allows at most ${COMPATIBILITY_MAX_LENGTH}`, + ); + } const agentsSkillsFrontmatter: AgentsSkillsSkillFrontmatter = { name: rulesyncFrontmatter.name, description: rulesyncFrontmatter.description, ...(agentsskillsSection?.license !== undefined && { license: agentsskillsSection.license }), - ...(agentsskillsSection?.compatibility !== undefined && { - compatibility: agentsskillsSection.compatibility, - }), + ...(compatibility !== undefined && { compatibility }), ...(agentsskillsSection?.metadata !== undefined && { - metadata: agentsskillsSection.metadata, + metadata: toStringMetadata(agentsskillsSection.metadata), }), ...(agentsskillsSection?.["allowed-tools"] !== undefined && { - "allowed-tools": agentsskillsSection["allowed-tools"], + "allowed-tools": toAllowedToolsString(agentsskillsSection["allowed-tools"]), }), }; + for (const violation of collectSpecViolations({ + name: rulesyncFrontmatter.name, + description: rulesyncFrontmatter.description, + dirName, + })) { + logger?.warn(`${skillPath}: ${violation}`); + } + return new this({ outputRoot, relativeDirPath: settablePaths.relativeDirPath, diff --git a/src/features/skills/skills-processor.ts b/src/features/skills/skills-processor.ts index 8111d4a97..e59e938f8 100644 --- a/src/features/skills/skills-processor.ts +++ b/src/features/skills/skills-processor.ts @@ -531,6 +531,7 @@ export class SkillsProcessor extends DirFeatureProcessor { outputRoot: this.outputRoot, rulesyncSkill: rulesyncSkill, global: this.global, + logger: this.logger, }); }) .filter((skill): skill is ToolSkill => skill !== null); diff --git a/src/features/skills/tool-skill.ts b/src/features/skills/tool-skill.ts index 5357cb487..e8f39fbe0 100644 --- a/src/features/skills/tool-skill.ts +++ b/src/features/skills/tool-skill.ts @@ -4,6 +4,7 @@ import { SKILL_FILE_NAME } from "../../constants/general.js"; import { AiDir } from "../../types/ai-dir.js"; import { fileExists, readFileContent } from "../../utils/file.js"; import { parseFrontmatter } from "../../utils/frontmatter.js"; +import type { Logger } from "../../utils/logger.js"; import { RulesyncSkill, SkillFile } from "./rulesync-skill.js"; export type ToolSkillFromRulesyncSkillParams = { @@ -11,6 +12,12 @@ export type ToolSkillFromRulesyncSkillParams = { rulesyncSkill: RulesyncSkill; validate?: boolean; global?: boolean; + /** + * Optional so subclasses that have nothing to report can ignore it. Used to + * surface lossy conversions and spec violations that must not fail the run + * (e.g. a skill name that conformant Agent Skills clients would reject). + */ + logger?: Logger; }; export type ToolSkillSettablePaths = { From 2233edf283d98948cd1e845d51b722dc565f485e Mon Sep 17 00:00:00 2001 From: dyoshikawa Date: Mon, 27 Jul 2026 00:26:51 -0700 Subject: [PATCH 2/4] fix(agentsskills): share the normalization with hermesagent and preserve YAML timestamps MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review feedback on the first commit. - `stringifyValue` ran a YAML timestamp through `JSON.stringify`, folding the encoder's own quotes into the emitted scalar: `released: 2024-01-01` became `released: '"2024-01-01T00:00:00.000Z"'`, which is worse than what it replaced. A Date is now rendered as its ISO form before the container branch. - `HermesagentSkill` reads the same `agentsskills` block from the same rulesync source but spread it verbatim, so one input produced a space-separated scalar at `.agents/skills/` and a YAML list at `~/.hermes/skills/`. Both targets now go through `toSpecConformantAgentSkillFields`, and Hermes reports the same diagnostics (it previously reported none). - Diagnostics use `warnWithFallback` instead of `logger?.warn`, per the existing convention, so a call site that passes no logger still surfaces them. - `toRulesyncSkill` normalizes `allowed-tools` back to the canonical rulesync array, mirroring `DeepagentsSkill` in both directions. Without it a generate → import round trip silently rewrote the `.rulesync` source from a list to a string. - A value that normalizes to the empty string (`compatibility: {}`, `allowed-tools: []`) is dropped rather than emitted as `''`, which the spec's 1-500 character `compatibility` rule rejects. - The `compatibility` length check and the new whitespace check for `allowed-tools` entries moved into `collectAgentSkillViolations`, so all diagnostics come from one place. Security review follow-up, same commit: `stringifyValue` encodes each object node at most once. YAML anchors let a hand-written SKILL.md produce shared or self-referential objects, which made a plain `JSON.stringify` throw on a cycle (aborting the run) and expand a few hundred bytes of aliases into tens of megabytes. Tests: emitted-YAML assertions via a new e2e case, logger propagation through SkillsProcessor, Date/boolean/empty-container metadata, post-flattening compatibility length, cycles and shared references, and whitespace in an `allowed-tools` entry. Co-Authored-By: Claude Opus 5 (1M context) --- docs/reference/file-formats.md | 2 +- skills/rulesync/file-formats.md | 2 +- src/e2e/e2e-skills.spec.ts | 35 +++ .../skills/agentsskills-skill.test.ts | 154 +++++++++++++- src/features/skills/agentsskills-skill.ts | 200 ++++++++++++++---- src/features/skills/hermesagent-skill.test.ts | 4 +- src/features/skills/hermesagent-skill.ts | 19 +- src/features/skills/skills-processor.test.ts | 28 +++ 8 files changed, 395 insertions(+), 49 deletions(-) diff --git a/docs/reference/file-formats.md b/docs/reference/file-formats.md index f662431b3..26e680b7b 100644 --- a/docs/reference/file-formats.md +++ b/docs/reference/file-formats.md @@ -324,7 +324,7 @@ The command body itself uses a Claude Code-compatible **universal syntax** (e.g. > **Devin note:** Devin's extensibility docs no longer document a standalone workflows/commands component — reusable prompts invoked as slash commands are [Skills](https://docs.devin.ai/cli/extensibility/skills/overview) (`/name`). Rulesync therefore emits each command onto the native skills surface as `.devin/skills//SKILL.md` (project) / `~/.config/devin/skills//SKILL.md` (global, via `--global`), with `name`/`description` frontmatter derived from the command file. The legacy Windsurf/Cascade-era `.devin/workflows/` and `~/.codeium/windsurf/global_workflows/` locations are no longer emitted (stale outputs there stay gitignored but are not cleaned up automatically). Commands import and `--delete` are no-ops for `devin` because the skills feature owns the `.devin/skills/` tree (importing it as commands would double-import every skill). Note that a command and a skill sharing the same name write the same `SKILL.md` path, so keep command and skill names distinct for this target. -> **Agent Skills standard note (`agentsskills`):** Rulesync accepts the legacy rulesync spellings on input but always **emits** the shapes the [specification](https://agentskills.io/specification) requires: `allowed-tools` becomes a space-separated scalar (a YAML list is joined), `compatibility` becomes a string (an object is flattened to `key: value` pairs), and `metadata` values are stringified so the block stays a string→string map. Generation also checks the normative constraints and warns — without failing the run — when `name` is empty, longer than 64 characters, contains anything but lowercase letters, digits and single hyphens, or does not match its parent directory name; when `description` is empty or longer than 1024 characters; or when `compatibility` exceeds 500 characters. These are warnings rather than errors because a conformant client only _skips_ such a skill, and because import stays lenient as the [client-implementation guide](https://agentskills.io/client-implementation/adding-skills-support) advises. Validate the result with the spec's own [`skills-ref validate`](https://github.com/agentskills/agentskills/tree/main/skills-ref). +> **Agent Skills standard note (`agentsskills`):** Rulesync accepts the legacy rulesync spellings on input but always **emits** the shapes the [specification](https://agentskills.io/specification) requires: `allowed-tools` becomes a space-separated scalar (a YAML list is joined), `compatibility` becomes a string (an object is flattened to `key: value` pairs), and `metadata` values are stringified so the block stays a string→string map. Generation also checks the normative constraints and warns — without failing the run — when `name` is empty, longer than 64 characters, contains anything but lowercase letters, digits and single hyphens, or does not match its parent directory name; when `description` is empty or longer than 1024 characters; when `compatibility` exceeds 500 characters; or when an `allowed-tools` list entry contains whitespace, which the space-separated form cannot represent. These are warnings rather than errors because a conformant client only _skips_ such a skill, and because import stays lenient as the [client-implementation guide](https://agentskills.io/client-implementation/adding-skills-support) advises. A value that normalizes to the empty string (`compatibility: {}`, `allowed-tools: []`) is dropped rather than written, since the spec requires `compatibility` to be 1–500 characters when present. On **import**, `allowed-tools` is normalized back to the canonical rulesync list, so a generate → import round trip leaves `.rulesync/skills/**` in the shape it started in (the `compatibility` and `metadata` coercions are one-way, because the legacy object/number forms have no conformant equivalent). `hermesagent` reads the same `agentsskills` block and applies the same normalization, so one rulesync source never produces two different on-disk spellings. Validate the result with the spec's own [`skills-ref validate`](https://github.com/agentskills/agentskills/tree/main/skills-ref). > **Hermes Agent note:** Commands are global-only and remain distinct from skills. Rulesync writes JSON command specs to `~/.hermes/rulesync/commands/.json`, installs the `rulesync-commands` plugin under `~/.hermes/plugins/`, and enables it in `~/.hermes/config.yaml`. The plugin registers each spec with Hermes's [`ctx.register_command()` plugin API](https://hermes-agent.nousresearch.com/docs/user-guide/features/plugins/) and dispatches the prompt through `delegate_task`; invocation arguments are appended to the prompt. `.rulesync/skills//SKILL.md` still generates a full [Hermes Agent Skill](https://hermes-agent.nousresearch.com/docs/user-guide/features/skills/) under `~/.hermes/skills//SKILL.md`, which Hermes also exposes as a dynamic slash command. Rulesync rejects command/skill names that would collide in Hermes's slash-command namespace, and rejects nested command paths that flatten to the same name. Generate commands with `rulesync generate --targets hermesagent --features commands --global`. > diff --git a/skills/rulesync/file-formats.md b/skills/rulesync/file-formats.md index acb6ff1ce..131f319d4 100644 --- a/skills/rulesync/file-formats.md +++ b/skills/rulesync/file-formats.md @@ -324,7 +324,7 @@ The command body itself uses a Claude Code-compatible **universal syntax** (e.g. > **Devin note:** Devin's extensibility docs no longer document a standalone workflows/commands component — reusable prompts invoked as slash commands are [Skills](https://docs.devin.ai/cli/extensibility/skills/overview) (`/name`). Rulesync therefore emits each command onto the native skills surface as `.devin/skills//SKILL.md` (project) / `~/.config/devin/skills//SKILL.md` (global, via `--global`), with `name`/`description` frontmatter derived from the command file. The legacy Windsurf/Cascade-era `.devin/workflows/` and `~/.codeium/windsurf/global_workflows/` locations are no longer emitted (stale outputs there stay gitignored but are not cleaned up automatically). Commands import and `--delete` are no-ops for `devin` because the skills feature owns the `.devin/skills/` tree (importing it as commands would double-import every skill). Note that a command and a skill sharing the same name write the same `SKILL.md` path, so keep command and skill names distinct for this target. -> **Agent Skills standard note (`agentsskills`):** Rulesync accepts the legacy rulesync spellings on input but always **emits** the shapes the [specification](https://agentskills.io/specification) requires: `allowed-tools` becomes a space-separated scalar (a YAML list is joined), `compatibility` becomes a string (an object is flattened to `key: value` pairs), and `metadata` values are stringified so the block stays a string→string map. Generation also checks the normative constraints and warns — without failing the run — when `name` is empty, longer than 64 characters, contains anything but lowercase letters, digits and single hyphens, or does not match its parent directory name; when `description` is empty or longer than 1024 characters; or when `compatibility` exceeds 500 characters. These are warnings rather than errors because a conformant client only _skips_ such a skill, and because import stays lenient as the [client-implementation guide](https://agentskills.io/client-implementation/adding-skills-support) advises. Validate the result with the spec's own [`skills-ref validate`](https://github.com/agentskills/agentskills/tree/main/skills-ref). +> **Agent Skills standard note (`agentsskills`):** Rulesync accepts the legacy rulesync spellings on input but always **emits** the shapes the [specification](https://agentskills.io/specification) requires: `allowed-tools` becomes a space-separated scalar (a YAML list is joined), `compatibility` becomes a string (an object is flattened to `key: value` pairs), and `metadata` values are stringified so the block stays a string→string map. Generation also checks the normative constraints and warns — without failing the run — when `name` is empty, longer than 64 characters, contains anything but lowercase letters, digits and single hyphens, or does not match its parent directory name; when `description` is empty or longer than 1024 characters; when `compatibility` exceeds 500 characters; or when an `allowed-tools` list entry contains whitespace, which the space-separated form cannot represent. These are warnings rather than errors because a conformant client only _skips_ such a skill, and because import stays lenient as the [client-implementation guide](https://agentskills.io/client-implementation/adding-skills-support) advises. A value that normalizes to the empty string (`compatibility: {}`, `allowed-tools: []`) is dropped rather than written, since the spec requires `compatibility` to be 1–500 characters when present. On **import**, `allowed-tools` is normalized back to the canonical rulesync list, so a generate → import round trip leaves `.rulesync/skills/**` in the shape it started in (the `compatibility` and `metadata` coercions are one-way, because the legacy object/number forms have no conformant equivalent). `hermesagent` reads the same `agentsskills` block and applies the same normalization, so one rulesync source never produces two different on-disk spellings. Validate the result with the spec's own [`skills-ref validate`](https://github.com/agentskills/agentskills/tree/main/skills-ref). > **Hermes Agent note:** Commands are global-only and remain distinct from skills. Rulesync writes JSON command specs to `~/.hermes/rulesync/commands/.json`, installs the `rulesync-commands` plugin under `~/.hermes/plugins/`, and enables it in `~/.hermes/config.yaml`. The plugin registers each spec with Hermes's [`ctx.register_command()` plugin API](https://hermes-agent.nousresearch.com/docs/user-guide/features/plugins/) and dispatches the prompt through `delegate_task`; invocation arguments are appended to the prompt. `.rulesync/skills//SKILL.md` still generates a full [Hermes Agent Skill](https://hermes-agent.nousresearch.com/docs/user-guide/features/skills/) under `~/.hermes/skills//SKILL.md`, which Hermes also exposes as a dynamic slash command. Rulesync rejects command/skill names that would collide in Hermes's slash-command namespace, and rejects nested command paths that flatten to the same name. Generate commands with `rulesync generate --targets hermesagent --features commands --global`. > diff --git a/src/e2e/e2e-skills.spec.ts b/src/e2e/e2e-skills.spec.ts index 8308238dd..bad1f6437 100644 --- a/src/e2e/e2e-skills.spec.ts +++ b/src/e2e/e2e-skills.spec.ts @@ -198,6 +198,41 @@ This is the test skill body content. }, ); + // The Agent Skills spec types `allowed-tools` as a space-separated string, + // `compatibility` as a string and `metadata` as a string→string map, so the + // legacy rulesync list/object/number spellings must not reach the file. + // https://agentskills.io/specification + it("should write spec-conformant scalar frontmatter for agentsskills", async () => { + const testDir = getTestDir(); + + await writeFileContent( + join(testDir, RULESYNC_SKILLS_RELATIVE_DIR_PATH, "test-skill", "SKILL.md"), + `--- +name: test-skill +description: "A test skill for E2E testing" +targets: ["*"] +agentsskills: + allowed-tools: ["Read", "Bash(git:*)"] + compatibility: + runtime: node + metadata: + version: 1 +--- +This is the test skill body content. +`, + ); + + await runGenerate({ target: "agentsskills", features: "skills" }); + + const generatedContent = await readFileContent( + join(testDir, ".agents", "skills", "test-skill", "SKILL.md"), + ); + expect(generatedContent).toContain("allowed-tools: Read Bash(git:*)"); + expect(generatedContent).toContain("compatibility: 'runtime: node'"); + expect(generatedContent).toContain("version: '1'"); + expect(generatedContent).not.toContain("- Read"); + }); + it.each([ { target: "agentsmd", diff --git a/src/features/skills/agentsskills-skill.test.ts b/src/features/skills/agentsskills-skill.test.ts index 1154cae2f..00c8ed0b1 100644 --- a/src/features/skills/agentsskills-skill.test.ts +++ b/src/features/skills/agentsskills-skill.test.ts @@ -59,7 +59,8 @@ describe("AgentsSkillsSkill", () => { license: "MIT", compatibility: { "agent-skills": ">=1.0.0" }, metadata: { version: "1.2.3" }, - "allowed-tools": "shell", + // Normalized back to the canonical rulesync array on import. + "allowed-tools": ["shell"], }); const roundTripped = AgentsSkillsSkill.fromRulesyncSkill({ rulesyncSkill }); @@ -277,9 +278,15 @@ Body.`; const warnings = logger.warn.mock.calls.map(([message]) => String(message)); expect(warnings).toHaveLength(3); - expect(warnings[0]).toContain("lowercase letters, digits and single hyphens"); - expect(warnings[1]).toContain('must match its parent directory name "My_Bad--Name"'); - expect(warnings[2]).toContain("`description` is required and must not be empty"); + expect(warnings.some((w) => w.includes("lowercase letters, digits and single hyphens"))).toBe( + true, + ); + expect( + warnings.some((w) => w.includes('must match its parent directory name "My_Bad--Name"')), + ).toBe(true); + expect( + warnings.some((w) => w.includes("`description` is required and must not be empty")), + ).toBe(true); for (const warning of warnings) { expect(warning).toContain(join(".agents", "skills", "My_Bad--Name", SKILL_FILE_NAME)); } @@ -308,6 +315,145 @@ Body.`; expect(warnings.some((w) => w.includes("`compatibility` is 501 characters"))).toBe(true); }); + it("should encode a self-referential metadata value instead of throwing", () => { + // YAML anchors let a hand-written SKILL.md produce a genuinely circular + // object, which a plain JSON.stringify would reject. + const circular: Record = { label: "root" }; + circular.self = circular; + + const rulesyncSkill = new RulesyncSkill({ + outputRoot: testDir, + relativeDirPath: RULESYNC_SKILLS_RELATIVE_DIR_PATH, + dirName: "demo-skill", + frontmatter: { + name: "demo-skill", + description: "Demo skill.", + agentsskills: { metadata: { graph: circular } }, + }, + body: "Body", + validate: true, + }); + + const metadata = AgentsSkillsSkill.fromRulesyncSkill({ rulesyncSkill }).getFrontmatter() + .metadata as Record; + expect(metadata.graph).toBe('{"label":"root","self":"[repeated reference]"}'); + }); + + it("should encode each shared metadata node once so aliases cannot blow up the output", () => { + // Without this, N levels of YAML aliases expand exponentially: a few + // hundred bytes of input becomes tens of megabytes of JSON. + const leaf = { value: "x" }; + const shared = { a: leaf, b: leaf, c: leaf }; + + const rulesyncSkill = new RulesyncSkill({ + outputRoot: testDir, + relativeDirPath: RULESYNC_SKILLS_RELATIVE_DIR_PATH, + dirName: "demo-skill", + frontmatter: { + name: "demo-skill", + description: "Demo skill.", + agentsskills: { metadata: { shared } }, + }, + body: "Body", + validate: true, + }); + + const metadata = AgentsSkillsSkill.fromRulesyncSkill({ rulesyncSkill }).getFrontmatter() + .metadata as Record; + expect(metadata.shared).toBe( + '{"a":{"value":"x"},"b":"[repeated reference]","c":"[repeated reference]"}', + ); + }); + + it("should warn when an allowed-tools entry contains whitespace", () => { + const logger = createMockLogger(); + const rulesyncSkill = new RulesyncSkill({ + outputRoot: testDir, + relativeDirPath: RULESYNC_SKILLS_RELATIVE_DIR_PATH, + dirName: "demo-skill", + frontmatter: { + name: "demo-skill", + description: "Demo skill.", + agentsskills: { "allowed-tools": ["Read", "Bash(git status)"] }, + }, + body: "Body", + validate: true, + }); + + const skill = AgentsSkillsSkill.fromRulesyncSkill({ rulesyncSkill, logger }); + + expect(skill.getFrontmatter()["allowed-tools"]).toBe("Read Bash(git status)"); + const warnings = logger.warn.mock.calls.map(([message]) => String(message)); + expect(warnings).toHaveLength(1); + expect(warnings[0]).toContain('"Bash(git status)" contains whitespace'); + }); + + it("should render a YAML timestamp as its ISO form rather than a quoted JSON string", () => { + // js-yaml resolves `released: 2024-01-01` into a Date; JSON-encoding it + // would fold its own quotes into the emitted scalar. + const rulesyncSkill = new RulesyncSkill({ + outputRoot: testDir, + relativeDirPath: RULESYNC_SKILLS_RELATIVE_DIR_PATH, + dirName: "demo-skill", + frontmatter: { + name: "demo-skill", + description: "Demo skill.", + agentsskills: { + metadata: { released: new Date("2024-01-01T00:00:00.000Z"), stable: true }, + }, + }, + body: "Body", + validate: true, + }); + + expect( + AgentsSkillsSkill.fromRulesyncSkill({ rulesyncSkill }).getFrontmatter().metadata, + ).toEqual({ released: "2024-01-01T00:00:00.000Z", stable: "true" }); + }); + + it("should drop values that normalize to the empty string instead of emitting them", () => { + // The spec requires `compatibility` to be 1-500 characters when present, + // and an empty `allowed-tools` says nothing. + const rulesyncSkill = new RulesyncSkill({ + outputRoot: testDir, + relativeDirPath: RULESYNC_SKILLS_RELATIVE_DIR_PATH, + dirName: "demo-skill", + frontmatter: { + name: "demo-skill", + description: "Demo skill.", + agentsskills: { compatibility: {}, "allowed-tools": [] }, + }, + body: "Body", + validate: true, + }); + + expect(AgentsSkillsSkill.fromRulesyncSkill({ rulesyncSkill }).getFrontmatter()).toEqual({ + name: "demo-skill", + description: "Demo skill.", + }); + }); + + it("should warn when an object compatibility exceeds 500 characters only after flattening", () => { + const logger = createMockLogger(); + const rulesyncSkill = new RulesyncSkill({ + outputRoot: testDir, + relativeDirPath: RULESYNC_SKILLS_RELATIVE_DIR_PATH, + dirName: "demo-skill", + frontmatter: { + name: "demo-skill", + description: "Demo skill.", + agentsskills: { compatibility: { runtime: "n".repeat(500) } }, + }, + body: "Body", + validate: true, + }); + + AgentsSkillsSkill.fromRulesyncSkill({ rulesyncSkill, logger }); + + const warnings = logger.warn.mock.calls.map(([message]) => String(message)); + expect(warnings.some((w) => w.includes("`compatibility` is 509 characters"))).toBe(true); + }); + it("should not warn for a fully conformant skill", () => { const logger = createMockLogger(); const rulesyncSkill = new RulesyncSkill({ diff --git a/src/features/skills/agentsskills-skill.ts b/src/features/skills/agentsskills-skill.ts index 76a6bc635..ae1232827 100644 --- a/src/features/skills/agentsskills-skill.ts +++ b/src/features/skills/agentsskills-skill.ts @@ -7,7 +7,13 @@ import { SKILL_FILE_NAME } from "../../constants/general.js"; import { RULESYNC_SKILLS_RELATIVE_DIR_PATH } from "../../constants/rulesync-paths.js"; import { ValidationResult } from "../../types/ai-dir.js"; import { formatError } from "../../utils/error.js"; -import { RulesyncSkill, RulesyncSkillFrontmatterInput, SkillFile } from "./rulesync-skill.js"; +import { type Logger, warnWithFallback } from "../../utils/logger.js"; +import { + RulesyncSkill, + type RulesyncSkillFrontmatter, + RulesyncSkillFrontmatterInput, + SkillFile, +} from "./rulesync-skill.js"; import { ToolSkill, ToolSkillForDeletionParams, @@ -41,17 +47,49 @@ const COMPATIBILITY_MAX_LENGTH = 500; // expressed as alphanumeric runs joined by single hyphens. const NAME_PATTERN = /^[a-z0-9]+(?:-[a-z0-9]+)*$/; +/** + * Placeholder for an object that has already been encoded once in the same + * value. YAML anchors let one document reference the same node repeatedly, and + * js-yaml resolves those into genuinely shared (possibly self-referential) + * objects — so encoding each node at most once is what keeps a hand-written + * `SKILL.md` from making the encoding throw on a cycle or blow up + * exponentially on a chain of aliases. + */ +const REPEATED_REFERENCE_PLACEHOLDER = "[repeated reference]"; + /** * Render a non-string YAML value as the string the spec requires. Scalars use - * their natural text form (`1` → `"1"`), containers are JSON-encoded so the - * original structure stays readable rather than collapsing to `[object Object]`. + * their natural text form (`1` → `"1"`, a YAML timestamp → its ISO form), + * containers are JSON-encoded so the original structure stays readable rather + * than collapsing to `[object Object]`. */ function stringifyValue(value: unknown): string { if (typeof value === "string") { return value; } + // js-yaml resolves a YAML timestamp into a Date, which is an object but not a + // container: JSON-encoding it would wrap its own quotes into the string. + if (value instanceof Date) { + return value.toISOString(); + } if (typeof value === "object" && value !== null) { - return JSON.stringify(value); + const seen = new WeakSet(); + try { + return JSON.stringify(value, (_key, entry: unknown) => { + if (typeof entry !== "object" || entry === null) { + return entry; + } + if (seen.has(entry)) { + return REPEATED_REFERENCE_PLACEHOLDER; + } + seen.add(entry); + return entry; + }); + } catch { + // Values JSON cannot represent at all (e.g. a BigInt) still have to + // become some string rather than aborting the whole generate run. + return String(value); + } } return String(value); } @@ -65,6 +103,16 @@ function toAllowedToolsString(value: string | string[]): string { return Array.isArray(value) ? value.join(" ") : value; } +/** + * Inverse of {@link toAllowedToolsString}: normalize back to the canonical + * rulesync array representation on import, so a generate → import round trip + * leaves `.rulesync/skills/**` in the shape it started in. Mirrors + * `DeepagentsSkill`. + */ +function toAllowedToolsArray(value: string | string[]): string[] { + return Array.isArray(value) ? value : value.split(/\s+/).filter((tool) => tool.length > 0); +} + /** * The spec types `compatibility` as a free-form string. An object from a legacy * rulesync input is flattened to `key: value` pairs instead of being emitted as @@ -89,24 +137,68 @@ function toStringMetadata(metadata: Record): Record; + "allowed-tools"?: string; +}; + +/** + * Convert the rulesync `agentsskills` block into the shapes the specification + * requires. Shared with `HermesagentSkill`, which writes the same fields to its + * own skill location, so one rulesync input can never produce two different + * on-disk spellings. + * + * A value that normalizes to the empty string is dropped rather than emitted: + * the spec requires `compatibility` to be 1–500 characters when present, and an + * empty `allowed-tools` says nothing. + * + * @see https://agentskills.io/specification + */ +export function toSpecConformantAgentSkillFields( + section: RulesyncSkillFrontmatter["agentsskills"] | undefined, +): AgentsSkillsSharedFields { + if (section === undefined) { + return {}; + } + const compatibility = + section.compatibility === undefined ? undefined : toCompatibilityString(section.compatibility); + const allowedTools = + section["allowed-tools"] === undefined + ? undefined + : toAllowedToolsString(section["allowed-tools"]); + + return { + ...(section.license !== undefined && { license: section.license }), + ...(compatibility !== undefined && compatibility.length > 0 && { compatibility }), + ...(section.metadata !== undefined && { metadata: toStringMetadata(section.metadata) }), + ...(allowedTools !== undefined && allowedTools.length > 0 && { "allowed-tools": allowedTools }), + }; +} + /** - * Collect the normative `name` / `description` violations the Agent Skills spec - * defines. These are reported as warnings rather than errors: import stays - * lenient per the spec's client-implementation guide, and failing generation - * outright would break existing skill directories. What must not happen is - * emitting a skill that conformant clients silently skip without saying so. + * Collect the normative violations the Agent Skills spec defines for a skill + * about to be written. These are reported as warnings rather than errors: + * import stays lenient per the spec's client-implementation guide, and failing + * generation outright would break existing skill directories. What must not + * happen is emitting a skill that conformant clients silently skip without + * saying so. * * @see https://agentskills.io/specification * @see https://agentskills.io/client-implementation/adding-skills-support */ -function collectSpecViolations({ +export function collectAgentSkillViolations({ name, description, dirName, + section, }: { name: string; description: string; dirName: string; + section?: RulesyncSkillFrontmatter["agentsskills"]; }): string[] { const violations: string[] = []; @@ -140,6 +232,22 @@ function collectSpecViolations({ ); } + const { compatibility } = toSpecConformantAgentSkillFields(section); + if (compatibility !== undefined && compatibility.length > COMPATIBILITY_MAX_LENGTH) { + violations.push( + `\`compatibility\` is ${compatibility.length} characters; the Agent Skills spec allows at most ${COMPATIBILITY_MAX_LENGTH}`, + ); + } + + const allowedTools = section?.["allowed-tools"]; + if (Array.isArray(allowedTools)) { + for (const tool of allowedTools.filter((entry) => /\s/.test(entry))) { + violations.push( + `\`allowed-tools\` entry "${tool}" contains whitespace, so the space-separated form the Agent Skills spec requires will read it back as several entries`, + ); + } + } + return violations; } @@ -233,13 +341,18 @@ export class AgentsSkillsSkill extends ToolSkill { toRulesyncSkill(): RulesyncSkill { const frontmatter = this.getFrontmatter(); + // `allowed-tools` is normalized back to the canonical rulesync array so a + // generate → import round trip leaves the source frontmatter unchanged. + const allowedTools = + frontmatter["allowed-tools"] === undefined + ? undefined + : toAllowedToolsArray(frontmatter["allowed-tools"]); const agentsskillsSection = { ...(frontmatter.license !== undefined && { license: frontmatter.license }), ...(frontmatter.compatibility !== undefined && { compatibility: frontmatter.compatibility }), ...(frontmatter.metadata !== undefined && { metadata: frontmatter.metadata }), - ...(frontmatter["allowed-tools"] !== undefined && { - "allowed-tools": frontmatter["allowed-tools"], - }), + ...(allowedTools !== undefined && + allowedTools.length > 0 && { "allowed-tools": allowedTools }), }; const rulesyncFrontmatter: RulesyncSkillFrontmatterInput = { name: frontmatter.name, @@ -269,45 +382,25 @@ export class AgentsSkillsSkill extends ToolSkill { }: ToolSkillFromRulesyncSkillParams): AgentsSkillsSkill { const settablePaths = AgentsSkillsSkill.getSettablePaths({ global }); const rulesyncFrontmatter = rulesyncSkill.getFrontmatter(); - const agentsskillsSection = rulesyncFrontmatter.agentsskills; const dirName = rulesyncSkill.getDirName(); - const skillPath = join(settablePaths.relativeDirPath, dirName, SKILL_FILE_NAME); - - const compatibility = - agentsskillsSection?.compatibility === undefined - ? undefined - : toCompatibilityString(agentsskillsSection.compatibility); - if (compatibility !== undefined && compatibility.length > COMPATIBILITY_MAX_LENGTH) { - logger?.warn( - `${skillPath}: \`compatibility\` is ${compatibility.length} characters; the Agent Skills spec allows at most ${COMPATIBILITY_MAX_LENGTH}`, - ); - } const agentsSkillsFrontmatter: AgentsSkillsSkillFrontmatter = { name: rulesyncFrontmatter.name, description: rulesyncFrontmatter.description, - ...(agentsskillsSection?.license !== undefined && { license: agentsskillsSection.license }), - ...(compatibility !== undefined && { compatibility }), - ...(agentsskillsSection?.metadata !== undefined && { - metadata: toStringMetadata(agentsskillsSection.metadata), - }), - ...(agentsskillsSection?.["allowed-tools"] !== undefined && { - "allowed-tools": toAllowedToolsString(agentsskillsSection["allowed-tools"]), - }), + ...toSpecConformantAgentSkillFields(rulesyncFrontmatter.agentsskills), }; - for (const violation of collectSpecViolations({ - name: rulesyncFrontmatter.name, - description: rulesyncFrontmatter.description, + AgentsSkillsSkill.reportSpecViolations({ + relativeDirPath: settablePaths.relativeDirPath, dirName, - })) { - logger?.warn(`${skillPath}: ${violation}`); - } + rulesyncFrontmatter, + logger, + }); return new this({ outputRoot, relativeDirPath: settablePaths.relativeDirPath, - dirName: rulesyncSkill.getDirName(), + dirName, frontmatter: agentsSkillsFrontmatter, body: rulesyncSkill.getBody(), otherFiles: rulesyncSkill.getOtherFiles(), @@ -316,6 +409,33 @@ export class AgentsSkillsSkill extends ToolSkill { }); } + /** + * Warn about every Agent Skills spec violation in the skill about to be + * written. Shared with `HermesagentSkill` so both locations report the same + * diagnostics for the same rulesync source. + */ + protected static reportSpecViolations({ + relativeDirPath, + dirName, + rulesyncFrontmatter, + logger, + }: { + relativeDirPath: string; + dirName: string; + rulesyncFrontmatter: RulesyncSkillFrontmatter; + logger?: Logger; + }): void { + const skillPath = join(relativeDirPath, dirName, SKILL_FILE_NAME); + for (const violation of collectAgentSkillViolations({ + name: rulesyncFrontmatter.name, + description: rulesyncFrontmatter.description, + dirName, + section: rulesyncFrontmatter.agentsskills, + })) { + warnWithFallback(logger, `${skillPath}: ${violation}`); + } + } + static isTargetedByRulesyncSkill(rulesyncSkill: RulesyncSkill): boolean { const targets = rulesyncSkill.getFrontmatter().targets; return targets.includes("*") || targets.includes("agentsskills"); diff --git a/src/features/skills/hermesagent-skill.test.ts b/src/features/skills/hermesagent-skill.test.ts index c7e8d68b3..c3a66f02c 100644 --- a/src/features/skills/hermesagent-skill.test.ts +++ b/src/features/skills/hermesagent-skill.test.ts @@ -95,7 +95,9 @@ describe("HermesagentSkill", () => { name: "Test Skill", description: "Test skill description", license: "MIT", - "allowed-tools": ["terminal"], + // Normalized to the Agent Skills space-separated form, exactly as the + // native `agentsskills` target writes it from the same rulesync input. + "allowed-tools": "terminal", version: "1.2.3", author: "Rulesync", platforms: ["darwin", "linux"], diff --git a/src/features/skills/hermesagent-skill.ts b/src/features/skills/hermesagent-skill.ts index d464ab24a..8633a4ef9 100644 --- a/src/features/skills/hermesagent-skill.ts +++ b/src/features/skills/hermesagent-skill.ts @@ -1,6 +1,10 @@ import { HERMESAGENT_SKILLS_DIR_PATH } from "../../constants/hermesagent-paths.js"; import { RULESYNC_SKILLS_RELATIVE_DIR_PATH } from "../../constants/rulesync-paths.js"; -import { AgentsSkillsSkill, type AgentsSkillsSkillParams } from "./agentsskills-skill.js"; +import { + AgentsSkillsSkill, + type AgentsSkillsSkillParams, + toSpecConformantAgentSkillFields, +} from "./agentsskills-skill.js"; import { RulesyncSkill, type RulesyncSkillFrontmatterInput } from "./rulesync-skill.js"; import type { ToolSkillForDeletionParams, @@ -36,11 +40,22 @@ export class HermesagentSkill extends AgentsSkillsSkill { rulesyncSkill, validate = true, global = false, + logger, }: ToolSkillFromRulesyncSkillParams): HermesagentSkill { const rulesyncFrontmatter = rulesyncSkill.getFrontmatter(); - const shared = rulesyncFrontmatter.agentsskills ?? {}; + // The `agentsskills` block is the same rulesync source the native Agent + // Skills target reads, so it goes through the same normalization: one input + // must not produce two different on-disk spellings. + const shared = toSpecConformantAgentSkillFields(rulesyncFrontmatter.agentsskills); const hermes = rulesyncFrontmatter.hermesagent ?? {}; + HermesagentSkill.reportSpecViolations({ + relativeDirPath: HERMESAGENT_SKILLS_DIR_PATH, + dirName: rulesyncSkill.getDirName(), + rulesyncFrontmatter, + logger, + }); + return new this({ outputRoot, relativeDirPath: HERMESAGENT_SKILLS_DIR_PATH, diff --git a/src/features/skills/skills-processor.test.ts b/src/features/skills/skills-processor.test.ts index 4533377b4..83c66d5ae 100644 --- a/src/features/skills/skills-processor.test.ts +++ b/src/features/skills/skills-processor.test.ts @@ -118,6 +118,34 @@ describe("SkillsProcessor", () => { expect(claudecodeSkill.getFrontmatter().description).toBe("Test skill description"); }); + it("should pass its logger to the tool skill so spec diagnostics reach the user", async () => { + const logger = createMockLogger(); + const agentsSkillsProcessor = new SkillsProcessor({ + logger, + outputRoot: testDir, + toolTarget: "agentsskills", + }); + const rulesyncSkill = new RulesyncSkill({ + outputRoot: testDir, + relativeDirPath: RULESYNC_SKILLS_RELATIVE_DIR_PATH, + dirName: "My_Bad--Name", + frontmatter: { + name: "My_Bad--Name", + description: "Test skill description", + }, + body: "Test skill content", + validate: false, + }); + + await agentsSkillsProcessor.convertRulesyncDirsToToolDirs([rulesyncSkill]); + + expect( + logger.warn.mock.calls.some(([message]) => + String(message).includes("lowercase letters, digits and single hyphens"), + ), + ).toBe(true); + }); + it("should filter out non-RulesyncSkill instances", async () => { const rulesyncSkill = new RulesyncSkill({ outputRoot: testDir, From 8cc69282ebc017620b5ae7509aa762f640d49deb Mon Sep 17 00:00:00 2001 From: dyoshikawa Date: Mon, 27 Jul 2026 00:41:36 -0700 Subject: [PATCH 3/4] fix(agentsskills): finish the hermesagent round trip and check the emitted frontmatter MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Second round of review feedback. - `HermesagentSkill.toRulesyncSkill` fully overrides the base method and was still returning `allowed-tools` verbatim, so a hermesagent generate → import round trip rewrote the rulesync source from a list to a string — the same bug the previous commit fixed on the base class. It now uses the shared `toAllowedToolsArray`. - Diagnostics run against the frontmatter actually being written rather than the rulesync source, so a `hermesagent:` override that reintroduces a YAML list or mapping is reported instead of slipping past. Two new violation messages cover those shapes. Tool-specific overrides still win, per the frontmatter precedence rule — they are now visible, not silently rewritten. - Hermes keeps structured `metadata`. Hermes resolves `metadata.hermes.*` (`requires_toolsets`, `tags`, …) as YAML, so applying the Agent Skills string→string coercion there broke working configurations. The coercion is now opt-out via `toSpecConformantAgentSkillFields`, and only the standard's own target opts in. - Warned paths include `outputRoot`, so a global-only skill points at the file under the home directory instead of a same-named project path that does not exist. - `collectAgentSkillViolations` and `AgentsSkillsSharedFields` are module-private again; only the two helpers Hermes needs are exported. Docs updated for the hermesagent round trip, the structured-metadata exemption and the override behavior. Co-Authored-By: Claude Opus 5 (1M context) --- docs/reference/file-formats.md | 4 +- skills/rulesync/file-formats.md | 4 +- src/features/skills/agentsskills-skill.ts | 75 +++++++++++++------ src/features/skills/hermesagent-skill.test.ts | 65 ++++++++++++++++ src/features/skills/hermesagent-skill.ts | 42 +++++++---- 5 files changed, 148 insertions(+), 42 deletions(-) diff --git a/docs/reference/file-formats.md b/docs/reference/file-formats.md index 26e680b7b..5b434143a 100644 --- a/docs/reference/file-formats.md +++ b/docs/reference/file-formats.md @@ -324,7 +324,7 @@ The command body itself uses a Claude Code-compatible **universal syntax** (e.g. > **Devin note:** Devin's extensibility docs no longer document a standalone workflows/commands component — reusable prompts invoked as slash commands are [Skills](https://docs.devin.ai/cli/extensibility/skills/overview) (`/name`). Rulesync therefore emits each command onto the native skills surface as `.devin/skills//SKILL.md` (project) / `~/.config/devin/skills//SKILL.md` (global, via `--global`), with `name`/`description` frontmatter derived from the command file. The legacy Windsurf/Cascade-era `.devin/workflows/` and `~/.codeium/windsurf/global_workflows/` locations are no longer emitted (stale outputs there stay gitignored but are not cleaned up automatically). Commands import and `--delete` are no-ops for `devin` because the skills feature owns the `.devin/skills/` tree (importing it as commands would double-import every skill). Note that a command and a skill sharing the same name write the same `SKILL.md` path, so keep command and skill names distinct for this target. -> **Agent Skills standard note (`agentsskills`):** Rulesync accepts the legacy rulesync spellings on input but always **emits** the shapes the [specification](https://agentskills.io/specification) requires: `allowed-tools` becomes a space-separated scalar (a YAML list is joined), `compatibility` becomes a string (an object is flattened to `key: value` pairs), and `metadata` values are stringified so the block stays a string→string map. Generation also checks the normative constraints and warns — without failing the run — when `name` is empty, longer than 64 characters, contains anything but lowercase letters, digits and single hyphens, or does not match its parent directory name; when `description` is empty or longer than 1024 characters; when `compatibility` exceeds 500 characters; or when an `allowed-tools` list entry contains whitespace, which the space-separated form cannot represent. These are warnings rather than errors because a conformant client only _skips_ such a skill, and because import stays lenient as the [client-implementation guide](https://agentskills.io/client-implementation/adding-skills-support) advises. A value that normalizes to the empty string (`compatibility: {}`, `allowed-tools: []`) is dropped rather than written, since the spec requires `compatibility` to be 1–500 characters when present. On **import**, `allowed-tools` is normalized back to the canonical rulesync list, so a generate → import round trip leaves `.rulesync/skills/**` in the shape it started in (the `compatibility` and `metadata` coercions are one-way, because the legacy object/number forms have no conformant equivalent). `hermesagent` reads the same `agentsskills` block and applies the same normalization, so one rulesync source never produces two different on-disk spellings. Validate the result with the spec's own [`skills-ref validate`](https://github.com/agentskills/agentskills/tree/main/skills-ref). +> **Agent Skills standard note (`agentsskills`):** Rulesync accepts the legacy rulesync spellings on input but always **emits** the shapes the [specification](https://agentskills.io/specification) requires: `allowed-tools` becomes a space-separated scalar (a YAML list is joined), `compatibility` becomes a string (an object is flattened to `key: value` pairs), and `metadata` values are stringified so the block stays a string→string map. Generation also checks the normative constraints and warns — without failing the run — when `name` is empty, longer than 64 characters, contains anything but lowercase letters, digits and single hyphens, or does not match its parent directory name; when `description` is empty or longer than 1024 characters; when `compatibility` exceeds 500 characters; or when an `allowed-tools` list entry contains whitespace, which the space-separated form cannot represent. These are warnings rather than errors because a conformant client only _skips_ such a skill, and because import stays lenient as the [client-implementation guide](https://agentskills.io/client-implementation/adding-skills-support) advises. A value that normalizes to the empty string (`compatibility: {}`, `allowed-tools: []`) is dropped rather than written, since the spec requires `compatibility` to be 1–500 characters when present. On **import**, `allowed-tools` is normalized back to the canonical rulesync list, so a generate → import round trip leaves `.rulesync/skills/**` in the shape it started in (the `compatibility` and `metadata` coercions are one-way, because the legacy object/number forms have no conformant equivalent). `hermesagent` reads the same `agentsskills` block and applies the same normalization in both directions, so one rulesync source never produces two different on-disk spellings — except for `metadata`, which stays structured there because Hermes reads `metadata.hermes.*` as YAML. A `hermesagent:` override still wins over the shared block (as for every tool-specific section), so a list or mapping written there is emitted as-is and reported as a spec violation rather than rewritten. Validate the result with the spec's own [`skills-ref validate`](https://github.com/agentskills/agentskills/tree/main/skills-ref). > **Hermes Agent note:** Commands are global-only and remain distinct from skills. Rulesync writes JSON command specs to `~/.hermes/rulesync/commands/.json`, installs the `rulesync-commands` plugin under `~/.hermes/plugins/`, and enables it in `~/.hermes/config.yaml`. The plugin registers each spec with Hermes's [`ctx.register_command()` plugin API](https://hermes-agent.nousresearch.com/docs/user-guide/features/plugins/) and dispatches the prompt through `delegate_task`; invocation arguments are appended to the prompt. `.rulesync/skills//SKILL.md` still generates a full [Hermes Agent Skill](https://hermes-agent.nousresearch.com/docs/user-guide/features/skills/) under `~/.hermes/skills//SKILL.md`, which Hermes also exposes as a dynamic slash command. Rulesync rejects command/skill names that would collide in Hermes's slash-command namespace, and rejects nested command paths that flatten to the same name. Generate commands with `rulesync generate --targets hermesagent --features commands --global`. > @@ -680,7 +680,7 @@ When `claudecode.scheduled-task: true` is set, that skill is emitted only as a C > **Reasonix note:** Reasonix discovers Anthropic-style directory-layout skills (`/SKILL.md`) under `.reasonix/skills/` (project) / `~/.reasonix/skills/` (global, via `--global`). Rulesync emits the portable `name`/`description` frontmatter (Reasonix supports additional optional keys, but only that pair is modeled); the schema is loose, so any extra keys on an imported `SKILL.md` survive the round-trip. See the [Reasonix GUIDE](https://github.com/esengine/DeepSeek-Reasonix/blob/main-v2/docs/GUIDE.md). -> **Hermes Agent note:** Hermes skills are global-only under `~/.hermes/skills//SKILL.md`. Standard Agent Skills fields (`license`, `compatibility`, and `allowed-tools`) round-trip through `agentsskills`; Hermes-native fields such as `version`, `author`, `platforms`, `environments`, `required_environment_variables`, `required_credential_files`, and `metadata.hermes` round-trip through `hermesagent`. Canonical `name` and `description` always own those two frontmatter keys. +> **Hermes Agent note:** Hermes skills are global-only under `~/.hermes/skills//SKILL.md`. Standard Agent Skills fields (`license`, `compatibility`, and `allowed-tools`) round-trip through `agentsskills` and are normalized to the Agent Skills spec shapes described above (so `allowed-tools` is written and imported the same way as for `agentsskills`); Hermes-native fields such as `version`, `author`, `platforms`, `environments`, `required_environment_variables`, `required_credential_files`, and `metadata.hermes` round-trip through `hermesagent`. Canonical `name` and `description` always own those two frontmatter keys. > **Kimi Code note:** Kimi Code discovers skills under `.kimi-code/skills/` (project) and `~/.kimi-code/skills/` (global), plus the shared `.agents/skills/` root at either scope. Rulesync generates the recommended directory layout (`/SKILL.md`) and imports both that layout and flat `.md` skills; for flat files, a missing `name` comes from the filename and a missing `description` falls back to the first non-empty body line (up to 240 characters), matching Kimi. Imported skills are written to `.rulesync/skills//SKILL.md`, using the normalized logical frontmatter name rather than the source directory or filename. Duplicate precedence follows Kimi's case-insensitive logical frontmatter `name`: the Kimi-specific root takes precedence over `.agents/skills/`, and a directory skill takes precedence over a same-named flat file within one root. Shared roots are import-only and are never removed by Kimi-target orphan deletion. Besides `name`/`description`, Rulesync maps Kimi's `type`, `whenToUse`, `disableModelInvocation`, and `arguments` frontmatter through the `kimi-code:` block and preserves supporting files beside directory-layout `SKILL.md`. The shared top-level `disable-model-invocation` value supplies the Kimi flag unless the tool-specific block overrides it. See the [Kimi Code Agent Skills docs](https://moonshotai.github.io/kimi-code/en/customization/skills.html). diff --git a/skills/rulesync/file-formats.md b/skills/rulesync/file-formats.md index 131f319d4..39a4af909 100644 --- a/skills/rulesync/file-formats.md +++ b/skills/rulesync/file-formats.md @@ -324,7 +324,7 @@ The command body itself uses a Claude Code-compatible **universal syntax** (e.g. > **Devin note:** Devin's extensibility docs no longer document a standalone workflows/commands component — reusable prompts invoked as slash commands are [Skills](https://docs.devin.ai/cli/extensibility/skills/overview) (`/name`). Rulesync therefore emits each command onto the native skills surface as `.devin/skills//SKILL.md` (project) / `~/.config/devin/skills//SKILL.md` (global, via `--global`), with `name`/`description` frontmatter derived from the command file. The legacy Windsurf/Cascade-era `.devin/workflows/` and `~/.codeium/windsurf/global_workflows/` locations are no longer emitted (stale outputs there stay gitignored but are not cleaned up automatically). Commands import and `--delete` are no-ops for `devin` because the skills feature owns the `.devin/skills/` tree (importing it as commands would double-import every skill). Note that a command and a skill sharing the same name write the same `SKILL.md` path, so keep command and skill names distinct for this target. -> **Agent Skills standard note (`agentsskills`):** Rulesync accepts the legacy rulesync spellings on input but always **emits** the shapes the [specification](https://agentskills.io/specification) requires: `allowed-tools` becomes a space-separated scalar (a YAML list is joined), `compatibility` becomes a string (an object is flattened to `key: value` pairs), and `metadata` values are stringified so the block stays a string→string map. Generation also checks the normative constraints and warns — without failing the run — when `name` is empty, longer than 64 characters, contains anything but lowercase letters, digits and single hyphens, or does not match its parent directory name; when `description` is empty or longer than 1024 characters; when `compatibility` exceeds 500 characters; or when an `allowed-tools` list entry contains whitespace, which the space-separated form cannot represent. These are warnings rather than errors because a conformant client only _skips_ such a skill, and because import stays lenient as the [client-implementation guide](https://agentskills.io/client-implementation/adding-skills-support) advises. A value that normalizes to the empty string (`compatibility: {}`, `allowed-tools: []`) is dropped rather than written, since the spec requires `compatibility` to be 1–500 characters when present. On **import**, `allowed-tools` is normalized back to the canonical rulesync list, so a generate → import round trip leaves `.rulesync/skills/**` in the shape it started in (the `compatibility` and `metadata` coercions are one-way, because the legacy object/number forms have no conformant equivalent). `hermesagent` reads the same `agentsskills` block and applies the same normalization, so one rulesync source never produces two different on-disk spellings. Validate the result with the spec's own [`skills-ref validate`](https://github.com/agentskills/agentskills/tree/main/skills-ref). +> **Agent Skills standard note (`agentsskills`):** Rulesync accepts the legacy rulesync spellings on input but always **emits** the shapes the [specification](https://agentskills.io/specification) requires: `allowed-tools` becomes a space-separated scalar (a YAML list is joined), `compatibility` becomes a string (an object is flattened to `key: value` pairs), and `metadata` values are stringified so the block stays a string→string map. Generation also checks the normative constraints and warns — without failing the run — when `name` is empty, longer than 64 characters, contains anything but lowercase letters, digits and single hyphens, or does not match its parent directory name; when `description` is empty or longer than 1024 characters; when `compatibility` exceeds 500 characters; or when an `allowed-tools` list entry contains whitespace, which the space-separated form cannot represent. These are warnings rather than errors because a conformant client only _skips_ such a skill, and because import stays lenient as the [client-implementation guide](https://agentskills.io/client-implementation/adding-skills-support) advises. A value that normalizes to the empty string (`compatibility: {}`, `allowed-tools: []`) is dropped rather than written, since the spec requires `compatibility` to be 1–500 characters when present. On **import**, `allowed-tools` is normalized back to the canonical rulesync list, so a generate → import round trip leaves `.rulesync/skills/**` in the shape it started in (the `compatibility` and `metadata` coercions are one-way, because the legacy object/number forms have no conformant equivalent). `hermesagent` reads the same `agentsskills` block and applies the same normalization in both directions, so one rulesync source never produces two different on-disk spellings — except for `metadata`, which stays structured there because Hermes reads `metadata.hermes.*` as YAML. A `hermesagent:` override still wins over the shared block (as for every tool-specific section), so a list or mapping written there is emitted as-is and reported as a spec violation rather than rewritten. Validate the result with the spec's own [`skills-ref validate`](https://github.com/agentskills/agentskills/tree/main/skills-ref). > **Hermes Agent note:** Commands are global-only and remain distinct from skills. Rulesync writes JSON command specs to `~/.hermes/rulesync/commands/.json`, installs the `rulesync-commands` plugin under `~/.hermes/plugins/`, and enables it in `~/.hermes/config.yaml`. The plugin registers each spec with Hermes's [`ctx.register_command()` plugin API](https://hermes-agent.nousresearch.com/docs/user-guide/features/plugins/) and dispatches the prompt through `delegate_task`; invocation arguments are appended to the prompt. `.rulesync/skills//SKILL.md` still generates a full [Hermes Agent Skill](https://hermes-agent.nousresearch.com/docs/user-guide/features/skills/) under `~/.hermes/skills//SKILL.md`, which Hermes also exposes as a dynamic slash command. Rulesync rejects command/skill names that would collide in Hermes's slash-command namespace, and rejects nested command paths that flatten to the same name. Generate commands with `rulesync generate --targets hermesagent --features commands --global`. > @@ -680,7 +680,7 @@ When `claudecode.scheduled-task: true` is set, that skill is emitted only as a C > **Reasonix note:** Reasonix discovers Anthropic-style directory-layout skills (`/SKILL.md`) under `.reasonix/skills/` (project) / `~/.reasonix/skills/` (global, via `--global`). Rulesync emits the portable `name`/`description` frontmatter (Reasonix supports additional optional keys, but only that pair is modeled); the schema is loose, so any extra keys on an imported `SKILL.md` survive the round-trip. See the [Reasonix GUIDE](https://github.com/esengine/DeepSeek-Reasonix/blob/main-v2/docs/GUIDE.md). -> **Hermes Agent note:** Hermes skills are global-only under `~/.hermes/skills//SKILL.md`. Standard Agent Skills fields (`license`, `compatibility`, and `allowed-tools`) round-trip through `agentsskills`; Hermes-native fields such as `version`, `author`, `platforms`, `environments`, `required_environment_variables`, `required_credential_files`, and `metadata.hermes` round-trip through `hermesagent`. Canonical `name` and `description` always own those two frontmatter keys. +> **Hermes Agent note:** Hermes skills are global-only under `~/.hermes/skills//SKILL.md`. Standard Agent Skills fields (`license`, `compatibility`, and `allowed-tools`) round-trip through `agentsskills` and are normalized to the Agent Skills spec shapes described above (so `allowed-tools` is written and imported the same way as for `agentsskills`); Hermes-native fields such as `version`, `author`, `platforms`, `environments`, `required_environment_variables`, `required_credential_files`, and `metadata.hermes` round-trip through `hermesagent`. Canonical `name` and `description` always own those two frontmatter keys. > **Kimi Code note:** Kimi Code discovers skills under `.kimi-code/skills/` (project) and `~/.kimi-code/skills/` (global), plus the shared `.agents/skills/` root at either scope. Rulesync generates the recommended directory layout (`/SKILL.md`) and imports both that layout and flat `.md` skills; for flat files, a missing `name` comes from the filename and a missing `description` falls back to the first non-empty body line (up to 240 characters), matching Kimi. Imported skills are written to `.rulesync/skills//SKILL.md`, using the normalized logical frontmatter name rather than the source directory or filename. Duplicate precedence follows Kimi's case-insensitive logical frontmatter `name`: the Kimi-specific root takes precedence over `.agents/skills/`, and a directory skill takes precedence over a same-named flat file within one root. Shared roots are import-only and are never removed by Kimi-target orphan deletion. Besides `name`/`description`, Rulesync maps Kimi's `type`, `whenToUse`, `disableModelInvocation`, and `arguments` frontmatter through the `kimi-code:` block and preserves supporting files beside directory-layout `SKILL.md`. The shared top-level `disable-model-invocation` value supplies the Kimi flag unless the tool-specific block overrides it. See the [Kimi Code Agent Skills docs](https://moonshotai.github.io/kimi-code/en/customization/skills.html). diff --git a/src/features/skills/agentsskills-skill.ts b/src/features/skills/agentsskills-skill.ts index ae1232827..3fe7b6d4a 100644 --- a/src/features/skills/agentsskills-skill.ts +++ b/src/features/skills/agentsskills-skill.ts @@ -109,7 +109,7 @@ function toAllowedToolsString(value: string | string[]): string { * leaves `.rulesync/skills/**` in the shape it started in. Mirrors * `DeepagentsSkill`. */ -function toAllowedToolsArray(value: string | string[]): string[] { +export function toAllowedToolsArray(value: string | string[]): string[] { return Array.isArray(value) ? value : value.split(/\s+/).filter((tool) => tool.length > 0); } @@ -138,10 +138,10 @@ function toStringMetadata(metadata: Record): Record; + metadata?: Record | Record; "allowed-tools"?: string; }; @@ -155,10 +155,16 @@ export type AgentsSkillsSharedFields = { * the spec requires `compatibility` to be 1–500 characters when present, and an * empty `allowed-tools` says nothing. * + * `coerceMetadata` exists for Hermes Agent, which reads structured values under + * `metadata.hermes` (`requires_toolsets`, `tags`, …). The spec's string→string + * rule governs the standard's own surface, not a tool that merely borrows the + * SKILL.md layout, so converting values to strings there would break working configurations. + * * @see https://agentskills.io/specification */ export function toSpecConformantAgentSkillFields( section: RulesyncSkillFrontmatter["agentsskills"] | undefined, + { coerceMetadata = true }: { coerceMetadata?: boolean } = {}, ): AgentsSkillsSharedFields { if (section === undefined) { return {}; @@ -173,7 +179,9 @@ export function toSpecConformantAgentSkillFields( return { ...(section.license !== undefined && { license: section.license }), ...(compatibility !== undefined && compatibility.length > 0 && { compatibility }), - ...(section.metadata !== undefined && { metadata: toStringMetadata(section.metadata) }), + ...(section.metadata !== undefined && { + metadata: coerceMetadata ? toStringMetadata(section.metadata) : section.metadata, + }), ...(allowedTools !== undefined && allowedTools.length > 0 && { "allowed-tools": allowedTools }), }; } @@ -186,21 +194,26 @@ export function toSpecConformantAgentSkillFields( * happen is emitting a skill that conformant clients silently skip without * saying so. * + * The checks run against `frontmatter` — the values actually being written — + * rather than the rulesync source, so a tool-specific override that reintroduces + * a non-conformant shape is caught too. `sourceAllowedTools` is the pre-join + * rulesync value, needed only because the whitespace problem is invisible once + * the entries have been joined. + * * @see https://agentskills.io/specification * @see https://agentskills.io/client-implementation/adding-skills-support */ -export function collectAgentSkillViolations({ - name, - description, +function collectAgentSkillViolations({ + frontmatter, dirName, - section, + sourceAllowedTools, }: { - name: string; - description: string; + frontmatter: AgentsSkillsSkillFrontmatter; dirName: string; - section?: RulesyncSkillFrontmatter["agentsskills"]; + sourceAllowedTools?: string | string[]; }): string[] { const violations: string[] = []; + const { name, description } = frontmatter; if (name.length === 0) { violations.push("`name` is required and must not be empty"); @@ -232,16 +245,23 @@ export function collectAgentSkillViolations({ ); } - const { compatibility } = toSpecConformantAgentSkillFields(section); - if (compatibility !== undefined && compatibility.length > COMPATIBILITY_MAX_LENGTH) { + const { compatibility } = frontmatter; + if (typeof compatibility !== "string" && compatibility !== undefined) { + violations.push( + "`compatibility` must be a string; the Agent Skills spec does not allow a mapping here", + ); + } else if (compatibility !== undefined && compatibility.length > COMPATIBILITY_MAX_LENGTH) { violations.push( `\`compatibility\` is ${compatibility.length} characters; the Agent Skills spec allows at most ${COMPATIBILITY_MAX_LENGTH}`, ); } - const allowedTools = section?.["allowed-tools"]; - if (Array.isArray(allowedTools)) { - for (const tool of allowedTools.filter((entry) => /\s/.test(entry))) { + if (Array.isArray(frontmatter["allowed-tools"])) { + violations.push( + "`allowed-tools` must be a space-separated string; the Agent Skills spec does not allow a list here", + ); + } else if (Array.isArray(sourceAllowedTools)) { + for (const tool of sourceAllowedTools.filter((entry) => /\s/.test(entry))) { violations.push( `\`allowed-tools\` entry "${tool}" contains whitespace, so the space-separated form the Agent Skills spec requires will read it back as several entries`, ); @@ -391,9 +411,11 @@ export class AgentsSkillsSkill extends ToolSkill { }; AgentsSkillsSkill.reportSpecViolations({ + outputRoot, relativeDirPath: settablePaths.relativeDirPath, dirName, - rulesyncFrontmatter, + frontmatter: agentsSkillsFrontmatter, + sourceAllowedTools: rulesyncFrontmatter.agentsskills?.["allowed-tools"], logger, }); @@ -412,25 +434,30 @@ export class AgentsSkillsSkill extends ToolSkill { /** * Warn about every Agent Skills spec violation in the skill about to be * written. Shared with `HermesagentSkill` so both locations report the same - * diagnostics for the same rulesync source. + * diagnostics. The reported path includes `outputRoot` so a global-scope + * skill points at the file that actually gets written under the home + * directory rather than a same-named project path. */ protected static reportSpecViolations({ + outputRoot, relativeDirPath, dirName, - rulesyncFrontmatter, + frontmatter, + sourceAllowedTools, logger, }: { + outputRoot: string; relativeDirPath: string; dirName: string; - rulesyncFrontmatter: RulesyncSkillFrontmatter; + frontmatter: AgentsSkillsSkillFrontmatter; + sourceAllowedTools?: string | string[]; logger?: Logger; }): void { - const skillPath = join(relativeDirPath, dirName, SKILL_FILE_NAME); + const skillPath = join(outputRoot, relativeDirPath, dirName, SKILL_FILE_NAME); for (const violation of collectAgentSkillViolations({ - name: rulesyncFrontmatter.name, - description: rulesyncFrontmatter.description, + frontmatter, dirName, - section: rulesyncFrontmatter.agentsskills, + sourceAllowedTools, })) { warnWithFallback(logger, `${skillPath}: ${violation}`); } diff --git a/src/features/skills/hermesagent-skill.test.ts b/src/features/skills/hermesagent-skill.test.ts index c3a66f02c..774de8d64 100644 --- a/src/features/skills/hermesagent-skill.test.ts +++ b/src/features/skills/hermesagent-skill.test.ts @@ -2,6 +2,7 @@ import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import { HERMESAGENT_SKILLS_DIR_PATH } from "../../constants/hermesagent-paths.js"; import { RULESYNC_SKILLS_RELATIVE_DIR_PATH } from "../../constants/rulesync-paths.js"; +import { createMockLogger } from "../../test-utils/mock-logger.js"; import { setupTestDirectory } from "../../test-utils/test-directories.js"; import { HermesagentSkill } from "./hermesagent-skill.js"; import { RulesyncSkill } from "./rulesync-skill.js"; @@ -54,6 +55,51 @@ describe("HermesagentSkill", () => { }); describe("fromRulesyncSkill", () => { + it("should keep structured metadata that Hermes reads natively", () => { + // Hermes resolves `metadata.hermes.*` as structured YAML, so the Agent + // Skills string-map coercion must not apply on this target. + const rulesyncSkill = new RulesyncSkill({ + outputRoot: testDir, + relativeDirPath: RULESYNC_SKILLS_RELATIVE_DIR_PATH, + dirName: "test-skill", + frontmatter: { + name: "test-skill", + description: "Test skill description", + agentsskills: { metadata: { hermes: { requires_toolsets: ["terminal"] } } }, + }, + body: "Test body content", + validate: true, + }); + + expect( + HermesagentSkill.fromRulesyncSkill({ rulesyncSkill }).getFrontmatter().metadata, + ).toEqual({ hermes: { requires_toolsets: ["terminal"] } }); + }); + + it("should warn when a hermesagent override reintroduces a non-conformant shape", () => { + const logger = createMockLogger(); + const rulesyncSkill = new RulesyncSkill({ + outputRoot: testDir, + relativeDirPath: RULESYNC_SKILLS_RELATIVE_DIR_PATH, + dirName: "test-skill", + frontmatter: { + name: "test-skill", + description: "Test skill description", + agentsskills: { "allowed-tools": ["Read"], compatibility: "fine" }, + hermesagent: { "allowed-tools": ["Read", "Write"], compatibility: { a: "b" } }, + }, + body: "Test body content", + validate: true, + }); + + HermesagentSkill.fromRulesyncSkill({ rulesyncSkill, logger }); + + const warnings = logger.warn.mock.calls.map(([message]) => String(message)); + expect( + warnings.some((w) => w.includes("`allowed-tools` must be a space-separated string")), + ).toBe(true); + expect(warnings.some((w) => w.includes("`compatibility` must be a string"))).toBe(true); + }); it("should create an instance routed to the Hermes skills directory", () => { const rulesyncSkill = new RulesyncSkill({ outputRoot: testDir, @@ -115,6 +161,25 @@ describe("HermesagentSkill", () => { }); describe("toRulesyncSkill", () => { + it("should normalize a space-separated allowed-tools scalar back to the canonical array", () => { + // Generation now writes the spec's scalar form, so import has to reverse + // it or a generate → import round trip rewrites the rulesync source. + const skill = new HermesagentSkill({ + outputRoot: testDir, + dirName: "test-skill", + frontmatter: { + name: "test-skill", + description: "Test description", + "allowed-tools": "Read Write", + }, + body: "Test body", + validate: true, + }); + + expect(skill.toRulesyncSkill().getFrontmatter().agentsskills).toEqual({ + "allowed-tools": ["Read", "Write"], + }); + }); it("should convert back to a RulesyncSkill", () => { const skill = new HermesagentSkill({ outputRoot: testDir, diff --git a/src/features/skills/hermesagent-skill.ts b/src/features/skills/hermesagent-skill.ts index 8633a4ef9..beeae01d4 100644 --- a/src/features/skills/hermesagent-skill.ts +++ b/src/features/skills/hermesagent-skill.ts @@ -3,6 +3,7 @@ import { RULESYNC_SKILLS_RELATIVE_DIR_PATH } from "../../constants/rulesync-path import { AgentsSkillsSkill, type AgentsSkillsSkillParams, + toAllowedToolsArray, toSpecConformantAgentSkillFields, } from "./agentsskills-skill.js"; import { RulesyncSkill, type RulesyncSkillFrontmatterInput } from "./rulesync-skill.js"; @@ -45,27 +46,35 @@ export class HermesagentSkill extends AgentsSkillsSkill { const rulesyncFrontmatter = rulesyncSkill.getFrontmatter(); // The `agentsskills` block is the same rulesync source the native Agent // Skills target reads, so it goes through the same normalization: one input - // must not produce two different on-disk spellings. - const shared = toSpecConformantAgentSkillFields(rulesyncFrontmatter.agentsskills); + // must not produce two different on-disk spellings. `metadata` is exempt + // because Hermes reads structured values under `metadata.hermes` + // (`requires_toolsets`, `tags`, …) that string coercion would break. + const shared = toSpecConformantAgentSkillFields(rulesyncFrontmatter.agentsskills, { + coerceMetadata: false, + }); const hermes = rulesyncFrontmatter.hermesagent ?? {}; + const dirName = rulesyncSkill.getDirName(); + const frontmatter = { + ...shared, + ...hermes, + name: rulesyncFrontmatter.name, + description: rulesyncFrontmatter.description, + }; HermesagentSkill.reportSpecViolations({ + outputRoot, relativeDirPath: HERMESAGENT_SKILLS_DIR_PATH, - dirName: rulesyncSkill.getDirName(), - rulesyncFrontmatter, + dirName, + frontmatter, + sourceAllowedTools: rulesyncFrontmatter.agentsskills?.["allowed-tools"], logger, }); return new this({ outputRoot, relativeDirPath: HERMESAGENT_SKILLS_DIR_PATH, - dirName: rulesyncSkill.getDirName(), - frontmatter: { - ...shared, - ...hermes, - name: rulesyncFrontmatter.name, - description: rulesyncFrontmatter.description, - }, + dirName, + frontmatter, body: rulesyncSkill.getBody(), otherFiles: rulesyncSkill.getOtherFiles(), validate, @@ -75,14 +84,19 @@ export class HermesagentSkill extends AgentsSkillsSkill { override toRulesyncSkill(): RulesyncSkill { const frontmatter = this.getFrontmatter(); + const allowedTools = + frontmatter["allowed-tools"] === undefined + ? undefined + : toAllowedToolsArray(frontmatter["allowed-tools"]); const agentsskills: NonNullable = { ...(frontmatter.license !== undefined && { license: frontmatter.license }), ...(frontmatter.compatibility !== undefined && { compatibility: frontmatter.compatibility, }), - ...(frontmatter["allowed-tools"] !== undefined && { - "allowed-tools": frontmatter["allowed-tools"], - }), + // Normalized back to the canonical rulesync array, matching the base + // class, so a generate → import round trip leaves the source unchanged. + ...(allowedTools !== undefined && + allowedTools.length > 0 && { "allowed-tools": allowedTools }), }; const hermesagent: Record = {}; From def684fe0e98122134e62e57f37244dbd01cc818 Mon Sep 17 00:00:00 2001 From: dyoshikawa Date: Mon, 27 Jul 2026 00:51:51 -0700 Subject: [PATCH 4/4] fix(agentsskills): do not warn about allowed-tools entries an override replaces MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Third round of review feedback, all low severity. - The whitespace warning still read the rulesync source unconditionally, so a `hermesagent:` override that replaces `allowed-tools` produced a warning about an entry that never reaches the file — contradicting the rule that diagnostics describe what is actually written. It now runs only when the emitted value is the joined source. - `AgentsSkillsSharedFields.metadata` was typed `Record | Record`, a union that collapses to the second member and so said nothing. Narrowed to one type. - The warned-path assertion now pins the `outputRoot` prefix that the previous commit added, instead of matching either form. Co-Authored-By: Claude Opus 5 (1M context) --- .../skills/agentsskills-skill.test.ts | 6 +++++- src/features/skills/agentsskills-skill.ts | 10 +++++++-- src/features/skills/hermesagent-skill.test.ts | 21 +++++++++++++++++++ 3 files changed, 34 insertions(+), 3 deletions(-) diff --git a/src/features/skills/agentsskills-skill.test.ts b/src/features/skills/agentsskills-skill.test.ts index 00c8ed0b1..9476cea0d 100644 --- a/src/features/skills/agentsskills-skill.test.ts +++ b/src/features/skills/agentsskills-skill.test.ts @@ -288,7 +288,11 @@ Body.`; warnings.some((w) => w.includes("`description` is required and must not be empty")), ).toBe(true); for (const warning of warnings) { - expect(warning).toContain(join(".agents", "skills", "My_Bad--Name", SKILL_FILE_NAME)); + // The reported path is rooted at outputRoot so a global-scope skill + // points at the file that actually gets written. + expect(warning).toContain( + join(testDir, ".agents", "skills", "My_Bad--Name", SKILL_FILE_NAME), + ); } }); diff --git a/src/features/skills/agentsskills-skill.ts b/src/features/skills/agentsskills-skill.ts index 3fe7b6d4a..3e57369c3 100644 --- a/src/features/skills/agentsskills-skill.ts +++ b/src/features/skills/agentsskills-skill.ts @@ -141,7 +141,7 @@ function toStringMetadata(metadata: Record): Record | Record; + metadata?: Record; "allowed-tools"?: string; }; @@ -260,7 +260,13 @@ function collectAgentSkillViolations({ violations.push( "`allowed-tools` must be a space-separated string; the Agent Skills spec does not allow a list here", ); - } else if (Array.isArray(sourceAllowedTools)) { + } else if ( + Array.isArray(sourceAllowedTools) && + // Only when the emitted value is the joined source. A tool-specific + // override replaces it outright, and warning about entries that never reach + // the file would contradict checking what is actually written. + frontmatter["allowed-tools"] === toAllowedToolsString(sourceAllowedTools) + ) { for (const tool of sourceAllowedTools.filter((entry) => /\s/.test(entry))) { violations.push( `\`allowed-tools\` entry "${tool}" contains whitespace, so the space-separated form the Agent Skills spec requires will read it back as several entries`, diff --git a/src/features/skills/hermesagent-skill.test.ts b/src/features/skills/hermesagent-skill.test.ts index 774de8d64..4744d507f 100644 --- a/src/features/skills/hermesagent-skill.test.ts +++ b/src/features/skills/hermesagent-skill.test.ts @@ -55,6 +55,27 @@ describe("HermesagentSkill", () => { }); describe("fromRulesyncSkill", () => { + it("should not warn about source allowed-tools entries a hermesagent override replaces", () => { + const logger = createMockLogger(); + const rulesyncSkill = new RulesyncSkill({ + outputRoot: testDir, + relativeDirPath: RULESYNC_SKILLS_RELATIVE_DIR_PATH, + dirName: "test-skill", + frontmatter: { + name: "test-skill", + description: "Test skill description", + agentsskills: { "allowed-tools": ["Bash(git log)"] }, + hermesagent: { "allowed-tools": "Read" }, + }, + body: "Test body content", + validate: true, + }); + + const skill = HermesagentSkill.fromRulesyncSkill({ rulesyncSkill, logger }); + + expect(skill.getFrontmatter()["allowed-tools"]).toBe("Read"); + expect(logger.warn).not.toHaveBeenCalled(); + }); it("should keep structured metadata that Hermes reads natively", () => { // Hermes resolves `metadata.hermes.*` as structured YAML, so the Agent // Skills string-map coercion must not apply on this target.