diff --git a/.gitignore b/.gitignore index aec5aeb6..70761ed8 100644 --- a/.gitignore +++ b/.gitignore @@ -44,5 +44,14 @@ docker-env2/*/data/ # Local credentials / login captures — never commit *login*.txt +# Ad-hoc scratch/credential captures at the repo root — never commit +# (arch-review 08 round 2 NF1: a live API key sat in an untracked test.txt) +/test.txt +/*credentials* +/*secret* + +# Claude tool worktrees (NF7) +.claire/ + # Sister-project deployment artifacts (not part of this solution) /deploy/ diff --git a/CHANGELOG.md b/CHANGELOG.md index 0d54c18a..0c9fcd39 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,13 +4,45 @@ All notable changes to ScadaBridge are documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). +Detailed per-change history lives in `git log`, +`archreview/plans/00-MASTER-TRACKER.md`, and `docs/plans/` — this file records +operator-facing and breaking changes only. + ## [Unreleased] +### Changed — Architecture-review hardening initiative (PLAN-01…PLAN-08, 2026-07-08 → 2026-07-10) + +Eight architecture reviews produced eight fix plans; 191 of 192 tasks landed. The +initiative added eager startup options-validation across 12+ components, ClusterClient +message-contract lock tests, secured-write pending-TTL, a login throttle, transport +import single-flight, store-and-forward sweep bounding, and a deploy-readiness +handshake; it excised the vestigial site-notification delivery surface and established +a canonical deferred-work register. Authoritative record: +`archreview/plans/00-MASTER-TRACKER.md` plus the per-plan docs. + +### Added — M8 Transport + +Encrypted bundle export/import extended from central-only config to site/instance-scoped +config (`Site`s, site `DataConnection`s, `Instance`s + `Area` by name), reconciled across +environments by the `BundleNameMap` name-mapping subsystem, with a per-line Myers diff and +the script trust gate enforced at import review. Post-initiative additions (both 2026-07-10): +an aggregated live alarm stream feeding the Alarm Summary page, and hourly KPI rollups +unblocking 30d/90d trend windows. + +### Added — M7 Native alarms + operational UX + +Read-only native-alarm mirroring (OPC UA Alarms & Conditions + MxAccess Gateway) onto a +unified condition model; an operator Alarm Summary page; OPC UA node browse/search/type-info +and site-local certificate trust; **two-person secured writes, introducing the `Operator` +and `Verifier` roles**; and an SMS (Twilio) notification delivery adapter. + ### Changed — BREAKING: canonical role names + audit separation-of-duties collapse (Task 1.7) Role string VALUES are standardized onto the canonical vocabulary -(`Administrator`/`Designer`/`Deployer`/`Viewer`; `Operator`/`Engineer` are unused -by ScadaBridge). The legacy ScadaBridge role names were renamed and two were +(`Administrator`/`Designer`/`Deployer`/`Viewer`; `Engineer` remains unused by +ScadaBridge; `Operator` has since been adopted by the M7 two-person secured-write +flow, together with the new `Verifier` role — see the M7 entry above). The legacy +ScadaBridge role names were renamed and two were **collapsed**: | Legacy role | Canonical role | Notes | diff --git a/archreview/plans/00-MASTER-TRACKER.md b/archreview/plans/00-MASTER-TRACKER.md index e1474bac..38b3ed2c 100644 --- a/archreview/plans/00-MASTER-TRACKER.md +++ b/archreview/plans/00-MASTER-TRACKER.md @@ -187,6 +187,8 @@ Run up to **five plan-lane executor sessions concurrently** (one each for PLAN-0 ## Deferred / Won't-Fix Registry (initiative-level) +> 2026-07-12 (plan R2-08 T10): the two live items below (SBR oldest-crash gap, wonder-app-vd03 overlay) now have canonical rows in `docs/plans/2026-07-08-deferred-work-register.md` — that register is the single tracking place; the subsections below remain as the historical narrative. + Consolidated in **PLAN-08 Task 11** → `docs/plans/2026-07-08-deferred-work-register.md` (triages the 23-item inventory from report 08 plus each plan's coverage-table deferrals: serializer swap, hash-chain/Parquet, aggregated live alarm stream, central cert-trust persistence, runtime script sandbox, clustered-GUID benchmark, TLS profile, Dockerfile HEALTHCHECK, etc.). Nothing from the review is silently dropped: every report finding is either a task, an ownership pointer, or a registered deferral with rationale. ### Follow-up discovered during P0 execution (2026-07-08) diff --git a/docs/plans/2026-07-08-deferred-work-register.md b/docs/plans/2026-07-08-deferred-work-register.md index dc2c361d..e9cb09e7 100644 --- a/docs/plans/2026-07-08-deferred-work-register.md +++ b/docs/plans/2026-07-08-deferred-work-register.md @@ -5,15 +5,8 @@ Single tracked list of consciously-deferred work. Rules: every deferral gets a r them and are removed from this table when that plan's task lands. ## Fix-now (owned by archreview plans) -| # | Item | Owner | -|---|------|-------| -| 1 | Transport: Area membership doesn't travel in bundles (EntitySerializer.cs:251,518) | PLAN-05 | -| 2 | CLI.Tests not in slnx | PLAN-08 Task 1 | -| 3 | AuditLog reconciliation dials site NodeA only (SiteEnumerator.cs:32,64) | PLAN-04 | -| 4 | Options validation missing on ~12 components | PLAN-08 Tasks 2–5 | -| 5 | SmsConfiguration.MaxRetryCount stored but not honored | PLAN-06 | -| 6 | Role-check case-sensitivity asymmetry (ManagementActor.cs:1910) | PLAN-07 | -| 23 | Vestigial site-side notification-list surface | PLAN-08 Task 6 | + +All 7 fix-now items landed via PLAN-04/05/06/07/08 (verified in review 08 round 2, 2026-07-12) — rows removed per the rule above; see the round-2 report §1 for the per-item evidence. ## Deferred (with rationale + revisit trigger) | # | Item | Where noted | Rationale for deferral | Revisit trigger | @@ -48,4 +41,12 @@ Rows removed from the Deferred table above once confirmed shipped. Kept here for | Communication → HealthMonitoring layering (ICentralHealthAggregator consumed by CentralCommunicationActor.cs:351) | Moving the interface + SiteHealthState to Commons ripples across 5 projects for a cosmetic inversion | Next breaking change to ICentralHealthAggregator | | docs/components reference docs for ScriptAnalysis, KpiHistory, DelmiaNotifier | Reference docs are substantial (StyleGuide-conformant); README claim scoped instead (PLAN-08 Task 10) | Next doc-writing session touching those components | | Test-coverage backfill: SiteCallAudit.Tests (31 tests/1.6k LOC), DeploymentManager.Tests | No defect identified; coverage partly lives in ManagementService/Host/Integration suites | First regression escaping either component | -| Failover-timing + broader perf envelope (S&F drain rate, per-subscriber backpressure) | Needs PLAN-01 two-node rig; placeholder harness shipped (PLAN-08 Task 8) | PLAN-01 rig landing | +| Failover-timing + broader perf envelope (S&F drain rate, per-subscriber backpressure) | **Trigger fired 2026-07-08** — the "PLAN-01 rig landing" trigger fired when `tests/ZB.MOM.WW.ScadaBridge.IntegrationTests/Cluster/TwoNodeClusterFixture.cs` landed (before the placeholder harness even shipped), and went unnoticed (NF2). **Owner: PLAN-R2-01** — `archreview/plans/PLAN-R2-01-*.md` (failover-envelope measurement task) owns wiring `FailoverTimingTests` to the fixture rig or recording the true blocker. | Owned by PLAN-R2-01; this register row tracks the handoff only | + +## Deferred — operational risk (from the initiative tracker, folded in 2026-07-12) +Two live items previously tracked ONLY in `archreview/plans/00-MASTER-TRACKER.md`'s registry are folded in here (NF5) so this register is the single tracking place. The tracker's narrative subsections remain as the historical evidence. + +| # | Item | Where noted | Rationale for deferral | Revisit trigger | +|---|------|-------------|------------------------|-----------------| +| SBR | **SBR oldest-crash total-outage gap** — 2-node `keep-oldest` downs the partition *without* the oldest, so a hard crash of the ACTIVE (oldest) central node makes the standby self-down (~10s) → total central outage until the crashed node restarts; only a younger-node crash fails over. | `archreview/plans/00-MASTER-TRACKER.md:194` + auto-memory `sbr-keep-oldest-2node-active-crash-gap` | Remedy is a production SBR topology/strategy decision (keep-majority + a 3rd/lighthouse seed node, static-quorum, or an accepted-risk note) — **owner: user decision**, not silently changeable. | Before the next production deployment that adds a central node, or the first real active-node crash. | +| vd03 | **`deploy/wonder-app-vd03/` overlay edits unapplied** — `appsettings.Central.json` needs `AllowSingleNodeCluster: true` + phantom-seed removal + `NodeName: central-a`; `install.ps1` needs `sc.exe failure` recovery actions. The `deploy/wonder-app-vd03/` artifact directory is intentionally untracked (production config out of source control), so the repo cannot ship the fix. | `archreview/plans/00-MASTER-TRACKER.md:198` (PLAN-01 T16/T20/T23) | Needs on-host access; without `NodeName` that deployment's audit rows stamp NULL `SourceNode` — **partially mitigated once PLAN-R2-08 Task 7 lands: the host now FAILS AT BOOT with a key-naming error instead of silently NULLing, so applying the overlay becomes mandatory at the next upgrade.** Owner: whoever maintains the host (user). | Next wonder-app-vd03 deployment/upgrade — **the Task 7 validator makes this row unskippable then.** | diff --git a/src/ZB.MOM.WW.ScadaBridge.AuditLog/Central/AuditLogPartitionMaintenanceOptionsValidator.cs b/src/ZB.MOM.WW.ScadaBridge.AuditLog/Central/AuditLogPartitionMaintenanceOptionsValidator.cs new file mode 100644 index 00000000..0ad5373a --- /dev/null +++ b/src/ZB.MOM.WW.ScadaBridge.AuditLog/Central/AuditLogPartitionMaintenanceOptionsValidator.cs @@ -0,0 +1,25 @@ +using ZB.MOM.WW.Configuration; + +namespace ZB.MOM.WW.ScadaBridge.AuditLog.Central; + +/// +/// Validates at host startup +/// (arch-review 08 round 2 NF4). A zero tick interval spins the maintenance +/// hosted service; a zero look-ahead leaves pf_AuditLog_Month without a +/// future monthly boundary, risking inserts into the unbounded tail partition. +/// +public sealed class AuditLogPartitionMaintenanceOptionsValidator + : OptionsValidatorBase +{ + /// + protected override void Validate(ValidationBuilder builder, AuditLogPartitionMaintenanceOptions options) + { + builder.RequireThat(options.IntervalSeconds > 0, + $"AuditLog:PartitionMaintenance:{nameof(AuditLogPartitionMaintenanceOptions.IntervalSeconds)} " + + $"({options.IntervalSeconds}) must be > 0."); + + builder.RequireThat(options.LookaheadMonths > 0, + $"AuditLog:PartitionMaintenance:{nameof(AuditLogPartitionMaintenanceOptions.LookaheadMonths)} " + + $"({options.LookaheadMonths}) must be > 0."); + } +} diff --git a/src/ZB.MOM.WW.ScadaBridge.AuditLog/Central/AuditLogPurgeOptionsValidator.cs b/src/ZB.MOM.WW.ScadaBridge.AuditLog/Central/AuditLogPurgeOptionsValidator.cs new file mode 100644 index 00000000..da5bb659 --- /dev/null +++ b/src/ZB.MOM.WW.ScadaBridge.AuditLog/Central/AuditLogPurgeOptionsValidator.cs @@ -0,0 +1,31 @@ +using ZB.MOM.WW.Configuration; + +namespace ZB.MOM.WW.ScadaBridge.AuditLog.Central; + +/// +/// Validates at host startup (arch-review 08 +/// round 2 NF4). The class self-clamps at resolve time, but a plainly-wrong +/// config (zero cadence, zero batch, zero maintenance-command timeout) should +/// still fail the boot so the operator learns immediately. +/// is test-only (per the +/// class remarks) and is deliberately not validated. +/// +public sealed class AuditLogPurgeOptionsValidator : OptionsValidatorBase +{ + /// + protected override void Validate(ValidationBuilder builder, AuditLogPurgeOptions options) + { + builder.RequireThat(options.IntervalHours > 0, + $"AuditLog:Purge:{nameof(AuditLogPurgeOptions.IntervalHours)} " + + $"({options.IntervalHours}) must be > 0."); + + // Backing field for the documented "ChannelPurgeBatchSize" config key. + builder.RequireThat(options.ChannelPurgeBatchSizeConfigured > 0, + $"AuditLog:Purge:{nameof(AuditLogPurgeOptions.ChannelPurgeBatchSizeConfigured)} " + + $"(config key ChannelPurgeBatchSize = {options.ChannelPurgeBatchSizeConfigured}) must be > 0."); + + builder.RequireThat(options.MaintenanceCommandTimeoutMinutes > 0, + $"AuditLog:Purge:{nameof(AuditLogPurgeOptions.MaintenanceCommandTimeoutMinutes)} " + + $"({options.MaintenanceCommandTimeoutMinutes}) must be > 0."); + } +} diff --git a/src/ZB.MOM.WW.ScadaBridge.AuditLog/Central/SiteAuditReconciliationOptionsValidator.cs b/src/ZB.MOM.WW.ScadaBridge.AuditLog/Central/SiteAuditReconciliationOptionsValidator.cs new file mode 100644 index 00000000..63bee720 --- /dev/null +++ b/src/ZB.MOM.WW.ScadaBridge.AuditLog/Central/SiteAuditReconciliationOptionsValidator.cs @@ -0,0 +1,31 @@ +using ZB.MOM.WW.Configuration; + +namespace ZB.MOM.WW.ScadaBridge.AuditLog.Central; + +/// +/// Validates at host startup +/// (arch-review 08 round 2 NF4). A zero tick interval spins the reconciliation +/// singleton, a zero batch pulls nothing per RPC, and a zero stalled-cycle +/// threshold would fire the stalled signal on the first non-draining cycle. +/// +/// is test-only (per the class remarks) and is deliberately not validated. +/// +public sealed class SiteAuditReconciliationOptionsValidator + : OptionsValidatorBase +{ + /// + protected override void Validate(ValidationBuilder builder, SiteAuditReconciliationOptions options) + { + builder.RequireThat(options.ReconciliationIntervalSeconds > 0, + $"AuditLog:Reconciliation:{nameof(SiteAuditReconciliationOptions.ReconciliationIntervalSeconds)} " + + $"({options.ReconciliationIntervalSeconds}) must be > 0."); + + builder.RequireThat(options.BatchSize > 0, + $"AuditLog:Reconciliation:{nameof(SiteAuditReconciliationOptions.BatchSize)} " + + $"({options.BatchSize}) must be > 0."); + + builder.RequireThat(options.StalledAfterNonDrainingCycles > 0, + $"AuditLog:Reconciliation:{nameof(SiteAuditReconciliationOptions.StalledAfterNonDrainingCycles)} " + + $"({options.StalledAfterNonDrainingCycles}) must be > 0."); + } +} diff --git a/src/ZB.MOM.WW.ScadaBridge.AuditLog/ServiceCollectionExtensions.cs b/src/ZB.MOM.WW.ScadaBridge.AuditLog/ServiceCollectionExtensions.cs index ef000387..54497c4a 100644 --- a/src/ZB.MOM.WW.ScadaBridge.AuditLog/ServiceCollectionExtensions.cs +++ b/src/ZB.MOM.WW.ScadaBridge.AuditLog/ServiceCollectionExtensions.cs @@ -92,20 +92,23 @@ public static class ServiceCollectionExtensions // failures as AuditRedactionFailure on the site health report. services.TryAddSingleton(); - // Site writer + telemetry options bindings. - // BindConfiguration is not used because the configuration root supplied - // by the caller may not be the application root — we go through the - // section explicitly so a partial IConfiguration (e.g. a test stub - // anchored on the AuditLog section's parent) still works. - services.AddOptions() - .Bind(config.GetSection(SiteWriterSectionName)); - services.AddOptions() - .Bind(config.GetSection(SiteTelemetrySectionName)); + // Site writer + telemetry options bindings — now bind-and-validate via + // the shared helper so a bad site config fails fast at boot (NF4). The + // helper's GetSection(sectionPath) still resolves the section explicitly + // (NOT BindConfiguration), preserving the partial-config-root rationale: + // the caller's IConfiguration may be a test stub anchored on the AuditLog + // section's parent rather than the application root. + services.AddValidatedOptions( + config, SiteWriterSectionName); + services.AddValidatedOptions( + config, SiteTelemetrySectionName); // Retention purge options (the SiteAuditRetentionService hosted job is - // registered site-only in AddAuditLogHealthMetricsBridge). Binding here is - // inert on central — no hosted service consumes it there. - services.AddOptions() - .Bind(config.GetSection(SiteAuditRetentionOptions.SectionName)); + // registered site-only in AddAuditLogHealthMetricsBridge). The binding + // here is inert on central — no hosted service consumes it there — but + // the defaults validate clean, so eager validation on the central root is + // harmless. + services.AddValidatedOptions( + config, SiteAuditRetentionOptions.SectionName); // SqliteAuditWriter is a singleton with a single owned SqliteConnection // and a background writer Task; multiple instances would race on the @@ -365,8 +368,8 @@ public static class ServiceCollectionExtensions ArgumentNullException.ThrowIfNull(services); ArgumentNullException.ThrowIfNull(config); - services.AddOptions() - .Bind(config.GetSection(PartitionMaintenanceSectionName)); + services.AddValidatedOptions(config, PartitionMaintenanceSectionName); services.AddHostedService(); // I1 (review): bind the two central-singleton options HERE rather than in @@ -382,10 +385,10 @@ public static class ServiceCollectionExtensions // 5 min reconciliation tick); production exposes IntervalHours / // ReconciliationIntervalSeconds only — the test-only *Override knobs are // not intended to be set from config (see the options classes' remarks). - services.AddOptions() - .Bind(config.GetSection(PurgeSectionName)); - services.AddOptions() - .Bind(config.GetSection(ReconciliationSectionName)); + services.AddValidatedOptions( + config, PurgeSectionName); + services.AddValidatedOptions( + config, ReconciliationSectionName); // Central health snapshot — a single object // that owns the CentralAuditWriteFailures + AuditRedactionFailure diff --git a/src/ZB.MOM.WW.ScadaBridge.AuditLog/Site/SiteAuditRetentionOptionsValidator.cs b/src/ZB.MOM.WW.ScadaBridge.AuditLog/Site/SiteAuditRetentionOptionsValidator.cs new file mode 100644 index 00000000..6c6ae34f --- /dev/null +++ b/src/ZB.MOM.WW.ScadaBridge.AuditLog/Site/SiteAuditRetentionOptionsValidator.cs @@ -0,0 +1,31 @@ +using ZB.MOM.WW.Configuration; + +namespace ZB.MOM.WW.ScadaBridge.AuditLog.Site; + +/// +/// Validates at host startup +/// (arch-review 08 round 2 NF4). The option class self-clamps at resolve time, +/// but a plainly-wrong config (zero retention window, zero/negative purge +/// period, negative initial delay) should still fail the boot so the operator +/// learns immediately rather than trusting a silent clamp. +/// is test-only +/// (per the class remarks) and is deliberately not validated. +/// +public sealed class SiteAuditRetentionOptionsValidator : OptionsValidatorBase +{ + /// + protected override void Validate(ValidationBuilder builder, SiteAuditRetentionOptions options) + { + builder.RequireThat(options.RetentionDays > 0, + $"AuditLog:SiteRetention:{nameof(SiteAuditRetentionOptions.RetentionDays)} " + + $"({options.RetentionDays}) must be > 0."); + + builder.RequireThat(options.PurgeInterval > TimeSpan.Zero, + $"AuditLog:SiteRetention:{nameof(SiteAuditRetentionOptions.PurgeInterval)} " + + $"({options.PurgeInterval}) must be > 0."); + + builder.RequireThat(options.InitialDelay >= TimeSpan.Zero, + $"AuditLog:SiteRetention:{nameof(SiteAuditRetentionOptions.InitialDelay)} " + + $"({options.InitialDelay}) must be >= 0."); + } +} diff --git a/src/ZB.MOM.WW.ScadaBridge.AuditLog/Site/SqliteAuditWriterOptionsValidator.cs b/src/ZB.MOM.WW.ScadaBridge.AuditLog/Site/SqliteAuditWriterOptionsValidator.cs new file mode 100644 index 00000000..68c586ce --- /dev/null +++ b/src/ZB.MOM.WW.ScadaBridge.AuditLog/Site/SqliteAuditWriterOptionsValidator.cs @@ -0,0 +1,38 @@ +using ZB.MOM.WW.Configuration; + +namespace ZB.MOM.WW.ScadaBridge.AuditLog.Site; + +/// +/// Validates at host startup +/// (arch-review 08 round 2 NF4). The channel/batch/flush knobs feed the +/// background writer task; a zero anywhere stalls it (nothing drains, nothing +/// flushes), and an empty DatabasePath leaves the SQLite writer with no +/// backing store. +/// is intentionally NOT validated — a non-positive value has a documented +/// fall-back-to-30s contract in SiteAuditBacklogReporter. +/// +public sealed class SqliteAuditWriterOptionsValidator : OptionsValidatorBase +{ + /// + protected override void Validate(ValidationBuilder builder, SqliteAuditWriterOptions options) + { + builder.RequireThat(!string.IsNullOrWhiteSpace(options.DatabasePath), + $"AuditLog:SiteWriter:{nameof(SqliteAuditWriterOptions.DatabasePath)} must be a non-empty path."); + + builder.RequireThat(options.ChannelCapacity > 0, + $"AuditLog:SiteWriter:{nameof(SqliteAuditWriterOptions.ChannelCapacity)} " + + $"({options.ChannelCapacity}) must be > 0."); + + builder.RequireThat(options.BatchSize > 0, + $"AuditLog:SiteWriter:{nameof(SqliteAuditWriterOptions.BatchSize)} " + + $"({options.BatchSize}) must be > 0."); + + builder.RequireThat(options.FlushIntervalMs > 0, + $"AuditLog:SiteWriter:{nameof(SqliteAuditWriterOptions.FlushIntervalMs)} " + + $"({options.FlushIntervalMs}) must be > 0."); + + // BacklogPollIntervalSeconds is deliberately unchecked: a non-positive + // value falls back to the reporter's 30s default (see the option's + // remarks + register row 21 resolution). + } +} diff --git a/src/ZB.MOM.WW.ScadaBridge.AuditLog/Site/Telemetry/SiteAuditTelemetryOptionsValidator.cs b/src/ZB.MOM.WW.ScadaBridge.AuditLog/Site/Telemetry/SiteAuditTelemetryOptionsValidator.cs new file mode 100644 index 00000000..30ebee9d --- /dev/null +++ b/src/ZB.MOM.WW.ScadaBridge.AuditLog/Site/Telemetry/SiteAuditTelemetryOptionsValidator.cs @@ -0,0 +1,36 @@ +using ZB.MOM.WW.Configuration; + +namespace ZB.MOM.WW.ScadaBridge.AuditLog.Site.Telemetry; + +/// +/// Validates at host startup +/// (arch-review 08 round 2 NF4). SiteAuditTelemetryActor uses the two +/// interval knobs as raw scheduling delays (busy after a productive/faulted +/// drain, idle after an empty one), so a zero interval spins the drain loop and +/// a zero batch drains nothing. Idle must be >= busy: a shorter idle interval +/// inverts the actor's back-off-when-empty intent. +/// +public sealed class SiteAuditTelemetryOptionsValidator : OptionsValidatorBase +{ + /// + protected override void Validate(ValidationBuilder builder, SiteAuditTelemetryOptions options) + { + builder.RequireThat(options.BatchSize > 0, + $"AuditLog:SiteTelemetry:{nameof(SiteAuditTelemetryOptions.BatchSize)} " + + $"({options.BatchSize}) must be > 0."); + + builder.RequireThat(options.BusyIntervalSeconds > 0, + $"AuditLog:SiteTelemetry:{nameof(SiteAuditTelemetryOptions.BusyIntervalSeconds)} " + + $"({options.BusyIntervalSeconds}) must be > 0."); + + builder.RequireThat(options.IdleIntervalSeconds > 0, + $"AuditLog:SiteTelemetry:{nameof(SiteAuditTelemetryOptions.IdleIntervalSeconds)} " + + $"({options.IdleIntervalSeconds}) must be > 0."); + + builder.RequireThat(options.IdleIntervalSeconds >= options.BusyIntervalSeconds, + $"AuditLog:SiteTelemetry:{nameof(SiteAuditTelemetryOptions.IdleIntervalSeconds)} " + + $"({options.IdleIntervalSeconds}) must be >= " + + $"{nameof(SiteAuditTelemetryOptions.BusyIntervalSeconds)} ({options.BusyIntervalSeconds}) " + + "— the idle drain must back off at least as far as the busy drain."); + } +} diff --git a/src/ZB.MOM.WW.ScadaBridge.Communication/CommunicationOptionsValidator.cs b/src/ZB.MOM.WW.ScadaBridge.Communication/CommunicationOptionsValidator.cs index 31151b67..ef43f354 100644 --- a/src/ZB.MOM.WW.ScadaBridge.Communication/CommunicationOptionsValidator.cs +++ b/src/ZB.MOM.WW.ScadaBridge.Communication/CommunicationOptionsValidator.cs @@ -4,7 +4,7 @@ namespace ZB.MOM.WW.ScadaBridge.Communication; /// /// Validates at startup (ValidateOnStart) so a -/// malformed "Communication" appsettings section fails fast at boot with a +/// malformed "ScadaBridge:Communication" appsettings section fails fast at boot with a /// key-naming message instead of surfacing later at first Ask/gRPC use. Every /// timeout feeds a per-pattern Ask deadline or a gRPC keepalive/stream-lifetime /// setting, and a zero/negative value there produces an opaque runtime failure @@ -16,79 +16,79 @@ public sealed class CommunicationOptionsValidator : OptionsValidatorBase TimeSpan.Zero, - $"Communication:DeploymentTimeout must be a positive duration (was {options.DeploymentTimeout})."); + $"ScadaBridge:Communication:DeploymentTimeout must be a positive duration (was {options.DeploymentTimeout})."); builder.RequireThat(options.LifecycleTimeout > TimeSpan.Zero, - $"Communication:LifecycleTimeout must be a positive duration (was {options.LifecycleTimeout})."); + $"ScadaBridge:Communication:LifecycleTimeout must be a positive duration (was {options.LifecycleTimeout})."); builder.RequireThat(options.ArtifactDeploymentTimeout > TimeSpan.Zero, - $"Communication:ArtifactDeploymentTimeout must be a positive duration (was {options.ArtifactDeploymentTimeout})."); + $"ScadaBridge:Communication:ArtifactDeploymentTimeout must be a positive duration (was {options.ArtifactDeploymentTimeout})."); builder.RequireThat(options.QueryTimeout > TimeSpan.Zero, - $"Communication:QueryTimeout must be a positive duration (was {options.QueryTimeout})."); + $"ScadaBridge:Communication:QueryTimeout must be a positive duration (was {options.QueryTimeout})."); builder.RequireThat(options.IntegrationTimeout > TimeSpan.Zero, - $"Communication:IntegrationTimeout must be a positive duration (was {options.IntegrationTimeout})."); + $"ScadaBridge:Communication:IntegrationTimeout must be a positive duration (was {options.IntegrationTimeout})."); builder.RequireThat(options.DebugViewTimeout > TimeSpan.Zero, - $"Communication:DebugViewTimeout must be a positive duration (was {options.DebugViewTimeout})."); + $"ScadaBridge:Communication:DebugViewTimeout must be a positive duration (was {options.DebugViewTimeout})."); builder.RequireThat(options.HealthReportTimeout > TimeSpan.Zero, - $"Communication:HealthReportTimeout must be a positive duration (was {options.HealthReportTimeout})."); + $"ScadaBridge:Communication:HealthReportTimeout must be a positive duration (was {options.HealthReportTimeout})."); builder.RequireThat(options.NotificationForwardTimeout > TimeSpan.Zero, - $"Communication:NotificationForwardTimeout must be a positive duration (was {options.NotificationForwardTimeout})."); + $"ScadaBridge:Communication:NotificationForwardTimeout must be a positive duration (was {options.NotificationForwardTimeout})."); builder.RequireThat(options.GrpcKeepAlivePingDelay > TimeSpan.Zero, - $"Communication:GrpcKeepAlivePingDelay must be a positive duration (was {options.GrpcKeepAlivePingDelay})."); + $"ScadaBridge:Communication:GrpcKeepAlivePingDelay must be a positive duration (was {options.GrpcKeepAlivePingDelay})."); builder.RequireThat(options.GrpcKeepAlivePingTimeout > TimeSpan.Zero, - $"Communication:GrpcKeepAlivePingTimeout must be a positive duration (was {options.GrpcKeepAlivePingTimeout})."); + $"ScadaBridge:Communication:GrpcKeepAlivePingTimeout must be a positive duration (was {options.GrpcKeepAlivePingTimeout})."); builder.RequireThat(options.GrpcMaxStreamLifetime > TimeSpan.Zero, - $"Communication:GrpcMaxStreamLifetime must be a positive duration (was {options.GrpcMaxStreamLifetime})."); + $"ScadaBridge:Communication:GrpcMaxStreamLifetime must be a positive duration (was {options.GrpcMaxStreamLifetime})."); builder.RequireThat(options.TransportHeartbeatInterval > TimeSpan.Zero, - $"Communication:TransportHeartbeatInterval must be a positive duration (was {options.TransportHeartbeatInterval})."); + $"ScadaBridge:Communication:TransportHeartbeatInterval must be a positive duration (was {options.TransportHeartbeatInterval})."); builder.RequireThat(options.ApplicationHeartbeatInterval > TimeSpan.Zero, - $"Communication:ApplicationHeartbeatInterval must be a positive duration (was {options.ApplicationHeartbeatInterval})."); + $"ScadaBridge:Communication:ApplicationHeartbeatInterval must be a positive duration (was {options.ApplicationHeartbeatInterval})."); builder.RequireThat(options.TransportFailureThreshold > TimeSpan.Zero, - $"Communication:TransportFailureThreshold must be a positive duration (was {options.TransportFailureThreshold})."); + $"ScadaBridge:Communication:TransportFailureThreshold must be a positive duration (was {options.TransportFailureThreshold})."); builder.RequireThat(options.PendingDeploymentTtl > TimeSpan.Zero, - $"Communication:PendingDeploymentTtl must be a positive duration (was {options.PendingDeploymentTtl})."); + $"ScadaBridge:Communication:PendingDeploymentTtl must be a positive duration (was {options.PendingDeploymentTtl})."); builder.RequireThat(options.PendingDeploymentPurgeInterval > TimeSpan.Zero, - $"Communication:PendingDeploymentPurgeInterval must be a positive duration (was {options.PendingDeploymentPurgeInterval})."); + $"ScadaBridge:Communication:PendingDeploymentPurgeInterval must be a positive duration (was {options.PendingDeploymentPurgeInterval})."); builder.RequireThat(options.GrpcMaxConcurrentStreams > 0, - $"Communication:GrpcMaxConcurrentStreams must be positive (was {options.GrpcMaxConcurrentStreams})."); + $"ScadaBridge:Communication:GrpcMaxConcurrentStreams must be positive (was {options.GrpcMaxConcurrentStreams})."); // ── Aggregated live alarm cache (plan #10, Task 6) ─────────────────────── // Linger drives a Timer dueTime; TimeSpan.Zero is valid (stop the aggregator // immediately when the last viewer leaves), only a negative value is invalid. builder.RequireThat(options.LiveAlarmCacheLinger >= TimeSpan.Zero, - $"Communication:LiveAlarmCacheLinger must be zero or a positive duration (was {options.LiveAlarmCacheLinger})."); + $"ScadaBridge:Communication:LiveAlarmCacheLinger must be zero or a positive duration (was {options.LiveAlarmCacheLinger})."); // Reconcile interval is a periodic timer cadence AND the start-retry cadence; a // zero/negative value would spin or never fire. builder.RequireThat(options.LiveAlarmCacheReconcileInterval > TimeSpan.Zero, - $"Communication:LiveAlarmCacheReconcileInterval must be a positive duration (was {options.LiveAlarmCacheReconcileInterval})."); + $"ScadaBridge:Communication:LiveAlarmCacheReconcileInterval must be a positive duration (was {options.LiveAlarmCacheReconcileInterval})."); // Seed fan-out concurrency gates a SemaphoreSlim; at least 1, capped so a // fat-fingered config can't open an unbounded burst of cross-cluster Asks. builder.RequireThat(options.LiveAlarmCacheSeedConcurrency is >= 1 and <= 64, - $"Communication:LiveAlarmCacheSeedConcurrency must be between 1 and 64 (was {options.LiveAlarmCacheSeedConcurrency})."); + $"ScadaBridge:Communication:LiveAlarmCacheSeedConcurrency must be between 1 and 64 (was {options.LiveAlarmCacheSeedConcurrency})."); // Per-site viewer cap must admit at least one viewer, else the page could never go live. builder.RequireThat(options.LiveAlarmCacheMaxSubscribersPerSite >= 1, - $"Communication:LiveAlarmCacheMaxSubscribersPerSite must be at least 1 (was {options.LiveAlarmCacheMaxSubscribersPerSite})."); + $"ScadaBridge:Communication:LiveAlarmCacheMaxSubscribersPerSite must be at least 1 (was {options.LiveAlarmCacheMaxSubscribersPerSite})."); // Publish-coalescing window drives a single-shot timer; TimeSpan.Zero is valid // (publish per delta — legacy), only a negative value is invalid. builder.RequireThat(options.LiveAlarmCachePublishCoalesce >= TimeSpan.Zero, - $"Communication:LiveAlarmCachePublishCoalesce must be zero or a positive duration (was {options.LiveAlarmCachePublishCoalesce})."); + $"ScadaBridge:Communication:LiveAlarmCachePublishCoalesce must be zero or a positive duration (was {options.LiveAlarmCachePublishCoalesce})."); } } diff --git a/src/ZB.MOM.WW.ScadaBridge.Communication/ServiceCollectionExtensions.cs b/src/ZB.MOM.WW.ScadaBridge.Communication/ServiceCollectionExtensions.cs index 0fb9ccdd..27838bc4 100644 --- a/src/ZB.MOM.WW.ScadaBridge.Communication/ServiceCollectionExtensions.cs +++ b/src/ZB.MOM.WW.ScadaBridge.Communication/ServiceCollectionExtensions.cs @@ -12,8 +12,12 @@ public static class ServiceCollectionExtensions /// The same to allow chaining. public static IServiceCollection AddCommunication(this IServiceCollection services) { + // Canonical section is ScadaBridge:Communication — the section every real + // appsettings actually writes (arch-review 08 round 2 NF8). The Host used to + // supply the real values via a duplicate Configure<> binding; that duplicate is + // gone, so AddCommunication must bind the canonical section itself. services.AddOptions() - .BindConfiguration("Communication") + .BindConfiguration("ScadaBridge:Communication") .ValidateOnStart(); services.TryAddEnumerable( ServiceDescriptor.Singleton, CommunicationOptionsValidator>()); diff --git a/src/ZB.MOM.WW.ScadaBridge.DataConnectionLayer/DataConnectionOptionsValidator.cs b/src/ZB.MOM.WW.ScadaBridge.DataConnectionLayer/DataConnectionOptionsValidator.cs index 1379bddd..e80dce81 100644 --- a/src/ZB.MOM.WW.ScadaBridge.DataConnectionLayer/DataConnectionOptionsValidator.cs +++ b/src/ZB.MOM.WW.ScadaBridge.DataConnectionLayer/DataConnectionOptionsValidator.cs @@ -4,7 +4,7 @@ namespace ZB.MOM.WW.ScadaBridge.DataConnectionLayer; /// /// Validates at startup (ValidateOnStart) so a -/// malformed "DataConnectionLayer" appsettings section fails fast at boot with a +/// malformed "ScadaBridge:DataConnection" appsettings section fails fast at boot with a /// key-naming message instead of surfacing later at runtime. The intervals drive /// the reconnect/tag-resolution timers and the seed-read retry loop; a /// zero/negative duration (or a non-positive SeedReadMaxAttempts) would @@ -16,24 +16,24 @@ public sealed class DataConnectionOptionsValidator : OptionsValidatorBase TimeSpan.Zero, - $"DataConnectionLayer:ReconnectInterval must be a positive duration (was {options.ReconnectInterval})."); + $"ScadaBridge:DataConnection:ReconnectInterval must be a positive duration (was {options.ReconnectInterval})."); builder.RequireThat(options.TagResolutionRetryInterval > TimeSpan.Zero, - $"DataConnectionLayer:TagResolutionRetryInterval must be a positive duration (was {options.TagResolutionRetryInterval})."); + $"ScadaBridge:DataConnection:TagResolutionRetryInterval must be a positive duration (was {options.TagResolutionRetryInterval})."); builder.RequireThat(options.WriteTimeout > TimeSpan.Zero, - $"DataConnectionLayer:WriteTimeout must be a positive duration (was {options.WriteTimeout})."); + $"ScadaBridge:DataConnection:WriteTimeout must be a positive duration (was {options.WriteTimeout})."); builder.RequireThat(options.SeedReadTimeout > TimeSpan.Zero, - $"DataConnectionLayer:SeedReadTimeout must be a positive duration (was {options.SeedReadTimeout})."); + $"ScadaBridge:DataConnection:SeedReadTimeout must be a positive duration (was {options.SeedReadTimeout})."); builder.RequireThat(options.StableConnectionThreshold > TimeSpan.Zero, - $"DataConnectionLayer:StableConnectionThreshold must be a positive duration (was {options.StableConnectionThreshold})."); + $"ScadaBridge:DataConnection:StableConnectionThreshold must be a positive duration (was {options.StableConnectionThreshold})."); builder.RequireThat(options.SeedReadRetryDelay > TimeSpan.Zero, - $"DataConnectionLayer:SeedReadRetryDelay must be a positive duration (was {options.SeedReadRetryDelay})."); + $"ScadaBridge:DataConnection:SeedReadRetryDelay must be a positive duration (was {options.SeedReadRetryDelay})."); builder.RequireThat(options.SeedReadMaxAttempts > 0, - $"DataConnectionLayer:SeedReadMaxAttempts must be positive (was {options.SeedReadMaxAttempts})."); + $"ScadaBridge:DataConnection:SeedReadMaxAttempts must be positive (was {options.SeedReadMaxAttempts})."); } } diff --git a/src/ZB.MOM.WW.ScadaBridge.DataConnectionLayer/MxGatewayGlobalOptions.cs b/src/ZB.MOM.WW.ScadaBridge.DataConnectionLayer/MxGatewayGlobalOptions.cs index 190f1017..464f6782 100644 --- a/src/ZB.MOM.WW.ScadaBridge.DataConnectionLayer/MxGatewayGlobalOptions.cs +++ b/src/ZB.MOM.WW.ScadaBridge.DataConnectionLayer/MxGatewayGlobalOptions.cs @@ -1,8 +1,8 @@ namespace ZB.MOM.WW.ScadaBridge.DataConnectionLayer; /// -/// Deployment-wide MxGateway defaults, bound from the "MxGateway" section of -/// appsettings.json. Per-endpoint behavior lives on MxGatewayEndpointConfig. +/// Deployment-wide MxGateway defaults, bound from the "ScadaBridge:MxGateway" section +/// of appsettings.json. Per-endpoint behavior lives on MxGatewayEndpointConfig. /// public class MxGatewayGlobalOptions { diff --git a/src/ZB.MOM.WW.ScadaBridge.DataConnectionLayer/OpcUaGlobalOptions.cs b/src/ZB.MOM.WW.ScadaBridge.DataConnectionLayer/OpcUaGlobalOptions.cs index 85754ed0..d423005f 100644 --- a/src/ZB.MOM.WW.ScadaBridge.DataConnectionLayer/OpcUaGlobalOptions.cs +++ b/src/ZB.MOM.WW.ScadaBridge.DataConnectionLayer/OpcUaGlobalOptions.cs @@ -1,8 +1,8 @@ namespace ZB.MOM.WW.ScadaBridge.DataConnectionLayer; /// -/// Deployment-wide OPC UA application identity. Bound from the "OpcUa" section -/// of appsettings.json. Per-endpoint behavior lives on OpcUaEndpointConfig. +/// Deployment-wide OPC UA application identity. Bound from the "ScadaBridge:OpcUa" +/// section of appsettings.json. Per-endpoint behavior lives on OpcUaEndpointConfig. /// Empty paths fall back to a default under Path.GetTempPath() so dev runs /// work without explicit configuration. /// diff --git a/src/ZB.MOM.WW.ScadaBridge.DataConnectionLayer/ServiceCollectionExtensions.cs b/src/ZB.MOM.WW.ScadaBridge.DataConnectionLayer/ServiceCollectionExtensions.cs index 5f175071..f4268d9c 100644 --- a/src/ZB.MOM.WW.ScadaBridge.DataConnectionLayer/ServiceCollectionExtensions.cs +++ b/src/ZB.MOM.WW.ScadaBridge.DataConnectionLayer/ServiceCollectionExtensions.cs @@ -14,20 +14,26 @@ public static class ServiceCollectionExtensions /// The same instance for chaining. public static IServiceCollection AddDataConnectionLayer(this IServiceCollection services) { + // Canonical section is ScadaBridge:DataConnection — the section every real + // appsettings actually writes (arch-review 08 round 2 NF8). The Host used to + // supply the real values via a duplicate Configure<> binding; that duplicate is + // gone, so AddDataConnectionLayer must bind the canonical section itself. services.AddOptions() - .BindConfiguration("DataConnectionLayer") + .BindConfiguration("ScadaBridge:DataConnection") .ValidateOnStart(); services.TryAddEnumerable( ServiceDescriptor.Singleton, DataConnectionOptionsValidator>()); // OpcUaGlobalOptions / MxGatewayGlobalOptions carry only string identity/path // fields (no numeric or duration knobs), so there is nothing to range-validate - // — no dedicated validator is registered for them. + // — no dedicated validator is registered for them. Canonical sections are + // ScadaBridge:OpcUa / ScadaBridge:MxGateway (NF8): the bare "OpcUa"/"MxGateway" + // sections exist in no appsettings, so binding them read nothing. services.AddOptions() - .BindConfiguration("OpcUa"); + .BindConfiguration("ScadaBridge:OpcUa"); services.AddOptions() - .BindConfiguration("MxGateway"); + .BindConfiguration("ScadaBridge:MxGateway"); // Register the factory for protocol extensibility services.AddSingleton(); diff --git a/src/ZB.MOM.WW.ScadaBridge.Host/DatabaseOptionsValidator.cs b/src/ZB.MOM.WW.ScadaBridge.Host/DatabaseOptionsValidator.cs new file mode 100644 index 00000000..823ec2f1 --- /dev/null +++ b/src/ZB.MOM.WW.ScadaBridge.Host/DatabaseOptionsValidator.cs @@ -0,0 +1,28 @@ +using ZB.MOM.WW.Configuration; + +namespace ZB.MOM.WW.ScadaBridge.Host; + +/// +/// Validates at host startup (arch-review 08 +/// round 2 NF4). All three connection settings are nullable and role-dependent +/// (central needs the SQL Server strings, a site needs only the SQLite path), +/// so a null value is valid. A present-but-whitespace value is not — it +/// signals a mis-set config key that would fail opaquely at first DB use. +/// +public sealed class DatabaseOptionsValidator : OptionsValidatorBase +{ + /// + protected override void Validate(ValidationBuilder builder, DatabaseOptions options) + { + RequireNotWhitespace(builder, options.ConfigurationDb, nameof(DatabaseOptions.ConfigurationDb)); + RequireNotWhitespace(builder, options.MachineDataDb, nameof(DatabaseOptions.MachineDataDb)); + RequireNotWhitespace(builder, options.SiteDbPath, nameof(DatabaseOptions.SiteDbPath)); + } + + // null is valid (role-dependent); reject only a present-but-blank value. + private static void RequireNotWhitespace(ValidationBuilder builder, string? value, string field) + { + builder.RequireThat(value is null || !string.IsNullOrWhiteSpace(value), + $"ScadaBridge:Database:{field} is set but blank — either remove it or give it a real value."); + } +} diff --git a/src/ZB.MOM.WW.ScadaBridge.Host/LoggingOptionsValidator.cs b/src/ZB.MOM.WW.ScadaBridge.Host/LoggingOptionsValidator.cs new file mode 100644 index 00000000..1adbffdd --- /dev/null +++ b/src/ZB.MOM.WW.ScadaBridge.Host/LoggingOptionsValidator.cs @@ -0,0 +1,27 @@ +using Serilog.Events; +using ZB.MOM.WW.Configuration; + +namespace ZB.MOM.WW.ScadaBridge.Host; + +/// +/// Validates at host startup (arch-review 08 round +/// 2 NF4). is parsed into a Serilog +/// by LoggerConfigurationFactory.ParseLevel, +/// which silently falls back to Information for an unrecognised value. +/// This validator surfaces that misconfiguration at boot instead: a present, +/// non-blank value must name a real level (Verbose/Debug/Information/Warning/ +/// Error/Fatal, case-insensitive). A null/blank value is treated as "unset" +/// (defaults to Information), mirroring the parser. +/// +public sealed class LoggingOptionsValidator : OptionsValidatorBase +{ + /// + protected override void Validate(ValidationBuilder builder, LoggingOptions options) + { + builder.RequireThat( + string.IsNullOrWhiteSpace(options.MinimumLevel) + || Enum.TryParse(options.MinimumLevel, ignoreCase: true, out _), + $"ScadaBridge:Logging:{nameof(LoggingOptions.MinimumLevel)} ('{options.MinimumLevel}') " + + $"is not a recognised Serilog level. Valid: {string.Join(", ", Enum.GetNames())}."); + } +} diff --git a/src/ZB.MOM.WW.ScadaBridge.Host/NodeOptionsValidator.cs b/src/ZB.MOM.WW.ScadaBridge.Host/NodeOptionsValidator.cs new file mode 100644 index 00000000..1b5a7623 --- /dev/null +++ b/src/ZB.MOM.WW.ScadaBridge.Host/NodeOptionsValidator.cs @@ -0,0 +1,34 @@ +using ZB.MOM.WW.Configuration; + +namespace ZB.MOM.WW.ScadaBridge.Host; + +/// +/// Validates at host startup (arch-review 08 round 2 +/// NF4). The headline invariant: must be +/// non-empty — an empty value normalises to a NULL SourceNode audit +/// column, silently degrading the audit trail (the wonder-app-vd03 failure +/// mode), so the host must fail fast at boot instead. Ports are bounded to the +/// TCP range; 0 stays valid because it requests a dynamically-assigned +/// port (CompositionRootTests rely on this). +/// +public sealed class NodeOptionsValidator : OptionsValidatorBase +{ + /// + protected override void Validate(ValidationBuilder builder, NodeOptions options) + { + builder.RequireThat(!string.IsNullOrWhiteSpace(options.NodeName), + $"ScadaBridge:Node:{nameof(NodeOptions.NodeName)} must be a non-empty label " + + "— an empty node name stamps the SourceNode audit column NULL."); + + RequirePort(builder, options.RemotingPort, nameof(NodeOptions.RemotingPort)); + RequirePort(builder, options.GrpcPort, nameof(NodeOptions.GrpcPort)); + RequirePort(builder, options.MetricsPort, nameof(NodeOptions.MetricsPort)); + } + + // 0 stays valid (dynamic-port request); reject only out-of-TCP-range values. + private static void RequirePort(ValidationBuilder builder, int port, string field) + { + builder.RequireThat(port is >= 0 and <= 65_535, + $"ScadaBridge:Node:{field} ({port}) must be in [0, 65535]."); + } +} diff --git a/src/ZB.MOM.WW.ScadaBridge.Host/SiteServiceRegistration.cs b/src/ZB.MOM.WW.ScadaBridge.Host/SiteServiceRegistration.cs index 104b38fe..36327763 100644 --- a/src/ZB.MOM.WW.ScadaBridge.Host/SiteServiceRegistration.cs +++ b/src/ZB.MOM.WW.ScadaBridge.Host/SiteServiceRegistration.cs @@ -137,7 +137,21 @@ public static class SiteServiceRegistration services.TryAddEnumerable( ServiceDescriptor.Singleton, SiteRuntimeOptionsValidator>()); - services.Configure(config.GetSection("ScadaBridge:DataConnection")); + // OperationTrackingOptions for the site-local cached-call tracking store + // (registered by AddSiteRuntime). Bind + eagerly validate here — the store + // opens its SQLite connection string at construction, so a blank connection + // string should fail the host at boot with a key-naming message rather than + // throw opaquely on first resolve (arch-review 08r2 NF4/T8). + services.AddOptions() + .Bind(config.GetSection("ScadaBridge:OperationTracking")) + .ValidateOnStart(); + services.TryAddEnumerable( + ServiceDescriptor.Singleton, + SiteRuntime.Tracking.OperationTrackingOptionsValidator>()); + + // NF8: DataConnectionOptions is now bound by AddDataConnectionLayer() itself + // (canonical ScadaBridge:DataConnection section) — the duplicate Host binding + // that used to mask the SCE's wrong section name is deleted. services.AddOptions() .Bind(config.GetSection("ScadaBridge:StoreAndForward")) @@ -157,19 +171,38 @@ public static class SiteServiceRegistration /// Application configuration supplying the option values. public static void BindSharedOptions(IServiceCollection services, IConfiguration config) { - services.Configure(config.GetSection("ScadaBridge:Node")); + // Bind + eagerly validate: an empty NodeName would stamp the SourceNode audit + // column NULL, so fail the host at boot with a key-naming message instead of + // silently degrading the audit trail (arch-review 08r2 NF4). + services.AddOptions().Bind(config.GetSection("ScadaBridge:Node")).ValidateOnStart(); + services.TryAddEnumerable( + ServiceDescriptor.Singleton, NodeOptionsValidator>()); // Bind + eagerly validate: ClusterOptionsValidator is registered (TryAddEnumerable) // by the ClusterInfrastructure module, so chaining ValidateOnStart() here makes a bad // ScadaBridge:Cluster section fail fast at host build instead of lazily on first resolve. services.AddOptions().Bind(config.GetSection("ScadaBridge:Cluster")).ValidateOnStart(); - services.Configure(config.GetSection("ScadaBridge:Database")); - services.Configure(config.GetSection("ScadaBridge:Communication")); + // Bind + eagerly validate: a present-but-blank connection setting fails fast + // here rather than opaquely at first DB use (arch-review 08r2 NF4). + services.AddOptions().Bind(config.GetSection("ScadaBridge:Database")).ValidateOnStart(); + services.TryAddEnumerable( + ServiceDescriptor.Singleton, DatabaseOptionsValidator>()); + // NF8: CommunicationOptions is now bound by AddCommunication() itself (canonical + // ScadaBridge:Communication section) — the duplicate Host binding that used to mask + // the SCE's wrong section name is deleted. // Bind + eagerly validate: HealthMonitoringOptionsValidator is registered (TryAddEnumerable) // by the HealthMonitoring module, so chaining ValidateOnStart() here makes a bad // ScadaBridge:HealthMonitoring section fail fast at host build instead of lazily on first resolve. services.AddOptions().Bind(config.GetSection("ScadaBridge:HealthMonitoring")).ValidateOnStart(); - services.Configure(config.GetSection("ScadaBridge:Notification")); - services.Configure(config.GetSection("ScadaBridge:Logging")); + // NF8: NotificationOptions is bound by AddNotificationService() (central-only, + // canonical ScadaBridge:Notification section). Sites don't deliver notifications + // and have no IOptions consumer, so the redundant Host + // duplicate that bound it on both paths is deleted. + // Bind + eagerly validate: an unrecognised MinimumLevel would silently fall back + // to Information; fail fast at boot so the operator's intended floor is honoured + // (arch-review 08r2 NF4). + services.AddOptions().Bind(config.GetSection("ScadaBridge:Logging")).ValidateOnStart(); + services.TryAddEnumerable( + ServiceDescriptor.Singleton, LoggingOptionsValidator>()); // Audit Log — exposes ScadaBridge:Node:NodeName to downstream audit // writers so they can stamp the SourceNode column. Registered here in diff --git a/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/ServiceCollectionExtensions.cs b/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/ServiceCollectionExtensions.cs index 20b612da..7352e990 100644 --- a/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/ServiceCollectionExtensions.cs +++ b/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/ServiceCollectionExtensions.cs @@ -60,6 +60,19 @@ public static class ServiceCollectionExtensions }); services.AddSingleton(sp => sp.GetRequiredService()); + // Site-local cached-operation tracking store — the SQLite source of truth + // that Tracking.Status(TrackedOperationId) reads and the cached-call telemetry + // forwarder writes. Registered HERE (site-only) so the two comments that claim + // "the operational tracking store is registered by AddSiteRuntime" are true + // again (AuditLog SCE + AkkaHostedService). Before this (arch-review 08 round 2 + // NF4/T8) it had no DI registration anywhere in src/, so every consumer resolved + // it as null and silently ran degraded (no cached-drain, no PullSiteCalls + // reconciliation, Tracking.Status audit-only). Central roots never call + // AddSiteRuntime, so this stays absent on central — matching the SITE-ONLY + // contract. OperationTrackingOptions is bound + validated by the Host's + // SiteServiceRegistration site-options block. + services.AddSingleton(); + // Site-local repository implementations backed by SQLite services.AddScoped(); diff --git a/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Tracking/OperationTrackingOptionsValidator.cs b/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Tracking/OperationTrackingOptionsValidator.cs new file mode 100644 index 00000000..56ddc469 --- /dev/null +++ b/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Tracking/OperationTrackingOptionsValidator.cs @@ -0,0 +1,26 @@ +using ZB.MOM.WW.Configuration; + +namespace ZB.MOM.WW.ScadaBridge.SiteRuntime.Tracking; + +/// +/// Validates at host startup (arch-review +/// 08 round 2 NF4). opens the SQLite +/// connection string in its constructor, so an empty/blank connection string +/// must fail the host at boot rather than throw opaquely on first resolve. The +/// retention window must be positive — a zero would make the host's terminal-row +/// purge delete every tracking row. +/// +public sealed class OperationTrackingOptionsValidator : OptionsValidatorBase +{ + /// + protected override void Validate(ValidationBuilder builder, OperationTrackingOptions options) + { + builder.RequireThat(!string.IsNullOrWhiteSpace(options.ConnectionString), + $"ScadaBridge:OperationTracking:{nameof(OperationTrackingOptions.ConnectionString)} " + + "must be a non-empty SQLite connection string."); + + builder.RequireThat(options.RetentionDays > 0, + $"ScadaBridge:OperationTracking:{nameof(OperationTrackingOptions.RetentionDays)} " + + $"({options.RetentionDays}) must be > 0."); + } +} diff --git a/src/ZB.MOM.WW.ScadaBridge.Transport/Serialization/EntitySerializer.cs b/src/ZB.MOM.WW.ScadaBridge.Transport/Serialization/EntitySerializer.cs index a8503879..4dd35d79 100644 --- a/src/ZB.MOM.WW.ScadaBridge.Transport/Serialization/EntitySerializer.cs +++ b/src/ZB.MOM.WW.ScadaBridge.Transport/Serialization/EntitySerializer.cs @@ -550,8 +550,14 @@ public sealed class EntitySerializer }) .ToList(); - // Instances: resolve template + site by name/identifier. Areas don't travel - // (see export TODO), so AreaId stays null. Connection bindings keep their + // Instances: resolve template + site by name/identifier. Areas DO travel — + // by NAME on InstanceDto.AreaName (export resolves AreaId→AreaNameById; the + // importer resolves-or-creates the area under the target site). This helper + // still materializes AreaId = null for the same reason the connection-binding + // FK below is a placeholder: FromBundleContent is NOT on the importer's path + // (BundleImporter walks the raw DTO) and has no target-DB name→id map to + // resolve against. Round-trip fidelity for the area lives on the DTO, not on + // this materialized entity. Connection bindings keep their // resolved DataConnection NAME on the wire DTO — the importer reads // bindings from BundleContentDto.Instances[].ConnectionBindings directly and // resolves ConnectionName→the target environment's DataConnectionId at apply diff --git a/tests/ZB.MOM.WW.ScadaBridge.AuditLog.Tests/Central/AuditLogPartitionMaintenanceOptionsValidatorTests.cs b/tests/ZB.MOM.WW.ScadaBridge.AuditLog.Tests/Central/AuditLogPartitionMaintenanceOptionsValidatorTests.cs new file mode 100644 index 00000000..2b45be34 --- /dev/null +++ b/tests/ZB.MOM.WW.ScadaBridge.AuditLog.Tests/Central/AuditLogPartitionMaintenanceOptionsValidatorTests.cs @@ -0,0 +1,41 @@ +using ZB.MOM.WW.ScadaBridge.AuditLog.Central; + +namespace ZB.MOM.WW.ScadaBridge.AuditLog.Tests.Central; + +/// +/// Eager startup validation for +/// (arch-review 08 round 2 NF4). A zero tick interval spins the maintenance +/// hosted service; a zero look-ahead leaves the partition function without a +/// future monthly boundary, risking inserts into the unbounded tail partition. +/// +public class AuditLogPartitionMaintenanceOptionsValidatorTests +{ + [Fact] + public void DefaultOptions_AreValid() + { + var validator = new AuditLogPartitionMaintenanceOptionsValidator(); + Assert.True(validator.Validate(null, new AuditLogPartitionMaintenanceOptions()).Succeeded); + } + + [Fact] + public void ZeroIntervalSeconds_Fails() + { + var validator = new AuditLogPartitionMaintenanceOptionsValidator(); + var result = validator.Validate(null, + new AuditLogPartitionMaintenanceOptions { IntervalSeconds = 0 }); + Assert.False(result.Succeeded); + Assert.Contains(result.Failures!, + f => f.Contains(nameof(AuditLogPartitionMaintenanceOptions.IntervalSeconds), StringComparison.Ordinal)); + } + + [Fact] + public void ZeroLookaheadMonths_Fails() + { + var validator = new AuditLogPartitionMaintenanceOptionsValidator(); + var result = validator.Validate(null, + new AuditLogPartitionMaintenanceOptions { LookaheadMonths = 0 }); + Assert.False(result.Succeeded); + Assert.Contains(result.Failures!, + f => f.Contains(nameof(AuditLogPartitionMaintenanceOptions.LookaheadMonths), StringComparison.Ordinal)); + } +} diff --git a/tests/ZB.MOM.WW.ScadaBridge.AuditLog.Tests/Central/AuditLogPurgeOptionsValidatorTests.cs b/tests/ZB.MOM.WW.ScadaBridge.AuditLog.Tests/Central/AuditLogPurgeOptionsValidatorTests.cs new file mode 100644 index 00000000..d2c05964 --- /dev/null +++ b/tests/ZB.MOM.WW.ScadaBridge.AuditLog.Tests/Central/AuditLogPurgeOptionsValidatorTests.cs @@ -0,0 +1,53 @@ +using ZB.MOM.WW.ScadaBridge.AuditLog.Central; + +namespace ZB.MOM.WW.ScadaBridge.AuditLog.Tests.Central; + +/// +/// Eager startup validation for +/// (arch-review 08 round 2 NF4). The class self-clamps at resolve time, but a +/// plainly-wrong config (zero cadence, zero batch, zero command timeout) should +/// still fail the boot so the operator learns immediately. +/// is test-only and left +/// unvalidated. +/// +public class AuditLogPurgeOptionsValidatorTests +{ + [Fact] + public void DefaultOptions_AreValid() + { + var validator = new AuditLogPurgeOptionsValidator(); + Assert.True(validator.Validate(null, new AuditLogPurgeOptions()).Succeeded); + } + + [Fact] + public void ZeroIntervalHours_Fails() + { + var validator = new AuditLogPurgeOptionsValidator(); + var result = validator.Validate(null, new AuditLogPurgeOptions { IntervalHours = 0 }); + Assert.False(result.Succeeded); + Assert.Contains(result.Failures!, + f => f.Contains(nameof(AuditLogPurgeOptions.IntervalHours), StringComparison.Ordinal)); + } + + [Fact] + public void ZeroChannelPurgeBatchSize_Fails() + { + var validator = new AuditLogPurgeOptionsValidator(); + var result = validator.Validate(null, + new AuditLogPurgeOptions { ChannelPurgeBatchSizeConfigured = 0 }); + Assert.False(result.Succeeded); + Assert.Contains(result.Failures!, + f => f.Contains(nameof(AuditLogPurgeOptions.ChannelPurgeBatchSizeConfigured), StringComparison.Ordinal)); + } + + [Fact] + public void ZeroMaintenanceCommandTimeout_Fails() + { + var validator = new AuditLogPurgeOptionsValidator(); + var result = validator.Validate(null, + new AuditLogPurgeOptions { MaintenanceCommandTimeoutMinutes = 0 }); + Assert.False(result.Succeeded); + Assert.Contains(result.Failures!, + f => f.Contains(nameof(AuditLogPurgeOptions.MaintenanceCommandTimeoutMinutes), StringComparison.Ordinal)); + } +} diff --git a/tests/ZB.MOM.WW.ScadaBridge.AuditLog.Tests/Central/SiteAuditReconciliationOptionsValidatorTests.cs b/tests/ZB.MOM.WW.ScadaBridge.AuditLog.Tests/Central/SiteAuditReconciliationOptionsValidatorTests.cs new file mode 100644 index 00000000..4d333045 --- /dev/null +++ b/tests/ZB.MOM.WW.ScadaBridge.AuditLog.Tests/Central/SiteAuditReconciliationOptionsValidatorTests.cs @@ -0,0 +1,54 @@ +using ZB.MOM.WW.ScadaBridge.AuditLog.Central; + +namespace ZB.MOM.WW.ScadaBridge.AuditLog.Tests.Central; + +/// +/// Eager startup validation for +/// (arch-review 08 round 2 NF4). A zero tick interval spins the reconciliation +/// singleton, a zero batch pulls nothing, and a zero stalled-cycle threshold +/// would fire the stalled signal on the first non-draining cycle. +/// +/// is test-only and left unvalidated. +/// +public class SiteAuditReconciliationOptionsValidatorTests +{ + [Fact] + public void DefaultOptions_AreValid() + { + var validator = new SiteAuditReconciliationOptionsValidator(); + Assert.True(validator.Validate(null, new SiteAuditReconciliationOptions()).Succeeded); + } + + [Fact] + public void ZeroReconciliationInterval_Fails() + { + var validator = new SiteAuditReconciliationOptionsValidator(); + var result = validator.Validate(null, + new SiteAuditReconciliationOptions { ReconciliationIntervalSeconds = 0 }); + Assert.False(result.Succeeded); + Assert.Contains(result.Failures!, + f => f.Contains(nameof(SiteAuditReconciliationOptions.ReconciliationIntervalSeconds), StringComparison.Ordinal)); + } + + [Fact] + public void ZeroBatchSize_Fails() + { + var validator = new SiteAuditReconciliationOptionsValidator(); + var result = validator.Validate(null, + new SiteAuditReconciliationOptions { BatchSize = 0 }); + Assert.False(result.Succeeded); + Assert.Contains(result.Failures!, + f => f.Contains(nameof(SiteAuditReconciliationOptions.BatchSize), StringComparison.Ordinal)); + } + + [Fact] + public void ZeroStalledCycles_Fails() + { + var validator = new SiteAuditReconciliationOptionsValidator(); + var result = validator.Validate(null, + new SiteAuditReconciliationOptions { StalledAfterNonDrainingCycles = 0 }); + Assert.False(result.Succeeded); + Assert.Contains(result.Failures!, + f => f.Contains(nameof(SiteAuditReconciliationOptions.StalledAfterNonDrainingCycles), StringComparison.Ordinal)); + } +} diff --git a/tests/ZB.MOM.WW.ScadaBridge.AuditLog.Tests/Site/SiteAuditRetentionOptionsValidatorTests.cs b/tests/ZB.MOM.WW.ScadaBridge.AuditLog.Tests/Site/SiteAuditRetentionOptionsValidatorTests.cs new file mode 100644 index 00000000..42c8d777 --- /dev/null +++ b/tests/ZB.MOM.WW.ScadaBridge.AuditLog.Tests/Site/SiteAuditRetentionOptionsValidatorTests.cs @@ -0,0 +1,53 @@ +using ZB.MOM.WW.ScadaBridge.AuditLog.Site; + +namespace ZB.MOM.WW.ScadaBridge.AuditLog.Tests.Site; + +/// +/// Eager startup validation for +/// (arch-review 08 round 2 NF4). Although the option class self-clamps at +/// resolve time, the validator still fails a boot with a plainly-wrong config +/// (zero retention window, zero/negative purge period, negative initial delay) +/// so the operator learns at startup rather than trusting a silent clamp. +/// PurgeIntervalOverride is test-only and left unvalidated. +/// +public class SiteAuditRetentionOptionsValidatorTests +{ + [Fact] + public void DefaultOptions_AreValid() + { + var validator = new SiteAuditRetentionOptionsValidator(); + Assert.True(validator.Validate(null, new SiteAuditRetentionOptions()).Succeeded); + } + + [Fact] + public void ZeroRetentionDays_Fails() + { + var validator = new SiteAuditRetentionOptionsValidator(); + var result = validator.Validate(null, new SiteAuditRetentionOptions { RetentionDays = 0 }); + Assert.False(result.Succeeded); + Assert.Contains(result.Failures!, + f => f.Contains(nameof(SiteAuditRetentionOptions.RetentionDays), StringComparison.Ordinal)); + } + + [Fact] + public void ZeroPurgeInterval_Fails() + { + var validator = new SiteAuditRetentionOptionsValidator(); + var result = validator.Validate(null, + new SiteAuditRetentionOptions { PurgeInterval = TimeSpan.Zero }); + Assert.False(result.Succeeded); + Assert.Contains(result.Failures!, + f => f.Contains(nameof(SiteAuditRetentionOptions.PurgeInterval), StringComparison.Ordinal)); + } + + [Fact] + public void NegativeInitialDelay_Fails() + { + var validator = new SiteAuditRetentionOptionsValidator(); + var result = validator.Validate(null, + new SiteAuditRetentionOptions { InitialDelay = TimeSpan.FromSeconds(-1) }); + Assert.False(result.Succeeded); + Assert.Contains(result.Failures!, + f => f.Contains(nameof(SiteAuditRetentionOptions.InitialDelay), StringComparison.Ordinal)); + } +} diff --git a/tests/ZB.MOM.WW.ScadaBridge.AuditLog.Tests/Site/SiteAuditTelemetryOptionsValidatorTests.cs b/tests/ZB.MOM.WW.ScadaBridge.AuditLog.Tests/Site/SiteAuditTelemetryOptionsValidatorTests.cs new file mode 100644 index 00000000..4d7c05a8 --- /dev/null +++ b/tests/ZB.MOM.WW.ScadaBridge.AuditLog.Tests/Site/SiteAuditTelemetryOptionsValidatorTests.cs @@ -0,0 +1,61 @@ +using ZB.MOM.WW.ScadaBridge.AuditLog.Site.Telemetry; + +namespace ZB.MOM.WW.ScadaBridge.AuditLog.Tests.Site; + +/// +/// Eager startup validation for +/// (arch-review 08 round 2 NF4). The drain-loop cadences are used as raw +/// scheduling delays in SiteAuditTelemetryActor; a zero interval spins +/// the drain and a zero batch drains nothing. Idle must back off at least as +/// far as busy (idle < busy inverts the actor's intent). +/// +public class SiteAuditTelemetryOptionsValidatorTests +{ + [Fact] + public void DefaultOptions_AreValid() + { + var validator = new SiteAuditTelemetryOptionsValidator(); + Assert.True(validator.Validate(null, new SiteAuditTelemetryOptions()).Succeeded); + } + + [Fact] + public void ZeroBatchSize_Fails() + { + var validator = new SiteAuditTelemetryOptionsValidator(); + var result = validator.Validate(null, new SiteAuditTelemetryOptions { BatchSize = 0 }); + Assert.False(result.Succeeded); + Assert.Contains(result.Failures!, + f => f.Contains(nameof(SiteAuditTelemetryOptions.BatchSize), StringComparison.Ordinal)); + } + + [Fact] + public void ZeroBusyInterval_Fails() + { + var validator = new SiteAuditTelemetryOptionsValidator(); + var result = validator.Validate(null, new SiteAuditTelemetryOptions { BusyIntervalSeconds = 0 }); + Assert.False(result.Succeeded); + Assert.Contains(result.Failures!, + f => f.Contains(nameof(SiteAuditTelemetryOptions.BusyIntervalSeconds), StringComparison.Ordinal)); + } + + [Fact] + public void ZeroIdleInterval_Fails() + { + var validator = new SiteAuditTelemetryOptionsValidator(); + var result = validator.Validate(null, new SiteAuditTelemetryOptions { IdleIntervalSeconds = 0 }); + Assert.False(result.Succeeded); + Assert.Contains(result.Failures!, + f => f.Contains(nameof(SiteAuditTelemetryOptions.IdleIntervalSeconds), StringComparison.Ordinal)); + } + + [Fact] + public void IdleShorterThanBusy_Fails() + { + var validator = new SiteAuditTelemetryOptionsValidator(); + var result = validator.Validate(null, + new SiteAuditTelemetryOptions { BusyIntervalSeconds = 30, IdleIntervalSeconds = 5 }); + Assert.False(result.Succeeded); + Assert.Contains(result.Failures!, + f => f.Contains(nameof(SiteAuditTelemetryOptions.IdleIntervalSeconds), StringComparison.Ordinal)); + } +} diff --git a/tests/ZB.MOM.WW.ScadaBridge.AuditLog.Tests/Site/SqliteAuditWriterOptionsValidatorTests.cs b/tests/ZB.MOM.WW.ScadaBridge.AuditLog.Tests/Site/SqliteAuditWriterOptionsValidatorTests.cs new file mode 100644 index 00000000..05ba0570 --- /dev/null +++ b/tests/ZB.MOM.WW.ScadaBridge.AuditLog.Tests/Site/SqliteAuditWriterOptionsValidatorTests.cs @@ -0,0 +1,72 @@ +using ZB.MOM.WW.ScadaBridge.AuditLog.Site; + +namespace ZB.MOM.WW.ScadaBridge.AuditLog.Tests.Site; + +/// +/// Eager startup validation for +/// (arch-review 08 round 2 NF4). The site hot-path writer's channel/batch/flush +/// knobs must be positive or the background writer task cannot make progress; +/// an empty DatabasePath would leave the SQLite writer with nowhere to go. +/// BacklogPollIntervalSeconds is deliberately NOT validated — a +/// non-positive value has a documented fall-back-to-30s contract. +/// +public class SqliteAuditWriterOptionsValidatorTests +{ + [Fact] + public void DefaultOptions_AreValid() + { + var validator = new SqliteAuditWriterOptionsValidator(); + Assert.True(validator.Validate(null, new SqliteAuditWriterOptions()).Succeeded); + } + + [Fact] + public void EmptyDatabasePath_Fails() + { + var validator = new SqliteAuditWriterOptionsValidator(); + var result = validator.Validate(null, new SqliteAuditWriterOptions { DatabasePath = "" }); + Assert.False(result.Succeeded); + Assert.Contains(result.Failures!, + f => f.Contains(nameof(SqliteAuditWriterOptions.DatabasePath), StringComparison.Ordinal)); + } + + [Fact] + public void ZeroChannelCapacity_Fails() + { + var validator = new SqliteAuditWriterOptionsValidator(); + var result = validator.Validate(null, new SqliteAuditWriterOptions { ChannelCapacity = 0 }); + Assert.False(result.Succeeded); + Assert.Contains(result.Failures!, + f => f.Contains(nameof(SqliteAuditWriterOptions.ChannelCapacity), StringComparison.Ordinal)); + } + + [Fact] + public void ZeroBatchSize_Fails() + { + var validator = new SqliteAuditWriterOptionsValidator(); + var result = validator.Validate(null, new SqliteAuditWriterOptions { BatchSize = 0 }); + Assert.False(result.Succeeded); + Assert.Contains(result.Failures!, + f => f.Contains(nameof(SqliteAuditWriterOptions.BatchSize), StringComparison.Ordinal)); + } + + [Fact] + public void ZeroFlushInterval_Fails() + { + var validator = new SqliteAuditWriterOptionsValidator(); + var result = validator.Validate(null, new SqliteAuditWriterOptions { FlushIntervalMs = 0 }); + Assert.False(result.Succeeded); + Assert.Contains(result.Failures!, + f => f.Contains(nameof(SqliteAuditWriterOptions.FlushIntervalMs), StringComparison.Ordinal)); + } + + [Theory] + [InlineData(0)] + [InlineData(-1)] + public void NonPositiveBacklogPollInterval_StillValid(int value) + { + // Documented fall-back-to-30s contract — must NOT be rejected. + var validator = new SqliteAuditWriterOptionsValidator(); + Assert.True(validator.Validate(null, + new SqliteAuditWriterOptions { BacklogPollIntervalSeconds = value }).Succeeded); + } +} diff --git a/tests/ZB.MOM.WW.ScadaBridge.Communication.Tests/CommunicationOptionsBindingTests.cs b/tests/ZB.MOM.WW.ScadaBridge.Communication.Tests/CommunicationOptionsBindingTests.cs new file mode 100644 index 00000000..ee9e292a --- /dev/null +++ b/tests/ZB.MOM.WW.ScadaBridge.Communication.Tests/CommunicationOptionsBindingTests.cs @@ -0,0 +1,30 @@ +using Microsoft.Extensions.Configuration; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Options; + +namespace ZB.MOM.WW.ScadaBridge.Communication.Tests; + +/// +/// Locks the config section +/// binds (arch-review 08 round 2 NF8). Every real appsettings nests this section under +/// ScadaBridge:; binding a root-level Communication section reads config +/// that no deployment writes — the drift the Host duplicate was silently masking. +/// AddCommunication alone must bind the canonical ScadaBridge:Communication section. +/// +public class CommunicationOptionsBindingTests +{ + [Fact] + public void AddCommunication_BindsTheScadaBridgeCommunicationSection() + { + var config = new ConfigurationBuilder().AddInMemoryCollection(new Dictionary + { + ["ScadaBridge:Communication:DeploymentTimeout"] = "00:01:23", + }).Build(); + var services = new ServiceCollection(); + services.AddSingleton(config); + services.AddCommunication(); + using var sp = services.BuildServiceProvider(); + Assert.Equal(TimeSpan.FromSeconds(83), + sp.GetRequiredService>().Value.DeploymentTimeout); + } +} diff --git a/tests/ZB.MOM.WW.ScadaBridge.DataConnectionLayer.Tests/DataConnectionOptionsBindingTests.cs b/tests/ZB.MOM.WW.ScadaBridge.DataConnectionLayer.Tests/DataConnectionOptionsBindingTests.cs new file mode 100644 index 00000000..59ec2a02 --- /dev/null +++ b/tests/ZB.MOM.WW.ScadaBridge.DataConnectionLayer.Tests/DataConnectionOptionsBindingTests.cs @@ -0,0 +1,30 @@ +using Microsoft.Extensions.Configuration; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Options; + +namespace ZB.MOM.WW.ScadaBridge.DataConnectionLayer.Tests; + +/// +/// Locks the config section +/// binds (arch-review 08 round 2 NF8). Every real appsettings nests this section under +/// ScadaBridge:; binding a root-level DataConnectionLayer section reads config +/// that no deployment writes — the drift the Host duplicate was silently masking. +/// AddDataConnectionLayer alone must bind the canonical ScadaBridge:DataConnection section. +/// +public class DataConnectionOptionsBindingTests +{ + [Fact] + public void AddDataConnectionLayer_BindsTheScadaBridgeDataConnectionSection() + { + var config = new ConfigurationBuilder().AddInMemoryCollection(new Dictionary + { + ["ScadaBridge:DataConnection:ReconnectInterval"] = "00:00:42", + }).Build(); + var services = new ServiceCollection(); + services.AddSingleton(config); + services.AddDataConnectionLayer(); + using var sp = services.BuildServiceProvider(); + Assert.Equal(TimeSpan.FromSeconds(42), + sp.GetRequiredService>().Value.ReconnectInterval); + } +} diff --git a/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/ActorPathTests.cs b/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/ActorPathTests.cs index 4bd84ab9..3cd1a9de 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/ActorPathTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/ActorPathTests.cs @@ -48,6 +48,7 @@ public class CentralActorPathTests : IAsyncLifetime { config.AddInMemoryCollection(new Dictionary { + ["ScadaBridge:Node:NodeName"] = "central-a", ["ScadaBridge:Node:NodeHostname"] = "localhost", ["ScadaBridge:Node:RemotingPort"] = "0", ["ScadaBridge:Cluster:SeedNodes:0"] = "akka.tcp://scadabridge@localhost:25510", @@ -171,6 +172,7 @@ public class SiteActorPathTests : IAsyncLifetime config.AddInMemoryCollection(new Dictionary { ["ScadaBridge:Node:Role"] = "Site", + ["ScadaBridge:Node:NodeName"] = "node-a", ["ScadaBridge:Node:NodeHostname"] = "localhost", ["ScadaBridge:Node:SiteId"] = "TestSite", ["ScadaBridge:Node:RemotingPort"] = "0", diff --git a/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/AkkaHostedServiceAuditWiringTests.cs b/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/AkkaHostedServiceAuditWiringTests.cs index 9e9cbf61..5fad810a 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/AkkaHostedServiceAuditWiringTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/AkkaHostedServiceAuditWiringTests.cs @@ -114,6 +114,7 @@ public class CentralAuditWiringTests : IDisposable { config.AddInMemoryCollection(new Dictionary { + ["ScadaBridge:Node:NodeName"] = "central-a", ["ScadaBridge:Node:NodeHostname"] = "localhost", ["ScadaBridge:Node:RemotingPort"] = "0", ["ScadaBridge:Cluster:SeedNodes:0"] = "akka.tcp://scadabridge@localhost:2551", @@ -294,6 +295,7 @@ public class SiteAuditWiringTests : IDisposable builder.Configuration.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", diff --git a/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/CompositionRootTests.cs b/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/CompositionRootTests.cs index f054b651..0e871b99 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/CompositionRootTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/CompositionRootTests.cs @@ -108,6 +108,7 @@ public class CentralCompositionRootTests : IDisposable { config.AddInMemoryCollection(new Dictionary { + ["ScadaBridge:Node:NodeName"] = "central-a", ["ScadaBridge:Node:NodeHostname"] = "localhost", ["ScadaBridge:Node:RemotingPort"] = "0", ["ScadaBridge:Cluster:SeedNodes:0"] = "akka.tcp://scadabridge@localhost:2551", @@ -338,21 +339,28 @@ public class SiteCompositionRootTests : IDisposable { private readonly WebApplication _host; private readonly string _tempDbPath; + private readonly string _tempTrackingDbPath; public SiteCompositionRootTests() { _tempDbPath = Path.Combine(Path.GetTempPath(), $"scadabridge_test_{Guid.NewGuid()}.db"); + _tempTrackingDbPath = Path.Combine(Path.GetTempPath(), $"scadabridge_track_{Guid.NewGuid()}.db"); var builder = WebApplication.CreateBuilder(); builder.Configuration.Sources.Clear(); builder.Configuration.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", ["ScadaBridge:Database:SiteDbPath"] = _tempDbPath, + // Point the site-local operation-tracking SQLite store (the ctor opens/creates + // the file eagerly) at a temp path so resolving IOperationTrackingStore in the + // composition-root test does not litter site-tracking.db in the test cwd. + ["ScadaBridge:OperationTracking:ConnectionString"] = $"Data Source={_tempTrackingDbPath}", // ClusterOptions requires at least one seed node (ClusterOptionsValidator). ["ScadaBridge:Cluster:SeedNodes:0"] = "akka.tcp://scadabridge@localhost:2551", ["ScadaBridge:Cluster:SeedNodes:1"] = "akka.tcp://scadabridge@localhost:2552", @@ -375,6 +383,20 @@ public class SiteCompositionRootTests : IDisposable { (_host as IDisposable)?.Dispose(); try { File.Delete(_tempDbPath); } catch { /* best effort */ } + try { File.Delete(_tempTrackingDbPath); } catch { /* best effort */ } + } + + [Fact] + public void Site_Resolves_IOperationTrackingStore() + { + // Site Call Audit's site-local source of truth. AkkaHostedService and + // ScriptRuntimeContext resolve this with GetService and silently degrade + // when absent (audit-only telemetry, no cached-drain, no PullSiteCalls) — + // so its presence on the SITE composition root must be locked here + // (arch-review 08 round 2 NF4/T8: AddSiteRuntime never registered it, + // despite two comments claiming it did). + var store = _host.Services.GetService(); + Assert.NotNull(store); } // --- Singletons --- diff --git a/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/HealthCheckTests.cs b/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/HealthCheckTests.cs index 1a17b58c..298d959b 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/HealthCheckTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/HealthCheckTests.cs @@ -44,6 +44,7 @@ public class HealthCheckTests : IDisposable { config.AddInMemoryCollection(new Dictionary { + ["ScadaBridge:Node:NodeName"] = "central-a", ["ScadaBridge:Node:NodeHostname"] = "localhost", ["ScadaBridge:Node:RemotingPort"] = "0", ["ScadaBridge:Cluster:SeedNodes:0"] = "akka.tcp://scadabridge@localhost:2551", diff --git a/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/HostStartupTests.cs b/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/HostStartupTests.cs index d1286046..2ff6c6d7 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/HostStartupTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/HostStartupTests.cs @@ -41,6 +41,7 @@ public class HostStartupTests : IDisposable { config.AddInMemoryCollection(new Dictionary { + ["ScadaBridge:Node:NodeName"] = "central-a", ["ScadaBridge:Node:NodeHostname"] = "localhost", ["ScadaBridge:Node:RemotingPort"] = "0", ["ScadaBridge:Cluster:SeedNodes:0"] = "akka.tcp://scadabridge@localhost:2551", @@ -74,6 +75,7 @@ public class HostStartupTests : IDisposable builder.Configuration.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", @@ -103,6 +105,7 @@ public class HostStartupTests : IDisposable builder.Configuration.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", diff --git a/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/MetricsEndpointTests.cs b/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/MetricsEndpointTests.cs index 90118bf4..f5aeb8ee 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/MetricsEndpointTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/MetricsEndpointTests.cs @@ -40,6 +40,7 @@ public class MetricsEndpointTests : IDisposable { config.AddInMemoryCollection(new Dictionary { + ["ScadaBridge:Node:NodeName"] = "central-a", ["ScadaBridge:Node:NodeHostname"] = "localhost", ["ScadaBridge:Node:RemotingPort"] = "0", ["ScadaBridge:Cluster:SeedNodes:0"] = "akka.tcp://scadabridge@localhost:2551", diff --git a/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/NodeOptionsValidatorTests.cs b/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/NodeOptionsValidatorTests.cs new file mode 100644 index 00000000..3a6267f8 --- /dev/null +++ b/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/NodeOptionsValidatorTests.cs @@ -0,0 +1,107 @@ +using ZB.MOM.WW.ScadaBridge.Host; + +namespace ZB.MOM.WW.ScadaBridge.Host.Tests; + +/// +/// Eager startup validation for the shared Host options (arch-review 08 round 2 +/// NF4). Covers , +/// , and +/// in one file. The headline case: an +/// empty normalises to a NULL SourceNode +/// audit column, so it must fail the host at boot rather than silently degrade +/// the audit trail. +/// +public class NodeOptionsValidatorTests +{ + // ---- NodeOptions ---- + + [Fact] + public void Node_DefaultsWithNodeName_AreValid() + { + var validator = new NodeOptionsValidator(); + Assert.True(validator.Validate(null, new NodeOptions { NodeName = "node-a" }).Succeeded); + } + + [Theory] + [InlineData("")] + [InlineData(" ")] + public void Node_EmptyOrWhitespaceNodeName_Fails(string nodeName) + { + var validator = new NodeOptionsValidator(); + var result = validator.Validate(null, new NodeOptions { NodeName = nodeName }); + Assert.False(result.Succeeded); + Assert.Contains(result.Failures!, + f => f.Contains(nameof(NodeOptions.NodeName), StringComparison.Ordinal)); + } + + [Fact] + public void Node_ZeroRemotingPort_IsValid() + { + // CompositionRootTests uses RemotingPort = 0 to request a dynamic port. + var validator = new NodeOptionsValidator(); + Assert.True(validator.Validate(null, + new NodeOptions { NodeName = "node-a", RemotingPort = 0 }).Succeeded); + } + + [Theory] + [InlineData(-1)] + [InlineData(65_536)] + public void Node_PortOutOfRange_Fails(int port) + { + var validator = new NodeOptionsValidator(); + var result = validator.Validate(null, + new NodeOptions { NodeName = "node-a", GrpcPort = port }); + Assert.False(result.Succeeded); + Assert.Contains(result.Failures!, + f => f.Contains(nameof(NodeOptions.GrpcPort), StringComparison.Ordinal)); + } + + // ---- DatabaseOptions ---- + + [Fact] + public void Database_AllNull_IsValid() + { + // All three fields are nullable and role-dependent; null stays valid. + var validator = new DatabaseOptionsValidator(); + Assert.True(validator.Validate(null, new DatabaseOptions()).Succeeded); + } + + [Fact] + public void Database_WhitespaceValue_Fails() + { + var validator = new DatabaseOptionsValidator(); + var result = validator.Validate(null, new DatabaseOptions { ConfigurationDb = " " }); + Assert.False(result.Succeeded); + Assert.Contains(result.Failures!, + f => f.Contains(nameof(DatabaseOptions.ConfigurationDb), StringComparison.Ordinal)); + } + + // ---- LoggingOptions ---- + + [Fact] + public void Logging_DefaultInformation_IsValid() + { + var validator = new LoggingOptionsValidator(); + Assert.True(validator.Validate(null, new LoggingOptions()).Succeeded); + } + + [Theory] + [InlineData("Verbose")] + [InlineData("debug")] + [InlineData("FATAL")] + public void Logging_RecognisedLevel_IsValid(string level) + { + var validator = new LoggingOptionsValidator(); + Assert.True(validator.Validate(null, new LoggingOptions { MinimumLevel = level }).Succeeded); + } + + [Fact] + public void Logging_UnknownLevel_Fails() + { + var validator = new LoggingOptionsValidator(); + var result = validator.Validate(null, new LoggingOptions { MinimumLevel = "Chatty" }); + Assert.False(result.Succeeded); + Assert.Contains(result.Failures!, + f => f.Contains(nameof(LoggingOptions.MinimumLevel), StringComparison.Ordinal)); + } +} diff --git a/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Tracking/OperationTrackingOptionsValidatorTests.cs b/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Tracking/OperationTrackingOptionsValidatorTests.cs new file mode 100644 index 00000000..519fc7a7 --- /dev/null +++ b/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Tracking/OperationTrackingOptionsValidatorTests.cs @@ -0,0 +1,40 @@ +using ZB.MOM.WW.ScadaBridge.SiteRuntime.Tracking; + +namespace ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests.Tracking; + +/// +/// Eager startup validation for +/// (arch-review 08 round 2 NF4). The store opens the SQLite connection string at +/// construction, so an empty connection string must fail the host at boot; the +/// retention window must be positive or terminal-row purge would delete +/// everything. +/// +public class OperationTrackingOptionsValidatorTests +{ + [Fact] + public void DefaultOptions_AreValid() + { + var validator = new OperationTrackingOptionsValidator(); + Assert.True(validator.Validate(null, new OperationTrackingOptions()).Succeeded); + } + + [Fact] + public void EmptyConnectionString_Fails() + { + var validator = new OperationTrackingOptionsValidator(); + var result = validator.Validate(null, new OperationTrackingOptions { ConnectionString = "" }); + Assert.False(result.Succeeded); + Assert.Contains(result.Failures!, + f => f.Contains(nameof(OperationTrackingOptions.ConnectionString), StringComparison.Ordinal)); + } + + [Fact] + public void ZeroRetentionDays_Fails() + { + var validator = new OperationTrackingOptionsValidator(); + var result = validator.Validate(null, new OperationTrackingOptions { RetentionDays = 0 }); + Assert.False(result.Succeeded); + Assert.Contains(result.Failures!, + f => f.Contains(nameof(OperationTrackingOptions.RetentionDays), StringComparison.Ordinal)); + } +} diff --git a/tools/hooks/README.md b/tools/hooks/README.md new file mode 100644 index 00000000..6d569873 --- /dev/null +++ b/tools/hooks/README.md @@ -0,0 +1,5 @@ +# Git hooks + +`pre-commit` scans the staged diff for obvious credential material (inbound API tokens `sbk_...`, plaintext password assignments in root `.txt`/`.md` additions) and blocks the commit if found (bypass a false positive with `git commit --no-verify`). + +Opt in once with: `git config core.hooksPath tools/hooks` diff --git a/tools/hooks/pre-commit b/tools/hooks/pre-commit new file mode 100755 index 00000000..d94b8347 --- /dev/null +++ b/tools/hooks/pre-commit @@ -0,0 +1,22 @@ +#!/bin/sh +# Root secret-capture guard (arch-review 08 round 2 NF1). +# Scans ONLY the staged diff for obvious credential material and blocks the +# commit if found. Opt-in: `git config core.hooksPath tools/hooks`. +# Bypass a knowing false positive with: git commit --no-verify + +diff=$(git diff --cached -U0) + +# 1) Inbound API token: sbk__ +token=$(printf '%s\n' "$diff" | grep -nE '^\+.*sbk_[0-9a-f]{16,}_[A-Za-z0-9_-]{20,}') + +# 2) Plaintext password assignment added to a root-level .txt/.md file. +pw=$(printf '%s\n' "$diff" | grep -nE '^\+.*[Pp]assword[0-9]*[[:space:]]*[:=][[:space:]]*[^[:space:]]') + +if [ -n "$token" ] || [ -n "$pw" ]; then + echo "pre-commit: possible secret in staged changes — commit blocked." >&2 + [ -n "$token" ] && echo " inbound API token (sbk_...) matched:" >&2 && printf '%s\n' "$token" >&2 + [ -n "$pw" ] && echo " plaintext password assignment matched:" >&2 && printf '%s\n' "$pw" >&2 + echo "Bypass a false positive with: git commit --no-verify" >&2 + exit 1 +fi +exit 0