feat(webhooks): add dispatch listing and replay#44
Conversation
|
Warning Review limit reached
Next review available in: 49 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (20)
📝 WalkthroughWalkthroughThis PR implements outbound webhook dispatch management, adding the ability to list delivery attempts with filters and replay failed dispatches. The implementation spans persistence queries, service-layer replay logic, HTTP endpoints, CLI commands, and comprehensive test coverage. ChangesWebhook Dispatch Management
Sequence DiagramsequenceDiagram
participant Client
participant WebhookAPI as Webhook API
participant Persistence as PersistenceLayer
participant Notification as NotificationService
participant Audit as AuditLog
Client->>WebhookAPI: POST /webhooks/dispatches/:id/replay (x-idempotency-key)
WebhookAPI->>Persistence: getById(dispatchId)
Persistence-->>WebhookAPI: delivery attempt record
WebhookAPI->>Persistence: loadNotificationEvent(attempt.eventId)
WebhookAPI->>Audit: query events for idempotency marker
Audit-->>WebhookAPI: already_replayed? / none
alt not already replayed
WebhookAPI->>Audit: append(replay-requested, idempotency)
WebhookAPI->>Notification: replayWebhookDispatch(event, idempotencyKey, tenant)
Notification->>Persistence: list prior webhook attempts
Notification-->>Notification: compute startAttempt
Notification->>Notification: deliverWithRetries(startAttempt)
Notification-->>WebhookAPI: replay result
WebhookAPI->>Audit: append(replayed, beforeAfter)
end
WebhookAPI-->>Client: 202 { status: "replayed" | "already_replayed" }
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
Replay idempotency leans on an audit-event lookup that runs before the webhook send, while the dedupe-marker audit event is appended after. Any audit-append failure or concurrent replay with the same key leaves a window where the webhook is delivered twice. Operator-triggered, so blast radius is bounded, but worth tightening before this lands.
TL;DR — Adds operator-facing list + single-dispatch replay for outbound webhook deliveries, with x-idempotency-key opt-in and authz scoped to operator/service principals. Implementation is clean and well-tested; the main rough edges are around replay idempotency robustness and a couple of fragile string-based checks.
Key changes
GET /webhooks/dispatchesandPOST /webhooks/dispatches/:id/replay— new operator-only routes for inspecting and replaying outbound webhook delivery attempts.replayWebhookDispatchservice — webhook-only replay that resumesdeliverWithRetriesfromnextWebhookAttemptNumber(...)so the new attempt-number continues the dispatch chain.- Audit-based replay dedupe — replay scans
audit_eventsfor a priorwebhook_dispatch_replayedentry matchingdispatchId+ caller-supplied idempotency key. - Persistence repo additions —
notificationDeliveryAttempts.getById/.listplusListNotificationDeliveryAttemptsInput; SQL list orders bycreated_at DESC, id DESC. - CLI surface —
dsar webhooks list/webhooks replay <id>via the parity map, plus--flag=valueparsing inparseCliInput.
Summary ｜ 24 files ｜ 1 commit ｜ base: main ← fix/6-dispatch-replay
Replay idempotency and ordering
Before: No way to replay a failed outbound webhook dispatch.
After: Operators can replay a single failed dispatch withx-idempotency-keyproviding best-effort dedupe via the audit log.
The replay handler dedupes by scanning audit_events for a prior webhook_dispatch_replayed entry, then calls replayWebhookDispatch, then appends the audit event. That ordering plus the lack of any guard between check and append leaves two related windows:
- if the send succeeds and the audit append fails, the webhook is delivered but no dedupe marker is written — the next replay re-sends.
- two concurrent replay requests with the same idempotency key both clear the dedupe check and both send.
Single-operator usage makes this unlikely in practice, but the contract advertised by x-idempotency-key ("safe to retry") leans on stronger guarantees than the current flow provides.
packages/backend/src/routes/webhooks/dispatches.ts · packages/backend/src/services/notifications/service.ts
Listing and pagination
Before: Outbound webhook dispatches were not introspectable from the API/CLI.
After:GET /webhooks/dispatchesreturns a paginated, filterable view of attempts with areplayableflag.
Filtering on status / channel / destination / time window happens in SQL, but the route then loads the full filtered set into memory and applies slice(offset, offset + limit); total is the unbounded count. Fine at current scale, but doesn't follow the keyset-pagination shape used by requests.listBySubject / auditEvents.list, so it will need revisiting if dispatch volume grows.
packages/backend/src/routes/webhooks/dispatches.ts · packages/internals/persistence/src/services/persistence.ts
CLI --flag=value parsing
Before:
parseFlagsonly handled space-separated flag values.
After:--flag=valueis split inline before lookahead, matching the CLI examples in the new docs.
Small, contained change; the new test in packages/cli/test/config.test.ts covers the happy path.
Claude Opus ｜ 𝕏
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/reference/api/webhooks.mdx`:
- Around line 90-96: The top-level introduction still reads “public inbound
endpoints only”; update that introductory paragraph to mention both public
inbound endpoints and protected outbound management endpoints (e.g., the GET
/webhooks/dispatches section). Edit the file-level scope text so it clearly
states that the page documents public inbound webhook endpoints and protected
outbound/management endpoints for delivery/dispatch records, and ensure any
existing phrase “public inbound endpoints only” is replaced with a balanced
description referencing both endpoint types.
- Line 135: The endpoint path string "POST /webhooks/dispatches/:id/replay" uses
colon-style path parameters while the rest of the docs use contract-style
`{id}`; update the route declaration for the replay endpoint (the "POST
/webhooks/dispatches/:id/replay" heading) to use "POST
/webhooks/dispatches/{id}/replay" so it matches the `{id}` convention used
elsewhere in the webhooks docs.
In `@packages/backend/src/routes/webhooks/dispatches.ts`:
- Around line 210-238: The current code calls
services.repos.persistence.notificationDeliveryAttempts.list and then does
in-memory slicing (attempts.slice) which causes scalability issues; update the
persistence call to support server-side pagination (e.g., accept limit and
offset or a cursor and return items plus total) and change this handler to call
notificationDeliveryAttempts.list with those pagination params instead of
slicing, then map the returned page through Effect.forEach and toDispatchSummary
(keeping the tenant pipe withTenant(tenantId)); ensure the repository returns a
{ items, total } shaped response and update the handler to use that shape
(remove attempts.slice and the page local) so large result sets aren’t loaded
into memory.
- Around line 290-314: The current idempotency check calls
services.repos.persistence.auditEvents.listByRequestId(event.requestId) and
scans all audit events, which can be large; change the query to pass a
ListAuditEventsInput with an action filter (e.g., the audit action used for
replay events) so only replay-related audit events are returned, then run the
same isPriorReplay check against that smaller result set. Update the call site
referencing listByRequestId and withTenant (and keep
replayIdempotencyKey/isPriorReplay logic unchanged) so the database only returns
events whose action matches the replay/audit action instead of all events for
the request.
In `@packages/backend/src/testing/minimal-persistence.ts`:
- Around line 599-601: The current filter treats an empty input?.status array as
a filter that excludes all rows; change the condition used in the list()
filtering so that status filtering only applies when input?.status is a
non-empty array (e.g., compute a clearly named boolean like hasStatusFilter from
Array.isArray(input?.status) && input.status.length > 0 and use it instead of
Array.isArray(input?.status) alone), and replace the inline magic condition with
that named boolean to keep behavior consistent with other in-memory fixtures and
clearer per the coding guidelines.
In `@packages/cli/src/commands/helpers.ts`:
- Around line 182-191: The helper handling route-specific request payloads
doesn't map the replay idempotency header; update the route dispatch mapping in
packages/cli/src/commands/helpers.ts to handle route.id ===
"webhooks_dispatches_replay" by returning an object that includes a headers
property mapping "x-idempotency-key" to the CLI flag (e.g.
input.flags["idempotency-key"]) so CLI calls for webhooks_dispatches_replay send
the idempotency header; locate the existing webhooks_dispatches_list branch and
add a new branch for webhooks_dispatches_replay that mirrors how other route
params are returned but includes headers: { "x-idempotency-key":
input.flags["idempotency-key"] }.
In `@packages/internals/persistence/src/services/persistence.ts`:
- Around line 1023-1064: The list handler (the list property in the notification
delivery attempts service) currently issues an unbounded SELECT; add a LIMIT to
the SQL and enforce a sane max: accept an optional input.limit from
ListNotificationDeliveryAttemptsInput, coerce/validate it (e.g., default to 100
and cap to MAX_PAGE_SIZE like 1000), then append a sql`LIMIT ${limit}` clause to
the query built in listNotificationDeliveryAttempts; ensure you reference
requireTenantId and mapNotificationDeliveryAttemptRecordEffect unchanged and
apply the validated/capped limit value so large result sets are bounded.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4c29f623-b73c-4ec4-a14f-4175e43254aa
⛔ Files ignored due to path filters (1)
packages/backend/test/__snapshots__/openapi.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (23)
docs/reference/api/index.mdxdocs/reference/api/webhooks.mdxdocs/reference/developer/cli.mdxpackages/backend/src/http-api/groups/webhooks.tspackages/backend/src/routes/webhooks.tspackages/backend/src/routes/webhooks/dispatches.tspackages/backend/src/services/notifications/service.tspackages/backend/src/testing/minimal-persistence.tspackages/backend/test/e2e/fixtures.tspackages/backend/test/e2e/tenant-isolation.test.tspackages/backend/test/lifecycle/idempotency.test.tspackages/backend/test/openapi.test.tspackages/backend/test/runtime.test.tspackages/backend/test/services/notifications.service.test.tspackages/cli/src/commands/helpers.tspackages/cli/src/commands/webhooks.tspackages/cli/src/config.tspackages/cli/src/parity/route-map.tspackages/cli/test/config.test.tspackages/cli/test/e2e/commands.e2e.test.tspackages/internals/persistence/src/index.tspackages/internals/persistence/src/services/persistence.tspackages/internals/persistence/src/types/domain.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use explicit types for function parameters and return values when they enhance clarity
Preferunknownoveranywhen the type is genuinely unknown
Use const assertions (as const) for immutable values and literal types
Leverage TypeScript's type narrowing instead of type assertions
Files:
packages/cli/src/commands/webhooks.tspackages/backend/test/services/notifications.service.test.tspackages/cli/test/config.test.tspackages/cli/test/e2e/commands.e2e.test.tspackages/cli/src/commands/helpers.tspackages/backend/test/openapi.test.tspackages/backend/src/routes/webhooks.tspackages/cli/src/parity/route-map.tspackages/internals/persistence/src/index.tspackages/backend/test/e2e/tenant-isolation.test.tspackages/backend/test/lifecycle/idempotency.test.tspackages/cli/src/config.tspackages/backend/test/e2e/fixtures.tspackages/backend/src/routes/webhooks/dispatches.tspackages/internals/persistence/src/types/domain.tspackages/backend/test/runtime.test.tspackages/backend/src/testing/minimal-persistence.tspackages/backend/src/http-api/groups/webhooks.tspackages/internals/persistence/src/services/persistence.tspackages/backend/src/services/notifications/service.ts
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,jsx,ts,tsx}: Use meaningful variable names instead of magic numbers - extract constants with descriptive names
Use arrow functions for callbacks and short functions
Preferfor...ofloops over.forEach()and indexedforloops
Use optional chaining (?.) and nullish coalescing (??) for safer property access
Prefer template literals over string concatenation
Use destructuring for object and array assignments
Useconstby default,letonly when reassignment is needed, nevervar
Alwaysawaitpromises in async functions - don't forget to use the return value
Useasync/awaitsyntax instead of promise chains for better readability
Handle errors appropriately in async code with try-catch blocks
Don't use async functions as Promise executors
Removeconsole.log,debugger, andalertstatements from production code
ThrowErrorobjects with descriptive messages, not strings or other values
Usetry-catchblocks meaningfully - don't catch errors just to rethrow them
Prefer early returns over nested conditionals for error cases
Extract complex conditions into well-named boolean variables
Use early returns to reduce nesting
Prefer simple conditionals over nested ternary operators
Don't useeval()or assign directly todocument.cookie
Avoid spread syntax in accumulators within loops
Use top-level regex literals instead of creating them in loops
Prefer specific imports over namespace imports
Use descriptive names for functions, variables, and types instead of relying on Oxlint + Oxfmt to catch naming issues
Add comments for complex logic, but prefer self-documenting code
Files:
packages/cli/src/commands/webhooks.tspackages/backend/test/services/notifications.service.test.tspackages/cli/test/config.test.tspackages/cli/test/e2e/commands.e2e.test.tspackages/cli/src/commands/helpers.tspackages/backend/test/openapi.test.tspackages/backend/src/routes/webhooks.tspackages/cli/src/parity/route-map.tspackages/internals/persistence/src/index.tspackages/backend/test/e2e/tenant-isolation.test.tspackages/backend/test/lifecycle/idempotency.test.tspackages/cli/src/config.tspackages/backend/test/e2e/fixtures.tspackages/backend/src/routes/webhooks/dispatches.tspackages/internals/persistence/src/types/domain.tspackages/backend/test/runtime.test.tspackages/backend/src/testing/minimal-persistence.tspackages/backend/src/http-api/groups/webhooks.tspackages/internals/persistence/src/services/persistence.tspackages/backend/src/services/notifications/service.ts
**/*.{test,spec}.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{test,spec}.{js,jsx,ts,tsx}: Write assertions insideit()ortest()blocks
Avoid done callbacks in async tests - use async/await instead
Don't use.onlyor.skipin committed code
Keep test suites reasonably flat - avoid excessivedescribenesting
Files:
packages/backend/test/services/notifications.service.test.tspackages/cli/test/config.test.tspackages/cli/test/e2e/commands.e2e.test.tspackages/backend/test/openapi.test.tspackages/backend/test/e2e/tenant-isolation.test.tspackages/backend/test/lifecycle/idempotency.test.tspackages/backend/test/runtime.test.ts
**/index.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Avoid barrel files (index files that re-export everything)
Files:
packages/internals/persistence/src/index.ts
🔇 Additional comments (25)
docs/reference/api/index.mdx (1)
16-16: LGTM!docs/reference/developer/cli.mdx (1)
55-55: LGTM!Also applies to: 87-93
packages/internals/persistence/src/types/domain.ts (2)
736-752: LGTM!
1464-1493: LGTM!packages/internals/persistence/src/services/persistence.ts (1)
998-1022: LGTM!packages/internals/persistence/src/index.ts (1)
36-36: LGTM!packages/backend/src/services/notifications/service.ts (4)
4-7: LGTM!Also applies to: 222-225
257-257: LGTM!Also applies to: 281-281, 287-288
290-300: LGTM!
309-377: LGTM!packages/backend/src/http-api/groups/webhooks.ts (2)
113-143: LGTM!
30-56: No action needed—Schema.Literalswith array syntax is correct.The code correctly uses
Schema.Literals(["pending", "delivered", "failed", "skipped"])for defining a union of literal types. This is the proper Effect Schema API for literal unions.packages/backend/src/routes/webhooks.ts (1)
2-5: LGTM!Also applies to: 18-19
packages/backend/src/routes/webhooks/dispatches.ts (8)
34-51: LGTM!
53-82: LGTM!
84-100: LGTM!
102-129: LGTM!
131-167: LGTM!
169-209: LGTM!Also applies to: 239-244
246-289: LGTM!
315-351: LGTM!packages/cli/src/config.ts (1)
27-33: LGTM!packages/cli/src/commands/webhooks.ts (1)
11-12: LGTM!packages/cli/src/parity/route-map.ts (1)
100-113: LGTM!packages/cli/test/e2e/commands.e2e.test.ts (1)
149-160: LGTM!
There was a problem hiding this comment.
No new issues. Reviewed the following changes:
- Reordered replay to append
webhook_dispatch_replay_requestedbefore the send, with a deterministic id (webhook-dispatch-replay:<tenant>:<dispatchId>:<encoded-caller-key>) so the audit-events unique-id constraint becomes the atomic dedupe primitive; concurrent claims fall through arecoverReplayRequestAppendbranch that re-queries and short-circuits toalready_replayed. - Promoted
x-idempotency-keyto a required header (400 otherwise) and dropped the synthesized-timestamp fallback; CLI surfaces the same requirement via--idempotency-keyinheadersForRoute. - Replaced the
key !== "outbound-resend"magic-string adapter selection with achannels-basedsupportsNotificationChannelcheck;NotificationAdapterContractgained an optionalchannelsfield andoutbound-resenddeclareschannels: ["email"]. - Pushed listing pagination into SQL via
LIMIT/OFFSETonnotificationDeliveryAttempts.listand a new.countquery for thetotal; memory persistence fixtures mirror the bounded slicing. - Added validation for
limit,offset,status,created_after,created_beforequery params (withIsoTimestampSchemanormalization) so listing returns 400 on malformed input instead of leaking through to SQL. - Tightened
isMissingWebhookDispatchErrorfrommessage.includes(dispatchId)to exactMissing ${dispatchId}equality, and the e2e fixture now raisesPersistenceEntityNotFoundErrorso the tag check is the primary path. - Updated docs (
{id}path convention, scope wording, required-header note), OpenAPI snapshot, and tests covering the new validation, 404, audit-trail, and CLI behaviors.
Claude Opus ｜ 𝕏
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/backend/src/services/notifications/service.ts`:
- Around line 351-352: input.event.eventType is being force-cast to
NotificationEventDraft["eventType"] which bypasses validation; instead add a
type guard (e.g., isNotificationEventType) that checks whether
input.event.eventType is one of the NotificationEventType union members, call
that guard before building dispatchInput, and if it returns false throw or
return an error (fail-fast) so invalid persisted strings cannot be passed into
dispatchInput/eventType; update the code around dispatchInput and any callers to
use the validated value (not a cast) and reference input.event.eventType,
NotificationEventType, NotificationEventDraft, and dispatchInput when locating
where to apply the guard.
In `@packages/backend/src/testing/minimal-persistence.ts`:
- Around line 362-369: The duplicate-audit-id check is non-atomic because it
uses Ref.get then Ref.set on auditEventsRef; replace that pair with a single
Ref.modify transaction on auditEventsRef to perform the check-and-append
atomically: inside Ref.modify read the current array, if any event.id ===
record.id return a failing Effect (new Error(`Duplicate audit event
${String(record.id)}`)) while leaving the state unchanged, otherwise return the
appended array and the record; ensure the function still returns the same Effect
type as before and remove the separate Ref.get/Ref.set usage.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1b8224e2-4d95-4fd9-8748-3a1738d1bf4e
⛔ Files ignored due to path filters (1)
packages/backend/test/__snapshots__/openapi.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (16)
docs/reference/api/webhooks.mdxdocs/reference/developer/cli.mdxpackages/backend/src/adapters/contract.tspackages/backend/src/audit/service.tspackages/backend/src/http-api/groups/webhooks.tspackages/backend/src/routes/webhooks/dispatches.tspackages/backend/src/services/notifications/service.tspackages/backend/src/testing/minimal-persistence.tspackages/backend/test/e2e/fixtures.tspackages/backend/test/runtime.test.tspackages/backend/test/services/notifications.service.test.tspackages/cli/src/commands/helpers.tspackages/cli/test/e2e/commands.e2e.test.tspackages/internals/persistence/src/services/persistence.tspackages/internals/persistence/src/types/domain.tspackages/outbound-resend/src/adapter.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use explicit types for function parameters and return values when they enhance clarity
Preferunknownoveranywhen the type is genuinely unknown
Use const assertions (as const) for immutable values and literal types
Leverage TypeScript's type narrowing instead of type assertions
Files:
packages/cli/src/commands/helpers.tspackages/backend/test/services/notifications.service.test.tspackages/outbound-resend/src/adapter.tspackages/cli/test/e2e/commands.e2e.test.tspackages/backend/src/audit/service.tspackages/backend/src/http-api/groups/webhooks.tspackages/backend/src/adapters/contract.tspackages/internals/persistence/src/types/domain.tspackages/backend/src/routes/webhooks/dispatches.tspackages/internals/persistence/src/services/persistence.tspackages/backend/src/services/notifications/service.tspackages/backend/test/e2e/fixtures.tspackages/backend/test/runtime.test.tspackages/backend/src/testing/minimal-persistence.ts
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,jsx,ts,tsx}: Use meaningful variable names instead of magic numbers - extract constants with descriptive names
Use arrow functions for callbacks and short functions
Preferfor...ofloops over.forEach()and indexedforloops
Use optional chaining (?.) and nullish coalescing (??) for safer property access
Prefer template literals over string concatenation
Use destructuring for object and array assignments
Useconstby default,letonly when reassignment is needed, nevervar
Alwaysawaitpromises in async functions - don't forget to use the return value
Useasync/awaitsyntax instead of promise chains for better readability
Handle errors appropriately in async code with try-catch blocks
Don't use async functions as Promise executors
Removeconsole.log,debugger, andalertstatements from production code
ThrowErrorobjects with descriptive messages, not strings or other values
Usetry-catchblocks meaningfully - don't catch errors just to rethrow them
Prefer early returns over nested conditionals for error cases
Extract complex conditions into well-named boolean variables
Use early returns to reduce nesting
Prefer simple conditionals over nested ternary operators
Don't useeval()or assign directly todocument.cookie
Avoid spread syntax in accumulators within loops
Use top-level regex literals instead of creating them in loops
Prefer specific imports over namespace imports
Use descriptive names for functions, variables, and types instead of relying on Oxlint + Oxfmt to catch naming issues
Add comments for complex logic, but prefer self-documenting code
Files:
packages/cli/src/commands/helpers.tspackages/backend/test/services/notifications.service.test.tspackages/outbound-resend/src/adapter.tspackages/cli/test/e2e/commands.e2e.test.tspackages/backend/src/audit/service.tspackages/backend/src/http-api/groups/webhooks.tspackages/backend/src/adapters/contract.tspackages/internals/persistence/src/types/domain.tspackages/backend/src/routes/webhooks/dispatches.tspackages/internals/persistence/src/services/persistence.tspackages/backend/src/services/notifications/service.tspackages/backend/test/e2e/fixtures.tspackages/backend/test/runtime.test.tspackages/backend/src/testing/minimal-persistence.ts
**/*.{test,spec}.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{test,spec}.{js,jsx,ts,tsx}: Write assertions insideit()ortest()blocks
Avoid done callbacks in async tests - use async/await instead
Don't use.onlyor.skipin committed code
Keep test suites reasonably flat - avoid excessivedescribenesting
Files:
packages/backend/test/services/notifications.service.test.tspackages/cli/test/e2e/commands.e2e.test.tspackages/backend/test/runtime.test.ts
🔇 Additional comments (14)
docs/reference/developer/cli.mdx (1)
55-55: LGTM!Also applies to: 87-94
packages/cli/src/commands/helpers.ts (1)
182-191: LGTM!Also applies to: 245-253
packages/backend/test/services/notifications.service.test.ts (1)
133-156: LGTM!Also applies to: 379-382
packages/outbound-resend/src/adapter.ts (1)
254-254: LGTM!packages/cli/test/e2e/commands.e2e.test.ts (1)
149-166: LGTM!packages/backend/src/audit/service.ts (1)
58-59: LGTM!Also applies to: 150-150
packages/backend/src/http-api/groups/webhooks.ts (1)
30-57: LGTM!Also applies to: 112-143
packages/backend/src/adapters/contract.ts (1)
140-155: LGTM!docs/reference/api/webhooks.mdx (1)
3-11: LGTM!Also applies to: 91-164
packages/internals/persistence/src/types/domain.ts (1)
736-756: LGTM!Also applies to: 1468-1507
packages/backend/src/routes/webhooks/dispatches.ts (1)
33-498: LGTM!packages/internals/persistence/src/services/persistence.ts (1)
998-1095: LGTM!packages/backend/test/e2e/fixtures.ts (1)
87-111: LGTM!Also applies to: 178-180, 429-463
packages/backend/test/runtime.test.ts (1)
151-168: LGTM!Also applies to: 520-787
There was a problem hiding this comment.
No new issues. Reviewed the following changes:
- Replaced the
as NotificationEventDraft["eventType"]cast inreplayWebhookDispatchwithparseNotificationEventType, a runtime guard against aNOTIFICATION_EVENT_TYPESconst list (as const satisfies readonly NotificationEventType[]); persisted events with unsupported event types now reject with a 400 carrying a specific message instead of being smuggled intotoDispatchInput. - Added
toReplayDispatchValidationErrorso the newRequestValidationErrorfromparseNotificationEventTypeis passed through unchanged rather than rewrapped as the generic "Failed to generate or dispatch notification event" error. - Made the in-memory
auditEvents.appendduplicate-id check atomic by collapsing the priorRef.get+some()+Ref.setpair into a singleRef.modifytransaction intesting/minimal-persistence.ts. - Updated the notifications-service test fixture event type from the non-existent
acknowledgement_duetoacknowledgement_sentso it survives the new guard, and added a service test asserting replay rejects unsupported event types without sending.
Claude Opus ｜ 𝕏
devcool20
left a comment
devcool20
left a comment
There was a problem hiding this comment.
Fixed all the Remaining issues, the code is clean now.
|
Maintainer review + update: strong original implementation — parameterized SQL only, tenant scoping via
Verified: full backend (106), cli (99), and persistence (23) suites pass; ultracite, tsdoc, error-docs, docs-lint, alpha-warning all clean. The async retry worker remains tracked in #5. Touches the same CLI parity files as #43 — whichever merges second needs a trivial rebase. |
There was a problem hiding this comment.
Important
The new dsar webhooks tail command can silently and permanently drop dispatches whenever a poll's matching backlog or burst exceeds --limit (default 200). The rest of the scope expansion — bulk replay, replay-all, docs, OpenAPI, tests — is sound; all five threads from the initial review are addressed and retired.
Reviewed changes — the "Maintainer Update" scope expansion since the prior pullfrog review (b72421a): bulk webhook-dispatch replay plus two new CLI commands.
- Add bulk replay
POST /webhooks/dispatches/replay— filter body (statusmust befailed,endpoint_id,created_after/created_before,limit1–100, empty body → 100 cap), idempotent per dispatch, continuing past per-dispatch delivery failures. Extracts a sharedreplayOneWebhookDispatchreused by the single-dispatch route. - Add
dsar webhooks replay-allCLI — wired through the parity map (webhooks_dispatches_replay_bulk), builds the bulk body from flags and requires--idempotency-key. - Add
dsar webhooks tailCLI — polling loop mirroringdsar audit tail(--once/--max-polls/--interval/--limit, SIGINT-aware). - Test + fixture updates —
fixtures.tsmakeMemoryPersistencegainedcount, filtering, sort and pagination; new bulk runtime tests, tenant-isolation probes,dsarminor changeset, and updated API/CLI docs + OpenAPI snapshot.
ℹ️ Bulk replay inherits single-replay fail-stop semantics at batch scale
The bulk endpoint reuses replayOneWebhookDispatch, which appends the webhook_dispatch_replay_requested marker before attempting the send. When a send fails for a dispatch — e.g. the webhook endpoint is temporarily misconfigured, or the persisted eventType is no longer supported — that dispatch is reported failed in results, but the marker is already committed.
- A retry of the same bulk request with the same
x-idempotency-keyreturnsalready_replayedfor that dispatch and never re-sends it. - Recovery requires issuing a fresh idempotency key.
This is the documented single-replay fail-stop behavior; the new surface simply applies it to up to 100 dispatches per call, so a single bulk request against a briefly-unhealthy endpoint can leave many dispatches durably marked already_replayed without delivery.
Technical details
# Bulk replay fail-stop poisoning at scale
## Affected sites
- `packages/backend/src/routes/webhooks/dispatches.ts` — `replayOneWebhookDispatch` appends the `webhook_dispatch_replay_requested` marker (deterministic id) before `replayWebhookDispatch`; the dedupe check keys off the *requested* marker, not a completed send.
- `packages/backend/src/routes/webhooks/dispatches.ts` — `bulkReplayWebhookDispatchesRoute` catches per-dispatch failures into `status: "failed"` but does not roll back the committed marker.
## Required outcome
- Confirm this is the intended contract for the bulk surface (it matches the documented single-replay behavior).
## Open questions for the human
- For bulk recovery specifically, is "retry with a new idempotency key after any per-dispatch failure" an acceptable operator workflow, or should a `failed` result leave the dispatch replayable under the same key? No code change requested — flagging for an explicit decision on the bulk surface.Claude Opus ｜ 𝕏
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes — the single follow-up commit 2ad354c ("Drain all dispatch pages per webhook tail poll"), which addresses the prior review's dsar webhooks tail silently dropped the oldest rows of any backlog/burst larger than --limit.
dsar webhooks tailnow drains every page per poll —pollOncewalks incrementingoffsetpages (via the extractedfetchDispatchPage) until a short page orPOLL_DRAIN_PAGE_CAP(50), collecting the full match set before advancing thecreated_afterwatermark. This is the same drain-with-cap shape the siblingdsar audit tailalready uses.- Cross-page duplicate handling — mid-drain inserts can repeat rows across pages;
runWebhookTailLoopdedupes by dispatch id via the ascending sort + monotonic watermark +seenAtWatermarkset, so no dispatch is emitted twice within a poll. - New burst test —
test/webhooks-tail.test.tsasserts that with--limit 2a 3-item burst walks offsets0and2and emits all three dispatches in creation order.
The drain, sort, and dedup logic all trace correctly, and the page cap mirrors the established audit tail bound (identical POLL_DRAIN_PAGE_CAP = 50), so the residual truncation only at >10k rows per poll is a deliberate, consistent design choice rather than a regression. Both prior pullfrog inline threads (the tail finding and the earlier audit-ordering thread) are already resolved.
Claude Opus ｜ 𝕏
|
Merge-sequencing note: #43 introduces per-command
I'll handle it during the rebase of whichever PR merges second. |
|
Done in 50b07af rather than deferring: |
There was a problem hiding this comment.
ℹ️ No critical issues — one minor suggestion inline.
Reviewed changes — the single follow-up commit 50b07af ("Declare command flag help for webhook dispatch commands"), which adds preformatted per-command flag help to the four webhook CLI commands.
- Add optional
flagHelpto command metadata —CommandDefinitionandRouteParityDefinitioninpackages/cli/src/types.tsgainflagHelp?: readonly string[], populated onwebhooks list/replay/replay-all(route-map.ts) andwebhooks tail(webhooks-tail.ts). - Wire route
flagHelpinto route commands —makeRouteCommandinfactory.tsspreadsroute.flagHelponto the generated command definition. - Add declaration test —
webhooks-tail.test.tsasserts each webhook command declares its expected flags.
ℹ️ Declared flagHelp is never rendered
The new flagHelp metadata is written on every webhook command but no code path reads it. renderHelp in packages/cli/src/runtime.ts prints only usage + description, and isHelpRequest short-circuits any --help to that global list before command resolution — so dsar webhooks list --help shows the global help, not the per-command flags.
The result is inert metadata whose doc comment advertises behavior the CLI doesn't have. The added test only asserts the field is declared, so it passes without exercising any rendering.
Technical details
# Declared `flagHelp` is never rendered
## Affected sites
- `packages/cli/src/types.ts:97-101` / `128-132` — doc comment states the lines are "rendered by `--help`".
- `packages/cli/src/runtime.ts:62-82` — `renderHelp` only emits `command.usage` + `command.description`; never reads `flagHelp`.
- `packages/cli/src/runtime.ts:84-85,124-127` — `isHelpRequest` matches any `--help`/`-h` in argv and returns the global help before a command is resolved, so there is no per-command help path.
- `packages/cli/test/webhooks-tail.test.ts` (flag-help block) — asserts declaration only, not rendering.
## Required outcome
- Either render `flagHelp` for the resolved command (per-command `--help`), or drop the field and the "rendered by `--help`" claim so the codebase carries no dead metadata.
## Open questions for the human
- Is per-command `--help` intended to land in a follow-up commit? If so, this is a harmless forward-declaration and can be left as-is with a note; if not, the metadata + doc comment are misleading today.Claude Opus ｜ 𝕏
| */ | ||
| export interface CommandDefinition { | ||
| /** | ||
| * Preformatted command-specific flag lines rendered by `--help`, e.g. |
There was a problem hiding this comment.
The doc comment claims these lines are "rendered by --help", but nothing consumes flagHelp. renderHelp (runtime.ts:62) prints only usage + description, and isHelpRequest short-circuits any --help to the global list before the command is resolved, so there is no per-command help path. Either wire flagHelp into the help renderer or drop the field and this claim.
2 participants
Summary
Closes #6.
This PR adds the first scoped recovery flow for outbound webhook dispatches:
GET /webhooks/dispatchesto list recorded outbound webhook delivery attempts.POST /webhooks/dispatches/:id/replayto replay one failed webhook dispatch.dsar webhooks list --status faileddsar webhooks replay <dispatch-id>Scope
This PR intentionally keeps the feature narrow and safe:
x-idempotency-keyso retrying the same replay request does not send the webhook twice.Maintainer Update
Follow-up commits on this PR complete the remaining issue #6 scope:
POST /webhooks/dispatches/replaybulk replay with filter body (status, endpoint_id, created_after/created_before, limit, capped at 100 per call), idempotent per dispatch, continuing past per-dispatch delivery failuresdsar webhooks replay-allCLI commanddsar webhooks tailpolling command (mirrorsdsar audit tail)dsar: minor)Out of scope (tracked separately): the async retry worker (#5) and replaying non-webhook email attempts.
Screenshots
Sequence Diagram
Commands showing the desired behaviour
Testing
Ran: