Allow non-SHA refs in skills frontmatter, pinned at compile time - #51455
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds compile-time SHA pinning for non-SHA skill references.
Changes:
- Expands skill-ref validation to branches, tags, and empty refs.
- Resolves refs through the shared action resolver.
- Updates tests and reference documentation.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/skills_ref_resolution.go |
Implements skill-ref resolution. |
pkg/workflow/skills_ref_resolution_test.go |
Tests resolution behavior. |
pkg/workflow/skills_frontmatter.go |
Expands ref validation. |
pkg/workflow/skills_frontmatter_test.go |
Tests accepted and rejected refs. |
pkg/workflow/compiler_validators.go |
Adds resolution to compilation. |
pkg/workflow/compiler_orchestrator_frontmatter_test.go |
Updates validation tests. |
docs/src/content/docs/reference/frontmatter.md |
Documents ref syntax. |
docs/src/content/docs/reference/frontmatter-full.md |
Updates full reference comments. |
Review details
Tip
Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
Suppressed comments (1)
docs/src/content/docs/reference/frontmatter-full.md:71
- This contradicts the updated validator, which explicitly rejects any skill spec containing a GitHub Actions expression. Describe expressions as unsupported rather than accepted at runtime.
# remote repospec by `gh aw add`. GitHub Actions expressions (`${{ ... }}`) are also
# accepted and are evaluated at runtime.
- Files reviewed: 8/8 changed files
- Comments generated: 4
- Review effort level: Balanced
| repoPath, ref, hasAt := strings.Cut(trimmed, "@") | ||
| if !hasAt || !skillRepoPathRegexp.MatchString(repoPath) { |
| ctx = context.Background() | ||
| } | ||
|
|
||
| sha, err := data.ActionResolver.ResolveSHA(ctx, repoPath, ref) |
| `<ref>` may be a branch, tag, or 40-character lowercase commit SHA. Non-SHA | ||
| refs are resolved and rewritten to the matching commit SHA at compile time | ||
| (the compiled `.lock.yml` always pins the resolved SHA). Omitting the ref | ||
| (`owner/repo@`) installs from the repository's default branch on every run | ||
| and is not pinned; the compiler emits a warning recommending an explicit ref. |
| # `.github/skills/my-skill`). `<ref>` may be a branch, tag, or full 40-character | ||
| # lowercase commit SHA; non-SHA refs are resolved and pinned to the matching commit | ||
| # SHA at compile time. Omitting the ref (`owner/repo@`) installs from the | ||
| # repository's default branch and is not pinned, which triggers a compiler warning. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Test Quality Sentinel completed test quality analysis.
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check.
|
|
✅ Ponytail Reviewer completed successfully!
|
|
✅ PR Code Quality Reviewer completed the code quality review. Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "api.individual.githubcopilot.com"See Network Configuration for more information.
|
🧪 Test Quality Sentinel ReportPR: Allow non-SHA refs in skills frontmatter, pinned at compile time Overview
Test Files Changed
Score BreakdownPer-test classification (27 tests)
(The remaining ~17 subtests in Quality Highlights
VerdictAPPROVE — 0% implementation tests (threshold: 30%). No violations.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 38.1 AIC · ⌖ 7.23 AIC · ⊞ 5.5K
| ctx = context.Background() | ||
| } | ||
|
|
||
| sha, err := data.ActionResolver.ResolveSHA(ctx, repoPath, ref) |
There was a problem hiding this comment.
[Blocking] Branch refs fail to resolve — resolveFromGitHub only queries /git/ref/tags/<ref>
The PR documents and advertises that non-SHA refs (branch names like main, release/1.0) are "resolved and pinned to the matching commit SHA at compile time". However, resolveFromGitHub (called via ResolveSHA) only hits the tags endpoint:
GET /repos/{owner}/{repo}/git/ref/tags/{version}
A branch name returns 404, which resolveSkillRefSpec treats as a resolution failure — emitting only a warning and leaving the ref unpinned. This silently violates the documented guarantee for the most common case (owner/repo@main).
Fix: after a tag lookup returns a non-zero error, fall back to the heads endpoint (/git/ref/heads/<ref>) before treating resolution as failed.
@copilot please address this.
|
|
||
| fn() | ||
|
|
||
| require.NoError(t, w.Close()) |
There was a problem hiding this comment.
[Minor] Double-close of w in withCapturedStderr
The pipe writer w is explicitly closed at line 42 (require.NoError(t, w.Close())), and then the deferred closure at line 37 (_ = w.Close()) closes it again when the helper returns. While the second close error is silently swallowed, this is misleading and could mask a genuine close failure on the first call if the defer were ever changed.
Either remove the explicit w.Close() on line 42 and close only in the defer (before reading from r), or remove the w.Close() in the defer since the explicit close already ran.
@copilot please address this.
Adds a draft Architecture Decision Record capturing the decision to accept branch/tag refs in the skills: frontmatter field and resolve them to commit SHAs at compile time. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
ADR Required — Draft GeneratedThis PR touches 335 lines in A draft Architecture Decision Record has been committed to this branch at:
What to do next
Why ADRs MatterArchitecture Decision Records capture why a decision was made, not just what was changed. A future engineer reading this code 12 months from now will understand the tradeoffs and constraints that led to this design, making it much easier to evolve the system safely. ADR Format Reference (Michael Nygard Template)The draft follows this structure:
Merge is blocked until this ADR is accepted.
|
There was a problem hiding this comment.
Ponytail review (over-engineering only): one finding.
net: -18 lines possible.
Generated by ✂️ Ponytail Reviewer for #51455 · auto · 45.5 AIC · ⌖ 6.58 AIC · ⊞ 6.8K
Comment /ponytail to run again
Comments that could not be inline-anchored
pkg/workflow/skills_ref_resolution_test.go:443
L427-444: stdlib: withCapturedStderr reimplements the existing testutil.CaptureStderr(t, fn) helper (pkg/testutil/tempdir.go:76) with an identical signature. Use that instead, 0 new lines.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /codebase-design — commenting with a few targeted issues; no blocking changes required.
📋 Key Themes & Highlights
Issues Found
- Weak test assertion (
skills_frontmatter_test.go): injection-safety test passes on any error, not specifically the unsafe-character error - Branch resolution gap (
skills_ref_resolution.go):ResolveSHAonly resolves tags; branch names will silently fall back to unpinned behavior without a clear test demonstrating this - Missing invariant guard (
skills_ref_resolution.go):Skills/SkillReferencessync assumes equal length; no defensive check exists - Ambiguous-SHA boundary coverage (
skills_frontmatter.go): 6-char and 41-char hex strings are not tested
Positive Highlights
- ✅ Graceful degradation (warn + keep original ref) is the right default for a compile-time enrichment pass
- ✅ Ambiguous-SHA rejection is a smart safety net — prevents silent truncation confusion
- ✅ Reusing the existing
ActionResolver/ cache infrastructure for skill ref resolution is a clean, consistent design - ✅ Test file structure is clear and well-organized;
withCapturedStderrhelper is a nice pattern
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 68.9 AIC · ⌖ 7.9 AIC · ⊞ 7.1K
Comment /matt to run again
| require.ErrorContains(t, err, "40-char-sha") | ||
| }) | ||
|
|
||
| t.Run("rejects 39-char sha", func(t *testing.T) { |
There was a problem hiding this comment.
[/tdd] The "rejects ref with unsafe characters" test only asserts require.Error(t, err) without checking the error message, so any incidental validation error would make it pass even if the injection-safety check regresses.
💡 Suggested assertion
Add an ErrorContains check to pin the test to the intended validation path:
require.Error(t, err)
require.ErrorContains(t, err, "unsupported characters")@copilot please address this.
| c.IncrementWarningCount() | ||
| return spec | ||
| } | ||
|
|
There was a problem hiding this comment.
[/tdd] ResolveSHA (action_resolver.go:199) only queries /git/ref/tags/<version>, so branch names like main cannot be resolved unless they happen to have a cached entry. The test pins a cache entry directly, bypassing the real API path. There is no test that validates the branch-resolution failure mode (when the tag API 404s and the fallback also misses).
💡 Why this matters
A user writing owner/repo@main will silently fall back to the original unpinned ref because the tag API returns 404 for branch names and the resolver has no branch-specific fallback. This is a correctness gap that the current tests do not surface.
Consider either:
- Adding a test that mocks a 404 from the tags API to confirm the fallback warning fires, or
- Documenting in the code that branch resolution requires the caller to pre-populate the cache.
@copilot please address this.
| data.Skills[i] = data.SkillReferences[i].Skill | ||
| } | ||
| } | ||
| return |
There was a problem hiding this comment.
[/codebase-design] data.SkillReferences and data.Skills are synced by assuming they are in the same order and have the same length. If a caller ever populates only one of these (or in a different order), the sync silently produces mismatched entries.
💡 Suggested guard
Add an invariant check at the top of resolveFrontmatterSkillRefs:
if len(data.SkillReferences) > 0 && len(data.Skills) != len(data.SkillReferences) {
skillsFrontmatterLog.Printf("warning: Skills/SkillReferences length mismatch (%d vs %d); skipping ref resolution",
len(data.Skills), len(data.SkillReferences))
return
}This makes the invariant explicit and prevents a silent partial-sync bug.
@copilot please address this.
| idx, | ||
| idx, | ||
| ) | ||
| } |
There was a problem hiding this comment.
[/tdd] There are no tests for the looksLikeAmbiguousSHA boundary conditions — specifically, a 6-char hex string (below the 7-char floor) and a 41-char hex string (above the 40-char ceiling). Both should be accepted as valid non-SHA refs, but a test regression in the boundary logic would go undetected.
💡 Suggested test cases
Add to TestValidateFrontmatterSkills:
t.Run("accepts 6-char hex string as branch/tag (below ambiguous-SHA floor)", func(t *testing.T) {
err := validateFrontmatterSkills(map[string]any{
"skills": []any{"owner/repo@abcdef"},
})
require.NoError(t, err)
})@copilot please address this.
|
@copilot Quick triage nudge for this PR. Please refresh the branch if GitHub can update it cleanly, address the latest review feedback, make sure the ADR is accepted if still required, run the Open items (newest first):
Branch refresh was requested. Run: https://github.com/github/gh-aw/actions/runs/31277244247
|
… commits endpoint; update docs for fallback behavior Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed the review feedback in two follow-up commits:
|
|
@copilot Quick triage nudge for this PR. Please refresh the branch if GitHub can update it cleanly, address the latest review feedback, make sure the ADR is accepted if still required, run the Open items (newest first):
Branch refresh was requested. Run: https://github.com/github/gh-aw/actions/runs/31277905762
|
|
Great work on enabling non-SHA refs in skills frontmatter! 🎉 This feature makes skill resolution much more flexible while maintaining compile-time pinning for reproducibility. The implementation is well-structured with: ✅ Comprehensive validation logic for ref formats This PR is ready for review.
|
|
🎉 This pull request is included in a new release. Release: |
skills:frontmatter entries required a fully-pinned 40-character lowercase commit SHA (owner/repo@<sha>), forcing authors to manually resolve and update SHAs by hand. This PR allows branch/tag refs instead, resolving them to a SHA automatically at compile time.Validation changes (
pkg/workflow/skills_frontmatter.go)owner/repo@<ref>andowner/repo/skill/path@<ref>where<ref>is a branch, tag, or full SHA (previously only a full 40-char lowercase SHA was accepted).owner/repo@(trailing@, empty ref) as an explicit "no ref" opt-out.Compile-time pinning (
pkg/workflow/skills_ref_resolution.go)resolveFrontmatterSkillRefspass resolves non-SHA refs to their commit SHA using the compiler's existing shared action resolver (same GitHub API + cache + embedded-pins infrastructure used foruses:action pinning), rewritingskills[]entries before YAML generation.owner/repo@) are left unpinned and trigger a compiler warning recommending an explicit ref.Docs
docs/src/content/docs/reference/frontmatter.mdandfrontmatter-full.mdto describe the new<ref>syntax and pinning/warning behavior.Run: https://github.com/github/gh-aw/actions/runs/31277244247> Generated by 👨🍳 PR Sous Chef · gpt54 · 9.11 AIC · ⌖ 5.12 AIC · ⊞ 8.5K · ◷