Skip to content

feat(skills): support paths in Claude Code skills#1604

Merged
dyoshikawa merged 2 commits into
dyoshikawa:mainfrom
jornetsimon:feat/skills-paths
May 11, 2026
Merged

feat(skills): support paths in Claude Code skills#1604
dyoshikawa merged 2 commits into
dyoshikawa:mainfrom
jornetsimon:feat/skills-paths

Conversation

@jornetsimon
Copy link
Copy Markdown
Contributor

@jornetsimon jornetsimon commented May 7, 2026

No description provided.

@jornetsimon jornetsimon marked this pull request as draft May 7, 2026 14:58
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@jornetsimon jornetsimon force-pushed the feat/skills-paths branch from 464dbc5 to 90d2acb Compare May 8, 2026 11:24
@jornetsimon jornetsimon marked this pull request as ready for review May 8, 2026 12:59
Copy link
Copy Markdown
Collaborator

@dyoshikawa-claw dyoshikawa-claw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a clean, well-tested addition. The schema, conversion, and round-trip all follow the existing patterns for allowed-tools, model, and disable-model-invocation exactly. Test coverage is thorough — string form, array form, validation rejects, and round-trip preservation are all covered.

One small thing: the fromDir method at line 201–225 already handles paths correctly since it delegates to the schema — but no test explicitly exercises importing a SKILL.md from disk with a paths field. The existing round-trip and conversion tests give good confidence, though.

Otherwise, looks good to merge.

Copy link
Copy Markdown
Collaborator

@dyoshikawa-claw dyoshikawa-claw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clean addition that follows the existing patterns well. The schema, both conversion directions, and the rulesync type are all updated consistently. The test coverage for string/array variants in object conversion and round-trip is thorough.

Two things to consider adding:

  • A fromDir test for paths, similar to the existing should load skill with model test, so that YAML parsing of comma-separated strings vs arrays is covered through the I/O path too.
  • A test that combines paths with another claudecode field like model or allowed-tools, to verify they don't interfere with each other during the spread-based conversion.

"allowed-tools": z.optional(z.array(z.string())),
model: z.optional(z.string()),
"disable-model-invocation": z.optional(z.boolean()),
paths: z.optional(z.union([z.string(), z.array(z.string())])),
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In claudecode-rule.ts, paths accepts only string[], while here it accepts string | string[]. A brief comment explaining this difference would help future maintainers understand it is intentional, not an oversight.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants