dohertj2/ScadaBridge
1
0
Fork 0
Code Issues Pull Requests 1 Actions Packages Projects Releases Wiki Activity

refactor: rename ScadaLink → ZB.MOM.WW.ScadaBridge (code + projects + namespaces)

Browse Source 
Solution + 23 src projects + 26 test projects renamed; folders, csproj,
namespaces, and ScadaLinkDbContext/ScadaBridgeDbContext class updated.
ActorSystem "scadalink" → "scadabridge", Akka seed-node URLs migrated.
SQL roles/logins, LDAP domains, CLI command name, and CLI config dir
(~/.scadalink → ~/.scadabridge) also renamed.

Build green; 5 Host.Tests fail awaiting SQL login rename in next commit.
Pre-existing StaleTagMonitor timing flakes unchanged.

Rename script committed at tools/rename-to-scadabridge.sh.
This commit is contained in:
Joseph Doherty
2026-05-28 09:37:45 -04:00
parent 6d87ee3c3b
commit 7b0b9c7365
1531 changed files with 11180 additions and 11054 deletions
Download Patch File Download Diff File Expand all files Collapse all files
code-reviews/NotificationService/findings.md
+38 -38
View File
@@ -2,7 +2,7 @@
| Field | Value |
|-------|-------|
| Module | `src/ScadaLink.NotificationService` |
| Module | `src/ZB.MOM.WW.ScadaBridge.NotificationService` |
| Design doc | `docs/requirements/Component-NotificationService.md` |
| Status | Reviewed |
| Last reviewed | 2026-05-28 |
@@ -124,7 +124,7 @@ prefix could be aggressively scrubbed out of unrelated log text (NS-025).
| Severity | Critical |
| Category | Error handling & resilience |
| Status | Resolved |
| Location | `src/ScadaLink.NotificationService/NotificationDeliveryService.cs:96`, `src/ScadaLink.NotificationService/ServiceCollectionExtensions.cs:8` |
| Location | `src/ZB.MOM.WW.ScadaBridge.NotificationService/NotificationDeliveryService.cs:96`, `src/ZB.MOM.WW.ScadaBridge.NotificationService/ServiceCollectionExtensions.cs:8` |
**Description**
@@ -153,7 +153,7 @@ path. Fixed by the commit whose message references `NotificationService-001`.
| Severity | High |
| Category | Error handling & resilience |
| Status | Resolved |
| Location | `src/ScadaLink.NotificationService/NotificationDeliveryService.cs:157-167` |
| Location | `src/ZB.MOM.WW.ScadaBridge.NotificationService/NotificationDeliveryService.cs:157-167` |
**Description**
@@ -181,7 +181,7 @@ plus `SocketException`/`TimeoutException` are treated as transient. Regression t
| Severity | High |
| Category | Error handling & resilience |
| Status | Resolved |
| Location | `src/ScadaLink.NotificationService/NotificationDeliveryService.cs:144-147`, `src/ScadaLink.NotificationService/NotificationDeliveryService.cs:163-166` |
| Location | `src/ZB.MOM.WW.ScadaBridge.NotificationService/NotificationDeliveryService.cs:144-147`, `src/ZB.MOM.WW.ScadaBridge.NotificationService/NotificationDeliveryService.cs:163-166` |
**Description**
@@ -211,7 +211,7 @@ Regression tests `Send_Smtp5xxCommandException_ClassifiedPermanent`,
| Severity | High |
| Category | Performance & resource management |
| Status | Resolved |
| Location | `src/ScadaLink.NotificationService/NotificationDeliveryService.cs:118-119` |
| Location | `src/ZB.MOM.WW.ScadaBridge.NotificationService/NotificationDeliveryService.cs:118-119` |
**Description**
@@ -243,7 +243,7 @@ the resulting client is disposed.
| Severity | Medium |
| Category | Correctness & logic bugs |
| Status | Resolved |
| Location | `src/ScadaLink.NotificationService/MailKitSmtpClientWrapper.cs:18`, `src/ScadaLink.NotificationService/NotificationDeliveryService.cs:123` |
| Location | `src/ZB.MOM.WW.ScadaBridge.NotificationService/MailKitSmtpClientWrapper.cs:18`, `src/ZB.MOM.WW.ScadaBridge.NotificationService/NotificationDeliveryService.cs:123` |
**Description**
@@ -273,7 +273,7 @@ parks a buffered message) instead of silently negotiating TLS. Regression tests:
| Severity | Medium |
| Category | Concurrency & thread safety |
| Status | Resolved |
| Location | `src/ScadaLink.NotificationService/OAuth2TokenService.cs:14-15`, `src/ScadaLink.NotificationService/OAuth2TokenService.cs:30-35` |
| Location | `src/ZB.MOM.WW.ScadaBridge.NotificationService/OAuth2TokenService.cs:14-15`, `src/ZB.MOM.WW.ScadaBridge.NotificationService/OAuth2TokenService.cs:30-35` |
**Description**
@@ -301,7 +301,7 @@ and `GetTokenAsync_SameCredentials_CachedPerCredential`.
| Severity | Medium |
| Category | Design-document adherence |
| Status | Resolved |
| Location | `src/ScadaLink.NotificationService/NotificationOptions.cs:11-14`, `src/ScadaLink.NotificationService/MailKitSmtpClientWrapper.cs:16-20`, `src/ScadaLink.NotificationService/NotificationDeliveryService.cs:111-140` |
| Location | `src/ZB.MOM.WW.ScadaBridge.NotificationService/NotificationOptions.cs:11-14`, `src/ZB.MOM.WW.ScadaBridge.NotificationService/MailKitSmtpClientWrapper.cs:16-20`, `src/ZB.MOM.WW.ScadaBridge.NotificationService/NotificationDeliveryService.cs:111-140` |
**Description**
@@ -332,7 +332,7 @@ remain as operational fallback defaults. Regression tests:
| Severity | Medium |
| Category | Correctness & logic bugs |
| Status | Resolved |
| Location | `src/ScadaLink.NotificationService/NotificationDeliveryService.cs:136-137`, `src/ScadaLink.NotificationService/MailKitSmtpClientWrapper.cs:50-53` |
| Location | `src/ZB.MOM.WW.ScadaBridge.NotificationService/NotificationDeliveryService.cs:136-137`, `src/ZB.MOM.WW.ScadaBridge.NotificationService/MailKitSmtpClientWrapper.cs:50-53` |
**Description**
@@ -362,7 +362,7 @@ the Central UI is a separate component and out of this module's scope.
| Severity | Medium — re-triaged: split into an in-scope log-leak fix (resolved) and a Commons-scoped at-rest-encryption / structured-credential follow-up (NotificationService-013, Deferred). |
| Category | Security |
| Status | Resolved |
| Location | `src/ScadaLink.NotificationService/NotificationDeliveryService.cs:127-134`, `src/ScadaLink.NotificationService/OAuth2TokenService.cs:30-65`, `src/ScadaLink.Commons/Entities/Notifications/SmtpConfiguration.cs:9` |
| Location | `src/ZB.MOM.WW.ScadaBridge.NotificationService/NotificationDeliveryService.cs:127-134`, `src/ZB.MOM.WW.ScadaBridge.NotificationService/OAuth2TokenService.cs:30-65`, `src/ZB.MOM.WW.ScadaBridge.Commons/Entities/Notifications/SmtpConfiguration.cs:9` |
**Description**
@@ -389,7 +389,7 @@ conflates two concerns with different ownership.
2. **At-rest encryption + structured-credential modelling (out of scope — Deferred).**
Encrypting `SmtpConfiguration.Credentials` at rest and replacing the brittle
colon-packed `string` with structured fields requires editing
`src/ScadaLink.Commons/Entities/Notifications/SmtpConfiguration.cs` and the
`src/ZB.MOM.WW.ScadaBridge.Commons/Entities/Notifications/SmtpConfiguration.cs` and the
ConfigurationDatabase EF layer — both outside this module. Tracked separately as
**NotificationService-013** (Deferred) so it is not lost.
@@ -400,7 +400,7 @@ conflates two concerns with different ownership.
| Severity | Medium |
| Category | Security |
| Status | Deferred |
| Location | `src/ScadaLink.Commons/Entities/Notifications/SmtpConfiguration.cs:9`, ConfigurationDatabase EF mapping |
| Location | `src/ZB.MOM.WW.ScadaBridge.Commons/Entities/Notifications/SmtpConfiguration.cs:9`, ConfigurationDatabase EF mapping |
**Description**
@@ -419,7 +419,7 @@ fields directly.
**Resolution**
Deferred — requires changes to `src/ScadaLink.Commons` and the ConfigurationDatabase
Deferred — requires changes to `src/ZB.MOM.WW.ScadaBridge.Commons` and the ConfigurationDatabase
component, which are outside the NotificationService module. To be addressed in a
Commons/ConfigurationDatabase-scoped change. The associated log-leak risk is resolved
under NotificationService-009.
@@ -431,7 +431,7 @@ under NotificationService-009.
| Severity | Low |
| Category | Performance & resource management |
| Status | Resolved |
| Location | `src/ScadaLink.NotificationService/NotificationDeliveryService.cs:121-154` |
| Location | `src/ZB.MOM.WW.ScadaBridge.NotificationService/NotificationDeliveryService.cs:121-154` |
**Description**
@@ -461,7 +461,7 @@ mask the original delivery exception. Regression tests
| Severity | Low |
| Category | Code organization & conventions |
| Status | Resolved |
| Location | `src/ScadaLink.NotificationService/NotificationDeliveryService.cs:173-177`, `src/ScadaLink.Commons/Entities/Notifications/SmtpConfiguration.cs:5-15` |
| Location | `src/ZB.MOM.WW.ScadaBridge.NotificationService/NotificationDeliveryService.cs:173-177`, `src/ZB.MOM.WW.ScadaBridge.Commons/Entities/Notifications/SmtpConfiguration.cs:5-15` |
**Description**
@@ -477,11 +477,11 @@ Resolved 2026-05-16 (commit pending). Both issues confirmed against source.
1. **`SmtpPermanentException` placement (in scope — fixed).** The public exception type
was extracted from the bottom of `NotificationDeliveryService.cs` into its own file,
`src/ScadaLink.NotificationService/SmtpPermanentException.cs`, restoring the
`src/ZB.MOM.WW.ScadaBridge.NotificationService/SmtpPermanentException.cs`, restoring the
one-type-per-file layout. No behaviour change, so no dedicated regression test — the
move is verified by the module test suite still compiling and passing (56 tests green).
2. **`SmtpConfiguration` non-nullable strings (out of scope — re-triaged).**
`SmtpConfiguration` lives in `src/ScadaLink.Commons`, which is outside the
`SmtpConfiguration` lives in `src/ZB.MOM.WW.ScadaBridge.Commons`, which is outside the
NotificationService module's edit scope; it cannot be changed here. This is the same
Commons entity already flagged for follow-up under **NotificationService-013**
(Deferred). The `required`-members / documented-constructor change should be folded into
@@ -497,7 +497,7 @@ Resolved 2026-05-16 (commit pending). Both issues confirmed against source.
| Severity | Low |
| Category | Testing coverage |
| Status | Resolved |
| Location | `tests/ScadaLink.NotificationService.Tests/NotificationDeliveryServiceTests.cs`, `tests/ScadaLink.NotificationService.Tests/OAuth2TokenServiceTests.cs` |
| Location | `tests/ZB.MOM.WW.ScadaBridge.NotificationService.Tests/NotificationDeliveryServiceTests.cs`, `tests/ZB.MOM.WW.ScadaBridge.NotificationService.Tests/OAuth2TokenServiceTests.cs` |
**Description**
@@ -543,7 +543,7 @@ Module test suite is green at 56 tests.
| Severity | High |
| Category | Error handling & resilience |
| Status | Resolved |
| Location | `src/ScadaLink.NotificationService/NotificationDeliveryService.cs:214-228`, `src/ScadaLink.NotificationService/NotificationDeliveryService.cs:308-312`, `src/ScadaLink.NotificationService/OAuth2TokenService.cs:56-84` |
| Location | `src/ZB.MOM.WW.ScadaBridge.NotificationService/NotificationDeliveryService.cs:214-228`, `src/ZB.MOM.WW.ScadaBridge.NotificationService/NotificationDeliveryService.cs:308-312`, `src/ZB.MOM.WW.ScadaBridge.NotificationService/OAuth2TokenService.cs:56-84` |
**Description**
@@ -564,7 +564,7 @@ Resolved 2026-05-17. Root cause confirmed against source — `DeliverBufferedAsy
| Severity | High |
| Category | Error handling & resilience |
| Status | Resolved |
| Location | `src/ScadaLink.NotificationService/NotificationDeliveryService.cs:96-148`, `src/ScadaLink.NotificationService/NotificationDeliveryService.cs:308-312` |
| Location | `src/ZB.MOM.WW.ScadaBridge.NotificationService/NotificationDeliveryService.cs:96-148`, `src/ZB.MOM.WW.ScadaBridge.NotificationService/NotificationDeliveryService.cs:308-312` |
**Description**
@@ -585,7 +585,7 @@ Resolved 2026-05-17. Root cause confirmed — `SendAsync` had only three catch c
| Severity | Medium |
| Category | Security |
| Status | Resolved |
| Location | `src/ScadaLink.NotificationService/MailKitSmtpClientWrapper.cs:46-67` |
| Location | `src/ZB.MOM.WW.ScadaBridge.NotificationService/MailKitSmtpClientWrapper.cs:46-67` |
**Description**
@@ -606,11 +606,11 @@ Resolved 2026-05-17. Root cause confirmed — `AuthenticateAsync` returned silen
| Severity | Low |
| Category | Code organization & conventions |
| Status | Resolved |
| Location | `src/ScadaLink.NotificationService/NotificationOptions.cs:1-15`, `src/ScadaLink.NotificationService/ServiceCollectionExtensions.cs:10-11`, `src/ScadaLink.Host/SiteServiceRegistration.cs:70` |
| Location | `src/ZB.MOM.WW.ScadaBridge.NotificationService/NotificationOptions.cs:1-15`, `src/ZB.MOM.WW.ScadaBridge.NotificationService/ServiceCollectionExtensions.cs:10-11`, `src/ZB.MOM.WW.ScadaBridge.Host/SiteServiceRegistration.cs:70` |
**Description**
`NotificationOptions` (with `ConnectionTimeoutSeconds` and `MaxConcurrentConnections`) is bound from the `ScadaLink:Notification` configuration section in two places — `ServiceCollectionExtensions.AddNotificationService` (`AddOptions<NotificationOptions>().BindConfiguration(...)`) and again in `Host/SiteServiceRegistration.cs:70` (`services.Configure<NotificationOptions>(...)`). However, a repo-wide search shows no code ever injects `IOptions<NotificationOptions>` or otherwise reads either property. When NS-007 enforced the connection timeout and concurrency limit, it sourced both values from the per-site `SmtpConfiguration` entity, not from `NotificationOptions` — so this options class is now entirely dead configuration. Its XML doc still claims it "provides fallback defaults and operational limits", which is misleading: nothing falls back to it. The double binding is also redundant. Dead, falsely-documented configuration invites an operator to set `ScadaLink:Notification:ConnectionTimeoutSeconds` and expect it to take effect, when it has no effect at all.
`NotificationOptions` (with `ConnectionTimeoutSeconds` and `MaxConcurrentConnections`) is bound from the `ScadaBridge:Notification` configuration section in two places — `ServiceCollectionExtensions.AddNotificationService` (`AddOptions<NotificationOptions>().BindConfiguration(...)`) and again in `Host/SiteServiceRegistration.cs:70` (`services.Configure<NotificationOptions>(...)`). However, a repo-wide search shows no code ever injects `IOptions<NotificationOptions>` or otherwise reads either property. When NS-007 enforced the connection timeout and concurrency limit, it sourced both values from the per-site `SmtpConfiguration` entity, not from `NotificationOptions` — so this options class is now entirely dead configuration. Its XML doc still claims it "provides fallback defaults and operational limits", which is misleading: nothing falls back to it. The double binding is also redundant. Dead, falsely-documented configuration invites an operator to set `ScadaBridge:Notification:ConnectionTimeoutSeconds` and expect it to take effect, when it has no effect at all.
**Recommendation**
@@ -627,7 +627,7 @@ Resolved 2026-05-17. Root cause confirmed — `NotificationOptions` was bound bu
| Severity | Low |
| Category | Concurrency & thread safety |
| Status | Resolved |
| Location | `src/ScadaLink.NotificationService/NotificationDeliveryService.cs:237-255` |
| Location | `src/ZB.MOM.WW.ScadaBridge.NotificationService/NotificationDeliveryService.cs:237-255` |
**Description**
@@ -648,9 +648,9 @@ Resolved 2026-05-17. All three issues confirmed against source. The hand-rolled
| Severity | High |
| Category | Design-document adherence |
| Status | Resolved |
| Location | `src/ScadaLink.NotificationService/NotificationDeliveryService.cs:18-442`, `src/ScadaLink.NotificationService/ServiceCollectionExtensions.cs:20-21`, `src/ScadaLink.Commons/Interfaces/Services/INotificationDeliveryService.cs:1-33`, `src/ScadaLink.Host/Program.cs:77` |
| Location | `src/ZB.MOM.WW.ScadaBridge.NotificationService/NotificationDeliveryService.cs:18-442`, `src/ZB.MOM.WW.ScadaBridge.NotificationService/ServiceCollectionExtensions.cs:20-21`, `src/ZB.MOM.WW.ScadaBridge.Commons/Interfaces/Services/INotificationDeliveryService.cs:1-33`, `src/ZB.MOM.WW.ScadaBridge.Host/Program.cs:77` |
**Resolution** — Executed option 1. Deleted `src/ScadaLink.NotificationService/NotificationDeliveryService.cs`, `src/ScadaLink.Commons/Interfaces/Services/INotificationDeliveryService.cs` (also retires `NotificationResult` + `BufferedNotification`), and the orphaned `tests/ScadaLink.NotificationService.Tests/NotificationDeliveryServiceTests.cs` suite; reduced `AddNotificationService` to the shared SMTP primitives (`OAuth2TokenService`, `Func<ISmtpClientWrapper>`, `NotificationOptions`), updated `CompositionRootTests` (assert the primitives instead of the dead types), and removed the `Notification_Send_MockSmtp_Delivers` assertion in `IntegrationSurfaceTests` (central delivery is covered by `EmailNotificationDeliveryAdapterTests`). Grep-verified `grep -rn "INotificationDeliveryService\|NotificationDeliveryService\|NotificationResult\|BufferedNotification\|DeliverBufferedAsync" --include="*.cs" src/ tests/` before delete: zero production callers (only XML-doc cross-references in NS, MailKit wrapper, NotificationOptions and `EmailNotificationDeliveryAdapter`, plus the dead test files); cross-reference comments updated to remove the stale class references. `dotnet build ScadaLink.slnx` succeeds (0 warnings, 0 errors); affected test projects all pass (`NotificationService.Tests` 52/52, `NotificationOutbox.Tests` 86/86 on rerun — one flaky timing-sensitive Akka.TestKit test unrelated to NS-019, `Host.Tests` 205/205); `IntegrationTests` 64/66 with two pre-existing failures in `NotificationOutboxFlowTests` (SQLite "near IF: syntax error", reproducible on pristine `main`, unrelated to NS-019).
**Resolution** — Executed option 1. Deleted `src/ZB.MOM.WW.ScadaBridge.NotificationService/NotificationDeliveryService.cs`, `src/ZB.MOM.WW.ScadaBridge.Commons/Interfaces/Services/INotificationDeliveryService.cs` (also retires `NotificationResult` + `BufferedNotification`), and the orphaned `tests/ZB.MOM.WW.ScadaBridge.NotificationService.Tests/NotificationDeliveryServiceTests.cs` suite; reduced `AddNotificationService` to the shared SMTP primitives (`OAuth2TokenService`, `Func<ISmtpClientWrapper>`, `NotificationOptions`), updated `CompositionRootTests` (assert the primitives instead of the dead types), and removed the `Notification_Send_MockSmtp_Delivers` assertion in `IntegrationSurfaceTests` (central delivery is covered by `EmailNotificationDeliveryAdapterTests`). Grep-verified `grep -rn "INotificationDeliveryService\|NotificationDeliveryService\|NotificationResult\|BufferedNotification\|DeliverBufferedAsync" --include="*.cs" src/ tests/` before delete: zero production callers (only XML-doc cross-references in NS, MailKit wrapper, NotificationOptions and `EmailNotificationDeliveryAdapter`, plus the dead test files); cross-reference comments updated to remove the stale class references. `dotnet build ZB.MOM.WW.ScadaBridge.slnx` succeeds (0 warnings, 0 errors); affected test projects all pass (`NotificationService.Tests` 52/52, `NotificationOutbox.Tests` 86/86 on rerun — one flaky timing-sensitive Akka.TestKit test unrelated to NS-019, `Host.Tests` 205/205); `IntegrationTests` 64/66 with two pre-existing failures in `NotificationOutboxFlowTests` (SQLite "near IF: syntax error", reproducible on pristine `main`, unrelated to NS-019).
**Description**
@@ -660,8 +660,8 @@ The current source does not match. `NotificationDeliveryService` is a site-shape
Who actually calls it?
- **Sites** do **not**. `SiteServiceRegistration.cs:33-38` documents the deliberate omission: "AddNotificationService() is intentionally NOT registered on the site path." Sites register `NotificationForwarder` (in `ScadaLink.StoreAndForward`) as the S&F handler for `StoreAndForwardCategory.Notification` (`AkkaHostedService.cs:654-660`), which Asks the central comms actor and never touches SMTP. `ScriptRuntimeContext.NotifyHelper` (in `SiteRuntime`) enqueues directly to S&F as a serialized `NotificationSubmit`, **not** via `INotificationDeliveryService.SendAsync`.
- **Central** registers it (`Program.cs:77` calls `AddNotificationService`) but no central component resolves it. The central notification dispatcher is `NotificationOutboxActor` → `INotificationDeliveryAdapter` → `EmailNotificationDeliveryAdapter`. The adapter is a full re-implementation of the connect/auth/send/disconnect sequence (see `EmailNotificationDeliveryAdapter.cs:163-222`) — it deliberately does not call `NotificationDeliveryService.DeliverAsync` (XML-doc on the adapter says "Reuses the `ScadaLink.NotificationService` SMTP machinery — `ISmtpClientWrapper`, `SmtpTlsModeParser`, `OAuth2TokenService` and the typed `SmtpPermanentException`", i.e. only the leaf primitives).
- **Sites** do **not**. `SiteServiceRegistration.cs:33-38` documents the deliberate omission: "AddNotificationService() is intentionally NOT registered on the site path." Sites register `NotificationForwarder` (in `ZB.MOM.WW.ScadaBridge.StoreAndForward`) as the S&F handler for `StoreAndForwardCategory.Notification` (`AkkaHostedService.cs:654-660`), which Asks the central comms actor and never touches SMTP. `ScriptRuntimeContext.NotifyHelper` (in `SiteRuntime`) enqueues directly to S&F as a serialized `NotificationSubmit`, **not** via `INotificationDeliveryService.SendAsync`.
- **Central** registers it (`Program.cs:77` calls `AddNotificationService`) but no central component resolves it. The central notification dispatcher is `NotificationOutboxActor` → `INotificationDeliveryAdapter` → `EmailNotificationDeliveryAdapter`. The adapter is a full re-implementation of the connect/auth/send/disconnect sequence (see `EmailNotificationDeliveryAdapter.cs:163-222`) — it deliberately does not call `NotificationDeliveryService.DeliverAsync` (XML-doc on the adapter says "Reuses the `ZB.MOM.WW.ScadaBridge.NotificationService` SMTP machinery — `ISmtpClientWrapper`, `SmtpTlsModeParser`, `OAuth2TokenService` and the typed `SmtpPermanentException`", i.e. only the leaf primitives).
The `NotificationDeliveryService` class, its `DeliverBufferedAsync`, the `Func<ISmtpClientWrapper>` registration consumed only by it, and the `INotificationDeliveryService` interface (still in Commons) and `NotificationResult` record are therefore dead code that contradicts the design. Worse, every prior finding NS-001..NS-018 was reviewed and resolved against this dead path. The 56-test green test suite (NS-012 resolution note) exercises behaviour no production caller invokes — it gives a false sense of coverage. The misleading XML doc on `NotificationDeliveryService` ("WP-11/12: Notification delivery via SMTP") tells a maintainer this is *the* delivery path; the registration on central does the same.
@@ -671,7 +671,7 @@ Risk: an operator following the design doc will look here for "the central email
Decide and execute one of:
1. **Delete `NotificationDeliveryService`, `DeliverBufferedAsync`, the `BufferedNotification` payload type, the `Func<ISmtpClientWrapper>` scoped registration (move it to NotificationOutbox if still needed there — it already has its own), and `INotificationDeliveryService`/`NotificationResult` in Commons.** Reduce `AddNotificationService` to registering the shared primitives — `OAuth2TokenService`, `ISmtpClientWrapper` factory, `NotificationOptions`. Delete the NS-001..NS-018 tests that target the orphaned path; rebase the ones that exercise primitives (`SmtpErrorClassifier`, `SmtpTlsModeParser`, `CredentialRedactor`, `EmailAddressValidator`, `MailKitSmtpClientWrapper`, `OAuth2TokenService`) which remain genuinely shared. Update `CompositionRootTests` (`tests/ScadaLink.Host.Tests/CompositionRootTests.cs:208-209`) and `IntegrationSurfaceTests` (`tests/ScadaLink.IntegrationTests/IntegrationSurfaceTests.cs:122-135`) to drop the stale assertions.
1. **Delete `NotificationDeliveryService`, `DeliverBufferedAsync`, the `BufferedNotification` payload type, the `Func<ISmtpClientWrapper>` scoped registration (move it to NotificationOutbox if still needed there — it already has its own), and `INotificationDeliveryService`/`NotificationResult` in Commons.** Reduce `AddNotificationService` to registering the shared primitives — `OAuth2TokenService`, `ISmtpClientWrapper` factory, `NotificationOptions`. Delete the NS-001..NS-018 tests that target the orphaned path; rebase the ones that exercise primitives (`SmtpErrorClassifier`, `SmtpTlsModeParser`, `CredentialRedactor`, `EmailAddressValidator`, `MailKitSmtpClientWrapper`, `OAuth2TokenService`) which remain genuinely shared. Update `CompositionRootTests` (`tests/ZB.MOM.WW.ScadaBridge.Host.Tests/CompositionRootTests.cs:208-209`) and `IntegrationSurfaceTests` (`tests/ZB.MOM.WW.ScadaBridge.IntegrationTests/IntegrationSurfaceTests.cs:122-135`) to drop the stale assertions.
2. **Keep the class as the central-only Email delivery primitive** and rewrite `EmailNotificationDeliveryAdapter` to delegate to it. This is the smaller diff but the larger semantic burden — `NotificationDeliveryService.SendAsync` returns `NotificationResult` (Success / WasBuffered) which cannot encode the three-way `DeliveryOutcome` (Success / Transient / Permanent) the outbox needs, so the contract still has to change.
@@ -684,7 +684,7 @@ Recommended path is option 1: the parallel implementation in `EmailNotificationD
| Severity | Medium |
| Category | Correctness & logic bugs |
| Status | Resolved |
| Location | `src/ScadaLink.Host/Actors/AkkaHostedService.cs:654-660`, NS-001 resolution note (this file) |
| Location | `src/ZB.MOM.WW.ScadaBridge.Host/Actors/AkkaHostedService.cs:654-660`, NS-001 resolution note (this file) |
**Resolution (2026-05-28):** Added a `_notificationDeliveryHandlerRegistered` sentinel field on `AkkaHostedService` and gated the canonical `NotificationForwarder` registration with an `InvalidOperationException` guard — a future code path that re-introduces the dead NS-001 site-SMTP handler now fails fast at startup with an explicit NS-020 diagnostic, rather than silently overwriting `RegisterDeliveryHandler`'s last-write-wins map and inverting the central-only design. The sentinel's XML doc cross-references NS-001/NS-019/NS-020 so a maintainer searching for the `Notification` S&F handler finds the one canonical registration and its history.
@@ -707,7 +707,7 @@ Mark the NS-001 resolution note in this file as **superseded by NS-019** with a
| Severity | High |
| Category | Correctness & logic bugs |
| Status | Resolved |
| Location | `src/ScadaLink.NotificationService/MailKitSmtpClientWrapper.cs:76-79` |
| Location | `src/ZB.MOM.WW.ScadaBridge.NotificationService/MailKitSmtpClientWrapper.cs:76-79` |
**Description**
@@ -736,7 +736,7 @@ Pass the sender mailbox into the wrapper's `AuthenticateAsync` path. The cleanes
| Severity | Low |
| Category | Performance & resource management |
| Status | Resolved |
| Location | `src/ScadaLink.NotificationService/MailKitSmtpClientWrapper.cs:14`, `src/ScadaLink.NotificationService/ServiceCollectionExtensions.cs:19` |
| Location | `src/ZB.MOM.WW.ScadaBridge.NotificationService/MailKitSmtpClientWrapper.cs:14`, `src/ZB.MOM.WW.ScadaBridge.NotificationService/ServiceCollectionExtensions.cs:19` |
**Resolution (2026-05-28):** Took option (a) — updated the class-level XML doc on `MailKitSmtpClientWrapper` with an explicit lifetime section (NS-022) describing single-connection ownership and the per-delivery factory contract (NOT a pool; `MailKit.Net.Smtp.SmtpClient` holds one TCP/TLS connection and is not thread-safe; the DI `Func<ISmtpClientWrapper>` is a factory, callers run connect/auth/send/disconnect/dispose per send). A field-level comment was added to the `_client` declaration cross-referencing the class docs so a maintainer hunting from the field-initializer immediately sees the constraint. The wrapper code itself is unchanged — option (b) (transient SmtpClient per Send) was deliberately not taken because it would re-handshake TLS per email which is materially more expensive than the current per-delivery factory model the callers already implement. The concurrent-connection limit regression on the central-side delivery path (per-site semaphore on `EmailNotificationDeliveryAdapter`) is out of NotificationService scope and tracked separately.
@@ -761,7 +761,7 @@ Document the per-send lifecycle on `MailKitSmtpClientWrapper` (XML on the class:
| Severity | Low |
| Category | Documentation & comments |
| Status | Resolved |
| Location | `src/ScadaLink.NotificationService/NotificationDeliveryService.cs:12-17`, `src/ScadaLink.Commons/Interfaces/Services/INotificationDeliveryService.cs:3-12`, `src/ScadaLink.NotificationService/ServiceCollectionExtensions.cs:8-9` |
| Location | `src/ZB.MOM.WW.ScadaBridge.NotificationService/NotificationDeliveryService.cs:12-17`, `src/ZB.MOM.WW.ScadaBridge.Commons/Interfaces/Services/INotificationDeliveryService.cs:3-12`, `src/ZB.MOM.WW.ScadaBridge.NotificationService/ServiceCollectionExtensions.cs:8-9` |
**Resolution (2026-05-28):** Closed by NS-019 — both `NotificationDeliveryService.cs` and `INotificationDeliveryService.cs` were removed in commit `ac96b83`, and `ServiceCollectionExtensions.AddNotificationService`'s XML doc was rewritten in the same commit to describe the central-only design (shared SMTP primitives consumed by `EmailNotificationDeliveryAdapter`, with an explicit NS-019 cross-reference and a note that sites no longer deliver notifications). No stale XML docs remain in this module.
@@ -787,9 +787,9 @@ Tied to NS-019: if the orphan classes are deleted, this finding closes itself. I
| Severity | Medium |
| Category | Testing coverage |
| Status | Resolved |
| Location | `tests/ScadaLink.NotificationService.Tests/NotificationDeliveryServiceTests.cs`, `tests/ScadaLink.IntegrationTests/IntegrationSurfaceTests.cs:118-136`, `tests/ScadaLink.Host.Tests/CompositionRootTests.cs:207-209` |
| Location | `tests/ZB.MOM.WW.ScadaBridge.NotificationService.Tests/NotificationDeliveryServiceTests.cs`, `tests/ZB.MOM.WW.ScadaBridge.IntegrationTests/IntegrationSurfaceTests.cs:118-136`, `tests/ZB.MOM.WW.ScadaBridge.Host.Tests/CompositionRootTests.cs:207-209` |
**Resolution (2026-05-28):** Closed by NS-019 — the orphaned `NotificationDeliveryService` class, its `INotificationDeliveryService` Commons interface, and the associated `NotificationDeliveryServiceTests.cs` test file (~40 tests asserting `SendAsync`/`DeliverBufferedAsync` behaviour against a code path no production caller resolves) were all deleted in the NS-019 fix commit. Verification: a directory listing of `tests/ScadaLink.NotificationService.Tests/` shows only `CredentialRedactorTests.cs`, `MailKitSmtpClientWrapperTests.cs`, `NotificationOptionsTests.cs`, `OAuth2TokenServiceTests.cs`, `SmtpErrorClassifierTests.cs`, and `SmtpTlsModeParserTests.cs` — every retained file exercises a primitive that the central NotificationOutbox `EmailNotificationDeliveryAdapter` still depends on, so the false-coverage signal this finding called out no longer exists. The "no test affirms the central-only invariant" gap was the consequence of the orphaned tests existing; with them gone, the module test suite genuinely scopes to the shared SMTP primitives. The architecture-test recommendation (banning new consumers of `INotificationDeliveryService`) is moot once the interface itself is gone.
**Resolution (2026-05-28):** Closed by NS-019 — the orphaned `NotificationDeliveryService` class, its `INotificationDeliveryService` Commons interface, and the associated `NotificationDeliveryServiceTests.cs` test file (~40 tests asserting `SendAsync`/`DeliverBufferedAsync` behaviour against a code path no production caller resolves) were all deleted in the NS-019 fix commit. Verification: a directory listing of `tests/ZB.MOM.WW.ScadaBridge.NotificationService.Tests/` shows only `CredentialRedactorTests.cs`, `MailKitSmtpClientWrapperTests.cs`, `NotificationOptionsTests.cs`, `OAuth2TokenServiceTests.cs`, `SmtpErrorClassifierTests.cs`, and `SmtpTlsModeParserTests.cs` — every retained file exercises a primitive that the central NotificationOutbox `EmailNotificationDeliveryAdapter` still depends on, so the false-coverage signal this finding called out no longer exists. The "no test affirms the central-only invariant" gap was the consequence of the orphaned tests existing; with them gone, the module test suite genuinely scopes to the shared SMTP primitives. The architecture-test recommendation (banning new consumers of `INotificationDeliveryService`) is moot once the interface itself is gone.
**Description**
@@ -799,7 +799,7 @@ In particular there is **no test in this module** that affirms the central-only
- No test that `AddNotificationService()` registered on a *site* role would be inert / no-op'd, or that `SiteServiceRegistration.Configure` does **not** call `AddNotificationService` (an obvious regression vector — re-adding it would silently restore the orphaned site-delivery path).
- No test that confirms `INotificationDeliveryService` has no production consumer (i.e. an architecture test that fails if anyone re-introduces a constructor parameter or `GetRequiredService<INotificationDeliveryService>()` call).
- The cross-module `CompositionRootTests` (`tests/ScadaLink.Host.Tests/CompositionRootTests.cs:208-209`) still asserts `NotificationDeliveryService` and `INotificationDeliveryService` are registered, locking in the orphan rather than catching it.
- The cross-module `CompositionRootTests` (`tests/ZB.MOM.WW.ScadaBridge.Host.Tests/CompositionRootTests.cs:208-209`) still asserts `NotificationDeliveryService` and `INotificationDeliveryService` are registered, locking in the orphan rather than catching it.
- `IntegrationSurfaceTests.cs:122-125` constructs `NotificationDeliveryService` directly to validate "the integration surface" — testing a surface that no script actually crosses.
**Recommendation**
@@ -807,7 +807,7 @@ In particular there is **no test in this module** that affirms the central-only
After NS-019 is decided:
1. If the orphan is deleted, remove the orphaned-path tests (NS-001/004/005/007/008/009/010/014/015/016/017/018-style tests targeting `SendAsync`/`DeliverBufferedAsync`). Retain `SmtpErrorClassifierTests`, `SmtpTlsModeParserTests`, `CredentialRedactorTests`, `OAuth2TokenServiceTests`, and `MailKitSmtpClientWrapperTests` (primitives genuinely shared). Update `CompositionRootTests` to drop the stale rows and `IntegrationSurfaceTests` to call the live path via `INotificationDeliveryAdapter`/`EmailNotificationDeliveryAdapter`.
2. Add a one-shot architecture test in `tests/ScadaLink.Architecture.Tests` (if it exists, else this module) that scans for direct references to `INotificationDeliveryService` outside this project and the obsolete-interface declaration in Commons, failing if any new consumer reappears.
2. Add a one-shot architecture test in `tests/ZB.MOM.WW.ScadaBridge.Architecture.Tests` (if it exists, else this module) that scans for direct references to `INotificationDeliveryService` outside this project and the obsolete-interface declaration in Commons, failing if any new consumer reappears.
### NotificationService-025 — `CredentialRedactor` over-masks: any 4-character credential component is masked anywhere it appears, including unrelated log text
@@ -816,7 +816,7 @@ After NS-019 is decided:
| Severity | Low |
| Category | Correctness & logic bugs |
| Status | Resolved |
| Location | `src/ScadaLink.NotificationService/CredentialRedactor.cs:34-48` |
| Location | `src/ZB.MOM.WW.ScadaBridge.NotificationService/CredentialRedactor.cs:34-48` |
**Resolution (2026-05-28):** Tightened the policy per the recommendation —
only the LAST colon-separated component (password in Basic / clientSecret