Peel annotated tags to commit SHA when pinning gh-aw-actions references#26482
Peel annotated tags to commit SHA when pinning gh-aw-actions references#26482
Conversation
…it SHA for gh-aw-actions pins When resolving GitHub Actions tags to SHAs, the GitHub API returns `.object.sha` which for annotated tags is the tag object SHA, not the underlying commit SHA. Renovate (and the ecosystem) expects the peeled commit SHA. Changes: - pkg/workflow/action_resolver.go: Changed jq query to `[.object.sha, .object.type] | @tsv`. For annotated tags (type "tag"), makes a second API call to peel the tag object to the underlying commit SHA. Extracted parsing into exported ParseTagRefTSV helper. - pkg/cli/update_actions.go: Same fix, uses workflow.ParseTagRefTSV to avoid duplication. - pkg/workflow/action_resolver_test.go: Added TestParseTagRefTSV with cases for lightweight tags, annotated tags, and error cases (empty input, missing separator, empty type, short SHA). Agent-Logs-Url: https://github.com/github/gh-aw/sessions/864c4772-61db-422b-a658-ce3882f90844 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…lidation - ParseTagRefTSV: add validation for empty objType field (parts[1] == "") - ParseTagRefTSV: more specific SHA error message (shows expected vs actual length) - resolveFromGitHub/getActionSHAForTag: more specific peeled SHA error messages - ParseTagRefTSV exported (was parseTagRefTSV); update_actions.go uses workflow.ParseTagRefTSV Agent-Logs-Url: https://github.com/github/gh-aw/sessions/864c4772-61db-422b-a658-ce3882f90844 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Updates action tag resolution so gh aw compile and gh aw update-actions emit the peeled commit SHA (not the annotated tag object SHA) when pinning uses: owner/repo@... references, preventing Renovate churn.
Changes:
- Fetches both tag object SHA and object type from the GitHub API (
[.object.sha, .object.type] | @tsv) to detect annotated tags. - Adds
workflow.ParseTagRefTSVhelper and uses it in both compile-time and update-time tag resolution. - Adds unit tests for
ParseTagRefTSVcovering valid and malformed TSV outputs.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/action_resolver.go |
Adds TSV parsing helper and peels annotated tags to commit SHAs during action SHA resolution. |
pkg/cli/update_actions.go |
Applies the same annotated-tag peeling behavior when updating pinned action SHAs. |
pkg/workflow/action_resolver_test.go |
Adds unit tests for TSV parsing behavior used by the tag resolver. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 3/3 changed files
- Comments generated: 6
| func ParseTagRefTSV(line string) (sha, objType string, err error) { | ||
| line = strings.TrimSpace(line) | ||
| parts := strings.SplitN(line, "\t", 2) | ||
| if len(parts) != 2 || parts[0] == "" || parts[1] == "" { | ||
| return "", "", fmt.Errorf("unexpected format: %q", line) | ||
| } | ||
| sha = parts[0] | ||
| objType = parts[1] | ||
| if len(sha) != 40 { | ||
| return "", "", fmt.Errorf("invalid SHA format: expected 40 hex characters, got %d (%s)", len(sha), sha) | ||
| } | ||
| return sha, objType, nil |
There was a problem hiding this comment.
ParseTagRefTSV claims the SHA is “40 hex characters” but only checks length. This can accept non-hex values and also makes the error message inaccurate. Consider validating with gitutil.IsHexString (or equivalent) and returning an error when non-hex characters are present.
| if objType == "tag" { | ||
| resolverLog.Printf("Detected annotated tag for %s@%s (tag object SHA: %s), peeling to commit SHA", repo, version, sha) | ||
| tagPath := fmt.Sprintf("/repos/%s/git/tags/%s", baseRepo, sha) | ||
| cmd2 := ExecGHContext(ctx, "api", tagPath, "--jq", ".object.sha") | ||
| output2, err := cmd2.Output() | ||
| if err != nil { | ||
| return "", fmt.Errorf("failed to peel annotated tag %s@%s: %w", repo, version, err) | ||
| } | ||
| sha = strings.TrimSpace(string(output2)) | ||
| if len(sha) != 40 { | ||
| return "", fmt.Errorf("invalid peeled SHA format for %s@%s: expected 40 hex characters, got %d (%s)", repo, version, len(sha), sha) | ||
| } | ||
| resolverLog.Printf("Peeled annotated tag %s@%s to commit SHA: %s", repo, version, sha) |
There was a problem hiding this comment.
The annotated-tag peel call only queries .object.sha from /git/tags/{sha}. Git tag objects can themselves point to another tag object (object.type == "tag"), in which case this logic would still return a tag object SHA rather than a commit SHA. Consider querying both .object.sha and .object.type here as well and iterating (with a small depth cap) until the object type is no longer "tag".
| if objType == "tag" { | ||
| resolverLog.Printf("Detected annotated tag for %s@%s (tag object SHA: %s), peeling to commit SHA", repo, version, sha) | ||
| tagPath := fmt.Sprintf("/repos/%s/git/tags/%s", baseRepo, sha) | ||
| cmd2 := ExecGHContext(ctx, "api", tagPath, "--jq", ".object.sha") |
There was a problem hiding this comment.
Both GitHub API calls share the same 30s context timeout. If the first call uses most of the budget, the peel request can time out even when GitHub is healthy. Consider using a fresh timeout for the peel request (or increasing the overall budget to account for up to two API calls).
| cmd2 := ExecGHContext(ctx, "api", tagPath, "--jq", ".object.sha") | |
| peelCtx, peelCancel := context.WithTimeout(context.Background(), 30*time.Second) | |
| defer peelCancel() | |
| cmd2 := ExecGHContext(peelCtx, "api", tagPath, "--jq", ".object.sha") |
| // Annotated tags point to a tag object, not directly to a commit. | ||
| // Peel the tag object to get the underlying commit SHA so that | ||
| // emitted action pins use the stable commit SHA rather than the | ||
| // mutable tag object SHA (which changes when the tag is re-created). | ||
| if objType == "tag" { | ||
| updateLog.Printf("Detected annotated tag for %s@%s (tag object SHA: %s), peeling to commit SHA", repo, tag, sha) | ||
| output2, err := workflow.RunGHContext(ctx, "Peeling annotated tag...", "api", fmt.Sprintf("/repos/%s/git/tags/%s", repo, sha), "--jq", ".object.sha") | ||
| if err != nil { | ||
| return "", fmt.Errorf("failed to peel annotated tag: %w", err) | ||
| } | ||
| sha = strings.TrimSpace(string(output2)) | ||
| if len(sha) != 40 { | ||
| return "", fmt.Errorf("invalid peeled SHA format: expected 40 hex characters, got %d (%s)", len(sha), sha) | ||
| } | ||
| updateLog.Printf("Peeled annotated tag %s@%s to commit SHA: %s", repo, tag, sha) |
There was a problem hiding this comment.
Peeling annotated tags fetches only .object.sha from /git/tags/{sha}; tag objects may reference other tag objects (object.type == "tag"), so this can still return a tag object SHA instead of a commit SHA. Consider fetching both sha+type and iterating (with a depth cap) until the type is not "tag".
| // Annotated tags point to a tag object, not directly to a commit. | |
| // Peel the tag object to get the underlying commit SHA so that | |
| // emitted action pins use the stable commit SHA rather than the | |
| // mutable tag object SHA (which changes when the tag is re-created). | |
| if objType == "tag" { | |
| updateLog.Printf("Detected annotated tag for %s@%s (tag object SHA: %s), peeling to commit SHA", repo, tag, sha) | |
| output2, err := workflow.RunGHContext(ctx, "Peeling annotated tag...", "api", fmt.Sprintf("/repos/%s/git/tags/%s", repo, sha), "--jq", ".object.sha") | |
| if err != nil { | |
| return "", fmt.Errorf("failed to peel annotated tag: %w", err) | |
| } | |
| sha = strings.TrimSpace(string(output2)) | |
| if len(sha) != 40 { | |
| return "", fmt.Errorf("invalid peeled SHA format: expected 40 hex characters, got %d (%s)", len(sha), sha) | |
| } | |
| updateLog.Printf("Peeled annotated tag %s@%s to commit SHA: %s", repo, tag, sha) | |
| // Annotated tags can point to another tag object rather than directly to | |
| // a commit. Iteratively peel until we reach a non-tag object so that | |
| // emitted action pins use the stable underlying object SHA rather than a | |
| // mutable tag object SHA. | |
| if objType == "tag" { | |
| const maxTagPeelDepth = 10 | |
| updateLog.Printf("Detected annotated tag for %s@%s (tag object SHA: %s), peeling to underlying object SHA", repo, tag, sha) | |
| for depth := 0; objType == "tag"; depth++ { | |
| if depth >= maxTagPeelDepth { | |
| return "", fmt.Errorf("failed to peel annotated tag: exceeded max depth %d for %s@%s", maxTagPeelDepth, repo, tag) | |
| } | |
| output2, err := workflow.RunGHContext( | |
| ctx, | |
| "Peeling annotated tag...", | |
| "api", | |
| fmt.Sprintf("/repos/%s/git/tags/%s", repo, sha), | |
| "--jq", | |
| "[.object.sha, .object.type] | @tsv", | |
| ) | |
| if err != nil { | |
| return "", fmt.Errorf("failed to peel annotated tag: %w", err) | |
| } | |
| sha, objType, err = workflow.ParseTagRefTSV(string(output2)) | |
| if err != nil { | |
| return "", fmt.Errorf("failed to parse peeled tag API response for %s@%s: %w", repo, tag, err) | |
| } | |
| if len(sha) != 40 { | |
| return "", fmt.Errorf("invalid peeled SHA format: expected 40 hex characters, got %d (%s)", len(sha), sha) | |
| } | |
| } | |
| updateLog.Printf("Peeled annotated tag %s@%s to %s SHA: %s", repo, tag, objType, sha) |
| return "", fmt.Errorf("failed to peel annotated tag: %w", err) | ||
| } | ||
| sha = strings.TrimSpace(string(output2)) | ||
| if len(sha) != 40 { |
There was a problem hiding this comment.
The peeled SHA validation only checks length. To match the error message (“40 hex characters”) and to avoid accepting invalid output, validate the peeled SHA with gitutil.IsHexString (or equivalent) in addition to length.
| if len(sha) != 40 { | |
| if len(sha) != 40 || !gitutil.IsHexString(sha) { |
| // TestParseTagRefTSV verifies that parseTagRefTSV correctly parses the tab-separated | ||
| // output produced by the GitHub API jq expression `[.object.sha, .object.type] | @tsv`. | ||
| // This is the core parsing step used when resolving action tags to SHAs; it must | ||
| // distinguish lightweight tags (type "commit") from annotated tags (type "tag") so | ||
| // that annotated tags can be peeled to their underlying commit SHA. |
There was a problem hiding this comment.
The test doc comment refers to parseTagRefTSV (unexported) but the helper is ParseTagRefTSV. Updating the comment will avoid confusion when searching for the helper name.
🧪 Test Quality Sentinel ReportTest Quality Score: 90/100✅ Excellent test quality
Test Classification Details
Language SupportTests analyzed:
Analysis Notes
Each Minor observation: The test uses stdlib Test inflation note: The 90/44 line ratio (2.05:1) technically exceeds the 2:1 threshold. However, this is clearly legitimate: the production code adds one focused parsing function ( Verdict
📖 Understanding Test ClassificationsDesign Tests (High Value) verify what the system does:
Implementation Tests (Low Value) verify how the system does it:
Goal: Shift toward tests that describe the system's behavioral contract — the promises it makes to its users and collaborators. References: §24474969198
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ Test Quality Sentinel: 90/100. Test quality is excellent — 0% of new tests are implementation tests (threshold: 30%). The new TestParseTagRefTSV table-driven test provides strong behavioral coverage including 4 error paths and 1 edge case for the annotated-tag peeling logic.
Generated by the Design Decision Gate workflow. Captures the two-call API strategy for resolving annotated Git tags to their underlying commit SHA when pinning GitHub Actions references. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Commit pushed:
|
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (159 new lines across AI has analyzed the PR diff and generated a draft ADR to help you get started: 📄 Draft ADR: The draft captures the key decision: using a two-call API strategy to peel annotated Git tags to their underlying commit SHA, including the introduction of the shared What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. Why ADRs Matter
ADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
References: §24474969210 Note 🔒 Integrity filter blocked 1 itemThe following item were blocked because they don't meet the GitHub integrity level.
To allow these resources, lower tools:
github:
min-integrity: approved # merged | approved | unapproved | none
|
|
@copilot merge main and recompile |
|
Great work on this fix, The PR is well-structured: the bug is clearly explained with the two distinct SHAs, the
|
…eouts, fix test doc comment - ParseTagRefTSV: add gitutil.IsHexString validation (not just length check) - resolveFromGitHub: iterative peel loop with depth cap (10) to handle chained tag objects - resolveFromGitHub: fresh 30s timeout per peel API call (was sharing the initial ctx) - getActionSHAForTag: same iterative peel loop with depth cap using ParseTagRefTSV - TestParseTagRefTSV: add non-hex SHA test case; fix doc comment (parseTagRefTSV → ParseTagRefTSV) Agent-Logs-Url: https://github.com/github/gh-aw/sessions/155168f8-6e6a-428a-a2c2-3b0b170aafbb Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Main was already merged into this branch (commit Also addressed the pending code review feedback in
Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
gh aw compilewas emitting the annotated tag object SHA instead of the underlying commit SHA forgithub/gh-aw-actionspins, causing Renovate to repeatedly rewrite the same references (the version comment stays the same, only the SHA differs).For
github/gh-aw-actions@v0.68.1, the GitHub API's/git/ref/tags/{tag}returns two distinct SHAs:2fe53acc...— annotated tag object (what the compiler was emitting)ea222e35...— peeled commit SHA (what Renovate expects)Changes
pkg/workflow/action_resolver.go— Changed the jq query from.object.shato[.object.sha, .object.type] | @tsv. Uses an iterative peel loop (depth cap: 10) to handle chained tag objects, with a fresh 30 s timeout per peel API call. Extracts the parsing logic into an exportedParseTagRefTSVhelper that validates both length and hex characters viagitutil.IsHexString.pkg/cli/update_actions.go— Same fix ingetActionSHAForTag, usingworkflow.ParseTagRefTSVand the same iterative peel loop to avoid duplication and handle chained tag objects.pkg/workflow/action_resolver_test.go—TestParseTagRefTSVcovers lightweight tags (type=commit), annotated tags (type=tag, must peel), and error cases (empty input, missing separator, empty type, short SHA, non-hex SHA).