feat: support local skill paths in frontmatter skills: with --from-local - #49115
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot update instructions, documentation about how to handle local skills |
There was a problem hiding this comment.
Pull request overview
Adds support for repository-local skill paths in workflow frontmatter.
Changes:
- Accepts local skill paths in validation and schema.
- Installs local skills using
--from-local. - Adds Go and JavaScript coverage.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/skills_frontmatter.go |
Recognizes local skill references. |
pkg/workflow/skills_frontmatter_test.go |
Tests local reference validation. |
pkg/parser/schemas/main_workflow_schema.json |
Adds local-path schema alternatives. |
actions/setup/js/install_frontmatter_skills.cjs |
Builds local installation commands. |
actions/setup/js/install_frontmatter_skills.test.cjs |
Tests local runtime installation. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comments suppressed due to low confidence (1)
pkg/parser/schemas/main_workflow_schema.json:105
- The object form has the same mismatch:
skill: ./skills/my-skillis accepted by the Go/runtime implementation but fails schema validation, and traversal components remain allowed. Use the same optional-./, component-based pattern as the string form.
"pattern": "^\\.{0,2}[A-Za-z0-9_][A-Za-z0-9_./-]*$",
- Files reviewed: 5/5 changed files
- Comments generated: 2
- Review effort level: Medium
| if (isLocalSkillRef(skillSpec)) { | ||
| const skillName = skillSpec.replace(/\\/g, "/").split("/").filter(Boolean).pop() || skillSpec; | ||
| return { | ||
| displaySpec: skillSpec, | ||
| args: ["skill", "install", skillSpec, skillName, "--from-local", ...agentArgs, "--dir", skillsDst, "--force"], |
| }, | ||
| { | ||
| "type": "string", | ||
| "pattern": "^\\.{0,2}[A-Za-z0-9_][A-Za-z0-9_./-]*$", |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Updated in |
|
/review |
|
/matt |
|
Warning threat detection engine error DetailsThe threat detection engine failed to produce results. Review the workflow run logs for details. @copilot please run the Unresolved feedback to address (newest first):
Failed checks:
|
|
✅ PR Code Quality Reviewer completed the code quality review. Warning threat detection engine error DetailsThe threat detection engine failed to produce results. Review the workflow run logs for details. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ Warning threat detection engine error DetailsThe threat detection engine failed to produce results. Review the workflow run logs for details. |
|
Warning threat detection engine error DetailsThe threat detection engine failed to produce results. Review the workflow run logs for details. 🧪 Test Quality Sentinel Report✅ Test Quality Score: 85/100 — Excellent
📊 Metrics (10 test functions)
📋 Test Breakdown
Verdict
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Warning
threat detection engine error
The threat detection engine encountered an error and could not complete analysis. This is a tooling failure, not a security finding.
Details
The threat detection engine failed to produce results.
Review the workflow run logs for details.
Skills-Based Review 🧠
Applied /tdd and /codebase-design — requesting changes for a schema-vs-runtime mismatch and an undocumented edge case.
📋 Key Themes & Highlights
Issues Found
- Schema mismatch: The JSON Schema pattern for local paths rejects
./skills/my-skillbut every other layer (Go validation, JS runtime, docs, tests) accepts it. This will cause unexpected LSP/editor validation failures and silent inconsistency. - Ambiguous unpinned owner/repo: A bare
owner/repowithout@is silently routed to--from-local, which will produce a confusing runtime error rather than a validation failure. The policy ("remote refs must include@") is expressed only as a comment, not enforced.
Positive Highlights
- ✅ Clean symmetric implementation across Go, JavaScript, and JSON Schema
- ✅ Good test coverage for
isLocalSkillRef— comprehensive case matrix - ✅ Backward compatible; existing pinned remote refs are unaffected
- ✅ Windows backslash normalisation in skill name derivation is a thoughtful touch
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 55.4 AIC · ⊞ 7K
Comment /matt to run again
There was a problem hiding this comment.
Warning
threat detection engine error
The threat detection engine encountered an error and could not complete analysis. This is a tooling failure, not a security finding.
Details
The threat detection engine failed to produce results.
Review the workflow run logs for details.
Verdict: Request changes
A security-relevant regression: unpinned remote skill specs (e.g. githubnext/awesome-skills) are now silently treated as local paths and installed with --from-local instead of being rejected for missing the required SHA pin. The schema also rejects the PR's own documented/tested local-path example (./skills/my-skill), so compilation would fail for that exact case despite passing unit tests (which stub the JS/Go functions directly, not the schema).
Themes
- Correctness (blocking):
isLocalSkillRefclassifies any unpinnedowner/repostring as local, removing the prior hard requirement that remote refs be pinned to a 40-char SHA. Anyone typo-ing@or omitting the pin gets silent misbehavior instead of a validation error. - Correctness (blocking): JSON Schema pattern for local paths doesn't actually allow
./-prefixed paths, contradicting docs, JS tests, and Go tests that all assert./skills/my-skillis a supported local form. - Maintainability (medium): skill name derivation from the last path segment risks silent overwrite collisions between differently-scoped local skills sharing a basename.
Sub-agent (grumpy-coder) did not return results in time; this review reflects independent analysis only.
🔎 Code quality review by PR Code Quality Reviewer · auto · 93.1 AIC · ⊞ 7.8K
Comment /review to run again
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed both unresolved review threads in commit
I also ran local validation via |
|
Warning threat detection engine error DetailsThe threat detection engine failed to produce results. Review the workflow run logs for details. @copilot please run the Failed checks:
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed in
Validation run locally with |
PR Triage
Rationale: Adds local skill path support with schema + compiler + docs updates and tests. Currently has a merge conflict (
|
|
🎉 This pull request is included in a new release. Release: |
Local skill paths (e.g.
skills/rig,.github/skills/my-skill) were rejected by validation and unsupported at runtime, making local skill library development require a full publish-and-pin cycle.Changes
pkg/workflow/skills_frontmatter.go— AddedisLocalSkillRef(no@, not${{) and updatedvalidateSkillSpecValueto accept local paths alongside pinned remote refsactions/setup/js/install_frontmatter_skills.cjs— Added matchingisLocalSkillRef;buildSkillInstallCommandnow routes local paths through--from-local, deriving the skill name from the last path componentpkg/parser/schemas/main_workflow_schema.json— Added local-pathoneOfalternative toskillsitems (string and object forms)add_skill_rewrite.go— Already handlesgh aw addrewriting; local refs are promoted toowner/repo/path@shaon publish (no changes needed)Example
At runtime this emits:
On
gh aw add, local refs are rewritten toowner/repo/skills/rig@<sha>so the published workflow is fully pinned.Warning
threat detection engine error
The threat detection engine encountered an error and could not complete analysis. This is a tooling failure, not a security finding.
Details
The threat detection engine failed to produce results.
Review the workflow run logs for details.