Feat/notification service by reecebedding · Pull Request #384 · compliance-framework/api

reecebedding · 2026-04-20T11:17:43Z

No description provided.

Copilot

Pull request overview

This PR introduces a unified notification runtime/service and migrates multiple worker-driven notifications (workflow tasks, risk digests/reminders, POAM notifications, evidence digests) to dispatch through that abstraction instead of bespoke per-channel logic.

Changes:

Added a new internal/service/notification package (runtime, registry, resolver, transport) plus email/slack provider adapters.
Refactored workflow/risk/POAM workers and schedulers to enqueue/dispatch “wrapper” jobs (often omitting explicit per-channel fanout) and delegate delivery selection to the notification service.
Added/updated Slack formatter implementations and expanded tests to cover multi-channel dispatch/enqueue behavior.

Reviewed changes

Copilot reviewed 74 out of 75 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
internal/service/worker/workflow_task_due_soon_worker_test.go	Updates worker construction/tests to use notification runtime provider.
internal/service/worker/workflow_task_digest_worker_test.go	Adjusts digest worker tests for new runtime/provider wiring and db-nil behavior.
internal/service/worker/workflow_task_digest_worker.go	Refactors digest worker to dispatch via notification service and slack provider types.
internal/service/worker/workflow_task_digest_checker.go	Changes digest checker to enqueue one job per subscribed user via notification repo.
internal/service/worker/workflow_task_assigned_worker_test.go	Updates assigned worker tests and adds enqueuer-based dispatch assertions.
internal/service/worker/workflow_notification_jobs_test.go	Updates job param tests and adds enqueuer-based due-soon/digest assertions.
internal/service/worker/workflow_notification_jobs.go	Introduces workflow notification job types + assigned/due-soon worker implementations using notification runtime provider.
internal/service/worker/workflow_execution_failed_worker_test.go	Expands failure worker tests for email+slack dispatch via notification service.
internal/service/worker/workflow_execution_failed_worker.go	Refactors failure worker to dispatch via notification service.
internal/service/worker/user_repository_test.go	Adds tests for worker GORM user repository Slack link behavior.
internal/service/worker/user_repository.go	Adjusts Slack link lookup to avoid ErrRecordNotFound as an error path.
internal/service/worker/risk_workers_test.go	Updates risk worker tests for channel-less wrapper jobs and new factory wiring.
internal/service/worker/risk_digest_worker_test.go	Updates risk digest scheduler/worker tests for wrapper job behavior + new dispatch path.
internal/service/worker/risk_digest_worker_integration_test.go	Updates integration test for wrapper job + new digest worker constructor.
internal/service/worker/risk_digest_worker.go	Refactors risk digest scheduling and worker dispatch to notification service factory.
internal/service/worker/poam_workers_integration_test.go	Updates POAM integration tests for notification service factory injection.
internal/service/worker/poam_overdue_transition_worker.go	Refactors POAM overdue notification worker to dispatch via notification service factory.
internal/service/worker/poam_overdue_notification_worker_test.go	Adds POAM overdue slack/email multi-channel worker tests.
internal/service/worker/poam_milestone_overdue_worker_test.go	Adds milestone overdue slack/email multi-channel worker tests.
internal/service/worker/poam_milestone_overdue_worker.go	Adds SSP display name resolution + refactors milestone overdue worker to notification dispatch.
internal/service/worker/poam_digest_worker.go	Refactors POAM digest worker to dispatch via notification service factory and typed model.
internal/service/worker/poam_deadline_reminder_worker_test.go	Adds POAM deadline reminder slack/email multi-channel worker tests.
internal/service/worker/poam_deadline_reminder_worker.go	Refactors POAM deadline reminder worker to dispatch via notification service factory.
internal/service/worker/notification_definition_helpers.go	Adds helpers to build typed notification definitions/requests and dispatch options.
internal/service/worker/notification_bridge.go	Bridges worker email/slack services + River enqueuer into notification transport/runtime.
internal/service/worker/due_soon_checker.go	Switches due-soon checker from per-channel enqueue to notification dispatch.
internal/service/slack/formatters/workflow_execution_failed_test.go	Adds tests for workflow execution failed Slack formatting.
internal/service/slack/formatters/workflow_execution_failed.go	Adds Slack formatter for workflow execution failed notification.
internal/service/slack/formatters/poam_overdue_notification_test.go	Adds tests for POAM overdue Slack formatting.
internal/service/slack/formatters/poam_overdue_notification.go	Adds Slack formatter for POAM overdue notification.
internal/service/slack/formatters/poam_milestone_overdue_reminder_test.go	Adds tests for POAM milestone overdue Slack formatting.
internal/service/slack/formatters/poam_milestone_overdue_reminder.go	Adds Slack formatter for POAM milestone overdue reminder.
internal/service/slack/formatters/poam_deadline_reminder_test.go	Adds tests for POAM deadline reminder Slack formatting.
internal/service/slack/formatters/poam_deadline_reminder.go	Adds Slack formatter for POAM deadline reminder.
internal/service/notification/types.go	Adds core notification types, validation, delivery/content/target structures.
internal/service/notification/transport_test.go	Adds transport tests covering worker enqueue vs direct send fallback.
internal/service/notification/transport.go	Adds delivery transport abstraction with provider registration and enqueue loop.
internal/service/notification/test_email_types_test.go	Adds test-only email content/delivery types for notification tests.
internal/service/notification/service.go	Adds notification service dispatch + fanout dispatch implementation.
internal/service/notification/runtime_provider.go	Adds runtime provider abstraction for scoped runtime factories.
internal/service/notification/runtime_factory.go	Adds runtime factory for building runtime/service with registered definitions.
internal/service/notification/runtime.go	Adds runtime container (registry/resolver/service).
internal/service/notification/resolver_test.go	Adds resolver tests for subscription gating, direct/configured audiences, ungated behavior.
internal/service/notification/renderer.go	Adds renderer wrapper ensuring provider/channel correctness.
internal/service/notification/registry_test.go	Adds registry tests for normalization and duplicate kind rejection.
internal/service/notification/registry.go	Adds registry implementation with normalization and concurrency protection.
internal/service/notification/providers/slack/provider.go	Adds Slack provider implementation (target resolution, enqueue/send).
internal/service/notification/providers/slack/helpers.go	Adds Slack provider helper constants + identity helpers.
internal/service/notification/providers/slack/formatters.go	Exposes Slack formatter adapters for notification provider usage.
internal/service/notification/providers/email/types.go	Adds email provider content/template/delivery types with validation/cloning.
internal/service/notification/providers/email/provider_test.go	Adds tests for email provider delivery and template rendering paths.
internal/service/notification/providers/email/provider.go	Adds email provider implementation (direct send, enqueue, template rendering).
internal/service/notification/providers/email/helpers.go	Adds email provider helper constants + identity helpers.
internal/service/notification/gorm_user_repository_test.go	Adds tests for notification GORM repo including Slack identity hydration.
internal/service/notification/definitions_test.go	Adds definition construction test for supported channels/renderers.
internal/service/notification/definitions.go	Adds notification definition model + normalization/validation.
internal/service/notification/constants.go	Adds ungated notification type constant + supported channel normalization rules.
internal/service/digest/service_test.go	Updates digest service test expectations to use notifier instead of slack service.
internal/service/digest/notifications.go	Introduces digest notification definitions + dispatch fanout using notifier/runtime provider.
internal/service/digest/email_renderer.go	Adds digest email template renderer compatible with notification email provider.
internal/api/handler/digest_integration_test.go	Updates digest API integration to build notifier/runtime provider.
cmd/run.go	Wires digest runtime provider + notifier and binds worker enqueuer after worker start.
cmd/riskdigest_direct.go	Updates direct risk digest command wiring to use notification service factory.
cmd/digest.go	Updates digest CLI wiring to create notifier/runtime provider before service.
.gitignore	Un-ignores `internal/service/notification/resolver.go` despite global `resolver.go` ignore.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-20T11:22:21Z

+	if args.AssignedToType == notification.DeliveryChannelEmail {
+		return w.dispatchToEmailAddress(ctx, args)
+	}


AssignedToType is a workflow assignment discriminator (workflows.AssignmentTypeEmail/User/Group), but this code compares it to notification.DeliveryChannelEmail. They currently share the string value "email", but coupling these concepts is brittle; use workflows.AssignmentTypeEmail.String() (and similarly elsewhere) to avoid accidental breakage if delivery channel IDs ever change.

Copilot · 2026-04-20T11:22:22Z

+		userName := ""
+		if w.userRepo != nil {
+			user, err := w.userRepo.FindUserByID(ctx, step.AssignedToID)
+			if err != nil {
+				w.logger.Warnw("DueSoonCheckerWorker: user not found, skipping",
+					"step_execution_id", step.ID.String(),
+					"user_id", step.AssignedToID,
+					"error", err,
+				)
+				continue
+			}
+			userName = user.FullName()
 		}
-	}

-	if len(params) == 0 {
-		return nil
-	}
-
-	if _, err := w.client.InsertMany(ctx, params); err != nil {
-		return fmt.Errorf("due-soon checker: failed to enqueue reminder jobs: %w", err)
+		if err := w.notifier.Dispatch(
+			ctx,
+			buildWorkflowTaskDueSoonNotificationRequest(baseArgs, userName, w.webBaseURL),
+		); err != nil {
+			return fmt.Errorf("due-soon checker: failed to dispatch reminder for step %s: %w", step.ID.String(), err)
+		}


DueSoonCheckerWorker now dispatches notifications synchronously per step, but it still does an explicit FindUserByID lookup to compute userName and then the notification resolver will call FindUserByID again to resolve the user audience. This creates duplicate DB lookups and can significantly increase runtime/DB load for large step sets. Consider caching the fetched user by seeding the notification user repository adapter with that user (or building the model from the notification.User inside dispatch), and/or batching enqueues instead of dispatching per-step to reduce InsertMany calls.

Copilot · 2026-04-20T11:22:22Z

 // DueSoonCheckerWorker scans for step executions due in a week and enqueues reminder notifications.
 type DueSoonCheckerWorker struct {


The type docstring says the checker "enqueues" reminder notifications, but Work now "dispatches" them directly via the notification service. Update the comment to match the new behavior (or revert to enqueue semantics if that was the intent).

Copilot

Pull request overview

Copilot reviewed 74 out of 75 changed files in this pull request and generated 2 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-20T11:41:48Z

@@ -15,21 +15,21 @@ import (

 // WorkflowExecutionFailedWorker sends a failure notification email to the workflow instance creator


The type doc comment still says this worker sends an "email", but the implementation now dispatches via the notification runtime (and tests cover Slack delivery as well). Update the comment to reflect that it sends a failure notification (email and/or Slack depending on available targets).

Copilot · 2026-04-20T11:41:49Z

 // PoamDeadlineReminderWorker sends a single POAM deadline approaching reminder
 // email to one recipient.
 type PoamDeadlineReminderWorker struct {


This doc comment still says the worker sends an "email" reminder, but the worker now dispatches through the notification service (potentially Slack as well). Please update the comment to avoid misleading future readers.

Copilot

Pull request overview

Copilot reviewed 75 out of 76 changed files in this pull request and generated 3 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-20T11:53:26Z

+		if err := notifier.Dispatch(
+			ctx,
+			buildWorkflowTaskDueSoonNotificationRequest(baseArgs, userName, w.webBaseURL),
+		); err != nil {
+			return fmt.Errorf("due-soon checker: failed to dispatch reminder for step %s: %w", step.ID.String(), err)
+		}


DueSoonCheckerWorker used to enqueue reminder jobs with River UniqueOpts (ByArgs + ByPeriod) to prevent duplicate reminders on repeated scans/retries. The new implementation dispatches notifications which enqueue SendEmail/SendSlack jobs without any uniqueness/idempotency, so rerunning the checker (or retrying after partial failure) can generate duplicate reminders. Consider restoring idempotency by setting UniqueOpts for notification delivery jobs (e.g., ByArgs=true and an appropriate ByPeriod) and/or incorporating the DispatchOptions.CorrelationID into River uniqueness for the enqueued deliveries.

Copilot · 2026-04-20T11:53:26Z

+func (e *workerNotificationEnqueuer) EnqueueNotificationEmail(ctx context.Context, delivery emailprovider.Delivery) error {
+	if e == nil || e.client == nil {
+		return fmt.Errorf("worker client is not initialized")
+	}
+
+	_, err := e.client.InsertMany(ctx, notificationEmailInsertParams(delivery, normalizedNotificationEmailQueue(e.emailQueue), e.maxAttempts))
+	if err != nil {
+		return fmt.Errorf("failed to enqueue notification email delivery: %w", err)
+	}
+
+	return nil
+}
+
+func (e *workerNotificationEnqueuer) EnqueueNotificationSlack(ctx context.Context, delivery slackprovider.Delivery) error {
+	if e == nil || e.client == nil {
+		return fmt.Errorf("worker client is not initialized")
+	}
+
+	params, err := notificationSlackInsertParams(delivery, defaultNotificationSlackQueue, e.maxAttempts)
+	if err != nil {
+		return err
+	}
+
+	_, err = e.client.InsertMany(ctx, params)
+	if err != nil {
+		return fmt.Errorf("failed to enqueue notification slack delivery: %w", err)
+	}


workerNotificationEnqueuer enqueues provider deliveries via notificationEmailInsertParams/notificationSlackInsertParams, which currently build River InsertOpts without UniqueOpts. This makes notification delivery jobs prone to duplication if the parent job is retried after enqueueing, or if the same notification is dispatched multiple times with the same CorrelationID. Suggest adding River uniqueness for these delivery jobs (e.g., based on NotificationKind+RecipientUserID+CorrelationID/Target) so delivery remains idempotent across retries.

Copilot · 2026-04-20T11:53:27Z

+
+	sender := p.sender()
+	if sender == nil || !sender.IsEnabled() {
+		return fmt.Errorf("email service is not enabled")


The email provider returns an error when no sender is configured/enabled ("email service is not enabled"), while the Slack provider treats a missing/disabled sender as a no-op. If email delivery is intended to be optional in some environments (e.g., dev/test/preview runs without an email backend), this will cause notification dispatch to fail rather than gracefully skipping. Consider aligning behavior (either both error for misconfig, or both no-op) and documenting the intended contract.

Suggested change

return fmt.Errorf("email service is not enabled")

return nil

Copilot

Pull request overview

Copilot reviewed 76 out of 77 changed files in this pull request and generated no new comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

gusfcarvalho · 2026-04-22T10:13:33Z

+	// Build digest notifier first; worker enqueuer is bound once worker starts.
+	var digestWorkerEnqueuer notification.WorkerEnqueuer
+	digestRuntimeProvider := digest.NewRuntimeProvider(
+		emailService,
+		cfg,
+		func() notification.WorkerEnqueuer { return digestWorkerEnqueuer },
+		func() slackprovider.Sender { return slackService },
+	)
+	digestNotifier := digest.NewNotificationService(
+		db,
+		cfg,
+		digestRuntimeProvider,
+	)


we would need to change the code here to add other channels right? 🤔

gusfcarvalho · 2026-04-22T10:16:32Z

+var supportedDeliveryChannels = map[string]struct{}{
+	DeliveryChannelEmail: {},
+	DeliveryChannelSlack: {},
+}
+


while this is a constant, might be good to separate this into a file so that its easier to add (notifications/delivery_channels.go perhaps? 🤔 )

gusfcarvalho · 2026-04-22T10:33:07Z

+	sspLookupItems := make([]poam.PoamItem, 0, len(rows))
+	seenSSPIDs := make(map[uuid.UUID]struct{}, len(rows))
+	for i := range rows {
+		if rows[i].ParentSspID == "" {
+			continue
+		}
+		sspID, err := uuid.Parse(rows[i].ParentSspID)
+		if err != nil {
+			continue
+		}
+		if _, ok := seenSSPIDs[sspID]; ok {
+			continue
+		}
+		seenSSPIDs[sspID] = struct{}{}
+		sspLookupItems = append(sspLookupItems, poam.PoamItem{SspID: sspID})
+	}
+
+	sspNames, err := resolvePoamSSPDisplayNames(ctx, w.db, sspLookupItems)
+	if err != nil {
+		w.logger.Warnw("MilestoneOverdueScannerWorker: failed to resolve SSP names", "error", err)
+		sspNames = map[string]string{}
+	}


was this a bug fix for something you found? seems unrelated with this PR

Notifications were showing UUIDs, using the lookup let us show friendly SSP Names

gusfcarvalho · 2026-04-22T10:37:03Z

-	err = r.db.WithContext(ctx).
+	result := r.db.WithContext(ctx).
 		Where("user_id = ?", user.ID.String()).
-		First(&slackLink).Error
-	if err == nil {
+		Limit(1).
+		Find(&slackLink)
+	if result.Error == nil && result.RowsAffected > 0 {
 		slackUserID = slackLink.SlackUserID
-	} else if !errors.Is(err, gorm.ErrRecordNotFound) {
-		return NotificationUser{}, fmt.Errorf("failed to fetch slack user link for user %s: %w", userID, err)
+	} else if result.Error != nil {
+		return NotificationUser{}, fmt.Errorf("failed to fetch slack user link for user %s: %w", userID, result.Error)


Bugfix for something you found?

Yea, if a user didn't have a linked slack account it would treat it as an error, now it is an optional record whilst still returning real DB errors.

gusfcarvalho · 2026-04-22T10:38:27Z


 local.go
 resolver.go
+!internal/service/notification/resolver.go


but why? :D

The resolver.go rule existed before, maybe for some tooling? I needed the notification resolver.go to be tracked. 😅

Copilot

Pull request overview

Copilot reviewed 78 out of 79 changed files in this pull request and generated 2 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

reecebedding added 16 commits April 15, 2026 10:51

feat: add new notification service

3c61fa0

feat: decouple slack with notification service

8361f6c

feat: migrate workflow digest checker

195e1b3

feat: migrate risk workers

835259c

feat: migreate poam digest

70ab3c4

feat: add email notification provider

46acfb4

fix: issue with slack id resolution

98fe433

chore: refactor

0e0dbd0

chore: consolodate notifications

170fa2d

chore: cleanup definition declaration

d5febdb

feat: migrate poam reminder notification

4415685

feat: migrate poam overdue notificiation

910c13e

feat: migrate poam milstone overdue notificiation

2269016

feat: migrate workflow exec fail notification

d804250

fix: slack is no longer a required channel

52d62bd

chore: simplification of notification surface

37c2abe

Copilot AI review requested due to automatic review settings April 20, 2026 11:17

Copilot started reviewing on behalf of reecebedding April 20, 2026 11:18 View session

Copilot AI reviewed Apr 20, 2026

View reviewed changes

reecebedding added 3 commits April 20, 2026 12:26

fix: lint issues

0442b60

Merge branch 'main' into feat/notification-service

70933ed

fix: copilot issues

96dcd22

Copilot AI review requested due to automatic review settings April 20, 2026 11:36

Copilot started reviewing on behalf of reecebedding April 20, 2026 11:37 View session

Copilot AI reviewed Apr 20, 2026

View reviewed changes

fix: copilot issues

dcff17c

reecebedding requested a review from Copilot April 20, 2026 11:48

Copilot started reviewing on behalf of reecebedding April 20, 2026 11:49 View session

Copilot AI reviewed Apr 20, 2026

View reviewed changes

fix: copilot issues

6ca5552

reecebedding requested a review from Copilot April 20, 2026 12:14

Copilot started reviewing on behalf of reecebedding April 20, 2026 12:15 View session

Copilot AI reviewed Apr 20, 2026

View reviewed changes

gusfcarvalho reviewed Apr 22, 2026

View reviewed changes

reecebedding added 2 commits April 22, 2026 14:25

fix: minor fixes from PR review

604c708

Merge branch 'main' into feat/notification-service

0a7d2e7

Copilot AI review requested due to automatic review settings April 22, 2026 15:41

Copilot started reviewing on behalf of reecebedding April 22, 2026 15:41 View session

Copilot AI reviewed Apr 22, 2026

View reviewed changes

Comment thread internal/service/notification/registry.go

Comment thread internal/service/worker/workflow_task_digest_checker.go Outdated

fix: copilot issues

5be2d96

reecebedding merged commit 2dd7669 into main Apr 24, 2026
4 checks passed

reecebedding deleted the feat/notification-service branch April 24, 2026 09:59

		// DueSoonCheckerWorker scans for step executions due in a week and enqueues reminder notifications.
		type DueSoonCheckerWorker struct {

		@@ -15,21 +15,21 @@ import (

		// WorkflowExecutionFailedWorker sends a failure notification email to the workflow instance creator

Conversation

reecebedding commented Apr 20, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Apr 20, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 20, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 20, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Apr 20, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 20, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Apr 20, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 20, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 20, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants