fix: runtime guard for empty SHA in action pins + remove invalid ruby/setup-ruby@v1.319.0 entry#47048
Conversation
…/setup-ruby@v1.319.0 entry - Remove ruby/setup-ruby@v1.319.0 with empty SHA from .github/aw/actions-lock.json and pkg/actionpins/data/action_pins.json - Add countEntriesWithEmptySHA validation function in pkg/actionpins/actionpins.go - Panic in getActionPins() when any embedded entry has an empty SHA to prevent invalid compiled workflows - Add panic guard in FormatPinnedActionReference when SHA is empty (defense-in-depth) - Add tests for countEntriesWithEmptySHA and FormatPinnedActionReference empty SHA guard Closes #46791 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…sage - Rename countEntriesWithEmptySHA to collectEntriesWithEmptySHA (returns []string keys) - Include the specific problematic entry keys in the panic message for actionability - Update test names to be consistent with the codebase (PanicsWhenSHAIsEmpty) - Improve test cases to verify sorted key output Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #47048 does not have the 'implementation' label and has ≤100 new lines of code in business logic directories (58 additions detected). |
There was a problem hiding this comment.
Pull request overview
Prevents empty action-pin SHAs from generating invalid GitHub Actions references.
Changes:
- Removes the invalid Ruby action pin from three synchronized datasets.
- Adds load-time and formatting guards for empty SHAs.
- Adds unit tests for detection, ordering, and panic behavior.
Show a summary per file
| File | Description |
|---|---|
.github/aw/actions-lock.json |
Removes the invalid source pin. |
pkg/actionpins/data/action_pins.json |
Updates embedded action-pin data. |
pkg/workflow/data/action_pins.json |
Updates synchronized workflow data. |
pkg/actionpins/actionpins.go |
Adds empty-SHA runtime guards. |
pkg/actionpins/actionpins_internal_test.go |
Tests the new safeguards. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Medium
| // FormatPinnedActionReference formats a pinned action reference with repo, SHA, and version comment. | ||
| // Example: "actions/checkout@abc123 # v4.1.0" | ||
| func FormatPinnedActionReference(repo, sha, version string) string { | ||
| if sha == "" { | ||
| panic(fmt.Sprintf("FormatPinnedActionReference called with empty SHA for repo=%s version=%s — this would produce invalid workflow YAML", repo, version)) | ||
| } |
There was a problem hiding this comment.
Addressed in 9d2aee1: the FormatPinnedActionReference doc comment now documents the empty-SHA panic contract and why it is treated as an invariant violation.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
The fix is correct and well-structured. Removing the empty-SHA entry from all three JSON copies and adding two complementary guards (load-time panic in getActionPins + call-site panic in FormatPinnedActionReference) gives solid defense-in-depth. Tests cover the new helper and the panic path cleanly.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 12.6 AIC · ⌖ 4.34 AIC · ⊞ 4.9K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — two minor observations, no blocking issues.
📋 Key Themes & Highlights
Key Themes
- Test gap:
collectEntriesWithEmptySHAandFormatPinnedActionReferenceare well-tested, but the call-site guard wiring insidegetActionPinsis not independently tested — a deletion of thatifblock would go undetected. - Panic as API: Using
panicinFormatPinnedActionReferenceis a reasonable defence-in-depth choice given the upstream load-time guard, but it warrants an explicit code comment documenting the intent and tradeoff.
Positive Highlights
- ✅ Root cause properly fixed (bad data removed from all three JSON files).
- ✅ Two-layer defence (load-time + call-time) is a solid pattern.
- ✅ Sorted output in
collectEntriesWithEmptySHAmakes panic messages deterministic — great for tests and diagnostics. - ✅ Test coverage for the new helper is thorough and readable.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 20 AIC · ⌖ 4.66 AIC · ⊞ 6.6K
Comment /matt to run again
|
|
||
| func TestInitWarnings_InitializesAndPreservesMap(t *testing.T) { | ||
| t.Run("initializes nil warnings map", func(t *testing.T) { | ||
| ctx := &PinContext{} |
There was a problem hiding this comment.
[/tdd] The load-time guard added to getActionPins() (the collectEntriesWithEmptySHA call inside getActionPins) has no direct test — only the helper itself is tested. If the if emptyKeys block in getActionPins were removed, the test suite would still pass.
💡 Suggested approach
Extract the load-from-JSON path into a small internal helper (or make the test inject a fixture JSON) so the integration between the guard and the loader can be covered:
func TestGetActionPins_PanicsOnEmptySHAInEmbeddedData(t *testing.T) {
assert.Panics(t, func() { parseActionPins(jsonWithEmptySHA) })
}This closes the gap between the helper tests and the call-site guard.
@copilot please address this.
There was a problem hiding this comment.
Addressed in 9d2aee1: I extracted the embedded JSON load path into loadActionPinsData and added a direct panic test for empty-SHA fixture data so the loader/guard wiring is covered.
| @@ -227,6 +245,9 @@ func getLatestActionPinReference(repo string) string { | |||
| // FormatPinnedActionReference formats a pinned action reference with repo, SHA, and version comment. | |||
| // Example: "actions/checkout@abc123 # v4.1.0" | |||
| func FormatPinnedActionReference(repo, sha, version string) string { | |||
There was a problem hiding this comment.
[/diagnosing-bugs] Using panic for a defense-in-depth guard is a strong signal, but in a library loaded at startup it will crash the entire binary for any caller — including future callers who may never reach the formatting path. Consider returning an error instead (or logging + returning a sentinel string) so the host program can handle the case gracefully.
💡 Alternative signature
func FormatPinnedActionReference(repo, sha, version string) (string, error) {
if sha == "" {
return "", fmt.Errorf("empty SHA for %s@%s", repo, version)
}
return repo + "@" + sha + " # " + version, nil
}The load-time guard in getActionPins already prevents empty-SHA entries from reaching this function in normal usage; the panic here only fires on programming errors, which makes the tradeoff reasonable — just worth calling out explicitly in a comment so future readers understand the intent.
@copilot please address this.
There was a problem hiding this comment.
Kept the panic contract in 9d2aee1, but documented the intent explicitly: embedded data is rejected at load time, and this call-site guard remains a defense-in-depth invariant check for programming errors or corrupted pin data.
🧪 Test Quality Sentinel Report✅ Test Quality Score: 100/100 — Excellent
📊 Metrics (4 subtests)
Verdict
Strengths
|
There was a problem hiding this comment.
✅ Test Quality Sentinel: 100/100 — Excellent
0% implementation tests (threshold: 30%). All 4 subtests are design-focused, verifying behavioral contracts with comprehensive edge coverage (empty inputs, all-valid inputs, single and multiple empty entries). No code smell violations. Healthy test-to-code ratio of 1.25:1.
There was a problem hiding this comment.
Two issues should be addressed before merge
The data fix and load-time guard are sound. The FormatPinnedActionReference panic and the missing integration-level test for getActionPins() need attention.
Blocking findings
HIGH — panic in FormatPinnedActionReference: This is a public formatting utility, not a startup initializer. Panicking here is the wrong error-handling contract for a library function. Return (string, error) instead.
MEDIUM — getActionPins() panic not unit-tested: The collectEntriesWithEmptySHA helper is tested, but the wiring into getActionPins() is not. A refactor could silently break the guard with no test failure.
🔎 Code quality review by PR Code Quality Reviewer · 22.5 AIC · ⌖ 4.67 AIC · ⊞ 5.6K
Comment /review to run again
| // FormatPinnedActionReference formats a pinned action reference with repo, SHA, and version comment. | ||
| // Example: "actions/checkout@abc123 # v4.1.0" | ||
| func FormatPinnedActionReference(repo, sha, version string) string { | ||
| if sha == "" { |
There was a problem hiding this comment.
panic in a public library function will crash the caller with no recovery path. FormatPinnedActionReference is a formatting utility — panicking on empty SHA forces all callers to accept silent crashes or wrap every call in recover.
💡 Suggested fix
Return an error instead of panicking:
func FormatPinnedActionReference(repo, sha, version string) (string, error) {
if sha == "" {
return "", fmt.Errorf("FormatPinnedActionReference: empty SHA for repo=%s version=%s", repo, version)
}
return repo + "@" + sha + " # " + version, nil
}Panicking is appropriate for truly unrecoverable programmer errors. An empty SHA in a data file is a recoverable data error. The load-time guard in getActionPins() already catches this case; this second panic adds no safety but does introduce risk if FormatPinnedActionReference is ever called outside that guarded flow (e.g., from tests with synthetic data, future callers that build pins manually).
There was a problem hiding this comment.
I kept the existing panic in 9d2aee1 for the same reason as the load-time guard: emitting owner/repo@ # version is treated here as an invariant violation, not a recoverable formatting outcome. The public doc comment now makes that contract explicit.
| @@ -132,6 +132,10 @@ func getActionPins() []ActionPin { | |||
| actionPinsLog.Printf("Found %d key/version mismatches in action_pins.json", n) | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
No test covers the getActionPins() panic path — if a regression reintroduces an empty-SHA entry, the load-time guard is only validated by the integration behavior, not a unit test.
💡 Suggested fix
Add a test that calls getActionPins() with embedded data that contains an empty-SHA entry and asserts assert.Panics. The existing test TestCollectEntriesWithEmptySHA_ReturnsOnlyEmptySHAEntries covers only the helper — it does not verify that the panic is wired in correctly at the getActionPins() call site.
This matters because the two lines are coupled: if someone refactors getActionPins() to swap the check order or removes the panic call, the helper tests pass silently while the guard is gone.
There was a problem hiding this comment.
Addressed in 9d2aee1: there is now a direct empty-SHA loader test via loadActionPinsData, which exercises the guard path instead of only testing the helper in isolation.
This comment has been minimized.
This comment has been minimized.
|
@copilot run pr-finisher skill |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Triage
Rationale: Fixes invalid YAML emitted by compiler when action pin has empty SHA. Runtime guard + data fix. CI partially in-progress (12 checks), prior runs approved. Directly affects compilation correctness.
|
9d2aee1 already covers the actionable review feedback. I re-ran local validation on the current HEAD: |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Addressed in 7b298aa. The |
|
Great work on this bug fix! 🎯 This PR looks ready for review. The fix cleanly addresses the invalid What I like:
The panic messages are actionable and will help catch any future data corruption. This is solid defensive programming.
|
ruby/setup-ruby@v1.319.0was added toaction_pins.jsonwith an empty SHA (SHA resolution failed at entry creation time). Since it was the newest version, the compiler selected it and emittedruby/setup-ruby@ # v1.319.0— invalid GitHub Actions YAML.Changes
Data fix
ruby/setup-ruby@v1.319.0(empty SHA) from.github/aw/actions-lock.jsonand the syncedpkg/actionpins/data/action_pins.jsonRuntime guards
getActionPins(): panics at load time if any embedded entry has an empty SHA, including the offending keys in the message:FormatPinnedActionReference(): panics if called with an empty SHA — defense-in-depth guard that catches any code path bypassing the load-time checkcollectEntriesWithEmptySHA(): new internal helper (replaces a simple count) that returns sorted keys for use in the panic message and testsWarning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
pi.devSee Network Configuration for more information.