From 98c9458830853070e769708fbe11ddb84d4b145b Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Sat, 15 Aug 2026 21:16:55 -0400 Subject: [PATCH 1/5] =?UTF-8?q?test(transport):=20row=2074=20=E2=80=94=20g?= =?UTF-8?q?auge=20asserts=20become=20delta-over-scoped-action,=20not=20abs?= =?UTF-8?q?olute?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit BundleSessionObservabilityTests' two gauge facts asserted absolute values (Assert.Equal(0, baseline), Assert.Equal(2, ReadGauge())) against what is a single process-global provider slot — the same latent shape as the row-32 QueueDepthGaugeTests flake, correct only while this file is the slot's sole registrant. Converted to the delta-over-scoped-action pattern the row-66 rejection-counter facts in the same file already use: a new ReadGaugeDelta helper measures how far the instrument MOVED across an action driven by this test's own store, so an inherited non-zero baseline cancels out. Two assertions got stronger in the process. The post-clear check was Assert.Equal(0, ReadGauge()) — "the instrument reports zero", which any unregistered slot satisfies; it is now "opening more sessions on the detached store moves the gauge by nothing", which is the actual claim. And the compare-and-clear fact gained a negative control: the superseded store's opens must NOT reach the instrument, so a slot still pointing at oldStore can no longer satisfy the assertion by coincidence when both stores hold equal counts. Fail-before (temporary probe, not committed): with a foreign provider returning 7 registered before the service starts, the OLD body fails `Assert.Equal(0, baseline)` with `Expected: 0 / Actual: 7`; the NEW delta body passes the identical scenario. Honest residual documented on the helper: a gauge has one slot and last-writer-wins, so a registrant that clobbers it BETWEEN the two reads is unrecoverable read-side — that needs row-32's mutual exclusion. It cannot arise today (this is the only file in the assembly constructing a BundleSessionEvictionService, and test assemblies are separate processes); a second such file should bring the collection treatment with it. Transport.Tests: 180/180 green. --- .../Import/BundleSessionObservabilityTests.cs | 93 +++++++++++++++---- 1 file changed, 77 insertions(+), 16 deletions(-) diff --git a/tests/ZB.MOM.WW.ScadaBridge.Transport.Tests/Import/BundleSessionObservabilityTests.cs b/tests/ZB.MOM.WW.ScadaBridge.Transport.Tests/Import/BundleSessionObservabilityTests.cs index ea6d17e2..1c85e604 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.Transport.Tests/Import/BundleSessionObservabilityTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.Transport.Tests/Import/BundleSessionObservabilityTests.cs @@ -215,30 +215,39 @@ public sealed class BundleSessionObservabilityTests public async Task EvictionService_RegistersTheGaugeProvider_AndClearsItOnStop() { // The eviction service is Transport's only lifecycle seam, so it owns the - // process-global gauge slot. The gauge is read as a delta against a baseline - // because the instrument is a process-wide static. + // process-global gauge slot. Every assertion here is a DELTA over a scoped action + // against this test's own store (register row 74) — see ReadGaugeDelta. var clock = new TestClock(DateTimeOffset.UtcNow); var store = new BundleSessionStore(Options.Create(new TransportOptions()), clock, NullLogger.Instance); var service = new BundleSessionEvictionService(store, NullLogger.Instance); - var baseline = ReadGauge(); - await service.StartAsync(CancellationToken.None); try { - store.Open(SessionExpiringAt(clock.GetUtcNow().AddMinutes(30))); - store.Open(SessionExpiringAt(clock.GetUtcNow().AddMinutes(30))); + // While registered, this store's opens are what the gauge reports. + var whileRegistered = ReadGaugeDelta(() => + { + store.Open(SessionExpiringAt(clock.GetUtcNow().AddMinutes(30))); + store.Open(SessionExpiringAt(clock.GetUtcNow().AddMinutes(30))); + }); - Assert.Equal(2, ReadGauge()); + Assert.Equal(2, whileRegistered); } finally { await service.StopAsync(CancellationToken.None); } - // After the clear the gauge falls back to reporting 0, not through a dead store. - Assert.Equal(0, ReadGauge()); - Assert.Equal(0, baseline); + // After the clear the gauge no longer reads through this store: opening more + // sessions on it moves the instrument by nothing. Stated as a delta this is a + // STRONGER claim than the old `Assert.Equal(0, ReadGauge())` — that one merely + // said "the instrument reports zero", which any unregistered slot satisfies, + // and which a foreign registrant would have failed for reasons unrelated to + // the clear. + var afterClear = ReadGaugeDelta(() => + store.Open(SessionExpiringAt(clock.GetUtcNow().AddMinutes(30)))); + + Assert.Equal(0, afterClear); } [Fact] @@ -257,29 +266,81 @@ public sealed class BundleSessionObservabilityTests await newService.StartAsync(CancellationToken.None); try { - newStore.Open(SessionExpiringAt(clock.GetUtcNow().AddMinutes(30))); - - // Late stop of the superseded instance. + // Late stop of the superseded instance, then prove the NEWER store is still + // the one driving the gauge — by moving it and measuring the movement. await oldService.StopAsync(CancellationToken.None); - Assert.Equal(1, ReadGauge()); + var newStoreStillDrivesIt = ReadGaugeDelta(() => + newStore.Open(SessionExpiringAt(clock.GetUtcNow().AddMinutes(30)))); + + Assert.Equal(1, newStoreStillDrivesIt); + + // Negative control on the same window: the superseded store is detached, so + // its opens must not reach the instrument. Without this, a provider slot left + // pointing at oldStore would still satisfy the assertion above by coincidence + // whenever both stores happened to hold equal counts. + var oldStoreIsDetached = ReadGaugeDelta(() => + oldStore.Open(SessionExpiringAt(clock.GetUtcNow().AddMinutes(30)))); + + Assert.Equal(0, oldStoreIsDetached); } finally { await newService.StopAsync(CancellationToken.None); } - Assert.Equal(0, ReadGauge()); + var afterClear = ReadGaugeDelta(() => + newStore.Open(SessionExpiringAt(clock.GetUtcNow().AddMinutes(30)))); + + Assert.Equal(0, afterClear); } // ------------------------------------------------------------------ // Helpers // ------------------------------------------------------------------ + /// + /// Runs between two observations of the open-import-sessions + /// gauge and returns how far the instrument MOVED across it. + /// + /// + /// + /// Register row 74. These tests used to assert absolute gauge values — + /// Assert.Equal(0, baseline), Assert.Equal(2, ReadGauge()) — against + /// what is a single process-global provider slot + /// (SetImportSessionCountProvider / ClearImportSessionCountProvider). + /// That is the same shape as the register row 32 flake in + /// QueueDepthGaugeTests: correct only for as long as this file is the slot's + /// sole registrant, and silently wrong the day a sibling suite writes it. A delta + /// carries no assumption about what the slot held on entry, so a non-zero inherited + /// baseline cancels out. + /// + /// + /// Honest residual, and the reason this is a delta rather than a full fix: unlike + /// — where every measurement in the window is + /// driven synchronously by this test's own Open call, making it genuinely + /// immune — a gauge has ONE slot and last-writer-wins. If a concurrent registrant + /// overwrote the slot BETWEEN the two reads, no read-side technique could recover; + /// that case needs mutual exclusion on the slot (row 32's serialized collection). + /// It cannot arise today: this is the only file in the assembly that constructs a + /// BundleSessionEvictionService, and test assemblies run in separate + /// processes. A second such file should bring the collection treatment with it. + /// + /// + /// The action to measure across. + /// The gauge's movement while ran. + private static long ReadGaugeDelta(Action act) + { + var before = ReadGauge(); + act(); + return ReadGauge() - before; + } + /// /// Forces one synchronous observation of the open-import-sessions gauge and returns /// the measured value. Mirrors the MeterListener helper used by the Communication - /// and StoreAndForward telemetry suites. + /// and StoreAndForward telemetry suites. Private to by + /// convention — see its remarks for why no test reads this absolutely. /// /// The gauge's current value, or 0 if it never reported. private static long ReadGauge() From 4ae57bdb6b48091abd97af7499e295b4592df6b9 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Sat, 15 Aug 2026 21:17:34 -0400 Subject: [PATCH 2/5] =?UTF-8?q?docs(cli):=20row=2076=20=E2=80=94=20audit-c?= =?UTF-8?q?onfig=20query=20JSON=20sample=20shows=20afterStateJson=20+=20bu?= =?UTF-8?q?ndleImportId?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The `entries` array is a serialized `AuditLogEntry`, so both fields are always on the wire; the README sample showed neither, which read as if the payload were the seven columns the table projection happens to keep. Row 57 added that projection and the table section already names both omitted fields — the JSON sample above it was the stale half. Added both to the sample and documented their nullability, which is the part a script author actually needs: `afterStateJson` is the post-change snapshot embedded as a string and is null for deletes (no after-state), `bundleImportId` is the import session id when the change came from `transport import` and null for a direct change. Also grew the sample from one entry to two so it now mirrors the table-output sample directly below it row for row — the same 412 Template/Created and 413 ExternalSystem/Deleted pair — which is what makes the "the table drops these two fields" sentence checkable against the JSON above it. Docs only. --- src/ZB.MOM.WW.ScadaBridge.CLI/README.md | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/src/ZB.MOM.WW.ScadaBridge.CLI/README.md b/src/ZB.MOM.WW.ScadaBridge.CLI/README.md index 5e9a4e38..0555e0dc 100644 --- a/src/ZB.MOM.WW.ScadaBridge.CLI/README.md +++ b/src/ZB.MOM.WW.ScadaBridge.CLI/README.md @@ -1829,13 +1829,32 @@ Output shape: "entityType": "Template", "entityId": "7", "entityName": "PumpTemplate", - "timestamp": "2026-08-15T12:00:00+00:00" + "afterStateJson": "{\"Id\":7,\"Name\":\"PumpTemplate\",\"FolderId\":3}", + "timestamp": "2026-08-15T12:00:00+00:00", + "bundleImportId": null + }, + { + "id": 413, + "user": "designer", + "action": "Deleted", + "entityType": "ExternalSystem", + "entityId": "3", + "entityName": "MES", + "afterStateJson": null, + "timestamp": "2026-08-15T12:05:00+00:00", + "bundleImportId": "9f2c1e40-5b31-4a7e-8d66-0c1a4b93ff21" } ], "totalCount": 137 } ``` +Both nullable fields are always present on the wire. `afterStateJson` is a JSON snapshot +of the entity's state *after* the change, embedded as a string — it is `null` for deletes, +since there is no after-state. `bundleImportId` is the import session id when the change +was made by a bundle import (`transport import`), and `null` for a change made directly; +it is the join key for "what did this bundle actually change". + `totalCount` is the unpaged number of rows matching the filters, so a client can page without a second call. JSON is the primary format for this command and is what scripts should consume — it is never altered by the table projection below. From 9d668861adfed07ea09eadea84ad32388c283320 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Sat, 15 Aug 2026 21:24:35 -0400 Subject: [PATCH 3/5] =?UTF-8?q?test(host):=20row=2062=20=E2=80=94=20pin=20?= =?UTF-8?q?ISiteEventLogger's=20Singleton=20lifetime=20in=20the=20site=20g?= =?UTF-8?q?raph?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Six SiteRuntime call sites resolve ISiteEventLogger straight off the root IServiceProvider with no scope: AlarmActor (cached in a field for the actor's lifetime), ScriptActor (x2), InstanceActor (x2), NativeAlarmActor, DeploymentManagerActor, and Scripts/ScriptRunLauncher. All correct today, and correct for exactly one reason — the registration is AddSingleton, so a root resolve yields what a scope would. That makes the lifetime a load-bearing invariant with no enforcement anywhere. If it ever changes, all six silently become register row 45, and nothing in the tree notices: ValidateOnBuild walks CONSTRUCTOR graphs and these are imperative GetService calls in method bodies; ValidateScopes would fire, but at runtime on a site node on whichever path logs an event first, not at boot and not in CI; CaptiveDependencyGuard catches the AlarmActor field case by design but cannot see the five per-call resolves at all. This pin is the tripwire, and its failure message names the six call sites that must be fixed alongside any such change. Asserts the REGISTRATION, not the call sites — the SiteRuntime actor files are correct as written and are untouched here. Two facts, not one, because the interface registration is a FACTORY (AddSingleton(sp => sp.GetRequiredService())): a Singleton lifetime on the interface alone does not prove callers share an instance, since downgrading the concrete SiteEventLogger would make that singleton factory resolve a scoped service from the root. Fail-before, both halves, by temporarily flipping the real registration: - AddScoped -> first pin FAILS, "Actual lifetime: Scoped", with the full row-62 pointer in the message. - AddScoped (interface left Singleton) -> the first pin stays GREEN and only the second catches it. That is the evidence the second fact is load-bearing rather than redundant. Registration restored; both green. Registration-only inspection — the provider is never built, so the pin costs ~30 ms and opens no SQLite file. Host.Tests: 507/507 green. --- .../SiteEventLoggerLifetimePinTests.cs | 146 ++++++++++++++++++ 1 file changed, 146 insertions(+) create mode 100644 tests/ZB.MOM.WW.ScadaBridge.Host.Tests/SiteEventLoggerLifetimePinTests.cs diff --git a/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/SiteEventLoggerLifetimePinTests.cs b/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/SiteEventLoggerLifetimePinTests.cs new file mode 100644 index 00000000..1f17c8a9 --- /dev/null +++ b/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/SiteEventLoggerLifetimePinTests.cs @@ -0,0 +1,146 @@ +using Microsoft.Extensions.Configuration; +using Microsoft.Extensions.DependencyInjection; +using ZB.MOM.WW.ScadaBridge.SiteEventLogging; + +namespace ZB.MOM.WW.ScadaBridge.Host.Tests; + +/// +/// Deferred-work register row 62 — hardening pin, not a bug fix. +/// +/// +/// Six SiteRuntime call sites reach ISiteEventLogger straight off the injected +/// , with no scope in between: +/// +/// +/// SiteRuntime/Actors/AlarmActor.cs — resolves once in the +/// constructor and CACHES it in _siteEventLogger, so it holds the instance for +/// the actor's whole lifetime. +/// SiteRuntime/Actors/ScriptActor.cs (two sites) — per-call +/// resolve. +/// SiteRuntime/Actors/InstanceActor.cs (two sites) — per-call +/// resolve. +/// SiteRuntime/Actors/NativeAlarmActor.cs — per-call +/// resolve. +/// SiteRuntime/Actors/DeploymentManagerActor.cs — per-call +/// resolve. +/// SiteRuntime/Scripts/ScriptRunLauncher.cs — per-run resolve. +/// Not among the five the register row named: it is the run body WP3.1 lifted verbatim +/// out of the deleted ScriptExecutionActor/AlarmExecutionActor, so it inherited the +/// pattern rather than adding it. +/// +/// +/// +/// Every one of those is CORRECT TODAY, and this test is the reason it stays that way. +/// They are correct for exactly one reason: ISiteEventLogger is registered +/// , so resolving it from the root provider yields +/// the same instance a scope would. The day that registration changes lifetime, every one +/// of those call sites silently becomes register row 45 — a captive dependency / +/// resolved-outside-its-scope bug — and nothing else in the tree would notice: +/// +/// +/// ValidateScopes catches a SCOPED service resolved from the +/// root provider — but the actors hold the root provider legitimately, and .NET's +/// check fires at the offending GetService call, i.e. at runtime on a site +/// node, on whatever code path happens to log an event first. Not at boot, and not +/// in CI. +/// ValidateOnBuild walks CONSTRUCTOR graphs. These are +/// imperative GetService calls inside method bodies, invisible to it. +/// CaptiveDependencyGuard catches the AlarmActor field case +/// (a cached resolve) by design, but cannot see the five per-call resolves at +/// all. +/// +/// +/// +/// So the lifetime is a load-bearing invariant with no other enforcement. Pinning it here +/// means a future lifetime change fails loudly, in CI, with a message naming the call +/// sites that have to be fixed alongside it — rather than shipping and being found on the +/// rig months later, which is how row 45 was found. +/// +/// +/// +/// This deliberately asserts the REGISTRATION, not the call sites: the SiteRuntime actor +/// files are correct as written and need no change. The pin is a tripwire on the +/// assumption they rest on. +/// +/// +public class SiteEventLoggerLifetimePinTests +{ + /// + /// The failure message is the whole point of the test — it has to carry the reader + /// from "a lifetime assertion failed" to "here is what breaks and why". + /// + private const string Why = + "ISiteEventLogger MUST be registered Singleton in the site service graph " + + "(deferred-work register row 62).\n" + + "Six SiteRuntime call sites resolve it straight off the root IServiceProvider with " + + "no scope: AlarmActor (cached in a field for the actor's lifetime), ScriptActor (x2), " + + "InstanceActor (x2), NativeAlarmActor, DeploymentManagerActor, and " + + "Scripts/ScriptRunLauncher.\n" + + "Under any non-singleton lifetime every one of those becomes a captive-dependency / " + + "resolved-outside-its-scope bug (register row 45), and NOTHING else catches it: " + + "ValidateOnBuild only walks constructor graphs, ValidateScopes fires at runtime on a " + + "site node rather than at boot, and CaptiveDependencyGuard sees only the AlarmActor " + + "field case.\n" + + "If this lifetime change is intended, fix those six call sites to resolve through an " + + "explicit scope in the same change, then update this pin."; + + /// + /// Builds the real Site service graph the way Program.cs's Site branch does and + /// asserts the lifetime of the registration the SiteRuntime root-resolves depend on. + /// Registration only — the provider is never built, so this costs nothing and touches + /// no SQLite file. + /// + /// The configured site service collection. + private static IServiceCollection BuildSiteServiceGraph() + { + var configuration = new ConfigurationBuilder() + .AddInMemoryCollection(new Dictionary + { + ["ScadaBridge:Node:Role"] = "Site", + ["ScadaBridge:Node:NodeName"] = "node-a", + ["ScadaBridge:Node:NodeHostname"] = "test-site", + ["ScadaBridge:Node:SiteId"] = "TestSite", + ["ScadaBridge:Node:RemotingPort"] = "0", + ["ScadaBridge:Node:GrpcPort"] = "0", + ["LocalDb:Path"] = "/tmp/sb-row62-pin-never-opened.db", + ["AuditLog:SiteWriter:DatabasePath"] = "/tmp/sb-row62-pin-never-opened-audit.db", + ["ScadaBridge:Cluster:SeedNodes:0"] = "akka.tcp://scadabridge@localhost:2551", + ["ScadaBridge:Cluster:SeedNodes:1"] = "akka.tcp://scadabridge@localhost:2552", + }) + .Build(); + + var services = new ServiceCollection(); + SiteServiceRegistration.Configure(services, configuration); + return services; + } + + [Fact] + public void ISiteEventLogger_IsRegisteredSingleton_InTheSiteServiceGraph() + { + var services = BuildSiteServiceGraph(); + + var descriptor = Assert.Single(services, d => d.ServiceType == typeof(ISiteEventLogger)); + + Assert.True(descriptor.Lifetime == ServiceLifetime.Singleton, + $"{Why}\nActual lifetime: {descriptor.Lifetime}."); + } + + [Fact] + public void SiteEventLogger_TheConcreteTypeTheInterfaceForwardsTo_IsAlsoSingleton() + { + // The interface registration is a FACTORY — + // AddSingleton(sp => sp.GetRequiredService()) + // — so a Singleton lifetime on ISiteEventLogger alone does not prove callers get a + // shared instance. Downgrading the concrete SiteEventLogger to Scoped would leave + // the assertion above passing while making the singleton factory resolve a scoped + // service from the root: the exact bug, with the pin still green. Both halves have + // to be pinned for the invariant to actually hold. + var services = BuildSiteServiceGraph(); + + var descriptor = Assert.Single(services, d => d.ServiceType == typeof(SiteEventLogger)); + + Assert.True(descriptor.Lifetime == ServiceLifetime.Singleton, + $"{Why}\nISiteEventLogger forwards to the concrete SiteEventLogger, whose actual " + + $"lifetime is: {descriptor.Lifetime}."); + } +} From 40944a0d04b76d8d0d5463be195589210675f394 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Sat, 15 Aug 2026 21:33:08 -0400 Subject: [PATCH 4/5] =?UTF-8?q?feat(centralui):=20row=2075=20=E2=80=94=20A?= =?UTF-8?q?pprove=20is=20caution=20amber,=20not=20destructive=20red?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Secured Writes page rendered Approve and Reject as IDENTICAL red buttons. Row 28 fixed the labels but left the styling, and the styling was the worse half: at the moment of a two-person-control decision the colour carried no discriminating information at all, and the affirmative half of the control wore the delete affordance. Root cause is that `danger` was a two-state flag for a three-state concept, so a consequential-but-not-destructive action had to choose between looking like a delete and looking like the safe default. Adds the missing middle rung: DialogSeverity { Normal -> btn-primary, Caution -> btn-warning, Destructive -> btn-danger } DECISION. Approve -> Caution (amber): approving executes a real write to live plant equipment, so it is not a benign primary action, but it is not destroying anything either. Reject KEEPS Destructive (red): it is terminal and irreversible for the request — the Operator must resubmit — and holding it red is what keeps the two opposite decisions visually distinct now that Approve is amber. Dropping Reject to neutral was considered and rejected: it would invert the bias, making declining look like the safe default. Colour is never the only signal — both buttons carry their verb. Only Destructive inherits the row-28 "Delete" label fallback; an unlabelled Caution reads "Confirm", since a caution action is by definition not a delete — otherwise row 28 would come back through the new door. ADDITIVE OVERLOAD, deliberately. The boolean form stays the signature for all ~34 existing call sites, which are genuinely two-state, so nothing in src/ outside SecuredWrites.razor changed. Adding a fifth optional parameter to the boolean form instead would have silently un-matched every existing four-argument NSubstitute stub — the exact row-28 gotcha (null Task, caller NREs, no compile error). The overload cannot: the only breakage was six hand-written IDialogService fakes failing to compile, which is the loud failure mode. That gotcha did land once, in SecuredWritesTests' own stub, and is now stubbed on both overloads with a comment saying why. DialogState's `bool Danger` became `DialogSeverity Severity` rather than gaining a second field — it is read only by DialogHost, and one field cannot disagree with itself. Fail-before, both halves: - Approve call site reverted to `danger: true` -> both new Approve pins fail. - Caution remapped to btn-danger in DialogHost -> the amber styling pin fails. Tests: DialogSeverityStylingTests renders the REAL DialogService behind a real DialogHost (FailoverConfirmLabelTests' harness), so it pins what the user sees, not what an argument was — all three severities, both label fallbacks, and the boolean adapter. SecuredWritesTests pins that the page asks for Caution, with a negative control that it did NOT also go through the boolean form (without which the assertion would pass on a call site left at danger:true). Reject's red is pinned too, so a later "make this page less alarming" pass cannot flatten both to one colour. No Playwright selectors needed updating — the suite has no Secured Writes coverage, and no `.btn-danger` selector elsewhere targets this page. Docs: Component-CentralUI.md gains the severity contract under Technology (next to the row-28 label contract) plus the decision on the Secured Writes section. CentralUI.Tests: 1055/1055 green. --- docs/requirements/Component-CentralUI.md | 7 +- .../Pages/Operations/SecuredWrites.razor | 14 +- .../Components/Shared/DialogHost.razor | 25 +++- .../Components/Shared/DialogService.cs | 58 ++++++++- .../Components/Shared/IDialogService.cs | 34 ++++- .../Components/SecuredWritesTests.cs | 58 ++++++++- .../Design/SchemaLibraryPageTests.cs | 6 + .../Pages/NotificationListsPageTests.cs | 6 + .../NotificationReportDetailModalTests.cs | 6 + .../Pages/NotificationReportPageTests.cs | 6 + .../Pages/QueryStringDrillInTests.cs | 6 + .../Pages/SiteCallsReportPageTests.cs | 6 + .../Shared/DialogSeverityStylingTests.cs | 123 ++++++++++++++++++ 13 files changed, 340 insertions(+), 15 deletions(-) create mode 100644 tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Shared/DialogSeverityStylingTests.cs diff --git a/docs/requirements/Component-CentralUI.md b/docs/requirements/Component-CentralUI.md index 8fe002eb..c1a65cd1 100644 --- a/docs/requirements/Component-CentralUI.md +++ b/docs/requirements/Component-CentralUI.md @@ -14,6 +14,11 @@ Central cluster only. Sites have no user interface. - Keeps the entire stack in C#/.NET, consistent with the rest of the system (Akka.NET, EF Core). - SignalR provides built-in support for real-time UI updates. - **Dialog service confirm labels.** Every confirmation goes through `IDialogService.ConfirmAsync(title, message, danger, confirmLabel)`, rendered by the single `DialogHost` in `MainLayout`. `danger` selects **styling only** (`btn-danger`); `confirmLabel` names the action. When no label is supplied the button falls back to "Delete" for a danger confirm and "Confirm" otherwise — a convenience for the ~20 genuine delete call sites, and the reason the Health dashboard's *Trigger failover* dialog used to offer a red **Delete** button (deferred-work register row 28). **Any destructive action that is not literally a delete must pass `confirmLabel`**: failover → *Fail over*, secured-write approve/reject → *Approve* / *Reject*, instance disable → *Disable*, and every parked-message / notification / cached-call discard → *Discard*. +- **Dialog confirm severity — three rungs, not two** (deferred-work register row 75). `DialogSeverity` drives the confirm button's colour: `Normal` → `btn-primary`, `Caution` → `btn-warning`, `Destructive` → `btn-danger`. Only `Destructive` inherits the "Delete" label fallback above; an unlabelled `Caution` reads *Confirm*, because a caution action is by definition not a delete. + - **Why a middle rung exists.** The boolean was a two-state flag for a three-state concept, so a consequential-but-not-destructive action had to choose between looking like a delete and looking like the safe default. The **Secured Writes** page is where that bit: *Approve* and *Reject* both rendered as identical red buttons, so at the moment of a two-person-control decision the colour carried **no discriminating information at all** — and the affirmative half of the control wore the delete affordance. + - **The decision.** *Approve* is `Caution` (amber): approving executes a real write to live plant equipment, so it is not a benign primary action, but it is not destroying anything either. *Reject* **keeps** `Destructive` (red): it is terminal and irreversible for the request — the Operator must resubmit — and holding it red is what keeps the two opposite decisions visually distinct. Dropping *Reject* to a neutral button was considered and rejected: it would simply invert the bias, making declining look like the safe default. + - Colour is never the only signal — the button always carries its verb (see the label contract above), so the mapping is an accessibility affordance, not the meaning. + - **`DialogSeverity` is an additive overload**, not a replacement: the boolean form remains the signature for the ~34 existing call sites, which are genuinely two-state. Adding a fifth optional parameter to the boolean form instead would have silently un-matched every existing four-argument NSubstitute stub — the row-28 gotcha, where a stub that stops matching returns a null `Task` and the caller NREs with no compile error. Reach for the overload only when `Caution` is what you mean. ## Failover Behavior @@ -226,7 +231,7 @@ Per-leaf alarm rendering (leaf nodes are individual conditions for native alarms - A **Secured Writes** page (`/operations/secured-writes`) drives the **two-person** authorization workflow for writes through the MxAccess Gateway: an **Operator** initiates the write, a separate **Verifier** approves it, and only an approved write reaches the site. - The **page itself is gated by `RequireSecuredWriteAccess`** — satisfied by any of `Operator` / `Verifier` / `Administrator`. Although the pending/history lists are read-only, they expose process-sensitive tag values, so the page is not open to every authenticated user; this aligns with the ManagementActor `ListSecuredWritesCommand` any-of gate (arch-review UA1). The submit and approve/reject sub-actions are further gated by `RequireOperator` / `RequireVerifier` respectively (below). - **Operator (submit)** — a submit form gated by `RequireOperator`: pick the site → an **MxGateway** connection on that site → the tag path → a typed value → an optional comment. Submission inserts a `Pending` `PendingSecuredWrite` row centrally; it does **not** write anything yet. -- **Verifier (approve / reject)** — a pending queue gated by `RequireVerifier` with **Approve** / **Reject** (+comment) actions. Approve shows a confirmation of the exact site / connection / tag / value before firing, and surfaces the **submission age** (`Submitted ago ()`) so a verifier can catch a stale setpoint before it reaches the device. The verifier's **own submissions are disabled in the UI and rejected server-side** (no self-approval). On approve, central marks the row `Approved` and relays the write to the site MxGateway (records `Executed` / `Failed`); reject moves it to `Rejected` with a reason. The pending queue is **unpaged** — it is bounded by the submission **TTL**: a `Pending` row that is neither approved nor rejected before the TTL elapses expires to the terminal **`Expired`** status and drops out of the queue into History. +- **Verifier (approve / reject)** — a pending queue gated by `RequireVerifier` with **Approve** / **Reject** (+comment) actions. Approve shows a confirmation of the exact site / connection / tag / value before firing, and surfaces the **submission age** (`Submitted ago ()`) so a verifier can catch a stale setpoint before it reaches the device. The two confirmations are deliberately styled apart: **Approve is amber (`DialogSeverity.Caution`) and Reject stays red** — see the confirm-severity contract under Technology for why (register row 75). The verifier's **own submissions are disabled in the UI and rejected server-side** (no self-approval). On approve, central marks the row `Approved` and relays the write to the site MxGateway (records `Executed` / `Failed`); reject moves it to `Rejected` with a reason. The pending queue is **unpaged** — it is bounded by the submission **TTL**: a `Pending` row that is neither approved nor rejected before the TTL elapses expires to the terminal **`Expired`** status and drops out of the queue into History. - **History** — terminal rows (Executed / Failed / Rejected / **Expired**, where `Expired` is a real TTL-driven terminal status) with the full who/when/outcome trail (operator, verifier, comments, timestamps, any execution error). Terminal rows accumulate without bound, so the History table is **offset-paged** (page-size 50, Prev/Next) driven by the server's unpaged `TotalCount` (`ListSecuredWritesCommand` `Skip`/`Take`); the pager buttons disable at the real bounds. - Every lifecycle event (submit / approve / reject / execute) is written to the central Audit Log; the rows share the `PendingSecuredWrite.Id` as `CorrelationId` so they join into one operation (see Component-ManagementService.md, Component-AuditLog.md). - **Dev caveat**: with `DisableLogin` on, the auto-login identity holds all roles, so the two-person flow cannot be exercised end-to-end by a single user via the dev UI — no-self-approval is covered by handler tests; real two-person use requires two real identities. diff --git a/src/ZB.MOM.WW.ScadaBridge.CentralUI/Components/Pages/Operations/SecuredWrites.razor b/src/ZB.MOM.WW.ScadaBridge.CentralUI/Components/Pages/Operations/SecuredWrites.razor index cbe873e3..e1ee95b4 100644 --- a/src/ZB.MOM.WW.ScadaBridge.CentralUI/Components/Pages/Operations/SecuredWrites.razor +++ b/src/ZB.MOM.WW.ScadaBridge.CentralUI/Components/Pages/Operations/SecuredWrites.razor @@ -424,7 +424,13 @@ $"Value: {row.ValueJson} ({row.ValueType})\n" + $"Submitted {FormatAge(row.SubmittedAtUtc)} ago ({row.SubmittedAtUtc:u})\n" + $"Operator: {row.OperatorUser}", - danger: true, + // Caution, NOT danger (deferred-work register row 75). Approving executes a + // real write to live plant equipment, so this is not a benign primary action + // — but it is not destroying anything either, and red made it look like a + // delete while rendering it identical to the Reject button right beside it. + // Amber keeps the beat of hesitation the write deserves and restores the + // colour contrast between the two opposite decisions. + DialogSeverity.Caution, confirmLabel: "Approve"); if (!confirmed) return; @@ -461,6 +467,12 @@ var confirmed = await Dialog.ConfirmAsync( "Reject secured write", $"Reject the secured write to {row.SiteId}/{row.ConnectionName}/{row.TagPath}?", + // Reject KEEPS danger styling (register row 75). It is terminal and + // irreversible for the request — the Operator has to resubmit — and holding + // it red is what makes the pair readable: with Approve amber, the two + // opposite decisions no longer look alike. Dropping Reject to a neutral + // button would just invert the bias, making declining look like the safe + // default. danger: true, confirmLabel: "Reject"); if (!confirmed) return; diff --git a/src/ZB.MOM.WW.ScadaBridge.CentralUI/Components/Shared/DialogHost.razor b/src/ZB.MOM.WW.ScadaBridge.CentralUI/Components/Shared/DialogHost.razor index 22bac228..ab5bea09 100644 --- a/src/ZB.MOM.WW.ScadaBridge.CentralUI/Components/Shared/DialogHost.razor +++ b/src/ZB.MOM.WW.ScadaBridge.CentralUI/Components/Shared/DialogHost.razor @@ -50,7 +50,7 @@
@@ -185,17 +185,32 @@ svc.Resolve(resolveValue); } - // An explicit label from the caller always wins. The kind/danger fallback below - // is a convenience for the common cases only: a bare danger confirm is assumed to - // be a delete, which is exactly the assumption that put "Delete" on the Trigger + // Bootstrap class for the confirm button, by severity (deferred-work register row 75). + // Caution is the middle rung: amber, for an action that is consequential but is not + // destroying anything — the Secured Writes "Approve" is the case it was added for, + // where red made the affirmative half of a two-person control look like a delete and + // made Approve and Reject visually indistinguishable. Colour is never the only signal: + // the button is always labelled with its verb. + private static string ConfirmButtonClass(DialogState state) => state.Severity switch + { + DialogSeverity.Destructive => "btn-danger", + DialogSeverity.Caution => "btn-warning", + _ => "btn-primary", + }; + + // An explicit label from the caller always wins. The kind/severity fallback below + // is a convenience for the common cases only: a bare destructive confirm is assumed + // to be a delete, which is exactly the assumption that put "Delete" on the Trigger // failover button (deferred-work register row 28). Callers whose destructive verb // is not "Delete" must pass ConfirmLabel — see IDialogService.ConfirmAsync. + // Note Caution deliberately does NOT inherit that fallback: an unlabelled caution + // dialog reads "Confirm", because a caution action is by definition not a delete. private static string ConfirmLabel(DialogState state) => !string.IsNullOrWhiteSpace(state.ConfirmLabel) ? state.ConfirmLabel : state.Kind switch { DialogKind.Prompt => "Save", - _ => state.Danger ? "Delete" : "Confirm", + _ => state.Severity == DialogSeverity.Destructive ? "Delete" : "Confirm", }; } diff --git a/src/ZB.MOM.WW.ScadaBridge.CentralUI/Components/Shared/DialogService.cs b/src/ZB.MOM.WW.ScadaBridge.CentralUI/Components/Shared/DialogService.cs index 3baa15e3..d2d457c7 100644 --- a/src/ZB.MOM.WW.ScadaBridge.CentralUI/Components/Shared/DialogService.cs +++ b/src/ZB.MOM.WW.ScadaBridge.CentralUI/Components/Shared/DialogService.cs @@ -35,7 +35,15 @@ public class DialogService : IDialogService private TaskCompletionSource? _tcs; /// - public Task ConfirmAsync(string title, string message, bool danger = false, string? confirmLabel = null) + public Task ConfirmAsync(string title, string message, bool danger = false, string? confirmLabel = null) => + ConfirmAsync( + title, + message, + danger ? DialogSeverity.Destructive : DialogSeverity.Normal, + confirmLabel); + + /// + public Task ConfirmAsync(string title, string message, DialogSeverity severity, string? confirmLabel = null) { EnsureNoActiveDialog(); var tcs = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); @@ -44,7 +52,7 @@ public class DialogService : IDialogService title, DialogKind.Confirm, message, - danger, + severity, PromptInitial: string.Empty, Placeholder: null, ConfirmLabel: confirmLabel); @@ -58,7 +66,7 @@ public class DialogService : IDialogService EnsureNoActiveDialog(); var tcs = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); _tcs = tcs; - Current = new DialogState(title, DialogKind.Prompt, label, Danger: false, PromptInitial: initialValue, Placeholder: placeholder); + Current = new DialogState(title, DialogKind.Prompt, label, DialogSeverity.Normal, PromptInitial: initialValue, Placeholder: placeholder); OnChange?.Invoke(); return Project(tcs.Task, static r => r as string); } @@ -85,7 +93,7 @@ public class DialogService : IDialogService title, DialogKind.Custom, Body: string.Empty, - Danger: false, + DialogSeverity.Normal, PromptInitial: string.Empty, Placeholder: null, Content: content, @@ -146,7 +154,9 @@ public class DialogService : IDialogService /// Modal title text. /// Discriminates between confirm, prompt, and custom rendering. /// For confirm: the message; for prompt: the input label; ignored for custom. -/// When true, the confirm button uses danger styling. +/// Drives the confirm button's styling, and the verb the host +/// falls back to when is blank. Single source of truth +/// for both — there is deliberately no separate boolean, so the two cannot disagree. /// Initial value for prompt-kind dialogs. /// Placeholder shown when the prompt input is empty. /// For : the body fragment to @@ -160,7 +170,7 @@ public record DialogState( string Title, DialogKind Kind, string Body, - bool Danger, + DialogSeverity Severity, string PromptInitial, string? Placeholder, RenderFragment? Content = null, @@ -173,3 +183,39 @@ public enum DialogKind Prompt, Custom } + +/// +/// How consequential the confirmed action is — drives the confirm button's colour. +/// +/// +/// Deferred-work register row 75. The dialog surface used to carry a two-state +/// bool danger, which forced every consequential-but-not-destructive action to +/// choose between looking like a delete and looking like the safe default. The Secured +/// Writes page is where that bit them: Approve and Reject both rendered +/// as identical red buttons, so at the moment of a two-person-control decision the colour +/// carried no discriminating information at all — and the affirmative half of the control +/// was painted with the delete affordance. +/// +/// is the middle rung that was missing. It is for actions that are +/// consequential and worth a beat of hesitation, but are not destroying anything: the +/// canonical case is a write to live plant equipment. +/// +/// +public enum DialogSeverity +{ + /// Ordinary confirmation — renders btn-primary. + Normal, + + /// + /// Consequential but not destructive — renders btn-warning. Does NOT inherit + /// the "Delete" label fallback; a caution dialog with no explicit label reads + /// "Confirm". + /// + Caution, + + /// + /// Destructive or irreversible — renders btn-danger, and is the only severity + /// that falls back to the "Delete" verb (see DialogHost.ConfirmLabel). + /// + Destructive +} diff --git a/src/ZB.MOM.WW.ScadaBridge.CentralUI/Components/Shared/IDialogService.cs b/src/ZB.MOM.WW.ScadaBridge.CentralUI/Components/Shared/IDialogService.cs index 0b90d48e..6a7fd9de 100644 --- a/src/ZB.MOM.WW.ScadaBridge.CentralUI/Components/Shared/IDialogService.cs +++ b/src/ZB.MOM.WW.ScadaBridge.CentralUI/Components/Shared/IDialogService.cs @@ -20,7 +20,10 @@ public interface IDialogService /// Body text shown to the user. /// When true, the confirm button renders in /// btn-danger styling; otherwise a primary button is shown. This - /// controls styling only — it does not name the action. + /// controls styling only — it does not name the action. For an action that + /// is consequential but NOT destructive, use the + /// overload and pass rather than reaching for + /// red (deferred-work register row 75). /// Verb rendered on the confirm button. When /// null or blank the button falls back to "Delete" for a danger /// dialog and "Confirm" otherwise. Any destructive action that is @@ -31,6 +34,35 @@ public interface IDialogService /// A task that resolves to true when the user confirms, or false when cancelled. Task ConfirmAsync(string title, string message, bool danger = false, string? confirmLabel = null); + /// + /// Shows a confirmation dialog with an explicit , for + /// actions the danger boolean cannot describe. + /// + /// + /// Deferred-work register row 75. This is an ADDITIVE overload, deliberately: the + /// boolean form stays the signature for the ~34 existing call sites, all of which are + /// genuinely two-state. Changing the boolean's type instead would have rippled into + /// pages this change has no business touching, and adding a fifth optional parameter + /// to the boolean form would have silently un-matched every existing four-argument + /// NSubstitute stub — the exact gotcha register row 28 recorded (a stub that stops + /// matching returns a null Task and the caller NREs, with no compile error). A + /// separate overload cannot do either. + /// + /// Reach for this only when is what you mean. + /// For Normal / Destructive the boolean form says the same thing more briefly. + /// + /// + /// Modal title text. + /// Body text shown to the user. + /// How consequential the action is; selects the confirm + /// button's styling. + /// Verb rendered on the confirm button. Only + /// falls back to "Delete" when this is + /// blank; the other severities fall back to "Confirm". The row-28 rule still + /// applies — name the action. + /// A task that resolves to true when the user confirms, or false when cancelled. + Task ConfirmAsync(string title, string message, DialogSeverity severity, string? confirmLabel = null); + /// /// Shows a single-line text prompt and resolves to the entered value, or /// null if the user cancels. diff --git a/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Components/SecuredWritesTests.cs b/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Components/SecuredWritesTests.cs index d57ba929..6dcbf49a 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Components/SecuredWritesTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Components/SecuredWritesTests.cs @@ -45,8 +45,14 @@ public class SecuredWritesTests : BunitContext Services.AddSingleton(_browse); // Confirm dialogs resolve to "yes" without a DialogHost in the render scope. + // BOTH overloads must be stubbed: Reject takes the bool form, Approve takes the + // DialogSeverity form (register row 75). An unstubbed NSubstitute call returns a + // null Task, so missing one here surfaces as an NRE inside the click handler + // rather than as anything that names the dialog — the row-28 gotcha. _dialog.ConfirmAsync(Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any()) .Returns(Task.FromResult(true)); + _dialog.ConfirmAsync(Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any()) + .Returns(Task.FromResult(true)); // Empty list by default; individual tests seed the queue/history. _service.ListAsync( @@ -303,10 +309,60 @@ public class SecuredWritesTests : BunitContext _dialog.Received(1).ConfirmAsync( Arg.Any(), Arg.Is(m => Regex.IsMatch(m, "Submitted .* ago")), - Arg.Any(), + Arg.Any(), Arg.Any()); } + // ── (f) Approve/Reject confirm-button severity (register row 75) ── + + [Fact] + public void Approve_ConfirmDialog_IsCautionNotDestructive() + { + // Row 75. Approving executes a real write to live plant equipment, so it is not a + // benign primary action — but it is not a delete either, and red made it both + // look like one AND look identical to the Reject button beside it, leaving colour + // carrying no information at the moment of a two-person-control decision. + AddAuth("verifier-user", Roles.Verifier); + SeedSites(new Site("Plant-A", "plant-a") { Id = 1 }); + SeedList(new[] { Dto(11, "Pending", "someone-else") }); + _service.ApproveAsync(Arg.Any(), Arg.Any(), Arg.Any()) + .Returns(Task.FromResult(SecuredWriteActionResult.Ok(Dto(11, "Executed", "someone-else", "verifier-user")))); + + var cut = RenderPage(); + cut.WaitForAssertion(() => Assert.NotNull(cut.Find("[data-test='secured-write-approve']"))); + cut.Find("[data-test='secured-write-approve']").Click(); + + _dialog.Received(1).ConfirmAsync( + Arg.Any(), Arg.Any(), DialogSeverity.Caution, "Approve"); + + // Negative control: it must not ALSO have gone through the boolean form, which is + // the one that renders red. Without this the assertion above would still pass if + // the call site had been left on danger:true and merely gained a severity call. + _dialog.DidNotReceive().ConfirmAsync( + Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any()); + } + + [Fact] + public void Reject_ConfirmDialog_KeepsDestructiveStyling() + { + // Row 75 decided Reject KEEPS red: it is terminal and irreversible for the request + // (the Operator must resubmit), and holding it red is what keeps the two opposite + // decisions visually distinct now that Approve is amber. Pinned so a later + // "make the page less alarming" pass cannot quietly flatten both to one colour. + AddAuth("verifier-user", Roles.Verifier); + SeedSites(new Site("Plant-A", "plant-a") { Id = 1 }); + SeedList(new[] { Dto(12, "Pending", "someone-else") }); + _service.RejectAsync(Arg.Any(), Arg.Any(), Arg.Any()) + .Returns(Task.FromResult(SecuredWriteActionResult.Ok(Dto(12, "Rejected", "someone-else", "verifier-user")))); + + var cut = RenderPage(); + cut.WaitForAssertion(() => Assert.NotNull(cut.Find("[data-test='secured-write-reject']"))); + cut.Find("[data-test='secured-write-reject']").Click(); + + _dialog.Received(1).ConfirmAsync( + Arg.Any(), Arg.Any(), danger: true, confirmLabel: "Reject"); + } + [Fact] public void Viewer_DoesNotSeeOperatorOrVerifierRegions() { diff --git a/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Design/SchemaLibraryPageTests.cs b/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Design/SchemaLibraryPageTests.cs index c9182cf0..2d4ad654 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Design/SchemaLibraryPageTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Design/SchemaLibraryPageTests.cs @@ -213,6 +213,12 @@ public class SchemaLibraryPageTests : BunitContext public Task ConfirmAsync(string title, string message, bool danger = false, string? confirmLabel = null) => Task.FromResult(true); + // Register row 75 added a DialogSeverity overload. This stub auto-confirms + // regardless of severity — it exists to let action paths run end to end, not to + // assert anything about styling. + public Task ConfirmAsync(string title, string message, DialogSeverity severity, string? confirmLabel = null) + => Task.FromResult(true); + public Task PromptAsync( string title, string label, string initialValue = "", string? placeholder = null) => Task.FromResult(null); diff --git a/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Pages/NotificationListsPageTests.cs b/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Pages/NotificationListsPageTests.cs index 196f7818..d8efa79f 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Pages/NotificationListsPageTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Pages/NotificationListsPageTests.cs @@ -194,6 +194,12 @@ public class NotificationListsPageTests : BunitContext public Task ConfirmAsync(string title, string message, bool danger = false, string? confirmLabel = null) => Task.FromResult(true); + // Register row 75 added a DialogSeverity overload. This stub auto-confirms + // regardless of severity — it exists to let action paths run end to end, not to + // assert anything about styling. + public Task ConfirmAsync(string title, string message, DialogSeverity severity, string? confirmLabel = null) + => Task.FromResult(true); + public Task PromptAsync( string title, string label, string initialValue = "", string? placeholder = null) => Task.FromResult(null); diff --git a/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Pages/NotificationReportDetailModalTests.cs b/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Pages/NotificationReportDetailModalTests.cs index d1daade5..22947bd3 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Pages/NotificationReportDetailModalTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Pages/NotificationReportDetailModalTests.cs @@ -339,6 +339,12 @@ public class NotificationReportDetailModalTests : BunitContext public Task ConfirmAsync(string title, string message, bool danger = false, string? confirmLabel = null) => Task.FromResult(true); + // Register row 75 added a DialogSeverity overload. This stub auto-confirms + // regardless of severity — it exists to let action paths run end to end, not to + // assert anything about styling. + public Task ConfirmAsync(string title, string message, DialogSeverity severity, string? confirmLabel = null) + => Task.FromResult(true); + public Task PromptAsync( string title, string label, string initialValue = "", string? placeholder = null) => Task.FromResult(null); diff --git a/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Pages/NotificationReportPageTests.cs b/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Pages/NotificationReportPageTests.cs index 7d9fda24..bb6a97b1 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Pages/NotificationReportPageTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Pages/NotificationReportPageTests.cs @@ -274,6 +274,12 @@ public class NotificationReportPageTests : BunitContext public Task ConfirmAsync(string title, string message, bool danger = false, string? confirmLabel = null) => Task.FromResult(true); + // Register row 75 added a DialogSeverity overload. This stub auto-confirms + // regardless of severity — it exists to let action paths run end to end, not to + // assert anything about styling. + public Task ConfirmAsync(string title, string message, DialogSeverity severity, string? confirmLabel = null) + => Task.FromResult(true); + public Task PromptAsync( string title, string label, string initialValue = "", string? placeholder = null) => Task.FromResult(null); diff --git a/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Pages/QueryStringDrillInTests.cs b/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Pages/QueryStringDrillInTests.cs index 72521239..de1864aa 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Pages/QueryStringDrillInTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Pages/QueryStringDrillInTests.cs @@ -213,6 +213,12 @@ public sealed class QueryStringDrillInTests { public Task ConfirmAsync(string title, string message, bool danger = false, string? confirmLabel = null) => Task.FromResult(true); + + // Register row 75 added a DialogSeverity overload. This stub auto-confirms + // regardless of severity — it exists to let action paths run end to end, not to + // assert anything about styling. + public Task ConfirmAsync(string title, string message, DialogSeverity severity, string? confirmLabel = null) + => Task.FromResult(true); public Task PromptAsync(string title, string label, string initialValue = "", string? placeholder = null) => Task.FromResult(null); diff --git a/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Pages/SiteCallsReportPageTests.cs b/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Pages/SiteCallsReportPageTests.cs index e7629476..71477834 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Pages/SiteCallsReportPageTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Pages/SiteCallsReportPageTests.cs @@ -682,6 +682,12 @@ public class SiteCallsReportPageTests : BunitContext public Task ConfirmAsync(string title, string message, bool danger = false, string? confirmLabel = null) => Task.FromResult(true); + // Register row 75 added a DialogSeverity overload. This stub auto-confirms + // regardless of severity — it exists to let action paths run end to end, not to + // assert anything about styling. + public Task ConfirmAsync(string title, string message, DialogSeverity severity, string? confirmLabel = null) + => Task.FromResult(true); + public Task PromptAsync( string title, string label, string initialValue = "", string? placeholder = null) => Task.FromResult(null); diff --git a/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Shared/DialogSeverityStylingTests.cs b/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Shared/DialogSeverityStylingTests.cs new file mode 100644 index 00000000..f48bb9fb --- /dev/null +++ b/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Shared/DialogSeverityStylingTests.cs @@ -0,0 +1,123 @@ +using Bunit; +using Microsoft.Extensions.DependencyInjection; +using ZB.MOM.WW.ScadaBridge.CentralUI.Components.Shared; + +namespace ZB.MOM.WW.ScadaBridge.CentralUI.Tests.Shared; + +/// +/// Deferred-work register row 75 — pins the confirm button's severity → Bootstrap class +/// mapping against the REAL behind a real DialogHost, +/// so these cover what the user actually sees rather than what an argument was. +/// +/// +/// Row 28 split the label from the styling but left the styling a two-state boolean, so a +/// consequential-but-not-destructive action had to choose between looking like a delete +/// and looking like the safe default. Row 75 added as +/// the middle rung. The three-way mapping is the whole of that change, and these are its +/// pins; SecuredWritesTests separately pins that the Secured Writes page asks for +/// the right one. +/// +/// +/// +/// bUnit cannot run real JS interop, so the host's sbDialog.* focus-trap calls run +/// in as no-ops (same arrangement as +/// FailoverConfirmLabelTests). +/// +/// +public class DialogSeverityStylingTests : BunitContext +{ + private readonly DialogService _dialog = new(); + + public DialogSeverityStylingTests() + { + Services.AddSingleton(_dialog); + JSInterop.Mode = JSRuntimeMode.Loose; + } + + /// + /// Opens a confirm dialog on a live host and returns its confirm button. + /// + /// + /// The ConfirmAsync task is deliberately not awaited — with the real service it + /// does not complete until someone answers the dialog, so awaiting it would deadlock + /// the test. The state is published synchronously before the task is returned. + /// + private AngleSharp.Dom.IElement OpenConfirm(Func> open) + { + var cut = Render(); + cut.InvokeAsync(() => { _ = open(_dialog); }).GetAwaiter().GetResult(); + cut.Render(); + + var footer = cut.FindAll("div.modal-footer button").ToList(); + return footer.Single(b => b.TextContent.Trim() != "Cancel"); + } + + [Fact] + public void Caution_rendersAmber_notRedAndNotPrimary() + { + // The row-75 case. Amber is the point: not red (which reads "delete"), and not + // the primary blue (which reads "safe default" — wrong for a live device write). + var confirm = OpenConfirm(d => d.ConfirmAsync("Approve thing", "Sure?", DialogSeverity.Caution, "Approve")); + + Assert.Contains("btn-warning", confirm.ClassList); + Assert.DoesNotContain("btn-danger", confirm.ClassList); + Assert.DoesNotContain("btn-primary", confirm.ClassList); + } + + [Fact] + public void Destructive_stillRendersRed() + { + // Regression guard on the ~20 genuine delete call sites: row 75 is additive and + // must not have shifted what danger looks like. + var confirm = OpenConfirm(d => d.ConfirmAsync("Delete thing", "Sure?", DialogSeverity.Destructive)); + + Assert.Contains("btn-danger", confirm.ClassList); + } + + [Fact] + public void Normal_stillRendersPrimary() + { + var confirm = OpenConfirm(d => d.ConfirmAsync("Do thing", "Sure?", DialogSeverity.Normal)); + + Assert.Contains("btn-primary", confirm.ClassList); + } + + [Theory] + [InlineData(true, "btn-danger")] + [InlineData(false, "btn-primary")] + public void BooleanForm_mapsOntoTheSameTwoSeverities(bool danger, string expectedClass) + { + // The boolean overload is now a thin adapter onto the severity one. Every existing + // call site goes through it, so this pins that the adapter did not shift any of + // them: true is still red, false is still primary. + // + // One case per fact, deliberately: DialogService permits only one open dialog at a + // time and throws on the second, so two OpenConfirm calls in one body would fail + // on the service's own nested-dialog guard rather than on anything about styling. + var confirm = OpenConfirm(d => d.ConfirmAsync("Do thing", "Sure?", danger: danger)); + + Assert.Contains(expectedClass, confirm.ClassList); + } + + [Fact] + public void UnlabelledCaution_readsConfirm_notDelete() + { + // Caution deliberately does NOT inherit the danger "Delete" fallback: a caution + // action is by definition not a delete, so falling back to that verb would + // reintroduce register row 28 through the new door. + var confirm = OpenConfirm(d => d.ConfirmAsync("Approve thing", "Sure?", DialogSeverity.Caution)); + + Assert.Equal("Confirm", confirm.TextContent.Trim()); + } + + [Fact] + public void UnlabelledDestructive_stillFallsBackToDelete() + { + // The row-28 fallback is retained deliberately (the Playwright suites click the + // literal "Delete"). Pinned here alongside its new sibling so the pair is read + // together. + var confirm = OpenConfirm(d => d.ConfirmAsync("Delete thing", "Sure?", DialogSeverity.Destructive)); + + Assert.Equal("Delete", confirm.TextContent.Trim()); + } +} From 851b528ca36cb996b0e9de0b815e6d05364dfc32 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Sat, 15 Aug 2026 21:34:17 -0400 Subject: [PATCH 5/5] =?UTF-8?q?docs(deployment):=20row=2079=20=E2=80=94=20?= =?UTF-8?q?OrbStack/virtiofs=20rig=20hazard=20+=20proven=20recovery=20runb?= =?UTF-8?q?ook?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Documents the host-level hazard behind row 69's LocalDb corruption, all live-verified 2026-08-15. Scope is the macOS docker rigs only — this is a property of the filesystem the rig's data files sit on, not a ScadaBridge defect and not something a production Linux deployment is exposed to. Every rig data file (site-localdb.db, auditlog.db, secrets) rides a relative bind mount out of the repo tree, which on macOS crosses virtiofs — and virtiofs does not give SQLite true fsync durability. `--force-recreate`, which deploy.sh uses, is exactly the abrupt mid-write kill that turns that into a torn file. Records both observed failure modes, and specifically how differently they present, which is the part that makes this hard to spot: site-a-b failed loudly while running (every sync call throwing, connected:false, backlog 892), while site-a-a was corrupt LATENTLY and only became fatal on the next upgrade, when a new build's boot-time re-registration read pages the old build never touched (SIGABRT crash-loop x12). A clean-looking node proves nothing about its file. Second mode is the VM idle-sleeping mid-operation — `vmgr ... msg=sleep` in ~/.orbstack/log/vmgr.log, then virtiofs `refresh_nodeid: race with new lookup` errors and a hung docker until `orb restart docker` (data intact, containers auto-restart). Written so a future session checks vmgr.log BEFORE investigating ScadaBridge, which is where ~20 minutes went last time. Recovery runbook is the one that actually worked: stop the pair together, quarantine site-localdb.db AND its -wal/-shm sidecars with a .corrupt- suffix (never delete — forensics; and a stale WAL beside a fresh database is its own corruption), restart together, then VERIFY convergence rather than assuming it. Notes the precondition that this is a replicated pair's recovery — it leans on the startup reconcile plus CDC snapshot resync, so an unreplicated node has no peer to reseed from and needs a backup instead. Mitigation options tabulated with their real costs (named volumes = native ext4 but loses the host-side inspectability the debugging sessions lean on; disabling OrbStack idle-pause = a host setting that cannot be version-controlled and silently does not exist on a new machine; accept + runbook = current), so the status quo reads as a choice rather than an oversight. Docs only — no compose change, no host config change, rig untouched. --- docs/deployment/topology-guide.md | 97 +++++++++++++++++++++++++++++++ 1 file changed, 97 insertions(+) diff --git a/docs/deployment/topology-guide.md b/docs/deployment/topology-guide.md index 5320168c..8836cd8c 100644 --- a/docs/deployment/topology-guide.md +++ b/docs/deployment/topology-guide.md @@ -311,6 +311,103 @@ a rejoin is safe and self-correcting (verified live: a node stopped and restarte with both nodes byte-identical and zero duplicates). Beyond it, rebuild the returning node's database from its peer rather than letting it rejoin. +### Docker-rig data files on macOS/OrbStack — a virtiofs corruption + availability hazard + +**Scope: the local `docker/` and `docker-env2/` development rigs on macOS only.** This is a +host-level property of the filesystem the rig's data files sit on, **not a ScadaBridge defect** and +not something a production Linux deployment is exposed to. It is documented here because it has +already cost two databases and half a session, and because the recovery is non-obvious. + +Every ScadaBridge data file on the rig rides a **relative bind mount** out of the repo working tree +— `./site-a-node-a/data:/app/data` and its siblings, which is where `site-localdb.db`, +`auditlog.db` and the secrets store all live. On macOS that path crosses **virtiofs** to reach the +Linux VM, and **virtiofs does not give SQLite true `fsync` durability**. SQLite's crash-safety +contract assumes a successful `fsync` means the bytes are on stable storage; when that is not true, +an abrupt container kill mid-write can leave a torn file. `docker compose up --force-recreate` +— which `deploy.sh` uses — is exactly such an abrupt kill. + +Two distinct failure modes have been observed live (both 2026-08-15): + +**1. Silent database corruption.** Both site-a LocalDb files were found malformed: + +``` +SQLite Error 11: 'database disk image is malformed' +``` + +Note how differently the two nodes presented, because this is the part that makes it hard to spot: + +- **site-a-b** surfaced it as a *running* fault — every sync call threw, replication reported + `connected: false` with a backlog of 892, and the faulted session retried about once a minute. + The node stayed up and otherwise healthy-looking. +- **site-a-a** was corrupt too, but **latently** — nothing on the running build ever read the + damaged pages. It only became fatal on the *next upgrade*, when a new build's boot-time + re-registration read pages the old build never touched: `SqliteLocalDb` construction threw and + the container **SIGABRT crash-looped** (×12). + +The lesson to carry: a clean-looking node proves nothing about its file. Corruption can sit latent +until an unrelated code change happens to read further into the database. + +**2. The OrbStack VM idle-sleeping mid-operation.** The VM can suspend under its idle policy while +work is in flight. `~/.orbstack/log/vmgr.log` records it plainly: + +``` +vmgr ... msg=sleep +``` + +On wake, the virtiofs layer logs repeated errors of the form: + +``` +refresh_nodeid: race with new lookup +``` + +and `docker` hangs — commands stop returning, `docker ps` comes back empty — for as long as it +takes to notice. **The fix is `orb restart docker`**; data survives and containers auto-restart. +Observed cost: roughly 20 minutes of a session lost to diagnosing what looked like a cluster +failure but was a host one. If the rig goes unresponsive as a whole, check `vmgr.log` **before** +investigating ScadaBridge. + +#### Recovery runbook — LocalDb corruption on a site pair (proven) + +This is the procedure that recovered site-a. It relies on the pair being able to reseed itself, +so it is a *replicated* pair's recovery; an unreplicated node has no peer to reseed from and must +be restored from a backup instead. + +1. **Stop BOTH nodes of the site pair, together.** Not one at a time — this is the same + stop-and-start-together rule as *Site Pair Upgrades* above, for the same reason. +2. **Quarantine the database and its sidecars — do not delete them.** Rename all three, keeping + them for forensics: + + ```sh + # per affected node's data directory + mv site-localdb.db site-localdb.db.corrupt-$(date +%F) + mv site-localdb.db-wal site-localdb.db-wal.corrupt-$(date +%F) + mv site-localdb.db-shm site-localdb.db-shm.corrupt-$(date +%F) + ``` + + The `-wal` and `-shm` files **must** move with the main file. Leaving a stale WAL beside a fresh + database is its own corruption. +3. **Start both nodes together.** The fresh files reseed themselves from two independent paths: + `SiteReconciliationActor`'s node-startup fetch pulls config back from central when central + reports gaps, and LocalDb's **CDC snapshot resync** merges the peer's rows under LWW. +4. **Verify convergence**, do not assume it: `LocalDbReplicationConnected` should read `true` and + `LocalDbOplogBacklog` should settle to ~0 on the site health report. (See *Reading the + replication backlog* above for why a transient spike is normal and not a fault.) + +On the live recovery, both quarantined files were large (394 MB and 416 MB), the pair converged to +`connected: true` / backlog 0, and a schema change in flight at the time additionally proved the +digest handshake green. + +#### Mitigation options, with their trade-offs + +None of these is applied today; the rig deliberately still runs on bind mounts, and the accepted +position is the runbook above. Recorded so the choice is a choice: + +| Option | Gains | Costs | +|--------|-------|-------| +| Move rig data dirs to **named docker volumes** | Files live on native ext4 inside the VM, out of virtiofs entirely — removes the corruption mechanism at the source | Loses the host-side inspectability the debugging sessions lean on constantly: no more opening `site-localdb.db` from the Mac, no `ls` of a node's data dir from the repo tree. Requires a compose change. | +| **Disable OrbStack's idle pause** | Removes failure mode 2 (the mid-operation sleep and the virtiofs `refresh_nodeid` storm) | A host-level setting outside this repo, so it cannot be version-controlled or applied by `deploy.sh` — it silently does not exist on a new machine. Does nothing about failure mode 1. | +| **Accept, and use this runbook** (current) | No changes to compose or host config; full host-side inspectability retained | Corruption recurs on the next unlucky `--force-recreate`, and can sit latent until a later upgrade turns it into a crash loop. | + ### Central-Site Communication Three transports cross the boundary, not one — **all now gRPC or HTTP; Akka ClusterClient was removed