Skip to content

Auto-pin unversioned action uses refs in compiler; fail compilation when no pin is available #40475

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
pelikhan merged 9 commits into from
Jun 20, 2026
Merged

Auto-pin unversioned action uses refs in compiler; fail compilation when no pin is available #40475

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
a6c799e
Fix skillet checkout action reference
Copilot Jun 20, 2026
51f6e8f
Auto-pin unversioned action uses during compile
Copilot Jun 20, 2026
1bb2fb0
Clarify variable name in unversioned pinning path
Copilot Jun 20, 2026
bb942f6
Refine naming in action pin auto-fix updates
Copilot Jun 20, 2026
fdce4aa
Polish naming around unversioned action pinning
Copilot Jun 20, 2026
2223db6
Simplify naming in unversioned pin path
Copilot Jun 20, 2026
0a1d9be
Clarify log for unversioned action pin fallback
Copilot Jun 20, 2026
af624cb
Fail compilation for unversioned actions with no available pin
Copilot Jun 20, 2026
b4149bd
Fix test helper to use getCachedActionPin; pin skillet.md checkout to…
Copilot Jun 20, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 2 additions & 3 deletions .github/workflows/skillet.lock.yml
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion .github/workflows/skillet.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ jobs:
pre-activation:
pre-steps:
- name: Checkout skills directory
uses: actions/checkout
uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0
with:
sparse-checkout: |
.github/skills
Expand Down
38 changes: 27 additions & 11 deletions pkg/workflow/action_pins.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package workflow

