feat(services): migrate notification CRUD (PR 5/N)

[mxkaske]

Summary

Stacked on #2105. Fifth domain onto the service layer. Migrates the four CRUD procedures of the notification tRPC router — list, new, updateNotifier, delete — to @openstatus/services/notification. The three integration-helper procedures (sendTest, createTelegramToken, getTelegramUpdates) stay inline at the tRPC layer and are explicitly out of scope for this PR.

What lands

Services (packages/services/src/notification/)

• createNotification — three-layer plan gate: (1) channel count ≤ notification-channels, (2) plan flag enabled for gated providers (sms / pagerduty / opsgenie / grafana-oncall / whatsapp), (3) NotificationDataSchema validation of the channel payload. Also validates monitor ids are in-workspace + not soft-deleted.
• updateNotification — same validation rules as create; replaces name / data / monitor associations in a single transaction.
• deleteNotification — hard-delete. FK cascade takes care of notifications_to_monitors. The tRPC wrapper catches NotFoundError to preserve the pre-migration idempotent behaviour.
• listNotifications / getNotification — batched IN query enriches monitors per notification; monitor enrichment is workspace-scoped + filters soft-deleted monitors (applying the Cubic defence-in-depth lesson from PR 3/4 preemptively).
• All mutations inside withTransaction, emit emitAudit.

Surface migrated

packages/api/src/router/notification.ts — the four CRUD procedures become thin service wrappers.

Deliberately out of scope

• sendTest migration — the dispatch switch imports from 10 @openstatus/notification-* packages. Moving it into services means pulling those as direct deps; the plan's phrasing ("Channel CRUD + test-dispatch") leaves space to defer. Follow-up PR.
• createTelegramToken / getTelegramUpdates — redis + external Telegram API helpers. Transport UX, not domain operations; keeping them at the router stays aligned with the plan's carve-outs (like telegram-updates.ts).
• Biome scope for notification.ts — deferred. The file still imports @openstatus/db/src/schema for the sendTest provider data schemas, so turning the ban on now would flag those. The scope will land with the sendTest migration.
• Connect RPC notification handler — stays on its own helpers. Follow-up matching the Connect monitor deferral from PR 4.

Tests

__tests__/notification.test.ts:

• createNotification: happy path + audit, ValidationError on malformed data, LimitExceededError when the free plan blocks pagerduty, ForbiddenError on cross-workspace monitor attachment.
• updateNotification: association replacement (monitors: [] clears), cross-workspace NotFoundError.
• deleteNotification: row removed.
• list / get: enriched monitors visible to owner, invisible cross-workspace.

Test plan

• Service integration tests (seeded turso)
• Dashboard /notifications list + create + edit + delete flows
• trpc.notification.sendTest for each provider type still works (unchanged)

Stack

• feat(services): scaffold domain service layer (PR 0/N) #2100 scaffold → feat(services): migrate status-report domain (PR 1/N) #2101 status-report → feat(services): migrate maintenance domain (PR 2/N) #2103 maintenance → feat(services): migrate incident domain (PR 3/N) #2104 incident → feat(services): migrate monitor domain tRPC (PR 4/N) #2105 monitor (tRPC) → this PR notification (CRUD)

🤖 Generated with Claude Code

[vercel]

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

Project	Deployment	Actions	Updated (UTC)
openstatus-dashboard	Ready	Preview, Comment	Apr 24, 2026 9:05am
openstatus-status-page	Ready	Preview, Comment	Apr 24, 2026 9:05am
openstatus-web	Ready	Preview, Comment	Apr 24, 2026 9:05am

[cubic-dev-ai]

2 issues found across 10 files

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="packages/services/src/notification/update.ts">

<violation number="1" location="packages/services/src/notification/update.ts:32">
P1: Update flow is missing plan-gated provider validation, so restricted-provider notifications can still be edited after downgrade.</violation>
</file>

<file name="packages/services/src/notification/create.ts">

<violation number="1" location="packages/services/src/notification/create.ts:46">
P1: `data` is validated independently of `provider`, so mismatched provider/payload combinations can be created and stored.</violation>
</file>

_{Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.}

[mxkaske]

Review response (Cubic findings)

Both valid — fixed in 565e9b5a.

✅ 1. Update flow missing plan-gated provider validation

createNotification was calling assertProviderAllowed; updateNotification wasn't. A user who created a pagerduty channel on the team plan and then downgraded to free could still edit the config — the plan gate only fired on new creates. Added assertProviderAllowed(ctx.workspace, existing.provider) to the update flow (uses the stored provider since the API doesn't allow provider changes on update).

Matches the intended semantics: if your plan doesn't entitle you to the provider, you can't tweak it. The old tRPC had the same gap; this is a strict tightening.

✅ 2. data validated independently of provider

NotificationDataSchema is a z.union([...]), so { provider: "discord", data: { slack: "…" } } passed the top-level parse because the slack variant matched — even though the payload key doesn't match the declared provider. Input like this would round-trip to a record with mismatched metadata.

