From 83136d03f678268527566deca9ee724c64d7bfbe Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 25 Jul 2026 03:48:18 +0000 Subject: [PATCH 1/5] feat(update): validate action and container SHAs after update Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/cli/update_command.go | 10 ++++ pkg/cli/update_validation.go | 81 +++++++++++++++++++++++++++++++ pkg/cli/update_validation_test.go | 70 ++++++++++++++++++++++++++ 3 files changed, 161 insertions(+) create mode 100644 pkg/cli/update_validation.go create mode 100644 pkg/cli/update_validation_test.go diff --git a/pkg/cli/update_command.go b/pkg/cli/update_command.go index ed2c9f3662d..75bb382b107 100644 --- a/pkg/cli/update_command.go +++ b/pkg/cli/update_command.go @@ -249,6 +249,16 @@ func RunUpdateWorkflows(ctx context.Context, opts UpdateWorkflowsOptions) error } } + if firstErr == nil { + updateLog.Print("Validating action and container SHAs in actions-lock.json") + if err := validateUpdateSHAEntries("."); err != nil { + return fmt.Errorf("update validation failed: %w", err) + } + if opts.Verbose { + fmt.Fprintln(os.Stderr, console.FormatInfoMessage("Validated action and container SHAs in actions-lock.json")) + } + } + updateLog.Printf("Update process complete: had_error=%v", firstErr != nil) return firstErr } diff --git a/pkg/cli/update_validation.go b/pkg/cli/update_validation.go new file mode 100644 index 00000000000..41f1f7ac495 --- /dev/null +++ b/pkg/cli/update_validation.go @@ -0,0 +1,81 @@ +package cli + +import ( + "fmt" + "os" + "path/filepath" + "regexp" + "sort" + "strings" + + "github.com/github/gh-aw/pkg/workflow" +) + +var sha256DigestPattern = regexp.MustCompile(`^sha256:[a-f0-9]{64}$`) + +func validateUpdateSHAEntries(repoRoot string) error { + actionsLockPath := filepath.Join(repoRoot, ".github", "aw", "actions-lock.json") + if _, err := os.Stat(actionsLockPath); err != nil { + if os.IsNotExist(err) { + return nil + } + return fmt.Errorf("failed to read actions-lock.json metadata: %w", err) + } + + cache := workflow.NewActionCache(repoRoot) + if err := cache.Load(); err != nil { + return fmt.Errorf("failed to load actions-lock.json: %w", err) + } + + var issues []string + + entryKeys := make([]string, 0, len(cache.Entries)) + for key := range cache.Entries { + entryKeys = append(entryKeys, key) + } + sort.Strings(entryKeys) + for _, key := range entryKeys { + entry := cache.Entries[key] + if entry.Repo == "" { + issues = append(issues, fmt.Sprintf("action entry %q has empty repo", key)) + } + if entry.Version == "" { + issues = append(issues, fmt.Sprintf("action entry %q has empty version", key)) + } + if entry.SHA == "" { + issues = append(issues, fmt.Sprintf("action entry %q has empty SHA", key)) + } else if !IsCommitSHA(entry.SHA) { + issues = append(issues, fmt.Sprintf("action entry %q has invalid SHA %q (expected 40-character commit SHA)", key, entry.SHA)) + } + if entry.Repo != "" && entry.Version != "" { + expectedKey := entry.Repo + "@" + entry.Version + if key != expectedKey { + issues = append(issues, fmt.Sprintf("action entry key/version mismatch: key %q should be %q", key, expectedKey)) + } + } + } + + containerKeys := make([]string, 0, len(cache.ContainerPins)) + for image := range cache.ContainerPins { + containerKeys = append(containerKeys, image) + } + sort.Strings(containerKeys) + for _, image := range containerKeys { + pin := cache.ContainerPins[image] + if pin.Image != image { + issues = append(issues, fmt.Sprintf("container pin key/image mismatch: key %q has image %q", image, pin.Image)) + } + if !sha256DigestPattern.MatchString(pin.Digest) { + issues = append(issues, fmt.Sprintf("container pin %q has invalid digest %q (expected sha256:<64 lowercase hex chars>)", image, pin.Digest)) + } + expectedPinnedImage := image + "@" + pin.Digest + if pin.PinnedImage != expectedPinnedImage { + issues = append(issues, fmt.Sprintf("container pin %q has inconsistent pinned_image %q (expected %q)", image, pin.PinnedImage, expectedPinnedImage)) + } + } + + if len(issues) > 0 { + return fmt.Errorf("actions-lock.json validation failed:\n - %s", strings.Join(issues, "\n - ")) + } + return nil +} diff --git a/pkg/cli/update_validation_test.go b/pkg/cli/update_validation_test.go new file mode 100644 index 00000000000..d0e319542f3 --- /dev/null +++ b/pkg/cli/update_validation_test.go @@ -0,0 +1,70 @@ +//go:build !integration + +package cli + +import ( + "os" + "path/filepath" + "strings" + "testing" + + "github.com/github/gh-aw/pkg/testutil" + "github.com/github/gh-aw/pkg/workflow" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestValidateUpdateSHAEntries_NoActionsLock(t *testing.T) { + tmpDir := testutil.TempDir(t, "validate-update-sha-*") + require.NoError(t, validateUpdateSHAEntries(tmpDir)) +} + +func TestValidateUpdateSHAEntries_ValidEntries(t *testing.T) { + tmpDir := testutil.TempDir(t, "validate-update-sha-*") + cache := workflow.NewActionCache(tmpDir) + cache.Set("actions/checkout", "v5", "93cb6efe18208431cddfb8368fd83d5badbf9bfd") + digest := "sha256:" + strings.Repeat("a", 64) + image := "ghcr.io/github/gh-aw-firewall/agent:0.27.9" + cache.SetContainerPin(image, digest, image+"@"+digest) + require.NoError(t, cache.Save()) + + require.NoError(t, validateUpdateSHAEntries(tmpDir)) +} + +func TestValidateUpdateSHAEntries_InvalidEntries(t *testing.T) { + tmpDir := testutil.TempDir(t, "validate-update-sha-*") + awDir := filepath.Join(tmpDir, ".github", "aw") + require.NoError(t, os.MkdirAll(awDir, 0o755)) + + const invalidActionsLock = `{ + "entries": { + "actions/checkout@v5": { + "repo": "actions/checkout", + "version": "v5", + "sha": "short" + }, + "actions/setup-node@v6": { + "repo": "actions/setup-node", + "version": "v7", + "sha": "395ad3262231945c25e8478fd5baf05154b1d79f" + } + }, + "containers": { + "ghcr.io/test/image:v1": { + "image": "ghcr.io/test/other:v1", + "digest": "sha256:XYZ", + "pinned_image": "ghcr.io/test/image:v1@sha256:bad" + } + } +} +` + require.NoError(t, os.WriteFile(filepath.Join(awDir, "actions-lock.json"), []byte(invalidActionsLock), 0o644)) + + err := validateUpdateSHAEntries(tmpDir) + require.Error(t, err) + assert.Contains(t, err.Error(), `action entry "actions/checkout@v5" has invalid SHA`) + assert.Contains(t, err.Error(), `action entry key/version mismatch: key "actions/setup-node@v6" should be "actions/setup-node@v7"`) + assert.Contains(t, err.Error(), `container pin key/image mismatch`) + assert.Contains(t, err.Error(), `container pin "ghcr.io/test/image:v1" has invalid digest`) + assert.Contains(t, err.Error(), `container pin "ghcr.io/test/image:v1" has inconsistent pinned_image`) +} From 337093622230a305b8136013b951eee6a6aa09cb Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 25 Jul 2026 04:09:58 +0000 Subject: [PATCH 2/5] Validate resolved action and container SHAs during update Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/cli/update_command.go | 2 +- pkg/cli/update_validation.go | 42 +++++++++++++++++++++++++++- pkg/cli/update_validation_test.go | 46 +++++++++++++++++++++++++++++-- 3 files changed, 85 insertions(+), 5 deletions(-) diff --git a/pkg/cli/update_command.go b/pkg/cli/update_command.go index 75bb382b107..a7922bbdc02 100644 --- a/pkg/cli/update_command.go +++ b/pkg/cli/update_command.go @@ -251,7 +251,7 @@ func RunUpdateWorkflows(ctx context.Context, opts UpdateWorkflowsOptions) error if firstErr == nil { updateLog.Print("Validating action and container SHAs in actions-lock.json") - if err := validateUpdateSHAEntries("."); err != nil { + if err := validateUpdateSHAEntries(ctx, "."); err != nil { return fmt.Errorf("update validation failed: %w", err) } if opts.Verbose { diff --git a/pkg/cli/update_validation.go b/pkg/cli/update_validation.go index 41f1f7ac495..30fe317e52b 100644 --- a/pkg/cli/update_validation.go +++ b/pkg/cli/update_validation.go @@ -1,6 +1,7 @@ package cli import ( + "context" "fmt" "os" "path/filepath" @@ -8,12 +9,31 @@ import ( "sort" "strings" + "github.com/github/gh-aw/pkg/gitutil" + "github.com/github/gh-aw/pkg/parser" "github.com/github/gh-aw/pkg/workflow" ) var sha256DigestPattern = regexp.MustCompile(`^sha256:[a-f0-9]{64}$`) -func validateUpdateSHAEntries(repoRoot string) error { +var resolveActionRefSHAForValidation = func(ctx context.Context, repo, ref string) (string, error) { + baseRepo := gitutil.ExtractBaseRepo(repo) + owner, name, ok := strings.Cut(baseRepo, "/") + if !ok || owner == "" || name == "" { + return "", fmt.Errorf("invalid action repository %q", repo) + } + return parser.ResolveRefToSHAForHost(ctx, owner, name, ref, "") +} + +var resolveContainerDigestForValidation = func(ctx context.Context, image string) (string, error) { + return fetchContainerDigest(ctx, image, false) +} + +func validateUpdateSHAEntries(ctx context.Context, repoRoot string) error { + if ctx == nil { + ctx = context.Background() + } + actionsLockPath := filepath.Join(repoRoot, ".github", "aw", "actions-lock.json") if _, err := os.Stat(actionsLockPath); err != nil { if os.IsNotExist(err) { @@ -46,12 +66,25 @@ func validateUpdateSHAEntries(repoRoot string) error { issues = append(issues, fmt.Sprintf("action entry %q has empty SHA", key)) } else if !IsCommitSHA(entry.SHA) { issues = append(issues, fmt.Sprintf("action entry %q has invalid SHA %q (expected 40-character commit SHA)", key, entry.SHA)) + } else if entry.Repo != "" { + resolvedSHA, err := resolveActionRefSHAForValidation(ctx, entry.Repo, entry.SHA) + if err != nil { + issues = append(issues, fmt.Sprintf("action entry %q has unresolved commit SHA %q in %q: %v", key, entry.SHA, entry.Repo, err)) + } else if !strings.EqualFold(resolvedSHA, entry.SHA) { + issues = append(issues, fmt.Sprintf("action entry %q resolved commit SHA mismatch: got %q from %q", key, resolvedSHA, entry.SHA)) + } } if entry.Repo != "" && entry.Version != "" { expectedKey := entry.Repo + "@" + entry.Version if key != expectedKey { issues = append(issues, fmt.Sprintf("action entry key/version mismatch: key %q should be %q", key, expectedKey)) } + resolvedVersionSHA, err := resolveActionRefSHAForValidation(ctx, entry.Repo, entry.Version) + if err != nil { + issues = append(issues, fmt.Sprintf("action entry %q has unresolved version %q in %q: %v", key, entry.Version, entry.Repo, err)) + } else if entry.SHA != "" && IsCommitSHA(entry.SHA) && !strings.EqualFold(resolvedVersionSHA, entry.SHA) { + issues = append(issues, fmt.Sprintf("action entry %q SHA/version mismatch: version %q resolves to %q but entry has %q", key, entry.Version, resolvedVersionSHA, entry.SHA)) + } } } @@ -67,6 +100,13 @@ func validateUpdateSHAEntries(repoRoot string) error { } if !sha256DigestPattern.MatchString(pin.Digest) { issues = append(issues, fmt.Sprintf("container pin %q has invalid digest %q (expected sha256:<64 lowercase hex chars>)", image, pin.Digest)) + } else { + resolvedDigest, err := resolveContainerDigestForValidation(ctx, image) + if err != nil { + issues = append(issues, fmt.Sprintf("container pin %q digest could not be resolved for %q: %v", image, image, err)) + } else if resolvedDigest != pin.Digest { + issues = append(issues, fmt.Sprintf("container pin %q digest/version mismatch: expected %q but resolved %q", image, pin.Digest, resolvedDigest)) + } } expectedPinnedImage := image + "@" + pin.Digest if pin.PinnedImage != expectedPinnedImage { diff --git a/pkg/cli/update_validation_test.go b/pkg/cli/update_validation_test.go index d0e319542f3..7e67bbaf997 100644 --- a/pkg/cli/update_validation_test.go +++ b/pkg/cli/update_validation_test.go @@ -3,6 +3,8 @@ package cli import ( + "context" + "errors" "os" "path/filepath" "strings" @@ -16,7 +18,7 @@ import ( func TestValidateUpdateSHAEntries_NoActionsLock(t *testing.T) { tmpDir := testutil.TempDir(t, "validate-update-sha-*") - require.NoError(t, validateUpdateSHAEntries(tmpDir)) + require.NoError(t, validateUpdateSHAEntries(context.Background(), tmpDir)) } func TestValidateUpdateSHAEntries_ValidEntries(t *testing.T) { @@ -28,7 +30,23 @@ func TestValidateUpdateSHAEntries_ValidEntries(t *testing.T) { cache.SetContainerPin(image, digest, image+"@"+digest) require.NoError(t, cache.Save()) - require.NoError(t, validateUpdateSHAEntries(tmpDir)) + origResolveAction := resolveActionRefSHAForValidation + origResolveContainer := resolveContainerDigestForValidation + resolveActionRefSHAForValidation = func(_ context.Context, _, ref string) (string, error) { + if ref == "v5" { + return "93cb6efe18208431cddfb8368fd83d5badbf9bfd", nil + } + return ref, nil + } + resolveContainerDigestForValidation = func(_ context.Context, _ string) (string, error) { + return digest, nil + } + t.Cleanup(func() { + resolveActionRefSHAForValidation = origResolveAction + resolveContainerDigestForValidation = origResolveContainer + }) + + require.NoError(t, validateUpdateSHAEntries(context.Background(), tmpDir)) } func TestValidateUpdateSHAEntries_InvalidEntries(t *testing.T) { @@ -60,10 +78,32 @@ func TestValidateUpdateSHAEntries_InvalidEntries(t *testing.T) { ` require.NoError(t, os.WriteFile(filepath.Join(awDir, "actions-lock.json"), []byte(invalidActionsLock), 0o644)) - err := validateUpdateSHAEntries(tmpDir) + origResolveAction := resolveActionRefSHAForValidation + origResolveContainer := resolveContainerDigestForValidation + resolveActionRefSHAForValidation = func(_ context.Context, repo, ref string) (string, error) { + switch { + case repo == "actions/setup-node" && ref == "v7": + return "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", nil + case ref == "395ad3262231945c25e8478fd5baf05154b1d79f": + return "", errors.New("commit not found") + default: + return ref, nil + } + } + resolveContainerDigestForValidation = func(_ context.Context, _ string) (string, error) { + return "sha256:" + strings.Repeat("b", 64), nil + } + t.Cleanup(func() { + resolveActionRefSHAForValidation = origResolveAction + resolveContainerDigestForValidation = origResolveContainer + }) + + err := validateUpdateSHAEntries(context.Background(), tmpDir) require.Error(t, err) assert.Contains(t, err.Error(), `action entry "actions/checkout@v5" has invalid SHA`) assert.Contains(t, err.Error(), `action entry key/version mismatch: key "actions/setup-node@v6" should be "actions/setup-node@v7"`) + assert.Contains(t, err.Error(), `action entry "actions/setup-node@v6" has unresolved commit SHA`) + assert.Contains(t, err.Error(), `action entry "actions/setup-node@v6" SHA/version mismatch`) assert.Contains(t, err.Error(), `container pin key/image mismatch`) assert.Contains(t, err.Error(), `container pin "ghcr.io/test/image:v1" has invalid digest`) assert.Contains(t, err.Error(), `container pin "ghcr.io/test/image:v1" has inconsistent pinned_image`) From 3519492f8c8e83d57c91c6a9dc5b4b8df5dc4a5d Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Sat, 25 Jul 2026 04:28:28 +0000 Subject: [PATCH 3/5] docs(adr): add draft ADR-47907 for post-update SHA integrity validation --- ...e-sha-integrity-validation-actions-lock.md | 44 +++++++++++++++++++ 1 file changed, 44 insertions(+) create mode 100644 docs/adr/47907-post-update-sha-integrity-validation-actions-lock.md diff --git a/docs/adr/47907-post-update-sha-integrity-validation-actions-lock.md b/docs/adr/47907-post-update-sha-integrity-validation-actions-lock.md new file mode 100644 index 00000000000..13eafa48990 --- /dev/null +++ b/docs/adr/47907-post-update-sha-integrity-validation-actions-lock.md @@ -0,0 +1,44 @@ +# ADR-47907: Post-Update SHA Integrity Validation for actions-lock.json + +**Date**: 2026-07-25 +**Status**: Draft +**Deciders**: pelikhan, copilot-swe-agent + +--- + +### Context + +The `gh aw update` command resolves and pins GitHub Action commit SHAs and container image digests into `.github/aw/actions-lock.json`. Prior to this change, the file was written without any final structural check, meaning malformed entries (e.g., a truncated SHA, a mismatched map key, or an incorrectly formatted container digest) could be silently persisted and later used in production workflows. The only time a bad entry would be caught was at workflow execution time — far downstream from where the corruption was introduced. + +### Decision + +We will add a terminal validation phase at the end of `RunUpdateWorkflows` that re-reads and structurally verifies all entries in `actions-lock.json` after the update, recompile, and pin-refresh steps complete. Validation failures surface as a hard error (`update validation failed: ...`) rather than being silently ignored. This validation only runs when no earlier error occurred (`firstErr == nil`), so it does not mask upstream failures. + +### Alternatives Considered + +#### Alternative 1: Inline validation during update (validate-as-you-write) + +Each action or container entry could be validated at the point it is written into the cache, as part of the existing resolve-and-pin logic. This would catch errors closer to their origin and avoid re-reading the file. It was not chosen because it would require threading validation logic into multiple existing write paths (action resolution, container pinning, recompile step), significantly increasing the coupling and risk of regressions. A single terminal pass is simpler and keeps validation isolated from write logic. + +#### Alternative 2: No post-update validation (status quo) + +The existing approach relied on correctness guarantees from the upstream resolution APIs and the write path. Malformed entries would only surface at workflow-run time. This was rejected because supply-chain integrity requires catching bad pins before they are persisted; silent failures at the lock-write layer are a security risk in an action-pinning tool. + +### Consequences + +#### Positive +- Malformed or inconsistent action commit SHAs and container digests are caught immediately at update time, before being committed to the repository. +- The validator produces aggregated, human-readable diagnostics covering all invalid entries at once, rather than stopping at the first error. +- Validation logic is isolated in a dedicated file (`update_validation.go`) with injected resolver functions, making it straightforward to unit-test without real network calls. + +#### Negative +- The terminal validation phase makes additional GitHub API calls (one per action entry, one per container entry) to re-resolve SHAs. For repositories with many pinned actions this adds latency to every `gh aw update` invocation. +- Transient API failures during validation will cause the update command to return an error even when the lock file is structurally correct, potentially creating spurious failures in CI. + +#### Neutral +- The validation step is gated on `firstErr == nil`, so it does not run when the main update phase already encountered an error. This is consistent with the existing error-propagation strategy but means validation is skipped on partial updates. +- Verbose mode logs a confirmation message to stderr when validation succeeds, matching the existing log pattern used elsewhere in the update flow. + +--- + +*ADR created by [adr-writer agent]. Review and finalize before changing status from Draft to Accepted.* From 2d8122a7b455252832f120b41d865a3af9c400d7 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 25 Jul 2026 04:59:13 +0000 Subject: [PATCH 4/5] fix(update-validation): real commit existence check, non-fatal network errors, container mismatch as warning MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Add VerifyCommitExists to parser package: calls GitHub commits API for the given SHA without short-circuiting (unlike ResolveRefToSHAForHost which returns full SHAs unchanged). Returns ErrVerificationSkipped (wrapped) for auth/network failures so callers can skip the check non-fatally. - Split resolveActionRefSHAForValidation into two injectable vars: verifyActionCommitExistsForValidation (commit existence) and resolveActionVersionToSHAForValidation (version-tag→SHA). - Commit existence check: uses VerifyCommitExists; ErrVerificationSkipped is logged and skipped; other errors (e.g. 404) are hard failures. - Version→SHA check: resolution failures (auth/network) are logged and skipped; only confirmed mismatches are reported as errors. - Container digest: lookup failures are logged and skipped (non-fatal); digest mismatches are surfaced as a warning on stderr rather than a hard failure, because UpdateContainerPins cannot auto-repair a drifted mutable tag. - Add two new tests: NonFatalErrors (all lookups fail → no error) and ContainerDigestMismatchIsWarning (digest drift → warning, no error). Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/cli/update_validation.go | 64 ++++++++++++++++----- pkg/cli/update_validation_test.go | 95 ++++++++++++++++++++++++++----- pkg/parser/remote_resolve_sha.go | 26 +++++++++ 3 files changed, 158 insertions(+), 27 deletions(-) diff --git a/pkg/cli/update_validation.go b/pkg/cli/update_validation.go index 30fe317e52b..6718253ab47 100644 --- a/pkg/cli/update_validation.go +++ b/pkg/cli/update_validation.go @@ -2,6 +2,7 @@ package cli import ( "context" + "errors" "fmt" "os" "path/filepath" @@ -9,14 +10,20 @@ import ( "sort" "strings" + "github.com/github/gh-aw/pkg/console" "github.com/github/gh-aw/pkg/gitutil" + "github.com/github/gh-aw/pkg/logger" "github.com/github/gh-aw/pkg/parser" "github.com/github/gh-aw/pkg/workflow" ) +var updateValidationLog = logger.New("cli:update_validation") + var sha256DigestPattern = regexp.MustCompile(`^sha256:[a-f0-9]{64}$`) -var resolveActionRefSHAForValidation = func(ctx context.Context, repo, ref string) (string, error) { +// resolveActionVersionToSHAForValidation resolves a version tag/ref to its commit SHA. +// Used to verify that the stored SHA in the lock entry matches the pinned version. +var resolveActionVersionToSHAForValidation = func(ctx context.Context, repo, ref string) (string, error) { baseRepo := gitutil.ExtractBaseRepo(repo) owner, name, ok := strings.Cut(baseRepo, "/") if !ok || owner == "" || name == "" { @@ -25,6 +32,18 @@ var resolveActionRefSHAForValidation = func(ctx context.Context, repo, ref strin return parser.ResolveRefToSHAForHost(ctx, owner, name, ref, "") } +// verifyActionCommitExistsForValidation checks that a commit SHA actually exists in the +// action repository. Returns parser.ErrVerificationSkipped for auth/network failures so +// callers can treat those as non-fatal. +var verifyActionCommitExistsForValidation = func(ctx context.Context, repo, sha string) error { + baseRepo := gitutil.ExtractBaseRepo(repo) + owner, name, ok := strings.Cut(baseRepo, "/") + if !ok || owner == "" || name == "" { + return fmt.Errorf("invalid action repository %q", repo) + } + return parser.VerifyCommitExists(ctx, owner, name, sha, "") +} + var resolveContainerDigestForValidation = func(ctx context.Context, image string) (string, error) { return fetchContainerDigest(ctx, image, false) } @@ -62,16 +81,23 @@ func validateUpdateSHAEntries(ctx context.Context, repoRoot string) error { if entry.Version == "" { issues = append(issues, fmt.Sprintf("action entry %q has empty version", key)) } + validSHA := false if entry.SHA == "" { issues = append(issues, fmt.Sprintf("action entry %q has empty SHA", key)) } else if !IsCommitSHA(entry.SHA) { issues = append(issues, fmt.Sprintf("action entry %q has invalid SHA %q (expected 40-character commit SHA)", key, entry.SHA)) - } else if entry.Repo != "" { - resolvedSHA, err := resolveActionRefSHAForValidation(ctx, entry.Repo, entry.SHA) - if err != nil { - issues = append(issues, fmt.Sprintf("action entry %q has unresolved commit SHA %q in %q: %v", key, entry.SHA, entry.Repo, err)) - } else if !strings.EqualFold(resolvedSHA, entry.SHA) { - issues = append(issues, fmt.Sprintf("action entry %q resolved commit SHA mismatch: got %q from %q", key, resolvedSHA, entry.SHA)) + } else { + validSHA = true + // Verify the commit SHA actually exists in the repository. Auth/network + // failures are non-fatal and logged; only a definitive not-found is an error. + if entry.Repo != "" { + if err := verifyActionCommitExistsForValidation(ctx, entry.Repo, entry.SHA); err != nil { + if errors.Is(err, parser.ErrVerificationSkipped) { + updateValidationLog.Printf("action entry %q: skipping commit existence check (auth/network error): %v", key, err) + } else { + issues = append(issues, fmt.Sprintf("action entry %q commit SHA %q does not exist in %q: %v", key, entry.SHA, entry.Repo, err)) + } + } } } if entry.Repo != "" && entry.Version != "" { @@ -79,11 +105,15 @@ func validateUpdateSHAEntries(ctx context.Context, repoRoot string) error { if key != expectedKey { issues = append(issues, fmt.Sprintf("action entry key/version mismatch: key %q should be %q", key, expectedKey)) } - resolvedVersionSHA, err := resolveActionRefSHAForValidation(ctx, entry.Repo, entry.Version) - if err != nil { - issues = append(issues, fmt.Sprintf("action entry %q has unresolved version %q in %q: %v", key, entry.Version, entry.Repo, err)) - } else if entry.SHA != "" && IsCommitSHA(entry.SHA) && !strings.EqualFold(resolvedVersionSHA, entry.SHA) { - issues = append(issues, fmt.Sprintf("action entry %q SHA/version mismatch: version %q resolves to %q but entry has %q", key, entry.Version, resolvedVersionSHA, entry.SHA)) + if validSHA { + // Verify the stored version tag resolves to the stored SHA. + // Auth/network failures are non-fatal; only a confirmed mismatch is an error. + resolvedVersionSHA, err := resolveActionVersionToSHAForValidation(ctx, entry.Repo, entry.Version) + if err != nil { + updateValidationLog.Printf("action entry %q: skipping version/SHA check (resolution failed): %v", key, err) + } else if !strings.EqualFold(resolvedVersionSHA, entry.SHA) { + issues = append(issues, fmt.Sprintf("action entry %q SHA/version mismatch: version %q resolves to %q but entry has %q", key, entry.Version, resolvedVersionSHA, entry.SHA)) + } } } } @@ -101,11 +131,17 @@ func validateUpdateSHAEntries(ctx context.Context, repoRoot string) error { if !sha256DigestPattern.MatchString(pin.Digest) { issues = append(issues, fmt.Sprintf("container pin %q has invalid digest %q (expected sha256:<64 lowercase hex chars>)", image, pin.Digest)) } else { + // Verify the stored digest matches the current digest for the image tag. + // Lookup failures are non-fatal (e.g. registry unavailable). A mismatch is + // surfaced as a warning rather than a hard failure because a mutable tag may + // have been updated after pinning and the update command cannot auto-repair it. resolvedDigest, err := resolveContainerDigestForValidation(ctx, image) if err != nil { - issues = append(issues, fmt.Sprintf("container pin %q digest could not be resolved for %q: %v", image, image, err)) + updateValidationLog.Printf("container pin %q: skipping digest check (lookup failed): %v", image, err) } else if resolvedDigest != pin.Digest { - issues = append(issues, fmt.Sprintf("container pin %q digest/version mismatch: expected %q but resolved %q", image, pin.Digest, resolvedDigest)) + fmt.Fprintln(os.Stderr, console.FormatWarningMessage( + fmt.Sprintf("container pin %q digest is stale: stored %q but image tag resolves to %q; re-run 'gh aw update' after removing the pin to refresh", image, pin.Digest, resolvedDigest), + )) } } expectedPinnedImage := image + "@" + pin.Digest diff --git a/pkg/cli/update_validation_test.go b/pkg/cli/update_validation_test.go index 7e67bbaf997..94df1edc9c2 100644 --- a/pkg/cli/update_validation_test.go +++ b/pkg/cli/update_validation_test.go @@ -5,11 +5,13 @@ package cli import ( "context" "errors" + "fmt" "os" "path/filepath" "strings" "testing" + "github.com/github/gh-aw/pkg/parser" "github.com/github/gh-aw/pkg/testutil" "github.com/github/gh-aw/pkg/workflow" "github.com/stretchr/testify/assert" @@ -30,9 +32,13 @@ func TestValidateUpdateSHAEntries_ValidEntries(t *testing.T) { cache.SetContainerPin(image, digest, image+"@"+digest) require.NoError(t, cache.Save()) - origResolveAction := resolveActionRefSHAForValidation + origVerifyCommit := verifyActionCommitExistsForValidation + origResolveVersion := resolveActionVersionToSHAForValidation origResolveContainer := resolveContainerDigestForValidation - resolveActionRefSHAForValidation = func(_ context.Context, _, ref string) (string, error) { + verifyActionCommitExistsForValidation = func(_ context.Context, _, _ string) error { + return nil + } + resolveActionVersionToSHAForValidation = func(_ context.Context, _, ref string) (string, error) { if ref == "v5" { return "93cb6efe18208431cddfb8368fd83d5badbf9bfd", nil } @@ -42,7 +48,8 @@ func TestValidateUpdateSHAEntries_ValidEntries(t *testing.T) { return digest, nil } t.Cleanup(func() { - resolveActionRefSHAForValidation = origResolveAction + verifyActionCommitExistsForValidation = origVerifyCommit + resolveActionVersionToSHAForValidation = origResolveVersion resolveContainerDigestForValidation = origResolveContainer }) @@ -78,23 +85,27 @@ func TestValidateUpdateSHAEntries_InvalidEntries(t *testing.T) { ` require.NoError(t, os.WriteFile(filepath.Join(awDir, "actions-lock.json"), []byte(invalidActionsLock), 0o644)) - origResolveAction := resolveActionRefSHAForValidation + origVerifyCommit := verifyActionCommitExistsForValidation + origResolveVersion := resolveActionVersionToSHAForValidation origResolveContainer := resolveContainerDigestForValidation - resolveActionRefSHAForValidation = func(_ context.Context, repo, ref string) (string, error) { - switch { - case repo == "actions/setup-node" && ref == "v7": + verifyActionCommitExistsForValidation = func(_ context.Context, repo, sha string) error { + if repo == "actions/setup-node" && sha == "395ad3262231945c25e8478fd5baf05154b1d79f" { + return errors.New("commit not found") + } + return nil + } + resolveActionVersionToSHAForValidation = func(_ context.Context, repo, ref string) (string, error) { + if repo == "actions/setup-node" && ref == "v7" { return "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", nil - case ref == "395ad3262231945c25e8478fd5baf05154b1d79f": - return "", errors.New("commit not found") - default: - return ref, nil } + return ref, nil } resolveContainerDigestForValidation = func(_ context.Context, _ string) (string, error) { return "sha256:" + strings.Repeat("b", 64), nil } t.Cleanup(func() { - resolveActionRefSHAForValidation = origResolveAction + verifyActionCommitExistsForValidation = origVerifyCommit + resolveActionVersionToSHAForValidation = origResolveVersion resolveContainerDigestForValidation = origResolveContainer }) @@ -102,9 +113,67 @@ func TestValidateUpdateSHAEntries_InvalidEntries(t *testing.T) { require.Error(t, err) assert.Contains(t, err.Error(), `action entry "actions/checkout@v5" has invalid SHA`) assert.Contains(t, err.Error(), `action entry key/version mismatch: key "actions/setup-node@v6" should be "actions/setup-node@v7"`) - assert.Contains(t, err.Error(), `action entry "actions/setup-node@v6" has unresolved commit SHA`) + assert.Contains(t, err.Error(), `action entry "actions/setup-node@v6" commit SHA`) assert.Contains(t, err.Error(), `action entry "actions/setup-node@v6" SHA/version mismatch`) assert.Contains(t, err.Error(), `container pin key/image mismatch`) assert.Contains(t, err.Error(), `container pin "ghcr.io/test/image:v1" has invalid digest`) assert.Contains(t, err.Error(), `container pin "ghcr.io/test/image:v1" has inconsistent pinned_image`) } + +func TestValidateUpdateSHAEntries_NonFatalErrors(t *testing.T) { + tmpDir := testutil.TempDir(t, "validate-update-sha-*") + cache := workflow.NewActionCache(tmpDir) + cache.Set("actions/checkout", "v5", "93cb6efe18208431cddfb8368fd83d5badbf9bfd") + digest := "sha256:" + strings.Repeat("a", 64) + image := "ghcr.io/github/gh-aw-firewall/agent:0.27.9" + cache.SetContainerPin(image, digest, image+"@"+digest) + require.NoError(t, cache.Save()) + + origVerifyCommit := verifyActionCommitExistsForValidation + origResolveVersion := resolveActionVersionToSHAForValidation + origResolveContainer := resolveContainerDigestForValidation + // Auth error on commit existence check — should be skipped (non-fatal). + verifyActionCommitExistsForValidation = func(_ context.Context, _, _ string) error { + return fmt.Errorf("%w: auth error", parser.ErrVerificationSkipped) + } + // Network error on version resolution — should be skipped (non-fatal). + resolveActionVersionToSHAForValidation = func(_ context.Context, _, _ string) (string, error) { + return "", errors.New("network timeout") + } + // Registry unavailable — should be skipped (non-fatal). + resolveContainerDigestForValidation = func(_ context.Context, _ string) (string, error) { + return "", errors.New("registry unavailable") + } + t.Cleanup(func() { + verifyActionCommitExistsForValidation = origVerifyCommit + resolveActionVersionToSHAForValidation = origResolveVersion + resolveContainerDigestForValidation = origResolveContainer + }) + + // All failures should be non-fatal; validation should still pass. + require.NoError(t, validateUpdateSHAEntries(context.Background(), tmpDir)) +} + +func TestValidateUpdateSHAEntries_ContainerDigestMismatchIsWarning(t *testing.T) { + tmpDir := testutil.TempDir(t, "validate-update-sha-*") + digest := "sha256:" + strings.Repeat("a", 64) + image := "ghcr.io/github/gh-aw-firewall/agent:latest" + cache := workflow.NewActionCache(tmpDir) + cache.SetContainerPin(image, digest, image+"@"+digest) + require.NoError(t, cache.Save()) + + origVerifyCommit := verifyActionCommitExistsForValidation + origResolveContainer := resolveContainerDigestForValidation + verifyActionCommitExistsForValidation = func(_ context.Context, _, _ string) error { return nil } + // Mutable tag has moved to a different digest. + resolveContainerDigestForValidation = func(_ context.Context, _ string) (string, error) { + return "sha256:" + strings.Repeat("b", 64), nil + } + t.Cleanup(func() { + verifyActionCommitExistsForValidation = origVerifyCommit + resolveContainerDigestForValidation = origResolveContainer + }) + + // Digest mismatch is a warning, not a hard failure. + require.NoError(t, validateUpdateSHAEntries(context.Background(), tmpDir)) +} diff --git a/pkg/parser/remote_resolve_sha.go b/pkg/parser/remote_resolve_sha.go index 608ff1d0fe9..373be8c47d5 100644 --- a/pkg/parser/remote_resolve_sha.go +++ b/pkg/parser/remote_resolve_sha.go @@ -212,3 +212,29 @@ func resolveRefToSHAViaPublicAPI(ctx context.Context, owner, repo, ref string) ( func ResolveRefToSHAForHost(ctx context.Context, owner, repo, ref, host string) (string, error) { return resolveRefToSHA(ctx, owner, repo, ref, host) } + +// ErrVerificationSkipped is returned by VerifyCommitExists when the check cannot be +// performed due to an auth or network failure. Callers should treat it as non-fatal. +var ErrVerificationSkipped = errors.New("commit verification skipped") + +// VerifyCommitExists confirms that a full commit SHA exists in the given repository by +// querying the GitHub commits API. Unlike ResolveRefToSHAForHost, it always performs +// an API call so that a missing commit is detected (not silently passed through). +// +// Returns nil if the commit is found, ErrVerificationSkipped (wrapped) for auth or +// network errors where existence cannot be determined, and an unwrapped error when the +// API definitively reports the commit does not exist (e.g. HTTP 404). +func VerifyCommitExists(ctx context.Context, owner, repo, sha, host string) error { + client, err := createRESTClientForHostFunc(host) + if err != nil { + return fmt.Errorf("%w: failed to create REST client: %w", ErrVerificationSkipped, err) + } + var result commitLookupResponse + if apiErr := client.DoWithContext(ctx, http.MethodGet, buildCommitLookupAPIPath(owner, repo, sha), nil, &result); apiErr != nil { + if isGitHubAPIAuthError(apiErr) { + return fmt.Errorf("%w: %w", ErrVerificationSkipped, apiErr) + } + return apiErr + } + return nil +} From e687d62670b44eb4882deea3e97ffc7614e3d544 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 25 Jul 2026 06:29:46 +0000 Subject: [PATCH 5/5] refactor(update-validation): struct-based resolvers, remove ctx nil guard, container structural-only MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address github-actions[bot] review threads: - Struct-based resolvers (Thread PRRT_kwDOPc1QR86TuFYr): replace package-level var seams with a `validationResolvers` struct. `validateUpdateSHAEntries` calls the internal `validateUpdateSHAEntriesWithResolvers` with defaults; tests pass their own resolver inline without touching globals — enabling t.Parallel() throughout. - Remove ctx==nil guard (Thread PRRT_kwDOPc1QR86TuFZh): callers supply a valid context; the nil-substitution antipattern is removed. - Container digest: structural checks only (Threads PRRT_kwDOPc1QR86TuFYx, PRRT_kwDOPc1QR86TuFZf): remove live registry lookup entirely. Format, key/ image consistency, and pinned_image consistency are sufficient. Refreshing a mutable tag remains an explicit update operation. - `resolveContainerDigestForValidation` var and `console` import removed. - Tests updated: all five run in parallel, use inline resolver structs instead of global save/restore. `ContainerDigestMismatchIsWarning` replaced with `ContainerStructuralOnly` which asserts no network call is made at all. Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com> --- pkg/cli/update_validation.go | 89 +++++++++--------- pkg/cli/update_validation_test.go | 146 +++++++++++++----------------- 2 files changed, 108 insertions(+), 127 deletions(-) diff --git a/pkg/cli/update_validation.go b/pkg/cli/update_validation.go index 6718253ab47..929099ccfa6 100644 --- a/pkg/cli/update_validation.go +++ b/pkg/cli/update_validation.go @@ -10,7 +10,6 @@ import ( "sort" "strings" - "github.com/github/gh-aw/pkg/console" "github.com/github/gh-aw/pkg/gitutil" "github.com/github/gh-aw/pkg/logger" "github.com/github/gh-aw/pkg/parser" @@ -21,38 +20,50 @@ var updateValidationLog = logger.New("cli:update_validation") var sha256DigestPattern = regexp.MustCompile(`^sha256:[a-f0-9]{64}$`) -// resolveActionVersionToSHAForValidation resolves a version tag/ref to its commit SHA. -// Used to verify that the stored SHA in the lock entry matches the pinned version. -var resolveActionVersionToSHAForValidation = func(ctx context.Context, repo, ref string) (string, error) { - baseRepo := gitutil.ExtractBaseRepo(repo) - owner, name, ok := strings.Cut(baseRepo, "/") - if !ok || owner == "" || name == "" { - return "", fmt.Errorf("invalid action repository %q", repo) - } - return parser.ResolveRefToSHAForHost(ctx, owner, name, ref, "") +// validationResolvers holds injectable resolver functions for testability. +// Using a struct instead of package-level vars allows callers to supply their own +// resolvers inline without touching globals, enabling t.Parallel() in tests. +type validationResolvers struct { + // verifyActionCommitExists checks that a commit SHA actually exists in the + // action repository. It must return parser.ErrVerificationSkipped (wrapped) + // for auth/network failures so callers can treat those as non-fatal. + verifyActionCommitExists func(ctx context.Context, repo, sha string) error + // resolveActionVersionToSHA resolves a version tag/ref to its commit SHA. + // Used to verify that the stored SHA matches the pinned version. + resolveActionVersionToSHA func(ctx context.Context, repo, ref string) (string, error) } -// verifyActionCommitExistsForValidation checks that a commit SHA actually exists in the -// action repository. Returns parser.ErrVerificationSkipped for auth/network failures so -// callers can treat those as non-fatal. -var verifyActionCommitExistsForValidation = func(ctx context.Context, repo, sha string) error { - baseRepo := gitutil.ExtractBaseRepo(repo) - owner, name, ok := strings.Cut(baseRepo, "/") - if !ok || owner == "" || name == "" { - return fmt.Errorf("invalid action repository %q", repo) +func defaultValidationResolvers() validationResolvers { + return validationResolvers{ + verifyActionCommitExists: func(ctx context.Context, repo, sha string) error { + baseRepo := gitutil.ExtractBaseRepo(repo) + owner, name, ok := strings.Cut(baseRepo, "/") + if !ok || owner == "" || name == "" { + return fmt.Errorf("invalid action repository %q", repo) + } + return parser.VerifyCommitExists(ctx, owner, name, sha, "") + }, + resolveActionVersionToSHA: func(ctx context.Context, repo, ref string) (string, error) { + baseRepo := gitutil.ExtractBaseRepo(repo) + owner, name, ok := strings.Cut(baseRepo, "/") + if !ok || owner == "" || name == "" { + return "", fmt.Errorf("invalid action repository %q", repo) + } + return parser.ResolveRefToSHAForHost(ctx, owner, name, ref, "") + }, } - return parser.VerifyCommitExists(ctx, owner, name, sha, "") -} - -var resolveContainerDigestForValidation = func(ctx context.Context, image string) (string, error) { - return fetchContainerDigest(ctx, image, false) } +// validateUpdateSHAEntries validates the structural and liveness integrity of +// .github/aw/actions-lock.json after a workflow update. Container pins are +// validated structurally only (format, key/image/pinned_image consistency); +// live digest re-resolution is not performed because refreshing a mutable tag +// is an explicit update operation. func validateUpdateSHAEntries(ctx context.Context, repoRoot string) error { - if ctx == nil { - ctx = context.Background() - } + return validateUpdateSHAEntriesWithResolvers(ctx, repoRoot, defaultValidationResolvers()) +} +func validateUpdateSHAEntriesWithResolvers(ctx context.Context, repoRoot string, r validationResolvers) error { actionsLockPath := filepath.Join(repoRoot, ".github", "aw", "actions-lock.json") if _, err := os.Stat(actionsLockPath); err != nil { if os.IsNotExist(err) { @@ -88,14 +99,15 @@ func validateUpdateSHAEntries(ctx context.Context, repoRoot string) error { issues = append(issues, fmt.Sprintf("action entry %q has invalid SHA %q (expected 40-character commit SHA)", key, entry.SHA)) } else { validSHA = true - // Verify the commit SHA actually exists in the repository. Auth/network - // failures are non-fatal and logged; only a definitive not-found is an error. + // Verify the commit SHA actually exists in the repository via the + // GitHub commits API. Auth/network failures are non-fatal and logged; + // only a definitive not-found (e.g. HTTP 422/404) is an error. if entry.Repo != "" { - if err := verifyActionCommitExistsForValidation(ctx, entry.Repo, entry.SHA); err != nil { + if err := r.verifyActionCommitExists(ctx, entry.Repo, entry.SHA); err != nil { if errors.Is(err, parser.ErrVerificationSkipped) { updateValidationLog.Printf("action entry %q: skipping commit existence check (auth/network error): %v", key, err) } else { - issues = append(issues, fmt.Sprintf("action entry %q commit SHA %q does not exist in %q: %v", key, entry.SHA, entry.Repo, err)) + issues = append(issues, fmt.Sprintf("action entry %q: commit SHA %q not found in %q: %v", key, entry.SHA, entry.Repo, err)) } } } @@ -108,11 +120,11 @@ func validateUpdateSHAEntries(ctx context.Context, repoRoot string) error { if validSHA { // Verify the stored version tag resolves to the stored SHA. // Auth/network failures are non-fatal; only a confirmed mismatch is an error. - resolvedVersionSHA, err := resolveActionVersionToSHAForValidation(ctx, entry.Repo, entry.Version) + resolvedVersionSHA, err := r.resolveActionVersionToSHA(ctx, entry.Repo, entry.Version) if err != nil { updateValidationLog.Printf("action entry %q: skipping version/SHA check (resolution failed): %v", key, err) } else if !strings.EqualFold(resolvedVersionSHA, entry.SHA) { - issues = append(issues, fmt.Sprintf("action entry %q SHA/version mismatch: version %q resolves to %q but entry has %q", key, entry.Version, resolvedVersionSHA, entry.SHA)) + issues = append(issues, fmt.Sprintf("action entry %q SHA/version mismatch: version %q resolves to %q but stored SHA is %q", key, entry.Version, resolvedVersionSHA, entry.SHA)) } } } @@ -130,19 +142,6 @@ func validateUpdateSHAEntries(ctx context.Context, repoRoot string) error { } if !sha256DigestPattern.MatchString(pin.Digest) { issues = append(issues, fmt.Sprintf("container pin %q has invalid digest %q (expected sha256:<64 lowercase hex chars>)", image, pin.Digest)) - } else { - // Verify the stored digest matches the current digest for the image tag. - // Lookup failures are non-fatal (e.g. registry unavailable). A mismatch is - // surfaced as a warning rather than a hard failure because a mutable tag may - // have been updated after pinning and the update command cannot auto-repair it. - resolvedDigest, err := resolveContainerDigestForValidation(ctx, image) - if err != nil { - updateValidationLog.Printf("container pin %q: skipping digest check (lookup failed): %v", image, err) - } else if resolvedDigest != pin.Digest { - fmt.Fprintln(os.Stderr, console.FormatWarningMessage( - fmt.Sprintf("container pin %q digest is stale: stored %q but image tag resolves to %q; re-run 'gh aw update' after removing the pin to refresh", image, pin.Digest, resolvedDigest), - )) - } } expectedPinnedImage := image + "@" + pin.Digest if pin.PinnedImage != expectedPinnedImage { diff --git a/pkg/cli/update_validation_test.go b/pkg/cli/update_validation_test.go index 94df1edc9c2..22cb1673e99 100644 --- a/pkg/cli/update_validation_test.go +++ b/pkg/cli/update_validation_test.go @@ -18,12 +18,27 @@ import ( "github.com/stretchr/testify/require" ) +// noopResolvers returns resolvers that perform no network calls. +// Tests that want specific resolver behaviour should override individual fields. +func noopResolvers() validationResolvers { + return validationResolvers{ + verifyActionCommitExists: func(_ context.Context, _, _ string) error { + return nil + }, + resolveActionVersionToSHA: func(_ context.Context, _, ref string) (string, error) { + return ref, nil + }, + } +} + func TestValidateUpdateSHAEntries_NoActionsLock(t *testing.T) { + t.Parallel() tmpDir := testutil.TempDir(t, "validate-update-sha-*") - require.NoError(t, validateUpdateSHAEntries(context.Background(), tmpDir)) + require.NoError(t, validateUpdateSHAEntriesWithResolvers(context.Background(), tmpDir, noopResolvers())) } func TestValidateUpdateSHAEntries_ValidEntries(t *testing.T) { + t.Parallel() tmpDir := testutil.TempDir(t, "validate-update-sha-*") cache := workflow.NewActionCache(tmpDir) cache.Set("actions/checkout", "v5", "93cb6efe18208431cddfb8368fd83d5badbf9bfd") @@ -32,31 +47,23 @@ func TestValidateUpdateSHAEntries_ValidEntries(t *testing.T) { cache.SetContainerPin(image, digest, image+"@"+digest) require.NoError(t, cache.Save()) - origVerifyCommit := verifyActionCommitExistsForValidation - origResolveVersion := resolveActionVersionToSHAForValidation - origResolveContainer := resolveContainerDigestForValidation - verifyActionCommitExistsForValidation = func(_ context.Context, _, _ string) error { - return nil + r := validationResolvers{ + verifyActionCommitExists: func(_ context.Context, _, _ string) error { + return nil + }, + resolveActionVersionToSHA: func(_ context.Context, _, ref string) (string, error) { + if ref == "v5" { + return "93cb6efe18208431cddfb8368fd83d5badbf9bfd", nil + } + return ref, nil + }, } - resolveActionVersionToSHAForValidation = func(_ context.Context, _, ref string) (string, error) { - if ref == "v5" { - return "93cb6efe18208431cddfb8368fd83d5badbf9bfd", nil - } - return ref, nil - } - resolveContainerDigestForValidation = func(_ context.Context, _ string) (string, error) { - return digest, nil - } - t.Cleanup(func() { - verifyActionCommitExistsForValidation = origVerifyCommit - resolveActionVersionToSHAForValidation = origResolveVersion - resolveContainerDigestForValidation = origResolveContainer - }) - require.NoError(t, validateUpdateSHAEntries(context.Background(), tmpDir)) + require.NoError(t, validateUpdateSHAEntriesWithResolvers(context.Background(), tmpDir, r)) } func TestValidateUpdateSHAEntries_InvalidEntries(t *testing.T) { + t.Parallel() tmpDir := testutil.TempDir(t, "validate-update-sha-*") awDir := filepath.Join(tmpDir, ".github", "aw") require.NoError(t, os.MkdirAll(awDir, 0o755)) @@ -85,35 +92,28 @@ func TestValidateUpdateSHAEntries_InvalidEntries(t *testing.T) { ` require.NoError(t, os.WriteFile(filepath.Join(awDir, "actions-lock.json"), []byte(invalidActionsLock), 0o644)) - origVerifyCommit := verifyActionCommitExistsForValidation - origResolveVersion := resolveActionVersionToSHAForValidation - origResolveContainer := resolveContainerDigestForValidation - verifyActionCommitExistsForValidation = func(_ context.Context, repo, sha string) error { - if repo == "actions/setup-node" && sha == "395ad3262231945c25e8478fd5baf05154b1d79f" { - return errors.New("commit not found") - } - return nil - } - resolveActionVersionToSHAForValidation = func(_ context.Context, repo, ref string) (string, error) { - if repo == "actions/setup-node" && ref == "v7" { - return "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", nil - } - return ref, nil + r := validationResolvers{ + // Commit existence check: setup-node SHA is not found; checkout SHA passes. + verifyActionCommitExists: func(_ context.Context, repo, sha string) error { + if repo == "actions/setup-node" && sha == "395ad3262231945c25e8478fd5baf05154b1d79f" { + return errors.New("commit not found") + } + return nil + }, + // Version resolution: setup-node v7 resolves to a different SHA (mismatch). + resolveActionVersionToSHA: func(_ context.Context, repo, ref string) (string, error) { + if repo == "actions/setup-node" && ref == "v7" { + return "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", nil + } + return ref, nil + }, } - resolveContainerDigestForValidation = func(_ context.Context, _ string) (string, error) { - return "sha256:" + strings.Repeat("b", 64), nil - } - t.Cleanup(func() { - verifyActionCommitExistsForValidation = origVerifyCommit - resolveActionVersionToSHAForValidation = origResolveVersion - resolveContainerDigestForValidation = origResolveContainer - }) - err := validateUpdateSHAEntries(context.Background(), tmpDir) + err := validateUpdateSHAEntriesWithResolvers(context.Background(), tmpDir, r) require.Error(t, err) assert.Contains(t, err.Error(), `action entry "actions/checkout@v5" has invalid SHA`) assert.Contains(t, err.Error(), `action entry key/version mismatch: key "actions/setup-node@v6" should be "actions/setup-node@v7"`) - assert.Contains(t, err.Error(), `action entry "actions/setup-node@v6" commit SHA`) + assert.Contains(t, err.Error(), `action entry "actions/setup-node@v6": commit SHA`) assert.Contains(t, err.Error(), `action entry "actions/setup-node@v6" SHA/version mismatch`) assert.Contains(t, err.Error(), `container pin key/image mismatch`) assert.Contains(t, err.Error(), `container pin "ghcr.io/test/image:v1" has invalid digest`) @@ -121,6 +121,7 @@ func TestValidateUpdateSHAEntries_InvalidEntries(t *testing.T) { } func TestValidateUpdateSHAEntries_NonFatalErrors(t *testing.T) { + t.Parallel() tmpDir := testutil.TempDir(t, "validate-update-sha-*") cache := workflow.NewActionCache(tmpDir) cache.Set("actions/checkout", "v5", "93cb6efe18208431cddfb8368fd83d5badbf9bfd") @@ -129,32 +130,23 @@ func TestValidateUpdateSHAEntries_NonFatalErrors(t *testing.T) { cache.SetContainerPin(image, digest, image+"@"+digest) require.NoError(t, cache.Save()) - origVerifyCommit := verifyActionCommitExistsForValidation - origResolveVersion := resolveActionVersionToSHAForValidation - origResolveContainer := resolveContainerDigestForValidation - // Auth error on commit existence check — should be skipped (non-fatal). - verifyActionCommitExistsForValidation = func(_ context.Context, _, _ string) error { - return fmt.Errorf("%w: auth error", parser.ErrVerificationSkipped) - } - // Network error on version resolution — should be skipped (non-fatal). - resolveActionVersionToSHAForValidation = func(_ context.Context, _, _ string) (string, error) { - return "", errors.New("network timeout") - } - // Registry unavailable — should be skipped (non-fatal). - resolveContainerDigestForValidation = func(_ context.Context, _ string) (string, error) { - return "", errors.New("registry unavailable") + r := validationResolvers{ + // Auth error on commit existence check — should be skipped (non-fatal). + verifyActionCommitExists: func(_ context.Context, _, _ string) error { + return fmt.Errorf("%w: auth error", parser.ErrVerificationSkipped) + }, + // Network error on version resolution — should be skipped (non-fatal). + resolveActionVersionToSHA: func(_ context.Context, _, _ string) (string, error) { + return "", errors.New("network timeout") + }, } - t.Cleanup(func() { - verifyActionCommitExistsForValidation = origVerifyCommit - resolveActionVersionToSHAForValidation = origResolveVersion - resolveContainerDigestForValidation = origResolveContainer - }) - - // All failures should be non-fatal; validation should still pass. - require.NoError(t, validateUpdateSHAEntries(context.Background(), tmpDir)) + + // All network failures should be non-fatal; validation should still pass. + require.NoError(t, validateUpdateSHAEntriesWithResolvers(context.Background(), tmpDir, r)) } -func TestValidateUpdateSHAEntries_ContainerDigestMismatchIsWarning(t *testing.T) { +func TestValidateUpdateSHAEntries_ContainerStructuralOnly(t *testing.T) { + t.Parallel() tmpDir := testutil.TempDir(t, "validate-update-sha-*") digest := "sha256:" + strings.Repeat("a", 64) image := "ghcr.io/github/gh-aw-firewall/agent:latest" @@ -162,18 +154,8 @@ func TestValidateUpdateSHAEntries_ContainerDigestMismatchIsWarning(t *testing.T) cache.SetContainerPin(image, digest, image+"@"+digest) require.NoError(t, cache.Save()) - origVerifyCommit := verifyActionCommitExistsForValidation - origResolveContainer := resolveContainerDigestForValidation - verifyActionCommitExistsForValidation = func(_ context.Context, _, _ string) error { return nil } - // Mutable tag has moved to a different digest. - resolveContainerDigestForValidation = func(_ context.Context, _ string) (string, error) { - return "sha256:" + strings.Repeat("b", 64), nil - } - t.Cleanup(func() { - verifyActionCommitExistsForValidation = origVerifyCommit - resolveContainerDigestForValidation = origResolveContainer - }) - - // Digest mismatch is a warning, not a hard failure. - require.NoError(t, validateUpdateSHAEntries(context.Background(), tmpDir)) + // No container resolver in the struct — container pins are structural-only. + // Even if the tag has moved to a different digest, the stored pin is still valid + // and validation passes without performing any live lookup. + require.NoError(t, validateUpdateSHAEntriesWithResolvers(context.Background(), tmpDir, noopResolvers())) }