import (
"fmt"
"strings"

actionpins "github.com/github/gh-aw/pkg/actionpins"
Expand Down Expand Up @@ -198,19 +199,29 @@ func getCachedActionPin(repo string, data *WorkflowData) string {
// applyActionPinToTypedStep applies SHA pinning to a WorkflowStep if it uses an action.
// Returns a modified copy of the step with pinned references.
// If the step doesn't use an action or the action is not pinned, returns the original step.
func applyActionPinToTypedStep(step *WorkflowStep, data *WorkflowData) *WorkflowStep {
// Returns an error if the step uses an unversioned action reference with no available pin,
// because emitting such a reference would produce invalid GitHub Actions workflow syntax.
func applyActionPinToTypedStep(step *WorkflowStep, data *WorkflowData) (*WorkflowStep, error) {
if step == nil || !step.IsUsesStep() {
return step
return step, nil
}

actionRepo := extractActionRepo(step.Uses)
if actionRepo == "" {
return step
return step, nil
}

version := extractActionVersion(step.Uses)
if version == "" {
return step
pin := getCachedActionPin(actionRepo, data)
if pin == "" {
return nil, fmt.Errorf("unversioned action %q has no available pin; add a @ref (e.g. @v1) or include it in action-pins.json", actionRepo)
}

actionPinsLog.Printf("Pinned action: %s (no ref) -> %s", actionRepo, pin)
result := step.Clone()
result.Uses = pin
return result, nil
}

// Strip the comment suffix before checking if it's already a SHA.
Expand All @@ -220,20 +231,21 @@ func applyActionPinToTypedStep(step *WorkflowStep, data *WorkflowData) *Workflow
pinnedRef, err := getActionPinWithData(actionRepo, rawVersion, data)
if err != nil || pinnedRef == "" {
actionPinsLog.Printf("Skipping pin for %s@%s: no pin available", actionRepo, rawVersion)
return step
return step, nil
}

actionPinsLog.Printf("Pinned action: %s@%s -> %s", actionRepo, rawVersion, pinnedRef)
result := step.Clone()
result.Uses = pinnedRef
return result
return result, nil
}

// applyActionPinsToTypedSteps applies SHA pinning to a slice of typed WorkflowStep pointers.
// Returns a new slice with pinned references.
func applyActionPinsToTypedSteps(steps []*WorkflowStep, data *WorkflowData) []*WorkflowStep {
// Returns a new slice with pinned references, or an error if any step has an unversioned
// action reference with no available pin.
func applyActionPinsToTypedSteps(steps []*WorkflowStep, data *WorkflowData) ([]*WorkflowStep, error) {
if steps == nil {
return nil
return nil, nil
}

result := make([]*WorkflowStep, 0, len(steps))
Expand All @@ -242,7 +254,11 @@ func applyActionPinsToTypedSteps(steps []*WorkflowStep, data *WorkflowData) []*W
result = append(result, nil)
continue
}
result = append(result, applyActionPinToTypedStep(step, data))
pinned, err := applyActionPinToTypedStep(step, data)
if err != nil {
return nil, err
}
result = append(result, pinned)
}
return result
return result, nil
}
110 changes: 101 additions & 9 deletions pkg/workflow/action_pins_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import (
const setupNodeV6ExpectedUsesPlaceholder = "__setup_node_v6__"
const checkoutV6ExpectedUsesPlaceholder = "__checkout_v6__"
const checkoutSHAExpectedUsesPlaceholder = "__checkout_sha__"
const checkoutLatestExpectedUsesPlaceholder = "__checkout_latest__"

func expectedPinnedUses(t *testing.T, repo, version string) string {
t.Helper()
Expand All @@ -29,6 +30,16 @@ func expectedPinnedUses(t *testing.T, repo, version string) string {
return result
}

func latestPinnedUsesForRepo(t *testing.T, repo string) string {

@github-actions github-actions Bot Jun 20, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[/tdd] latestPinnedUsesForRepo resolves via getLatestActionPinByRepo → getActionPinWithData — the same call chain used by the implementation under test. This makes the assertions circular: they verify that the output matches itself, not a known-good SHA.

💡 Suggestion

Either snapshot a fixed SHA in the test constant (updating it when pins rotate) or, at minimum, assert that the returned string matches the {repo}@{40-char-sha} # {version} format:

if !strings.HasPrefix(result, "actions/checkout@") {
    t.Errorf("expected pinned SHA reference, got %q", result)
}

A format-only assertion is still far stronger than nothing — it catches the regression where unversioned refs pass through unchanged.

Copilot AI Jun 20, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in the latest commit: latestPinnedUsesForRepo now calls getCachedActionPin(repo, &WorkflowData{}) directly, matching the production code path.

t.Helper()

result := getCachedActionPin(repo, &WorkflowData{})
if result == "" {
t.Fatalf("getCachedActionPin(%s) returned empty", repo)
}
return result
}

// TestGetActionPinFallback tests that getActionPin returns empty string for unknown actions
func TestGetActionPinFallback(t *testing.T) {
result := getActionPin("unknown/action")
Expand Down Expand Up @@ -173,6 +184,15 @@ func TestApplyActionPinToStep(t *testing.T) {
expectPinned: false,
expectedUses: "my-org/my-action@v1",
},
{
name: "step with unversioned action",
stepMap: map[string]any{
"name": "Checkout",
"uses": "actions/checkout",
},
expectPinned: true,
expectedUses: checkoutLatestExpectedUsesPlaceholder,
},
{
name: "step without uses field",
stepMap: map[string]any{
Expand Down Expand Up @@ -205,7 +225,10 @@ func TestApplyActionPinToStep(t *testing.T) {
}

// Apply action pinning using typed version
pinnedStep := applyActionPinToTypedStep(typedStep, data)
pinnedStep, pinErr := applyActionPinToTypedStep(typedStep, data)
if pinErr != nil {
t.Fatalf("applyActionPinToTypedStep() returned error: %v", pinErr)
}
if pinnedStep == nil {
t.Fatal("applyActionPinToTypedStep returned nil")
}
Expand All @@ -231,6 +254,9 @@ func TestApplyActionPinToStep(t *testing.T) {
if expectedUses == checkoutSHAExpectedUsesPlaceholder {
expectedUses = expectedPinnedUses(t, "actions/checkout", "de0fac2e4500dabe0009e67214ff5f5447ce83dd")
}
if expectedUses == checkoutLatestExpectedUsesPlaceholder {
expectedUses = latestPinnedUsesForRepo(t, "actions/checkout")
}
if usesStr != expectedUses {
t.Errorf("applyActionPinToTypedStep uses = %q, want %q", usesStr, expectedUses)
}
Expand Down Expand Up @@ -319,6 +345,7 @@ func TestApplyActionPinToTypedStep(t *testing.T) {
step *WorkflowStep
expectPinned bool
expectedUses string
wantErr bool
}{
{
name: "step with pinned action (checkout)",
Expand Down Expand Up @@ -350,6 +377,23 @@ func TestApplyActionPinToTypedStep(t *testing.T) {
expectPinned: false,
expectedUses: "my-org/my-action@v1",
},
{
name: "step with unversioned action in embedded pins",
step: &WorkflowStep{
Name: "Checkout",
Uses: "actions/checkout",
},
expectPinned: true,
expectedUses: checkoutLatestExpectedUsesPlaceholder,

@github-actions github-actions Bot Jun 20, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

No test for the cachedPin == "" failure branch: both new test cases only exercise the happy path (actions/checkout is always in embedded pins), so the if cachedPin == "" { return step } branch in applyActionPinToTypedStep is never hit.

💡 Suggested fix

Add a case for an action that is NOT in the embedded pin set, in both TestApplyActionPinToStep and TestApplyActionPinToTypedStep:

{
    name: "step with unversioned action not in embedded pins",
    step: &WorkflowStep{
        Name: "Unknown Action",
        Uses: "unknown-org/not-in-pins-action",
    },
    expectPinned: false,
    expectedUses: "unknown-org/not-in-pins-action",
},

This exercises the fallback path and records the current accepted behavior: an unversioned uses: ref with no available pin is returned unchanged (still invalid GitHub Actions syntax). Without it, a future regression in that fallthrough — say accidentally surfacing an error or mutating the ref — is invisible to CI. The same case should be added to the map-based TestApplyActionPinToStep variant.

Copilot AI Jun 20, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The error path is already covered: TestApplyActionPinToTypedStep has a "step with unversioned action not in pins returns error" case (Uses: "my-org/my-unpinned-action", wantErr: true), and TestApplyActionPinsToTypedSteps has the same via "unversioned action not in pins returns error".

},

@github-actions github-actions Bot Jun 20, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[/tdd] The two new test cases only cover the happy path (a known embedded action). The skip path — where getCachedActionPin returns "" because the action is unknown — has no test. Without a regression test, a future change could accidentally start emitting unversioned refs for unknown actions.

💡 Suggested table entry 
{
    name: "step with unversioned unknown action is unchanged",
    step: &WorkflowStep{
        Name: "Deploy",
        Uses: "my-org/private-action",
    },
    expectPinned: false,
    expectedUses: "my-org/private-action",
},

This documents the current graceful-skip contract and prevents it from silently breaking.

Copilot AI Jun 20, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The behavior now returns an error for unversioned actions not in the pin set (rather than passing through). The error path is covered by the "step with unversioned action not in pins returns error" test case in TestApplyActionPinToTypedStep.

{
name: "step with unversioned action not in pins returns error",
step: &WorkflowStep{
Name: "Custom action",
Uses: "my-org/my-unpinned-action",
},
wantErr: true,
},
{
name: "step without uses field",
step: &WorkflowStep{
Expand Down Expand Up @@ -388,15 +432,29 @@ func TestApplyActionPinToTypedStep(t *testing.T) {
// Create a test WorkflowData
data := &WorkflowData{}

result := applyActionPinToTypedStep(tt.step, data)
result, err := applyActionPinToTypedStep(tt.step, data)

if tt.step == nil {
if err != nil {
t.Errorf("applyActionPinToTypedStep(nil) returned error: %v", err)
}
if result != nil {
t.Errorf("applyActionPinToTypedStep(nil) = %v, want nil", result)
}
return
}

if tt.wantErr {
if err == nil {
t.Errorf("applyActionPinToTypedStep() expected error, got nil")
}
return
}

if err != nil {
t.Fatalf("applyActionPinToTypedStep() returned error: %v", err)
}

if result == nil {
t.Fatalf("applyActionPinToTypedStep() returned nil")
}
Expand All @@ -409,6 +467,9 @@ func TestApplyActionPinToTypedStep(t *testing.T) {
if expectedUses == checkoutV6ExpectedUsesPlaceholder {
expectedUses = expectedPinnedUses(t, "actions/checkout", "v6")
}
if expectedUses == checkoutLatestExpectedUsesPlaceholder {
expectedUses = latestPinnedUsesForRepo(t, "actions/checkout")
}
if result.Uses != expectedUses {
t.Errorf("applyActionPinToTypedStep() uses = %q, want %q", result.Uses, expectedUses)
}
Expand Down Expand Up @@ -453,7 +514,10 @@ func TestApplyActionPinToTypedStep_Immutability(t *testing.T) {
originalUses := originalStep.Uses

data := &WorkflowData{}
result := applyActionPinToTypedStep(originalStep, data)
result, err := applyActionPinToTypedStep(originalStep, data)
if err != nil {
t.Fatalf("applyActionPinToTypedStep() returned error: %v", err)
}

// Verify the original step was not modified
if originalStep.Uses != originalUses {
Expand Down Expand Up @@ -649,9 +713,10 @@ func TestApplyActionPinsToTypedSteps(t *testing.T) {
}

tests := []struct {
name string
steps []*WorkflowStep
want []*WorkflowStep
name string
steps []*WorkflowStep
want []*WorkflowStep
wantErr bool
}{
{
name: "nil steps",
Expand Down Expand Up @@ -725,11 +790,32 @@ func TestApplyActionPinsToTypedSteps(t *testing.T) {
},
},
},
{
name: "unversioned action not in pins returns error",
steps: []*WorkflowStep{
{
Name: "Unknown action",
Uses: "my-org/my-unpinned-action",
},
},
wantErr: true,
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
got := applyActionPinsToTypedSteps(tt.steps, data)
got, err := applyActionPinsToTypedSteps(tt.steps, data)

if tt.wantErr {
if err == nil {
t.Errorf("applyActionPinsToTypedSteps() expected error, got nil")
}
return
}

if err != nil {
t.Fatalf("applyActionPinsToTypedSteps() returned unexpected error: %v", err)
}

if len(got) != len(tt.want) {
t.Errorf("applyActionPinsToTypedSteps() returned %d steps, want %d", len(got), len(tt.want))
Expand Down Expand Up @@ -1224,7 +1310,10 @@ func TestMapToStepWithActionPinning(t *testing.T) {
}

// Apply action pinning using typed version
pinnedStep := applyActionPinToTypedStep(typedStep, data)
pinnedStep, pinErr := applyActionPinToTypedStep(typedStep, data)
if pinErr != nil {
t.Fatalf("applyActionPinToTypedStep() returned error: %v", pinErr)
}
if pinnedStep == nil {
t.Fatal("applyActionPinToTypedStep returned nil")
}
Expand Down Expand Up @@ -1328,7 +1417,10 @@ func TestSliceToStepsWithActionPinning(t *testing.T) {
}

// Apply action pinning using typed version
pinnedSteps := applyActionPinsToTypedSteps(typedSteps, data)
pinnedSteps, pinErr := applyActionPinsToTypedSteps(typedSteps, data)
if pinErr != nil {
t.Fatalf("applyActionPinsToTypedSteps() returned unexpected error: %v", pinErr)
}
if len(pinnedSteps) != len(typedSteps) {
t.Errorf("applyActionPinsToTypedSteps() returned %d steps, want %d", len(pinnedSteps), len(typedSteps))
}
Expand Down
5 changes: 4 additions & 1 deletion pkg/workflow/compiler_jobs.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1216,7 +1216,10 @@ func (c *Compiler) extractPinnedJobSteps(fieldName string, jobName string, confi
return nil, fmt.Errorf("failed to convert %s to typed step for job '%s': %w", fieldName, jobName, err)
}

pinnedStep := applyActionPinToTypedStep(typedStep, data)
pinnedStep, err := applyActionPinToTypedStep(typedStep, data)
if err != nil {
return nil, fmt.Errorf("failed to pin action for %s in job '%s': %w", fieldName, jobName, err)
}
finalStepMap := pinnedStep.ToMap()
sanitizedMap, warnings, _ := sanitizeRunStepExpressions(finalStepMap)
for _, w := range warnings {
Expand Down
21 changes: 16 additions & 5 deletions pkg/workflow/compiler_orchestrator_workflow.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -196,10 +196,18 @@ func (c *Compiler) populateWorkflowBuildContext(ctx *workflowBuildContext) error
if err := c.mergeImportedWorkflowConfiguration(ctx); err != nil {
return err
}
c.processAndMergeSteps(ctx.frontmatter.Frontmatter, ctx.workflowData, ctx.engineSetup.importsResult)
c.processAndMergePreSteps(ctx.frontmatter.Frontmatter, ctx.workflowData, ctx.engineSetup.importsResult)
c.processAndMergePreAgentSteps(ctx.frontmatter.Frontmatter, ctx.workflowData, ctx.engineSetup.importsResult)
c.processAndMergePostSteps(ctx.frontmatter.Frontmatter, ctx.workflowData, ctx.engineSetup.importsResult)
if err := c.processAndMergeSteps(ctx.frontmatter.Frontmatter, ctx.workflowData, ctx.engineSetup.importsResult); err != nil {
return formatCompilerError(ctx.cleanPath, "error", err.Error(), err)
}
if err := c.processAndMergePreSteps(ctx.frontmatter.Frontmatter, ctx.workflowData, ctx.engineSetup.importsResult); err != nil {
return formatCompilerError(ctx.cleanPath, "error", err.Error(), err)
}
if err := c.processAndMergePreAgentSteps(ctx.frontmatter.Frontmatter, ctx.workflowData, ctx.engineSetup.importsResult); err != nil {
return formatCompilerError(ctx.cleanPath, "error", err.Error(), err)
}
if err := c.processAndMergePostSteps(ctx.frontmatter.Frontmatter, ctx.workflowData, ctx.engineSetup.importsResult); err != nil {
return formatCompilerError(ctx.cleanPath, "error", err.Error(), err)
}
c.processAndMergeServices(ctx.frontmatter.Frontmatter, ctx.workflowData, ctx.engineSetup.importsResult)
ctx.workflowData.KnownActionCredentialEnvVars = DetectKnownCredentialLeakingActionsFromWorkflowData(ctx.workflowData)
if err := c.extractAdditionalConfigurations(ctx.frontmatter.Frontmatter, ctx.toolsResult.tools, ctx.markdownDir, ctx.workflowData, ctx.engineSetup.importsResult, ctx.toolsResult.rawMainMarkdown, ctx.toolsResult.safeOutputs); err != nil {
Expand Down Expand Up @@ -661,7 +669,10 @@ func (c *Compiler) processOnSectionAndFilters(
}
typedSteps, convErr := SliceToSteps(anySteps)
if convErr == nil {
typedSteps = applyActionPinsToTypedSteps(typedSteps, workflowData)
typedSteps, convErr = applyActionPinsToTypedSteps(typedSteps, workflowData)
if convErr != nil {
return fmt.Errorf("on.steps: %w", convErr)
}
for i, s := range typedSteps {
onSteps[i] = s.ToMap()
}
Expand Down
Loading
Loading