From 45711e437d34848aba71dc40b54a087bfcccb758 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Fri, 19 Jun 2026 12:22:53 -0400 Subject: [PATCH] review(Driver.Galaxy.Contracts): first review; [Range] guard on EventPumpChannelCapacity First review at 7286d320. -002 (Medium) fixed: EventPumpChannelCapacity now [Range(1,..)] (a 0 passed AdminUI validation but faulted the driver at InitializeAsync). -001 stale finding-id doc fixed. -003 ResolveApiKey dedup Open (3-module coordination). --- .../Driver.Galaxy.Contracts/findings.md | 140 ++++++++++++++++++ .../GalaxyDriverOptions.cs | 8 +- 2 files changed, 145 insertions(+), 3 deletions(-) create mode 100644 code-reviews/Driver.Galaxy.Contracts/findings.md diff --git a/code-reviews/Driver.Galaxy.Contracts/findings.md b/code-reviews/Driver.Galaxy.Contracts/findings.md new file mode 100644 index 00000000..ff7c7dc4 --- /dev/null +++ b/code-reviews/Driver.Galaxy.Contracts/findings.md @@ -0,0 +1,140 @@ +# Code Review — Driver.Galaxy.Contracts + + + +| Field | Value | +|---|---| +| Module | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy.Contracts` | +| Reviewer | Claude Code | +| Review date | 2026-06-19 | +| Commit reviewed | `7286d320` | +| Status | Reviewed | +| Open findings | 1 | + +## Checklist coverage + +A comprehensive review completes every category, recording "No issues found" where +a category produced nothing rather than leaving it blank. + +| # | Category | Result | +|---|---|---| +| 1 | Correctness & logic bugs | Driver.Galaxy.Contracts-002 (Medium): EventPumpChannelCapacity has no Range guard; EventPump enforces ≥ 1 at runtime | +| 2 | OtOpcUa conventions | No issues found | +| 3 | Concurrency & thread safety | No issues found (pure data-contract records; no mutable shared state) | +| 4 | Error handling & resilience | No issues found | +| 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 | +| 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 | + +## Findings + + + +### Driver.Galaxy.Contracts-001 + +| Field | Value | +|---|---| +| Severity | Low | +| Category | Documentation & comments | +| Location | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy.Contracts/GalaxyDriverOptions.cs:43` | +| Status | Resolved | + +**Description:** The XML doc on `GalaxyGatewayOptions.ApiKeySecretRef` contained the +parenthetical `(Driver.Galaxy-010)` — an internal code-review finding ID — embedded +in the shipped `` tag. Once emitted via `dotnet doc` or IntelliSense, this +noise is meaningless to any consumer of the contracts assembly (AdminUI, Driver.Galaxy, +Driver.Galaxy.Browser) and will become stale if the finding ID is ever renumbered or +retired. The underlying issue that spawned Driver.Galaxy-010 (startup warning on +cleartext-literal API key) was resolved in that review; the reference in source code +is now pure noise. + +**Recommendation:** Remove the parenthetical `(Driver.Galaxy-010)` from the XML doc. +The surrounding sentence already explains the behavior; the cross-reference adds no +value. + +**Resolution:** Fixed 2026-06-19 in this review pass. Removed `(Driver.Galaxy-010)` from +the `ApiKeySecretRef` doc comment. Verified by build (no test project). + +--- + +### Driver.Galaxy.Contracts-002 + +| Field | Value | +|---|---| +| Severity | Medium | +| Category | Correctness & logic bugs | +| Location | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy.Contracts/GalaxyDriverOptions.cs:91` | +| Status | Resolved | + +**Description:** `GalaxyMxAccessOptions.EventPumpChannelCapacity` defaults to `50_000` +but has no `[Range]` annotation. `EventPump` (the consumer) enforces `channelCapacity >= 1` +via `ArgumentOutOfRangeException` at construction: + +```csharp +if (channelCapacity < 1) + throw new ArgumentOutOfRangeException(nameof(channelCapacity), + "channelCapacity must be >= 1; ..."); +``` + +An operator who enters `0` or a negative number in the AdminUI Galaxy driver form will +pass Blazor's `DataAnnotationsValidator` silently (no annotation to check), save the +config to the central config DB, and only encounter the failure at `GalaxyDriver.InitializeAsync` +time — surfaced as a cryptic internal `ArgumentOutOfRangeException` rather than a +friendly form-validation message. The gap between the option's declared type (`int`) and +the consumer's enforced floor (`>= 1`) is not visible at the contracts layer. + +**Recommendation:** Add `[Range(1, int.MaxValue)]` to `EventPumpChannelCapacity` and +update the `` doc to state the minimum. The `[Range]` attribute is already used on +`ProbeTimeoutSeconds` in the same class; the pattern is established. + +**Resolution:** Fixed 2026-06-19 in this review pass. Added `[Range(1, int.MaxValue)]` +to the `EventPumpChannelCapacity` positional parameter and extended the `` doc to +note the minimum and that `EventPump` enforces it at construction. Verified by build +(no test project). + +--- + +### Driver.Galaxy.Contracts-003 + +| Field | Value | +|---|---| +| 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 | + +**Description:** `GalaxyDriver.ResolveApiKey` (the four-form `env:`/`file:`/`dev:`/literal +resolver, ~45 LOC) is duplicated verbatim as `GalaxyDriverBrowser.ResolveApiKey`. The +Browser copy acknowledges this in its XML doc ("Slim mirror of `GalaxyDriver.ResolveApiKey` +— the runtime version lives in a sibling project the Browser intentionally doesn't +reference"). The duplication is intentional to avoid a project reference from +`Driver.Galaxy.Browser` to `Driver.Galaxy`, but the `Driver.Galaxy.Contracts` project is +already referenced by both (and by `AdminUI`). If a new secret-ref prefix (e.g. `vault:`) +is added to the runtime resolver, the Browser copy will silently fall through to the +back-compat literal arm and emit a spurious startup warning — exactly the drift that +`MapSecurityClass` suffered in `Driver.Galaxy.Browser-001`. + +This finding was first raised in the sibling review (Driver.Galaxy.Browser-003), which +identified `Driver.Galaxy.Contracts` as the natural home. Extracting the helper into +Contracts would be a one-file change with no public-contract break; both `Driver.Galaxy` +and `Driver.Galaxy.Browser` would lose their copies and delegate to the single +authoritative implementation. The current project has no package references beyond +`System.ComponentModel.DataAnnotations` (in-box). Adding a pure-BCL static helper +(`File.ReadAllText`, `Environment.GetEnvironmentVariable`) fits within this scope; the +`ILogger?` overload would require importing `Microsoft.Extensions.Logging.Abstractions`, +which ships in-box with .NET 10's BCL and adds no new NuGet dependency. + +**Recommendation:** Move `ResolveApiKey(string, ILogger?)` (and its `ILogger`-less +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.)_ 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 818cbfee..1e38f5a6 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 @@ -40,8 +40,8 @@ public sealed record GalaxyDriverOptions( /// no startup warning. /// Anything else — treated as a literal cleartext API key for back-compat. /// The resolver emits a Warning at startup so an operator who accidentally -/// committed a cleartext key sees it (Driver.Galaxy-010); production should -/// migrate to env: or file:. +/// committed a cleartext key sees it; production should migrate to env: +/// or file:. /// /// // PR 6.5 tuning notes: @@ -83,12 +83,14 @@ public sealed record GalaxyGatewayOptions( /// fan-out loop (PR 6.2). Default 50_000 = one second of headroom at 50k tags / 1Hz; /// raise it when galaxy.events.dropped shows up under transient consumer /// slowness, lower it on a memory-tight host where the headroom isn't needed. +/// Must be ≥ 1 — EventPump enforces this at construction and throws +/// if the value is zero or negative. /// public sealed record GalaxyMxAccessOptions( string ClientName, int PublishingIntervalMs = 1000, int WriteUserId = 0, - int EventPumpChannelCapacity = 50_000); + [Range(1, int.MaxValue)] int EventPumpChannelCapacity = 50_000); /// /// Galaxy Repository browse-side knobs consumed by PR 4.1's GalaxyDiscoverer.