validateNotificationData now takes the provider as its first arg and asserts provider in data after the union parse. Every canonical data schema in this package uses the provider name as its required key (discord, slack, pagerduty, google-chat, etc.), so this single check is exhaustive. Applied in both create (with the input's provider) and update (with the stored existing.provider, since the update API doesn't support provider changes).

Added a test for the mismatch case: provider: "discord" with data: { slack: "…" } now rejects with ValidationError instead of silently passing.

---

Local typecheck + biome clean.

[cubic-dev-ai]

1 issue found across 4 files (changes from recent commits).

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="packages/services/src/notification/internal.ts">

<violation number="1" location="packages/services/src/notification/internal.ts:111">
P2: The new provider check validates only key presence, so invalid payload for the selected provider can pass when another provider field is valid.</violation>
</file>

_{Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.}

[mxkaske]

Follow-up on the last fix

Fair catch — fixed in `141be110`.

My previous attempt did NotificationDataSchema.safeParse(data) + provider in data. That misses the shape check on the provider's own payload: { discord: "not-a-url", slack: "valid-url" } parses against the union (the slack variant matches and silently ignores the extra discord key), then my key check sees "discord" and lets it through.

Replaced with a provider-specific schema lookup:

const providerDataSchemas = {
  discord: discordDataSchema,
  slack: slackDataSchema,
  pagerduty: pagerdutyDataSchema,
  // …all 12
} satisfies Record<NotificationProvider, ZodType>;

const parsed = providerDataSchemas[provider].safeParse(data);

Each canonical schema is keyed by its provider name and validates the shape + content of the value — one parse covers both the mismatched-provider case and the malformed-payload case. The satisfies Record<NotificationProvider, ZodType> clause keeps the map exhaustive at the type level; adding a new provider enum member becomes a compile error until the map gets its entry.

Added the exact case from the finding as a test — invalid discord URL alongside a valid slack URL now rejects with ValidationError instead of slipping through.

[mxkaske]

@claude review

[claude]

Claude finished @mxkaske's task in 1m 51s —— View job

---

Code Review

• Gather context
• Read changed files
• Review service layer (create, update, delete, list, internal)
• Review tRPC router changes
• Review test coverage
• Post review feedback

---

Overall this is a clean migration. The iterative Cubic review cycle worked well — the three-commit fix chain correctly tightened from "independent union parse + key-presence check" down to "provider-specific schema lookup, exhaustive at compile time." Nothing blocking. A few things worth noting below.

list.ts — redundant conditions array

// list.ts:89-90
const conditions: SQL[] = [eq(notification.workspaceId, ctx.workspace.id)];
const whereClause = and(...conditions);

The array is initialised with exactly one element and never pushed to. Unlike monitor/list.ts which has two real conditions (workspaceId + isNull(deletedAt)), notifications have no soft-delete, so the indirection adds noise without value. Consider collapsing to const whereClause = eq(notification.workspaceId, ctx.workspace.id) when convenient.

Fix this →

notification.ts — dataInputSchema duplicated from services

notification.ts:51-63 re-declares a dataInputSchema that is structurally identical to the dataSchema in packages/services/src/notification/schemas.ts. If the schemas drift (e.g. someone adds a new value type to the services schema without updating the router), inputs could be accepted at the tRPC layer but fail with an opaque error at the service layer. The services package already exports NotificationDataInput — the router could import and reuse it.

enrichNotificationsBatch — manual type cast is fragile

