diff --git a/docs/adr/51455-allow-non-sha-refs-in-skills-frontmatter.md b/docs/adr/51455-allow-non-sha-refs-in-skills-frontmatter.md new file mode 100644 index 00000000000..c82bd9f222c --- /dev/null +++ b/docs/adr/51455-allow-non-sha-refs-in-skills-frontmatter.md @@ -0,0 +1,56 @@ +# ADR-51455: Allow Non-SHA Refs in Skills Frontmatter, Pinned at Compile Time + +**Date**: 2026-08-08 +**Status**: Draft +**Deciders**: pelikhan (PR author), copilot-swe-agent (implementation) + +--- + +### Context + +The `skills:` frontmatter field in workflow markdown files previously required every remote skill reference to be pinned to a full 40-character lowercase commit SHA (e.g., `owner/repo@abc123...def456`). This forced authors to manually look up the current SHA for a branch or tag, paste it in, and update it by hand whenever they wanted to upgrade a dependency. The requirement was intended to guarantee reproducible, tamper-resistant builds — the same `.lock.yml` would always activate the exact same skill code. However, the manual SHA-management burden reduced the ergonomics of skill authoring without adding a meaningful security benefit beyond what compile-time pinning already provides, since the compiler already pins `uses:` action references using a shared GitHub API + cache resolver. + +### Decision + +We will relax the validation of the `skills:` frontmatter field to accept branch names, tag names, or full commit SHAs as the `` portion of `owner/repo@` and `owner/repo/skill/path@` entries. Non-SHA refs are resolved to their current commit SHA at compile time using the compiler's existing `ActionResolver` (the same infrastructure used to pin `uses:` action references), and the resolved SHA is written into the compiled `.lock.yml`. The original source frontmatter retains the human-readable branch/tag name for authoring convenience. Ambiguous SHA-like strings (hex chars, 7–39 chars) are still rejected to prevent ref confusion. GitHub Actions expressions (`${{ ... }}`) remain unsupported as skill refs. Entries with no ref (`owner/repo@`) are permitted but emit a compiler warning recommending an explicit ref. + +### Alternatives Considered + +#### Alternative 1: Keep Requiring Full SHA-Only Refs (Status Quo) + +Authors continue to specify a 40-character lowercase SHA for every remote skill. The compiler does not need a ref-resolution pass; validation remains a simple regexp. + +Rejected because: the ergonomic cost is high — updating a skill pin requires looking up the SHA via `gh api` or browsing GitHub, and there is no automation to help. The security guarantee is not meaningfully stronger than compile-time pinning, since the `.lock.yml` would still be the authoritative artifact used at runtime. + +#### Alternative 2: Accept Branch/Tag Refs Without Pinning at Compile Time + +Accept any branch/tag ref in frontmatter and pass it through to the `.lock.yml` as-is, relying on the runtime skill installer to resolve the ref at activation time. + +Rejected because: this breaks reproducibility — two activations of the same `.lock.yml` at different times can pick up different code if the branch has moved. It would also undermine the security posture that SHA pinning provides, since a compromised branch could silently change what code runs in a workflow. + +#### Alternative 3: Resolve Refs at a Separate Pre-Compile Step (CI/CD Automation) + +A separate CI job or bot resolves branch/tag refs to SHAs and opens a PR to update the frontmatter, similar to Dependabot-style pin management. + +Rejected because: it requires additional infrastructure, introduces lag between authoring and pinning, and does not improve the immediate authoring experience. The compile-time resolution already has access to the resolver and cache, making a separate step redundant. + +### Consequences + +#### Positive +- Authors can reference skill dependencies by branch or tag name (e.g., `@main`, `@v1.2.3`, `@release/1.0`), eliminating the need to manually look up and maintain SHA strings. +- The compiled `.lock.yml` always contains fully-pinned SHAs, preserving the existing reproducibility and tamper-resistance guarantee at the artifact level. +- Reuses the existing `ActionResolver` + cache infrastructure, keeping the implementation surface small and consistent with how `uses:` action pinning already works. +- Ambiguous SHA-like strings (truncated or malformed SHAs) are explicitly rejected, preventing a class of ref-confusion bugs. + +#### Negative +- Resolution failures (e.g., no network access, missing GitHub auth) degrade gracefully to a compiler warning rather than a hard error, which means a `.lock.yml` can be emitted with an unpinned ref in offline or restricted-auth build environments. +- The new `resolveFrontmatterSkillRefs` compiler pass adds a GitHub API call per non-SHA skill ref during compilation; in environments where the cache is cold this adds latency. +- The `owner/repo@` (no-ref) form is now syntactically valid, which may be accidentally used by authors who omit a ref, producing non-reproducible builds that only show a warning. + +#### Neutral +- The validation regexp is replaced by structured parsing (split on `@`, validate repo path and ref separately), which is slightly more complex but allows clearer, per-field error messages. +- Existing workflows using full SHA refs are unaffected — they pass through the new validation unchanged and are not re-resolved. + +--- + +*ADR created by [adr-writer agent]. Review and finalize before changing status from Draft to Accepted.* diff --git a/docs/src/content/docs/reference/frontmatter-full.md b/docs/src/content/docs/reference/frontmatter-full.md index 8e8d8ccdc98..301ff785bed 100644 --- a/docs/src/content/docs/reference/frontmatter-full.md +++ b/docs/src/content/docs/reference/frontmatter-full.md @@ -60,12 +60,17 @@ labels: [] # Array of strings # Optional list of skill references to install during activation. Supports remote -# repository-wide installs (`owner/repo@`), remote path-scoped installs -# (`owner/repo/skill/path@`), and local path references (e.g. `skills/rig` or -# `.github/skills/my-skill`). Remote static references must be pinned to a full -# 40-character lowercase commit SHA. Local paths are installed with --from-local -# at runtime and are rewritten to a remote repospec by `gh aw add`. GitHub Actions -# expressions (`${{ ... }}`) are also accepted and are evaluated at runtime. +# repository-wide installs (`owner/repo@`), remote path-scoped installs +# (`owner/repo/skill/path@`), and local path references (e.g. `skills/rig` or +# `.github/skills/my-skill`). `` may be a branch, tag, or full 40-character +# lowercase commit SHA; non-SHA refs are resolved and rewritten to the matching commit +# SHA at compile time. If resolution fails (e.g. no network access or authentication), +# the compiler keeps the original unpinned ref and emits a warning. Omitting the ref +# (`owner/repo@`) installs from the +# repository's default branch and is not pinned, which triggers a compiler warning. +# Local paths are installed with --from-local at runtime and are rewritten to a +# remote repospec by `gh aw add`. GitHub Actions expressions (`${{ ... }}`) are also +# accepted and are evaluated at runtime. # Entries may also be objects to configure per-skill authentication via # github-token or github-app. # (optional) diff --git a/docs/src/content/docs/reference/frontmatter.md b/docs/src/content/docs/reference/frontmatter.md index c53cb66e7ba..4f03892fd40 100644 --- a/docs/src/content/docs/reference/frontmatter.md +++ b/docs/src/content/docs/reference/frontmatter.md @@ -237,8 +237,8 @@ Supported entry formats: - String form (shared authentication): - `skills/name` or `.github/skills/name` (local development path; installed with `--from-local`) - - `owner/repo@<40-char-sha>` - - `owner/repo/skill/path@<40-char-sha>` + - `owner/repo@` + - `owner/repo/skill/path@` - Object form (per-skill authentication): - `skill` (required) - `github-token` (optional) @@ -247,12 +247,17 @@ Supported entry formats: `github-token` and `github-app` are mutually exclusive for each object entry. `github-token` must be an expression such as `${{ secrets.NAME }}` or `${{ needs.auth.outputs.token }}`. -Static external references must be pinned to a 40-character lowercase commit SHA. +`` 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. +If resolution fails (for example, due to missing network access or authentication), +the compiler keeps the original unpinned ref and emits a warning. 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. ```yaml wrap skills: - # Shared auth via workflow-level activation token - - mattpocock/skills/tdd@801dca688564c529fa84f247f64472520d9ebe28 + # Shared auth via workflow-level activation token; sha-pinned automatically at compile time + - mattpocock/skills/tdd@main # Per-skill PAT (or fallback) for private skill repositories - skill: mattpocock/skills/diagnosing-bugs@801dca688564c529fa84f247f64472520d9ebe28 diff --git a/pkg/parser/schemas/main_workflow_schema.json b/pkg/parser/schemas/main_workflow_schema.json index c945c1a5c77..24aeeadc33c 100644 --- a/pkg/parser/schemas/main_workflow_schema.json +++ b/pkg/parser/schemas/main_workflow_schema.json @@ -58,12 +58,12 @@ }, "skills": { "type": "array", - "description": "Optional list of skill references to install during activation. Supports remote repository-wide installs (`owner/repo@`), remote path-scoped installs (`owner/repo/skill/path@`), and local path references (e.g. `skills/rig` or `.github/skills/my-skill`). Remote static references must be pinned to a full 40-character lowercase commit SHA. Local paths are installed with --from-local at runtime and are rewritten to a remote repospec by `gh aw add`. GitHub Actions expressions (`${{ ... }}`) are also accepted and are evaluated at runtime. Entries may also be objects to configure per-skill authentication via github-token or github-app.", + "description": "Optional list of skill references to install during activation. Supports remote repository-wide installs (`owner/repo@`), remote path-scoped installs (`owner/repo/skill/path@`), and local path references (e.g. `skills/rig` or `.github/skills/my-skill`). Remote static references may use a full 40-character lowercase commit SHA, or a branch/tag name (resolved to a SHA at compile time). Omitting the ref (`owner/repo@`) opts out of pinning and triggers a compile-time warning. Local paths are installed with --from-local at runtime and are rewritten to a remote repospec by `gh aw add`. GitHub Actions expressions (`${{ ... }}`) are also accepted and are evaluated at runtime. Entries may also be objects to configure per-skill authentication via github-token or github-app.", "items": { "oneOf": [ { "type": "string", - "pattern": "^[A-Za-z0-9_.-]+\\/[A-Za-z0-9_.-]+(?:\\/[A-Za-z0-9_.-]+(?:\\/[A-Za-z0-9_.-]+)*)?@[0-9a-f]{40}$" + "pattern": "^[A-Za-z0-9_.-]+\\/[A-Za-z0-9_.-]+(?:\\/[A-Za-z0-9_.-]+(?:\\/[A-Za-z0-9_.-]+)*)?@(?:[0-9a-f]{40}|[A-Za-z0-9](?:[A-Za-z0-9_./-]*[A-Za-z0-9_.-])?)?$" }, { "type": "string", @@ -90,7 +90,7 @@ "oneOf": [ { "type": "string", - "pattern": "^[A-Za-z0-9_.-]+\\/[A-Za-z0-9_.-]+(?:\\/[A-Za-z0-9_.-]+(?:\\/[A-Za-z0-9_.-]+)*)?@[0-9a-f]{40}$" + "pattern": "^[A-Za-z0-9_.-]+\\/[A-Za-z0-9_.-]+(?:\\/[A-Za-z0-9_.-]+(?:\\/[A-Za-z0-9_.-]+)*)?@(?:[0-9a-f]{40}|[A-Za-z0-9](?:[A-Za-z0-9_./-]*[A-Za-z0-9_.-])?)?$" }, { "type": "string", diff --git a/pkg/workflow/action_resolver.go b/pkg/workflow/action_resolver.go index 584e1f96176..d9d2f66635f 100644 --- a/pkg/workflow/action_resolver.go +++ b/pkg/workflow/action_resolver.go @@ -189,7 +189,11 @@ func ParseTagRefTSV(line string) (sha, objType string, err error) { return sha, objType, nil } -// resolveFromGitHub uses gh CLI to resolve the SHA for an action@version +// resolveFromGitHub uses gh CLI to resolve the SHA for an action@version. +// It first attempts to resolve as a tag via the git/refs/tags endpoint (which +// also handles annotated-tag peeling). If the tag lookup fails — indicating the +// ref is a branch name or an arbitrary commit ref — it falls back to the commits +// endpoint, which accepts branch names, tag names, and SHAs. func (r *ActionResolver) resolveFromGitHub(ctx context.Context, repo, version string) (string, error) { // Extract base repository (for actions like "github/codeql-action/upload-sarif") baseRepo := gitutil.ExtractBaseRepo(repo) @@ -214,7 +218,11 @@ func (r *ActionResolver) resolveFromGitHub(ctx context.Context, repo, version st ForceGHHostEnv(cmd, "github.com") output, err := cmd.Output() if err != nil { - return "", fmt.Errorf("failed to resolve %s@%s: %w", repo, version, err) + // Tag lookup failed. The ref may be a branch name rather than a tag. + // Fall back to the commits endpoint, which resolves both branch and tag + // names as well as SHAs, so that authors can pin to branches (e.g. "main"). + resolverLog.Printf("Tag lookup for %s@%s failed (%v); falling back to commits endpoint", repo, version, err) + return r.resolveRefViaCommitsEndpoint(ctx, baseRepo, repo, version) } sha, objType, err := ParseTagRefTSV(string(output)) @@ -246,6 +254,29 @@ func (r *ActionResolver) resolveFromGitHub(ctx context.Context, repo, version st return sha, nil } +// resolveRefViaCommitsEndpoint resolves a branch name, tag name, or arbitrary ref +// to its commit SHA using the GitHub API commits endpoint +// (GET /repos/{owner}/{repo}/commits/{ref}), which accepts all ref types. +// This is the fallback used when the tags-specific endpoint returns an error. +func (r *ActionResolver) resolveRefViaCommitsEndpoint(ctx context.Context, baseRepo, repo, version string) (string, error) { + commitsPath := fmt.Sprintf("/repos/%s/commits/%s", baseRepo, version) + resolverLog.Printf("Querying commits endpoint: %s", commitsPath) + callCtx, cancel := context.WithTimeout(ctx, 30*time.Second) + defer cancel() + cmd := ExecGHContext(callCtx, "api", commitsPath, "--jq", ".sha") + ForceGHHostEnv(cmd, "github.com") + output, err := cmd.Output() + if err != nil { + return "", fmt.Errorf("failed to resolve %s@%s: %w", repo, version, err) + } + sha := strings.TrimSpace(string(output)) + if !gitutil.IsValidFullSHA(sha) { + return "", fmt.Errorf("unexpected response resolving %s@%s: got %q (expected 40-char hex SHA)", repo, version, sha) + } + resolverLog.Printf("Resolved %s@%s to commit SHA %s via commits endpoint", repo, version, sha) + return sha, nil +} + // peelTagObject resolves a single annotated-tag object to its underlying object by // querying the GitHub API. It is called iteratively for chained tag objects. // The timeout context is created and immediately deferred within this function so diff --git a/pkg/workflow/compiler_orchestrator_frontmatter_test.go b/pkg/workflow/compiler_orchestrator_frontmatter_test.go index 5d955960d75..ae2ac89865c 100644 --- a/pkg/workflow/compiler_orchestrator_frontmatter_test.go +++ b/pkg/workflow/compiler_orchestrator_frontmatter_test.go @@ -273,7 +273,7 @@ func TestParseFrontmatterSection_InvalidSkillsRef(t *testing.T) { on: workflow_dispatch engine: copilot skills: - - githubnext/skills@main + - githubnext/skills@1f181b37d3fe5862ab590648f25a292e345b5de --- # Workflow @@ -288,7 +288,7 @@ skills: require.Error(t, err) assert.Nil(t, result) assert.True(t, - strings.Contains(err.Error(), "40-char-sha") || strings.Contains(err.Error(), "does not match pattern"), + strings.Contains(err.Error(), "truncated or malformed") || strings.Contains(err.Error(), "does not match pattern"), "expected skills validation error, got: %v", err, ) } @@ -316,7 +316,7 @@ skills: require.Error(t, err, "expected error: GitHub Actions expressions are not allowed in skills refs") assert.Nil(t, result) assert.True(t, - strings.Contains(err.Error(), "40-char-sha") || strings.Contains(err.Error(), "does not match pattern"), + strings.Contains(err.Error(), "does not support expressions") || strings.Contains(err.Error(), "does not match pattern"), "expected skills validation error, got: %v", err, ) } diff --git a/pkg/workflow/compiler_validators.go b/pkg/workflow/compiler_validators.go index dafd3c86208..2a932c64806 100644 --- a/pkg/workflow/compiler_validators.go +++ b/pkg/workflow/compiler_validators.go @@ -157,6 +157,7 @@ func (c *Compiler) validateToolConfiguration(workflowData *WorkflowData, markdow return err } c.emitGeneralToolWarnings(workflowData, markdownPath) + c.resolveFrontmatterSkillRefs(workflowData, markdownPath) if err := c.validateThreatDetectionSandboxRequirement(workflowData, markdownPath); err != nil { return err } diff --git a/pkg/workflow/skills_frontmatter.go b/pkg/workflow/skills_frontmatter.go index e89ee07e7ac..e3f512c1fc3 100644 --- a/pkg/workflow/skills_frontmatter.go +++ b/pkg/workflow/skills_frontmatter.go @@ -6,22 +6,42 @@ import ( "regexp" "strings" + "github.com/github/gh-aw/pkg/gitutil" "github.com/github/gh-aw/pkg/logger" ) var skillsFrontmatterLog = logger.New("workflow:skills_frontmatter") -var skillSpecRegexp = regexp.MustCompile(`^[A-Za-z0-9_.-]+/[A-Za-z0-9_.-]+(?:/[A-Za-z0-9_.-]+(?:/[A-Za-z0-9_.-]+)*)?@[0-9a-f]{40}$`) +// skillRepoPathRegexp matches the repository (and optional skill sub-path) portion +// of a remote skill spec, e.g. "owner/repo" or "owner/repo/skill/path". +var skillRepoPathRegexp = regexp.MustCompile(`^[A-Za-z0-9_.-]+/[A-Za-z0-9_.-]+(?:/[A-Za-z0-9_.-]+)*$`) + +// skillRefCharsRegexp restricts non-SHA refs (branch/tag names) to a safe character +// set. This is deliberately more permissive than a SHA (branch names may contain "/" +// for hierarchical names such as "release/1.0") while still preventing shell/argument +// injection when the ref is later passed to "gh" subprocesses. +var skillRefCharsRegexp = regexp.MustCompile(`^[A-Za-z0-9][A-Za-z0-9_./-]*$`) + var localSkillPathRegexp = regexp.MustCompile(`^(?:\./)?(?:\.[A-Za-z0-9_-][A-Za-z0-9_.-]*|[A-Za-z0-9_-][A-Za-z0-9_.-]*)(?:/(?:\.[A-Za-z0-9_-][A-Za-z0-9_.-]*|[A-Za-z0-9_-][A-Za-z0-9_.-]*))*$`) var skillsGitHubTokenExpressionRegexp = regexp.MustCompile(`^\$\{\{\s*(secrets\.[A-Za-z_][A-Za-z0-9_]*(\s*\|\|\s*secrets\.[A-Za-z_][A-Za-z0-9_]*)*|needs\.[A-Za-z_][A-Za-z0-9_]*\.outputs\.[A-Za-z_][A-Za-z0-9_]*)\s*\}\}$`) +// looksLikeAmbiguousSHA reports whether ref is composed entirely of hex characters +// with a length between 7 and 40 (inclusive) but is not itself a valid, canonical +// (lowercase, 40-char) full SHA. Such values are rejected as skill refs because they +// could be mistaken for (or silently truncate to) a real commit SHA, which is a +// well-known ref-confusion/collision risk; authors should use the full 40-character +// lowercase SHA or a clearly non-SHA branch/tag name instead. +func looksLikeAmbiguousSHA(ref string) bool { + return len(ref) >= 7 && len(ref) <= 40 && gitutil.IsHexString(ref) && !gitutil.IsValidFullSHA(ref) +} + // isLocalSkillRef reports whether spec is a local skill reference — a // repository-relative path that should be installed with --from-local at // runtime. A spec is treated as local when it: // - is not empty, // - does not begin with "${{" (not a GitHub Actions expression), and -// - contains no "@" separator (and therefore cannot be a fully-pinned -// remote reference such as "owner/repo/path@<40-char-sha>"). +// - contains no "@" separator (and therefore cannot be a remote reference +// such as "owner/repo/path@" or "owner/repo/path@<40-char-sha>"). func isLocalSkillRef(spec string) bool { spec = strings.TrimSpace(spec) return spec != "" && !strings.HasPrefix(spec, "${{") && !strings.Contains(spec, "@") @@ -36,14 +56,15 @@ type SkillReference struct { } func validateSkillSpecValue(skillSpec string, idx int) error { - if strings.TrimSpace(skillSpec) == "" { + trimmed := strings.TrimSpace(skillSpec) + if trimmed == "" { return fmt.Errorf("skills[%d] must be a non-empty string. Example: skills[%d]: \"owner/repo@abc1234...\"", idx, idx) } // Local path references (no "@" and not an expression) are allowed; they // are installed with --from-local at runtime and rewritten to a remote // repospec by "gh aw add". if isLocalSkillRef(skillSpec) { - if !localSkillPathRegexp.MatchString(strings.TrimSpace(skillSpec)) { + if !localSkillPathRegexp.MatchString(trimmed) { return fmt.Errorf( "skills[%d] local paths must be repository-relative without '..' traversal segments (got %q). Example: skills[%d]: \"./skills/my-skill\"", idx, @@ -53,14 +74,60 @@ func validateSkillSpecValue(skillSpec string, idx int) error { } return nil } - if !skillSpecRegexp.MatchString(skillSpec) { + + // GitHub Actions expressions are not supported as skill refs: they cannot be + // syntax-validated or resolved to a SHA at compile time. + if strings.Contains(trimmed, "${{") { return fmt.Errorf( - "skills[%d] must use owner/repo@<40-char-sha> or owner/repo/skill/path@<40-char-sha> (got %q). Example: skills[%d]: \"owner/repo@abcdef1234567890abcdef1234567890abcdef12\"", + "skills[%d] must use owner/repo@ or owner/repo/skill/path@ and does not support expressions (got %q). Example: skills[%d]: \"owner/repo@main\" or skills[%d]: \"owner/repo@abcdef1234567890abcdef1234567890abcdef12\"", idx, skillSpec, idx, + idx, ) } + + repoPath, ref, hasAt := strings.Cut(trimmed, "@") + if !hasAt || !skillRepoPathRegexp.MatchString(repoPath) { + return fmt.Errorf( + "skills[%d] must use owner/repo@ or owner/repo/skill/path@ (got %q). Example: skills[%d]: \"owner/repo@main\" or skills[%d]: \"owner/repo@abcdef1234567890abcdef1234567890abcdef12\"", + idx, + skillSpec, + idx, + idx, + ) + } + + // An empty ref ("owner/repo@") explicitly opts out of pinning: the skill is + // installed from the repository's default branch. This is allowed, but + // triggers a compile-time warning recommending an explicit ref (see + // emitSkillPinningWarnings). + if ref == "" { + return nil + } + + if gitutil.IsValidFullSHA(ref) { + return nil + } + + if looksLikeAmbiguousSHA(ref) { + return fmt.Errorf( + "skills[%d] ref %q looks like a truncated or malformed commit SHA (got %q); use the full 40-character lowercase SHA or a branch/tag name", + idx, + ref, + skillSpec, + ) + } + + if !skillRefCharsRegexp.MatchString(ref) || strings.Contains(ref, "..") { + return fmt.Errorf( + "skills[%d] ref %q contains unsupported characters; refs may only contain letters, digits, '.', '_', '-', and '/', must start with a letter or digit, and must not contain '..' (got %q)", + idx, + ref, + skillSpec, + ) + } + return nil } diff --git a/pkg/workflow/skills_frontmatter_test.go b/pkg/workflow/skills_frontmatter_test.go index 6b4e3f1799e..241583d2f56 100644 --- a/pkg/workflow/skills_frontmatter_test.go +++ b/pkg/workflow/skills_frontmatter_test.go @@ -52,14 +52,44 @@ func TestValidateFrontmatterSkills(t *testing.T) { require.ErrorContains(t, err, "without '..' traversal segments") }) - t.Run("rejects non-sha refs", func(t *testing.T) { + t.Run("accepts non-sha refs (branch/tag)", func(t *testing.T) { err := validateFrontmatterSkills(map[string]any{ "skills": []any{ "githubnext/skills@main", + "githubnext/skills/review/security@v1.2.3", + "githubnext/skills@release/1.0", + }, + }) + require.NoError(t, err) + }) + + t.Run("accepts remote spec with no ref specified", func(t *testing.T) { + err := validateFrontmatterSkills(map[string]any{ + "skills": []any{ + "githubnext/skills@", + "githubnext/skills/review/security@", + }, + }) + require.NoError(t, err) + }) + + t.Run("rejects invalid remote spec shape", func(t *testing.T) { + err := validateFrontmatterSkills(map[string]any{ + "skills": []any{ + "owner@main", + }, + }) + require.Error(t, err) + require.ErrorContains(t, err, "owner/repo@") + }) + + t.Run("rejects ref with unsafe characters", func(t *testing.T) { + err := validateFrontmatterSkills(map[string]any{ + "skills": []any{ + "githubnext/skills@main; rm -rf /", }, }) require.Error(t, err) - require.ErrorContains(t, err, "40-char-sha") }) t.Run("rejects 39-char sha", func(t *testing.T) { @@ -88,7 +118,7 @@ func TestValidateFrontmatterSkills(t *testing.T) { }, }) require.Error(t, err) - require.ErrorContains(t, err, "40-char-sha") + require.ErrorContains(t, err, "does not support expressions") }) t.Run("accepts empty skills array", func(t *testing.T) { diff --git a/pkg/workflow/skills_ref_resolution.go b/pkg/workflow/skills_ref_resolution.go new file mode 100644 index 00000000000..1ff1d71d7e0 --- /dev/null +++ b/pkg/workflow/skills_ref_resolution.go @@ -0,0 +1,97 @@ +package workflow + +import ( + "context" + "fmt" + "os" + "strings" + + "github.com/github/gh-aw/pkg/gitutil" +) + +// resolveFrontmatterSkillRefs pins non-SHA remote skill refs (owner/repo[/path]@ref) to +// their resolved commit SHA at compile time, using the compiler's shared action resolver +// (the same GitHub API/cache infrastructure used to pin "uses:" action references). Refs +// that are already a full 40-character SHA are left untouched. Remote skill entries with +// no ref specified (owner/repo[/path]@) are left unpinned and trigger an advisory warning +// recommending an explicit ref. Local skill paths and GitHub Actions expressions are +// ignored. +// +// data.SkillReferences and data.Skills are always populated together from the same +// frontmatter "skills" entries (in the same order), so resolving SkillReferences and +// mirroring the result into Skills keeps both in sync without resolving each entry twice. +func (c *Compiler) resolveFrontmatterSkillRefs(data *WorkflowData, markdownPath string) { + if data == nil { + return + } + if len(data.SkillReferences) > 0 { + for i := range data.SkillReferences { + data.SkillReferences[i].Skill = c.resolveSkillRefSpec(data, markdownPath, data.SkillReferences[i].Skill, i) + } + for i := range data.Skills { + if i < len(data.SkillReferences) { + data.Skills[i] = data.SkillReferences[i].Skill + } + } + return + } + for i := range data.Skills { + data.Skills[i] = c.resolveSkillRefSpec(data, markdownPath, data.Skills[i], i) + } +} + +// resolveSkillRefSpec resolves a single skills[] entry, returning the (possibly +// SHA-pinned) spec to use going forward. It never returns an error: resolution +// failures degrade to a warning and the original, unpinned spec is kept so +// compilation can proceed. +func (c *Compiler) resolveSkillRefSpec(data *WorkflowData, markdownPath, spec string, idx int) string { + trimmed := strings.TrimSpace(spec) + if trimmed == "" || strings.HasPrefix(trimmed, "${{") || isLocalSkillRef(trimmed) { + return spec + } + + repoPath, ref, hasAt := strings.Cut(trimmed, "@") + if !hasAt { + return spec + } + + if ref == "" { + fmt.Fprintln(os.Stderr, formatCompilerMessage(markdownPath, "warning", + fmt.Sprintf( + "skills[%d] %q has no ref pinned; the skill will be installed from the repository's default branch on every run. "+ + "Pin a branch, tag, or commit SHA for reproducible builds, e.g. skills[%d]: \"%s@main\".", + idx, trimmed, idx, repoPath))) + c.IncrementWarningCount() + return spec + } + + if gitutil.IsValidFullSHA(ref) { + skillsFrontmatterLog.Printf("skills[%d] %q is already SHA-pinned", idx, trimmed) + return spec + } + + if data.ActionResolver == nil { + skillsFrontmatterLog.Printf("skills[%d]: no action resolver available, skipping SHA pinning for %q", idx, trimmed) + return spec + } + + ctx := data.Ctx + if ctx == nil { + ctx = context.Background() + } + + sha, err := data.ActionResolver.ResolveSHA(ctx, repoPath, ref) + if err != nil { + skillsFrontmatterLog.Printf("skills[%d]: failed to resolve ref %q for %q to a SHA: %v", idx, ref, repoPath, err) + fmt.Fprintln(os.Stderr, formatCompilerMessage(markdownPath, "warning", + fmt.Sprintf( + "skills[%d]: failed to resolve ref %q for %q to a commit SHA (%v); the workflow will use the unpinned ref as-is.", + idx, ref, repoPath, err))) + c.IncrementWarningCount() + return spec + } + + pinned := fmt.Sprintf("%s@%s", repoPath, sha) + skillsFrontmatterLog.Printf("skills[%d]: pinned %q to %q", idx, trimmed, pinned) + return pinned +} diff --git a/pkg/workflow/skills_ref_resolution_test.go b/pkg/workflow/skills_ref_resolution_test.go new file mode 100644 index 00000000000..962a496f9f1 --- /dev/null +++ b/pkg/workflow/skills_ref_resolution_test.go @@ -0,0 +1,127 @@ +//go:build !integration + +package workflow + +import ( + "bytes" + "context" + "os" + "strings" + "testing" + + "github.com/github/gh-aw/pkg/testutil" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func newTestSkillWorkflowData(skills []string) *WorkflowData { + refs := make([]SkillReference, 0, len(skills)) + for _, s := range skills { + refs = append(refs, SkillReference{Skill: s}) + } + return &WorkflowData{ + Skills: append([]string(nil), skills...), + SkillReferences: refs, + Ctx: context.Background(), + } +} + +func withCapturedStderr(t *testing.T, fn func()) string { + t.Helper() + old := os.Stderr + r, w, err := os.Pipe() + require.NoError(t, err) + os.Stderr = w + defer func() { + os.Stderr = old + _ = w.Close() + }() + + fn() + + require.NoError(t, w.Close()) + var buf bytes.Buffer + _, _ = buf.ReadFrom(r) + return buf.String() +} + +func TestResolveFrontmatterSkillRefs_PinsNonSHARefUsingCache(t *testing.T) { + tmpDir := testutil.TempDir(t, "skill-ref-cache") + cache := NewActionCache(tmpDir) + resolver := NewActionResolver(cache) + const sha = "1f181b37d3fe5862ab590648f25a292e345b5de6" + cache.Set("githubnext/skills", "main", sha) + + compiler := NewCompiler(WithVersion("dev")) + data := newTestSkillWorkflowData([]string{"githubnext/skills@main"}) + data.ActionResolver = resolver + + output := withCapturedStderr(t, func() { + compiler.resolveFrontmatterSkillRefs(data, "workflow.md") + }) + + assert.Equal(t, "githubnext/skills@"+sha, data.Skills[0]) + assert.Equal(t, "githubnext/skills@"+sha, data.SkillReferences[0].Skill) + assert.Empty(t, strings.TrimSpace(output), "no warning expected when resolution succeeds") +} + +func TestResolveFrontmatterSkillRefs_LeavesFullSHAUnchanged(t *testing.T) { + compiler := NewCompiler(WithVersion("dev")) + const sha = "1f181b37d3fe5862ab590648f25a292e345b5de6" + data := newTestSkillWorkflowData([]string{"githubnext/skills@" + sha}) + + output := withCapturedStderr(t, func() { + compiler.resolveFrontmatterSkillRefs(data, "workflow.md") + }) + + assert.Equal(t, "githubnext/skills@"+sha, data.Skills[0]) + assert.Empty(t, strings.TrimSpace(output)) +} + +func TestResolveFrontmatterSkillRefs_WarnsWhenNoRefSpecified(t *testing.T) { + compiler := NewCompiler(WithVersion("dev")) + data := newTestSkillWorkflowData([]string{"githubnext/skills@"}) + + output := withCapturedStderr(t, func() { + compiler.resolveFrontmatterSkillRefs(data, "workflow.md") + }) + + // Unpinned spec is kept as-is. + assert.Equal(t, "githubnext/skills@", data.Skills[0]) + assert.Contains(t, output, "has no ref pinned") + assert.Contains(t, output, "warning") + assert.Equal(t, 1, compiler.GetWarningCount()) +} + +func TestResolveFrontmatterSkillRefs_LeavesLocalPathUnchanged(t *testing.T) { + compiler := NewCompiler(WithVersion("dev")) + data := newTestSkillWorkflowData([]string{"skills/rig"}) + + output := withCapturedStderr(t, func() { + compiler.resolveFrontmatterSkillRefs(data, "workflow.md") + }) + + assert.Equal(t, "skills/rig", data.Skills[0]) + assert.Empty(t, strings.TrimSpace(output)) +} + +func TestResolveFrontmatterSkillRefs_WarnsAndKeepsUnpinnedRefOnResolutionFailure(t *testing.T) { + tmpDir := testutil.TempDir(t, "skill-ref-cache-fail") + cache := NewActionCache(tmpDir) + resolver := NewActionResolver(cache) + // Mark this resolution as already failed so ResolveSHA short-circuits without + // attempting a network call. + resolver.failedResolutions[formatActionCacheKey("githubnext/does-not-exist", "no-such-ref")] = struct{}{} + + compiler := NewCompiler(WithVersion("dev")) + data := newTestSkillWorkflowData([]string{"githubnext/does-not-exist@no-such-ref"}) + data.ActionResolver = resolver + + output := withCapturedStderr(t, func() { + compiler.resolveFrontmatterSkillRefs(data, "workflow.md") + }) + + assert.Equal(t, "githubnext/does-not-exist@no-such-ref", data.Skills[0]) + assert.Contains(t, output, "failed to resolve ref") + assert.Equal(t, 1, compiler.GetWarningCount()) +}