Refs #406: Skip stale bounty refs in webhook payouts#560
Conversation
📝 WalkthroughWalkthroughThe pull request modifies the GitHub webhook handler to make bounty selection award-aware. A new helper function identifies bounties eligible to receive additional awards, and the webhook's accepted-label handler now prefers such bounties while retaining fallback behavior for cases where no open bounties with award capacity exist. ChangesBounty Award Eligibility
🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ae4b1ac5-a83b-433b-8f1a-89e69565e4f5
📒 Files selected for processing (2)
app/webhooks/github.pytests/test_webhooks.py
| def test_accepted_pr_label_skips_closed_bounty_for_later_open_reference( | ||
| sqlite_url: str, | ||
| ) -> None: | ||
| create_schema(sqlite_url) | ||
| body = json.dumps( | ||
| { | ||
| "action": "labeled", | ||
| "label": {"name": "mrwk:accepted"}, | ||
| "pull_request": { | ||
| "number": 13, | ||
| "html_url": "https://github.com/ramimbo/mergework/pull/13", | ||
| "body": "Fixes #3\n\nBounty #4", | ||
| "user": {"login": "reviewer"}, | ||
| }, | ||
| "repository": {"full_name": "ramimbo/mergework"}, | ||
| "sender": {"login": "maintainer"}, | ||
| }, | ||
| separators=(",", ":"), | ||
| ).encode() | ||
|
|
||
| with session_scope(sqlite_url) as session: | ||
| ensure_genesis(session) | ||
| closed_bounty = create_bounty( | ||
| session, | ||
| repo="ramimbo/mergework", | ||
| issue_number=3, | ||
| issue_url="https://github.com/ramimbo/mergework/issues/3", | ||
| title="Closed stale bounty", | ||
| reward_mrwk="40", | ||
| acceptance="Already closed bounty should not block later open refs.", | ||
| ) | ||
| close_bounty( | ||
| session, | ||
| bounty_id=closed_bounty.id, | ||
| closed_by="maintainer", | ||
| reference="https://github.com/ramimbo/mergework/issues/3#close", | ||
| ) | ||
| create_bounty( | ||
| session, | ||
| repo="ramimbo/mergework", | ||
| issue_number=4, | ||
| issue_url="https://github.com/ramimbo/mergework/issues/4", | ||
| title="Current open bounty", | ||
| reward_mrwk="25", | ||
| acceptance="Accepted PR can mention stale refs before the bounty target.", | ||
| ) | ||
|
|
||
| result = handle_github_webhook( | ||
| sqlite_url, | ||
| { | ||
| "X-GitHub-Delivery": "delivery-closed-then-open-pr", | ||
| "X-GitHub-Event": "pull_request", | ||
| "X-Hub-Signature-256": _signature("secret", body), | ||
| }, | ||
| body, | ||
| "secret", | ||
| ) | ||
|
|
||
| assert result["status"] == "paid" | ||
| with session_scope(sqlite_url) as session: | ||
| assert get_balance(session, "github:reviewer") == 25_000_000 | ||
| event = session.get(WebhookEvent, "delivery-closed-then-open-pr") | ||
| assert event is not None | ||
| assert event.processed_status == "paid" | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Add an exhausted-before-open regression case.
This test proves the closed-then-open path, but the selector change also targets exhausted bounties. Please add a companion case where the first referenced bounty is exhausted and a later referenced bounty is still payable.
As per coding guidelines, tests/**/*.py: Do not request docstrings. Focus on whether tests prove the changed behavior and include negative, replay, boundary, or regression cases where relevant.
|
Reviewed PR #560 at current head No blocker found. The selection change is scoped to the accepted-label bounty lookup: it keeps the first matching bounty as the legacy fallback, but now prefers the first referenced bounty that is still open and has award capacity. That lets a PR body mention an older closed or exhausted bounty before the current payable bounty without changing the existing failure behavior when none of the referenced bounties can pay. I also checked the exhausted-before-open case raised by CodeRabbit with a local smoke scenario: create issue #3 with Validation run on this head:
GitHub readback shows the PR is open, non-draft, |
xingxi0614-cpu
left a comment
xingxi0614-cpu
left a comment
There was a problem hiding this comment.
Reviewed PR #560 current head 74bf772e65de9076013fe2c8fd1645387c8b13ae.
Verdict: no blocker found in this webhook payout bounty-selection slice.
Evidence:
- Inspected
app/webhooks/github.py: the accepted-label handler now records the first linked bounty it finds, but prefers the first linked bounty that is still open and has award capacity. This prevents an older closed/exhausted bounty reference from blocking a later open bounty reference in the same PR body. - Inspected
tests/test_webhooks.py: the new regression creates a closed stale bounty for issue#3, an open bounty for issue#4, labels a PR body containingFixes #3beforeBounty #4, and verifies the payout goes to the open bounty instead of failing on the stale one. - Confirmed the diff is focused to
app/webhooks/github.pyandtests/test_webhooks.py. - Confirmed the PR is open, non-draft, and current head is
74bf772e65de9076013fe2c8fd1645387c8b13ae.
Validation:
uv run --extra dev python -m pytest tests/test_webhooks.py -q-> 19 passeduv run --extra dev python -m pytest -q-> 415 passeduv run --extra dev ruff check app/webhooks/github.py tests/test_webhooks.py-> passeduv run --extra dev ruff format --check app/webhooks/github.py tests/test_webhooks.py-> 2 files already formatteduv run --extra dev python -m mypy app/webhooks/github.py-> successuv run --extra dev python scripts/docs_smoke.py-> docs smoke okgit diff --check origin/main...HEAD-> clean
No secrets, private data, cookies, wallet material, signatures, production mutation, private security details, price claims, liquidity claims, exchange claims, bridge claims, or off-ramp promises were used.
ramimbo
left a comment
ramimbo
left a comment
There was a problem hiding this comment.
Maintainer review: no blocker found. The change is focused to webhook bounty selection, keeps fallback behavior when no payable bounty is referenced, and has a regression for stale closed refs before a payable ref. This is safe to include in the low-conflict cleanup batch.
ramimbo
added
mrwk:accepted
4 participants
Summary
Why
A valid accepted PR body can mention an older issue with a closed MRWK bounty, then identify the current bounty target later in the body. Before this change, the webhook picked the first matching bounty row and failed with
bounty_is_not_openinstead of paying the later open bounty.Distinctness for Bounty #406: this is not another parser-form change or native/internal bounty-id guard. It handles the selection step after parsing real GitHub bounty references, specifically closed/exhausted refs before a payable ref.
Validation
./.venv/Scripts/python.exe -m pytest tests/test_webhooks.py -q-> 19 passed./.venv/Scripts/python.exe -m pytest tests/test_security.py::test_admin_page_renders_safe_webhook_events_for_cookie_admin tests/test_security.py::test_admin_webhook_events_api_lists_and_filters_processing_outcomes tests/test_security.py::test_signed_webhook_with_invalid_json_is_rejected tests/test_security.py::test_duplicate_webhook_delivery_with_different_payload_is_rejected tests/test_security.py::test_webhook_rejects_unapproved_accepted_labeler -q-> 5 passed./.venv/Scripts/python.exe -m pytest -q-> 415 passed./.venv/Scripts/python.exe -m ruff check .-> passed./.venv/Scripts/python.exe -m ruff format --check .-> 79 files already formatted./.venv/Scripts/python.exe -m mypy app-> success, no issues found in 30 source filesgit diff --check-> cleanRefs #406
Summary by CodeRabbit
Bug Fixes
Tests