// list.ts:64-67
for (const row of assocRows as Array<{
  monitor: typeof monitor.$inferSelect;
  notifications_to_monitors: { notificationId: number };
}>) {

The cast relies on the internal Drizzle table alias matching the literal string "notifications_to_monitors". If the join table is aliased or renamed, monitors will silently disappear from the result (no TS error, no runtime error — just empty arrays). This pattern exists in monitor/list.ts too so it's consistent, but it's a latent footgun. A runtime shape-check or a snapshot test asserting that monitors is non-empty on a known fixture would catch a regression.

Missing test: update audit + post-downgrade provider gate

The updateNotification test suite covers name/data replacement and cross-workspace NotFoundError, but two things are untested:

1. Audit emission on update — createNotification tests expectAuditRow; updateNotification doesn't. The audit call is in the code, but it's the only mutation that's unverified by the test suite.
2. Plan gate on update (the Cubic-flagged fix) — assertProviderAllowed is now called in updateNotification using existing.provider, which is exactly what Cubic flagged. A test for the "pagerduty notification on a free plan → update blocked" path would give direct regression coverage for the fix.

Minor: update.ts audit record lacks provider metadata

// update.ts:69-75
await emitAudit(tx, ctx, {
  action: "notification.update",
  entityType: "notification",
  entityId: existing.id,
  before: existing,
  after: updated,
  // no metadata: { provider }
});

createNotification includes metadata: { provider: input.provider } on the audit record. updateNotification omits it. Since before/after rows include the provider field it's queryable, but the create/update audit records are inconsistent in their metadata.

schemas.ts — dataSchema type is intentionally wide

The UpdateNotificationInput doesn't carry provider, so dataSchema accepts Partial<Record<Provider, string | Record>> — any provider key can appear. Provider/payload alignment is enforced later by validateNotificationData(existing.provider, input.data) at the service boundary. This is correct by design, just noting that the Zod schema itself is not the enforcement point.

---

All three Cubic findings are addressed correctly. The satisfies Record<NotificationProvider, ZodType> exhaustiveness guard on providerDataSchemas is the right long-term approach — adding a new provider enum member will be a compile error until the map gets its entry.

[cubic-dev-ai]

12 issues found across 99 files (changes from recent commits).

Note: This PR contains a large number of files. cubic only reviews up to 75 files per PR, so some files may not have been reviewed. cubic prioritises the most important files to review.

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="packages/services/src/page-component/internal.ts">

<violation number="1" location="packages/services/src/page-component/internal.ts:39">
P2: Exclude soft-deleted monitors in `validateMonitorIds`; otherwise deleted monitor IDs are treated as valid and can still be attached to page components.</violation>
</file>

<file name="packages/services/src/import/phase-writers.ts">

<violation number="1" location="packages/services/src/import/phase-writers.ts:107">
P1: Missing component-link reconciliation for existing subscribers on re-run. Both `writeIncidentsPhase` and `writeMaintenancesPhase` reconcile their join-table links (with `onConflictDoNothing`) when the parent entity already exists, so a re-import repairs links that were incomplete on the first pass. `writeSubscribersPhase` skips this step, so `pageSubscriberToPageComponent` rows lost to a partial first run are never recovered.</violation>
</file>

<file name="packages/api/src/router/import.test.ts">

<violation number="1" location="packages/api/src/router/import.test.ts:424">
P2: These router-based limit tests never see the overridden workspace limits; tRPC replaces ctx.workspace from the DB, so they run against the seeded team-plan workspace instead of free/starter limits.</violation>
</file>

<file name="packages/api/src/router/user.ts">

<violation number="1" location="packages/api/src/router/user.ts:22">
P2: `deleteAccount` no longer preserves the previous `PRECONDITION_FAILED` error code for paid-workspace users; it now surfaces `FORBIDDEN` via generic service-error mapping.</violation>
</file>

<file name="packages/services/src/invitation/accept.ts">

<violation number="1" location="packages/services/src/invitation/accept.ts:75">
P2: Re-assert invitation expiry in the conditional UPDATE to prevent accepting just-expired invitations.</violation>

<violation number="2" location="packages/services/src/invitation/accept.ts:82">
P2: Handle duplicate workspace membership on accept to avoid unique-constraint failures for already-member invitees.</violation>
</file>

<file name="packages/services/src/page-component/list.ts">

<violation number="1" location="packages/services/src/page-component/list.ts:71">
P2: Filter out soft-deleted monitors during page-component enrichment to avoid returning deleted monitor relations.</violation>
</file>

<file name="packages/services/src/import/limits.ts">

<violation number="1" location="packages/services/src/import/limits.ts:33">
P2: Limit warnings ignore run import options, so users can get warnings for phases that are intentionally skipped (e.g., subscribers by default).</violation>
</file>

<file name="packages/services/src/page-component/update-order.ts">

<violation number="1" location="packages/services/src/page-component/update-order.ts:277">
P2: The static component update/insert branch uses a stale pre-delete ID set, so deleted IDs can take the update path and be silently lost instead of reinserted.</violation>
</file>

<file name="packages/services/src/page/schemas.ts">

<violation number="1" location="packages/services/src/page/schemas.ts:129">
P1: `updatePageAppearance` input accepts any theme string, which can persist invalid configuration and cause status page reads to fail validation.</violation>

<violation number="2" location="packages/services/src/page/schemas.ts:160">
P1: `updatePageConfiguration` input is too permissive and can store invalid configuration values that break status-page parsing.</violation>
</file>

<file name="packages/services/src/invitation/__tests__/invitation.test.ts">

<violation number="1" location="packages/services/src/invitation/__tests__/invitation.test.ts:86">
P3: Seed a member for the free workspace before asserting the members cap, otherwise this test won’t actually reach `LimitExceededError`.</violation>
</file>

_{Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.}

[cubic-dev-ai]

1 issue found across 22 files (changes from recent commits).

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="packages/services/src/invitation/accept.ts">

<violation number="1" location="packages/services/src/invitation/accept.ts:82">
P2: The new expiry predicate can make `claimed.length === 0` mean "expired during transaction", but the code still always throws "already accepted", returning the wrong error reason.</violation>
</file>

_{Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.}

[cubic-dev-ai]

P2: The new expiry predicate can make claimed.length === 0 mean "expired during transaction", but the code still always throws "already accepted", returning the wrong error reason.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At packages/services/src/invitation/accept.ts, line 82:

<comment>The new expiry predicate can make `claimed.length === 0` mean "expired during transaction", but the code still always throws "already accepted", returning the wrong error reason.</comment>

<file context>
@@ -66,24 +66,43 @@ export async function acceptInvitation(args: {
+        and(
+          eq(invitation.id, input.id),
+          isNull(invitation.acceptedAt),
+          gte(invitation.expiresAt, new Date()),
+        ),
+      )
</file context>