Summary

Dispatch GitHub App webhook events to handlers.

Review Request #15335 — Created Sept. 24, 2026 and updated Oct. 9, 2026, 3:38 p.m.

Information

Owner

maubin

Repository

Review Board

Branch

release-9.x

Bugs

Depends On

15334

Blocks

15337

Reviewers

Groups

reviewboard

People

Description

Exceptions raised from handlers are logged and we always respond with a HTTP
204 so that the webhook is considered healthy. We add the same logging to our
existing signature and payload verification exception paths, and exceptions
raised during the handling of installation events.

This also adds find_repository_for_installation() which matches by owner
and name against the repositories connected through the installation
(excluding archived ones), to pass the repository to the webhook handler.

Testing Done


Ran unit tests.
Used in the Pull Request integration work.

Commits

Summary	ID
Dispatch GitHub App webhook events to handlers. Previously, the GitHub App webhook view only handled installation related events and ignored all others. This change makes the view pass events to all the registered handlers that support them, with the core of the view still handling installation events. Exceptions raised from handlers are logged and we always respond with a HTTP 204 so that the webhook is considered healthy. We add the same logging to our existing signature and payload verification exception paths, and exceptions raised during the handling of installation events. This also adds `find_repository_for_installation()` which matches by owner and name against the repositories connected through the installation (excluding archived ones), to pass the repository to the webhook handler.	43d3094a939ab2e0552eaa862096e3c88d3417ff

Summary

Dispatch GitHub App webhook events to handlers.

Previously, the GitHub App webhook view only handled installation related events and ignored all others. This change makes the view pass events to all the registered handlers that support them, with the core of the view still handling installation events. Exceptions raised from handlers are logged and we always respond with a HTTP 204 so that the webhook is considered healthy. We add the same logging to our existing signature and payload verification exception paths, and exceptions raised during the handling of installation events. This also adds `find_repository_for_installation()` which matches by owner and name against the repositories connected through the installation (excluding archived ones), to pass the repository to the webhook handler.

43d3094a939ab2e0552eaa862096e3c88d3417ff

Issues

Description	From	Last Updated
I don't know that we need this comment. Imports inside methods are always because of this.	david	Sept. 30, 2026, 9:50 a.m.
This seems like something we should definitely get in before we ship.	david	Sept. 30, 2026, 9:51 a.m.
We can check the account owner first by comparing it to the account: if owner.lower() != account.username.lower(): return None for …	david	Sept. 30, 2026, 9:52 a.m.
I'm about to land my update for github owner/name. Can you update this to use that?	david	Sept. 30, 2026, 10:22 a.m.
If an extension registered for an event without an installation object, every delivery would get a 400 error and GitHub …	david	Sept. 30, 2026, 10:23 a.m.
Add a comma after "ones"	david	Sept. 30, 2026, 10:26 a.m.
A generated ID looks real in logs but matches nothing in GitHub's delivery history. An empty string or clearly fake …	david	Sept. 30, 2026, 10:37 a.m.
Can we add a test that covers this? (AppInstallationWebhookPayload failing after the base payload succeeds)	david	Sept. 30, 2026, 10:40 a.m.
Before this, if any of our built-in handlers failed, we'd return a 500 to github, which would report a failed …	david	Sept. 30, 2026, 10:52 a.m.
Can we put a TODO in here about farming this out to an async task once we can do that? …	david	Sept. 30, 2026, 10:59 a.m.
This should go above with the other full-package imports.	david	Sept. 30, 2026, 10:59 a.m.
Can we include an assertLogs in here?	david	Sept. 30, 2026, 11:07 a.m.
Other test handlers don't use "test." in the ID.	david	Sept. 30, 2026, 11:07 a.m.
owner is already lowercase here.	david	Oct. 8, 2026, 4:39 p.m.
This isn't including the X-GitHub-Deliver header. Does this test run?	david	Oct. 8, 2026, 4:40 p.m.

flake8 passed.

JSHint passed.

reviewboard/hostingsvcs/github/accounts.py (Diff revision 1)

I'll update this method for the changes in /r/15342/ later once they've landed.

reviewboard/hostingsvcs/github/accounts.py (Diff revision 1)

The issue has been resolved. Show all issues

I don't know that we need this comment. Imports inside methods are always because of this.

maubin

Oct. 2, 2026, 4:12 p.m.

It's something that Christian has commented on for my code in the past. I agree it doesn't really add much but don't feel strongly either way.

reviewboard/hostingsvcs/github/accounts.py (Diff revision 1)
The issue has been resolved. Show all issues
```
This seems like something we should definitely get in before we ship.
```
1. maubin
  
  Oct. 2, 2026, 4:12 p.m.
  I'll take care of it in a separate change.

reviewboard/hostingsvcs/github/accounts.py (Diff revision 1)

The issue has been resolved. Show all issues

We can check the account owner first by comparing it to the account:

if owner.lower() != account.username.lower():
    return None

for repository in account.repositories.filter(archived=False):
    ...
    if repo_name.lower() == name:
        return repository

reviewboard/hostingsvcs/github/accounts.py (Diff revision 1)

The issue has been resolved. Show all issues

I'm about to land my update for github owner/name. Can you update this to use that?

reviewboard/hostingsvcs/github/api.py (Diff revision 1)

The issue has been dropped. Show all issues

If an extension registered for an event without an installation object, every delivery would get a 400 error and GitHub would mark the webhook as unhealthy.

We should either specify this as (WebhookInstallation | None) = None or document in BaseGitHubAppWebhookHandler.events that only installation-scoped events are supported.

maubin

Oct. 2, 2026, 4:12 p.m.

Payloads will always contain an installation when the event is configured for and sent to a GitHub App. Except for the ping event, which I meant to always catch early before decoding the payload. I'll add that check in.

reviewboard/hostingsvcs/github/views.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Add a comma after "ones"
```

reviewboard/hostingsvcs/github/views.py (Diff revision 1)

The issue has been resolved. Show all issues

A generated ID looks real in logs but matches nothing in GitHub's delivery history. An empty string or clearly fake value would be better.

reviewboard/hostingsvcs/github/views.py (Diff revision 1)

The issue has been resolved. Show all issues

Can we add a test that covers this? (AppInstallationWebhookPayload failing after the base payload succeeds)

reviewboard/hostingsvcs/github/views.py (Diff revision 1)

The issue has been resolved. Show all issues

Before this, if any of our built-in handlers failed, we'd return a 500 to github, which would report a failed delivery that an admin could trigger a retry for. Now it's logged but we return 204, so the installation status could silently stay stale.
Given that the intent is to swallow failures for third-party handlers, I think we should leave core state tracking un-wrapped and let the 500 happen.

reviewboard/hostingsvcs/github/views.py (Diff revision 1)

The issue has been resolved. Show all issues

Can we put a TODO in here about farming this out to an async task once we can do that? A long-running third-party handler could cause GitHub to timeout the delivery.

reviewboard/hostingsvcs/tests/github/test_app_webhook_view.py (Diff revision 1)
The issue has been resolved. Show all issues
```
This should go above with the other full-package imports.
```

reviewboard/hostingsvcs/tests/github/test_app_webhook_view.py (Diff revision 1)

The issue has been resolved. Show all issues

Can we include an assertLogs in here?

reviewboard/hostingsvcs/tests/github/test_app_webhook_view.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Other test handlers don't use "test." in the ID.
```

Change Summary:


Use the new GitHub.get_repository_ids() instead of the old repo owner and name parsing methods.
Lets installation events fail and report a 500 back to GitHub.

Commits:

	Summary	ID
	Dispatch GitHub App webhook events to handlers. Previously, the GitHub App webhook view only handled installation related events and ignored all others. This change makes the view pass events to all the registered handlers that support them, with the core of the view still handling installation events. Exceptions raised from handlers are logged and we always respond with a HTTP 204 so that the webhook is considered healthy. We add the same logging to our existing signature and payload verification exception paths, and exceptions raised during the handling of installation events. This also adds `find_repository_for_installation()` which matches by owner and name against the repositories connected through the installation (excluding archived ones), to pass the repository to the webhook handler.	efd66f621ee9a28467273b6d0e6700156cf54daa
	Dispatch GitHub App webhook events to handlers. Previously, the GitHub App webhook view only handled installation related events and ignored all others. This change makes the view pass events to all the registered handlers that support them, with the core of the view still handling installation events. Exceptions raised from handlers are logged and we always respond with a HTTP 204 so that the webhook is considered healthy. We add the same logging to our existing signature and payload verification exception paths, and exceptions raised during the handling of installation events. This also adds `find_repository_for_installation()` which matches by owner and name against the repositories connected through the installation (excluding archived ones), to pass the repository to the webhook handler.	ce5e77bc8897b9acf78f7382231969488b65ff33

Diff:

Revision 2 (+1490 -68)

Show changes

	reviewboard/hostingsvcs/github/accounts.py
	reviewboard/hostingsvcs/github/api.py
	reviewboard/hostingsvcs/github/views.py
	reviewboard/hostingsvcs/github/webhooks.py
	reviewboard/hostingsvcs/tests/github/test_accounts.py
	reviewboard/hostingsvcs/tests/github/test_app_webhook_view.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

reviewboard/hostingsvcs/github/accounts.py (Diff revision 2)
The issue has been resolved. Show all issues
```
owner is already lowercase here.
```
reviewboard/hostingsvcs/tests/github/test_app_webhook_view.py (Diff revision 2)
The issue has been resolved. Show all issues
```
This isn't including the X-GitHub-Deliver header. Does this test run?
```

Commits:

	Summary	ID
	Dispatch GitHub App webhook events to handlers. Previously, the GitHub App webhook view only handled installation related events and ignored all others. This change makes the view pass events to all the registered handlers that support them, with the core of the view still handling installation events. Exceptions raised from handlers are logged and we always respond with a HTTP 204 so that the webhook is considered healthy. We add the same logging to our existing signature and payload verification exception paths, and exceptions raised during the handling of installation events. This also adds `find_repository_for_installation()` which matches by owner and name against the repositories connected through the installation (excluding archived ones), to pass the repository to the webhook handler.	ce5e77bc8897b9acf78f7382231969488b65ff33
	Dispatch GitHub App webhook events to handlers. Previously, the GitHub App webhook view only handled installation related events and ignored all others. This change makes the view pass events to all the registered handlers that support them, with the core of the view still handling installation events. Exceptions raised from handlers are logged and we always respond with a HTTP 204 so that the webhook is considered healthy. We add the same logging to our existing signature and payload verification exception paths, and exceptions raised during the handling of installation events. This also adds `find_repository_for_installation()` which matches by owner and name against the repositories connected through the installation (excluding archived ones), to pass the repository to the webhook handler.	43d3094a939ab2e0552eaa862096e3c88d3417ff

Diff:

Revision 3 (+1492 -68)

Show changes

	reviewboard/hostingsvcs/github/accounts.py
	reviewboard/hostingsvcs/github/api.py
	reviewboard/hostingsvcs/github/views.py
	reviewboard/hostingsvcs/github/webhooks.py
	reviewboard/hostingsvcs/tests/github/test_accounts.py
	reviewboard/hostingsvcs/tests/github/test_app_webhook_view.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```