Strengthen actionpins spec tests for cache keys, fallback auditing, and container digest validation - #48497
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
actionpins spec tests for cache keys, fallback auditing, and container digest validation
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #48497 does not have the 'implementation' label and has ≤100 new lines of code in business logic directories (41 additions detected, threshold is 100). |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Test Quality Sentinel completed test quality analysis. |
There was a problem hiding this comment.
Pull request overview
Strengthens actionpins public API specification tests for edge cases and failure behavior.
Changes:
- Adds empty cache-key input coverage.
- Verifies unknown repositories do not emit resolution failures.
- Tests malformed container digest rejection and improves assertion diagnostics.
Show a summary per file
| File | Description |
|---|---|
pkg/actionpins/spec_test.go |
Expands edge-case and auditing assertions. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 1/1 changed files
- Comments generated: 0
- Review effort level: Medium
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd — the changes are a solid incremental improvement to spec coverage.
📋 Key Themes & Highlights
Positive Highlights
- ✅ Empty-input cache key cases are a good boundary test — they pin the
repo@versionformat contract explicitly - ✅ Adding an assertion message to the
ErrorTypecheck is a clear quality improvement - ✅ Capturing
RecordResolutionFailurein the unknown-repo subtest correctly extends the observable spec - ✅ The malformed-digest container subtest meaningfully distinguishes from the "no digest" case
Minor Gaps (see inline comments)
- Malformed-digest subtest — missing assertion on
ctx.Warningsabsence to fully cover the rejection contract - Unknown-repo subtest name — the callback-silence guarantee isn't reflected in the test name, making the spec harder to read
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 36.4 AIC · ⌖ 4.79 AIC · ⊞ 6.7K
Comment /matt to run again
| } | ||
| result := actionpins.ApplyContainerPinMapping("ghcr.io/owner/image:latest", ctx) | ||
| assert.Equal(t, "ghcr.io/owner/image:latest", result, | ||
| "mapping with malformed @sha256 digest should be rejected and image returned unchanged") |
There was a problem hiding this comment.
[/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.
| }, | ||
| } | ||
|
|
||
| result := actionpins.ResolveLatestActionPin("does-not-exist/x", ctx) |
There was a problem hiding this comment.
[/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.
Test Quality Sentinel ReportTest Quality Score: 96/100 — Excellent
Metrics (4 test groups, 17 sub-cases)
Verdictpassed. 0% implementation tests (threshold: 30%). All new tests verify user-visible behavioral contracts. Highlights:
References: https://github.com/github/gh-aw/actions/runs/30326214265
|
There was a problem hiding this comment.
Test-only PR, low risk overall. One non-blocking observation posted inline about a duplicative subtest.
💡 Review notes
- Empty-repo/empty-version cache-key cases are correct and trivially verify
FormatCacheKey's simple concatenation. - The added assertion message on
ErrorTypeand theRecordResolutionFailurecallback assertion in the unknown-repo fallback subtest are solid, meaningful additions. - The new "malformed digest" container-mapping subtest, however, exercises the exact same regex-rejection branch as the pre-existing "without digest" subtest — see inline comment for a stronger alternative (e.g. wrong-length/case digest or wrong algorithm prefix) that would actually add coverage.
No correctness, concurrency, or security-adjacent issues found in this diff.
🔎 Code quality review by PR Code Quality Reviewer · sonnet50 · 47.2 AIC · ⌖ 4.41 AIC · ⊞ 7.4K
Comment /review to run again
| "ghcr.io/owner/image:latest": "registry.acme.com/image:latest@sha256:not-a-valid-digest", | ||
| }, | ||
| } | ||
| result := actionpins.ApplyContainerPinMapping("ghcr.io/owner/image:latest", ctx) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
The four additions are all correct and high-signal:
- Cache key boundary cases:
FormatCacheKeyisrepo+"@"+version, so@v4,actions/checkout@, and@are the exact expected values. - Failure message on assert.Equal: Minor clarity improvement, no issues.
- Fallback auditing assertion:
ResolveLatestActionPinon an unknown repo bypassesRecordResolutionFailureentirely, soassert.Empty(failures)is accurate and adds real behavioral coverage. - Malformed digest container test:
containerDigestPinPatternrequires exactly 64[a-f0-9]hex chars;not-a-valid-digestcorrectly fails, and the new subtest documents the rejection path complementing the existing no-digest case.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
proxy.golang.org
To allow these domains, add them to the
network.allowedlist in your workflow frontmatter:
network:
allowed:
- defaults
- "proxy.golang.org"See Network Configuration for more information.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 25.6 AIC · ⌖ 4.57 AIC · ⊞ 5K
This PR tightens
pkg/actionpins/spec_test.goin the exact areas flagged by the test-quality issue: missing edge-case coverage, one weak assertion, and a missing malformed-digest scenario in container mapping behavior.Cache key edge coverage
TestSpec_PublicAPI_FormatCacheKeywith explicit empty-input cases:Enforce-pinned assertion clarity
assert.EqualcheckingResolutionFailure.ErrorTypeinTestSpec_PublicAPI_ResolveActionPin_EnforcePinned.Fallback auditing behavior for unknown repos
TestSpec_PublicAPI_ResolveLatestActionPin_FallbackOnEnforceError(unknown-repo subtest), added aRecordResolutionFailurecallback capture and assertion to validate callback behavior on the no-embedded-fallback path.Container mapping malformed digest case
TestSpec_PublicAPI_ApplyContainerPinMappingfor mappings that include@sha256:but with a malformed digest, asserting mapping rejection and unchanged image output.