fix(crafter): use actual PR head commit instead of merge commit in GitHub Actions#3066
fix(crafter): use actual PR head commit instead of merge commit in GitHub Actions#3066waveywaves wants to merge 1 commit intochainloop-dev:mainfrom
Conversation
…tHub Actions GitHub Actions creates a temporary merge commit for pull_request events, so .git/HEAD (and GITHUB_SHA) points to that synthetic commit instead of the actual PR branch head. This causes the attestation to reference a ghost commit that doesn't exist on any branch, breaking the referral graph when cross-referencing with local or agentic attestations. Read the actual PR head SHA from the GitHub event payload (pull_request.head.sha in GITHUB_EVENT_PATH) and resolve that commit from the local repo instead. Falls back gracefully to the merge commit if the override SHA can't be resolved. Fixes: chainloop-dev#3064 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Vibhav Bobade <vibhav.bobde@gmail.com>
f7f19ae to
13b5e95
Compare
There was a problem hiding this comment.
2 issues found across 3 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="pkg/attestation/crafter/cioverride.go">
<violation number="1" location="pkg/attestation/crafter/cioverride.go:85">
P2: New CI override duplicates HEAD commit assembly logic with different error handling than `gracefulGitRepoHead`, risking inconsistent attestation metadata across execution contexts.</violation>
</file>
<file name="pkg/attestation/crafter/crafter.go">
<violation number="1" location="pkg/attestation/crafter/crafter.go:444">
P2: Nil-pointer panic risk: added error logging path dereferences `opts.Logger` without checking for nil.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review, or fix all with cubic.
| return nil, err | ||
| } | ||
|
|
||
| c := &HeadCommit{ |
There was a problem hiding this comment.
P2: New CI override duplicates HEAD commit assembly logic with different error handling than gracefulGitRepoHead, risking inconsistent attestation metadata across execution contexts.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At pkg/attestation/crafter/cioverride.go, line 85:
<comment>New CI override duplicates HEAD commit assembly logic with different error handling than `gracefulGitRepoHead`, risking inconsistent attestation metadata across execution contexts.</comment>
<file context>
@@ -0,0 +1,118 @@
+ return nil, err
+ }
+
+ c := &HeadCommit{
+ Hash: commit.Hash.String(),
+ AuthorEmail: commit.Author.Email,
</file context>
| if actualSHA := resolveGitHubPRHeadSHA(); actualSHA != "" && actualSHA != headCommit.Hash { | ||
| override, err := resolveCommitByHash(cwd, actualSHA, opts.Logger) | ||
| if err != nil { | ||
| opts.Logger.Warn().Err(err).Str("sha", actualSHA).Msg("could not resolve PR head commit, keeping merge commit") |
There was a problem hiding this comment.
P2: Nil-pointer panic risk: added error logging path dereferences opts.Logger without checking for nil.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At pkg/attestation/crafter/crafter.go, line 444:
<comment>Nil-pointer panic risk: added error logging path dereferences `opts.Logger` without checking for nil.</comment>
<file context>
@@ -433,6 +433,21 @@ func initialCraftingState(cwd string, opts *InitOpts) (*api.CraftingState, error
+ if actualSHA := resolveGitHubPRHeadSHA(); actualSHA != "" && actualSHA != headCommit.Hash {
+ override, err := resolveCommitByHash(cwd, actualSHA, opts.Logger)
+ if err != nil {
+ opts.Logger.Warn().Err(err).Str("sha", actualSHA).Msg("could not resolve PR head commit, keeping merge commit")
+ } else {
+ headCommit = override
</file context>
| opts.Logger.Warn().Err(err).Str("sha", actualSHA).Msg("could not resolve PR head commit, keeping merge commit") | |
| if opts.Logger != nil { | |
| opts.Logger.Warn().Err(err).Str("sha", actualSHA).Msg("could not resolve PR head commit, keeping merge commit") | |
| } |
|
@migmartri @jiparis Ready for review. Fixes the merge commit SHA bug in GitHub Actions PR workflows (#3064). The hash is always overridden from the event payload — works with default |
Summary
Fixes #3064.
GitHub Actions creates a temporary merge commit for
pull_requestevents, so.git/HEADpoints to that synthetic commit. This causesattestation initto record a ghost commit SHA ingit.headthat doesn't exist on any branch — breaking the referral graph when cross-referencing with local or agentic attestations.Root cause
gracefulGitRepoHead()readsrepo.Head()directly via go-git, which in a GH Actions PR checkout returns the merge commit. The CI runner already capturesGITHUB_EVENT_NAMEandGITHUB_EVENT_PATHbut these weren't used to correct the git HEAD.Fix
After
gracefulGitRepoHead()resolves HEAD, check if we're in a GitHub Actionspull_requestorpull_request_targetevent. If so, read the actual PR head SHA from the event payload (pull_request.head.sha) and look up that commit in the local repo instead.pkg/attestation/crafter/cioverride.goresolveGitHubPRHeadSHA()— reads event JSON, extracts SHAresolveCommitByHash()— opens repo, looks up commit by hashpkg/attestation/crafter/crafter.go—initialCraftingStatenow calls the override aftergracefulGitRepoHeadDesign choices
cioverride.go) keeps CI-specific logic out of the general-purposecrafter.goGITHUB_EVENT_NAMEandGITHUB_EVENT_PATHonly exist in GitHub Actions, so a runner-type check would be redundantgracefulGitRepoHead— the perf cost is negligible for a one-timeatt initoperation, and it keeps the function signatures simpleFuture extensibility
The same pattern can be extended for GitLab MR pipelines (
CI_MERGE_REQUEST_SOURCE_BRANCH_SHA) by adding another resolver incioverride.go.Test plan
go build ./pkg/attestation/crafter/...passesresolveGitHubPRHeadSHA: PR event, PR target event, push event, no event, malformed JSON, missing head SHAgofmt -lclean🤖 Generated with Claude Code