feat: add webhook notification on merge completion#25
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a new feature that enhances the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a webhook notification feature for merge completions, which is a great addition for CI/CD integration. The implementation is clean and uses Python's standard library. My review includes two main suggestions for improvement: refining the exception handling in the new webhook function to be more specific, and executing the webhook call in a background thread to prevent blocking the main process and improve the command-line user experience. These changes will make the new feature more robust and performant.
| except Exception as exc: | ||
| logger.warning(f"Failed to send webhook notification: {exc}") |
There was a problem hiding this comment.
Catching a broad Exception is generally discouraged as it can mask unexpected errors and make debugging harder. It's better to catch more specific exceptions that you anticipate from the operations in the try block. For this block, OSError (which covers network errors like URLError and HTTPError), ValueError (for URL issues), and TypeError (for JSON serialization) would be more appropriate.
| except Exception as exc: | |
| logger.warning(f"Failed to send webhook notification: {exc}") | |
| except (OSError, ValueError, TypeError) as exc: | |
| logger.warning(f"Failed to send webhook notification: {exc}") |
| if notify: | ||
| _send_webhook(notify, { | ||
| "event": "merge_complete", | ||
| "merged": merged, | ||
| "skipped": skipped, | ||
| "failed": failed, | ||
| "success": not (failed or stopped or exited_early), | ||
| }) |
There was a problem hiding this comment.
Sending the webhook is a blocking network call that can delay the command's completion by up to 10 seconds. To provide a better user experience, it's best to send this notification in a background thread. You can use the already imported ThreadPoolExecutor to achieve this in a non-blocking way.
| if notify: | |
| _send_webhook(notify, { | |
| "event": "merge_complete", | |
| "merged": merged, | |
| "skipped": skipped, | |
| "failed": failed, | |
| "success": not (failed or stopped or exited_early), | |
| }) | |
| if notify: | |
| executor = ThreadPoolExecutor(max_workers=1) | |
| executor.submit(_send_webhook, notify, { | |
| "event": "merge_complete", | |
| "merged": merged, | |
| "skipped": skipped, | |
| "failed": failed, | |
| "success": not (failed or stopped or exited_early), | |
| }) | |
| executor.shutdown(wait=False) |
Greptile SummaryThis PR adds a Two minor remaining items:
Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant CLI as merge_all (cli.py)
participant GH as GitHub API
participant WH as Webhook URL
User->>CLI: pr-split merge [--notify URL]
CLI->>GH: get_pr_state() for each PR
GH-->>CLI: PR states
loop For each batch in DAG
CLI->>GH: merge_pr() or enable auto-merge
alt --auto mode
CLI->>GH: _poll_for_merged() (polls every 10s, 600s timeout)
GH-->>CLI: MERGED / CLOSED / timeout
end
end
CLI->>CLI: Build payload (merged, skipped_structured, failed, exit_reason)
alt notify URL provided
CLI->>WH: POST /webhook (JSON payload, timeout=10s)
WH-->>CLI: HTTP response (read & closed)
alt HTTP/network error
CLI->>CLI: logger.warning (merge flow unaffected)
end
end
CLI->>User: Exit 0 (success) or Exit 1 (failure/incomplete)
Prompt To Fix All With AIThis is a comment left during a code review.
Path: pr_split/cli.py
Line: 816-819
Comment:
**`reason` field duplicates the `id`**
The `reason` value is set to `s` — the full formatted string (e.g. `"pr-3 (draft)"`) — so it repeats the group ID that is already present in the `id` field. A consumer expecting `reason` to contain only the human-readable annotation (e.g. `"draft"`, `"fetch error"`, `"no PR"`) will instead have to strip the ID prefix out again, which defeats the purpose of splitting the two fields.
```suggestion
skipped_structured = [
{
"id": s.split(" (")[0],
"reason": s.split(" (")[1].rstrip(")") if " (" in s else s,
}
for s in skipped
]
```
This produces `{"id": "pr-3", "reason": "draft"}` — clean, non-redundant, and consistent with the structured format the previous review suggested.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: pr_split/cli.py
Line: 671-681
Comment:
**Broad `except Exception` swallows unexpected errors silently**
Catching `Exception` catches everything from `ValueError` (bad URL) to `MemoryError`, masking programming mistakes as mere log warnings. Consider narrowing to the specific error classes that `urlopen` can raise for network/HTTP failures, and let truly unexpected exceptions propagate:
```suggestion
def _send_webhook(url: str, payload: dict[str, object]) -> None:
try:
data = json_mod.dumps(payload).encode("utf-8")
req = urllib.request.Request(
url, data=data, headers={"Content-Type": "application/json"}
)
with urllib.request.urlopen(req, timeout=10) as resp:
resp.read()
logger.info(f"Webhook notification sent to {url}")
except (urllib.error.URLError, OSError) as exc:
logger.warning(f"Failed to send webhook notification: {exc}")
```
`urllib.error.URLError` (and its subclass `urllib.error.HTTPError`) covers all network-level and HTTP-level failures. `OSError` catches socket-level errors. This is specific enough to not silence real bugs.
How can I resolve this? If you propose a fix, please make it concise.Reviews (3): Last reviewed commit: "fix: add exit_reason to webhook payload ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
vitali87
force-pushed
the
feat/merge-notifications
branch
from
b0c80f3 to
a79180c
Compare
|
@greptile |
|
@greptile |
1 participant
Summary
--notify <url>flag to themergecommand (also readsPR_SPLIT_WEBHOOK_URLenv var)urllib.request— no new dependenciesExample payload:
{ "event": "merge_complete", "merged": ["pr-1", "pr-2"], "skipped": ["pr-3 (draft)"], "failed": [], "success": true }Test plan
merge --notify https://httpbin.org/postand verify payload