Skip to content

fix: reject non-http webhook URLs for merge --notify up front - #164

Closed
vitali87 wants to merge 2 commits into
fix/stacked-merge-retargets-basefrom
fix/notify-url-scheme
Closed

vitali87 wants to merge 2 commits into
fix/stacked-merge-retargets-basefrom
fix/notify-url-scheme

Conversation

@vitali87

Copy link
Copy Markdown
Owner

Summary

merge --notify (and PR_SPLIT_WEBHOOK_URL) accepted any string. urllib.request.urlopen happily opened file:// targets — the tool then logged "Webhook notification sent to file:///…" — and a bare hostname failed only after all merges with an obscure "unknown url type" warning.

The option now has a typer callback requiring an http(s) scheme with a host; anything else exits 2 with must be an http(s) URL, got '…' before the plan is even read. The help text and README's --notify paragraph say so, and the README now also states that a delivery failure (timeout, non-2xx) is a warning that does not change the exit code.

Stacked on #160 (top of stack #99 = #67→#98→#103→#160), which owns the merge command.

Test plan

  • TestNotifyUrlValidation: file://, bare word, ftp://, https:// without a host → exit 2 before plan_exists; env var validated; http URL passes — four of six fail on the base
  • Review probed uppercase schemes, credentials in the URL, IPv6 hosts, javascript: and the absent-option path
  • 466 tests pass, ruff clean
  • Local review: 5/5

@coderabbitai

coderabbitai Bot commented Aug 30, 2026 •

Copy link
Copy Markdown

Warning

Review limit reached

Next included review available in 57 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: 1ecc7dd0-1c5a-4814-957b-cff58f45752d

📥 Commits

Reviewing files that changed from the base of the PR and between e830066 and f9292bd.

📒 Files selected for processing (3)
  • README.md
  • pr_split/cli.py
  • tests/test_cli_coverage.py

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

Resolved via the updated fix/stacked-merge-retargets-base (which already
contains origin/main).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015rnEEjXWDBdePRhKMmZr1w

Copy link
Copy Markdown
Owner Author

This change is already in main: it landed via the merge train #243, which includes this PR's head commit. GitHub can't mark a stacked PR merged when it lands that way, so I'm closing it as done.


Generated by Claude Code

@vitali87 vitali87 closed this Sep 27, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants