Fix pr merge detection #151
Conversation
📝 WalkthroughWalkthroughThe assignment workflow's PR merge verification is updated to replace the merge endpoint status check with a direct PR fetch that evaluates the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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.
Actionable comments posted: 2
🧹 Nitpick comments (1)
scripts/assign.py (1)
148-151: Recommended: Add error handling for API calls.The PR fetch request lacks error handling. If the API call fails due to network issues or rate limiting, the code could behave unexpectedly or crash.
🔎 Proposed error handling
pr_url = f"https://api.github.com/repos/{owner}/{repo}/pulls/{pr_number}" - pr_response = requests.get(pr_url, headers=headers) - - if pr_response.status_code == 200: - pr_data = pr_response.json() + try: + pr_response = requests.get(pr_url, headers=headers) + pr_response.raise_for_status() + pr_data = pr_response.json() + except requests.RequestException as e: + print(f"Failed to fetch PR #{pr_number}: {str(e)}") + continue
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
scripts/assign.py
🧰 Additional context used
🪛 Ruff (0.14.10)
scripts/assign.py
145-145: unindent does not match any outer indentation level
(invalid-syntax)
🔇 Additional comments (1)
scripts/assign.py (1)
153-157: Approved: Core logic correctly uses themergedfield.The change from using the merge endpoint to checking the
mergedfield in the PR details is correct and addresses the original bug. The GitHub API'smergedfield accurately indicates whether a PR has been merged, unlike the merge endpoint which only checks mergeability.
| if search_data.get("total_count", 0) > 0: | ||
| pr_number = search_data["items"][0]["number"] | ||
| pr_url = f"https://api.github.com/repos/{owner}/{repo}/pulls/{pr_number}" | ||
| pr_response = requests.get(pr_url, headers=headers) | ||
|
|
||
| if pr_response.status_code == 200: | ||
| pr_data = pr_response.json() | ||
|
|
||
| if pr_data.get("merged"): | ||
| print( | ||
| f"PR #{pr_number} is merged for issue #{assigned_issue['number']}." | ||
| ) | ||
| continue | ||
|
|
There was a problem hiding this comment.
Critical: Fix indentation error.
The entire block from line 145 onwards has incorrect indentation, which will cause a Python syntax error. The static analysis tool correctly flagged this issue.
🔎 Proposed fix for indentation
- if search_data.get("total_count", 0) > 0:
- pr_number = search_data["items"][0]["number"]
- pr_url = f"https://api.github.com/repos/{owner}/{repo}/pulls/{pr_number}"
- pr_response = requests.get(pr_url, headers=headers)
-
- if pr_response.status_code == 200:
- pr_data = pr_response.json()
-
- if pr_data.get("merged"):
- print(
- f"PR #{pr_number} is merged for issue #{assigned_issue['number']}."
- )
+ if search_data.get("total_count", 0) > 0:
+ pr_number = search_data["items"][0]["number"]
+ pr_url = f"https://api.github.com/repos/{owner}/{repo}/pulls/{pr_number}"
+ pr_response = requests.get(pr_url, headers=headers)
+
+ if pr_response.status_code == 200:
+ pr_data = pr_response.json()
+
+ if pr_data.get("merged"):
+ print(
+ f"PR #{pr_number} is merged for issue #{assigned_issue['number']}."
+ )
continue
-Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Ruff (0.14.10)
145-145: unindent does not match any outer indentation level
(invalid-syntax)
🤖 Prompt for AI Agents
In scripts/assign.py around lines 145 to 158 the nested if/continue block is
mis-indented causing a Python syntax error; re-indent the block so that all
statements inside the outer if (search_data.get("total_count", 0) > 0) are
consistently indented one level further (pr_number, pr_url, pr_response, the
inner if checking pr_response.status_code, pr_data assignment, the inner if
pr_data.get("merged") and its print and continue) ensuring each nested block
uses a consistent 4-space (or project standard) indent and that the continue is
inside the merged-check branch.
| pr_number = search_data["items"][0]["number"] | ||
| pr_url = f"https://api.github.com/repos/{owner}/{repo}/pulls/{pr_number}" | ||
| pr_response = requests.get(pr_url, headers=headers) | ||
|
|
||
| if pr_response.status_code == 200: | ||
| pr_data = pr_response.json() | ||
|
|
||
| if pr_data.get("merged"): | ||
| print( | ||
| f"PR #{pr_number} is merged for issue #{assigned_issue['number']}." | ||
| ) | ||
| continue |
There was a problem hiding this comment.
Major: Check all PRs, not just the first one.
The code only examines the first PR from search results (search_data["items"][0]). If multiple PRs reference the same issue and the first PR is open while a later PR is merged, the issue will be incorrectly flagged as incomplete.
🔎 Proposed fix to check all PRs
if search_data.get("total_count", 0) > 0:
- pr_number = search_data["items"][0]["number"]
- pr_url = f"https://api.github.com/repos/{owner}/{repo}/pulls/{pr_number}"
- pr_response = requests.get(pr_url, headers=headers)
-
- if pr_response.status_code == 200:
- pr_data = pr_response.json()
-
- if pr_data.get("merged"):
- print(
- f"PR #{pr_number} is merged for issue #{assigned_issue['number']}."
- )
+ has_merged_pr = False
+ for pr_item in search_data["items"]:
+ pr_number = pr_item["number"]
+ pr_url = f"https://api.github.com/repos/{owner}/{repo}/pulls/{pr_number}"
+ pr_response = requests.get(pr_url, headers=headers)
+
+ if pr_response.status_code == 200:
+ pr_data = pr_response.json()
+
+ if pr_data.get("merged"):
+ print(
+ f"PR #{pr_number} is merged for issue #{assigned_issue['number']}."
+ )
+ has_merged_pr = True
+ break
+
+ if has_merged_pr:
continueCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In scripts/assign.py around lines 146 to 157, the code only checks the first PR
in search_data["items"][0] which can miss a merged PR later in the list; update
the logic to iterate over all items in search_data["items"], for each extract
the PR number, fetch the PR URL, verify the response, and if any
pr_data.get("merged") is True treat the issue as merged (print the message and
continue to next issue); ensure you handle the case of an empty items list and
break/continue appropriately once a merged PR is found.
1 participant
This PR fixes an issue in the assignment workflow where pull requests were incorrectly treated as merged.
Previously, the code used the GET /pulls/{pull_number}/merge endpoint and checked for a 204 status code. However, this endpoint only indicates whether a pull request is mergeable, not whether it has already been merged. As a result, unmerged PRs could be mistakenly considered completed.
This change updates the logic to:
Fetch pull request details using GET /pulls/{pull_number}
Reliably determine merge completion by checking the merged field in the response
This ensures that only actually merged pull requests are treated as completed when validating active issue assignments.
Fixes #149
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.