Add PR skip detection, conflict reporting, and cherry-pick support#689
Add PR skip detection, conflict reporting, and cherry-pick support#689nocturnalastro wants to merge 2 commits intoopenshift:mainfrom
Conversation
When PRs are skipped, the script cherry-picks only kept merge commits (with -m 1) onto the downstream branch instead of pointing at upstream HEAD. Merge conflicts are detected and reported in the PR body. Skip detection layers: 1. SKIP_PRS / SKIP_COMMITS env vars for manual exclusion 2. Automatic: PRs whose files don't exist downstream are skipped 3. .upstream-sync-ignore file for path-based pattern exclusion Also updates the header docstring with all config options. Generated-by: Cursor
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 12 minutes and 46 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughUpdated the upstream-sync script to add manual and automatic PR skipping/filtering, conflict detection, altered sync branch preparation when skips occur, expanded bug/PR extraction, new CLI flags and environment variables, and enhanced PR body generation to report skipped PRs and merge conflicts. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes ✨ 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 |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: nocturnalastro The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
hack/upstream-sync.sh (1)
342-348:⚠️ Potential issue | 🟠 Major
scan_bugs()still reads skipped commits.The title/body scan honors
FILTERED_PRS, but thesegit logcalls still walk the full${MERGE_BASE}..${UPSTREAM_REMOTE}/${UPSTREAM_BRANCH}range. Once a PR is skipped, its bug IDs can still leak intoBUG_LISTand the downstream PR title. These scans need to switch to the kept commit set whenHAS_SKIPS=true.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hack/upstream-sync.sh` around lines 342 - 348, scan_bugs() currently uses the full git range "${MERGE_BASE}..${UPSTREAM_REMOTE}/${UPSTREAM_BRANCH}" for bugs_from_commits and bugs_from_trailers which lets bug IDs from skipped PRs leak into BUG_LIST; change those git log invocations (and the BUG_LIST aggregation) to use the kept-commit set when HAS_SKIPS=true (i.e., switch the range/commit list to the variable the script already builds for kept commits/PRs such as the kept commit range or KEPT_COMMITS used elsewhere), updating the bugs_from_commits, bugs_from_trailers and BUG_LIST lines so they conditionally read from the kept-commit set when HAS_SKIPS is true while preserving the FILTERED_PRS behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@hack/upstream-sync.sh`:
- Around line 321-325: FILTERED_PRS and SYNC_COMMITS are computed once before
any cherry-picks and never updated, which lets the script claim PRs/commits that
failed to land; change the flow so you maintain a separate list (e.g.,
SUCCESSFUL_PRS or APPLIED_COMMITS) that you append to only after each
cherry-pick successfully completes, and recompute or derive SYNC_COMMITS from
that list rather than the original FILTERED_PRS; also after each cherry-pick
iteration check if any commit was applied and, if none were applied (or
conflicts prevented all remaining picks), exit/stop cleanly instead of
proceeding with stale metadata; update references in the downstream publishing
steps to use the new SUCCESSFUL_PRS/APPLIED_COMMITS variable.
- Around line 543-579: The selective cherry-pick path (when HAS_SKIPS is true)
replays upstream commits onto ${DOWNSTREAM_REMOTE}/${DOWNSTREAM_BRANCH} breaking
ancestry and causing get_sync_range() (which relies on git merge-base) to
re-detect already-synced PRs; fix by persisting a “last synced upstream SHA” and
updating it as part of the selective-sync flow instead of relying on merge-base.
Concretely: add durable storage (e.g., a file or tag) for LAST_SYNCED_UPSTREAM,
update LAST_SYNCED_UPSTREAM to the upstream SHA of the last successfully applied
commit after the loop that processes SYNC_COMMITS (and use branch_name context),
and change get_sync_range() to use LAST_SYNCED_UPSTREAM as the lower bound when
present; alternatively, if you prefer ancestry preservation, perform an
ancestry-preserving operation (e.g., git replace --graft or create proper merge
commits) when applying commits in the HAS_SKIPS branch so that git merge-base
still advances.
- Around line 214-215: The GH API call that populates changed_files (the command
assigning changed_files=$(gh api
"repos/${UPSTREAM_REPO}/pulls/${pr_number}/files" --jq '.[].filename'
2>/dev/null) || return 1) currently fetches only the first page; update this
call to use pagination (add the --paginate flag to gh api) so all filenames
across pages are returned, ensuring downstream skip decisions are made against
the complete file list.
---
Outside diff comments:
In `@hack/upstream-sync.sh`:
- Around line 342-348: scan_bugs() currently uses the full git range
"${MERGE_BASE}..${UPSTREAM_REMOTE}/${UPSTREAM_BRANCH}" for bugs_from_commits and
bugs_from_trailers which lets bug IDs from skipped PRs leak into BUG_LIST;
change those git log invocations (and the BUG_LIST aggregation) to use the
kept-commit set when HAS_SKIPS=true (i.e., switch the range/commit list to the
variable the script already builds for kept commits/PRs such as the kept commit
range or KEPT_COMMITS used elsewhere), updating the bugs_from_commits,
bugs_from_trailers and BUG_LIST lines so they conditionally read from the
kept-commit set when HAS_SKIPS is true while preserving the FILTERED_PRS
behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 11249d57-54bc-41f3-bbb3-0653780ade80
📒 Files selected for processing (1)
hack/upstream-sync.sh
| changed_files=$(gh api "repos/${UPSTREAM_REPO}/pulls/${pr_number}/files" \ | ||
| --jq '.[].filename' 2>/dev/null) || return 1 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
repo="${UPSTREAM_REPO:-k8snetworkplumbingwg/ptp-operator}"
owner="${repo%/*}"
name="${repo#*/}"
pr=$(gh api graphql -f query='
query($owner: String!, $name: String!) {
repository(owner: $owner, name: $name) {
pullRequests(first: 100, states: MERGED, orderBy: {field: UPDATED_AT, direction: DESC}) {
nodes {
number
files(first: 1) {
totalCount
}
}
}
}
}' -F owner="$owner" -F name="$name" |
jq -r '.data.repository.pullRequests.nodes[] | select(.files.totalCount > 30) | .number' | head -n1)
if [ -z "$pr" ]; then
echo "No recent merged PR with >30 files found; the bug is still latent once one appears."
exit 0
fi
echo "PR #$pr"
echo "files.totalCount from GraphQL:"
gh api graphql -f query='
query($owner: String!, $name: String!, $number: Int!) {
repository(owner: $owner, name: $name) {
pullRequest(number: $number) {
files(first: 1) {
totalCount
}
}
}
}' -F owner="$owner" -F name="$name" -F number="$pr" --jq '.data.repository.pullRequest.files.totalCount'
echo "items returned without --paginate:"
gh api "repos/$repo/pulls/$pr/files" --jq 'length'
echo "items returned with --paginate:"
gh api --paginate "repos/$repo/pulls/$pr/files" --jq '.[].filename' | wc -l | tr -d ' 'Repository: openshift/ptp-operator
Length of output: 154
🏁 Script executed:
cat -n hack/upstream-sync.sh | sed -n '210,220p'Repository: openshift/ptp-operator
Length of output: 388
🏁 Script executed:
# Check for other gh api calls in the file to see pagination patterns
rg 'gh api' hack/upstream-sync.sh -nRepository: openshift/ptp-operator
Length of output: 146
🌐 Web query:
GitHub REST API pulls files endpoint pagination default limit
💡 Result:
For the GitHub REST API endpoint “List pull request files” (GET /repos/{owner}/{repo}/pulls/{pull_number}/files), the response is paginated and returns 30 files per page by default. You can increase the page size with per_page (up to 100) and paginate with page / the Link response header. [1][2]
Example: .../pulls/123/files?per_page=100&page=2 [2]
Sources: [1] GitHub Docs – Pulls API (“List pull requests files”), [2] GitHub Docs – “Using pagination in the REST API”.
Add pagination to the PR-files API call for correct skip decisions.
The GitHub REST pulls/{pull_number}/files endpoint returns only 30 files per page by default. Without --paginate, any PR touching more than 30 files will have downstream-relevant paths silently ignored, causing incorrect skip classifications and breaking the sync logic.
Suggested fix
- changed_files=$(gh api "repos/${UPSTREAM_REPO}/pulls/${pr_number}/files" \
+ changed_files=$(gh api --paginate "repos/${UPSTREAM_REPO}/pulls/${pr_number}/files" \
--jq '.[].filename' 2>/dev/null) || return 1📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| changed_files=$(gh api "repos/${UPSTREAM_REPO}/pulls/${pr_number}/files" \ | |
| --jq '.[].filename' 2>/dev/null) || return 1 | |
| changed_files=$(gh api --paginate "repos/${UPSTREAM_REPO}/pulls/${pr_number}/files" \ | |
| --jq '.[].filename' 2>/dev/null) || return 1 |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@hack/upstream-sync.sh` around lines 214 - 215, The GH API call that populates
changed_files (the command assigning changed_files=$(gh api
"repos/${UPSTREAM_REPO}/pulls/${pr_number}/files" --jq '.[].filename'
2>/dev/null) || return 1) currently fetches only the first page; update this
call to use pagination (add the --paginate flag to gh api) so all filenames
across pages are returned, ensuring downstream skip decisions are made against
the complete file list.
nocturnalastro
force-pushed
the
upstream_sync_with_auto_skip
branch
from
071c1d7 to
836fec0
Compare
Add automatic detection of upstream-only PRs (files not present downstream), .upstream-sync-ignore pattern matching, and manual SKIP_PRS/SKIP_COMMITS env vars. Skipped PRs are reverted on top of the full upstream history, preserving all original commit SHAs so git merge-base continues to work on subsequent runs. Revert commits are prefixed with "downstream-only:" for easy identification. Add --new-pr flag to force creating a new PR when lacking permissions to update an existing one, and --branch-suffix=TEXT to disambiguate branch names. Fix macOS compatibility: replace GNU sed multi-line join with portable paste+sed, and replace grep -oP with sed equivalents. Generated-by: Cursor Made-with: Cursor
nocturnalastro
force-pushed
the
upstream_sync_with_auto_skip
branch
from
836fec0 to
901c527
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@hack/upstream-sync.sh`:
- Around line 389-390: The bug: skipped PRs still contribute IDs because
scan_bugs() and the Jira fallback operate on the unfiltered upstream range; to
fix, ensure filtering happens before any bug extraction and that scan_bugs() and
the Jira fallback use the already-filtered PR list (FILTERED_PRS) instead of the
raw upstream range. Concretely, move the call to filter_skipped so it runs
before scan_bugs(), update scan_bugs() to accept/consume FILTERED_PRS (or
iterate over FILTERED_PRS instead of the full range), and change the Jira
fallback logic that repopulates PR numbers (the code path that runs when
FILTERED_PRS is empty) to derive PRs from FILTERED_PRS rather than the raw
commit range so skipped PR numbers cannot leak into the downstream PR
title/body.
- Around line 576-585: The current loop over REVERT_COMMITS silently continues
when git revert -m 1 conflicts; change the else branch in the REVERT_COMMITS
handling so the script stops the sync with a non-zero exit (or calls a
fatal/abort helper) after reporting the failure instead of continuing;
specifically modify the block handling the failing git revert (the git revert -m
1 --no-commit "$sha" failure path) to log a clear error including the short SHA
and original subject, avoid continuing, and exit(1) so detect_conflicts() or the
caller can surface the problem and prevent an inconsistent SKIPPED_PRS state.
- Around line 228-239: The skip check only inspects the new path from gh api
(variable changed_files / filename) and misses renamed files whose old paths are
reported in previous_filename; update the logic that builds/iterates
changed_files (the gh api "repos/${UPSTREAM_REPO}/pulls/${pr_number}/files" call
and the while loop that reads variable file) to also include previous_filename
values, and when testing existence against the downstream repo (the git cat-file
-e check that sets any_exists_downstream=true) test both filename and
previous_filename for each entry so renamed-from downstream paths cannot be
incorrectly skipped.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c3d5148e-68b1-4dc7-996f-8c1292a94e87
📒 Files selected for processing (1)
hack/upstream-sync.sh
| changed_files=$(gh api "repos/${UPSTREAM_REPO}/pulls/${pr_number}/files" \ | ||
| --jq '.[].filename' 2>/dev/null) || return 1 | ||
|
|
||
| if [ -z "$changed_files" ]; then | ||
| return 1 | ||
| fi | ||
|
|
||
| local any_exists_downstream=false | ||
| while IFS= read -r file; do | ||
| [ -z "$file" ] && continue | ||
| if git cat-file -e "${DOWNSTREAM_REMOTE}/${DOWNSTREAM_BRANCH}:${file}" 2>/dev/null; then | ||
| any_exists_downstream=true |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
GitHub REST API pull request files endpoint previous_filename renamed
💡 Result:
For List pull request files (GET /repos/{owner}/{repo}/pulls/{pull_number}/files), a renamed file is represented as a normal “pull request file” object where:
statuswill be"renamed"filenameis the new pathprevious_filenameis the old path (present when the file was detected as a rename) [2]
GitHub’s REST docs for this endpoint also note it’s paginated (30/page by default, per_page up to 100) and responses include up to 3000 files total [1].
Example (rename):
{
"filename": "new/path/file.txt",
"previous_filename": "old/path/file.txt",
"status": "renamed"
}[2]
Notes:
previous_filenameis typically absent unless the change is a rename (i.e.,status: "renamed"). This behavior is also described generally for GitHub “files” payloads (e.g., commit file objects) [3] and called out by GitHub staff/community when comparing REST vs GraphQL capabilities [4].
Sources: GitHub REST docs (pulls endpoint page) [1], example schema including previous_filename [2], GitHub REST docs explanation of previous_filename for renamed files [3], GitHub Community discussion referencing the REST field for renamed files [4].
🏁 Script executed:
cd /tmp && find . -name "upstream-sync.sh" 2>/dev/null | head -5Repository: openshift/ptp-operator
Length of output: 48
🏁 Script executed:
cat -n hack/upstream-sync.sh | sed -n '200,280p'Repository: openshift/ptp-operator
Length of output: 2876
Include renamed source paths in the skip check.
GitHub reports the old path of renamed files in previous_filename; filename is only the new path. With the current lookup, a PR that renames a downstream file to a new upstream-only path can be auto-skipped even though it absolutely touches downstream content.
Possible fix
- changed_files=$(gh api "repos/${UPSTREAM_REPO}/pulls/${pr_number}/files" \
- --jq '.[].filename' 2>/dev/null) || return 1
+ changed_files=$(gh api "repos/${UPSTREAM_REPO}/pulls/${pr_number}/files" \
+ --jq '.[] | [.filename, .previous_filename // empty] | .[]' 2>/dev/null) || return 1📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| changed_files=$(gh api "repos/${UPSTREAM_REPO}/pulls/${pr_number}/files" \ | |
| --jq '.[].filename' 2>/dev/null) || return 1 | |
| if [ -z "$changed_files" ]; then | |
| return 1 | |
| fi | |
| local any_exists_downstream=false | |
| while IFS= read -r file; do | |
| [ -z "$file" ] && continue | |
| if git cat-file -e "${DOWNSTREAM_REMOTE}/${DOWNSTREAM_BRANCH}:${file}" 2>/dev/null; then | |
| any_exists_downstream=true | |
| changed_files=$(gh api "repos/${UPSTREAM_REPO}/pulls/${pr_number}/files" \ | |
| --jq '.[] | [.filename, .previous_filename // empty] | .[]' 2>/dev/null) || return 1 | |
| if [ -z "$changed_files" ]; then | |
| return 1 | |
| fi | |
| local any_exists_downstream=false | |
| while IFS= read -r file; do | |
| [ -z "$file" ] && continue | |
| if git cat-file -e "${DOWNSTREAM_REMOTE}/${DOWNSTREAM_BRANCH}:${file}" 2>/dev/null; then | |
| any_exists_downstream=true |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@hack/upstream-sync.sh` around lines 228 - 239, The skip check only inspects
the new path from gh api (variable changed_files / filename) and misses renamed
files whose old paths are reported in previous_filename; update the logic that
builds/iterates changed_files (the gh api
"repos/${UPSTREAM_REPO}/pulls/${pr_number}/files" call and the while loop that
reads variable file) to also include previous_filename values, and when testing
existence against the downstream repo (the git cat-file -e check that sets
any_exists_downstream=true) test both filename and previous_filename for each
entry so renamed-from downstream paths cannot be incorrectly skipped.
| for sha in $REVERT_COMMITS; do | ||
| log "Reverting skipped merge commit ${sha:0:9}..." | ||
| if git revert -m 1 --no-commit "$sha" 2>/dev/null; then | ||
| local orig_subject | ||
| orig_subject=$(git log -1 --format=%s "$sha") | ||
| git commit -m "downstream-only: Revert \"${orig_subject}\"" 2>/dev/null | ||
| else | ||
| log "WARNING: Revert of ${sha:0:9} had conflicts, aborting revert" | ||
| git revert --abort 2>/dev/null || true | ||
| fi |
There was a problem hiding this comment.
Stop the sync when a skipped merge cannot be reverted.
When git revert -m 1 conflicts, the code aborts the revert and continues. That leaves the skipped PR's changes on the branch while SKIPPED_PRS and the PR body still say it was excluded, and detect_conflicts() won't surface the failed skip because the worktree is clean again.
Safer fallback
- else
- log "WARNING: Revert of ${sha:0:9} had conflicts, aborting revert"
- git revert --abort 2>/dev/null || true
+ else
+ CONFLICT_FILES=$(printf '%s\n%s' "$CONFLICT_FILES" "$(git diff --name-only --diff-filter=U 2>/dev/null || true)" | sed '/^$/d' | sort -u)
+ git revert --abort 2>/dev/null || true
+ log "ERROR: Revert of ${sha:0:9} conflicted; cannot safely honor the skip list"
+ cd "$original_dir"
+ return 1
fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@hack/upstream-sync.sh` around lines 576 - 585, The current loop over
REVERT_COMMITS silently continues when git revert -m 1 conflicts; change the
else branch in the REVERT_COMMITS handling so the script stops the sync with a
non-zero exit (or calls a fatal/abort helper) after reporting the failure
instead of continuing; specifically modify the block handling the failing git
revert (the git revert -m 1 --no-commit "$sha" failure path) to log a clear
error including the short SHA and original subject, avoid continuing, and
exit(1) so detect_conflicts() or the caller can surface the problem and prevent
an inconsistent SKIPPED_PRS state.
|
@nocturnalastro: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
1 participant
When PRs are skipped, the script cherry-picks only kept merge commits (with -m 1) onto the downstream branch instead of pointing at upstream HEAD. Merge conflicts are detected and reported in the PR body.
Skip detection layers:
Also updates the header docstring with all config options.
Generated-by: Cursor