From 2499878000f7e033c307915b9806d9adf43923c6 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 20 Apr 2026 20:22:13 +0000 Subject: [PATCH 01/11] Initial plan From b9f83fec029d0152423221ef081d0b9bc8bf8b5d Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 20 Apr 2026 20:41:42 +0000 Subject: [PATCH 02/11] fix add sha resolution retries and lock hash parity safeguards Agent-Logs-Url: https://github.com/github/gh-aw/sessions/ce841919-e57f-4c67-b519-9fada0a7a89b Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- actions/setup/md/stale_lock_file_failed.md | 7 ++ pkg/cli/add_command.go | 7 +- pkg/cli/add_integration_test.go | 43 +++++++++ pkg/cli/fetch.go | 101 ++++++++++++++++++--- pkg/cli/remote_workflow_test.go | 94 +++++++++++++++++++ pkg/workflow/stale_check_test.go | 52 +++++++++++ 6 files changed, 291 insertions(+), 13 deletions(-) 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/pkg/cli/add_command.go b/pkg/cli/add_command.go index 688fcf7bc74..24f6890d8a0 100644 --- a/pkg/cli/add_command.go +++ b/pkg/cli/add_command.go @@ -494,12 +494,17 @@ 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 so all downstream processing uses the exact bytes on disk. + 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_integration_test.go b/pkg/cli/add_integration_test.go index d9c34e138a6..c890c968b35 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, err := parser.ComputeFrontmatterHashFromFile(destWorkflowFile, parser.NewImportCache(setup.tempDir)) + require.NoError(t, err, "should compute frontmatter hash from added markdown file") + metadata, _, err := workflow.ExtractMetadataFromLockFile(lockContentStr) + require.NoError(t, err, "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 verifies 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() + + workflowSpec := "github/gh-aw-does-not-exist/.github/workflows/not-real.md@main" + + cmd := exec.Command(setup.binaryPath, "add", workflowSpec) + 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@", + "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/fetch.go b/pkg/cli/fetch.go index 94fe710eeca..31ee59117b8 100644 --- a/pkg/cli/fetch.go +++ b/pkg/cli/fetch.go @@ -4,6 +4,7 @@ import ( "fmt" "os" "strings" + "time" "github.com/github/gh-aw/pkg/console" "github.com/github/gh-aw/pkg/logger" @@ -12,6 +13,16 @@ import ( var remoteWorkflowLog = logger.New("cli:remote_workflow") +var resolveRefToSHAForHost = parser.ResolveRefToSHAForHost +var downloadFileFromGitHubForHost = parser.DownloadFileFromGitHubForHost +var sleepBeforeSHAResolutionRetry = time.Sleep + +var shaResolutionRetryDelays = []time.Duration{ + 1 * time.Second, + 3 * time.Second, + 9 * time.Second, +} + // 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 { @@ -82,21 +93,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(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 +114,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 +138,73 @@ func fetchRemoteWorkflow(spec *WorkflowSpec, verbose bool) (*FetchedWorkflow, er SourcePath: spec.WorkflowPath, }, nil } + +func resolveCommitSHAWithRetries(owner, repo, ref, workflowPath, host string, verbose bool) (string, error) { + retryCommand := fmt.Sprintf("gh aw add %s/%s/%s@", owner, repo, workflowPath) + 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) { + message := fmt.Sprintf( + "failed to resolve '%s' to commit SHA for '%s/%s': %v. Expected the GitHub API to return a commit SHA for the ref. Try: %s.", + ref, owner, repo, err, retryCommand, + ) + fmt.Fprintln(os.Stderr, console.FormatErrorMessage(message)) + return "", fmt.Errorf("%s: %w", message, 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), + )) + } + sleepBeforeSHAResolutionRetry(delay) + } + } + + message := fmt.Sprintf( + "failed to resolve '%s' to commit SHA after %d retries for '%s/%s': %v. Expected the GitHub API to return a commit SHA for the ref. Check rate limits or try: %s.", + ref, len(shaResolutionRetryDelays), owner, repo, lastErr, retryCommand, + ) + fmt.Fprintln(os.Stderr, console.FormatErrorMessage(message)) + return "", fmt.Errorf("%s: %w", message, lastErr) +} + +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 + } + + for statusCode := 500; statusCode <= 599; statusCode++ { + if strings.Contains(errorText, fmt.Sprintf("http %d", statusCode)) { + return true + } + } + + return false +} diff --git a/pkg/cli/remote_workflow_test.go b/pkg/cli/remote_workflow_test.go index 190b21f6592..d73b395b473 100644 --- a/pkg/cli/remote_workflow_test.go +++ b/pkg/cli/remote_workflow_test.go @@ -9,6 +9,7 @@ import ( "os" "path/filepath" "testing" + "time" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -138,6 +139,99 @@ 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 + originalSleep := sleepBeforeSHAResolutionRetry + originalDelays := shaResolutionRetryDelays + defer func() { + resolveRefToSHAForHost = originalResolve + sleepBeforeSHAResolutionRetry = originalSleep + shaResolutionRetryDelays = originalDelays + }() + + shaResolutionRetryDelays = []time.Duration{time.Millisecond, 2 * time.Millisecond, 3 * time.Millisecond} + 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) + sleepBeforeSHAResolutionRetry = func(delay time.Duration) { + sleeps = append(sleeps, delay) + } + + sha, err := resolveCommitSHAWithRetries("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 + originalSleep := sleepBeforeSHAResolutionRetry + originalDelays := shaResolutionRetryDelays + defer func() { + resolveRefToSHAForHost = originalResolve + sleepBeforeSHAResolutionRetry = originalSleep + shaResolutionRetryDelays = originalDelays + }() + + shaResolutionRetryDelays = []time.Duration{time.Millisecond, 2 * time.Millisecond, 3 * time.Millisecond} + resolveAttempts := 0 + resolveRefToSHAForHost = func(owner, repo, ref, host string) (string, error) { + resolveAttempts++ + return "", errors.New("HTTP 404: Not Found") + } + + sleepCalls := 0 + sleepBeforeSHAResolutionRetry = func(delay time.Duration) { + sleepCalls++ + } + + sha, err := resolveCommitSHAWithRetries("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") +} + +func TestResolveCommitSHAWithRetries_TransientFailureExhaustsRetries(t *testing.T) { + originalResolve := resolveRefToSHAForHost + originalSleep := sleepBeforeSHAResolutionRetry + originalDelays := shaResolutionRetryDelays + defer func() { + resolveRefToSHAForHost = originalResolve + sleepBeforeSHAResolutionRetry = originalSleep + shaResolutionRetryDelays = originalDelays + }() + + shaResolutionRetryDelays = []time.Duration{time.Millisecond, 2 * time.Millisecond, 3 * time.Millisecond} + resolveAttempts := 0 + resolveRefToSHAForHost = func(owner, repo, ref, host string) (string, error) { + resolveAttempts++ + return "", errors.New("timeout waiting for GitHub API") + } + + sleepCalls := 0 + sleepBeforeSHAResolutionRetry = func(delay time.Duration) { + sleepCalls++ + } + + sha, err := resolveCommitSHAWithRetries("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 TestFetchIncludeFromSource_WorkflowSpecParsing(t *testing.T) { tests := []struct { name string diff --git a/pkg/workflow/stale_check_test.go b/pkg/workflow/stale_check_test.go index aa062f6084c..6eae00f52de 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") + + recomputedHash, err := parser.ComputeFrontmatterHashFromFile(workflowPath, parser.NewImportCache(tmpDir)) + require.NoError(t, err, "Frontmatter hash should be recomputable from workflow markdown") + + assert.Equal(t, recomputedHash, metadata.FrontmatterHash, + "Frontmatter hash in lock metadata should match markdown source hash") + }) + } +} From 8ee019e6c1b191f05c750f610f5e5c47f153bca9 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 20 Apr 2026 20:47:20 +0000 Subject: [PATCH 03/11] test and docs: cover sha resolution failures and frontmatter hash parity Agent-Logs-Url: https://github.com/github/gh-aw/sessions/ce841919-e57f-4c67-b519-9fada0a7a89b Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/cli/add_command.go | 3 ++- pkg/cli/add_integration_test.go | 16 ++++++++-------- pkg/cli/fetch.go | 17 +++++++++-------- pkg/cli/remote_workflow_test.go | 13 ++++++++++--- pkg/workflow/stale_check_test.go | 4 ++-- 5 files changed, 31 insertions(+), 22 deletions(-) diff --git a/pkg/cli/add_command.go b/pkg/cli/add_command.go index 24f6890d8a0..2a26341a2da 100644 --- a/pkg/cli/add_command.go +++ b/pkg/cli/add_command.go @@ -494,7 +494,8 @@ 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 so all downstream processing uses the exact bytes on disk. + // 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) diff --git a/pkg/cli/add_integration_test.go b/pkg/cli/add_integration_test.go index c890c968b35..de9555b6639 100644 --- a/pkg/cli/add_integration_test.go +++ b/pkg/cli/add_integration_test.go @@ -360,24 +360,24 @@ Please analyze the repository and provide a summary. assert.Contains(t, lockContentStr, "jobs:", "lock file should have jobs section") // Verify frontmatter hash parity between source markdown and lock metadata. - computedHash, err := parser.ComputeFrontmatterHashFromFile(destWorkflowFile, parser.NewImportCache(setup.tempDir)) - require.NoError(t, err, "should compute frontmatter hash from added markdown file") - metadata, _, err := workflow.ExtractMetadataFromLockFile(lockContentStr) - require.NoError(t, err, "should extract lock metadata from compiled lock file") + 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 verifies add fails loudly when ref-to-SHA +// 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() - workflowSpec := "github/gh-aw-does-not-exist/.github/workflows/not-real.md@main" + nonExistentWorkflowSpec := "github/gh-aw-does-not-exist/.github/workflows/not-real.md@main" - cmd := exec.Command(setup.binaryPath, "add", workflowSpec) + cmd := exec.Command(setup.binaryPath, "add", nonExistentWorkflowSpec) cmd.Dir = setup.tempDir output, err := cmd.CombinedOutput() outputStr := string(output) @@ -387,7 +387,7 @@ func TestAddRemoteWorkflowFailsWhenSHAResolutionFails(t *testing.T) { "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@", + 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") diff --git a/pkg/cli/fetch.go b/pkg/cli/fetch.go index 31ee59117b8..259e6d11f2d 100644 --- a/pkg/cli/fetch.go +++ b/pkg/cli/fetch.go @@ -3,6 +3,7 @@ package cli import ( "fmt" "os" + "regexp" "strings" "time" @@ -23,6 +24,8 @@ var shaResolutionRetryDelays = []time.Duration{ 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 { @@ -140,7 +143,6 @@ func fetchRemoteWorkflow(spec *WorkflowSpec, verbose bool) (*FetchedWorkflow, er } func resolveCommitSHAWithRetries(owner, repo, ref, workflowPath, host string, verbose bool) (string, error) { - retryCommand := fmt.Sprintf("gh aw add %s/%s/%s@", owner, repo, workflowPath) attempts := len(shaResolutionRetryDelays) + 1 var lastErr error @@ -155,6 +157,7 @@ func resolveCommitSHAWithRetries(owner, repo, ref, workflowPath, host string, ve 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) message := fmt.Sprintf( "failed to resolve '%s' to commit SHA for '%s/%s': %v. Expected the GitHub API to return a commit SHA for the ref. Try: %s.", ref, owner, repo, err, retryCommand, @@ -174,6 +177,7 @@ func resolveCommitSHAWithRetries(owner, repo, ref, workflowPath, host string, ve } } + retryCommand := fmt.Sprintf("gh aw add %s/%s/%s@<40-char-sha>", owner, repo, workflowPath) message := fmt.Sprintf( "failed to resolve '%s' to commit SHA after %d retries for '%s/%s': %v. Expected the GitHub API to return a commit SHA for the ref. Check rate limits or try: %s.", ref, len(shaResolutionRetryDelays), owner, repo, lastErr, retryCommand, @@ -182,6 +186,9 @@ func resolveCommitSHAWithRetries(owner, repo, ref, workflowPath, host string, ve return "", fmt.Errorf("%s: %w", message, lastErr) } +// 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 @@ -200,11 +207,5 @@ func isTransientSHAResolutionError(err error) bool { return true } - for statusCode := 500; statusCode <= 599; statusCode++ { - if strings.Contains(errorText, fmt.Sprintf("http %d", statusCode)) { - return true - } - } - - return false + return transientHTTP5xxPattern.MatchString(errorText) } diff --git a/pkg/cli/remote_workflow_test.go b/pkg/cli/remote_workflow_test.go index d73b395b473..9aa65bdd72b 100644 --- a/pkg/cli/remote_workflow_test.go +++ b/pkg/cli/remote_workflow_test.go @@ -15,6 +15,12 @@ import ( "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 @@ -149,7 +155,7 @@ func TestResolveCommitSHAWithRetries_TransientFailureThenSuccess(t *testing.T) { shaResolutionRetryDelays = originalDelays }() - shaResolutionRetryDelays = []time.Duration{time.Millisecond, 2 * time.Millisecond, 3 * time.Millisecond} + shaResolutionRetryDelays = testSHAResolutionRetryDelays resolveAttempts := 0 resolveRefToSHAForHost = func(owner, repo, ref, host string) (string, error) { resolveAttempts++ @@ -181,7 +187,7 @@ func TestResolveCommitSHAWithRetries_PermanentFailureDoesNotRetry(t *testing.T) shaResolutionRetryDelays = originalDelays }() - shaResolutionRetryDelays = []time.Duration{time.Millisecond, 2 * time.Millisecond, 3 * time.Millisecond} + shaResolutionRetryDelays = testSHAResolutionRetryDelays resolveAttempts := 0 resolveRefToSHAForHost = func(owner, repo, ref, host string) (string, error) { resolveAttempts++ @@ -200,6 +206,7 @@ func TestResolveCommitSHAWithRetries_PermanentFailureDoesNotRetry(t *testing.T) 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) { @@ -212,7 +219,7 @@ func TestResolveCommitSHAWithRetries_TransientFailureExhaustsRetries(t *testing. shaResolutionRetryDelays = originalDelays }() - shaResolutionRetryDelays = []time.Duration{time.Millisecond, 2 * time.Millisecond, 3 * time.Millisecond} + shaResolutionRetryDelays = testSHAResolutionRetryDelays resolveAttempts := 0 resolveRefToSHAForHost = func(owner, repo, ref, host string) (string, error) { resolveAttempts++ diff --git a/pkg/workflow/stale_check_test.go b/pkg/workflow/stale_check_test.go index 6eae00f52de..1b1c23fc759 100644 --- a/pkg/workflow/stale_check_test.go +++ b/pkg/workflow/stale_check_test.go @@ -144,10 +144,10 @@ Hash parity regression coverage. require.NoError(t, err, "Lock metadata should be parseable") require.NotNil(t, metadata, "Lock metadata should exist") - recomputedHash, err := parser.ComputeFrontmatterHashFromFile(workflowPath, parser.NewImportCache(tmpDir)) + currentHash, err := parser.ComputeFrontmatterHashFromFile(workflowPath, parser.NewImportCache(tmpDir)) require.NoError(t, err, "Frontmatter hash should be recomputable from workflow markdown") - assert.Equal(t, recomputedHash, metadata.FrontmatterHash, + assert.Equal(t, currentHash, metadata.FrontmatterHash, "Frontmatter hash in lock metadata should match markdown source hash") }) } From 9438981c4bf1e599314e7c3543de30f7e7f04b87 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 20 Apr 2026 21:26:03 +0000 Subject: [PATCH 04/11] fix add retry errors and make SHA backoff context-cancelable Agent-Logs-Url: https://github.com/github/gh-aw/sessions/8ddf9ab4-df4f-4bfa-b6b3-634452eb614e Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/cli/add_command.go | 9 ++++- pkg/cli/add_interactive_orchestrator.go | 2 +- pkg/cli/add_workflow_resolution.go | 8 +++- pkg/cli/fetch.go | 53 +++++++++++++++++-------- pkg/cli/remote_workflow_test.go | 53 +++++++++++++++++++------ pkg/cli/trial_repository.go | 4 +- 6 files changed, 95 insertions(+), 34 deletions(-) diff --git a/pkg/cli/add_command.go b/pkg/cli/add_command.go index 2a26341a2da..b88b360278e 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 := AddWorkflowsWithContext(cmd.Context(), workflows, opts) return err }, } @@ -173,8 +173,13 @@ Note: For guided interactive setup, use the 'add-wizard' command instead.`, // 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) { + return AddWorkflowsWithContext(context.Background(), workflows, opts) +} + +// AddWorkflowsWithContext adds one or more workflows and supports cancellation. +func AddWorkflowsWithContext(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 := ResolveWorkflowsWithContext(ctx, workflows, opts.Verbose) if err != nil { return nil, err } diff --git a/pkg/cli/add_interactive_orchestrator.go b/pkg/cli/add_interactive_orchestrator.go index 5911e37b3b7..f2732923515 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 := ResolveWorkflowsWithContext(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..63d0fc59e3b 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" @@ -47,6 +48,11 @@ type ResolvedWorkflows struct { // 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) { + return ResolveWorkflowsWithContext(context.Background(), workflows, verbose) +} + +// ResolveWorkflowsWithContext resolves workflow specifications and supports cancellation. +func ResolveWorkflowsWithContext(ctx context.Context, workflows []string, verbose bool) (*ResolvedWorkflows, error) { resolutionLog.Printf("Resolving workflows: count=%d", len(workflows)) if len(workflows) == 0 { @@ -117,7 +123,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/fetch.go b/pkg/cli/fetch.go index 259e6d11f2d..e94d1ed4dfe 100644 --- a/pkg/cli/fetch.go +++ b/pkg/cli/fetch.go @@ -1,6 +1,7 @@ package cli import ( + "context" "fmt" "os" "regexp" @@ -16,7 +17,7 @@ var remoteWorkflowLog = logger.New("cli:remote_workflow") var resolveRefToSHAForHost = parser.ResolveRefToSHAForHost var downloadFileFromGitHubForHost = parser.DownloadFileFromGitHubForHost -var sleepBeforeSHAResolutionRetry = time.Sleep +var waitBeforeSHAResolutionRetry = sleepForSHAResolutionRetry var shaResolutionRetryDelays = []time.Duration{ 1 * time.Second, @@ -42,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 @@ -50,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 @@ -73,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) @@ -97,7 +104,7 @@ func fetchRemoteWorkflow(spec *WorkflowSpec, verbose bool) (*FetchedWorkflow, er } // Resolve the ref to a commit SHA for source tracking. - commitSHA, err := resolveCommitSHAWithRetries(owner, repo, ref, spec.WorkflowPath, spec.Host, verbose) + commitSHA, err := resolveCommitSHAWithRetries(ctx, owner, repo, ref, spec.WorkflowPath, spec.Host, verbose) if err != nil { return nil, err } @@ -142,7 +149,7 @@ func fetchRemoteWorkflow(spec *WorkflowSpec, verbose bool) (*FetchedWorkflow, er }, nil } -func resolveCommitSHAWithRetries(owner, repo, ref, workflowPath, host string, verbose bool) (string, error) { +func resolveCommitSHAWithRetries(ctx context.Context, owner, repo, ref, workflowPath, host string, verbose bool) (string, error) { attempts := len(shaResolutionRetryDelays) + 1 var lastErr error @@ -158,12 +165,10 @@ func resolveCommitSHAWithRetries(owner, repo, ref, workflowPath, host string, ve if !isTransientSHAResolutionError(err) { retryCommand := fmt.Sprintf("gh aw add %s/%s/%s@<40-char-sha>", owner, repo, workflowPath) - message := fmt.Sprintf( - "failed to resolve '%s' to commit SHA for '%s/%s': %v. Expected the GitHub API to return a commit SHA for the ref. Try: %s.", - ref, owner, repo, err, retryCommand, + 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, ) - fmt.Fprintln(os.Stderr, console.FormatErrorMessage(message)) - return "", fmt.Errorf("%s: %w", message, err) } if attempt < attempts { @@ -173,17 +178,33 @@ func resolveCommitSHAWithRetries(owner, repo, ref, workflowPath, host string, ve fmt.Sprintf("Transient SHA resolution failure for '%s' (attempt %d/%d). Retrying in %s...", ref, attempt, attempts, delay), )) } - sleepBeforeSHAResolutionRetry(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) - message := fmt.Sprintf( - "failed to resolve '%s' to commit SHA after %d retries for '%s/%s': %v. Expected the GitHub API to return a commit SHA for the ref. Check rate limits or try: %s.", - ref, len(shaResolutionRetryDelays), owner, repo, lastErr, retryCommand, + 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, ) - fmt.Fprintln(os.Stderr, console.FormatErrorMessage(message)) - return "", fmt.Errorf("%s: %w", message, lastErr) +} + +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 diff --git a/pkg/cli/remote_workflow_test.go b/pkg/cli/remote_workflow_test.go index 9aa65bdd72b..cec7a099924 100644 --- a/pkg/cli/remote_workflow_test.go +++ b/pkg/cli/remote_workflow_test.go @@ -147,11 +147,11 @@ func TestFetchWorkflowFromSource_RemoteRoutingWithInvalidSlug(t *testing.T) { func TestResolveCommitSHAWithRetries_TransientFailureThenSuccess(t *testing.T) { originalResolve := resolveRefToSHAForHost - originalSleep := sleepBeforeSHAResolutionRetry + originalWait := waitBeforeSHAResolutionRetry originalDelays := shaResolutionRetryDelays defer func() { resolveRefToSHAForHost = originalResolve - sleepBeforeSHAResolutionRetry = originalSleep + waitBeforeSHAResolutionRetry = originalWait shaResolutionRetryDelays = originalDelays }() @@ -166,11 +166,12 @@ func TestResolveCommitSHAWithRetries_TransientFailureThenSuccess(t *testing.T) { } sleeps := make([]time.Duration, 0) - sleepBeforeSHAResolutionRetry = func(delay time.Duration) { + waitBeforeSHAResolutionRetry = func(ctx context.Context, delay time.Duration) error { sleeps = append(sleeps, delay) + return nil } - sha, err := resolveCommitSHAWithRetries("owner", "repo", "main", ".github/workflows/test.md", "", false) + 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") @@ -179,11 +180,11 @@ func TestResolveCommitSHAWithRetries_TransientFailureThenSuccess(t *testing.T) { func TestResolveCommitSHAWithRetries_PermanentFailureDoesNotRetry(t *testing.T) { originalResolve := resolveRefToSHAForHost - originalSleep := sleepBeforeSHAResolutionRetry + originalWait := waitBeforeSHAResolutionRetry originalDelays := shaResolutionRetryDelays defer func() { resolveRefToSHAForHost = originalResolve - sleepBeforeSHAResolutionRetry = originalSleep + waitBeforeSHAResolutionRetry = originalWait shaResolutionRetryDelays = originalDelays }() @@ -195,11 +196,12 @@ func TestResolveCommitSHAWithRetries_PermanentFailureDoesNotRetry(t *testing.T) } sleepCalls := 0 - sleepBeforeSHAResolutionRetry = func(delay time.Duration) { + waitBeforeSHAResolutionRetry = func(ctx context.Context, delay time.Duration) error { sleepCalls++ + return nil } - sha, err := resolveCommitSHAWithRetries("owner", "repo", "main", ".github/workflows/test.md", "", false) + 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") @@ -211,11 +213,11 @@ func TestResolveCommitSHAWithRetries_PermanentFailureDoesNotRetry(t *testing.T) func TestResolveCommitSHAWithRetries_TransientFailureExhaustsRetries(t *testing.T) { originalResolve := resolveRefToSHAForHost - originalSleep := sleepBeforeSHAResolutionRetry + originalWait := waitBeforeSHAResolutionRetry originalDelays := shaResolutionRetryDelays defer func() { resolveRefToSHAForHost = originalResolve - sleepBeforeSHAResolutionRetry = originalSleep + waitBeforeSHAResolutionRetry = originalWait shaResolutionRetryDelays = originalDelays }() @@ -227,11 +229,12 @@ func TestResolveCommitSHAWithRetries_TransientFailureExhaustsRetries(t *testing. } sleepCalls := 0 - sleepBeforeSHAResolutionRetry = func(delay time.Duration) { + waitBeforeSHAResolutionRetry = func(ctx context.Context, delay time.Duration) error { sleepCalls++ + return nil } - sha, err := resolveCommitSHAWithRetries("owner", "repo", "main", ".github/workflows/test.md", "", false) + 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") @@ -239,6 +242,32 @@ func TestResolveCommitSHAWithRetries_TransientFailureExhaustsRetries(t *testing. 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 context.Canceled + } + + sha, err := resolveCommitSHAWithRetries(context.Background(), "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 { From 895e58f002977b63b3181f1582147297e3a28a48 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 20 Apr 2026 21:26:52 +0000 Subject: [PATCH 05/11] docs: clarify context-aware retry sleep helper behavior Agent-Logs-Url: https://github.com/github/gh-aw/sessions/8ddf9ab4-df4f-4bfa-b6b3-634452eb614e Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/cli/fetch.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/pkg/cli/fetch.go b/pkg/cli/fetch.go index e94d1ed4dfe..c4b69e06637 100644 --- a/pkg/cli/fetch.go +++ b/pkg/cli/fetch.go @@ -195,6 +195,8 @@ func resolveCommitSHAWithRetries(ctx context.Context, owner, repo, ref, workflow ) } +// sleepForSHAResolutionRetry waits for the retry delay or context cancellation. +// It returns ctx.Err() when cancelled, otherwise nil when the delay elapses. func sleepForSHAResolutionRetry(ctx context.Context, delay time.Duration) error { timer := time.NewTimer(delay) defer timer.Stop() From f38becb7771688ac5ec91a9c2e7afbe906ff227d Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 20 Apr 2026 21:28:18 +0000 Subject: [PATCH 06/11] test/docs: refine cancellation test and retry sleep docs Agent-Logs-Url: https://github.com/github/gh-aw/sessions/8ddf9ab4-df4f-4bfa-b6b3-634452eb614e Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/cli/fetch.go | 3 ++- pkg/cli/remote_workflow_test.go | 4 +++- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/pkg/cli/fetch.go b/pkg/cli/fetch.go index c4b69e06637..6f138c57a76 100644 --- a/pkg/cli/fetch.go +++ b/pkg/cli/fetch.go @@ -196,7 +196,8 @@ func resolveCommitSHAWithRetries(ctx context.Context, owner, repo, ref, workflow } // sleepForSHAResolutionRetry waits for the retry delay or context cancellation. -// It returns ctx.Err() when cancelled, otherwise nil when the delay elapses. +// The timer is stopped on return in all cases; it returns ctx.Err() when cancelled +// and nil when the delay elapses normally. func sleepForSHAResolutionRetry(ctx context.Context, delay time.Duration) error { timer := time.NewTimer(delay) defer timer.Stop() diff --git a/pkg/cli/remote_workflow_test.go b/pkg/cli/remote_workflow_test.go index cec7a099924..fd627afcebd 100644 --- a/pkg/cli/remote_workflow_test.go +++ b/pkg/cli/remote_workflow_test.go @@ -258,7 +258,9 @@ func TestResolveCommitSHAWithRetries_ContextCanceledDuringBackoff(t *testing.T) } waitBeforeSHAResolutionRetry = func(ctx context.Context, delay time.Duration) error { - return context.Canceled + cancelledCtx, cancel := context.WithCancel(ctx) + cancel() + return cancelledCtx.Err() } sha, err := resolveCommitSHAWithRetries(context.Background(), "owner", "repo", "main", ".github/workflows/test.md", "", false) From 4f16a512b4209ee786a02a747908a4e8a03f60f1 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 20 Apr 2026 21:29:33 +0000 Subject: [PATCH 07/11] docs/test: align retry cancellation contract and test behavior Agent-Logs-Url: https://github.com/github/gh-aw/sessions/8ddf9ab4-df4f-4bfa-b6b3-634452eb614e Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/cli/fetch.go | 4 ++-- pkg/cli/remote_workflow_test.go | 9 +++++---- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/pkg/cli/fetch.go b/pkg/cli/fetch.go index 6f138c57a76..8b9438d3575 100644 --- a/pkg/cli/fetch.go +++ b/pkg/cli/fetch.go @@ -196,8 +196,8 @@ func resolveCommitSHAWithRetries(ctx context.Context, owner, repo, ref, workflow } // sleepForSHAResolutionRetry waits for the retry delay or context cancellation. -// The timer is stopped on return in all cases; it returns ctx.Err() when cancelled -// and nil when the delay elapses normally. +// 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() diff --git a/pkg/cli/remote_workflow_test.go b/pkg/cli/remote_workflow_test.go index fd627afcebd..92544bc5614 100644 --- a/pkg/cli/remote_workflow_test.go +++ b/pkg/cli/remote_workflow_test.go @@ -258,12 +258,13 @@ func TestResolveCommitSHAWithRetries_ContextCanceledDuringBackoff(t *testing.T) } waitBeforeSHAResolutionRetry = func(ctx context.Context, delay time.Duration) error { - cancelledCtx, cancel := context.WithCancel(ctx) - cancel() - return cancelledCtx.Err() + return ctx.Err() } - sha, err := resolveCommitSHAWithRetries(context.Background(), "owner", "repo", "main", ".github/workflows/test.md", "", false) + 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") From 4d28e3fcb337bfc30866c5b5542eb102ce7fc0f3 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 20 Apr 2026 21:52:32 +0000 Subject: [PATCH 08/11] feat: require pinned action refs by default with allow-action-refs override Agent-Logs-Url: https://github.com/github/gh-aw/sessions/45b2aa41-2063-45e0-838b-d6afb0860d52 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- cmd/gh-aw/main.go | 3 ++ pkg/actionpins/actionpins.go | 12 +++++ pkg/cli/add_command.go | 11 ++-- pkg/cli/add_command_test.go | 3 +- pkg/cli/add_current_repo_test.go | 7 +-- pkg/cli/add_interactive_orchestrator.go | 2 +- pkg/cli/add_workflow_resolution.go | 7 +-- pkg/cli/compile_compiler_setup.go | 1 + pkg/cli/compile_config.go | 1 + pkg/cli/validate_command.go | 35 ++++++------- pkg/workflow/action_pins_logging_test.go | 65 +++++++++++++++++------- pkg/workflow/action_pins_test.go | 2 + pkg/workflow/action_reference_test.go | 12 ++--- pkg/workflow/compiler_types.go | 14 ++++- pkg/workflow/workflow_builder.go | 1 + 15 files changed, 115 insertions(+), 61 deletions(-) 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..96b1da491c9 100644 --- a/pkg/actionpins/actionpins.go +++ b/pkg/actionpins/actionpins.go @@ -57,6 +57,11 @@ 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 @@ -301,6 +306,13 @@ func ResolveActionPin(actionRepo, version string, ctx *PinContext) (string, erro ctx.Warnings = make(map[string]bool) } cacheKey := FormatCacheKey(actionRepo, version) + 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 b88b360278e..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 := AddWorkflowsWithContext(cmd.Context(), workflows, opts) + _, err := AddWorkflows(cmd.Context(), workflows, opts) return err }, } @@ -172,14 +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) { - return AddWorkflowsWithContext(context.Background(), workflows, opts) -} - -// AddWorkflowsWithContext adds one or more workflows and supports cancellation. -func AddWorkflowsWithContext(ctx context.Context, 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 := ResolveWorkflowsWithContext(ctx, workflows, opts.Verbose) + resolved, err := ResolveWorkflows(ctx, workflows, opts.Verbose) if err != nil { return nil, err } 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_interactive_orchestrator.go b/pkg/cli/add_interactive_orchestrator.go index f2732923515..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 := ResolveWorkflowsWithContext(c.Ctx, 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 63d0fc59e3b..21a21e5e364 100644 --- a/pkg/cli/add_workflow_resolution.go +++ b/pkg/cli/add_workflow_resolution.go @@ -47,12 +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) { - return ResolveWorkflowsWithContext(context.Background(), workflows, verbose) -} - -// ResolveWorkflowsWithContext resolves workflow specifications and supports cancellation. -func ResolveWorkflowsWithContext(ctx context.Context, 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 { 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/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..7e8dea1c421 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,37 @@ 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) + } +} + // 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..eae3958dce7 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 @@ -472,6 +479,7 @@ type WorkflowData struct { 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 + 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) @@ -504,8 +512,10 @@ 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, } // 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/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, From e49dddccd8e5a58aa78df4b2182d1f2b31b66227 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 20 Apr 2026 22:06:52 +0000 Subject: [PATCH 09/11] feat: audit unresolved action ref pinning failures in lock manifest Agent-Logs-Url: https://github.com/github/gh-aw/sessions/e68b7938-ba90-4e65-91a4-f434dd9579e2 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/actionpins/actionpins.go | 35 ++++++ pkg/workflow/action_pins_logging_test.go | 7 ++ pkg/workflow/compiler_types.go | 144 ++++++++++++---------- pkg/workflow/compiler_yaml.go | 2 +- pkg/workflow/safe_update_manifest.go | 66 ++++++++-- pkg/workflow/safe_update_manifest_test.go | 29 ++++- 6 files changed, 201 insertions(+), 82 deletions(-) diff --git a/pkg/actionpins/actionpins.go b/pkg/actionpins/actionpins.go index 96b1da491c9..8f7b1e26d96 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. @@ -65,6 +82,8 @@ type PinContext struct { // 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 ( @@ -215,6 +234,17 @@ func findCompatiblePin(pins []ActionPin, version string) (ActionPin, bool) { return ActionPin{}, false } +func recordResolutionFailure(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. @@ -306,6 +336,11 @@ 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 + } + recordResolutionFailure(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) diff --git a/pkg/workflow/action_pins_logging_test.go b/pkg/workflow/action_pins_logging_test.go index 7e8dea1c421..167e870285e 100644 --- a/pkg/workflow/action_pins_logging_test.go +++ b/pkg/workflow/action_pins_logging_test.go @@ -223,6 +223,13 @@ func TestActionPinResolutionWithAllowActionRefs(t *testing.T) { 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" || failure.Ref != "v1" || failure.ErrorType != "pin_not_found" { + t.Fatalf("Unexpected recorded resolution failure: %+v", failure) + } } // TestActionCacheDuplicateSHAWarning verifies that we log warnings when multiple diff --git a/pkg/workflow/compiler_types.go b/pkg/workflow/compiler_types.go index eae3958dce7..9903ce26be0 100644 --- a/pkg/workflow/compiler_types.go +++ b/pkg/workflow/compiler_types.go @@ -431,74 +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 - 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 + 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. @@ -516,6 +517,13 @@ func (d *WorkflowData) PinContext() *actionpins.PinContext { 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..0e13cb596ea 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,39 @@ func parseActionRefs(refs []string) []GHAWManifestAction { return actions } +func normalizeResolutionFailures(failures []GHAWManifestResolutionFailure) []GHAWManifestResolutionFailure { + seen := make(map[string]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 := repo + "@" + ref + ":" + 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..407ea9c05c2 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 + wantFailureTypes []string wantContainerImages []string wantRedirect string }{ @@ -76,6 +78,17 @@ func TestNewGHAWManifest(t *testing.T) { wantSecrets: []string{}, wantActionRepos: []string{"actions/checkout"}, }, + { + name: "resolution failures are sorted and deduplicated", + resolutionFailures: []GHAWManifestResolutionFailure{ + {Repo: "actions/setup-node", Ref: "v6", ErrorType: "dynamic_resolution_failed"}, + {Repo: "actions/setup-node", Ref: "v6", ErrorType: "dynamic_resolution_failed"}, + {Repo: "actions/checkout", Ref: "v5", ErrorType: "pin_not_found"}, + }, + wantVersion: 1, + wantSecrets: []string{}, + wantFailureTypes: []string{"pin_not_found", "dynamic_resolution_failed"}, + }, { name: "containers are sorted and deduplicated", containers: []GHAWManifestContainer{ @@ -120,7 +133,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 +153,13 @@ func TestNewGHAWManifest(t *testing.T) { } assert.Equal(t, tt.wantContainerImages, images, "container images") } + if tt.wantFailureTypes != nil { + types := make([]string, len(m.ResolutionFailures)) + for i, f := range m.ResolutionFailures { + types[i] = f.ErrorType + } + assert.Equal(t, tt.wantFailureTypes, types, "resolution failure types") + } assert.Equal(t, tt.wantRedirect, m.Redirect, "manifest redirect") }) } @@ -156,7 +176,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 +207,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 +221,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") } From c7052858f6e17df985044d147b7bf07d174ebd74 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 20 Apr 2026 22:12:50 +0000 Subject: [PATCH 10/11] test: strengthen manifest failure auditing assertions Agent-Logs-Url: https://github.com/github/gh-aw/sessions/e68b7938-ba90-4e65-91a4-f434dd9579e2 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/workflow/action_pins_logging_test.go | 10 ++++++++-- pkg/workflow/safe_update_manifest_test.go | 23 ++++++++++++----------- 2 files changed, 20 insertions(+), 13 deletions(-) diff --git a/pkg/workflow/action_pins_logging_test.go b/pkg/workflow/action_pins_logging_test.go index 167e870285e..154d243970c 100644 --- a/pkg/workflow/action_pins_logging_test.go +++ b/pkg/workflow/action_pins_logging_test.go @@ -227,8 +227,14 @@ func TestActionPinResolutionWithAllowActionRefs(t *testing.T) { t.Fatalf("Expected one recorded resolution failure, got: %d", len(data.ActionResolutionFailures)) } failure := data.ActionResolutionFailures[0] - if failure.Repo != "actions/ai-inference" || failure.Ref != "v1" || failure.ErrorType != "pin_not_found" { - t.Fatalf("Unexpected recorded resolution failure: %+v", failure) + 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) } } diff --git a/pkg/workflow/safe_update_manifest_test.go b/pkg/workflow/safe_update_manifest_test.go index 407ea9c05c2..ae81cd431a7 100644 --- a/pkg/workflow/safe_update_manifest_test.go +++ b/pkg/workflow/safe_update_manifest_test.go @@ -20,7 +20,7 @@ func TestNewGHAWManifest(t *testing.T) { wantVersion int wantSecrets []string wantActionRepos []string - wantFailureTypes []string + wantFailures []GHAWManifestResolutionFailure wantContainerImages []string wantRedirect string }{ @@ -79,15 +79,20 @@ func TestNewGHAWManifest(t *testing.T) { wantActionRepos: []string{"actions/checkout"}, }, { - name: "resolution failures are sorted and deduplicated", + 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{}, - wantFailureTypes: []string{"pin_not_found", "dynamic_resolution_failed"}, + 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", @@ -153,12 +158,8 @@ func TestNewGHAWManifest(t *testing.T) { } assert.Equal(t, tt.wantContainerImages, images, "container images") } - if tt.wantFailureTypes != nil { - types := make([]string, len(m.ResolutionFailures)) - for i, f := range m.ResolutionFailures { - types[i] = f.ErrorType - } - assert.Equal(t, tt.wantFailureTypes, types, "resolution failure types") + if tt.wantFailures != nil { + assert.Equal(t, tt.wantFailures, m.ResolutionFailures, "resolution failures") } assert.Equal(t, tt.wantRedirect, m.Redirect, "manifest redirect") }) From f2ebd1e1b0ecfc7971231c5a87d4d16af22f1c4f Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 20 Apr 2026 22:19:09 +0000 Subject: [PATCH 11/11] refactor: tighten failure audit normalization helpers Agent-Logs-Url: https://github.com/github/gh-aw/sessions/e68b7938-ba90-4e65-91a4-f434dd9579e2 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/actionpins/actionpins.go | 4 ++-- pkg/workflow/safe_update_manifest.go | 9 +++++++-- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/pkg/actionpins/actionpins.go b/pkg/actionpins/actionpins.go index 8f7b1e26d96..2a560b27421 100644 --- a/pkg/actionpins/actionpins.go +++ b/pkg/actionpins/actionpins.go @@ -234,7 +234,7 @@ func findCompatiblePin(pins []ActionPin, version string) (ActionPin, bool) { return ActionPin{}, false } -func recordResolutionFailure(ctx *PinContext, actionRepo, version string, errorType ResolutionErrorType) { +func notifyResolutionFailure(ctx *PinContext, actionRepo, version string, errorType ResolutionErrorType) { if ctx == nil || ctx.RecordResolutionFailure == nil { return } @@ -340,7 +340,7 @@ func ResolveActionPin(actionRepo, version string, ctx *PinContext) (string, erro if ctx.Resolver != nil { errorType = ResolutionErrorTypeDynamicResolutionFailed } - recordResolutionFailure(ctx, actionRepo, version, errorType) + 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) diff --git a/pkg/workflow/safe_update_manifest.go b/pkg/workflow/safe_update_manifest.go index 0e13cb596ea..75288fc984a 100644 --- a/pkg/workflow/safe_update_manifest.go +++ b/pkg/workflow/safe_update_manifest.go @@ -176,7 +176,12 @@ func parseActionRefs(refs []string) []GHAWManifestAction { } func normalizeResolutionFailures(failures []GHAWManifestResolutionFailure) []GHAWManifestResolutionFailure { - seen := make(map[string]bool) + 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) @@ -185,7 +190,7 @@ func normalizeResolutionFailures(failures []GHAWManifestResolutionFailure) []GHA if repo == "" || ref == "" || errorType == "" { continue } - key := repo + "@" + ref + ":" + errorType + key := failureKey{Repo: repo, Ref: ref, ErrorType: errorType} if seen[key] { continue }