diff --git a/code-reviews/Driver.AbCip.Contracts/findings.md b/code-reviews/Driver.AbCip.Contracts/findings.md index bc197ed5..d455be9e 100644 --- a/code-reviews/Driver.AbCip.Contracts/findings.md +++ b/code-reviews/Driver.AbCip.Contracts/findings.md @@ -11,7 +11,7 @@ | Review date | 2026-06-19 | | Commit reviewed | `a19b0f86` | | Status | Reviewed | -| Open findings | 4 | +| Open findings | 0 | ## Checklist coverage @@ -44,7 +44,7 @@ a category produced nothing rather than leaving it blank. | Severity | Medium | | Category | Correctness & logic bugs | | Location | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Contracts/AbCipEquipmentTagParser.cs:42` | -| Status | Open | +| Status | Resolved | **Description:** `AbCipEquipmentTagParser.TryParse` hard-codes `Writable: true` on every equipment-tag definition it produces, regardless of any `writable` field in the TagConfig JSON. @@ -70,7 +70,14 @@ record's `Writable` parameter. (2) Either return `false` from `TryParse` when `d to `AbCipDataType.Structure` (equipment-tag flow cannot declare members), or add an explicit comment documenting the black-box dotted-path behaviour so the next reader understands the intent. -**Resolution:** _(empty until closed)_ +**Resolution:** Resolved 2026-06-20 — (1) `TryParse` now reads the optional `"writable"` boolean +field from the TagConfig JSON and threads it into `AbCipTagDefinition.Writable`, defaulting to +`true` when the field is absent. The `` on `TryParse` was updated to document this +behaviour. (2) For the Structure concern, zero-behaviour-change option taken: an inline comment +was added at the `dataType` parse site in `TryParse` documenting that a `Structure` dataType on an +equipment tag is treated as a black-box dotted-path read (libplctag resolves the full path; the +equipment-tag flow does not enumerate UDT members). New tests in `AbCipEquipmentTagParserTests` +cover `writable:false`, `writable` absent, and the `Structure` path. Suite green (322 tests). --- @@ -81,7 +88,7 @@ comment documenting the black-box dotted-path behaviour so the next reader under | Severity | Low | | Category | Correctness & logic bugs | | Location | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Contracts/AbCipDataType.cs:28` | -| Status | Open | +| Status | Resolved | **Description:** The inline comment on `AbCipDataType.Dt` reads: @@ -114,7 +121,9 @@ Dt, // Logix DATE (0xCD — 4-byte unsigned days since 1984-01-01) or DATE_A No behaviour change; documentation only. -**Resolution:** _(empty until closed)_ +**Resolution:** Resolved 2026-06-20 — replaced the inaccurate single-line comment on `Dt` with a +3-line comment describing both mapped CIP types (`0xCD` DATE / `0xCF` DT), the 4-byte read stride, +and the truncation note for DATE_AND_TIME. Build green (0 errors, 0 warnings). --- @@ -125,7 +134,7 @@ No behaviour change; documentation only. | Severity | Low | | Category | Code organization & conventions | | Location | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Contracts/AbCipDriverOptions.cs:84-85` | -| Status | Open | +| Status | Resolved | **Description:** `AbCipDriverOptions.ProbeTimeoutSeconds` carries `[Display]` and `[Range(1, 60)]` attributes from `System.ComponentModel.DataAnnotations`. No other driver contracts project @@ -144,7 +153,11 @@ If the intent is to document the valid range, an `` tag (e.g. "Valid ra seconds; the AdminUI clamps to 60s server-side.") achieves the same goal without the attribute dependency. -**Resolution:** _(empty until closed)_ +**Resolution:** Resolved 2026-06-20 — removed `[Display]` and `[Range(1, 60)]` from +`ProbeTimeoutSeconds` and removed the `using System.ComponentModel.DataAnnotations;` directive +(confirmed it was used only by those two attributes). Added a `` element reading +"Valid range: 1–60 seconds; the AdminUI clamps to 60s server-side." to preserve the intent +in documentation. Build green (0 errors, 0 warnings). --- @@ -155,7 +168,7 @@ dependency. | Severity | Low | | Category | Testing coverage | | Location | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Contracts/AbCipEquipmentTagParser.cs` (entire file) | -| Status | Open | +| Status | Resolved | **Description:** The contracts module has no dedicated test project, and `AbCipEquipmentTagParser.TryParse` is the module's only non-trivial logic. Its `ReadArrayShape` helper has four distinct outcome @@ -175,7 +188,14 @@ is needed. Cover: valid scalar round-trip, 1-element array, N-element array, eac array-shape combination, non-JSON and non-object input, missing/blank `tagPath`, the Structure DataType path, and the `Writable` default. -**Resolution:** _(empty until closed)_ +**Resolution:** Resolved 2026-06-20 — added `AbCipEquipmentTagParserTests.cs` to +`tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/` (15 tests). Covers: valid scalar +round-trip; 1-element array (`isArray:true, arrayLength:1`); N-element array; `isArray:true + +arrayLength:0` → scalar; `isArray:true + arrayLength absent` → scalar; non-JSON input → false; +non-object JSON array → false; non-object JSON string → false; missing `tagPath` → false; blank +`tagPath` → false; `tagPath` as number → false; `writable:false` honoured; `writable` absent +defaults to `true`; `writable:true` explicit; `dataType:"Structure"` accepted with `Members:null` +(documents current black-box behaviour). Suite green (322 tests). --- diff --git a/code-reviews/Driver.AbCip/findings.md b/code-reviews/Driver.AbCip/findings.md index 350d585c..78d6a5a4 100644 --- a/code-reviews/Driver.AbCip/findings.md +++ b/code-reviews/Driver.AbCip/findings.md @@ -7,7 +7,7 @@ | Review date | 2026-06-19 | | Commit reviewed | `7286d320` | | Status | Reviewed | -| Open findings | 1 | +| Open findings | 0 | ## Checklist coverage @@ -337,7 +337,7 @@ flag and the legacy `ElementCount > 1` paths opt out). Full suite green (303 tes | Severity | Low | | Category | Documentation & comments | | Location | `AbCipAlarmProjection.cs:173-185` (`Tick`) | -| Status | Open | +| Status | Resolved | **Description:** `AbCipAlarmProjection.Tick` gates each node on the `InFaulted` snapshot's `StatusCode` (`if (inFaultedDv.StatusCode != Good) continue;`) but reads the `Severity` @@ -357,6 +357,12 @@ severity read is Bad, or add an XML/inline comment on `Tick` stating that severi decision (what severity to surface when it is genuinely unknown) rather than a mechanical fix, and the impact is negligible given the single-batch read shape. +**Resolution:** Resolved 2026-06-20 — added an inline comment on the `severity` read line in +`Tick` documenting that `severityDv.StatusCode` is not checked, that a Bad Severity read yields +`ToInt(null)=0` → `MapSeverity` buckets as Low, and that this is acceptable because InFaulted +and Severity are members of the same ALMD UDT read in one batch (a Good InFaulted almost always +implies a Good Severity). No behaviour change. + ### Driver.AbCip-018 | Field | Value | diff --git a/code-reviews/Driver.AbLegacy.Contracts/findings.md b/code-reviews/Driver.AbLegacy.Contracts/findings.md index 299e6506..bc843f07 100644 --- a/code-reviews/Driver.AbLegacy.Contracts/findings.md +++ b/code-reviews/Driver.AbLegacy.Contracts/findings.md @@ -11,7 +11,7 @@ | Review date | 2026-06-19 | | Commit reviewed | `a19b0f86` | | Status | Reviewed | -| Open findings | 3 | +| Open findings | 0 | ## Checklist coverage @@ -69,7 +69,7 @@ verified by build (no test project in this module). | Severity | Low | | Category | Documentation & comments | | Location | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Contracts/AbLegacyEquipmentTagParser.cs:15` | -| Status | Open | +| Status | Resolved | **Description:** `AbLegacyEquipmentTagParser.TryParse` has an undocumented edge case: when `isArray` is the JSON literal `true` but `arrayLength` is absent, zero, or negative, @@ -85,7 +85,13 @@ without a valid positive `arrayLength` silently produces a scalar (null `ArrayLe Optionally add a unit test to `AbLegacyEquipmentTagTests` covering `isArray:true, arrayLength:0/absent -> null` for regression protection. -**Resolution:** _(empty until closed)_ +**Resolution:** Resolved 2026-06-20. Added a `` block to `TryParse` in +`AbLegacyEquipmentTagParser.cs` documenting that `isArray:true` without a valid positive +`arrayLength` silently produces a scalar (`ArrayLength` is `null`), referencing the +in-source review C-2 rationale. Added three regression unit tests to +`AbLegacyEquipmentTagTests`: `IsArray_true_with_arrayLength_zero_produces_scalar`, +`IsArray_true_with_no_arrayLength_produces_scalar`, and +`IsArray_true_with_negative_arrayLength_produces_scalar`. Suite: 199 passed, 0 failed. --- @@ -96,7 +102,7 @@ Optionally add a unit test to `AbLegacyEquipmentTagTests` covering | Severity | Low | | Category | OtOpcUa conventions | | Location | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Contracts/AbLegacyPlcFamilyProfile.cs:10` | -| Status | Open | +| Status | Resolved | **Description:** `AbLegacyPlcFamilyProfile.MaxTagBytes` is a record constructor parameter populated with distinct values per family (240/232/240/240), but a global search finds zero @@ -111,7 +117,11 @@ produce off-by-one sizing. If reserved for future clamping, the intent is undocu clamping and is not currently enforced, or (b) remove the field and its four initialisers in a dedicated cleanup PR (signature change is out of scope for this review). -**Resolution:** _(empty until closed)_ +**Resolution:** Resolved 2026-06-20. Applied option (a): added XML doc on the `MaxTagBytes` +constructor parameter in `AbLegacyPlcFamilyProfile.cs` noting it is reserved for future +array-length clamping, is NOT currently enforced anywhere in the driver, and that the values +are approximate PCCC packet payload caps (not libplctag fragment limits). Field and +initialisers unchanged. --- @@ -122,7 +132,7 @@ in a dedicated cleanup PR (signature change is out of scope for this review). | Severity | Low | | Category | Code organization & conventions | | Location | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Contracts/AbLegacyPlcFamilyProfile.cs:22` | -| Status | Open | +| Status | Resolved | **Description:** `AbLegacyPlcFamilyProfile.ForFamily` has a catch-all arm `_ => Slc500` that silently returns the SLC 500 profile for any unrecognised `AbLegacyPlcFamily` value @@ -139,4 +149,8 @@ added), or (b) replace `_ => Slc500` with `_ => throw new ArgumentOutOfRangeException(nameof(family), family, null)` and apply it in a cleanup PR. Semantic change -- defer. -**Resolution:** _(empty until closed)_ +**Resolution:** Resolved 2026-06-20. Applied option (a): added a `` block to the +`ForFamily` XML doc in `AbLegacyPlcFamilyProfile.cs` documenting that any unrecognised +`family` value silently returns the `Slc500` profile, and noting this is intentional for +forward-compatibility with configs authored before a new family enum member is added. The +switch body is unchanged. diff --git a/code-reviews/Driver.FOCAS.Contracts/findings.md b/code-reviews/Driver.FOCAS.Contracts/findings.md index 4f927b84..5caa94d8 100644 --- a/code-reviews/Driver.FOCAS.Contracts/findings.md +++ b/code-reviews/Driver.FOCAS.Contracts/findings.md @@ -11,7 +11,7 @@ | Review date | 2026-06-19 | | Commit reviewed | `7286d320` | | Status | Reviewed | -| Open findings | 1 | +| Open findings | 0 | ## Checklist coverage @@ -61,13 +61,13 @@ a category produced nothing rather than leaving it blank. | Severity | Low | | Category | Design-document adherence | | Location | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Contracts/FocasDriverOptions.cs:139-145` | -| Status | Open | +| Status | Resolved | **Description:** `FocasTagDefinition` carries a `WriteIdempotent` field (default `false`) that the FOCAS driver never reads anywhere — neither in `DiscoverAsync`, `ReadAsync`, nor `WriteAsync`. `DiscoverAsync` always passes `WriteIdempotent: false` to `DriverAttributeInfo` (hardcoded), so the field has no runtime effect. `docs/drivers/FOCAS.md` does not mention `WriteIdempotent` in its configuration tables, making its purpose undiscoverable to operators. **Recommendation:** Either (a) thread `FocasTagDefinition.WriteIdempotent` through to `DriverAttributeInfo` in `DiscoverAsync` so the field has runtime effect and matches the pattern of other drivers, or (b) remove it from the record and the `FocasDriverConfigDto` wire DTO. Option (a) is the safer fix and matches the design of Modbus and other drivers. This is deferred because the fix touches `FocasDriver.DiscoverAsync` outside this module and requires a driver-level test change. -**Resolution:** _(empty until closed)_ +**Resolution:** Fixed (option a). In `FocasDriver.DiscoverAsync` the hardcoded `WriteIdempotent: false` was replaced with `tag.WriteIdempotent` so each user-authored tag's per-tag value is now threaded through to `DriverAttributeInfo`. The `` doc on `FocasTagDefinition` was updated to state it is now threaded through `DiscoverAsync` to `DriverAttributeInfo`. A new test `DiscoverAsync_surfaces_WriteIdempotent_from_tag_definition` was added to `FocasDriverMediumFindingsTests` asserting both the `true` and `false` cases surface correctly. The full `Driver.FOCAS.Tests` suite (including `FocasPmcBitRmwTests` and `FocasReadWriteTests` write tests) passes. --- diff --git a/code-reviews/Driver.Galaxy.Browser/findings.md b/code-reviews/Driver.Galaxy.Browser/findings.md index 3b1750da..ca2dc145 100644 --- a/code-reviews/Driver.Galaxy.Browser/findings.md +++ b/code-reviews/Driver.Galaxy.Browser/findings.md @@ -11,7 +11,7 @@ | Review date | 2026-06-19 | | Commit reviewed | `7286d320` | | Status | Reviewed | -| Open findings | 1 | +| Open findings | 0 | ## Checklist coverage @@ -27,7 +27,7 @@ a category produced nothing rather than leaving it blank. | 5 | Security | No issues found | | 6 | Performance & resource management | No issues found | | 7 | Design-document adherence | No issues found | -| 8 | Code organization & conventions | Driver.Galaxy.Browser-003 (Low): ResolveApiKey duplicated from GalaxyDriver with no sync mechanism | +| 8 | Code organization & conventions | Driver.Galaxy.Browser-003 (Low, Resolved): ResolveApiKey duplicated from GalaxyDriver; extracted to `GalaxySecretRef` in Driver.Galaxy.Contracts | | 9 | Testing coverage | Driver.Galaxy.Browser-004 (Low): MapSecurityClass not unit-tested; pure static method with no gateway dependency | | 10 | Documentation & comments | No issues found | @@ -119,7 +119,7 @@ mirroring the existing pattern for `_client.DisposeAsync()`. Regression test | Severity | Low | | Category | Code organization & conventions | | Location | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy.Browser/GalaxyDriverBrowser.cs:149` | -| Status | Open | +| Status | Resolved | **Description:** `GalaxyDriverBrowser.ResolveApiKey` is a verbatim copy of `GalaxyDriver.ResolveApiKey`. The comment acknowledges this and explains why the Browser @@ -133,8 +133,17 @@ and emit a spurious warning. which both `Driver.Galaxy` and `Driver.Galaxy.Browser` already reference. This is a one-line addition to Contracts — no migration, no public-contract break. -**Resolution:** _(deferred — requires a change in the Galaxy.Contracts project, outside -this module's boundary; tracked for a future consolidation pass)_ +**Resolution:** Resolved 2026-06-20 — closed by the same fix as the cross-referenced +Driver.Galaxy.Contracts-003. The resolver was extracted into a new +`public static class GalaxySecretRef` in `Driver.Galaxy.Contracts` +(`ResolveApiKey(string secretRef, ILogger? logger = null)`), which both Driver.Galaxy +and Driver.Galaxy.Browser already reference. `GalaxyDriverBrowser.BuildClientOptions` +now calls `GalaxySecretRef.ResolveApiKey(gw.ApiKeySecretRef, _logger)` and its private +`ResolveApiKey` copy is deleted — eliminating the drift risk (a future `vault:` prefix +is now added in one place). Behaviour is preserved exactly: the Browser's `_logger` is +never null (defaults to `NullLogger`), so the literal-arm cleartext warning still fires. +The Browser project builds clean against the shared resolver. See +Driver.Galaxy.Contracts-003 for the full extraction details. --- diff --git a/code-reviews/Driver.Galaxy.Contracts/findings.md b/code-reviews/Driver.Galaxy.Contracts/findings.md index ff7c7dc4..179f82ec 100644 --- a/code-reviews/Driver.Galaxy.Contracts/findings.md +++ b/code-reviews/Driver.Galaxy.Contracts/findings.md @@ -11,7 +11,7 @@ | Review date | 2026-06-19 | | Commit reviewed | `7286d320` | | Status | Reviewed | -| Open findings | 1 | +| Open findings | 0 | ## Checklist coverage @@ -27,7 +27,7 @@ a category produced nothing rather than leaving it blank. | 5 | Security | No issues found (ApiKeySecretRef is documented as an indirection reference; no defaults store a cleartext secret; no ToString override leaks the ref) | | 6 | Performance & resource management | No issues found (pure records; no allocations, disposables, or resource lifetimes) | | 7 | Design-document adherence | No issues found (records match CLAUDE.md Gateway/MxAccess/Repository/Reconnect section layout) | -| 8 | Code organization & conventions | Driver.Galaxy.Contracts-003 (Low, Open): ResolveApiKey helper duplicated between GalaxyDriver and GalaxyBrowseSession; Contracts is the natural home | +| 8 | Code organization & conventions | Driver.Galaxy.Contracts-003 (Low, Resolved): ResolveApiKey helper duplicated between GalaxyDriver and GalaxyBrowseSession; extracted to `GalaxySecretRef` in Contracts | | 9 | Testing coverage | No issues found (no logic to test; pure data records with default values) | | 10 | Documentation & comments | Driver.Galaxy.Contracts-001 (Low, Resolved): Internal code-review finding ID `(Driver.Galaxy-010)` in shipped XML doc | @@ -108,7 +108,7 @@ note the minimum and that `EventPump` enforces it at construction. Verified by b | Severity | Low | | Category | Code organization & conventions | | Location | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy.Browser/GalaxyDriverBrowser.cs:149` (duplicate) and `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy/GalaxyDriver.cs:472` (original) | -| Status | Open | +| Status | Resolved | **Description:** `GalaxyDriver.ResolveApiKey` (the four-form `env:`/`file:`/`dev:`/literal resolver, ~45 LOC) is duplicated verbatim as `GalaxyDriverBrowser.ResolveApiKey`. The @@ -135,6 +135,20 @@ which ships in-box with .NET 10's BCL and adds no new NuGet dependency. overload) into a `GalaxySecretRef` static class in this project; update both call sites to delegate to it. -**Resolution:** _(deferred — cross-module coordination change; Driver.Galaxy and -Driver.Galaxy.Browser must both be updated in the same commit. Tracked for a future -consolidation pass. See also Driver.Galaxy.Browser-003.)_ +**Resolution:** Resolved 2026-06-20 — extracted the four-form resolver into a new +`public static class GalaxySecretRef` (`src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy.Contracts/GalaxySecretRef.cs`, +namespace `ZB.MOM.WW.OtOpcUa.Driver.Galaxy.Config`) as a single +`ResolveApiKey(string secretRef, ILogger? logger = null)` method (the two former +`GalaxyDriver` overloads collapsed into one optional-logger signature). The exact +resolution semantics are preserved byte-for-byte: `env:NAME` (throws when unset), +`file:PATH` (throws when missing/empty, trims), `dev:KEY` (literal, no warning), and +the back-compat literal arm (returns the literal and emits the same `Warning` when a +logger is supplied). The Contracts `.csproj` gained a single +`Microsoft.Extensions.Logging.Abstractions` PackageReference for the `ILogger` +parameter. Both call sites now delegate: `GalaxyDriver.BuildClientOptions` calls +`GalaxySecretRef.ResolveApiKey(gw.ApiKeySecretRef, _logger)` and the two private +`GalaxyDriver.ResolveApiKey` overloads are deleted; `GalaxyDriverBrowser.BuildClientOptions` +likewise delegates (passing its non-null `_logger`) and its private copy is deleted. No +migration, no public wire-contract change. Regression coverage: `GalaxyDriverApiKeyResolverTests` +was repointed at `GalaxySecretRef.ResolveApiKey` (all 10 facts green). This finding and +the sibling Driver.Galaxy.Browser-003 are closed by this one extraction. diff --git a/code-reviews/Driver.Galaxy/findings.md b/code-reviews/Driver.Galaxy/findings.md index ce67e145..55669027 100644 --- a/code-reviews/Driver.Galaxy/findings.md +++ b/code-reviews/Driver.Galaxy/findings.md @@ -7,7 +7,7 @@ | Review date | 2026-06-19 | | Commit reviewed | `7286d320` | | Status | Reviewed | -| Open findings | 1 | +| Open findings | 0 | ## Checklist coverage @@ -83,7 +83,7 @@ observation + single `_ownedRepositoryClient`, the `ResolveApiKey` estimate, the O(1) `SubscriptionRegistry` indices, and the `ReinitializeAsync` equivalent-config gate all survive intact. -One new finding (Driver.Galaxy-019, Low, Open-deferred). No Critical / High / +One new finding (Driver.Galaxy-019, Low, Resolved 2026-06-20). No Critical / High / Medium new findings. Category results: | # | Category | Result | @@ -91,12 +91,12 @@ Medium new findings. Category results: | 1 | Correctness & logic bugs | No new issues found | | 2 | OtOpcUa conventions | No new issues found | | 3 | Concurrency & thread safety | No new issues found | -| 4 | Error handling & resilience | Driver.Galaxy-019 | +| 4 | Error handling & resilience | Driver.Galaxy-019 (Resolved) | | 5 | Security | No new issues found (vendoring retired; no secret logging; `ResolveApiKey` warns on cleartext) | | 6 | Performance & resource management | No new issues found | | 7 | Design-document adherence | No new issues found | | 8 | Code organization & conventions | No new issues found | -| 9 | Testing coverage | No new issues found (31 suites; `GatewayGalaxySubscriber` untestable per Driver.Galaxy-019) | +| 9 | Testing coverage | No new issues found (31 suites; `GatewayGalaxySubscriber` now has its first unit test — `GatewayGalaxySubscriberClassifyTests` for the Driver.Galaxy-019 classifier) | | 10 | Documentation & comments | No new issues found | ## Findings @@ -441,7 +441,7 @@ the existing csproj works correctly. | Severity | Low | | Category | Error handling & resilience | | Location | `Runtime/GatewayGalaxySubscriber.cs:89-100` | -| Status | Open | +| Status | Resolved | **Description:** `GatewayGalaxySubscriber.EnsureSessionIntervalAsync` applies the session-level `SetBufferedUpdateInterval` command and then caches the @@ -479,17 +479,21 @@ extract the reply→outcome classification into a pure internal helper the sealed, internal-ctor `MxGatewaySession` — `GatewayGalaxySubscriber` currently has zero unit tests for exactly this reason. -**Resolution:** Deferred 2026-06-19 — the fix is small but cannot be landed -with a real TDD red→green cycle in this module: the behaviour lives entirely -behind the sealed `MxGatewaySession` (no public/internal ctor; the same -constraint that forced every other gw call onto an injectable `IGalaxy*` seam), -so there is no way to drive a synthetic `MxCommandReply` through -`EnsureSessionIntervalAsync` without first refactoring out the pure-classifier -helper the recommendation calls for. That refactor plus giving the subscriber a -logger is a low-risk but non-trivial change to the production gw session path, -and the underlying intent (is caching on `MxaccessFailure` deliberate "the -gateway processed it, don't retry" behaviour, or an oversight?) is a design -question better confirmed against the gateway contract than guessed at in a -review sweep. Tracked for a follow-up that lands the classifier extraction + -its unit tests together. Impact is bounded to a sub-optimal (not broken) -publish cadence that self-heals on reconnect. +**Resolution:** Resolved 2026-06-20 — extracted the pure classifier +`internal static bool ClassifyIntervalReply(ProtocolStatusCode? code) => code == ProtocolStatusCode.Ok` +and routed `EnsureSessionIntervalAsync` through it, so `_lastAppliedIntervalMs` +is now set **only** on `Ok`. `MxaccessFailure` (COM-side set did not apply), any +other unexpected code, and a missing status all leave the cache untouched, so the +requested cadence is re-attempted on the next subscribe rather than permanently +pinned at the gateway default after a transient hiccup. The subscriber gained an +optional `ILogger? logger = null` ctor parameter (defaulting to `NullLogger.Instance`), +threaded through from `BuildProductionRuntimeAsync`'s `_logger` at the one +`new GatewayGalaxySubscriber(_ownedMxSession, _logger)` call site; it now emits a +`Warning` on both the `MxaccessFailure` soft-failure path and the unexpected-code +early-return path (no secret/key is logged), so the comment's promised signal exists. +Regression coverage: new `GatewayGalaxySubscriberClassifyTests` +(`tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy.Tests/`) — the subscriber's first +unit test — pins `Ok → true`, `MxaccessFailure → false`, `Unspecified → false`, +`null → false`. The sealed `MxGatewaySession` ctor that previously blocked a TDD +cycle is now sidestepped because the classifier is a pure static helper testable in +isolation. diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Contracts/AbCipDataType.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Contracts/AbCipDataType.cs index c79fbb05..3db3e903 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Contracts/AbCipDataType.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Contracts/AbCipDataType.cs @@ -25,7 +25,9 @@ public enum AbCipDataType Real, // 32-bit IEEE-754 LReal, // 64-bit IEEE-754 String, // Logix STRING (DINT Length + SINT[82] DATA — flattened to .NET string by libplctag) - Dt, // Date/Time — Logix DT == DINT representing seconds-since-epoch per Rockwell conventions + Dt, // Logix DATE (0xCD — 4-byte unsigned days since 1984-01-01) or DATE_AND_TIME / DT + // (0xCF — 8-byte unsigned microseconds since 1970-01-01). The driver reads 4 bytes + // via GetInt32; DATE decodes correctly, DATE_AND_TIME is truncated to the low 4 bytes. /// /// UDT / Predefined Structure (Timer / Counter / Control / Message / Axis). Shape is /// resolved at discovery time; reads + writes fan out to member Variables unless the diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Contracts/AbCipDriverOptions.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Contracts/AbCipDriverOptions.cs index 6c4d081e..4064e475 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Contracts/AbCipDriverOptions.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Contracts/AbCipDriverOptions.cs @@ -1,5 +1,3 @@ -using System.ComponentModel.DataAnnotations; - namespace ZB.MOM.WW.OtOpcUa.Driver.AbCip; /// @@ -81,8 +79,7 @@ public sealed class AbCipDriverOptions /// Timeout for the AdminUI Test Connect probe, in seconds. The AdminUI clamps to a /// 60s server-side maximum; this default is what the form pre-fills for new instances. /// - [Display(Name = "Probe timeout (seconds)", Description = "Connection test timeout. Default 5s.", GroupName = "Diagnostics")] - [Range(1, 60)] + /// Valid range: 1–60 seconds; the AdminUI clamps to 60s server-side. public int ProbeTimeoutSeconds { get; init; } = 5; } diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Contracts/AbCipEquipmentTagParser.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Contracts/AbCipEquipmentTagParser.cs index 062da496..7a5597d7 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Contracts/AbCipEquipmentTagParser.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Contracts/AbCipEquipmentTagParser.cs @@ -13,10 +13,11 @@ public static class AbCipEquipmentTagParser /// The transient definition when parsing succeeds. /// when is an AbCip TagConfig object. /// - /// The produced is always true: the - /// TagConfig JSON format for equipment tags does not carry a writability field, so the - /// PLC's ExternalAccess attribute is the effective write gate. Operators who need a - /// read-only OPC UA surface must rely on the PLC's ExternalAccess rejecting the write. + /// is read from the optional "writable" + /// boolean field in the TagConfig JSON; it defaults to true when the field is absent, + /// matching the record's documented default and the behaviour of pre-declared tags. Operators + /// who need a read-only OPC UA surface can author "writable":false in the TagConfig; + /// the PLC's ExternalAccess attribute remains the effective write gate at the wire level. /// public static bool TryParse(string reference, out AbCipTagDefinition def) { @@ -36,6 +37,11 @@ public static class AbCipEquipmentTagParser if (string.IsNullOrWhiteSpace(tagPath)) return false; var deviceHostAddress = ReadString(root, "deviceHostAddress"); + // A "dataType":"Structure" input is accepted and produces a Structure-typed definition + // with Members:null. The driver treats this as a black-box dotted-path read: libplctag + // resolves the full tag path (e.g. "Motor.Speed") without enumerating UDT members. + // The address space emits a placeholder String variable; UDT member declarations are + // not supported in the equipment-tag flow. var dataType = ReadEnum(root, "dataType", AbCipDataType.DInt); // Review I-1 — an equipment tag is an ARRAY ⟺ isArray:true AND arrayLength >= 1. A // 1-element array (isArray:true, arrayLength:1) is a VALID 1-element array — the @@ -43,9 +49,12 @@ public static class AbCipEquipmentTagParser // scalar. ElementCount can't carry the signal (a scalar and a 1-element array both // have a count of 1), so the explicit IsArray flag does. var (isArray, elementCount) = ReadArrayShape(root); + // "writable" defaults to true when absent — matches AbCipTagDefinition.Writable default. + var writable = !root.TryGetProperty("writable", out var writableEl) + || writableEl.ValueKind != JsonValueKind.False; def = new AbCipTagDefinition( Name: reference, DeviceHostAddress: deviceHostAddress, TagPath: tagPath, - DataType: dataType, Writable: true, ElementCount: elementCount, IsArray: isArray); + DataType: dataType, Writable: writable, ElementCount: elementCount, IsArray: isArray); return true; } catch (JsonException) { return false; } diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipAlarmProjection.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipAlarmProjection.cs index 2bb58691..5d839281 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipAlarmProjection.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipAlarmProjection.cs @@ -181,6 +181,11 @@ internal sealed class AbCipAlarmProjection : IAsyncDisposable if (inFaultedDv.StatusCode != AbCipStatusMapper.Good) continue; var nowFaulted = ToBool(inFaultedDv.Value); + // severityDv.StatusCode is not checked here. When the Severity read is Bad (value null), + // ToInt(null) returns 0 and MapSeverity buckets it as Low. This is acceptable because + // InFaulted and Severity are members of the same ALMD UDT read in one batch, so a Good + // InFaulted almost always implies a Good Severity. The "unknown severity → Low" fallback + // is intentional and matches the behaviour documented on Driver.AbCip-017. var severity = ToInt(severityDv.Value); var wasFaulted = sub.LastInFaulted.GetValueOrDefault(nodeId, false); diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Contracts/AbLegacyEquipmentTagParser.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Contracts/AbLegacyEquipmentTagParser.cs index 2c31de77..6f54ce2b 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Contracts/AbLegacyEquipmentTagParser.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Contracts/AbLegacyEquipmentTagParser.cs @@ -12,6 +12,17 @@ public static class AbLegacyEquipmentTagParser /// The equipment tag's TagConfig JSON (also used as the def identity). /// The transient definition when parsing succeeds. /// when is an AbLegacy address object. + /// + /// + /// When isArray is the JSON literal but arrayLength is + /// absent, zero, or negative, the result is silently a scalar + /// ( is ). + /// A valid positive arrayLength is required to produce an array tag; isArray:true + /// alone is not sufficient. This is intentional: a stale length behind a cleared or absent + /// isArray flag must never produce an orphan array tag that mismatches its scalar OPC UA + /// node (see in-source comment, review C-2). + /// + /// public static bool TryParse(string reference, out AbLegacyTagDefinition def) { def = null!; diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Contracts/AbLegacyPlcFamilyProfile.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Contracts/AbLegacyPlcFamilyProfile.cs index 8fef30e6..7bedc9f3 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Contracts/AbLegacyPlcFamilyProfile.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Contracts/AbLegacyPlcFamilyProfile.cs @@ -7,12 +7,25 @@ namespace ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.PlcFamilies; public sealed record AbLegacyPlcFamilyProfile( string LibplctagPlcAttribute, string DefaultCipPath, + /// + /// Reserved for future array-length clamping. Not currently enforced anywhere in the + /// driver. The values are approximate upper bounds derived from PCCC packet payload + /// limits (e.g. SLC 5/05 240 bytes is the PCCC-over-EIP data cap, not a libplctag fragment + /// limit). Do not rely on this field for sizing decisions until an enforcement point is added. + /// int MaxTagBytes, bool SupportsStringFile, bool SupportsLongFile) { /// Gets the profile for the specified PLC family. /// The PLC family. + /// + /// Any unrecognised value (e.g. an integer cast to the enum, or a + /// value added to before this switch is updated) silently + /// returns the profile. This is intentional: it preserves + /// forward-compatibility for device configs authored against a build that predates a new + /// family enum member, preferring a safe default over a startup exception. + /// public static AbLegacyPlcFamilyProfile ForFamily(AbLegacyPlcFamily family) => family switch { AbLegacyPlcFamily.Slc500 => Slc500, diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Contracts/FocasDriverOptions.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Contracts/FocasDriverOptions.cs index 61e5e3ad..7e7ef820 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Contracts/FocasDriverOptions.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Contracts/FocasDriverOptions.cs @@ -157,9 +157,9 @@ public sealed record FocasDeviceOptions( /// FocasReadWriteTests). Defaults to true. /// /// -/// Whether repeated writes of the same value are safe. Carried for parity; not yet -/// threaded through to DriverAttributeInfo in DiscoverAsync (see -/// Driver.FOCAS.Contracts-002). Defaults to false. +/// Whether repeated writes of the same value are safe. Threaded through to +/// DriverAttributeInfo.WriteIdempotent by DiscoverAsync so OPC UA +/// clients can optimise write coalescing for idempotent tags. Defaults to false. /// public sealed record FocasTagDefinition( string Name, diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS/FocasDriver.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS/FocasDriver.cs index a7636c99..bdd6b0af 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS/FocasDriver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS/FocasDriver.cs @@ -487,7 +487,7 @@ public sealed class FocasDriver : IDriver, IReadable, IWritable, ITagDiscovery, SecurityClass: SecurityClassification.ViewOnly, IsHistorized: false, IsAlarm: false, - WriteIdempotent: false)); + WriteIdempotent: tag.WriteIdempotent)); } } return Task.CompletedTask; diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy.Browser/GalaxyDriverBrowser.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy.Browser/GalaxyDriverBrowser.cs index 35fcdccd..a53a2386 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy.Browser/GalaxyDriverBrowser.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy.Browser/GalaxyDriverBrowser.cs @@ -122,14 +122,14 @@ public sealed class GalaxyDriverBrowser : IDriverBrowser /// /// Build the gateway client options from the form's Gateway section. Mirrors the /// runtime driver's GalaxyDriver.BuildClientOptions field-for-field so the - /// gateway sees an identical option shape. The API-key reference is resolved - /// inline (a slim version of GalaxyDriver.ResolveApiKey) because the - /// Browser project doesn't reference Driver.Galaxy. + /// gateway sees an identical option shape. The API-key reference is resolved via + /// the shared in Driver.Galaxy.Contracts + /// (the same resolver the runtime driver uses), so browse and runtime stay in lock-step. /// private MxGatewayClientOptions BuildClientOptions(GalaxyGatewayOptions gw) => new() { Endpoint = new Uri(gw.Endpoint, UriKind.Absolute), - ApiKey = ResolveApiKey(gw.ApiKeySecretRef), + ApiKey = GalaxySecretRef.ResolveApiKey(gw.ApiKeySecretRef, _logger), UseTls = gw.UseTls, CaCertificatePath = gw.CaCertificatePath, ConnectTimeout = TimeSpan.FromSeconds(gw.ConnectTimeoutSeconds), @@ -138,57 +138,4 @@ public sealed class GalaxyDriverBrowser : IDriverBrowser ? TimeSpan.FromSeconds(gw.StreamTimeoutSeconds) : null, }; - - /// - /// Resolves env:NAME, file:PATH, and dev:KEY prefixes; - /// anything else is treated as a literal cleartext key with a startup warning. - /// Slim mirror of GalaxyDriver.ResolveApiKey — the runtime version lives - /// in a sibling project the Browser intentionally doesn't reference. - /// - /// The secret reference string to resolve. - private string ResolveApiKey(string secretRef) - { - ArgumentException.ThrowIfNullOrEmpty(secretRef); - - if (secretRef.StartsWith("env:", StringComparison.OrdinalIgnoreCase)) - { - var name = secretRef[4..]; - var value = Environment.GetEnvironmentVariable(name); - return !string.IsNullOrEmpty(value) - ? value - : throw new InvalidOperationException( - $"Galaxy.Gateway.ApiKeySecretRef='{secretRef}' resolves to env var '{name}', but it is unset."); - } - - if (secretRef.StartsWith("file:", StringComparison.OrdinalIgnoreCase)) - { - var path = secretRef[5..]; - if (!File.Exists(path)) - { - throw new InvalidOperationException( - $"Galaxy.Gateway.ApiKeySecretRef='{secretRef}' points at '{path}', which doesn't exist."); - } - var contents = File.ReadAllText(path).Trim(); - return !string.IsNullOrEmpty(contents) - ? contents - : throw new InvalidOperationException( - $"Galaxy.Gateway.ApiKeySecretRef='{secretRef}' file '{path}' is empty."); - } - - if (secretRef.StartsWith("dev:", StringComparison.OrdinalIgnoreCase)) - { - // Explicit dev opt-in — no warning, the operator deliberately chose a - // cleartext literal (dev box, parity rig). - return secretRef[4..]; - } - - // Back-compat literal arm. An unprefixed string is treated as the literal - // API key — but emit a warning so an operator who accidentally committed a - // cleartext key into DriverConfig sees it when they open the address picker. - _logger.LogWarning( - "Galaxy.Gateway.ApiKeySecretRef is being treated as a literal cleartext API key. " + - "Prefer env:NAME, file:PATH, or the explicit dev:KEY prefix for dev rigs — " + - "a literal key in DriverConfig JSON is stored in cleartext in the central config DB."); - return secretRef; - } } diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy.Contracts/GalaxyDriverOptions.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy.Contracts/GalaxyDriverOptions.cs index 1e38f5a6..68446e17 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy.Contracts/GalaxyDriverOptions.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy.Contracts/GalaxyDriverOptions.cs @@ -30,7 +30,7 @@ public sealed record GalaxyDriverOptions( /// /// Connection details for the MxAccess gateway. is -/// resolved by GalaxyDriver.ResolveApiKey at InitializeAsync time. Four forms +/// resolved by at InitializeAsync time. Four forms /// supported, in priority order: /// /// env:NAME — read from an environment variable (recommended for diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy.Contracts/GalaxySecretRef.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy.Contracts/GalaxySecretRef.cs new file mode 100644 index 00000000..f11efe4e --- /dev/null +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy.Contracts/GalaxySecretRef.cs @@ -0,0 +1,88 @@ +using Microsoft.Extensions.Logging; + +namespace ZB.MOM.WW.OtOpcUa.Driver.Galaxy.Config; + +/// +/// Resolves Gateway.ApiKeySecretRef to the actual API-key string. Four +/// forms supported, evaluated in order: +/// +/// env:NAME — reads Environment.GetEnvironmentVariable(NAME). +/// Throws when the variable is unset, so a misconfigured deployment fails +/// fast rather than silently sending an empty key. +/// file:PATH — reads UTF-8 text from PATH, trimming +/// whitespace. Lets operators stash the key in an ACL'd file outside the +/// repo (the same pattern as the legacy .local/galaxy-host-secret.txt). +/// dev:KEY — explicit cleartext literal. The dev: prefix +/// is a deliberate opt-in signal (dev box, parity rig) so the resolver +/// doesn't emit a warning; production should never use this arm. +/// Anything else — used as the literal API key for back-compat with +/// configs that pre-date this resolver. When a logger is supplied the +/// resolver emits a startup warning so an operator who accidentally +/// committed a cleartext key sees it (Driver.Galaxy-010). +/// +/// A future PR can swap any of these arms for a DPAPI-backed lookup without +/// changing the call site. +/// +/// +/// Lives in the Contracts project so both the runtime GalaxyDriver and the +/// AdminUI GalaxyDriverBrowser (which intentionally don't reference each +/// other) share a single resolver rather than each maintaining a copy. +/// +public static class GalaxySecretRef +{ + /// + /// Resolves the supplied secret reference. When the ref falls through to the + /// back-compat literal arm (an unprefixed cleartext API key in + /// DriverConfig JSON) and a is supplied, emits + /// a . The dev: prefix is the explicit + /// opt-in path that doesn't warn. + /// + /// The secret reference string to resolve. + /// Optional logger for warning on cleartext keys. + public static string ResolveApiKey(string secretRef, ILogger? logger = null) + { + ArgumentException.ThrowIfNullOrEmpty(secretRef); + + if (secretRef.StartsWith("env:", StringComparison.OrdinalIgnoreCase)) + { + var name = secretRef[4..]; + var value = Environment.GetEnvironmentVariable(name); + return !string.IsNullOrEmpty(value) + ? value + : throw new InvalidOperationException( + $"Galaxy.Gateway.ApiKeySecretRef='{secretRef}' resolves to env var '{name}', but it is unset."); + } + + if (secretRef.StartsWith("file:", StringComparison.OrdinalIgnoreCase)) + { + var path = secretRef[5..]; + if (!File.Exists(path)) + { + throw new InvalidOperationException( + $"Galaxy.Gateway.ApiKeySecretRef='{secretRef}' points at '{path}', which doesn't exist."); + } + var contents = File.ReadAllText(path).Trim(); + return !string.IsNullOrEmpty(contents) + ? contents + : throw new InvalidOperationException( + $"Galaxy.Gateway.ApiKeySecretRef='{secretRef}' file '{path}' is empty."); + } + + if (secretRef.StartsWith("dev:", StringComparison.OrdinalIgnoreCase)) + { + // Explicit dev opt-in — no warning, the operator deliberately chose a + // cleartext literal (dev box, parity rig). + return secretRef[4..]; + } + + // Back-compat literal arm. An unprefixed string is treated as the literal + // API key — but emit a warning so an operator who accidentally committed a + // cleartext key into DriverConfig sees it. Use the dev: prefix to suppress + // this warning when the literal is intentional. + logger?.LogWarning( + "Galaxy.Gateway.ApiKeySecretRef is being treated as a literal cleartext API key. " + + "Prefer env:NAME, file:PATH, or the explicit dev:KEY prefix for dev rigs — " + + "a literal key in DriverConfig JSON is stored in cleartext in the central config DB."); + return secretRef; + } +} diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy.Contracts/ZB.MOM.WW.OtOpcUa.Driver.Galaxy.Contracts.csproj b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy.Contracts/ZB.MOM.WW.OtOpcUa.Driver.Galaxy.Contracts.csproj index 3896f7bd..3c4f01ec 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy.Contracts/ZB.MOM.WW.OtOpcUa.Driver.Galaxy.Contracts.csproj +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy.Contracts/ZB.MOM.WW.OtOpcUa.Driver.Galaxy.Contracts.csproj @@ -5,5 +5,9 @@ enable true - + + + + + diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy/GalaxyDriver.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy/GalaxyDriver.cs index c6f2ab5f..75f01621 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy/GalaxyDriver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy/GalaxyDriver.cs @@ -252,7 +252,7 @@ public sealed class GalaxyDriver // listener (OTLP exporter, dotnet-trace, etc.) consumes these without the driver // taking a dependency on the OpenTelemetry packages. _subscriber = new TracedGalaxySubscriber( - new GatewayGalaxySubscriber(_ownedMxSession), _options.MxAccess.ClientName); + new GatewayGalaxySubscriber(_ownedMxSession, _logger), _options.MxAccess.ClientName); _dataWriter = new TracedGalaxyDataWriter( // Let the writer borrow live MXAccess item handles the subscription registry already // holds, so the first write to an already-subscribed tag skips a redundant AddItem. @@ -437,91 +437,14 @@ public sealed class GalaxyDriver } } - /// - /// Resolves Gateway.ApiKeySecretRef to the actual API-key bytes. Four - /// forms supported, evaluated in order: - /// - /// env:NAME — reads Environment.GetEnvironmentVariable(NAME). - /// Throws when the variable is unset, so a misconfigured deployment fails - /// fast at InitializeAsync rather than silently sending an empty key. - /// file:PATH — reads UTF-8 text from PATH, trimming - /// whitespace. Lets operators stash the key in an ACL'd file outside the - /// repo (the same pattern as the legacy .local/galaxy-host-secret.txt). - /// dev:KEY — explicit cleartext literal. The dev: prefix - /// is a deliberate opt-in signal (dev box, parity rig) so the resolver - /// doesn't emit a warning; production should never use this arm. - /// Anything else — used as the literal API key for back-compat with - /// configs that pre-date this resolver. When a logger is supplied the - /// resolver emits a startup warning so an operator who accidentally - /// committed a cleartext key sees it (Driver.Galaxy-010). - /// - /// A future PR can swap any of these arms for a DPAPI-backed lookup without - /// changing the call site. - /// - /// The secret reference string to resolve. - internal static string ResolveApiKey(string secretRef) => ResolveApiKey(secretRef, logger: null); - - /// - /// Logger-aware overload. Emits a if the secret - /// ref falls through to the back-compat literal arm (an unprefixed cleartext - /// API key in DriverConfig JSON). The dev: prefix is the explicit - /// opt-in path that doesn't warn. - /// - /// The secret reference string to resolve. - /// Optional logger for warning on cleartext keys. - internal static string ResolveApiKey(string secretRef, ILogger? logger) - { - ArgumentException.ThrowIfNullOrEmpty(secretRef); - - if (secretRef.StartsWith("env:", StringComparison.OrdinalIgnoreCase)) - { - var name = secretRef[4..]; - var value = Environment.GetEnvironmentVariable(name); - return !string.IsNullOrEmpty(value) - ? value - : throw new InvalidOperationException( - $"Galaxy.Gateway.ApiKeySecretRef='{secretRef}' resolves to env var '{name}', but it is unset."); - } - - if (secretRef.StartsWith("file:", StringComparison.OrdinalIgnoreCase)) - { - var path = secretRef[5..]; - if (!File.Exists(path)) - { - throw new InvalidOperationException( - $"Galaxy.Gateway.ApiKeySecretRef='{secretRef}' points at '{path}', which doesn't exist."); - } - var contents = File.ReadAllText(path).Trim(); - return !string.IsNullOrEmpty(contents) - ? contents - : throw new InvalidOperationException( - $"Galaxy.Gateway.ApiKeySecretRef='{secretRef}' file '{path}' is empty."); - } - - if (secretRef.StartsWith("dev:", StringComparison.OrdinalIgnoreCase)) - { - // Explicit dev opt-in — no warning, the operator deliberately chose a - // cleartext literal (dev box, parity rig). - return secretRef[4..]; - } - - // Back-compat literal arm. An unprefixed string is treated as the literal - // API key — but emit a warning so an operator who accidentally committed a - // cleartext key into DriverConfig sees it at startup. Use the dev: prefix - // to suppress this warning when the literal is intentional. - logger?.LogWarning( - "Galaxy.Gateway.ApiKeySecretRef is being treated as a literal cleartext API key. " + - "Prefer env:NAME, file:PATH, or the explicit dev:KEY prefix for dev rigs — " + - "a literal key in DriverConfig JSON is stored in cleartext in the central config DB."); - return secretRef; - } - private MxGatewayClientOptions BuildClientOptions(GalaxyGatewayOptions gw) => new() { Endpoint = new Uri(gw.Endpoint, UriKind.Absolute), // Driver.Galaxy-010: pass the logger so the literal-arm cleartext fallback - // surfaces a startup warning rather than silently shipping the key. - ApiKey = ResolveApiKey(gw.ApiKeySecretRef, _logger), + // surfaces a startup warning rather than silently shipping the key. The + // resolver lives in Driver.Galaxy.Contracts (GalaxySecretRef) so the runtime + // driver and the AdminUI browser share one implementation. + ApiKey = GalaxySecretRef.ResolveApiKey(gw.ApiKeySecretRef, _logger), UseTls = gw.UseTls, CaCertificatePath = gw.CaCertificatePath, ConnectTimeout = TimeSpan.FromSeconds(gw.ConnectTimeoutSeconds), diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy/Runtime/GatewayGalaxySubscriber.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy/Runtime/GatewayGalaxySubscriber.cs index e38430f6..6dee938b 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy/Runtime/GatewayGalaxySubscriber.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy/Runtime/GatewayGalaxySubscriber.cs @@ -1,3 +1,5 @@ +using Microsoft.Extensions.Logging; +using Microsoft.Extensions.Logging.Abstractions; using ZB.MOM.WW.MxGateway.Client; using ZB.MOM.WW.MxGateway.Contracts.Proto; // Use the generated nested status enum for the SetBufferedUpdateInterval reply check. @@ -18,14 +20,21 @@ namespace ZB.MOM.WW.OtOpcUa.Driver.Galaxy.Runtime; public sealed class GatewayGalaxySubscriber : IGalaxySubscriber { private readonly GalaxyMxSession _session; + private readonly ILogger _logger; private readonly Lock _intervalLock = new(); private int _lastAppliedIntervalMs = -1; // -1 = never applied; 0 = explicit "use gw default" /// Initializes a new instance of GatewayGalaxySubscriber. /// The Galaxy MX session to use for subscription operations. - public GatewayGalaxySubscriber(GalaxyMxSession session) + /// + /// Optional logger; surfaces a warning when SetBufferedUpdateInterval + /// soft-fails so the cadence-not-applied condition isn't silent. Null is allowed + /// for unit-test construction and falls back to . + /// + public GatewayGalaxySubscriber(GalaxyMxSession session, ILogger? logger = null) { _session = session ?? throw new ArgumentNullException(nameof(session)); + _logger = logger ?? NullLogger.Instance; } /// Subscribes to a bulk list of Galaxy references with optional buffered update interval. @@ -86,11 +95,33 @@ public sealed class GatewayGalaxySubscriber : IGalaxySubscriber }, cancellationToken).ConfigureAwait(false); - if (reply.ProtocolStatus?.Code is not (ProtocolStatusCode.Ok or ProtocolStatusCode.MxaccessFailure)) + var code = reply.ProtocolStatus?.Code; + + // MxaccessFailure means the COM-side SetBufferedUpdateInterval did NOT apply, so + // we must NOT cache the requested value — caching it would suppress the retry on + // the next subscribe at this interval. Only Ok records the value as applied. + if (!ClassifyIntervalReply(code)) { // Don't throw on a soft failure — the SubscribeBulk will still succeed at the // gw's default cadence, which is functional just not the requested cadence. - // The trace span (PR 6.1) plus the warning here gives ops the signal. + // The trace span (PR 6.1) plus this warning gives ops the signal, and leaving + // _lastAppliedIntervalMs unchanged lets the next subscribe re-attempt the set. + if (code == ProtocolStatusCode.MxaccessFailure) + { + _logger.LogWarning( + "Galaxy SetBufferedUpdateInterval({IntervalMs}ms) soft-failed (MxaccessFailure); " + + "buffered subscriptions on server handle {ServerHandle} will publish at the gateway's " + + "default cadence. The requested interval was not cached, so a later subscribe will retry it.", + intervalMs, serverHandle); + } + else + { + _logger.LogWarning( + "Galaxy SetBufferedUpdateInterval({IntervalMs}ms) returned an unexpected protocol status " + + "{Code} on server handle {ServerHandle}; treating it as not-applied and leaving the " + + "requested interval uncached so a later subscribe retries it.", + intervalMs, code, serverHandle); + } return; } @@ -100,6 +131,17 @@ public sealed class GatewayGalaxySubscriber : IGalaxySubscriber } } + /// + /// Classifies a SetBufferedUpdateInterval reply: returns true only when + /// the requested interval was actually applied and may be cached as last-applied. + /// This is alone — + /// means the COM-side set did NOT take effect (so caching it would prevent a retry), + /// and a null or any other unexpected code is treated as not-applied. + /// + /// The protocol status code from the gateway reply, or null when absent. + /// true if the interval should be recorded as applied; otherwise false. + internal static bool ClassifyIntervalReply(ProtocolStatusCode? code) => code == ProtocolStatusCode.Ok; + /// Unsubscribes from a bulk list of item handles. /// The item handles to unsubscribe from. /// The cancellation token. diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipEquipmentTagParserTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipEquipmentTagParserTests.cs new file mode 100644 index 00000000..c434d1b4 --- /dev/null +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipEquipmentTagParserTests.cs @@ -0,0 +1,142 @@ +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Driver.AbCip; + +namespace ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests; + +/// +/// Dedicated unit tests for . +/// Covers all distinct outcome branches: valid scalar, 1-element array, N-element array, +/// degenerate array shapes, non-JSON input, non-object JSON, blank/missing tagPath, +/// the writable field, and the Structure dataType path (Driver.AbCip.Contracts-004). +/// +[Trait("Category", "Unit")] +public class AbCipEquipmentTagParserTests +{ + // ── Happy-path scalar ──────────────────────────────────────────────────────────────── + + [Fact] + public void Valid_scalar_round_trip_parses_all_fields() + { + var json = """{"deviceHostAddress":"ab://10.0.0.1/1,0","tagPath":"Motor.Speed","dataType":"Real"}"""; + AbCipEquipmentTagParser.TryParse(json, out var def).ShouldBeTrue(); + def!.Name.ShouldBe(json); + def.TagPath.ShouldBe("Motor.Speed"); + def.DeviceHostAddress.ShouldBe("ab://10.0.0.1/1,0"); + def.DataType.ShouldBe(AbCipDataType.Real); + def.Writable.ShouldBeTrue(); + def.IsArray.ShouldBeFalse(); + def.ElementCount.ShouldBe(1); + } + + // ── Array shape ────────────────────────────────────────────────────────────────────── + + [Fact] + public void One_element_array_isArray_true_arrayLength_1_is_an_array_not_a_scalar() + { + var json = """{"tagPath":"Tags[0]","dataType":"DInt","isArray":true,"arrayLength":1}"""; + AbCipEquipmentTagParser.TryParse(json, out var def).ShouldBeTrue(); + def!.IsArray.ShouldBeTrue(); + def.ElementCount.ShouldBe(1); + } + + [Fact] + public void N_element_array_isArray_true_arrayLength_N_parses_correctly() + { + var json = """{"tagPath":"Buf","dataType":"SInt","isArray":true,"arrayLength":8}"""; + AbCipEquipmentTagParser.TryParse(json, out var def).ShouldBeTrue(); + def!.IsArray.ShouldBeTrue(); + def.ElementCount.ShouldBe(8); + } + + [Fact] + public void IsArray_true_arrayLength_0_is_canonical_scalar() + { + // Canonical rule: isArray:true AND arrayLength < 1 → scalar. + var json = """{"tagPath":"PT_101","isArray":true,"arrayLength":0}"""; + AbCipEquipmentTagParser.TryParse(json, out var def).ShouldBeTrue(); + def!.IsArray.ShouldBeFalse(); + def.ElementCount.ShouldBe(1); + } + + [Fact] + public void IsArray_true_arrayLength_absent_is_canonical_scalar() + { + // Canonical rule: isArray:true but arrayLength absent → scalar. + var json = """{"tagPath":"PT_101","isArray":true}"""; + AbCipEquipmentTagParser.TryParse(json, out var def).ShouldBeTrue(); + def!.IsArray.ShouldBeFalse(); + def.ElementCount.ShouldBe(1); + } + + // ── Rejection paths ────────────────────────────────────────────────────────────────── + + [Fact] + public void Non_JSON_input_returns_false() + => AbCipEquipmentTagParser.TryParse("not json at all", out _).ShouldBeFalse(); + + [Fact] + public void Non_object_JSON_array_returns_false() + => AbCipEquipmentTagParser.TryParse("""["tagPath","foo"]""", out _).ShouldBeFalse(); + + [Fact] + public void Non_object_JSON_string_returns_false() + => AbCipEquipmentTagParser.TryParse("\"Motor.Speed\"", out _).ShouldBeFalse(); + + [Fact] + public void Missing_tagPath_returns_false() + => AbCipEquipmentTagParser.TryParse("""{"dataType":"DInt"}""", out _).ShouldBeFalse(); + + [Fact] + public void Blank_tagPath_returns_false() + => AbCipEquipmentTagParser.TryParse("""{"tagPath":" "}""", out _).ShouldBeFalse(); + + [Fact] + public void TagPath_as_number_returns_false() + => AbCipEquipmentTagParser.TryParse("""{"tagPath":42}""", out _).ShouldBeFalse(); + + // ── Writable field (Driver.AbCip.Contracts-001) ─────────────────────────────────────── + + [Fact] + public void Writable_false_is_honoured() + { + var json = """{"tagPath":"Sensor.Val","writable":false}"""; + AbCipEquipmentTagParser.TryParse(json, out var def).ShouldBeTrue(); + def!.Writable.ShouldBeFalse(); + } + + [Fact] + public void Writable_absent_defaults_to_true() + { + var json = """{"tagPath":"Sensor.Val"}"""; + AbCipEquipmentTagParser.TryParse(json, out var def).ShouldBeTrue(); + def!.Writable.ShouldBeTrue(); + } + + [Fact] + public void Writable_true_explicit_is_honoured() + { + var json = """{"tagPath":"Sensor.Val","writable":true}"""; + AbCipEquipmentTagParser.TryParse(json, out var def).ShouldBeTrue(); + def!.Writable.ShouldBeTrue(); + } + + // ── Structure dataType (Driver.AbCip.Contracts-001 Structure concern) ───────────────── + + /// + /// A "dataType":"Structure" equipment-tag input is accepted and produces a Structure-typed + /// definition with Members:null. The driver treats the tag path as a black-box dotted-path + /// read (libplctag resolves the full path); UDT member declarations are not supported in the + /// equipment-tag flow. This test documents current behaviour so a future change to reject + /// Structure is a conscious choice. + /// + [Fact] + public void Structure_dataType_is_accepted_with_null_Members_and_returns_true() + { + var json = """{"tagPath":"Motor","dataType":"Structure"}"""; + AbCipEquipmentTagParser.TryParse(json, out var def).ShouldBeTrue(); + def!.DataType.ShouldBe(AbCipDataType.Structure); + def.Members.ShouldBeNull(); + def.TagPath.ShouldBe("Motor"); + } +} diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Tests/AbLegacyEquipmentTagTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Tests/AbLegacyEquipmentTagTests.cs index b485229c..5433cdef 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Tests/AbLegacyEquipmentTagTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Tests/AbLegacyEquipmentTagTests.cs @@ -37,6 +37,32 @@ public sealed class AbLegacyEquipmentTagTests => AbLegacyEquipmentTagParser.TryParse( """{"address":"","dataType":"Int"}""", out _).ShouldBeFalse(); + // -002 regression: isArray:true without a valid positive arrayLength → scalar (null ArrayLength). + + [Fact] + public void IsArray_true_with_arrayLength_zero_produces_scalar() + { + var json = """{"address":"N7:0","dataType":"Int","isArray":true,"arrayLength":0}"""; + AbLegacyEquipmentTagParser.TryParse(json, out var def).ShouldBeTrue(); + def!.ArrayLength.ShouldBeNull(); + } + + [Fact] + public void IsArray_true_with_no_arrayLength_produces_scalar() + { + var json = """{"address":"N7:0","dataType":"Int","isArray":true}"""; + AbLegacyEquipmentTagParser.TryParse(json, out var def).ShouldBeTrue(); + def!.ArrayLength.ShouldBeNull(); + } + + [Fact] + public void IsArray_true_with_negative_arrayLength_produces_scalar() + { + var json = """{"address":"N7:0","dataType":"Int","isArray":true,"arrayLength":-5}"""; + AbLegacyEquipmentTagParser.TryParse(json, out var def).ShouldBeTrue(); + def!.ArrayLength.ShouldBeNull(); + } + /// /// End-to-end driver-level proof: an AbLegacy driver with NO authored tags can still read an /// equipment-tag ref (the raw TagConfig JSON) — the resolver parses it into a transient diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Tests/FocasDriverMediumFindingsTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Tests/FocasDriverMediumFindingsTests.cs index 593a4712..a3d99867 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Tests/FocasDriverMediumFindingsTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Tests/FocasDriverMediumFindingsTests.cs @@ -116,6 +116,39 @@ public sealed class FocasDriverMediumFindingsTests .ShouldBe(SecurityClassification.ViewOnly); } + // ---- Driver.FOCAS.Contracts-002: WriteIdempotent threaded through DiscoverAsync ---- + + /// + /// Verifies that a tag authored with WriteIdempotent: true surfaces + /// WriteIdempotent == true on its discovered , + /// and that a tag with the default WriteIdempotent: false surfaces false. + /// Resolves Driver.FOCAS.Contracts-002 — the field was previously hardcoded to + /// false in DiscoverAsync and had no runtime effect. + /// + [Fact] + public async Task DiscoverAsync_surfaces_WriteIdempotent_from_tag_definition() + { + var builder = new RecordingBuilder(); + var drv = new FocasDriver(new FocasDriverOptions + { + Devices = [new FocasDeviceOptions("focas://10.0.0.5:8193")], + Tags = + [ + new FocasTagDefinition("Idempotent", "focas://10.0.0.5:8193", "R100", FocasDataType.Int16, WriteIdempotent: true), + new FocasTagDefinition("NonIdempotent", "focas://10.0.0.5:8193", "R200", FocasDataType.Int16, WriteIdempotent: false), + ], + Probe = new FocasProbeOptions { Enabled = false }, + }, "drv-1", new FakeFocasClientFactory()); + await drv.InitializeAsync("{}", CancellationToken.None); + + await drv.DiscoverAsync(builder, CancellationToken.None); + + builder.Variables.Single(v => v.BrowseName == "Idempotent").Info.WriteIdempotent + .ShouldBeTrue("a tag declared WriteIdempotent:true must surface true on DriverAttributeInfo"); + builder.Variables.Single(v => v.BrowseName == "NonIdempotent").Info.WriteIdempotent + .ShouldBeFalse("a tag declared WriteIdempotent:false (the default) must surface false"); + } + // ---- Driver.FOCAS-005: Volatile-guarded _health survives concurrent reads ---- /// Verifies that GetHealth reflects state updated from concurrent reads. diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy.Tests/GalaxyDriverApiKeyResolverTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy.Tests/GalaxyDriverApiKeyResolverTests.cs index cef0fcfd..ba3a52b5 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy.Tests/GalaxyDriverApiKeyResolverTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy.Tests/GalaxyDriverApiKeyResolverTests.cs @@ -1,14 +1,17 @@ using Microsoft.Extensions.Logging; using Shouldly; using Xunit; +using ZB.MOM.WW.OtOpcUa.Driver.Galaxy.Config; namespace ZB.MOM.WW.OtOpcUa.Driver.Galaxy.Tests; /// /// Follow-up #2 — pins the three resolution forms supported by -/// : env:NAME, file:PATH, +/// : env:NAME, file:PATH, /// and the literal-string fallback. A future DPAPI arm slots in here without -/// touching the call site. +/// touching the call site. (The resolver was extracted from GalaxyDriver to +/// the shared GalaxySecretRef in Driver.Galaxy.Contracts so the runtime +/// driver and the AdminUI browser share one copy.) /// public sealed class GalaxyDriverApiKeyResolverTests { @@ -16,7 +19,7 @@ public sealed class GalaxyDriverApiKeyResolverTests [Fact] public void Literal_string_is_returned_unchanged() { - GalaxyDriver.ResolveApiKey("plain-text-key").ShouldBe("plain-text-key"); + GalaxySecretRef.ResolveApiKey("plain-text-key").ShouldBe("plain-text-key"); } /// Verifies that env: prefix resolves to an environment variable. @@ -27,7 +30,7 @@ public sealed class GalaxyDriverApiKeyResolverTests Environment.SetEnvironmentVariable(name, "key-from-env"); try { - GalaxyDriver.ResolveApiKey($"env:{name}").ShouldBe("key-from-env"); + GalaxySecretRef.ResolveApiKey($"env:{name}").ShouldBe("key-from-env"); } finally { @@ -43,7 +46,7 @@ public sealed class GalaxyDriverApiKeyResolverTests Environment.SetEnvironmentVariable(name, null); var ex = Should.Throw(() => - GalaxyDriver.ResolveApiKey($"env:{name}")); + GalaxySecretRef.ResolveApiKey($"env:{name}")); ex.Message.ShouldContain(name); ex.Message.ShouldContain("unset"); } @@ -56,7 +59,7 @@ public sealed class GalaxyDriverApiKeyResolverTests File.WriteAllText(path, " key-from-file \n"); try { - GalaxyDriver.ResolveApiKey($"file:{path}").ShouldBe("key-from-file"); + GalaxySecretRef.ResolveApiKey($"file:{path}").ShouldBe("key-from-file"); } finally { @@ -70,7 +73,7 @@ public sealed class GalaxyDriverApiKeyResolverTests { var path = Path.Combine(Path.GetTempPath(), $"does-not-exist-{Guid.NewGuid():N}.txt"); var ex = Should.Throw(() => - GalaxyDriver.ResolveApiKey($"file:{path}")); + GalaxySecretRef.ResolveApiKey($"file:{path}")); ex.Message.ShouldContain(path); ex.Message.ShouldContain("doesn't exist"); } @@ -85,7 +88,7 @@ public sealed class GalaxyDriverApiKeyResolverTests // in the DriverConfig JSON. The resolver must surface a warning so an // operator who committed one by accident sees it at startup. var logger = new CaptureLogger(); - var key = GalaxyDriver.ResolveApiKey("plain-text-key", logger); + var key = GalaxySecretRef.ResolveApiKey("plain-text-key", logger); key.ShouldBe("plain-text-key"); logger.Entries.ShouldContain(e => @@ -100,7 +103,7 @@ public sealed class GalaxyDriverApiKeyResolverTests // key (dev / parity rig). The resolver must accept it AND suppress the // warning so production logs aren't polluted on a deliberate dev choice. var logger = new CaptureLogger(); - var key = GalaxyDriver.ResolveApiKey("dev:plain-text-key", logger); + var key = GalaxySecretRef.ResolveApiKey("dev:plain-text-key", logger); key.ShouldBe("plain-text-key"); logger.Entries.ShouldNotContain(e => e.Level == LogLevel.Warning); @@ -115,7 +118,7 @@ public sealed class GalaxyDriverApiKeyResolverTests try { var logger = new CaptureLogger(); - GalaxyDriver.ResolveApiKey($"env:{name}", logger); + GalaxySecretRef.ResolveApiKey($"env:{name}", logger); logger.Entries.ShouldNotContain(e => e.Level == LogLevel.Warning); } finally @@ -150,7 +153,7 @@ public sealed class GalaxyDriverApiKeyResolverTests try { var ex = Should.Throw(() => - GalaxyDriver.ResolveApiKey($"file:{path}")); + GalaxySecretRef.ResolveApiKey($"file:{path}")); ex.Message.ShouldContain("empty"); } finally diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy.Tests/GatewayGalaxySubscriberClassifyTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy.Tests/GatewayGalaxySubscriberClassifyTests.cs new file mode 100644 index 00000000..b26a27cb --- /dev/null +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy.Tests/GatewayGalaxySubscriberClassifyTests.cs @@ -0,0 +1,45 @@ +using Shouldly; +using Xunit; +using ZB.MOM.WW.MxGateway.Contracts.Proto; +using ZB.MOM.WW.OtOpcUa.Driver.Galaxy.Runtime; + +namespace ZB.MOM.WW.OtOpcUa.Driver.Galaxy.Tests; + +/// +/// Driver.Galaxy-019 — pins : +/// a SetBufferedUpdateInterval reply is only "applied" (and therefore cacheable +/// as the last-applied interval) when the gateway returns . +/// means the COM-side set did NOT take +/// effect, so caching it would suppress the retry on the next subscribe — it must classify +/// as not-applied, as must any other unexpected code or a missing status. +/// +public sealed class GatewayGalaxySubscriberClassifyTests +{ + /// Ok is the only code that records the interval as applied. + [Fact] + public void Ok_classifies_as_applied() + { + GatewayGalaxySubscriber.ClassifyIntervalReply(ProtocolStatusCode.Ok).ShouldBeTrue(); + } + + /// MxaccessFailure must NOT cache — the COM-side set did not apply. + [Fact] + public void MxaccessFailure_classifies_as_not_applied() + { + GatewayGalaxySubscriber.ClassifyIntervalReply(ProtocolStatusCode.MxaccessFailure).ShouldBeFalse(); + } + + /// Any other unexpected code is treated as not-applied. + [Fact] + public void Unexpected_code_classifies_as_not_applied() + { + GatewayGalaxySubscriber.ClassifyIntervalReply(ProtocolStatusCode.Unspecified).ShouldBeFalse(); + } + + /// A missing protocol status (null) is treated as not-applied. + [Fact] + public void Null_classifies_as_not_applied() + { + GatewayGalaxySubscriber.ClassifyIntervalReply(null).ShouldBeFalse(); + } +}