refactor(webhook): give the module a service layer and a typed payload - #68
Merged
Conversation
The webhook module was the only one without a service layer: the router *was* the use case. One handler did header validation, JSON decoding, a type guard, field extraction, log binding, persistence and task scheduling, then returned an untyped dict with no response_model. None of that could be tested without an HTTP request carrying a valid HMAC signature. `parse_delivery` and `record_delivery` now hold those use cases, the router validates and calls them, and the response is a `WebhookAck` model. Raw GitHub JSON also stopped travelling into the domain. installation's `create_installation_from_webhook` indexed the payload itself, which made a GitHub shape change a KeyError in the middle of a use case -- the one place in the codebase where a use case parsed an external payload. It is now `create_installation(session, InstallationPayload)`. The model lives in installation's own schemas, not webhook's, so the dependency runs webhook -> installation rather than the other way round. The three lifecycle handlers (deleted / suspended / unsuspended) were byte-identical apart from one service call and two log-event names, sitting directly above `_apply_lifecycle_change`, which already parameterises the same axis. They share one helper now. Docs: CLAUDE.md described a layering the webhook module did not have, and still said Python 3.12 and "per-module mypy overrides".
|
helPRs session created for this PR. Skill: |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The architecture lot from the 2026-08-01 audit. No behaviour changes intended — this is layering, naming and one deduplication.
The webhook module had no service layer
It was the only module where the router was the use case.
receive_github_webhookdid six jobs — header validation, JSON decoding, aisinstancetype guard, field extraction, log-context binding, persistence and task scheduling — and returned an untypeddict[str, object]with noresponse_model. None of it could be exercised without an HTTP request carrying a valid HMAC signature.webhook/service.pynow holdsparse_delivery(raw request → validatedWebhookDelivery, raising 400s for anything malformed) andrecord_delivery(persist, returningNonefor a redelivery rather than raising). The router validates, calls them, and returns aWebhookAck.That parsing now has direct tests, including the two cases that quietly produced a wrong value before: an
installationthat is a scalar rather than an object, and anidarriving as a string.Raw GitHub JSON stopped reaching the domain
create_installation_from_webhook(session, webhook_data: dict)indexed the payload inside the use case:A GitHub shape change surfaced as a
KeyError-turned-ValueErrordeep in a use case. Every other external payload in the codebase is parsed into a Pydantic model at a named boundary — this was the one hole.It is now
create_installation(session, payload: InstallationPayload). The model lives ininstallation/schemas.py, not the webhook module: it describes an installation, so putting it there keeps the dependency runningwebhook -> installationinstead of inverting it.Three handlers that were the same handler
handle_installation_deleted,_suspendedand_unsuspendedwere byte-identical apart from which service function they called and two log-event strings — roughly 45 triplicated lines, sitting one layer above_apply_lifecycle_change, which already parameterises exactly that axis in the service. They share_handle_lifecycle_changenow.Smaller things in the same files
_announce_sessionhad two unannotated parameters and is now typed.installation/service.py's__all__was out of date: it named the renamed function and omittedverify_session_access, whichcontainer/router.pyimports seven times.__init__s, PyJWT,SecretStr, and why the SSE route takes no DB dependency.Verification
ruff+ruff format+mypyclean with no overridesparse_delivery/record_deliverydirectlyStill open after this
Two architecture findings from the audit are deliberately not in here, because each is a real change rather than a move:
create_container_sessionis still a 45-line router handler that decrypts a Fernet credential in the HTTP layer. Moving it into the service is worth doing, but it interacts with the commit-ordering fix in fix(api): break the import cycle, stop pinning DB connections, revive retries #66 and deserves its own diff.finalize_session: a validated JSON parser inservice.pyand an unvalidated markdown regex inpr_comment.py. Collapsing them onto the JSON one changes what gets posted to PRs, so it needs a decision about the skill contract first.cleanup_all_running/reconcile_stale_sessionsare still unscoped per worker (carried over from fix(api): break the import cycle, stop pinning DB connections, revive retries #66) — one worker restarting cancels its peers' live sessions.