fix: SHA-pinned actions lose version comment when SkipHardcodedFallback is set

.github/workflows/agentic-auto-upgrade.yml

@@ -34,7 +34,7 @@ name: Agentic Auto-Upgrade
 
 on:
   schedule:
-    - cron: "11 4 * * 6"  # Weekly (auto-upgrade)
+    - cron: "21 3 * * 5"  # Weekly (auto-upgrade)
   workflow_dispatch:
 
 permissions:

pkg/actionpins/actionpins.go

@@ -98,11 +98,12 @@ type PinContext struct {
 	Warnings map[string]bool
 	// RecordResolutionFailure receives unresolved pinning failures for auditing.
 	RecordResolutionFailure func(f ResolutionFailure)
-	// SkipHardcodedFallback skips the entire hardcoded-pin lookup (both exact/strict
-	// and non-strict matches) when dynamic resolution fails. Set this when GH_HOST is
-	// configured to a non-github.com host: the dynamic resolver will query the wrong
-	// host and fail, so silently falling back to bundled pins would produce unverified
-	// SHA pins and mask the real misconfiguration.
+	// SkipHardcodedFallback skips version→SHA hardcoded fallback when dynamic
+	// resolution fails. Exact SHA→version labeling is still allowed so
+	// already-pinned actions keep their human-readable version comments. Set this
+	// when GH_HOST is configured to a non-github.com host: the dynamic resolver
+	// will query the wrong host and fail, so silently falling back to bundled pins
+	// would produce unverified SHA pins and mask the real misconfiguration.
 	SkipHardcodedFallback bool
 	// Mappings redirects action repository@version references to replacement
 	// repository@version references before pin resolution. Keys and values use
@@ -391,9 +392,13 @@ func resolveActionPinFromHardcodedPins(actionRepo, version string, isAlreadySHA
 	// When the caller is targeting a non-github.com host (e.g. GHES/GHEC), the
 	// dynamic resolver already failed because it queried the wrong host.  Silently
 	// falling back to bundled pins in that case produces unverified SHA pins and
-	// masks the real problem, so skip this fallback entirely.
-	if ctx.SkipHardcodedFallback {
-		actionPinsLog.Printf("SkipHardcodedFallback set, skipping hardcoded pin lookup for %s@%s", actionRepo, version)
+	// masks the real problem, so skip this fallback for version→SHA resolution.
+	//
+	// 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 {
+		actionPinsLog.Printf("SkipHardcodedFallback set, skipping version→SHA hardcoded pin lookup for %s@%s", actionRepo, version)
 		return "", false
 	}
 

pkg/actionpins/actionpins_internal_test.go

@@ -430,14 +430,33 @@ func TestResolveNonStrictHardcodedPin_FallsBackToHighestWhenNoCompatible(t *test
 }
 
 func TestResolveActionPinFromHardcodedPins_SkipHardcodedFallback(t *testing.T) {
-	t.Run("returns false immediately when SkipHardcodedFallback is set", func(t *testing.T) {
+	t.Run("returns false immediately when SkipHardcodedFallback is set and version is a tag", func(t *testing.T) {
 		ctx := &PinContext{SkipHardcodedFallback: true, Warnings: make(map[string]bool)}
 
-		// actions/checkout has hardcoded pins, but SkipHardcodedFallback should prevent use
+		// actions/checkout has hardcoded pins, but SkipHardcodedFallback should prevent version→SHA lookup
 		result, ok := resolveActionPinFromHardcodedPins("actions/checkout", "v4", false, ctx)
 
-		assert.False(t, ok, "Expected SkipHardcodedFallback to prevent hardcoded pin lookup")
-		assert.Empty(t, result, "Expected no pinned result when SkipHardcodedFallback is set")
+		assert.False(t, ok, "Expected SkipHardcodedFallback to prevent version→SHA hardcoded pin lookup")
+		assert.Empty(t, result, "Expected no pinned result when SkipHardcodedFallback is set for version tag")
+	})
+
+	t.Run("allows SHA→version lookup even when SkipHardcodedFallback is set", func(t *testing.T) {
+		// This is the regression test for the non-deterministic pinning bug.
+		// When a workflow already pins an action with a SHA (e.g. @9c091bb... # v7.0.0)
+		// and SkipHardcodedFallback is true (e.g. because GH_HOST is a non-github.com host),
+		// the SHA→version lookup must still succeed to preserve the human-readable version comment.
+		// Without the fix, the fallback would emit FormatPinnedActionReference(repo, sha, sha),
+		// producing "# 9c091bb..." instead of "# v7.0.0".
+		latestPin, ok := GetLatestActionPinByRepo("actions/checkout")
+		require.True(t, ok, "expected embedded pin for actions/checkout")
+
+		ctx := &PinContext{SkipHardcodedFallback: true, Warnings: make(map[string]bool)}
+
+		result, found := resolveActionPinFromHardcodedPins("actions/checkout", latestPin.SHA, true, ctx)
+
+		require.True(t, found, "Expected SHA→version lookup to succeed even with SkipHardcodedFallback=true")
+		assert.Equal(t, FormatPinnedActionReference("actions/checkout", latestPin.SHA, latestPin.Version), result,
+			"Expected version comment to use tag, not SHA")
 	})
 
 	t.Run("allows hardcoded pins when SkipHardcodedFallback is not set", func(t *testing.T) {

pkg/actionpins/spec_test.go

@@ -334,8 +334,8 @@ func TestSpec_PublicAPI_ResolveActionPin_EnforcePinned(t *testing.T) {
 }
 
 // TestSpec_PublicAPI_ResolveActionPin_SkipHardcodedFallback validates that setting
-// PinContext.SkipHardcodedFallback=true prevents the embedded hardcoded pins from
-// being consulted, even for a well-known action that is present in the embedded data.
+// PinContext.SkipHardcodedFallback=true blocks version→SHA fallback against the
+// embedded hardcoded pins while still allowing SHA→version comment labeling.
 func TestSpec_PublicAPI_ResolveActionPin_SkipHardcodedFallback(t *testing.T) {
 	t.Run("known action with SkipHardcodedFallback=true returns empty result", func(t *testing.T) {
 		// actions/checkout has entries in the embedded pins.
@@ -360,6 +360,33 @@ func TestSpec_PublicAPI_ResolveActionPin_SkipHardcodedFallback(t *testing.T) {
 		require.NotEmpty(t, result, "SkipHardcodedFallback=false should allow hardcoded pin lookup")
 		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) {
+		// Regression test for non-deterministic pin comments bug.
+		//
+		// When a workflow already uses a SHA-pinned action reference (e.g.
+		// actions/checkout@9c091bb... # v7.0.0) and SkipHardcodedFallback=true
+		// is set (triggered when GH_HOST points to a non-github.com host), the
+		// SHA→version lookup must still succeed so that the human-readable version
+		// tag is preserved in the comment.
+		//
+		// Before the fix, the hardcoded-pin lookup was skipped entirely when
+		// SkipHardcodedFallback=true, causing the fallback to emit
+		// FormatPinnedActionReference(repo, sha, sha) which produces "# <sha>"
+		// instead of "# v7.0.0", making the lock files non-deterministic.
+		latestPin, ok := actionpins.GetLatestActionPinByRepo("actions/checkout")
+		require.True(t, ok, "expected embedded pin for actions/checkout")
+
+		ctx := &actionpins.PinContext{
+			SkipHardcodedFallback: true,
+			Warnings:              make(map[string]bool),
+		}
+		result, err := actionpins.ResolveActionPin("actions/checkout", latestPin.SHA, ctx)
+		require.NoError(t, err, "SHA resolution should not return an error")
+		expected := actionpins.FormatPinnedActionReference("actions/checkout", latestPin.SHA, latestPin.Version)
+		assert.Equal(t, expected, result, "SHA-pinned action should use version tag as comment, not the SHA itself")
+		assert.Contains(t, result, "# "+latestPin.Version, "version comment must use the human-readable tag, not the SHA")
+	})
 }
 
 // TestSpec_PublicAPI_ResolveLatestActionPin validates latest-version resolution behavior.