Repository navigation
[codex] Fix post-merge upload SHA correlation - #2593
Erik Osterman (Cloud Posse) (osterman) wants to merge 5 commits into
Conversation
|
Tip Atmos Pro
No affected stacks workflow was detected for this pull request. 
|
Dependency Review✅ No vulnerabilities or license issues found.Scanned FilesNone |
Erik Osterman (Cloud Posse) (osterman)
added
the
patch
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughThis PR makes upload-correlation event-aware for GitHub Actions: it records PR/head/merge metadata in BaseResolution, uses ChangesMerged PR upload correlation support
sequenceDiagram
participant GitHubActions
participant resolvePRBase
participant BaseResolution
participant DescribeAffected
participant AtmosProUpload
GitHubActions->>resolvePRBase: deliver pull_request event (pull_request.head.sha, pull_request.merged, pull_request.merge_commit_sha)
resolvePRBase->>BaseResolution: populate PullRequestHeadSHA, PullRequestMergeCommitSHA, PullRequestMerged, EventAction, LocalHeadSHA, HeadSHA (upload correlation)
DescribeAffected->>BaseResolution: read HeadSHAOverride / LocalHeadSHA for upload correlation
DescribeAffected->>AtmosProUpload: send upload correlated by HeadSHA
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/ci/providers/github/base.go (1)
154-163:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGate
HEAD~1fallback to merged PRs only.
pull_request.closedincludes both merged and unmerged closures. RunningHEAD~1for unmerged closures can pick an unrelated parent and produce incorrect base resolution. The fallback should only execute for closed and merged PRs.Suggested fix
- // 2) Closed/merged PRs: HEAD~1. + // 2) Closed merged PRs: HEAD~1. // Correct when the merge commit is checked out (merge/squash strategies). - if action == "closed" { + if action == "closed" && merged { if sha, parentErr := resolveParentCommit(); parentErr == nil { return withPRMetadata(&provider.BaseResolution{ SHA: sha, Source: "HEAD~1 (merged PR, merge-base unavailable)", }), nil } else { - log.Debug("HEAD~1 failed for merged PR", "error", parentErr) + log.Debug("HEAD~1 failed for closed merged PR", "error", parentErr) } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/ci/providers/github/base.go` around lines 154 - 163, The fallback to use HEAD~1 should only run for PRs that are both closed and merged; update the conditional around the resolveParentCommit() call (the block that currently checks if action == "closed") to also verify the PR is merged (e.g., check the pull request payload's merged flag or equivalent before calling resolveParentCommit), so withPRMetadata(&provider.BaseResolution{SHA: sha, Source: "HEAD~1 (merged PR, merge-base unavailable)"}) only executes for merged PRs and not for unmerged/closed-but-not-merged events.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@pkg/ci/providers/github/base.go`:
- Around line 154-163: The fallback to use HEAD~1 should only run for PRs that
are both closed and merged; update the conditional around the
resolveParentCommit() call (the block that currently checks if action ==
"closed") to also verify the PR is merged (e.g., check the pull request
payload's merged flag or equivalent before calling resolveParentCommit), so
withPRMetadata(&provider.BaseResolution{SHA: sha, Source: "HEAD~1 (merged PR,
merge-base unavailable)"}) only executes for merged PRs and not for
unmerged/closed-but-not-merged events.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 753477f0-2cf3-4c9f-b422-822433ff75f5
📒 Files selected for processing (6)
docs/prd/native-ci/framework/base-resolution.mdinternal/exec/describe_affected.gointernal/exec/describe_affected_test.gopkg/ci/internal/provider/types.gopkg/ci/providers/github/base.gopkg/ci/providers/github/base_test.go
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2593 +/- ##
==========================================
+ Coverage 78.91% 78.94% +0.03%
==========================================
Files 1207 1207
Lines 116464 116505 +41
==========================================
+ Hits 91906 91979 +73
+ Misses 19488 19454 -34
- Partials 5070 5072 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
💥 This pull request now has conflicts. Could you fix it Erik Osterman (Cloud Posse) (@osterman)? 🙏 |
This branch has not been deployed
1 participant
what
pull_request.merge_commit_shafor native CI uploads from raw mergedpull_request.closedevents.why
settings.pro.pull_request.mergedevent behavior unchanged while making the native CI edge explicit.references
pull_request.merge_commit_shaas the landed commit SHA after merge, squash, or rebase.go test ./pkg/ci/providers/github ./internal/execandgo test ./pkg/ci/....Summary by CodeRabbit
Improvements
Bug Fixes
Documentation
Tests
Chores
Examples