Add post-update SHA integrity validation for actions-lock entries#47907
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Implemented in
|
There was a problem hiding this comment.
Pull request overview
Adds terminal integrity validation for action and container pins after workflow updates.
Changes:
- Validates lock-file keys, SHAs, digests, and pinned images.
- Adds remote action and container resolution checks.
- Adds unit coverage for valid, invalid, and missing lock files.
Show a summary per file
| File | Description |
|---|---|
pkg/cli/update_validation.go |
Implements lock-file validation. |
pkg/cli/update_validation_test.go |
Tests validator behavior and diagnostics. |
pkg/cli/update_command.go |
Runs validation after successful updates. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 3/3 changed files
- Comments generated: 3
- Review effort level: Medium
| 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)) | ||
| } | ||
| } |
There was a problem hiding this comment.
Fixed in 2d8122a + e687d62. VerifyCommitExists was added to the parser package and calls GET /repos/{owner}/{repo}/commits/{sha} directly, bypassing the 40-char hex short-circuit in ResolveRefToSHAForHost.
| 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)) |
There was a problem hiding this comment.
Fixed in 2d8122a. Auth/network failures from both the commit-existence and version-resolution checks are treated as non-fatal: they are logged and skipped rather than appended to issues.
| 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)) | ||
| } | ||
| } |
There was a problem hiding this comment.
Fixed in e687d62. Live registry lookup is removed entirely. Container pins are validated structurally only (digest format, key/image consistency, pinned_image format). A mutable-tag drift no longer causes any failure or warning.
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
🧪 Test Quality Sentinel Report✅ Test Quality Score: 97/100 — Excellent
📊 Metrics (3 tests)
Verdict
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: Add post-update SHA integrity validation
The PR adds a meaningful validation phase, but has three blocking correctness issues already captured in inline comments.
Blocking issues
-
SHA re-resolution is a no-op for commit SHAs (line 76):
ResolveRefToSHAForHostreturns any 40-char hex ref unchanged, so the round-trip never verifies the commit actually exists in the upstream repo. -
Network/auth errors fail the update (line 86): failures from
resolveActionRefSHAForValidationare treated as validation issues, breakinggh aw updatein offline or auth-restricted environments and reversing existing non-fatal handling. -
Live digest re-fetch rejects valid historical pins (line 110):
UpdateContainerPinsdeliberately keeps existing pins when a mutable tag moves forward; re-resolving the tag here incorrectly flags those still-valid entries as digest mismatches.
Please address the three existing inline comments before merging.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 15.4 AIC · ⌖ 4.55 AIC · ⊞ 5K
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (241 new lines in 📄 Draft ADR committed:
📋 What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. ❓ Why ADRs Matter
ADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs, /tdd, and /codebase-design — requesting changes on three correctness issues in the liveness-check logic.
📋 Key Themes & Highlights
Blocking Issues
-
Dead SHA round-trip (line 76):
ResolveRefToSHAForHostechoes back any 40-char hex unchanged, so the SHA existence check never fires — it gives a false sense of verified liveness. Either remove it or replace it with a commit-existence API call. -
Network errors poison structural validation (lines 84–86, 91–93): Transient auth/rate-limit failures from version resolution are appended as hard validation issues, causing
gh aw updateto fail even when the lock file is structurally sound. These should be warnings, not errors. -
Container digest re-resolution breaks valid pins (line 110): Stored digests are intentionally immutable (the update path skips existing pins). Re-resolving the current digest of a mutable tag and diffing it against a historical pin will produce false positives on every update after a tag moves.
Positive Highlights
- ✅ Multi-error aggregation with sorted iteration is a great pattern — surfaces all issues at once instead of fail-fast.
- ✅ Testable seam via function variable overrides is the right direction; moving to a resolver struct would make tests parallel-safe too.
- ✅ Test coverage for no-file, valid, and multi-error paths is solid; test names read as clear specifications.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 40 AIC · ⌖ 5.2 AIC · ⊞ 6.7K
Comment /matt to run again
| var sha256DigestPattern = regexp.MustCompile(`^sha256:[a-f0-9]{64}$`) | ||
|
|
||
| var resolveActionRefSHAForValidation = func(ctx context.Context, repo, ref string) (string, error) { | ||
| baseRepo := gitutil.ExtractBaseRepo(repo) |
There was a problem hiding this comment.
[/codebase-design] Package-level var resolver functions block parallel test execution.
Using mutable package-level vars as seams means tests must save/restore them, preventing t.Parallel() and making them fragile under refactoring. Consider passing resolvers as a struct or via functional options instead.
💡 Suggested alternative seam
type validationResolvers struct {
resolveActionRef func(ctx context.Context, repo, ref string) (string, error)
resolveContainerDigest func(ctx context.Context, image string) (string, error)
}
func validateUpdateSHAEntries(ctx context.Context, repoRoot string, r validationResolvers) error { ... }Tests can supply their own validationResolvers inline without touching globals, enabling t.Parallel().
@copilot please address this.
There was a problem hiding this comment.
Fixed in e687d62. Package-level vars removed. Resolvers are now passed via a validationResolvers struct to validateUpdateSHAEntriesWithResolvers. All five tests use t.Parallel() and supply their resolvers inline — no global state to save/restore.
| } 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)) | ||
| } | ||
| } |
There was a problem hiding this comment.
[/diagnosing-bugs] Re-resolving a commit SHA against the upstream API is not a meaningful integrity check — and existing comments note this.
ResolveRefToSHAForHost returns any 40-char hex ref unchanged (it only resolves symbolic refs like branch names). So resolveActionRefSHAForValidation(ctx, repo, sha) will echo back the SHA rather than verify it exists. The check on line 78 (!strings.EqualFold(resolvedSHA, entry.SHA)) will therefore never fire, giving a false sense of verified liveness.
💡 Suggested fix
Either:
- Remove the SHA-round-trip check — the format check (
IsCommitSHA) is sufficient for structural validation. - Use a commit-existence API (e.g.
GET /repos/{owner}/{repo}/commits/{sha}) instead ofResolveRefToSHAForHost.
@copilot please address this.
There was a problem hiding this comment.
Fixed in e687d62. The SHA round-trip via ResolveRefToSHAForHost is gone. Commit existence is now verified via VerifyCommitExists (parser.VerifyCommitExists), which calls GET /repos/{owner}/{repo}/commits/{sha} directly — it never short-circuits on a 40-char hex string. Auth/network failures return ErrVerificationSkipped and are logged non-fatally; a definitive not-found (e.g. HTTP 422/404) fails validation.
| 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)) |
There was a problem hiding this comment.
[/diagnosing-bugs] Network/auth failures from resolveActionRefSHAForValidation are treated as validation errors, making gh aw update fail in offline or rate-limited environments.
Version resolution errors on lines 84–86 and 91–93 are appended as issues, which causes the whole update to return an error even though the lock entry may be structurally valid. Transient failures should not invalidate an otherwise correct lock file.
💡 Suggested fix
Distinguish between structural errors (always fail) and liveness/network errors (warn and continue, or skip):
resolvedVersionSHA, err := resolveActionRefSHAForValidation(ctx, entry.Repo, entry.Version)
if err != nil {
// non-fatal: log a warning but do not block the update
updateLog.Printf("warning: could not resolve version %q for %q: %v", entry.Version, key, err)
} else if ... {
issues = append(issues, ...)
}@copilot please address this.
There was a problem hiding this comment.
Fixed in 2d8122a. Network/auth failures from both the commit-existence check and the version resolution are now non-fatal: ErrVerificationSkipped is logged and skipped, and resolution errors are logged (updateValidationLog.Printf) without appending to issues.
| } 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)) | ||
| } | ||
| } |
There was a problem hiding this comment.
[/diagnosing-bugs] Container digest liveness check rejects valid historical pins whenever a mutable tag moves.
UpdateContainerPins deliberately skips already-pinned images, so their stored digest correctly reflects the image at pin time. Re-resolving the current digest of a mutable tag (e.g. :latest, :0.27.9) and comparing against the immutable stored digest will flag structurally-correct, intentional pins as broken.
💡 Suggested fix
Remove the live re-resolution from the container section — structural checks (format, key/image/pinned_image consistency) are sufficient and safe. If liveness verification is desired, scope it explicitly to an opt-in flag (--verify-live) so users can choose the tradeoff.
@copilot please address this.
There was a problem hiding this comment.
Fixed in e687d62. The live registry lookup is removed entirely. Container pins now receive only structural checks: digest format (sha256:<64 hex>), key/image consistency, and pinned_image = image@digest consistency. No network call is made. The new TestValidateUpdateSHAEntries_ContainerStructuralOnly test passes no container resolver and asserts that a valid structural pin produces no error regardless of what the registry currently holds.
| return ref, nil | ||
| } | ||
| resolveContainerDigestForValidation = func(_ context.Context, _ string) (string, error) { | ||
| return digest, nil |
There was a problem hiding this comment.
[/tdd] The TestValidateUpdateSHAEntries_ValidEntries test mocks the version resolver to return the correct SHA only for ref == "v5", but also catches the SHA re-resolution path (ref == sha) via the fallback return ref, nil. This means the SHA round-trip check passes vacuously (as noted above, ResolveRefToSHAForHost echoes SHAs), masking the dead-code issue in production.
Adding an explicit test case where the SHA resolver is called with the stored SHA and asserting the call never happens (or asserting the expected no-op path) would expose the behavior clearly.
@copilot please address this.
There was a problem hiding this comment.
Fixed in e687d62. The SHA round-trip path is gone — verifyActionCommitExists now makes a real API call so the test stub exercises a real code path. Tests use inline resolver structs; there is no return ref, nil fallback that could accidentally make the check a no-op.
There was a problem hiding this comment.
Review: post-update SHA integrity validation
The three existing comments (lines 60, 76, 86, 110) capture the most significant correctness issues — the SHA-resolve no-op, network failures treated as validation errors, and mutable-tag comparison breaking historical pins. Three additional comments added:
Summary of new findings
- Serial O(2N) network round-trips (line 109): each action entry fires two sequential HTTP calls; the first (SHA→SHA) is already a documented no-op. With no concurrency, this will be noticeably slow on real lock files.
ctx == nilguard (line 60): antipattern per Go convention; should be removed.- Misleading error message (line 101):
"got %q from %q"implies the second arg is a source repo, but it is the stored SHA — direction of mismatch is confusing.
None of these block CI on their own, but the three pre-existing high-severity issues (network failures causing spurious update failures, mutable-tag re-validation, SHA no-op) together make the validation logic fragile enough that COMMENT is the appropriate verdict until those are addressed.
🔎 Code quality review by PR Code Quality Reviewer · sonnet46 · 40.8 AIC · ⌖ 4.99 AIC · ⊞ 5.7K
Comment /review to run again
| 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)) | ||
| } |
There was a problem hiding this comment.
Serial network calls per entry with no parallelism or deduplication: each action entry triggers two separate remote round-trips (once for the SHA, once for the version tag), adding O(2N) sequential HTTP calls during gh aw update, potentially minutes of latency for even modest lock files.
💡 Detail
Lines 97 and 109 both call resolveActionRefSHAForValidation — first to verify the stored SHA (which the existing comment on line 76 notes is a no-op, since ResolveRefToSHAForHost returns commit SHAs unchanged), then to resolve the version tag. For N action entries that is 2N sequential HTTP calls.
Consider:
- Dropping the redundant SHA-resolve call (lines 97–102) entirely, leaving only the version-tag resolution
- Running resolutions concurrently via
errgroup - Or documenting the cost and adding a
--no-network/--offlineflag
There was a problem hiding this comment.
The container digest lookup is removed entirely in e687d62, eliminating those registry round-trips. Per action entry there are still two calls: verifyActionCommitExists (commit existence) and resolveActionVersionToSHA (version tag → SHA). Both are meaningful and non-redundant. Parallelism can be added later if lock-file sizes make latency observable.
| for _, key := range entryKeys { | ||
| entry := cache.Entries[key] | ||
| if entry.Repo == "" { | ||
| issues = append(issues, fmt.Sprintf("action entry %q has empty repo", key)) |
There was a problem hiding this comment.
ctx == nil guard is an antipattern: Go convention prohibits passing nil contexts; the correct response is to panic (or return an error), not silently substitute context.Background().
💡 Detail
From the Go standard library docs: "Do not pass a nil Context, even if a function permits it. Pass context.Background() if you are unsure about which Context to use."
Silently papering over a nil context masks bugs in callers and creates a divergence between test behavior (where callers pass real contexts) and any future caller that accidentally passes nil. Remove the guard and let the caller supply a valid context:
func validateUpdateSHAEntries(ctx context.Context, repoRoot string) error {
// no nil check — callers must not pass nil
...
}If there is a legitimate code path that has no context, pass context.Background() at the call site explicitly.
There was a problem hiding this comment.
Fixed in e687d62. The ctx == nil guard is removed. validateUpdateSHAEntries now passes the context straight through to validateUpdateSHAEntriesWithResolvers without any nil-substitution.
| 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) { |
There was a problem hiding this comment.
Misleading error message: the format string "got %q from %q" makes it appear the second argument is the source repo, but it is actually entry.SHA — the stored SHA that failed to match.
💡 Detail
Current (line 101):
issues = append(issues, fmt.Sprintf(
"action entry %q resolved commit SHA mismatch: got %q from %q",
key, resolvedSHA, entry.SHA))"from %q" reads as "from source X", but entry.SHA is the stored/expected value, not a source. The message is backwards: resolvedSHA is what the remote returned; entry.SHA is what was stored. Suggested wording:
issues = append(issues, fmt.Sprintf(
"action entry %q SHA mismatch: stored %q, resolved to %q via %q",
key, entry.SHA, resolvedSHA, entry.Repo))This makes the direction of the mismatch unambiguous for whoever reads the error during a failed update.
There was a problem hiding this comment.
The "got %q from %q" format is no longer in the code. The SHA/version mismatch message (introduced in e687d62) reads: "action entry %q SHA/version mismatch: version %q resolves to %q but stored SHA is %q" — making it unambiguous which value was resolved and which was stored.
|
@copilot run pr-finisher skill |
…k errors, container mismatch as warning - 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>
Implemented in the latest commit. Action commit SHA existence — added Container SHA / version validation — live digest lookup is preserved and now surfaces a stderr warning when the stored digest doesn't match the current image tag. Lookup failures (registry unavailable, auth errors) are logged and skipped rather than hard-failing, since Two new tests cover both paths:
PR-finisher pass complete. Summary below. |
|
@copilot please run the
|
…uard, container structural-only 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>
Head branch was pushed to by a user without write access
All blocking review items addressed in
Threads replied to and ready for human resolution. |
🤖 PR Triage
Rationale: Adds SHA integrity validation to Next step: Address outstanding review requests, then fast-track for human approval.
|
gh aw updatenow performs a final integrity pass over.github/aw/actions-lock.jsonto catch malformed or inconsistent action/container pins before the command exits. This adds an explicit validation phase that double-checks both action SHAs and container SHA256 digests for structural correctness.Update flow: add terminal validation phase
RunUpdateWorkflowsnow invokes a new SHA validation step after update/recompile/pin-refresh work completes successfully.update validation failederror instead of being silently persisted.Action entry integrity checks
repo@versionkey must match entry fields).Container pin integrity checks
sha256:<64 lowercase hex>.pin.image).pinned_imageis exactlyimage@digest.Targeted unit coverage for new validator