fix(crafter): use actual PR head commit instead of merge commit in GitHub Actions#3066
Open
waveywaves wants to merge 1 commit into
Open
fix(crafter): use actual PR head commit instead of merge commit in GitHub Actions#3066waveywaves wants to merge 1 commit into
waveywaves wants to merge 1 commit into
Conversation
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>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review, or fix all with cubic.
Contributor
Author
|
@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 |
447ea9e to
9cfa64e
Compare
…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>
9cfa64e to
eca15ec
Compare
Contributor
Author
|
@migmartri @jiparis ptal |
Member
|
@waveywaves please fix the commit signatures, thanks
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

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