fix: SHA-pinned actions lose version comment when SkipHardcodedFallback is set#46122
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…edFallback is set Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…hedule churn) Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #46122 does not have the 'implementation' label and has only 56 new lines of code in business logic directories (threshold: 100). |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
There was a problem hiding this comment.
Pull request overview
Fixes SHA-pinned action version comments when hardcoded version-to-SHA fallback is disabled.
Changes:
- Allows exact SHA-to-version metadata lookup.
- Adds internal and public regression tests.
- Regenerates the release lock manifest.
Show a summary per file
| File | Description |
|---|---|
pkg/actionpins/actionpins.go |
Permits SHA-to-version lookup during skipped fallback. |
pkg/actionpins/actionpins_internal_test.go |
Tests internal resolution behavior. |
pkg/actionpins/spec_test.go |
Tests the public API regression. |
.github/workflows/release.lock.yml |
Restores the setup-go version metadata. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 4/4 changed files
- Comments generated: 2
- Review effort level: Medium
| // However, when version is already a SHA (isAlreadySHA), the lookup is purely | ||
| // SHA→version (to find a human-readable version label for the comment). That | ||
| // operation carries no security risk regardless of host, so it is always allowed. | ||
| if ctx.SkipHardcodedFallback && !isAlreadySHA { |
| assert.Contains(t, result, "actions/checkout@", "result should reference actions/checkout") | ||
| }) | ||
|
|
||
| t.Run("SHA-pinned action with SkipHardcodedFallback=true still produces version comment", func(t *testing.T) { |
There was a problem hiding this comment.
Clean, well-scoped fix. The narrowed guard () is correct — SHA→version lookup is purely informational and host-independent, so allowing it through even on non-github.com hosts carries no security risk. The regression tests are explicit and adequately cover both the blocked path (version→SHA still skipped) and the newly allowed path (SHA→version proceeds). Lock file change confirms the fix works end-to-end.
🔎 Code quality review by PR Code Quality Reviewer · 10.5 AIC · ⌖ 4.28 AIC · ⊞ 5.6K
Comment /review to run again
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — approving with one minor suggestion.
📋 Key Themes & Highlights
Fix Quality
- ✅ Root cause correctly identified:
SkipHardcodedFallbackguarded SHA→version lookups even though those are host-independent and informational only - ✅ The one-line condition change (
&& !isAlreadySHA) is minimal and surgical - ✅ Regression tests added at both internal and public API levels — solid coverage of the exact failure mode
- ✅ Test names read as specifications and clearly document the pre-fix failure
Minor Issue
- Log message on line 399 still says
skipping hardcoded pin lookupbut now only fires for version→SHA requests — see inline comment for a suggested update.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 16.5 AIC · ⌖ 4.41 AIC · ⊞ 6.7K
Comment /matt to run again
| // However, when version is already a SHA (isAlreadySHA), the lookup is purely | ||
| // SHA→version (to find a human-readable version label for the comment). That | ||
| // operation carries no security risk regardless of host, so it is always allowed. | ||
| if ctx.SkipHardcodedFallback && !isAlreadySHA { |
There was a problem hiding this comment.
[/diagnosing-bugs] The log message still says "skipping hardcoded pin lookup" but now only fires for version→SHA lookups — slightly misleading when reading logs.
💡 Suggested update
actionPinsLog.Printf("SkipHardcodedFallback set, skipping version→SHA hardcoded pin lookup for %s@%s", actionRepo, version)This makes it clearer in debug logs that SHA→version lookups are still allowed.
@copilot please address this.
There was a problem hiding this comment.
The fix is correct and well-tested.
Root cause & fix: SkipHardcodedFallback was unconditionally skipping all hardcoded-pin lookups. The fix restricts the skip to version→SHA lookups (!isAlreadySHA), which is the only case where bundled data could introduce stale/wrong pins on non-github.com hosts. SHA→version lookups are purely informational (no security risk), so they are now always allowed.
Tests: Both the internal and public-API regression tests directly exercise the before/after behavior using a real embedded pin (actions/checkout), making them meaningful rather than mock-driven.
Lock file: The manifest update (actions/setup-go gaining its v7.0.0 comment) is a correct side-effect of the fix.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 12.9 AIC · ⌖ 4.31 AIC · ⊞ 5K
🧪 Test Quality Sentinel Report✅ Test Quality Score: 85/100 — Excellent
📊 Metrics (6 tests)
Verdict
Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
|
@copilot run pr-finisher skill |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done in 6c23e60. I addressed the in-scope review feedback by clarifying the exported |
|
🎉 This pull request is included in a new release. Release: |
When
GH_HOSTis non-github.com or the git remote doesn't resolve togithub.com(common in sandbox/CI),SkipHardcodedFallback=truecausedresolveActionPinFromHardcodedPinsto skip all hardcoded lookups — including SHA→version resolution. The fallback then emittedFormatPinnedActionReference(repo, sha, sha), replacing# v7.0.0with# 9c091bb...in lock files on every recompile.Root cause
SkipHardcodedFallbackguards against using bundled version→SHA mappings on non-github.com hosts (stale pin risk). The guard was also applied to SHA→version lookups, which are purely informational and host-independent.Fix
pkg/actionpins/actionpins.go: Change early-return condition fromif ctx.SkipHardcodedFallbacktoif ctx.SkipHardcodedFallback && !isAlreadySHAinresolveActionPinFromHardcodedPinsactionpins_internal_test.go/spec_test.go: Regression tests — SHA-pinned action withSkipHardcodedFallback=truemust still produce# v7.0.0, not# <sha>.