diff --git a/actions/setup/md/stale_lock_file_failed.md b/actions/setup/md/stale_lock_file_failed.md index 1eb136a1daa..8a6eb026a95 100644 --- a/actions/setup/md/stale_lock_file_failed.md +++ b/actions/setup/md/stale_lock_file_failed.md @@ -45,6 +45,13 @@ The workflow run logs contain a verbose debug pass that shows exactly what was h This makes it easy to spot accidental whitespace changes, encoding differences, or import path drift. +This mismatch can also happen on a **fresh install** (even without any manual edits) if `gh aw add @` could not resolve the ref to an exact commit SHA during installation. In that case, rerun the add command with an exact SHA (or retry when API/rate-limit conditions recover), then recompile: + +```bash +gh aw add @ +gh aw compile +``` +
diff --git a/cmd/gh-aw/main.go b/cmd/gh-aw/main.go index 068e182d0ce..e120827cb72 100644 --- a/cmd/gh-aw/main.go +++ b/cmd/gh-aw/main.go @@ -276,6 +276,7 @@ Examples: forceOverwrite, _ := cmd.Flags().GetBool("force") refreshStopTime, _ := cmd.Flags().GetBool("refresh-stop-time") forceRefreshActionPins, _ := cmd.Flags().GetBool("force-refresh-action-pins") + allowActionRefs, _ := cmd.Flags().GetBool("allow-action-refs") zizmor, _ := cmd.Flags().GetBool("zizmor") poutine, _ := cmd.Flags().GetBool("poutine") actionlint, _ := cmd.Flags().GetBool("actionlint") @@ -335,6 +336,7 @@ Examples: ForceOverwrite: forceOverwrite, RefreshStopTime: refreshStopTime, ForceRefreshActionPins: forceRefreshActionPins, + AllowActionRefs: allowActionRefs, Zizmor: zizmor, Poutine: poutine, Actionlint: actionlint, @@ -684,6 +686,7 @@ Use "` + string(constants.CLIExtensionPrefix) + ` help all" to show help for all compileCmd.Flags().Bool("force", false, "Force overwrite of existing dependency files (e.g., dependabot.yml)") compileCmd.Flags().Bool("refresh-stop-time", false, "Force regeneration of stop-after times instead of preserving existing values from lock files") compileCmd.Flags().Bool("force-refresh-action-pins", false, "Force refresh of action pins by clearing the cache and resolving all action SHAs from GitHub API") + compileCmd.Flags().Bool("allow-action-refs", false, "Allow unresolved action refs and emit warnings instead of failing compilation") compileCmd.Flags().Bool("zizmor", false, "Run zizmor security scanner on generated .lock.yml files") compileCmd.Flags().Bool("poutine", false, "Run poutine security scanner on generated .lock.yml files") compileCmd.Flags().Bool("actionlint", false, "Run actionlint linter on generated .lock.yml files") diff --git a/pkg/actionpins/actionpins.go b/pkg/actionpins/actionpins.go index 7bc0b3d7291..2a560b27421 100644 --- a/pkg/actionpins/actionpins.go +++ b/pkg/actionpins/actionpins.go @@ -49,6 +49,23 @@ type SHAResolver interface { ResolveSHA(repo, version string) (string, error) } +// ResolutionErrorType classifies unresolved action-ref pinning outcomes for auditing. +type ResolutionErrorType string + +const ( + // ResolutionErrorTypeDynamicResolutionFailed indicates dynamic tag/ref -> SHA resolution failed. + ResolutionErrorTypeDynamicResolutionFailed ResolutionErrorType = "dynamic_resolution_failed" + // ResolutionErrorTypePinNotFound indicates no usable hardcoded pin was found for the ref. + ResolutionErrorTypePinNotFound ResolutionErrorType = "pin_not_found" +) + +// ResolutionFailure captures an unresolved action-ref pinning event. +type ResolutionFailure struct { + Repo string + Ref string + ErrorType ResolutionErrorType +} + // PinContext provides the runtime context needed for action pin resolution. // Callers construct one from their own state (e.g. WorkflowData fields). // The Warnings map is mutated in place to deduplicate warning output. @@ -57,9 +74,16 @@ type PinContext struct { Resolver SHAResolver // StrictMode controls how resolution failures are handled. StrictMode bool + // EnforcePinned requires unresolved refs to fail unless AllowActionRefs is true. + EnforcePinned bool + // AllowActionRefs lowers unresolved pinning failures to warnings. + // When false, unresolved action refs return an error. + AllowActionRefs bool // Warnings is a shared map for deduplicating warning messages. // Keys are cache keys in the form "repo@version". Warnings map[string]bool + // RecordResolutionFailure receives unresolved pinning failures for auditing. + RecordResolutionFailure func(f ResolutionFailure) } var ( @@ -210,6 +234,17 @@ func findCompatiblePin(pins []ActionPin, version string) (ActionPin, bool) { return ActionPin{}, false } +func notifyResolutionFailure(ctx *PinContext, actionRepo, version string, errorType ResolutionErrorType) { + if ctx == nil || ctx.RecordResolutionFailure == nil { + return + } + ctx.RecordResolutionFailure(ResolutionFailure{ + Repo: actionRepo, + Ref: version, + ErrorType: errorType, + }) +} + // ResolveActionPin returns the pinned action reference for a given action@version. // It consults ctx.Resolver first, then falls back to embedded pins. // If ctx is nil, only embedded pins are consulted. @@ -301,6 +336,18 @@ func ResolveActionPin(actionRepo, version string, ctx *PinContext) (string, erro ctx.Warnings = make(map[string]bool) } cacheKey := FormatCacheKey(actionRepo, version) + errorType := ResolutionErrorTypePinNotFound + if ctx.Resolver != nil { + errorType = ResolutionErrorTypeDynamicResolutionFailed + } + notifyResolutionFailure(ctx, actionRepo, version, errorType) + if ctx.EnforcePinned && !ctx.AllowActionRefs { + if ctx.Resolver != nil { + return "", fmt.Errorf("unable to pin action %s@%s: resolution failed", actionRepo, version) + } + return "", fmt.Errorf("unable to pin action %s@%s", actionRepo, version) + } + if !ctx.Warnings[cacheKey] { warningMsg := fmt.Sprintf("Unable to pin action %s@%s", actionRepo, version) if ctx.Resolver != nil { diff --git a/pkg/cli/add_command.go b/pkg/cli/add_command.go index 688fcf7bc74..8a105dc8511 100644 --- a/pkg/cli/add_command.go +++ b/pkg/cli/add_command.go @@ -118,7 +118,7 @@ Note: For guided interactive setup, use the 'add-wizard' command instead.`, StopAfter: stopAfter, DisableSecurityScanner: disableSecurityScanner, } - _, err := AddWorkflows(workflows, opts) + _, err := AddWorkflows(cmd.Context(), workflows, opts) return err }, } @@ -172,9 +172,9 @@ Note: For guided interactive setup, use the 'add-wizard' command instead.`, // AddWorkflows adds one or more workflows from components to .github/workflows // with optional repository installation and PR creation. // Returns AddWorkflowsResult containing PR number (if created) and other metadata. -func AddWorkflows(workflows []string, opts AddOptions) (*AddWorkflowsResult, error) { +func AddWorkflows(ctx context.Context, workflows []string, opts AddOptions) (*AddWorkflowsResult, error) { // Resolve workflows first - fetches content directly from GitHub - resolved, err := ResolveWorkflows(workflows, opts.Verbose) + resolved, err := ResolveWorkflows(ctx, workflows, opts.Verbose) if err != nil { return nil, err } @@ -494,12 +494,18 @@ func addWorkflowWithTracking(resolved *ResolvedWorkflow, tracker *FileTracker, o if err := os.WriteFile(destFile, []byte(content), 0600); err != nil { return fmt.Errorf("failed to write destination file '%s': %w", destFile, err) } + // Read back the just-written file to ensure downstream processing (including + // frontmatter hash computation) uses the exact bytes on disk and avoids parity drift. + writtenContent, err := os.ReadFile(destFile) + if err != nil { + return fmt.Errorf("failed to read back destination file '%s': %w", destFile, err) + } // Show output if !opts.Quiet { fmt.Fprintln(os.Stderr, console.FormatSuccessMessage("Added workflow: "+destFile)) - if description := ExtractWorkflowDescription(content); description != "" { + if description := ExtractWorkflowDescription(string(writtenContent)); description != "" { fmt.Fprintln(os.Stderr, "") fmt.Fprintln(os.Stderr, console.FormatInfoMessage(description)) fmt.Fprintln(os.Stderr, "") diff --git a/pkg/cli/add_command_test.go b/pkg/cli/add_command_test.go index 7c0bcb1dd47..fb1ad142cdd 100644 --- a/pkg/cli/add_command_test.go +++ b/pkg/cli/add_command_test.go @@ -3,6 +3,7 @@ package cli import ( + "context" "testing" "github.com/spf13/cobra" @@ -99,7 +100,7 @@ func TestAddWorkflows(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { opts := AddOptions{} - _, err := AddWorkflows(tt.workflows, opts) + _, err := AddWorkflows(context.Background(), tt.workflows, opts) if tt.expectError { require.Error(t, err, "Expected error for test case: %s", tt.name) diff --git a/pkg/cli/add_current_repo_test.go b/pkg/cli/add_current_repo_test.go index d98dc92016d..433b2fc6246 100644 --- a/pkg/cli/add_current_repo_test.go +++ b/pkg/cli/add_current_repo_test.go @@ -3,6 +3,7 @@ package cli import ( + "context" "os" "os/exec" "strings" @@ -69,7 +70,7 @@ func TestAddWorkflowsFromCurrentRepository(t *testing.T) { ClearCurrentRepoSlugCache() opts := AddOptions{} - _, err := AddWorkflows(tt.workflowSpecs, opts) + _, err := AddWorkflows(context.Background(), tt.workflowSpecs, opts) if tt.expectError { if err == nil { @@ -153,7 +154,7 @@ func TestAddWorkflowsFromCurrentRepositoryMultiple(t *testing.T) { ClearCurrentRepoSlugCache() opts := AddOptions{} - _, err := AddWorkflows(tt.workflowSpecs, opts) + _, err := AddWorkflows(context.Background(), tt.workflowSpecs, opts) if tt.expectError { if err == nil { @@ -195,7 +196,7 @@ func TestAddWorkflowsFromCurrentRepositoryNotInGitRepo(t *testing.T) { // When not in a git repo, the check should be skipped (can't determine current repo) // The function should proceed and fail for other reasons (e.g., workflow not found) opts := AddOptions{} - _, err = AddWorkflows([]string{"some-owner/some-repo/workflow"}, opts) + _, err = AddWorkflows(context.Background(), []string{"some-owner/some-repo/workflow"}, opts) // Should NOT get the "cannot add workflows from the current repository" error if err != nil && strings.Contains(err.Error(), "cannot add workflows from the current repository") { diff --git a/pkg/cli/add_integration_test.go b/pkg/cli/add_integration_test.go index d9c34e138a6..de9555b6639 100644 --- a/pkg/cli/add_integration_test.go +++ b/pkg/cli/add_integration_test.go @@ -10,6 +10,8 @@ import ( "testing" "github.com/github/gh-aw/pkg/fileutil" + "github.com/github/gh-aw/pkg/parser" + "github.com/github/gh-aw/pkg/workflow" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -356,6 +358,47 @@ Please analyze the repository and provide a summary. assert.Contains(t, lockContentStr, "name: \"Test Local Workflow\"", "lock file should have workflow name") assert.Contains(t, lockContentStr, "workflow_dispatch", "lock file should have trigger") assert.Contains(t, lockContentStr, "jobs:", "lock file should have jobs section") + + // Verify frontmatter hash parity between source markdown and lock metadata. + computedHash, hashErr := parser.ComputeFrontmatterHashFromFile(destWorkflowFile, parser.NewImportCache(setup.tempDir)) + require.NoError(t, hashErr, "should compute frontmatter hash from added markdown file") + metadata, _, metadataErr := workflow.ExtractMetadataFromLockFile(lockContentStr) + require.NoError(t, metadataErr, "should extract lock metadata from compiled lock file") + require.NotNil(t, metadata, "lock metadata should be present") + assert.Equal(t, computedHash, metadata.FrontmatterHash, + "lock file frontmatter hash should match the hash recomputed from markdown file bytes") +} + +// TestAddRemoteWorkflowFailsWhenSHAResolutionFails tests that add fails loudly when ref-to-SHA +// resolution fails and does not write partial workflow artifacts. +func TestAddRemoteWorkflowFailsWhenSHAResolutionFails(t *testing.T) { + setup := setupAddIntegrationTest(t) + defer setup.cleanup() + + nonExistentWorkflowSpec := "github/gh-aw-does-not-exist/.github/workflows/not-real.md@main" + + cmd := exec.Command(setup.binaryPath, "add", nonExistentWorkflowSpec) + cmd.Dir = setup.tempDir + output, err := cmd.CombinedOutput() + outputStr := string(output) + + require.Error(t, err, "add command should fail when SHA resolution fails") + assert.Contains(t, outputStr, "failed to resolve 'main' to commit SHA", + "error output should clearly explain SHA resolution failure") + assert.Contains(t, outputStr, "Expected the GitHub API to return a commit SHA for the ref", + "error output should explain expected behavior") + assert.Contains(t, outputStr, "gh aw add github/gh-aw-does-not-exist/.github/workflows/not-real.md@<40-char-sha>", + "error output should provide a concrete retry example") + + workflowsDir := filepath.Join(setup.tempDir, ".github", "workflows") + workflowFile := filepath.Join(workflowsDir, "not-real.md") + lockFile := filepath.Join(workflowsDir, "not-real.lock.yml") + + _, workflowErr := os.Stat(workflowFile) + assert.True(t, os.IsNotExist(workflowErr), "workflow markdown file should not be written on SHA resolution failure") + + _, lockErr := os.Stat(lockFile) + assert.True(t, os.IsNotExist(lockErr), "lock file should not be written on SHA resolution failure") } // TestAddWorkflowWithCustomName tests adding a workflow with a custom name diff --git a/pkg/cli/add_interactive_orchestrator.go b/pkg/cli/add_interactive_orchestrator.go index 5911e37b3b7..48c22b49d61 100644 --- a/pkg/cli/add_interactive_orchestrator.go +++ b/pkg/cli/add_interactive_orchestrator.go @@ -167,7 +167,7 @@ func RunAddInteractive(ctx context.Context, workflowSpecs []string, verbose bool func (c *AddInteractiveConfig) resolveWorkflows() error { addInteractiveLog.Print("Resolving workflows early for description display") - resolved, err := ResolveWorkflows(c.WorkflowSpecs, c.Verbose) + resolved, err := ResolveWorkflows(c.Ctx, c.WorkflowSpecs, c.Verbose) if err != nil { return fmt.Errorf("failed to resolve workflows: %w", err) } diff --git a/pkg/cli/add_workflow_resolution.go b/pkg/cli/add_workflow_resolution.go index 646fa26188f..21a21e5e364 100644 --- a/pkg/cli/add_workflow_resolution.go +++ b/pkg/cli/add_workflow_resolution.go @@ -1,6 +1,7 @@ package cli import ( + "context" "errors" "fmt" "os" @@ -46,7 +47,7 @@ type ResolvedWorkflows struct { // ResolveWorkflows resolves workflow specifications by parsing specs and fetching workflow content. // For remote workflows, content is fetched directly from GitHub without cloning. // Wildcards are only supported for local workflows (not remote repositories). -func ResolveWorkflows(workflows []string, verbose bool) (*ResolvedWorkflows, error) { +func ResolveWorkflows(ctx context.Context, workflows []string, verbose bool) (*ResolvedWorkflows, error) { resolutionLog.Printf("Resolving workflows: count=%d", len(workflows)) if len(workflows) == 0 { @@ -117,7 +118,7 @@ func ResolveWorkflows(workflows []string, verbose bool) (*ResolvedWorkflows, err for _, spec := range parsedSpecs { // Fetch workflow content - FetchWorkflowFromSource handles both local and remote - fetched, err := FetchWorkflowFromSource(spec, verbose) + fetched, err := FetchWorkflowFromSourceWithContext(ctx, spec, verbose) if err != nil { return nil, fmt.Errorf("workflow '%s' not found: %w", spec.String(), err) } diff --git a/pkg/cli/compile_compiler_setup.go b/pkg/cli/compile_compiler_setup.go index a8c35be81d3..6032e0aeacb 100644 --- a/pkg/cli/compile_compiler_setup.go +++ b/pkg/cli/compile_compiler_setup.go @@ -134,6 +134,7 @@ func configureCompilerFlags(compiler *workflow.Compiler, config CompileConfig) { // Set strict mode if specified compiler.SetStrictMode(config.Strict) + compiler.SetAllowActionRefs(config.AllowActionRefs) // Set trial mode if specified if config.TrialMode { diff --git a/pkg/cli/compile_config.go b/pkg/cli/compile_config.go index 055b7d37599..32115979db2 100644 --- a/pkg/cli/compile_config.go +++ b/pkg/cli/compile_config.go @@ -18,6 +18,7 @@ type CompileConfig struct { ForceOverwrite bool // Force overwrite of existing files (dependabot.yml) RefreshStopTime bool // Force regeneration of stop-after times instead of preserving existing ones ForceRefreshActionPins bool // Force refresh of action pins by clearing cache and resolving from GitHub API + AllowActionRefs bool // Allow unresolved action refs as warnings instead of errors Zizmor bool // Run zizmor security scanner on generated .lock.yml files Poutine bool // Run poutine security scanner on generated .lock.yml files Actionlint bool // Run actionlint linter on generated .lock.yml files diff --git a/pkg/cli/fetch.go b/pkg/cli/fetch.go index 94fe710eeca..8b9438d3575 100644 --- a/pkg/cli/fetch.go +++ b/pkg/cli/fetch.go @@ -1,9 +1,12 @@ package cli import ( + "context" "fmt" "os" + "regexp" "strings" + "time" "github.com/github/gh-aw/pkg/console" "github.com/github/gh-aw/pkg/logger" @@ -12,6 +15,18 @@ import ( var remoteWorkflowLog = logger.New("cli:remote_workflow") +var resolveRefToSHAForHost = parser.ResolveRefToSHAForHost +var downloadFileFromGitHubForHost = parser.DownloadFileFromGitHubForHost +var waitBeforeSHAResolutionRetry = sleepForSHAResolutionRetry + +var shaResolutionRetryDelays = []time.Duration{ + 1 * time.Second, + 3 * time.Second, + 9 * time.Second, +} + +var transientHTTP5xxPattern = regexp.MustCompile(`http 5\d{2}`) + // FetchedWorkflow contains content and metadata from a directly fetched workflow file. // This is the unified type that combines content with source information. type FetchedWorkflow struct { @@ -28,6 +43,12 @@ type FetchedWorkflow struct { // For local workflows (local filesystem paths), it reads from the local filesystem. // For remote workflows, it uses the GitHub API to fetch the file content. func FetchWorkflowFromSource(spec *WorkflowSpec, verbose bool) (*FetchedWorkflow, error) { + return FetchWorkflowFromSourceWithContext(context.Background(), spec, verbose) +} + +// FetchWorkflowFromSourceWithContext fetches a workflow file from local disk or GitHub. +// The context is used to cancel remote ref resolution retries (for example, on Ctrl-C). +func FetchWorkflowFromSourceWithContext(ctx context.Context, spec *WorkflowSpec, verbose bool) (*FetchedWorkflow, error) { remoteWorkflowLog.Printf("Fetching workflow from source: spec=%s", spec.String()) // Handle local workflows @@ -36,7 +57,7 @@ func FetchWorkflowFromSource(spec *WorkflowSpec, verbose bool) (*FetchedWorkflow } // Handle remote workflows from GitHub - return fetchRemoteWorkflow(spec, verbose) + return fetchRemoteWorkflow(ctx, spec, verbose) } // fetchLocalWorkflow reads a workflow file from the local filesystem @@ -59,7 +80,7 @@ func fetchLocalWorkflow(spec *WorkflowSpec, verbose bool) (*FetchedWorkflow, err } // fetchRemoteWorkflow fetches a workflow file directly from GitHub using the API -func fetchRemoteWorkflow(spec *WorkflowSpec, verbose bool) (*FetchedWorkflow, error) { +func fetchRemoteWorkflow(ctx context.Context, spec *WorkflowSpec, verbose bool) (*FetchedWorkflow, error) { remoteWorkflowLog.Printf("Fetching remote workflow: repo=%s, path=%s, version=%s", spec.RepoSlug, spec.WorkflowPath, spec.Version) @@ -82,21 +103,17 @@ func fetchRemoteWorkflow(spec *WorkflowSpec, verbose bool) (*FetchedWorkflow, er fmt.Fprintln(os.Stderr, console.FormatInfoMessage(fmt.Sprintf("Fetching %s/%s/%s@%s...", owner, repo, spec.WorkflowPath, ref))) } - // Resolve the ref to a commit SHA for source tracking - commitSHA, err := parser.ResolveRefToSHAForHost(owner, repo, ref, spec.Host) + // Resolve the ref to a commit SHA for source tracking. + commitSHA, err := resolveCommitSHAWithRetries(ctx, owner, repo, ref, spec.WorkflowPath, spec.Host, verbose) if err != nil { - remoteWorkflowLog.Printf("Failed to resolve ref to SHA: %v", err) - // Continue without SHA - we can still fetch the content - commitSHA = "" - } else { - remoteWorkflowLog.Printf("Resolved ref %s to SHA: %s", ref, commitSHA) - if verbose { - fmt.Fprintln(os.Stderr, console.FormatInfoMessage("Resolved to commit: "+commitSHA[:7])) - } + return nil, err + } + if verbose { + fmt.Fprintln(os.Stderr, console.FormatInfoMessage("Resolved to commit: "+commitSHA[:7])) } // Download the workflow file from GitHub - content, err := parser.DownloadFileFromGitHubForHost(owner, repo, spec.WorkflowPath, ref, spec.Host) + content, err := downloadFileFromGitHubForHost(owner, repo, spec.WorkflowPath, ref, spec.Host) if err != nil { // Try with common workflow directory prefixes if the direct path fails. // This handles short workflow names without path separators (e.g. "my-workflow.md"). @@ -107,7 +124,7 @@ func fetchRemoteWorkflow(spec *WorkflowSpec, verbose bool) (*FetchedWorkflow, er altPath += ".md" } remoteWorkflowLog.Printf("Direct path failed, trying: %s", altPath) - if altContent, altErr := parser.DownloadFileFromGitHubForHost(owner, repo, altPath, ref, spec.Host); altErr == nil { + if altContent, altErr := downloadFileFromGitHubForHost(owner, repo, altPath, ref, spec.Host); altErr == nil { return &FetchedWorkflow{ Content: altContent, CommitSHA: commitSHA, @@ -131,3 +148,88 @@ func fetchRemoteWorkflow(spec *WorkflowSpec, verbose bool) (*FetchedWorkflow, er SourcePath: spec.WorkflowPath, }, nil } + +func resolveCommitSHAWithRetries(ctx context.Context, owner, repo, ref, workflowPath, host string, verbose bool) (string, error) { + attempts := len(shaResolutionRetryDelays) + 1 + var lastErr error + + for attempt := 1; attempt <= attempts; attempt++ { + commitSHA, err := resolveRefToSHAForHost(owner, repo, ref, host) + if err == nil { + remoteWorkflowLog.Printf("Resolved ref %s to SHA: %s", ref, commitSHA) + return commitSHA, nil + } + + lastErr = err + remoteWorkflowLog.Printf("Failed to resolve ref %s to SHA (attempt %d/%d): %v", ref, attempt, attempts, err) + + if !isTransientSHAResolutionError(err) { + retryCommand := fmt.Sprintf("gh aw add %s/%s/%s@<40-char-sha>", owner, repo, workflowPath) + return "", fmt.Errorf( + "failed to resolve '%s' to commit SHA for '%s/%s'. Expected the GitHub API to return a commit SHA for the ref. Try: %s: %w", + ref, owner, repo, retryCommand, err, + ) + } + + if attempt < attempts { + delay := shaResolutionRetryDelays[attempt-1] + if verbose { + fmt.Fprintln(os.Stderr, console.FormatWarningMessage( + fmt.Sprintf("Transient SHA resolution failure for '%s' (attempt %d/%d). Retrying in %s...", ref, attempt, attempts, delay), + )) + } + if waitErr := waitBeforeSHAResolutionRetry(ctx, delay); waitErr != nil { + retryCommand := fmt.Sprintf("gh aw add %s/%s/%s@<40-char-sha>", owner, repo, workflowPath) + return "", fmt.Errorf( + "failed to resolve '%s' to commit SHA because retry wait was cancelled. Expected the GitHub API to return a commit SHA for the ref. Try: %s: %w", + ref, retryCommand, waitErr, + ) + } + } + } + + retryCommand := fmt.Sprintf("gh aw add %s/%s/%s@<40-char-sha>", owner, repo, workflowPath) + return "", fmt.Errorf( + "failed to resolve '%s' to commit SHA after %d retries for '%s/%s'. Expected the GitHub API to return a commit SHA for the ref. Check rate limits or try: %s: %w", + ref, len(shaResolutionRetryDelays), owner, repo, retryCommand, lastErr, + ) +} + +// sleepForSHAResolutionRetry waits for the retry delay or context cancellation. +// It returns ctx.Err() when the context is cancelled before the delay elapses, +// otherwise nil when the delay completes normally. +func sleepForSHAResolutionRetry(ctx context.Context, delay time.Duration) error { + timer := time.NewTimer(delay) + defer timer.Stop() + + select { + case <-ctx.Done(): + return ctx.Err() + case <-timer.C: + return nil + } +} + +// isTransientSHAResolutionError returns true when the ref-to-SHA failure appears +// transient and worth retrying (rate limits, network/timeout failures, or HTTP 5xx). +// All other errors are treated as permanent and fail immediately. +func isTransientSHAResolutionError(err error) bool { + if err == nil { + return false + } + + errorText := strings.ToLower(err.Error()) + if strings.Contains(errorText, "http 429") || + strings.Contains(errorText, "rate limit") || + strings.Contains(errorText, "timeout") || + strings.Contains(errorText, "timed out") || + strings.Contains(errorText, "context deadline exceeded") || + strings.Contains(errorText, "temporary") || + strings.Contains(errorText, "connection reset") || + strings.Contains(errorText, "connection refused") || + strings.Contains(errorText, "eof") { + return true + } + + return transientHTTP5xxPattern.MatchString(errorText) +} diff --git a/pkg/cli/remote_workflow_test.go b/pkg/cli/remote_workflow_test.go index 190b21f6592..92544bc5614 100644 --- a/pkg/cli/remote_workflow_test.go +++ b/pkg/cli/remote_workflow_test.go @@ -9,11 +9,18 @@ import ( "os" "path/filepath" "testing" + "time" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) +var testSHAResolutionRetryDelays = []time.Duration{ + time.Millisecond, + 2 * time.Millisecond, + 3 * time.Millisecond, +} + func TestFetchLocalWorkflow(t *testing.T) { tests := []struct { name string @@ -138,6 +145,132 @@ func TestFetchWorkflowFromSource_RemoteRoutingWithInvalidSlug(t *testing.T) { assert.Contains(t, err.Error(), "invalid repository slug", "error should mention invalid slug") } +func TestResolveCommitSHAWithRetries_TransientFailureThenSuccess(t *testing.T) { + originalResolve := resolveRefToSHAForHost + originalWait := waitBeforeSHAResolutionRetry + originalDelays := shaResolutionRetryDelays + defer func() { + resolveRefToSHAForHost = originalResolve + waitBeforeSHAResolutionRetry = originalWait + shaResolutionRetryDelays = originalDelays + }() + + shaResolutionRetryDelays = testSHAResolutionRetryDelays + resolveAttempts := 0 + resolveRefToSHAForHost = func(owner, repo, ref, host string) (string, error) { + resolveAttempts++ + if resolveAttempts == 1 { + return "", errors.New("HTTP 429: rate limit exceeded") + } + return "0123456789abcdef0123456789abcdef01234567", nil + } + + sleeps := make([]time.Duration, 0) + waitBeforeSHAResolutionRetry = func(ctx context.Context, delay time.Duration) error { + sleeps = append(sleeps, delay) + return nil + } + + sha, err := resolveCommitSHAWithRetries(context.Background(), "owner", "repo", "main", ".github/workflows/test.md", "", false) + require.NoError(t, err, "Transient failure should be retried and eventually succeed") + assert.Equal(t, "0123456789abcdef0123456789abcdef01234567", sha, "Resolved SHA should be returned") + assert.Equal(t, 2, resolveAttempts, "Resolution should retry once after initial transient failure") + assert.Equal(t, []time.Duration{time.Millisecond}, sleeps, "Backoff should use first retry delay") +} + +func TestResolveCommitSHAWithRetries_PermanentFailureDoesNotRetry(t *testing.T) { + originalResolve := resolveRefToSHAForHost + originalWait := waitBeforeSHAResolutionRetry + originalDelays := shaResolutionRetryDelays + defer func() { + resolveRefToSHAForHost = originalResolve + waitBeforeSHAResolutionRetry = originalWait + shaResolutionRetryDelays = originalDelays + }() + + shaResolutionRetryDelays = testSHAResolutionRetryDelays + resolveAttempts := 0 + resolveRefToSHAForHost = func(owner, repo, ref, host string) (string, error) { + resolveAttempts++ + return "", errors.New("HTTP 404: Not Found") + } + + sleepCalls := 0 + waitBeforeSHAResolutionRetry = func(ctx context.Context, delay time.Duration) error { + sleepCalls++ + return nil + } + + sha, err := resolveCommitSHAWithRetries(context.Background(), "owner", "repo", "main", ".github/workflows/test.md", "", false) + require.Error(t, err, "Permanent failures should stop immediately") + assert.Empty(t, sha, "No SHA should be returned when resolution fails") + assert.Equal(t, 1, resolveAttempts, "Permanent failures should not retry") + assert.Equal(t, 0, sleepCalls, "No backoff sleep should happen for permanent failures") + assert.Contains(t, err.Error(), "Expected the GitHub API to return a commit SHA for the ref", + "Error should explain expected behavior") + assert.Contains(t, err.Error(), "@<40-char-sha>", "Error should include retry command with full SHA placeholder") +} + +func TestResolveCommitSHAWithRetries_TransientFailureExhaustsRetries(t *testing.T) { + originalResolve := resolveRefToSHAForHost + originalWait := waitBeforeSHAResolutionRetry + originalDelays := shaResolutionRetryDelays + defer func() { + resolveRefToSHAForHost = originalResolve + waitBeforeSHAResolutionRetry = originalWait + shaResolutionRetryDelays = originalDelays + }() + + shaResolutionRetryDelays = testSHAResolutionRetryDelays + resolveAttempts := 0 + resolveRefToSHAForHost = func(owner, repo, ref, host string) (string, error) { + resolveAttempts++ + return "", errors.New("timeout waiting for GitHub API") + } + + sleepCalls := 0 + waitBeforeSHAResolutionRetry = func(ctx context.Context, delay time.Duration) error { + sleepCalls++ + return nil + } + + sha, err := resolveCommitSHAWithRetries(context.Background(), "owner", "repo", "main", ".github/workflows/test.md", "", false) + require.Error(t, err, "Retries should fail after repeated transient failures") + assert.Empty(t, sha, "No SHA should be returned when retries are exhausted") + assert.Equal(t, 4, resolveAttempts, "Should attempt initial call plus three retries") + assert.Equal(t, 3, sleepCalls, "Should sleep between each retry") + assert.Contains(t, err.Error(), "after 3 retries", "Error should report retry exhaustion") +} + +func TestResolveCommitSHAWithRetries_ContextCanceledDuringBackoff(t *testing.T) { + originalResolve := resolveRefToSHAForHost + originalWait := waitBeforeSHAResolutionRetry + originalDelays := shaResolutionRetryDelays + defer func() { + resolveRefToSHAForHost = originalResolve + waitBeforeSHAResolutionRetry = originalWait + shaResolutionRetryDelays = originalDelays + }() + + shaResolutionRetryDelays = testSHAResolutionRetryDelays + resolveRefToSHAForHost = func(owner, repo, ref, host string) (string, error) { + return "", errors.New("HTTP 429: rate limit exceeded") + } + + waitBeforeSHAResolutionRetry = func(ctx context.Context, delay time.Duration) error { + return ctx.Err() + } + + ctx, cancel := context.WithCancel(context.Background()) + cancel() + + sha, err := resolveCommitSHAWithRetries(ctx, "owner", "repo", "main", ".github/workflows/test.md", "", false) + require.Error(t, err, "Cancellation during retry backoff should fail fast") + assert.Empty(t, sha, "No SHA should be returned when retry wait is canceled") + assert.Contains(t, err.Error(), "retry wait was cancelled", "Error should explain cancellation reason") + assert.Contains(t, err.Error(), "@<40-char-sha>", "Error should include exact SHA retry guidance") +} + func TestFetchIncludeFromSource_WorkflowSpecParsing(t *testing.T) { tests := []struct { name string diff --git a/pkg/cli/trial_repository.go b/pkg/cli/trial_repository.go index ba35f9a6dde..22f19427057 100644 --- a/pkg/cli/trial_repository.go +++ b/pkg/cli/trial_repository.go @@ -232,14 +232,14 @@ func installWorkflowInTrialMode(ctx context.Context, tempDir string, parsedSpec if opts.Verbose { fmt.Fprintln(os.Stderr, console.FormatInfoMessage(fmt.Sprintf("Installing local workflow '%s' from '%s' in trial mode", parsedSpec.WorkflowName, parsedSpec.WorkflowPath))) } - return FetchWorkflowFromSource(specToFetch, opts.Verbose) + return FetchWorkflowFromSourceWithContext(ctx, specToFetch, opts.Verbose) }() } else { // Remote workflows can be fetched from any directory if opts.Verbose { fmt.Fprintln(os.Stderr, console.FormatInfoMessage(fmt.Sprintf("Installing workflow '%s' from '%s' in trial mode", parsedSpec.WorkflowName, parsedSpec.RepoSlug))) } - fetched, err = FetchWorkflowFromSource(specToFetch, opts.Verbose) + fetched, err = FetchWorkflowFromSourceWithContext(ctx, specToFetch, opts.Verbose) } if err != nil { diff --git a/pkg/cli/validate_command.go b/pkg/cli/validate_command.go index 28fa910bd79..adddc6c3697 100644 --- a/pkg/cli/validate_command.go +++ b/pkg/cli/validate_command.go @@ -1,8 +1,6 @@ package cli import ( - "context" - "github.com/github/gh-aw/pkg/constants" "github.com/github/gh-aw/pkg/logger" "github.com/spf13/cobra" @@ -40,6 +38,7 @@ Examples: jsonOutput, _ := cmd.Flags().GetBool("json") failFast, _ := cmd.Flags().GetBool("fail-fast") stats, _ := cmd.Flags().GetBool("stats") + allowActionRefs, _ := cmd.Flags().GetBool("allow-action-refs") noCheckUpdate, _ := cmd.Flags().GetBool("no-check-update") validateImages, _ := cmd.Flags().GetBool("validate-images") verbose, _ := cmd.Flags().GetBool("verbose") @@ -54,22 +53,23 @@ Examples: validateLog.Printf("Running validate command: workflows=%v, dir=%s", args, dir) config := CompileConfig{ - MarkdownFiles: args, - Verbose: verbose, - EngineOverride: engineOverride, - Validate: true, - NoEmit: true, - Zizmor: true, - Actionlint: true, - Poutine: true, - WorkflowDir: dir, - Strict: strict, - JSONOutput: jsonOutput, - FailFast: failFast, - Stats: stats, - ValidateImages: validateImages, + MarkdownFiles: args, + Verbose: verbose, + EngineOverride: engineOverride, + Validate: true, + NoEmit: true, + Zizmor: true, + Actionlint: true, + Poutine: true, + WorkflowDir: dir, + Strict: strict, + JSONOutput: jsonOutput, + FailFast: failFast, + Stats: stats, + AllowActionRefs: allowActionRefs, + ValidateImages: validateImages, } - if _, err := CompileWorkflows(context.Background(), config); err != nil { + if _, err := CompileWorkflows(cmd.Context(), config); err != nil { return err } return nil @@ -83,6 +83,7 @@ Examples: cmd.Flags().Bool("fail-fast", false, "Stop at the first validation error instead of collecting all errors") cmd.Flags().Bool("validate-images", false, "Require Docker to be available for container image validation. Without this flag, container image validation is silently skipped when Docker is not installed or the daemon is not running") cmd.Flags().Bool("stats", false, "Display statistics table sorted by workflow file size (shows jobs, steps, scripts, and shells)") + cmd.Flags().Bool("allow-action-refs", false, "Allow unresolved action refs and emit warnings instead of failing validation") cmd.Flags().Bool("no-check-update", false, "Skip checking for gh-aw updates") // Register completions diff --git a/pkg/workflow/action_pins_logging_test.go b/pkg/workflow/action_pins_logging_test.go index 6ec02f40381..154d243970c 100644 --- a/pkg/workflow/action_pins_logging_test.go +++ b/pkg/workflow/action_pins_logging_test.go @@ -118,28 +118,27 @@ func TestActionPinResolutionWithMismatchedVersions(t *testing.T) { } // TestActionPinResolutionWithStrictMode tests action pin resolution in strict mode -// Note: Strict mode now emits warnings instead of errors when SHA resolution fails, -// as it's not always possible to resolve pins +// with compiler-enforced action pinning. func TestActionPinResolutionWithStrictMode(t *testing.T) { tests := []struct { name string repo string requestedVer string - expectWarning bool + expectError bool expectSuccess bool }{ { - name: "ai-inference v1 emits warning in strict mode", + name: "ai-inference v1 returns error when pin cannot be resolved", repo: "actions/ai-inference", requestedVer: "v1", - expectWarning: true, + expectError: true, expectSuccess: false, }, { - name: "checkout v6.0.2 succeeds in strict mode", + name: "checkout v6.0.2 succeeds when exact pin exists", repo: "actions/checkout", requestedVer: "v6.0.2", - expectWarning: false, + expectError: false, expectSuccess: true, }, } @@ -166,19 +165,17 @@ func TestActionPinResolutionWithStrictMode(t *testing.T) { buf.ReadFrom(r) stderrOutput := buf.String() - // Strict mode should never return an error for resolution failures - if err != nil { - t.Errorf("Unexpected error in strict mode for %s@%s: %v", tt.repo, tt.requestedVer, err) - } - - if tt.expectWarning { - // Should emit warning and return empty result - if !strings.Contains(stderrOutput, "Unable to pin action") { - t.Errorf("Expected warning message for %s@%s, got: %s", tt.repo, tt.requestedVer, stderrOutput) + if tt.expectError { + if err == nil { + t.Errorf("Expected error for %s@%s", tt.repo, tt.requestedVer) + } + if !strings.Contains(err.Error(), "unable to pin action") { + t.Errorf("Expected pinning error message for %s@%s, got: %v", tt.repo, tt.requestedVer, err) } if result != "" { - t.Errorf("Expected empty result on warning, got: %s", result) + t.Errorf("Expected empty result on pinning error, got: %s", result) } + return } if tt.expectSuccess { @@ -186,6 +183,9 @@ func TestActionPinResolutionWithStrictMode(t *testing.T) { if err != nil { t.Errorf("Unexpected error: %v", err) } + if stderrOutput != "" { + t.Errorf("Expected no warning output for successful pin, got: %s", stderrOutput) + } if result == "" { t.Errorf("Expected non-empty result") } @@ -194,6 +194,50 @@ func TestActionPinResolutionWithStrictMode(t *testing.T) { } } +func TestActionPinResolutionWithAllowActionRefs(t *testing.T) { + data := &WorkflowData{ + StrictMode: true, + AllowActionRefs: true, + ActionResolver: nil, + } + + oldStderr := os.Stderr + r, w, _ := os.Pipe() + os.Stderr = w + + result, err := getActionPinWithData("actions/ai-inference", "v1", data) + + w.Close() + os.Stderr = oldStderr + + var buf bytes.Buffer + buf.ReadFrom(r) + stderrOutput := buf.String() + + if err != nil { + t.Fatalf("Expected warning mode with --allow-action-refs behavior, got error: %v", err) + } + if result != "" { + t.Fatalf("Expected empty result when unresolved action ref is allowed, got: %s", result) + } + if !strings.Contains(stderrOutput, "Unable to pin action") { + t.Fatalf("Expected warning output for unresolved action ref, got: %s", stderrOutput) + } + if len(data.ActionResolutionFailures) != 1 { + t.Fatalf("Expected one recorded resolution failure, got: %d", len(data.ActionResolutionFailures)) + } + failure := data.ActionResolutionFailures[0] + if failure.Repo != "actions/ai-inference" { + t.Fatalf("Unexpected resolution failure repo: got %q", failure.Repo) + } + if failure.Ref != "v1" { + t.Fatalf("Unexpected resolution failure ref: got %q", failure.Ref) + } + if failure.ErrorType != "pin_not_found" { + t.Fatalf("Unexpected resolution failure error type: got %q", failure.ErrorType) + } +} + // TestActionCacheDuplicateSHAWarning verifies that we log warnings when multiple // version references resolve to the same SHA, which can cause version comment flipping func TestActionCacheDuplicateSHAWarning(t *testing.T) { diff --git a/pkg/workflow/action_pins_test.go b/pkg/workflow/action_pins_test.go index c9ba96f6dd8..0e9a74a578d 100644 --- a/pkg/workflow/action_pins_test.go +++ b/pkg/workflow/action_pins_test.go @@ -1094,6 +1094,7 @@ func TestActionPinWarningDeduplication(t *testing.T) { // Create a shared WorkflowData with the warning cache data := &WorkflowData{ StrictMode: false, + AllowActionRefs: true, ActionPinWarnings: make(map[string]bool), } @@ -1137,6 +1138,7 @@ func TestActionPinWarningDeduplicationAcrossDifferentVersions(t *testing.T) { // Create a shared WorkflowData with the warning cache data := &WorkflowData{ StrictMode: false, + AllowActionRefs: true, ActionPinWarnings: make(map[string]bool), } diff --git a/pkg/workflow/action_reference_test.go b/pkg/workflow/action_reference_test.go index cfa93494c6c..ef16016259e 100644 --- a/pkg/workflow/action_reference_test.go +++ b/pkg/workflow/action_reference_test.go @@ -131,12 +131,12 @@ func TestResolveActionReference(t *testing.T) { description: "Dev mode should return local path", }, { - name: "release mode with version tag", - actionMode: ActionModeRelease, - localPath: "./actions/create-issue", - version: "v1.0.0", - expectedRef: "github/gh-aw/actions/create-issue@v1.0.0", - description: "Release mode should return version-based reference", + name: "release mode with version tag and unresolved pin", + actionMode: ActionModeRelease, + localPath: "./actions/create-issue", + version: "v1.0.0", + shouldBeEmpty: true, + description: "Release mode should fail closed when action cannot be pinned", }, { name: "release mode with dev version", diff --git a/pkg/workflow/compiler_types.go b/pkg/workflow/compiler_types.go index dee9caa5e6d..9903ce26be0 100644 --- a/pkg/workflow/compiler_types.go +++ b/pkg/workflow/compiler_types.go @@ -63,6 +63,7 @@ type Compiler struct { skipValidation bool // If true, skip schema validation noEmit bool // If true, validate without generating lock files strictMode bool // If true, enforce strict validation requirements + allowActionRefs bool // If true, unresolved action refs are warnings instead of errors approve bool // If true, approve safe update changes (skip safe update enforcement) trialMode bool // If true, suppress safe outputs for trial mode execution trialLogicalRepoSlug string // If set in trial mode, the logical repository to checkout @@ -189,6 +190,12 @@ func (c *Compiler) SetStrictMode(strict bool) { c.strictMode = strict } +// SetAllowActionRefs configures whether unresolved action refs are warnings. +// When false (default), unresolved action refs are compiler errors. +func (c *Compiler) SetAllowActionRefs(allow bool) { + c.allowActionRefs = allow +} + // SetRefreshStopTime configures whether to force regeneration of stop-after times func (c *Compiler) SetRefreshStopTime(refresh bool) { c.refreshStopTime = refresh @@ -424,73 +431,75 @@ type WorkflowData struct { AgentImportSpec string // Original import specification for agent file (e.g., "owner/repo/path@ref") RepositoryImports []string // Repository-only imports (format: "owner/repo@ref") for .github folder merging StopTime string - SkipIfMatch *SkipIfMatchConfig // skip-if-match configuration with query and max threshold - SkipIfNoMatch *SkipIfNoMatchConfig // skip-if-no-match configuration with query and min threshold - SkipIfCheckFailing *SkipIfCheckFailingConfig // skip-if-check-failing configuration - SkipRoles []string // roles to skip workflow for (e.g., [admin, maintainer, write]) - SkipBots []string // users to skip workflow for (e.g., [user1, user2]) - OnSteps []map[string]any // steps to inject into the pre-activation job from on.steps - OnPermissions *Permissions // additional permissions for the pre-activation job from on.permissions - ManualApproval string // environment name for manual approval from on: section - Command []string // for /command trigger support - multiple command names - CommandEvents []string // events where command should be active (nil = all events) - CommandOtherEvents map[string]any // for merging command with other events - LabelCommand []string // for label-command trigger support - label names that act as commands - LabelCommandEvents []string // events where label-command should be active (nil = all: issues, pull_request, discussion) - LabelCommandOtherEvents map[string]any // for merging label-command with other events - LabelCommandRemoveLabel bool // whether to automatically remove the triggering label (default: true) - AIReaction string // AI reaction type like "eyes", "heart", etc. - ReactionIssues *bool // whether reactions are allowed on issues/issue_comment triggers (default: true) - ReactionPullRequests *bool // whether reactions are allowed on pull_request/pull_request_review_comment triggers (default: true) - ReactionDiscussions *bool // whether reactions are allowed on discussion/discussion_comment triggers (default: true) - StatusComment *bool // whether to post status comments (default: true when ai-reaction is set, false otherwise) - StatusCommentIssues *bool // whether status comments are allowed on issues/issue_comment triggers (default: true) - StatusCommentPullRequests *bool // whether status comments are allowed on pull_request/pull_request_review_comment triggers (default: true) - StatusCommentDiscussions *bool // whether status comments are allowed on discussion/discussion_comment triggers (default: true) - ActivationGitHubToken string // custom github token from on.github-token for reactions/comments - ActivationGitHubApp *GitHubAppConfig // github app config from on.github-app for minting activation tokens - TopLevelGitHubApp *GitHubAppConfig // top-level github-app fallback for all nested github-app token minting operations - LockForAgent bool // whether to lock the issue during agent workflow execution - Jobs map[string]any // custom job configurations with dependencies - Cache string // cache configuration - NeedsTextOutput bool // whether the workflow uses ${{ needs.task.outputs.text }} - NetworkPermissions *NetworkPermissions // parsed network permissions - SandboxConfig *SandboxConfig // parsed sandbox configuration (AWF or SRT) - SafeOutputs *SafeOutputsConfig // output configuration for automatic output routes - MCPScripts *MCPScriptsConfig // mcp-scripts configuration for custom MCP tools - Roles []string // permission levels required to trigger workflow - Bots []string // allow list of bot identifiers that can trigger workflow - RateLimit *RateLimitConfig // rate limiting configuration for workflow triggers - CacheMemoryConfig *CacheMemoryConfig // parsed cache-memory configuration - RepoMemoryConfig *RepoMemoryConfig // parsed repo-memory configuration - Runtimes map[string]any // runtime version overrides from frontmatter - ToolsTimeout string // timeout for tool/MCP operations: numeric string (seconds) or GitHub Actions expression (empty = use engine default) - ToolsStartupTimeout string // timeout for MCP server startup: numeric string (seconds) or GitHub Actions expression (empty = use engine default) - Features map[string]any // feature flags and configuration options from frontmatter (supports bool and string values) - ActionCache *ActionCache // cache for action pin resolutions - ActionResolver *ActionResolver // resolver for action pins - DockerImages []string // container images collected at compile time (pinned refs when pins are cached) - DockerImagePins []GHAWManifestContainer // full container pin info (image, digest, pinned_image) for manifest - StrictMode bool // strict mode for action pinning - SecretMasking *SecretMaskingConfig // secret masking configuration - ParsedFrontmatter *FrontmatterConfig // cached parsed frontmatter configuration (for performance optimization) - RawFrontmatter map[string]any // raw parsed frontmatter map (for passing to hash functions without re-parsing) - OTLPEndpoint string // resolved OTLP endpoint (from observability.otlp.endpoint, including imports; set by injectOTLPConfig) - ResolvedMCPServers map[string]any // fully merged mcp-servers from main workflow and all imports (for mcp inspect) - ActionPinWarnings map[string]bool // cache of already-warned action pin failures (key: "repo@version") - ActionMode ActionMode // action mode for workflow compilation (dev, release, script) - HasExplicitGitHubTool bool // true if tools.github was explicitly configured in frontmatter - InlinedImports bool // if true, inline all imports at compile time (from inlined-imports frontmatter field) - CheckoutConfigs []*CheckoutConfig // user-configured checkout settings from frontmatter - CheckoutDisabled bool // true when checkout: false is set in frontmatter - HasDispatchItemNumber bool // true when workflow_dispatch has item_number input (generated by label trigger shorthand) - ConcurrencyJobDiscriminator string // optional discriminator expression appended to job-level concurrency groups (from concurrency.job-discriminator) - IsDetectionRun bool // true when this WorkflowData is used for inline threat detection (not the main agent run) - UpdateCheckDisabled bool // true when check-for-updates: false is set in frontmatter (disables version check step in activation job) - StaleCheckDisabled bool // true when on.stale-check: false is set in frontmatter (disables frontmatter hash check step in activation job) - EngineConfigSteps []map[string]any // steps returned by engine.RenderConfig — prepended before execution steps - ServicePortExpressions string // comma-separated ${{ job.services[''].ports[''] }} expressions for AWF --allow-host-service-ports - RunInstallScripts bool // true when run-install-scripts: true is set (globally or per node runtime); disables --ignore-scripts on generated npm install steps + SkipIfMatch *SkipIfMatchConfig // skip-if-match configuration with query and max threshold + SkipIfNoMatch *SkipIfNoMatchConfig // skip-if-no-match configuration with query and min threshold + SkipIfCheckFailing *SkipIfCheckFailingConfig // skip-if-check-failing configuration + SkipRoles []string // roles to skip workflow for (e.g., [admin, maintainer, write]) + SkipBots []string // users to skip workflow for (e.g., [user1, user2]) + OnSteps []map[string]any // steps to inject into the pre-activation job from on.steps + OnPermissions *Permissions // additional permissions for the pre-activation job from on.permissions + ManualApproval string // environment name for manual approval from on: section + Command []string // for /command trigger support - multiple command names + CommandEvents []string // events where command should be active (nil = all events) + CommandOtherEvents map[string]any // for merging command with other events + LabelCommand []string // for label-command trigger support - label names that act as commands + LabelCommandEvents []string // events where label-command should be active (nil = all: issues, pull_request, discussion) + LabelCommandOtherEvents map[string]any // for merging label-command with other events + LabelCommandRemoveLabel bool // whether to automatically remove the triggering label (default: true) + AIReaction string // AI reaction type like "eyes", "heart", etc. + ReactionIssues *bool // whether reactions are allowed on issues/issue_comment triggers (default: true) + ReactionPullRequests *bool // whether reactions are allowed on pull_request/pull_request_review_comment triggers (default: true) + ReactionDiscussions *bool // whether reactions are allowed on discussion/discussion_comment triggers (default: true) + StatusComment *bool // whether to post status comments (default: true when ai-reaction is set, false otherwise) + StatusCommentIssues *bool // whether status comments are allowed on issues/issue_comment triggers (default: true) + StatusCommentPullRequests *bool // whether status comments are allowed on pull_request/pull_request_review_comment triggers (default: true) + StatusCommentDiscussions *bool // whether status comments are allowed on discussion/discussion_comment triggers (default: true) + ActivationGitHubToken string // custom github token from on.github-token for reactions/comments + ActivationGitHubApp *GitHubAppConfig // github app config from on.github-app for minting activation tokens + TopLevelGitHubApp *GitHubAppConfig // top-level github-app fallback for all nested github-app token minting operations + LockForAgent bool // whether to lock the issue during agent workflow execution + Jobs map[string]any // custom job configurations with dependencies + Cache string // cache configuration + NeedsTextOutput bool // whether the workflow uses ${{ needs.task.outputs.text }} + NetworkPermissions *NetworkPermissions // parsed network permissions + SandboxConfig *SandboxConfig // parsed sandbox configuration (AWF or SRT) + SafeOutputs *SafeOutputsConfig // output configuration for automatic output routes + MCPScripts *MCPScriptsConfig // mcp-scripts configuration for custom MCP tools + Roles []string // permission levels required to trigger workflow + Bots []string // allow list of bot identifiers that can trigger workflow + RateLimit *RateLimitConfig // rate limiting configuration for workflow triggers + CacheMemoryConfig *CacheMemoryConfig // parsed cache-memory configuration + RepoMemoryConfig *RepoMemoryConfig // parsed repo-memory configuration + Runtimes map[string]any // runtime version overrides from frontmatter + ToolsTimeout string // timeout for tool/MCP operations: numeric string (seconds) or GitHub Actions expression (empty = use engine default) + ToolsStartupTimeout string // timeout for MCP server startup: numeric string (seconds) or GitHub Actions expression (empty = use engine default) + Features map[string]any // feature flags and configuration options from frontmatter (supports bool and string values) + ActionCache *ActionCache // cache for action pin resolutions + ActionResolver *ActionResolver // resolver for action pins + DockerImages []string // container images collected at compile time (pinned refs when pins are cached) + DockerImagePins []GHAWManifestContainer // full container pin info (image, digest, pinned_image) for manifest + ActionResolutionFailures []GHAWManifestResolutionFailure // unresolved action-ref pinning failures for lock manifest auditing + StrictMode bool // strict mode for action pinning + AllowActionRefs bool // if true, unresolved action refs are warnings instead of errors + SecretMasking *SecretMaskingConfig // secret masking configuration + ParsedFrontmatter *FrontmatterConfig // cached parsed frontmatter configuration (for performance optimization) + RawFrontmatter map[string]any // raw parsed frontmatter map (for passing to hash functions without re-parsing) + OTLPEndpoint string // resolved OTLP endpoint (from observability.otlp.endpoint, including imports; set by injectOTLPConfig) + ResolvedMCPServers map[string]any // fully merged mcp-servers from main workflow and all imports (for mcp inspect) + ActionPinWarnings map[string]bool // cache of already-warned action pin failures (key: "repo@version") + ActionMode ActionMode // action mode for workflow compilation (dev, release, script) + HasExplicitGitHubTool bool // true if tools.github was explicitly configured in frontmatter + InlinedImports bool // if true, inline all imports at compile time (from inlined-imports frontmatter field) + CheckoutConfigs []*CheckoutConfig // user-configured checkout settings from frontmatter + CheckoutDisabled bool // true when checkout: false is set in frontmatter + HasDispatchItemNumber bool // true when workflow_dispatch has item_number input (generated by label trigger shorthand) + ConcurrencyJobDiscriminator string // optional discriminator expression appended to job-level concurrency groups (from concurrency.job-discriminator) + IsDetectionRun bool // true when this WorkflowData is used for inline threat detection (not the main agent run) + UpdateCheckDisabled bool // true when check-for-updates: false is set in frontmatter (disables version check step in activation job) + StaleCheckDisabled bool // true when on.stale-check: false is set in frontmatter (disables frontmatter hash check step in activation job) + EngineConfigSteps []map[string]any // steps returned by engine.RenderConfig — prepended before execution steps + ServicePortExpressions string // comma-separated ${{ job.services[''].ports[''] }} expressions for AWF --allow-host-service-ports + RunInstallScripts bool // true when run-install-scripts: true is set (globally or per node runtime); disables --ignore-scripts on generated npm install steps } // PinContext returns an actionpins.PinContext backed by this WorkflowData. @@ -504,8 +513,17 @@ func (d *WorkflowData) PinContext() *actionpins.PinContext { d.ActionPinWarnings = make(map[string]bool) } ctx := &actionpins.PinContext{ - StrictMode: d.StrictMode, - Warnings: d.ActionPinWarnings, + StrictMode: d.StrictMode, + EnforcePinned: true, + AllowActionRefs: d.AllowActionRefs, + Warnings: d.ActionPinWarnings, + RecordResolutionFailure: func(f actionpins.ResolutionFailure) { + d.ActionResolutionFailures = append(d.ActionResolutionFailures, GHAWManifestResolutionFailure{ + Repo: f.Repo, + Ref: f.Ref, + ErrorType: string(f.ErrorType), + }) + }, } // Only set Resolver if non-nil to avoid passing a typed nil interface value // (which would be non-nil in actionpins but crash on method call). diff --git a/pkg/workflow/compiler_yaml.go b/pkg/workflow/compiler_yaml.go index d2ab35ca8c0..b8eaa39ab3d 100644 --- a/pkg/workflow/compiler_yaml.go +++ b/pkg/workflow/compiler_yaml.go @@ -118,7 +118,7 @@ func (c *Compiler) generateWorkflowHeader(yaml *strings.Builder, data *WorkflowD // Embed the gh-aw-manifest immediately after gh-aw-metadata for easy machine parsing. // The manifest records all secrets, external actions, and container images detected at // compile time so that subsequent compilations can perform safe update enforcement. - manifest := NewGHAWManifest(secrets, actions, data.DockerImagePins, data.Redirect) + manifest := NewGHAWManifest(secrets, actions, data.ActionResolutionFailures, data.DockerImagePins, data.Redirect) if manifestJSON, err := manifest.ToJSON(); err == nil { fmt.Fprintf(yaml, "# gh-aw-manifest: %s\n", manifestJSON) } else { diff --git a/pkg/workflow/safe_update_manifest.go b/pkg/workflow/safe_update_manifest.go index 47f37d60ade..75288fc984a 100644 --- a/pkg/workflow/safe_update_manifest.go +++ b/pkg/workflow/safe_update_manifest.go @@ -34,17 +34,26 @@ type GHAWManifestContainer struct { PinnedImage string `json:"pinned_image,omitempty"` // Full ref, e.g. "node:lts-alpine@sha256:abc123..." } +// GHAWManifestResolutionFailure represents an action-ref pinning failure captured +// during compilation. These failures are embedded for lock-file auditing. +type GHAWManifestResolutionFailure struct { + Repo string `json:"repo"` + Ref string `json:"ref"` + ErrorType string `json:"error_type"` +} + // GHAWManifest is the single-line JSON payload embedded as a "# gh-aw-manifest: ..." // comment in generated lock files. It records the secrets, external actions, and // container images that were detected at the time the lock file was last compiled // so that subsequent compilations can detect newly introduced secrets when safe // update mode is enabled. type GHAWManifest struct { - Version int `json:"version"` - Secrets []string `json:"secrets"` - Actions []GHAWManifestAction `json:"actions"` - Containers []GHAWManifestContainer `json:"containers,omitempty"` // container images used, with digest when available - Redirect string `json:"redirect,omitempty"` // frontmatter redirect target for moved workflows + Version int `json:"version"` + Secrets []string `json:"secrets"` + Actions []GHAWManifestAction `json:"actions"` + ResolutionFailures []GHAWManifestResolutionFailure `json:"resolution_failures,omitempty"` // unresolved action-ref pinning failures + Containers []GHAWManifestContainer `json:"containers,omitempty"` // container images used, with digest when available + Redirect string `json:"redirect,omitempty"` // frontmatter redirect target for moved workflows } // NewGHAWManifest builds a GHAWManifest from the raw secret names, action reference @@ -57,7 +66,7 @@ type GHAWManifest struct { // "actions/checkout@abc1234 # v4" // // containers is the list of container image entries with full digest info (when available). -func NewGHAWManifest(secretNames []string, actionRefs []string, containers []GHAWManifestContainer, redirect string) *GHAWManifest { +func NewGHAWManifest(secretNames []string, actionRefs []string, failures []GHAWManifestResolutionFailure, containers []GHAWManifestContainer, redirect string) *GHAWManifest { safeUpdateManifestLog.Printf("Building gh-aw-manifest: raw_secrets=%d, raw_actions=%d, containers=%d", len(secretNames), len(actionRefs), len(containers)) // Normalize secret names to full "secrets.NAME" form and deduplicate. @@ -73,6 +82,7 @@ func NewGHAWManifest(secretNames []string, actionRefs []string, containers []GHA sort.Strings(secrets) actions := parseActionRefs(actionRefs) + resolutionFailures := normalizeResolutionFailures(failures) // Deduplicate container entries by image name and sort for deterministic output. seenContainers := make(map[string]bool, len(containers)) @@ -91,11 +101,12 @@ func NewGHAWManifest(secretNames []string, actionRefs []string, containers []GHA currentGHAWManifestVersion, len(secrets), len(actions), len(sortedContainers)) return &GHAWManifest{ - Version: currentGHAWManifestVersion, - Secrets: secrets, - Actions: actions, - Containers: sortedContainers, - Redirect: strings.TrimSpace(redirect), + Version: currentGHAWManifestVersion, + Secrets: secrets, + Actions: actions, + ResolutionFailures: resolutionFailures, + Containers: sortedContainers, + Redirect: strings.TrimSpace(redirect), } } @@ -164,6 +175,44 @@ func parseActionRefs(refs []string) []GHAWManifestAction { return actions } +func normalizeResolutionFailures(failures []GHAWManifestResolutionFailure) []GHAWManifestResolutionFailure { + type failureKey struct { + Repo string + Ref string + ErrorType string + } + seen := make(map[failureKey]bool) + normalized := make([]GHAWManifestResolutionFailure, 0, len(failures)) + for _, f := range failures { + repo := strings.TrimSpace(f.Repo) + ref := strings.TrimSpace(f.Ref) + errorType := strings.TrimSpace(f.ErrorType) + if repo == "" || ref == "" || errorType == "" { + continue + } + key := failureKey{Repo: repo, Ref: ref, ErrorType: errorType} + if seen[key] { + continue + } + seen[key] = true + normalized = append(normalized, GHAWManifestResolutionFailure{ + Repo: repo, + Ref: ref, + ErrorType: errorType, + }) + } + sort.Slice(normalized, func(i, j int) bool { + if normalized[i].Repo != normalized[j].Repo { + return normalized[i].Repo < normalized[j].Repo + } + if normalized[i].Ref != normalized[j].Ref { + return normalized[i].Ref < normalized[j].Ref + } + return normalized[i].ErrorType < normalized[j].ErrorType + }) + return normalized +} + // ToJSON serialises the manifest to a compact, single-line JSON string suitable // for embedding in a YAML comment header. func (m *GHAWManifest) ToJSON() (string, error) { diff --git a/pkg/workflow/safe_update_manifest_test.go b/pkg/workflow/safe_update_manifest_test.go index d3d01d5768b..ae81cd431a7 100644 --- a/pkg/workflow/safe_update_manifest_test.go +++ b/pkg/workflow/safe_update_manifest_test.go @@ -14,11 +14,13 @@ func TestNewGHAWManifest(t *testing.T) { name string secretNames []string actionRefs []string + resolutionFailures []GHAWManifestResolutionFailure containers []GHAWManifestContainer redirect string wantVersion int wantSecrets []string wantActionRepos []string + wantFailures []GHAWManifestResolutionFailure wantContainerImages []string wantRedirect string }{ @@ -76,6 +78,22 @@ func TestNewGHAWManifest(t *testing.T) { wantSecrets: []string{}, wantActionRepos: []string{"actions/checkout"}, }, + { + name: "resolution failures are normalized, deduplicated, and sorted", + resolutionFailures: []GHAWManifestResolutionFailure{ + {Repo: "actions/setup-node", Ref: "v6", ErrorType: "dynamic_resolution_failed"}, + {Repo: "actions/setup-node", Ref: "v6", ErrorType: "dynamic_resolution_failed"}, + {Repo: "actions/setup-node", Ref: "v6", ErrorType: "pin_not_found"}, + {Repo: "actions/checkout", Ref: "v5", ErrorType: "pin_not_found"}, + }, + wantVersion: 1, + wantSecrets: []string{}, + wantFailures: []GHAWManifestResolutionFailure{ + {Repo: "actions/checkout", Ref: "v5", ErrorType: "pin_not_found"}, + {Repo: "actions/setup-node", Ref: "v6", ErrorType: "dynamic_resolution_failed"}, + {Repo: "actions/setup-node", Ref: "v6", ErrorType: "pin_not_found"}, + }, + }, { name: "containers are sorted and deduplicated", containers: []GHAWManifestContainer{ @@ -120,7 +138,7 @@ func TestNewGHAWManifest(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - m := NewGHAWManifest(tt.secretNames, tt.actionRefs, tt.containers, tt.redirect) + m := NewGHAWManifest(tt.secretNames, tt.actionRefs, tt.resolutionFailures, tt.containers, tt.redirect) require.NotNil(t, m, "manifest should not be nil") assert.Equal(t, tt.wantVersion, m.Version, "manifest version") if tt.wantSecrets != nil { @@ -140,6 +158,9 @@ func TestNewGHAWManifest(t *testing.T) { } assert.Equal(t, tt.wantContainerImages, images, "container images") } + if tt.wantFailures != nil { + assert.Equal(t, tt.wantFailures, m.ResolutionFailures, "resolution failures") + } assert.Equal(t, tt.wantRedirect, m.Redirect, "manifest redirect") }) } @@ -156,7 +177,7 @@ func TestNewGHAWManifestContainerDigest(t *testing.T) { Image: "alpine:3.14", // no digest }, } - m := NewGHAWManifest(nil, nil, containers, "") + m := NewGHAWManifest(nil, nil, nil, containers, "") require.Len(t, m.Containers, 2, "should have two containers") // Sorted: alpine before node @@ -187,6 +208,9 @@ func TestGHAWManifestToJSON(t *testing.T) { Actions: []GHAWManifestAction{ {Repo: "actions/checkout", SHA: "abc123", Version: "v4"}, }, + ResolutionFailures: []GHAWManifestResolutionFailure{ + {Repo: "actions/setup-node", Ref: "v6", ErrorType: "dynamic_resolution_failed"}, + }, Redirect: "owner/repo/workflows/new.md@main", } @@ -198,6 +222,8 @@ func TestGHAWManifestToJSON(t *testing.T) { assert.Contains(t, json, `"actions/checkout"`, "action repo in JSON") assert.Contains(t, json, `"abc123"`, "action SHA in JSON") assert.Contains(t, json, `"v4"`, "action version in JSON") + assert.Contains(t, json, `"resolution_failures"`, "resolution failures in JSON") + assert.Contains(t, json, `"dynamic_resolution_failed"`, "error type in JSON") assert.Contains(t, json, `"redirect":"owner/repo/workflows/new.md@main"`, "redirect in JSON") } diff --git a/pkg/workflow/stale_check_test.go b/pkg/workflow/stale_check_test.go index aa062f6084c..1b1c23fc759 100644 --- a/pkg/workflow/stale_check_test.go +++ b/pkg/workflow/stale_check_test.go @@ -8,6 +8,7 @@ import ( "strings" "testing" + "github.com/github/gh-aw/pkg/parser" "github.com/github/gh-aw/pkg/stringutil" "github.com/github/gh-aw/pkg/testutil" "github.com/stretchr/testify/assert" @@ -100,3 +101,54 @@ Test workflow for stale check step explicitly enabled. }) } } + +func TestStaleCheckFrontmatterHashParityForPinnedAndUnpinnedSource(t *testing.T) { + tests := []struct { + name string + sourceLine string + }{ + { + name: "pinned source sha", + sourceLine: "source: github/gh-aw/.github/workflows/test.md@0123456789abcdef0123456789abcdef01234567", + }, + { + name: "unpinned source ref", + sourceLine: "source: github/gh-aw/.github/workflows/test.md@main", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + tmpDir := testutil.TempDir(t, "stale-check-hash-parity") + workflowPath := filepath.Join(tmpDir, "hash-parity.md") + workflowMD := `--- +engine: copilot +on: + workflow_dispatch: +` + tt.sourceLine + ` +--- +Hash parity regression coverage. +` + + require.NoError(t, os.WriteFile(workflowPath, []byte(workflowMD), 0644), "Should write workflow file") + + compiler := NewCompiler() + err := compiler.CompileWorkflow(workflowPath) + require.NoError(t, err, "Workflow should compile without errors") + + lockPath := stringutil.MarkdownToLockFile(workflowPath) + lockContent, err := os.ReadFile(lockPath) + require.NoError(t, err, "Lock file should be readable") + + metadata, _, err := ExtractMetadataFromLockFile(string(lockContent)) + require.NoError(t, err, "Lock metadata should be parseable") + require.NotNil(t, metadata, "Lock metadata should exist") + + currentHash, err := parser.ComputeFrontmatterHashFromFile(workflowPath, parser.NewImportCache(tmpDir)) + require.NoError(t, err, "Frontmatter hash should be recomputable from workflow markdown") + + assert.Equal(t, currentHash, metadata.FrontmatterHash, + "Frontmatter hash in lock metadata should match markdown source hash") + }) + } +} diff --git a/pkg/workflow/workflow_builder.go b/pkg/workflow/workflow_builder.go index 3c812a3ecee..b19b3db4d5d 100644 --- a/pkg/workflow/workflow_builder.go +++ b/pkg/workflow/workflow_builder.go @@ -63,6 +63,7 @@ func (c *Compiler) buildInitialWorkflowData( TrialMode: c.trialMode, TrialLogicalRepo: c.trialLogicalRepoSlug, StrictMode: c.strictMode, + AllowActionRefs: c.allowActionRefs, SecretMasking: toolsResult.secretMasking, ParsedFrontmatter: toolsResult.parsedFrontmatter, RawFrontmatter: result.Frontmatter,