From 4307c38117c0deb31e678f05d5d0c231bf6d7162 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Fri, 19 Jun 2026 15:30:32 -0400 Subject: [PATCH] docs(code-review): record SMS-feature review findings + reconcile NotificationService doc MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Per-module code review of the SMS notifications feature (reviewed at d6ead8ae) following code-reviews/REVIEW-PROCESS.md. 19 findings across 7 modules — 1 High, 5 Medium, 13 Low: - ManagementService-024 (High): provider-config updates Admin-gated to match the UI. - ConfigurationDatabase-025, CentralUI-034/035, ManagementService-025 (Medium): migration data-safety guard, type-aware recipient badge, FromNumber-optional (Messaging-Service-only), empty-token-clear guard. - Remaining Low: secret-encryption + diff + dispatch + factory + contract tests, truncation, ctor guard, reserved retry-field docs. - Won't Fix: Transport-015/016 (shared repo-wide import patterns, not SMS-specific), Commons-026 (breaking ergonomics-only change). Deferred: ConfigurationDatabase-027 (live-SQL migration test). All findings closed (0 pending). README.md regenerated; Component-NotificationService.md updated for the FromNumber-optional + reserved-retry-fields outcomes. --- code-reviews/CLI/findings.md | 37 +++++++- code-reviews/CentralUI/findings.md | 94 ++++++++++++++++++- code-reviews/Commons/findings.md | 88 ++++++++++++++++- .../ConfigurationDatabase/findings.md | 92 +++++++++++++++++- code-reviews/ManagementService/findings.md | 89 +++++++++++++++++- code-reviews/NotificationOutbox/findings.md | 90 +++++++++++++++++- code-reviews/README.md | 14 +-- code-reviews/Transport/findings.md | 93 +++++++++++++++++- .../Component-NotificationService.md | 8 +- 9 files changed, 579 insertions(+), 26 deletions(-) diff --git a/code-reviews/CLI/findings.md b/code-reviews/CLI/findings.md index e79da3cc..5d849c22 100644 --- a/code-reviews/CLI/findings.md +++ b/code-reviews/CLI/findings.md @@ -5,9 +5,9 @@ | Module | `src/ZB.MOM.WW.ScadaBridge.CLI` | | Design doc | `docs/requirements/Component-CLI.md` | | Status | Reviewed | -| Last reviewed | 2026-05-28 | +| Last reviewed | 2026-06-19 | | Reviewer | claude-agent | -| Commit reviewed | `1eb6e97` | +| Commit reviewed | `d6ead8ae` | | Open findings | 1 | ## Summary @@ -1082,3 +1082,36 @@ authoritative source. **Resolution** _Unresolved._ + +#### Re-review 2026-06-19 (commit `d6ead8ae`) — SMS notifications feature + +Per-module review of the CLI SMS surface: `--type email|sms` / `--phones` on `notification list`, +the `notification sms list|update` subcommands, and `bundle export --sms-configs`. Channel-aware +validation and the AuthToken-never-printed projection are correct and tested. One Low test-coverage +finding; Resolved. + +### CLI-024 — No contract test pinning `notification sms update` required fields + +| | | +|--|--| +| Severity | Low | +| Category | Testing coverage | +| Status | Resolved | +| Location | `tests/ZB.MOM.WW.ScadaBridge.CLI.Tests/UpdateCommandContractTests.cs` | + +**Description** + +`UpdateCommandContractTests` pins the `Required`/optional contract for every other update command's +core flags, but had no entry for `notification sms update` — a future regression making `--id` or +`--account-sid` optional (silently sending null) would go uncaught. + +**Recommendation** + +Add a contract test asserting the required/optional flags for `notification sms update`. + +**Resolution** + +Resolved 2026-06-19 (commit `a9393c89`): added `SmsUpdate_CoreFieldsRequired`. (Updated in commit +`33e1802e` when `--from-number` became conditionally validated rather than `Required` — see +CentralUI-035 — so the test now pins `--id`/`--account-sid` Required and `--from-number`/`--auth-token` +optional, with the either-or validation covered in `NotificationSmsCommandTests`.) diff --git a/code-reviews/CentralUI/findings.md b/code-reviews/CentralUI/findings.md index 83c4e713..76abd643 100644 --- a/code-reviews/CentralUI/findings.md +++ b/code-reviews/CentralUI/findings.md @@ -5,9 +5,9 @@ | Module | `src/ZB.MOM.WW.ScadaBridge.CentralUI` | | Design doc | `docs/requirements/Component-CentralUI.md` | | Status | Reviewed | -| Last reviewed | 2026-05-28 | +| Last reviewed | 2026-06-19 | | Reviewer | claude-agent | -| Commit reviewed | `1eb6e97` | +| Commit reviewed | `d6ead8ae` | | Open findings | 0 | ## Summary @@ -1595,3 +1595,93 @@ advances to Step 2 without opening a session; a wrong passphrase increments the counter and writes the `BundleImportUnlockFailed` audit row; the lockout resets the wizard to Step 1 once `MaxUnlockAttemptsPerSession` is reached; a successful unlock resets the counter and advances to Step 3. + +#### Re-review 2026-06-19 (commit `d6ead8ae`) — SMS notifications feature + +Per-module review of the CentralUI SMS surface: the `/notifications/sms` Admin-only config page, the +adapter-gated Type selector + per-type recipient input on the list form, the Type column, and the +Transport-export SMS-config selection. Security is solid — the page is `RequireAdmin`, the AuthToken is +never rendered (presence-only) and never pre-filled, preserve-if-blank is correct. Two Medium +correctness findings and one Low test gap; all Resolved. + +### CentralUI-034 — Notification Lists recipient badge renders "Name <>" for SMS recipients + +| | | +|--|--| +| Severity | Medium | +| Category | Correctness & logic bugs | +| Status | Resolved | +| Location | `src/ZB.MOM.WW.ScadaBridge.CentralUI/Components/Pages/Notifications/NotificationLists.razor:70` | + +**Description** + +The recipient badge rendered `@r.Name <@r.EmailAddress>` unconditionally. An SMS recipient carries a +PhoneNumber and a null EmailAddress, so an SMS list rendered "Jane <>" — an empty contact field. The +same commit added the Type column to this page but did not make the badge type-aware (the list form's +recipients table WAS made type-aware; only this summary page was missed). + +**Recommendation** + +Make the badge type-aware: show PhoneNumber for SMS lists, EmailAddress otherwise. + +**Resolution** + +Resolved 2026-06-19 (commit `cd8e4872`): the badge now selects the contact field by `list.Type` (phone +for SMS, email otherwise). Locked by `SmsListRecipientBadge_ShowsPhoneNumber_NotEmptyAngleBrackets` +(commit `a9393c89`). + +### CentralUI-035 — SMS config Save hard-required a From Number, contradicting the either-or contract + +| | | +|--|--| +| Severity | Medium | +| Category | Correctness & logic bugs | +| Status | Resolved | +| Location | `src/ZB.MOM.WW.ScadaBridge.CentralUI/Components/Pages/Notifications/SmsConfiguration.razor:219` | + +**Description** + +`Save()` required a From Number unconditionally, but the design doc and the Twilio delivery adapter +treat FromNumber and MessagingServiceSid as either-or (the adapter only errors when BOTH are blank, and +uses the Messaging Service SID instead of From when set). An admin could not create a Twilio +Messaging-Service-only configuration through the UI, despite the entity and adapter supporting it. +Investigation found the drift ran deeper than the UI: the entity ctor and the EF `FromNumber` column +were also non-null, so a pure UI fix was insufficient. + +**Recommendation** + +Reconcile the implementation with the spec — make FromNumber optional end-to-end (entity, schema, UI, +CLI) so a Messaging-Service-only config is valid, with at-least-one-of (FromNumber, MessagingServiceSid) +validated at the boundaries. + +**Resolution** + +Resolved 2026-06-19 (commit `33e1802e`): made FromNumber optional end-to-end — `SmsConfiguration.FromNumber` +→ nullable (ctor param optional), EF `IsRequired(false)` + migration `SmsFromNumberOptional` (ALTER +COLUMN nullable), `SmsConfigDto.FromNumber` → nullable, `UpdateSmsConfigCommand.FromNumber` → nullable, +UI validation now requires AccountSid + at-least-one-of(FromNumber, MessagingServiceSid), and CLI +`--from-number` is conditionally validated. Direction chosen by the design owner. + +### CentralUI-036 — No test for the From-Number/Messaging-Service-SID either-or rule + +| | | +|--|--| +| Severity | Low | +| Category | Testing coverage | +| Status | Resolved | +| Location | `tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Pages/SmsConfigurationPageTests.cs` | + +**Description** + +No test pinned the sender-identity validation, so the divergence in CentralUI-035 (UI stricter than the +adapter contract) was not caught. + +**Recommendation** + +Add a test that saving succeeds with a Messaging Service SID and no From Number. + +**Resolution** + +Resolved 2026-06-19 (commit `33e1802e`): added `SavingNewConfig_MessagingServiceSidOnly_NoFromNumber_Saves`, +which asserts the config persists with a null FromNumber. Complemented by ManagementActor + CLI +either-or tests in the same commit. diff --git a/code-reviews/Commons/findings.md b/code-reviews/Commons/findings.md index ba715008..4eb36196 100644 --- a/code-reviews/Commons/findings.md +++ b/code-reviews/Commons/findings.md @@ -5,9 +5,9 @@ | Module | `src/ZB.MOM.WW.ScadaBridge.Commons` | | Design doc | `docs/requirements/Component-Commons.md` | | Status | Reviewed | -| Last reviewed | 2026-05-28 | +| Last reviewed | 2026-06-19 | | Reviewer | claude-agent | -| Commit reviewed | `1eb6e97` | +| Commit reviewed | `d6ead8ae` | | Open findings | 2 | ## Summary @@ -1155,3 +1155,87 @@ fields on positional records MUST be added at the end of the parameter list AND carry a `= null` (or other safe) default value, so existing positional construction sites keep compiling. Apply that rule retroactively to `TrackingStatusSnapshot` and any other recent record that did not adopt it. No behavioral change required. + +#### Re-review 2026-06-19 (commit `d6ead8ae`) — SMS notifications feature + +Per-module review of the SMS (Twilio) notifications feature (#21 Notification Outbox / #8 +Notification Service slice). Three Low-severity findings in Commons; all closed. The new +`SmsConfiguration` POCO, `NotificationType.Sms`, the `NotificationRecipient` `ForEmail`/`ForSms` +factories, and the additive message-contract changes all follow the established conventions. + +### Commons-024 — `NotificationRecipient` public constructor name-guard weaker than the factories + +| | | +|--|--| +| Severity | Low | +| Category | Correctness & logic bugs | +| Status | Resolved | +| Location | `src/ZB.MOM.WW.ScadaBridge.Commons/Entities/Notifications/NotificationRecipient.cs:21-25` | + +**Description** + +The public `NotificationRecipient(string name, string emailAddress)` constructor validated `name` +with a null-only check (`name ?? throw`), while the `ForEmail`/`ForSms` factories reject +null-or-whitespace names. A caller using the public ctor directly could produce a recipient with a +whitespace name, an invariant the factories forbid. (EF materializes via the private parameterless +ctor + property injection, so this ctor is reached only by intentional email-path callers.) + +**Recommendation** + +Tighten the public ctor's name guard to `IsNullOrWhiteSpace` to match the factories. + +**Resolution** + +Resolved 2026-06-19 (commit `cd8e4872`): the public ctor now throws `ArgumentException` on a +null-or-whitespace name, matching `ForEmail`. Locked by `NotificationRecipientTests` (commit `a9393c89`). + +### Commons-025 — No unit tests for `NotificationRecipient.ForEmail`/`ForSms` factory invariants + +| | | +|--|--| +| Severity | Low | +| Category | Testing coverage | +| Status | Resolved | +| Location | `tests/ZB.MOM.WW.ScadaBridge.Commons.Tests/Entities/NotificationRecipientTests.cs` | + +**Description** + +The recipient construction invariants (email-only for `ForEmail`, phone-only for `ForSms`, +non-blank name on every path) were exercised only transitively via ManagementActor tests, with no +focused Commons-layer unit tests. + +**Recommendation** + +Add a `NotificationRecipientTests` covering both factories' happy paths, the null/whitespace guards, +and the contact-field exclusivity. + +**Resolution** + +Resolved 2026-06-19 (commit `a9393c89`): added `NotificationRecipientTests` (14 cases) covering +`ForEmail`/`ForSms` happy paths, contact-field exclusivity, null/whitespace name guards, and the +public-ctor guards. + +### Commons-026 — `Create/UpdateNotificationListCommand.RecipientEmails` non-nullable even for SMS lists + +| | | +|--|--| +| Severity | Low | +| Category | Code organization & conventions | +| Status | Won't Fix | +| Location | `src/ZB.MOM.WW.ScadaBridge.Commons/Messages/Management/NotificationCommands.cs:7-8` | + +**Description** + +`CreateNotificationListCommand`/`UpdateNotificationListCommand` carry `RecipientEmails` as a +required positional parameter even when `Type = Sms`, where it is semantically irrelevant (the +handler ignores it and reads `RecipientPhones`). This is an ergonomics wart, not a correctness bug. + +**Recommendation** + +Optionally default `RecipientEmails` to an empty list, or normalize null→empty in the handler. + +**Resolution** + +Won't Fix (2026-06-19): not a correctness defect — the handler already ignores `RecipientEmails` +for SMS lists. Changing a required positional parameter to defaulted/nullable is a breaking +contract change for existing callers for a pure-ergonomics gain; deferred indefinitely. diff --git a/code-reviews/ConfigurationDatabase/findings.md b/code-reviews/ConfigurationDatabase/findings.md index 0e6a1c6c..837941b2 100644 --- a/code-reviews/ConfigurationDatabase/findings.md +++ b/code-reviews/ConfigurationDatabase/findings.md @@ -5,9 +5,9 @@ | Module | `src/ZB.MOM.WW.ScadaBridge.ConfigurationDatabase` | | Design doc | `docs/requirements/Component-ConfigurationDatabase.md` | | Status | Reviewed | -| Last reviewed | 2026-05-28 | +| Last reviewed | 2026-06-19 | | Reviewer | claude-agent | -| Commit reviewed | `1eb6e97` | +| Commit reviewed | `d6ead8ae` | | Open findings | 0 | ## Summary @@ -1373,3 +1373,91 @@ later months. This pins down the resolution of CD-019. (2) Add a `DeploymentRecord`, clear the change tracker, call `DeleteDeploymentRecordAsync`, and assert the row is gone — pinning the resolution of CD-018. Both tests should be `[SkippableFact]` so the suite still passes when no MS SQL Server is available. + +#### Re-review 2026-06-19 (commit `d6ead8ae`) — SMS notifications feature + +Per-module review of the SMS (Twilio) notifications data layer: the `SmsConfiguration` EF mapping + +encrypted `AuthToken`, the `AddSmsNotifications` migration, and the `NotificationRecipient` nullable +contact change. The encryption wiring (`EncryptedStringConverter` on `AuthToken` via +`ApplySecretColumnEncryption` + `GuardSecretWritesHaveAKeyRing`) mirrors `SmtpConfiguration.Credentials` +and is correct. Two Medium findings (one data-safety, one test gap) and one Low test gap; the two +Mediums are Resolved. + +### ConfigurationDatabase-025 — `AddSmsNotifications.Down()` silently corrupts SMS-only recipients + +| | | +|--|--| +| Severity | Medium | +| Category | Correctness & logic bugs | +| Status | Resolved | +| Location | `src/ZB.MOM.WW.ScadaBridge.ConfigurationDatabase/Migrations/20260619135543_AddSmsNotifications.cs:52-66` | + +**Description** + +The `Down()` migration drops the `PhoneNumber` column and backfills every NULL `EmailAddress` to `''` +so the column can revert to NOT NULL. An SMS-only recipient (PhoneNumber set, EmailAddress +legitimately NULL) is therefore corrupted on rollback: its phone is dropped and its email becomes an +invalid empty string. In a database where SMS lists have been created, the rollback destroys contact data. + +**Recommendation** + +Before dropping `PhoneNumber`, guard the rollback: throw if any SMS-only recipient (PhoneNumber set, +EmailAddress NULL) exists, so the operator must reassign/remove them first instead of losing data silently. + +**Resolution** + +Resolved 2026-06-19 (commit `cd8e4872`): `Down()` now runs a `THROW`-on-`IF EXISTS` guard for +SMS-only recipients as its first statement (while `PhoneNumber` is still queryable), refusing the +rollback instead of corrupting data. Comments document the invariant. + +### ConfigurationDatabase-026 — No encryption-at-rest regression test for `SmsConfiguration.AuthToken` + +| | | +|--|--| +| Severity | Medium | +| Category | Testing coverage | +| Status | Resolved | +| Location | `tests/ZB.MOM.WW.ScadaBridge.ConfigurationDatabase.Tests/SecretEncryptionTests.cs` | + +**Description** + +`SecretEncryptionTests` covered the database-connection, SMTP-credentials, and external-system secrets +with "stored-encrypted / raw != plaintext / round-trips" tests, but had no parallel test for the +Twilio `AuthToken`. A refactor dropping `SmsConfiguration` from `ApplySecretColumnEncryption` would go +uncaught. + +**Recommendation** + +Add `SmsConfiguration_AuthToken_StoredEncrypted_RoundTrips` and a null-round-trip test mirroring the +SMTP shape. + +**Resolution** + +Resolved 2026-06-19 (commit `a9393c89`): added both tests; the encrypted test also asserts the raw +column is ciphertext and that `AccountSid` stays plaintext. + +### ConfigurationDatabase-027 — No MS SQL migration integration test for `AddSmsNotifications` + +| | | +|--|--| +| Severity | Low | +| Category | Testing coverage | +| Status | Deferred | +| Location | `tests/ZB.MOM.WW.ScadaBridge.ConfigurationDatabase.Tests/Migrations/` | + +**Description** + +Prior structural migrations have MS SQL integration tests asserting the resulting columns/types/nullability. +`AddSmsNotifications` (and the follow-up `SmsFromNumberOptional`) have none, so the column shapes and the +idempotency of the `ALTER COLUMN` statements are unverified against a real SQL Server. + +**Recommendation** + +Add an `AddSmsNotificationsMigrationTests` using the `MsSqlMigrationFixture` pattern. + +**Resolution** + +Deferred (2026-06-19): the migrations are standard idempotent `ALTER`/guarded-`CREATE` statements +verified by the model snapshot and the build, and `AddSmsNotifications` was applied to the real +docker MS SQL during the feature's integration pass. A live-SQL migration test is tracked for a +future ConfigurationDatabase test-stability pass. diff --git a/code-reviews/ManagementService/findings.md b/code-reviews/ManagementService/findings.md index cc850f87..15ad9c58 100644 --- a/code-reviews/ManagementService/findings.md +++ b/code-reviews/ManagementService/findings.md @@ -5,9 +5,9 @@ | Module | `src/ZB.MOM.WW.ScadaBridge.ManagementService` | | Design doc | `docs/requirements/Component-ManagementService.md` | | Status | Reviewed | -| Last reviewed | 2026-05-28 | +| Last reviewed | 2026-06-19 | | Reviewer | claude-agent | -| Commit reviewed | `1eb6e97` | +| Commit reviewed | `d6ead8ae` | | Open findings | 0 (1 Deferred — see ManagementService-012) | ## Summary @@ -1131,3 +1131,88 @@ entirely. Defer until a noticeable hot path emerges, but track it: this is the only N+1 in `ManagementActor` once 002 / 014 are folded in. + +#### Re-review 2026-06-19 (commit `d6ead8ae`) — SMS notifications feature + +Per-module review of the `ManagementActor` SMS surface: the SMS/SMTP provider-config command gating, +`HandleUpdateSmsConfig`, and the `SmsConfigPublicShape` secret projection. The secret-redaction chain +(`SmsConfigPublicShape` exposes `HasAuthToken` only; the AuthToken never reaches the response or audit +afterState) is correct and tested. One High authorization finding, one Medium secret-clear footgun, one +Low stale comment; all Resolved. + +### ManagementService-024 — SMS/SMTP provider-config updates were Designer-gated despite Admin-only UI + +| | | +|--|--| +| Severity | High | +| Category | Security | +| Status | Resolved | +| Location | `src/ZB.MOM.WW.ScadaBridge.ManagementService/ManagementActor.cs:189-190` | + +**Description** + +`UpdateSmsConfigCommand` (and the pre-existing `UpdateSmtpConfigCommand`) were placed in the Designer +arm of `GetRequiredRole`, but both the `/notifications/sms` and `/notifications/smtp` Central UI pages +enforce `RequireAdmin`. This split-brain let a Designer who is blocked in the UI still rotate a +production credential (the Twilio Auth Token / SMTP password) via the CLI / Management API. The design +(D8) specifies SMS config management as Admin-only. + +**Recommendation** + +Move `UpdateSmsConfigCommand` (and `UpdateSmtpConfigCommand`, whose UI page is also RequireAdmin) to the +Administrator arm so the actor gate matches the UI. + +**Resolution** + +Resolved 2026-06-19 (commit `cd8e4872`): both update commands moved to the Administrator arm. Locked by +`UpdateSmsConfig_WithDesignerRole_ReturnsUnauthorized` and updated success-case tests (commit `a9393c89`). + +### ManagementService-025 — Empty-string `--auth-token` silently clears the stored Twilio token + +| | | +|--|--| +| Severity | Medium | +| Category | Security | +| Status | Resolved | +| Location | `src/ZB.MOM.WW.ScadaBridge.ManagementService/ManagementActor.cs:1859` | + +**Description** + +`HandleUpdateSmsConfig` used `if (cmd.AuthToken is not null)` for preserve-if-omitted. An explicit empty +/ whitespace `--auth-token ""` passes the null check and overwrites the stored token with empty, silently +clearing the credential and 401-ing every subsequent send. A Twilio Auth Token is always required, so an +empty value is never a valid rotation. + +**Recommendation** + +Guard on `IsNullOrWhiteSpace` so an empty token is treated as omitted (preserve). (SMTP `Credentials` +keeps its null-only guard, since empty may be valid for anonymous SMTP.) + +**Resolution** + +Resolved 2026-06-19 (commit `cd8e4872`): the guard is now `!string.IsNullOrWhiteSpace(cmd.AuthToken)`. +Locked by `UpdateSmsConfig_WithEmptyAuthToken_PreservesExistingToken` (commit `a9393c89`). + +### ManagementService-026 — Stale "Admin-only" comment on `HandleListSmtpConfigs` + +| | | +|--|--| +| Severity | Low | +| Category | Documentation & comments | +| Status | Resolved | +| Location | `src/ZB.MOM.WW.ScadaBridge.ManagementService/ManagementActor.cs:1788-1790` | + +**Description** + +The comment on `HandleListSmtpConfigs` called the `UpdateSmtpConfig` path "Admin-only", which did not +match its (then) Designer gating; the SMS counterpart comment said "Designer-gated". The two were +inconsistent. + +**Recommendation** + +Make the comments accurate after fixing the gating. + +**Resolution** + +Resolved 2026-06-19 (commit `cd8e4872`): with both update commands moved to Administrator (MS-024), the +SMTP "Admin-only" comment is now accurate and the SMS list comment was updated to "Admin-only". diff --git a/code-reviews/NotificationOutbox/findings.md b/code-reviews/NotificationOutbox/findings.md index 0448914a..cabf5591 100644 --- a/code-reviews/NotificationOutbox/findings.md +++ b/code-reviews/NotificationOutbox/findings.md @@ -5,9 +5,9 @@ | Module | `src/ZB.MOM.WW.ScadaBridge.NotificationOutbox` | | Design doc | `docs/requirements/Component-NotificationOutbox.md` | | Status | Reviewed | -| Last reviewed | 2026-05-28 | +| Last reviewed | 2026-06-19 | | Reviewer | claude-agent | -| Commit reviewed | `1eb6e97` | +| Commit reviewed | `d6ead8ae` | | Open findings | 7 | ## Summary @@ -486,3 +486,89 @@ task-construction throw and is otherwise unreachable." **Resolution** _Unresolved._ + +#### Re-review 2026-06-19 (commit `d6ead8ae`) — SMS notifications feature + +Per-module review of the Twilio `SmsNotificationDeliveryAdapter`, `SmsErrorClassifier`, `SmsOptions`, +the DI registration, and the `NotificationOutboxActor` ingest type-stamping. This is the risk centre of +the feature; it is well-built. The Twilio request shape, the transient/permanent error classification, +the per-recipient rollup, and — critically — the AuthToken redaction on every error path +(`CredentialRedactor.Scrub`) are all correct and well-tested. Three Low findings; all closed. + +### NotificationOutbox-011 — `SmsConfiguration.MaxRetries`/`RetryDelay` are operator-settable but never honored + +| | | +|--|--| +| Severity | Low | +| Category | Design-document adherence | +| Status | Resolved | +| Location | `src/ZB.MOM.WW.ScadaBridge.NotificationOutbox/NotificationOutboxActor.cs:370-404` | + +**Description** + +The dispatcher's `ResolveRetryPolicyAsync` derives the retry policy (max-retries + interval) from the +central SMTP configuration for all notification types. `SmsConfiguration.MaxRetries`/`RetryDelay` are +operator-editable (management, UI, Transport) but never read at dispatch time — they are dead config, +which misleads an operator who sets them. (`ConnectionTimeoutSeconds`, by contrast, IS honored by the +adapter.) + +**Recommendation** + +Either honor the per-SMS values type-aware in `ResolveRetryPolicyAsync`, or document the fields as +reserved so operators are not misled. + +**Resolution** + +Resolved 2026-06-19 (commit `cd8e4872`): the fields are now documented as RESERVED on the entity +(XML doc) and in `Component-NotificationService.md` — the dispatcher reuses the shared SMTP-derived +retry policy per the documented "retry reuses central SMTP" decision. Honoring them per-type would +supersede that decision and is recorded as a deferred enhancement (not changed unilaterally for a Low finding). + +### NotificationOutbox-012 — SMS body truncation can split a surrogate pair at the cap boundary + +| | | +|--|--| +| Severity | Low | +| Category | Correctness & logic bugs | +| Status | Resolved | +| Location | `src/ZB.MOM.WW.ScadaBridge.NotificationOutbox/Delivery/SmsNotificationDeliveryAdapter.cs:359-367` | + +**Description** + +`ComposeBody` truncates by UTF-16 code-unit index. If `MaxMessageLength` falls on a surrogate-pair +boundary (e.g. an emoji in the alarm subject/body), the cut emits a lone surrogate — one malformed +character at the boundary. Cosmetic and only at the exact cap. + +**Recommendation** + +Back off one code unit if the cut would split a surrogate pair. + +**Resolution** + +Resolved 2026-06-19 (commit `cd8e4872`): the truncation now backs off by one code unit when the cut +index lands immediately after a high surrogate, keeping the body well-formed and within the cap. + +### NotificationOutbox-013 — No actor-level dispatch test for SMS-typed routing + +| | | +|--|--| +| Severity | Low | +| Category | Testing coverage | +| Status | Resolved | +| Location | `tests/ZB.MOM.WW.ScadaBridge.NotificationOutbox.Tests/NotificationOutboxActorDispatchTests.cs` | + +**Description** + +The adapter, ingest type-stamping, and DI registration were independently tested, but no actor-level +test exercised the Type-keyed adapter selection landing an SMS-typed notification on the SMS adapter +(vs the Email adapter). + +**Recommendation** + +Add a dispatch test that routes an SMS-typed notification through an SMS stub adapter and asserts the +outcome. + +**Resolution** + +Resolved 2026-06-19 (commit `a9393c89`): made the dispatch-test stub adapter Type-configurable and +added `Dispatch_SmsTypedNotification_RoutesToSmsAdapter_NotEmailAdapter`. diff --git a/code-reviews/README.md b/code-reviews/README.md index 9a60a7d6..fb22104b 100644 --- a/code-reviews/README.md +++ b/code-reviews/README.md @@ -50,20 +50,20 @@ module file and counted in **Total**. | Module | Last reviewed | Commit | Open (C/H/M/L) | Open | Total | |--------|---------------|--------|----------------|------|-------| | [AuditLog](AuditLog/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/0/0 | 0 | 11 | -| [CLI](CLI/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/0/0 | 0 | 23 | -| [CentralUI](CentralUI/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/0/0 | 0 | 33 | +| [CLI](CLI/findings.md) | 2026-06-19 | `d6ead8ae` | 0/0/0/0 | 0 | 24 | +| [CentralUI](CentralUI/findings.md) | 2026-06-19 | `d6ead8ae` | 0/0/0/0 | 0 | 36 | | [ClusterInfrastructure](ClusterInfrastructure/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/0/0 | 0 | 14 | -| [Commons](Commons/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/0/0 | 0 | 23 | +| [Commons](Commons/findings.md) | 2026-06-19 | `d6ead8ae` | 0/0/0/0 | 0 | 26 | | [Communication](Communication/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/0/0 | 0 | 22 | -| [ConfigurationDatabase](ConfigurationDatabase/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/0/0 | 0 | 24 | +| [ConfigurationDatabase](ConfigurationDatabase/findings.md) | 2026-06-19 | `d6ead8ae` | 0/0/0/0 | 0 | 27 | | [DataConnectionLayer](DataConnectionLayer/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/0/0 | 0 | 22 | | [DeploymentManager](DeploymentManager/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/0/0 | 0 | 24 | | [ExternalSystemGateway](ExternalSystemGateway/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/0/0 | 0 | 23 | | [HealthMonitoring](HealthMonitoring/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/0/0 | 0 | 23 | | [Host](Host/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/0/0 | 0 | 22 | | [InboundAPI](InboundAPI/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/0/0 | 0 | 25 | -| [ManagementService](ManagementService/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/0/0 | 0 | 23 | -| [NotificationOutbox](NotificationOutbox/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/0/0 | 0 | 10 | +| [ManagementService](ManagementService/findings.md) | 2026-06-19 | `d6ead8ae` | 0/0/0/0 | 0 | 26 | +| [NotificationOutbox](NotificationOutbox/findings.md) | 2026-06-19 | `d6ead8ae` | 0/0/0/0 | 0 | 13 | | [NotificationService](NotificationService/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/0/0 | 0 | 25 | | [Security](Security/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/0/0 | 0 | 21 | | [SiteCallAudit](SiteCallAudit/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/0/0 | 0 | 6 | @@ -71,7 +71,7 @@ module file and counted in **Total**. | [SiteRuntime](SiteRuntime/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/0/0 | 0 | 26 | | [StoreAndForward](StoreAndForward/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/0/0 | 0 | 24 | | [TemplateEngine](TemplateEngine/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/0/0 | 0 | 22 | -| [Transport](Transport/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/0/0 | 0 | 12 | +| [Transport](Transport/findings.md) | 2026-06-19 | `d6ead8ae` | 0/0/0/0 | 0 | 15 | ## Pending Findings diff --git a/code-reviews/Transport/findings.md b/code-reviews/Transport/findings.md index 9a4ed67f..4a5532af 100644 --- a/code-reviews/Transport/findings.md +++ b/code-reviews/Transport/findings.md @@ -5,10 +5,10 @@ | Module | `src/ZB.MOM.WW.ScadaBridge.Transport` | | Design doc | `docs/requirements/Component-Transport.md` | | Status | Reviewed | -| Last reviewed | 2026-05-28 | +| Last reviewed | 2026-06-19 | | Reviewer | claude-agent | -| Commit reviewed | `1eb6e97` | -| Open findings | 7 | +| Commit reviewed | `d6ead8ae` | +| Open findings | 0 | ## Summary @@ -527,3 +527,90 @@ but the Audit-Log-Viewer UI surface — the dropdown + `BundleImported` hyperlin is a deferred UI follow-up. Operators have a workaround via the existing `audit query --bundle-import-id` CLI flag. The UI work belongs in the CentralUI backlog; implementing it here would expand scope beyond a doc fix. + +#### Re-review 2026-06-19 (commit `d6ead8ae`) — SMS notifications feature + +Per-module review of the Transport SMS surface: `SmsConfigDto`, the entity serializer round-trip, +`ApplySmsConfigsAsync` (AccountSid-keyed upsert), and `CompareSmsConfiguration`. The secret isolation is +correct — the AuthToken rides only the encrypted `SecretsBlock`, never the plaintext DTO/diff, and the +diff is presence-only. Backward-compat with pre-SMS bundles holds (`SmsConfigs` defaults to empty). Two +findings carried over as Won't Fix (shared/inherited patterns), one Low test gap Resolved. + +### Transport-015 — `ApplySmsConfigsAsync` Rename branch falls back to the original key when `RenameTo` is null + +| | | +|--|--| +| Severity | Medium | +| Category | Correctness & logic bugs | +| Status | Won't Fix | +| Location | `src/ZB.MOM.WW.ScadaBridge.Transport/Import/BundleImporter.cs:2573` | + +**Description** + +The Rename branch uses `resolution.RenameTo ?? dto.AccountSid`; a null `RenameTo` on a Rename action +would add a config under the original key, risking a duplicate. Flagged Medium by the reviewer. + +**Recommendation** + +Guard the Rename branch against a null `RenameTo`. + +**Resolution** + +Won't Fix (2026-06-19): on investigation this is the established repo-wide import pattern — every entity +type's apply method uses the identical `resolution.RenameTo ?? ` fallback (templates, +folders, sites, data connections, instances, SMTP at line 2497, SMS at line 2573, ...). It is not +SMS-specific, the import-wizard / CLI map step supplies `RenameTo` for a Rename action (the `??` is +belt-and-suspenders), and the reviewer's proposed guard would merely relabel the duplicate as an Add. If +the null-`RenameTo` edge is a real concern it is a Transport-wide question outside the SMS review scope; +changing only the SMS branch would break the cross-entity consistency. + +### Transport-016 — New `SmsConfiguration` audit rows log entity id "0" + +| | | +|--|--| +| Severity | Low | +| Category | Correctness & logic bugs | +| Status | Won't Fix | +| Location | `src/ZB.MOM.WW.ScadaBridge.Transport/Import/BundleImporter.cs:2576,2594` | + +**Description** + +Audit rows for newly created `SmsConfiguration` entities log `"0"` as the entity id (the surrogate key +is unknown before EF assigns it), mirroring the SMTP path. Correlation is still possible by name +(AccountSid), so the id is misleading but not lost. + +**Recommendation** + +Read back the assigned id after save and use it in the audit `LogAsync` call. + +**Resolution** + +Won't Fix (2026-06-19): this is the established inherited pattern (SMTP at lines 2500/2518 does the same) +and is not a regression introduced by the SMS work. The audit row carries the AccountSid as +`entityName`, so the row is correlatable. A cross-entity cleanup to log real ids is out of the SMS +review scope. + +### Transport-017 — No unit tests for `CompareSmsConfiguration` + +| | | +|--|--| +| Severity | Low | +| Category | Testing coverage | +| Status | Resolved | +| Location | `tests/ZB.MOM.WW.ScadaBridge.Transport.Tests/Import/ArtifactDiffTests.cs` | + +**Description** + +`CompareSmsConfiguration` (the new diff method) had no dedicated unit tests; in particular the +secret presence-only invariant (the AuthToken value must never be echoed) relied on parallelism with the +SMTP/DataConnection patterns rather than a direct assertion. + +**Recommendation** + +Add unit tests for New/Identical/Modified plus the AuthToken presence-only invariant. + +**Resolution** + +Resolved 2026-06-19 (commit `a9393c89`): added five `CompareSmsConfiguration` tests — existing-null→New, +all-match→Identical, FromNumber-differs→Modified, and the two secret cases (value-changes-but-presence- +preserved → no echo; presence-flip → `` marker only, token never echoed). diff --git a/docs/requirements/Component-NotificationService.md b/docs/requirements/Component-NotificationService.md index 2b79c105..1dbf33ff 100644 --- a/docs/requirements/Component-NotificationService.md +++ b/docs/requirements/Component-NotificationService.md @@ -58,11 +58,11 @@ The `SmsConfiguration` entity is defined centrally and used by the central SMS d - **Account SID**: Twilio Account SID (plaintext; also appears in the API URL path). - **Auth Token**: Twilio Auth Token, **encrypted at rest** via ASP.NET Data Protection (`EncryptedStringConverter`). The Auth Token is never returned from the list command — the listing reports it only as a `hasAuthToken` presence flag. -- **From number**: Sender phone number in E.164 format (e.g., `+15551234567`). Used unless a Messaging Service SID is specified. -- **Messaging Service SID** (optional): Twilio Messaging Service SID. When present, Twilio uses it for sender selection; overrides the From number. +- **From number** (optional): Sender phone number in E.164 format (e.g., `+15551234567`). Used unless a Messaging Service SID is specified. A valid configuration carries a From number **and/or** a Messaging Service SID — at least one is required (validated in the Central UI, the CLI, and again by the delivery adapter). +- **Messaging Service SID** (optional): Twilio Messaging Service SID. When present, Twilio uses it for sender selection; used instead of the From number (so a Messaging-Service-only config needs no From number). - **API base URL** (optional): Override for the Twilio REST API base URL (default: `https://api.twilio.com`). Allows pointing at a test/stub handler or a regional endpoint. -- **Connection timeout**: Maximum time to wait for a Twilio API response. -- **Max retries** / **Retry delay**: Delivery retry settings, mirroring the SMTP retry model. +- **Connection timeout**: Maximum time to wait for a Twilio API response (honored per-send by the delivery adapter). +- **Max retries** / **Retry delay**: Reserved. The Notification Outbox dispatcher currently derives the retry policy (max-retries + interval) from the central SMTP configuration for all notification types; these per-SMS values are persisted and transported for forward-compatibility but are not yet read at dispatch time. Honoring them per-type is a deferred enhancement. The `SmsConfiguration` entity travels in Transport bundles — the Auth Token rides the encrypted `SecretsBlock` (keyed by Account SID), consistent with how SMTP credentials are bundled.