Skip to content

Strengthen actionpins spec tests for cache keys, fallback auditing, and container digest validation #48497

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
pelikhan merged 2 commits into from
Jul 28, 2026
+41 −2
Merged

Strengthen actionpins spec tests for cache keys, fallback auditing, and container digest validation #48497

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c830bc3
Initial plan
Copilot Jul 28, 2026
3ff6d90
Improve actionpins spec edge-case and callback assertions
Copilot Jul 28, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 41 additions & 2 deletions pkg/actionpins/spec_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,24 @@ func TestSpec_PublicAPI_FormatCacheKey(t *testing.T) {
version: "v3.0.0",
expected: "actions/setup-node@v3.0.0",
},
{
name: "formats cache key with empty repo",
repo: "",
version: "v4",
expected: "@v4",
},
{
name: "formats cache key with empty version",
repo: "actions/checkout",
version: "",
expected: "actions/checkout@",
},
{
name: "formats cache key with empty repo and version",
repo: "",
version: "",
expected: "@",
},
}

for _, tt := range tests {
Expand Down Expand Up @@ -361,7 +379,8 @@ func TestSpec_PublicAPI_ResolveActionPin_EnforcePinned(t *testing.T) {

require.Len(t, failures, tt.wantFailureCount, "resolution failures should be audited consistently")
if tt.wantFailureCount > 0 {
assert.Equal(t, tt.wantFailureType, failures[0].ErrorType)
assert.Equal(t, tt.wantFailureType, failures[0].ErrorType,
"resolution failure type should match the expected classification")
}
if tt.wantWarningKey {
assert.True(t, ctx.Warnings[actionpins.FormatCacheKey("does-not-exist/x", "v1")],
Expand Down Expand Up @@ -468,10 +487,18 @@ func TestSpec_PublicAPI_ResolveLatestActionPin_FallbackOnEnforceError(t *testing
})

t.Run("unknown repo returns empty when no embedded fallback exists", func(t *testing.T) {
ctx := &actionpins.PinContext{EnforcePinned: true, Warnings: make(map[string]bool)}
var failures []actionpins.ResolutionFailure
ctx := &actionpins.PinContext{
EnforcePinned: true,
Warnings: make(map[string]bool),
RecordResolutionFailure: func(f actionpins.ResolutionFailure) {
failures = append(failures, f)
},
}

result := actionpins.ResolveLatestActionPin("does-not-exist/x", ctx)

@github-actions github-actions Bot Jul 28, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/tdd] The new failures capture in the unknown-repo subtest asserts assert.Empty(t, failures), which is valuable. However, it also hides a subtle contract question: should an unknown repo with EnforcePinned: true not fire RecordResolutionFailure? If the intent is "empty result, no audit" that's fine, but the test name doesn't communicate this expectation. Consider renaming to make it explicit.

💡 Suggested rename 
t.Run("unknown repo returns empty and does not audit a failure when no embedded fallback exists", func(t *testing.T) {

This makes the callback-silence guarantee part of the spec, not just an incidental assertion.

@copilot please address this.

assert.Empty(t, result, "unknown repo should return empty when no embedded fallback exists")
assert.Empty(t, failures, "unknown repo without embedded pins should not invoke resolution failure callback")
})
}

Expand Down Expand Up @@ -985,4 +1012,16 @@ func TestSpec_PublicAPI_ApplyContainerPinMapping(t *testing.T) {
assert.Equal(t, "ghcr.io/owner/image:latest", result,
"mapping without valid @sha256: digest should be rejected and image returned unchanged")
})

t.Run("invalid mapping - mapped value with malformed digest returns image unchanged", func(t *testing.T) {
ctx := &actionpins.PinContext{
Warnings: make(map[string]bool),
ContainerMappings: map[string]string{
"ghcr.io/owner/image:latest": "registry.acme.com/image:latest@sha256:not-a-valid-digest",
},
}
result := actionpins.ApplyContainerPinMapping("ghcr.io/owner/image:latest", ctx)

@github-actions github-actions Bot Jul 28, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This subtest largely duplicates the coverage of the adjacent "without digest" case rather than adding a distinct regression check.

💡 Details

containerDigestPinPattern is ^[a-zA-Z0-9][a-zA-Z0-9/:_.-]*@sha256:[a-f0-9]{64}$. Both "registry.acme.com/image:latest" (no digest at all) and "registry.acme.com/image:latest@sha256:not-a-valid-digest" fail the same [a-f0-9]{64} length/charset check and hit the identical MatchString false branch in ApplyContainerPinMapping. The two subtests exercise the same code path and produce the same behavior, so this addition does not actually close a coverage gap.

A more valuable malformed-digest case would target a distinct failure mode within the digest-present branch, e.g.:

// 64 hex chars but uppercase - fails [a-f0-9] charset only, digest length correct
"registry.acme.com/image:latest@sha256:AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA"
// correct length/charset but wrong algorithm name
"registry.acme.com/image:latest@sha512:"

Either would validate a different branch of the regex than the already-covered no-digest case.

assert.Equal(t, "ghcr.io/owner/image:latest", result,
"mapping with malformed @sha256 digest should be rejected and image returned unchanged")

@github-actions github-actions Bot Jul 28, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/tdd] The new malformed-digest subtest doesn't assert the warning side-effect. The production code on line 577–580 of actionpins.go emits a warning to stderr but does NOT set ctx.Warnings, so the test is correct not to assert it — but it's worth verifying the non-presence of the mapping-applied warning key to prevent silent regressions.

💡 Suggested addition

Add a warning-key assertion after the assert.Equal to cover the full observable contract:

assert.False(t, ctx.Warnings["container-map:ghcr.io/owner/image:latest"],
    "invalid (malformed digest) mapping should not mark the mapping as applied")

This mirrors a similar assertion that would be valuable in the existing without-digest subtest too.

@copilot please address this.

})
}
Loading