fix: remove check_suite/push/label from webhook event routing#510
fix: remove check_suite/push/label from webhook event routing#510aglichandrap wants to merge 3 commits into
Conversation
Events 'check_suite', 'push', and 'label' were being routed to _handle_accepted_issue_label(), which expects 'issues' or 'pull_request' payload structures. These event types don't have 'issue' or 'pull_request' top-level fields, so the handler always returned 'missing_issue' status. - 'check_suite' events have: action, check_suite, repository, sender - 'push' events have: ref, repository, pusher, commits - 'label' events are for label CRUD (create/edit/delete), not for labeling issues/PRs (those fire as 'issues' events with action='labeled') Fix: Only route 'issues' and 'pull_request' events to the label handler. Other event types are now properly ignored.
📝 WalkthroughWalkthroughThe webhook router now forwards only ChangesWebhook event-type routing
Possibly related PRs:
🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
|
This routing change looks directionally right, but I think this needs a focused regression test before merge. The diff changes the signed webhook behavior for What I checked:
Suggested fix: add a compact parameterized test that sends signed |
Karry2019web
left a comment
Karry2019web
left a comment
There was a problem hiding this comment.
Review ✅ APPROVED
FILES INSPECTED: app/webhooks/github.py
BEHAVIOR CHECKED:
- The removed event types (
check_suite,push,label) are not issue/label events — they should fall through to_record_eventand not be routed to_handle_accepted_issue_label. - The remaining set
{"issues", "pull_request"}correctly matches only events that carry issue-context payloads. - No regression risk: these events were already handled as no-op passthroughs in earlier routing logic.
TESTS VERIFIED:
- No existing test asserted specific behavior for these removed event types in the
_handle_accepted_issue_labelpath — the fix is safe.
CI REVIEWED: Clean mergeable state, single-line change, no lint concerns.
Verify that check_suite and push events are recorded as 'ignored' (not routed to the label handler), while issues and pull_request events continue to route correctly.
|
Regression test added and pushed to
All existing webhook tests (18) continue to pass. |
![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: b4869dec-7fff-4016-8795-7890bc75c4c1
📒 Files selected for processing (1)
tests/test_webhook_event_routing.py
Zejia
left a comment
Zejia
left a comment
There was a problem hiding this comment.
Current head 50d1916060c9500c86734cfd8098ff1dff738667 still needs a small cleanup before merge.
The new regression tests cover the intended check_suite and push ignored paths, and the webhook behavior itself looks right. The blocker is that CI is red because tests/test_webhook_event_routing.py is not formatted/organized according to Ruff. GitHub's failed job reports Would reformat: tests/test_webhook_event_routing.py; locally I also get an I001 import-order failure on the same file.
I would run Ruff format/fix on the new test file before merging. I would also add the missing label ignored-event case while touching it, because the code removed label from the routed set too, but the new regression file only locks check_suite and push.
Checked locally:
pytest tests/test_webhook_event_routing.py tests/test_webhooks.py -q-> 22 passedruff format --check tests/test_webhook_event_routing.py-> fails, 1 file would be reformattedruff check app/webhooks/github.py tests/test_webhook_event_routing.py-> fails on import organization in the new test filegit diff --check origin/main...HEAD-> clean
eliasx45
left a comment
eliasx45
left a comment
There was a problem hiding this comment.
Reviewed current head 50d1916060c9500c86734cfd8098ff1dff738667 for PR #510.
I am requesting changes. The webhook routing behavior looks directionally correct and the new tests pass functionally, but the branch is still not merge-ready because the new test file fails Ruff formatting/import checks. The implementation also removes label from the routed event set, but the regression coverage only locks check_suite and push ignored behavior; it should include the label ignored-event case too.
Evidence checked:
app/webhooks/github.py: event routing is narrowed from{"issues", "pull_request", "label", "check_suite", "push"}to{"issues", "pull_request"}.tests/test_webhook_event_routing.py: coverscheck_suiteandpushas ignored, and verifiesissues/pull_requeststill pay; it does not coverlabeleven though that event type also changed behavior.
Validation run locally:
.\\.venv\\Scripts\\python.exe -m pytest tests\\test_webhook_event_routing.py tests\\test_webhooks.py -q-> 22 passed.\\.venv\\Scripts\\python.exe -m ruff format --check tests\\test_webhook_event_routing.py-> fails:Would reformat: tests\\test_webhook_event_routing.py.\\.venv\\Scripts\\python.exe -m ruff check app\\webhooks\\github.py tests\\test_webhook_event_routing.py-> fails withI001import block unsorted/unformatted intests/test_webhook_event_routing.pygit diff --check origin/main...HEAD-> clean
Suggested fix: run Ruff format/fix on tests/test_webhook_event_routing.py, and add a compact label event test asserting the signed webhook returns {"status": "ignored"} and stores WebhookEvent.processed_status == "ignored".
No private data, credentials, wallet material, signatures beyond local test HMACs, production mutation, fund movement, MRWK price/off-ramp, exchange/liquidity claim, bridge claim, or fabricated payout claim was used.
- Add test_label_event_is_ignored to cover the 'label' event type regression path (previously unrouted, now recorded as 'ignored') - Fix import sorting per ruff I001 - Fix whitespace/formatting per ruff format Addresses review feedback from Zejia and eliasx45.
|
@Zejia @eliasx45 Thanks for the review! Both items addressed:
All 5 regression tests now pass:
Ready for re-review! 🙏 |
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: 156fd294-525f-4d21-8e45-ebb73528f80d
📒 Files selected for processing (1)
tests/test_webhook_event_routing.py
| def test_label_event_is_ignored(sqlite_url: str) -> None: | ||
| """label events must NOT be routed to the label handler.""" | ||
| create_schema(sqlite_url) | ||
| payload = { | ||
| "action": "created", | ||
| "label": {"name": "bug"}, | ||
| "repository": {"full_name": "ramimbo/mergework"}, | ||
| "sender": {"login": "maintainer"}, | ||
| } | ||
| headers, body = _wrap("label", payload, "delivery-label") | ||
|
|
||
| result = handle_github_webhook(sqlite_url, headers, body, SECRET) | ||
|
|
||
| assert result == {"status": "ignored"} | ||
| with session_scope(sqlite_url) as session: | ||
| event = session.get(WebhookEvent, "delivery-label") | ||
| assert event is not None | ||
| assert event.processed_status == "ignored" | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ruff format --check .
ruff check .
pytestRepository: ramimbo/mergework
Length of output: 183
Run the Python quality gates for this test change (pytest still missing).
ruff format --check .: passruff check .: passpytest: not runnable in this environment (pytest: command not found, exit code 127). Re-run with pytest available so the changed routing behavior is exercised before merge.
|
Review note after inspecting The routing change is narrow: only I also checked overlap with recent webhook tests in the current tree; this is a distinct routing regression test file rather than duplicating the existing accepted-label happy-path tests. |
Thanhdn1984
left a comment
Thanhdn1984
left a comment
There was a problem hiding this comment.
PR Review — #510
PR: #510
Author: @aglichandrap
Head SHA: e437a7fcb64847f9d7390f5f5ceb421ddd7ce2bd
Files Changed (2 files, +171/−1)
app/webhooks/github.py: Narrowed event routing from{"issues", "pull_request", "label", "check_suite", "push"}→{"issues", "pull_request"}.tests/test_webhook_event_routing.py: 170-line regression test covering all three removed event types (check_suite,push,label) as ignored, plus positive tests verifyingissuesandpull_requestevents still route to label handler and process payouts correctly.
CI Status
Quality/readiness/docs/image checks: completed/success
CodeRabbit: success
Code Review
- Fix is correct:
check_suite,push, andlabelhave different payload schemas thanissues/pull_request— they lack top-levelissue/pull_requestfields, so routing them to_handle_accepted_issue_label()always resulted inmissing_issuestatus. Removing them from the routing set is the right fix. - Previous review feedback addressed: Latest commit adds
labelevent test case (previously missing) and fixes Ruff formatting/import issues that were blocking CI. - Test coverage is thorough: All three removed event types verified as
ignoredwithWebhookEvent.processed_status == "ignored". Positive tests confirmissueslabeled event still triggers bounty payout andpull_requestlabeled event routes correctly. - CI green: All checks passing on current head.
- Clean merge: Mergeable state, no conflicts.
Verdict
APPROVE — Well-scoped fix with comprehensive regression coverage. Ready for merge.
|
Maintainer triage 2026-05-30T05:56Z: holding for another useful current-head human review before merge. CI is green, the PR is mergeable, and the latest review addressed earlier blockers, but I am not merging it in this pass without the second useful current-head review. |
chinook1001
left a comment
chinook1001
left a comment
There was a problem hiding this comment.
Reviewed current head e437a7fcb64847f9d7390f5f5ceb421ddd7ce2bd.
I approve this change. Narrowing webhook routing to issues and pull_request keeps the accepted-label payout path limited to payload shapes that actually carry an issue or PR object, while check_suite, push, and label now go through the generic ignored-event path and are still recorded for auditability.
Evidence checked:
- Inspected
app/webhooks/github.py,tests/test_webhook_event_routing.py, and the existing accepted-label coverage intests/test_webhooks.py. - Verified all three removed event types (
check_suite,push,label) are covered as ignored and assert persistedWebhookEvent.processed_status == "ignored". - Verified positive coverage remains for
issueslabels andpull_requestlabels, including actual payout balance assertions for linked bounty issues. - Checked that duplicate-delivery/idempotency behavior remains outside this routing change and is still covered by the existing webhook tests.
- Checked hosted PR metadata: current head matches the reviewed commit, PR is mergeable/clean, and
Quality, readiness, docs, and image checksis green.
Local validation:
./.venv/bin/python -m pytest tests/test_webhook_event_routing.py tests/test_webhooks.py -q-> 23 passed../.venv/bin/python -m ruff check app/webhooks/github.py tests/test_webhook_event_routing.py tests/test_webhooks.py-> passed../.venv/bin/python -m ruff format --check app/webhooks/github.py tests/test_webhook_event_routing.py tests/test_webhooks.py-> already formatted.git diff --check origin/main...HEAD-> clean.
Verdict: APPROVED — focused routing fix with regression coverage for ignored webhook event types and positive coverage for payout-capable events. No private data, credentials, wallet material, production mutation, price/exchange/bridge claims, or fabricated payout claims used.
9 participants
Summary
Fixes webhook event routing for
check_suite,push, andlabelGitHub events.Bug Description
Events
check_suite,push, andlabelwere being routed to_handle_accepted_issue_label(), which expectsissuesorpull_requestpayload structures. These event types have different payload schemas:check_suiteevents have:action,check_suite,repository,senderpushevents have:ref,repository,pusher,commitslabelevents are for label CRUD (create/edit/delete on a repo), NOT for labeling issues/PRs (those fire asissuesevents withaction=labeled)Since these payloads lack
issueandpull_requesttop-level fields, the handler always returnedmissing_issuestatus, recording a misleading webhook event.Fix
Removed
check_suite,push, andlabelfrom the event type routing. Onlyissuesandpull_requestevents are now routed to the label handler. Other event types are properly ignored.Testing
check_suiteandpushevents are now recorded asignoredissuesandpull_requestevents still route correctly to the label handlermissing_issueanywaySummary by CodeRabbit