fix(agentsskills): emit spec-conformant SKILL.md and diagnose name/description violations - #2435
Merged
dyoshikawa merged 4 commits intoJul 27, 2026
Merged
Conversation
…scription 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) <noreply@anthropic.com>
…rve YAML timestamps
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) <noreply@anthropic.com>
…itted frontmatter 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) <noreply@anthropic.com>
…e replaces 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<string, string> | Record<string, unknown>`, 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) <noreply@anthropic.com>
Owner
Author
|
@dyoshikawa Thank you! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Background
The
agentsskillstarget is rulesync's implementation of the Agent Skills standard, yet theSKILL.mdit generated was not conformant, and the normativename/descriptionconstraints were never checked.Re-verified against the current specification (fetched 2026-07-27):
namea-z0-9and hyphens, no leading/trailing hyphen, no--, must match the parent directory namedescriptioncompatibilitymetadataallowed-toolsChanges
1. Export conformance
fromRulesyncSkillcopiedallowed-tools,compatibilityandmetadataverbatim out of the rulesync frontmatter, so a legacy list or object input was written as a YAML sequence / mapping that the spec's ownskills-ref validaterejects. They are now normalized on the way out:allowed-tools: a list is joined with spaces — the same treatmentDeepagentsSkillalready applied to the identical field.compatibility: an object is flattened tokey: valuepairs.metadata: non-string values are stringified (scalars use their natural text form, containers are JSON-encoded) so the block stays a string→string map.Input schemas are untouched, so existing
.rulesync/skills/**frontmatter keeps working — only the emitted shape changes.Before / after, for
agentsskills: { allowed-tools: ["Read", "Bash(git:*)"], compatibility: { runtime: node }, metadata: { version: 1 } }:2. Normative
name/descriptiondiagnosticsGeneration now reports, per skill:
nameempty, longer than 64 characters, containing anything but lowercase letters, digits and single hyphens, or not equal to its parent directory name;descriptionempty or longer than 1024 characters;compatibilitylonger than 500 characters.These are warnings, not errors. A conformant client only skips a non-conformant skill, import must stay lenient per the client-implementation guide, and failing the run outright would break existing skill directories. What must not happen — and no longer does — is emitting a skill that clients drop without rulesync saying anything.
Reporting needs a logger, so
ToolSkillFromRulesyncSkillParamsgains an optionalloggerthatSkillsProcessorpasses through. Other skill classes ignore it.Verification
A skill directory
My_Bad--Namewithname: Totally-Different-NAME--xand an empty description now produces, ongenerate --targets agentsskills --features skills:Tests cover the three serializations, the already-conformant pass-through, every violation message, the length limits, and the no-warning case.
docs/reference/file-formats.mddocuments the new contract.pnpm cicheckpasses.Closes #2429