refactor(alerts): add a notification dispatch seam

[jordan-simonovski]

Why

Alert notifications are delivered inline, on the evaluation tick. That is the right default, but it hardwires one delivery strategy into the render path: anything wanting to queue deliveries, bound concurrency, or survive across ticks has to edit renderAlertTemplate itself.

This adds the seam so delivery becomes swappable. No behaviour changes — the inline dispatcher is the default and does exactly what the code did before.

The contract worth reading

dispatch() has two legitimate implementations with different timing, and the difference matters to callers:

• inline resolves after delivery, so errors propagate to the caller and keep landing in executionErrors.
• queueing resolves after enqueue; its errors surface in its own logs and metrics, not to the caller.

Anything depending on synchronous error propagation from dispatch() is depending on the inline implementation specifically. That is documented on the interface.

Naming

NotificationJob.populatedChannel is deliberately not called channel. A job may carry both a serializable channel reference and the resolved document; reusing one name for both makes the two indistinguishable. Only the resolved form exists here today — the name is chosen so it stays correct when the other appears.

Not a behaviour change

checkAlerts.int.test.ts (277) and renderAlertTemplate.int.test.ts (74) pass unmodified — that is the gate. The dispatcher parameter is optional and defaults to inline, so no existing caller changed. eventId is byte-identical: same hash, same inputs, same place.

[changeset-bot]

⚠️ No Changeset found

Latest commit: 6a4464b

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Actions	Updated (UTC)
hyperdx-oss	Ready	Preview	Aug 17, 2026 4:01am
hyperdx-storybook	Ready	Preview	Aug 17, 2026 4:01am

[greptile-apps]

Greptile Summary

The PR extracts alert notification delivery behind an injectable dispatcher while retaining synchronous inline delivery as the default.

• Adds a notification job and dispatcher contract, including an inline implementation.
• Routes rendered alert notifications through the dispatcher without changing the transport payload or error propagation.
• Adds unit coverage for inline completion, rejection, and shutdown behavior.

Confidence Score: 5/5

The PR appears safe to merge with the existing inline notification behavior preserved.

The default dispatcher forwards the same populated channel, message, and group to the existing transport and continues awaiting delivery so failures propagate through the existing alert error path.

Important Files Changed

Filename	Overview
packages/api/src/tasks/checkAlerts/notifications.ts	Defines the notification job and dispatcher contracts and provides an inline implementation that preserves delivery timing and errors.
packages/api/src/tasks/checkAlerts/template.ts	Replaces direct transport invocation with an optional dispatcher while preserving the existing default payload and behavior.
packages/api/src/tasks/checkAlerts/tests/notifications.test.ts	Verifies inline dispatch waits for delivery, propagates failures, and has an immediate no-op shutdown.

Sequence Diagram

sequenceDiagram
  participant Eval as Alert evaluation
  participant Template as renderAlertTemplate
  participant Dispatcher as NotificationDispatcher
  participant Transport as deliverToChannel
  participant Webhook as Webhook
  Eval->>Template: Render alert notification
  Template->>Template: Build NotificationJob
  Template->>Dispatcher: await dispatch(job)
  Dispatcher->>Transport: deliverNotification(job)
  Transport->>Webhook: Send message
  Webhook-->>Transport: Delivery result
  Transport-->>Dispatcher: Resolve or reject
  Dispatcher-->>Template: Resolve or reject
  Template-->>Eval: Preserve inline result

Loading

_{Reviews (1): Last reviewed commit: "refactor(alerts): add a notification dis..." | Re-trigger Greptile}

[github-actions]

Deep Review

Refactor introducing a notification dispatch seam in the alerts task: a new NotificationDispatcher interface, NotificationJob type, and default InlineNotificationDispatcher, with renderAlertTemplate gaining an optional dispatcher parameter. Correctness and adversarial reviewers independently confirmed the inline path is behavior-preserving — arguments map 1:1 to the prior deliverToChannel call, eventId is computed from an unchanged objectHash input, and delivery rejections still propagate through the promised-handlebars helper into executionErrors.

✅ No critical issues found.

🟡 P2 -- recommended

• packages/api/src/tasks/checkAlerts/template.ts:309 -- The new injectable dispatcher parameter has no test that wires a custom dispatcher through renderAlertTemplate, so the swappability this change adds is unverified and no test pins the shape of the NotificationJob handed to dispatch().
  • Fix: Add a test that calls renderAlertTemplate with a fake NotificationDispatcher and asserts dispatch receives a job with the expected eventId, alertId, group, populatedChannel, and message fields.
  • _{testing, api-contract, kieran-typescript}

🔵 P3 nitpicks (4)

• packages/api/src/tasks/checkAlerts/notifications.ts:18 -- NotificationJob.teamId is declared optional but never populated at the sole construction site, leaving a field that always reads undefined.
  • Fix: Populate teamId from the caller's team scope in template.ts, or drop the field until a consumer reads it.
  • _{maintainability, kieran-typescript, correctness}
• packages/api/src/tasks/checkAlerts/notifications.ts:1 -- PopulatedAlertChannel is imported from @/tasks/checkAlerts/transports here while template.ts imports the same type from @/tasks/checkAlerts/providers, contradicting the re-export's stated single-source intent.
  • Fix: Standardize both sibling files on one canonical import path for the type.
  • _{maintainability, kieran-typescript}
• packages/api/src/tasks/checkAlerts/notifications.ts:63 -- shutdown(deadlineMs) is a no-op with an unused parameter and no caller in any process-lifecycle path, so a later queueing dispatcher could ship without a flush hook and silently drop buffered notifications on exit.
  • Fix: Either wire a module-level dispatcher.shutdown into the worker graceful-shutdown path, or drop the method until an implementation needs it.
  • _{maintainability, adversarial, reliability}
• packages/api/src/tasks/checkAlerts/notifications.ts:57 -- The dispatcher interface plus deliver-fn injection currently has one implementation and no overriding caller, adding indirection ahead of a consumer.
  • Fix: Consider collapsing to a direct deliverNotification call and reintroducing the interface when a second dispatcher actually lands.
  • _{maintainability}

Pre-existing (not counted toward verdict)

• packages/api/src/tasks/checkAlerts/template.ts:1 -- template.ts was already 540 lines before this change (over the project's 300-line guideline) and grows to ~548; the diff enlarges rather than causes the violation.
  • Fix: Split the notify-helper/delivery logic out of template.ts in a follow-up.

---

Reviewers (9): correctness, adversarial, testing, maintainability, project-standards, api-contract, reliability, kieran-typescript, agent-native.

Testing gaps:

• deliverNotification (the default deliver fn wired to deliverToChannel) has no direct unit test; coverage is only transitive through the existing integration suite.
• No regression test asserts that a dispatch() rejection still surfaces into executionErrors after the seam was introduced.
• No test pins the "eventId byte-identical" claim across the refactor (the objectHash input shape, including the grouped-alert branch).

Coverage: Two low-confidence single-reviewer findings were suppressed below the confidence gate — eventId being duplicated at the NotificationJob top level and inside message (currently set from one variable, so it cannot diverge today), and the test's opaque fakeJob: any fixture (project-standards flagged this as an accepted convention already pervasive in the directory). A residual note: no changeset was added for this packages/api change; the author should confirm it qualifies for the internal-refactor exemption.