Publish: use shared discovery logic instead of requiring skills/ directory#13167
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates gh skill publish to use the shared local skill discovery logic (matching install), removing the hard requirement that skills live under a top-level skills/ directory.
Changes:
- Switch
publishto discover skills viadiscovery.DiscoverLocalSkills()rather than enumeratingskills/. - Update security/code-scanning heuristics to operate over multiple discovered skill directories.
- Update and extend publish tests to cover root-level and namespaced skill discovery.
Show a summary per file
| File | Description |
|---|---|
pkg/cmd/skills/publish/publish.go |
Replaces hardcoded skills/ enumeration with shared discovery, updates downstream logic to handle multiple discovered directories. |
pkg/cmd/skills/publish/publish_test.go |
Adjusts expectations for “no skills” cases and adds coverage for root-level and namespaced discovery. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 2
1d21aa6 to
a40d072
Compare
babakks
left a comment
There was a problem hiding this comment.
LGTM, with a few suggestions and a question.
| Skills are discovered using the same conventions as install: | ||
|
|
||
| - skills/*/SKILL.md | ||
| - skills/{scope}/*/SKILL.md | ||
| - */SKILL.md (root-level) | ||
| - plugins/{scope}/skills/*/SKILL.md | ||
|
|
There was a problem hiding this comment.
As a general comment, these docs will be regarded as markdown and rendered on the website (example). So, it's best to have that in mind when writing --help docs. So, please apply this comment to other skill commands.
In this particular case, let's surround verbatim text with backticks:
| Skills are discovered using the same conventions as install: | |
| - skills/*/SKILL.md | |
| - skills/{scope}/*/SKILL.md | |
| - */SKILL.md (root-level) | |
| - plugins/{scope}/skills/*/SKILL.md | |
| Skills are discovered using the same conventions as install: | |
| - `skills/*/SKILL.md` | |
| - `skills/{scope}/*/SKILL.md` | |
| - `*/SKILL.md` (root-level) | |
| - `plugins/{scope}/skills/*/SKILL.md` | |
There was a problem hiding this comment.
Here too:
| - Install metadata (`metadata.github-*`) is stripped if present |
There was a problem hiding this comment.
And also here:
| Use `--dry-run` to validate without publishing. | |
| Use `--tag` to publish non-interactively with a specific tag. | |
| Use `--fix` to automatically strip install metadata from committed files. |
| // Discover skills using shared discovery logic (same conventions as install) | ||
| skills, err := discovery.DiscoverLocalSkills(dir) |
| if hasCode && hasManifests { | ||
| return filepath.SkipAll | ||
| } |
There was a problem hiding this comment.
This has probably been discussed/clarified but just to double check.
question: returning SkipAll causes filepath.Walk to stop walking the tree and return immediately. Is that what we want? I mean, don't we want to go through all other directories under dir?
There was a problem hiding this comment.
Good Q. Let me dig in.
Replace the hardcoded skills/ directory requirement in the publish
command with the shared DiscoverLocalSkills() function used by install
and other commands. This removes the opinionated restriction that skills
must live under a skills/ directory and supports all discovery
conventions:
- skills/*/SKILL.md
- skills/{scope}/*/SKILL.md
- */SKILL.md (root-level)
- plugins/{scope}/skills/*/SKILL.md
All per-skill validation (frontmatter, spec-compliant naming, metadata
stripping, etc.) is preserved.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
a40d072 to
b63f5bf
Compare
Summary
Replaces the hardcoded
skills/directory requirement in thepublishcommand with the sharedDiscoverLocalSkills()function already used byinstalland other commands.Motivation
The publish command previously required all skills to live under a
skills/directory, but the rest of the CLI (install,search, etc.) already supports multiple discovery conventions. There's no reason for publish to be more restrictive — it's a pointless opinionated restriction that we've moved on from.Changes
publish.go: Replaced manualskills/directory enumeration withdiscovery.DiscoverLocalSkills(dir). All per-skill validation (frontmatter fields, spec-compliant naming, metadata stripping, license checks, body length) is preserved.publish.go: UpdatedcheckSecuritySettingsanddetectCodeAndManifeststo accept multiple skill directories instead of a singleskills/path.publish_test.go: Updated existing tests for the new behavior and added tests for root-level and namespaced skill discovery.Supported conventions (now all discoverable by publish)
skills/*/SKILL.mdskills/{scope}/*/SKILL.md*/SKILL.md(root-level)plugins/{scope}/skills/*/SKILL.mdStacked on
#13165