docs(code-review): record SMS-feature review findings + reconcile NotificationService doc
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.
This commit is contained in:
Joseph Doherty
@@ -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`.
|
||||
|
||||
Reference in New Issue
Block a user