Follow-ups from the PR 251 review: two comments state the wrong mechanism, and the resubmit route's middleware is untested #252

New Issue

2026-08-24T00:53:49+02:00

clawbot commented

2026-08-24 00:53:49 +02:00

Non-blocking follow-ups from the review of #251. The behaviour is correct in every case below; these are accuracy and coverage gaps.

loadResubmitSource credits GORM's soft-delete scope for refusing a reaped event, but the retention reaper hard-deletes — internal/database/retention.go:288 uses Unscoped(). Right behaviour, wrong stated mechanism, and a reader who trusts the comment will assume reaped events are still recoverable.
createAndFanOut's comment claims it is "the only path by which an event and its deliveries are created". Per-delivery replay creates a delivery without an event, so the claim overreaches.
The resubmit tests call HandleEventResubmit() directly, bypassing router middleware, so auth, CSRF, ownership and rate limiting have no CI coverage on this route. All four were verified live during review (unauthenticated 403, missing/garbage/foreign CSRF 403, foreign sourceID/eventID 404, rate limit 429 at ~30 with replay's bucket spending independently), but nothing pins them against regression.

Done when the two comments describe the actual mechanism and the route's middleware chain is covered by tests that go through the router.

Non-blocking follow-ups from the review of https://git.eeqj.de/sneak/webhooker/pulls/251. The behaviour is correct in every case below; these are accuracy and coverage gaps. - `loadResubmitSource` credits GORM's soft-delete scope for refusing a reaped event, but the retention reaper hard-deletes — `internal/database/retention.go:288` uses `Unscoped()`. Right behaviour, wrong stated mechanism, and a reader who trusts the comment will assume reaped events are still recoverable. - `createAndFanOut`'s comment claims it is "the only path by which an event and its deliveries are created". Per-delivery replay creates a delivery without an event, so the claim overreaches. - The resubmit tests call `HandleEventResubmit()` directly, bypassing router middleware, so auth, CSRF, ownership and rate limiting have no CI coverage on this route. All four were verified live during review (unauthenticated 403, missing/garbage/foreign CSRF 403, foreign `sourceID`/`eventID` 404, rate limit 429 at ~30 with replay's bucket spending independently), but nothing pins them against regression. Done when the two comments describe the actual mechanism and the route's middleware chain is covered by tests that go through the router.

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: sneak/webhooker#252