Joseph Doherty
960d76ffcb
Review at HEAD 7286d320. Driver.Galaxy.Browser-001 (High): MapSecurityClass codes 2-6 were
all shifted vs the runtime SecurityClassification enum (wrong security labels in the picker)
-> corrected all 7 arms + tests. -002: DisposeAsync swallows concurrent ObjectDisposedException.
-003 (ResolveApiKey dup) deferred to Contracts.
7.0 KiB
Code Review — Driver.Galaxy.Browser
| Field | Value |
|---|---|
| Module | src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy.Browser |
| 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.Browser-001 (High): MapSecurityClass codes are shifted — mismatches the runtime SecurityMap |
| 2 | OtOpcUa conventions | No issues found |
| 3 | Concurrency & thread safety | Driver.Galaxy.Browser-002 (Medium): DisposeAsync has a TOCTOU race on _rootGate.Dispose() |
| 4 | Error handling & resilience | No issues found |
| 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 |
| 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 |
Findings
Driver.Galaxy.Browser-001
| Field | Value |
|---|---|
| Severity | High |
| Category | Correctness & logic bugs |
| Location | src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy.Browser/GalaxyBrowseSession.cs:152 |
| Status | Resolved |
Description: GalaxyBrowseSession.MapSecurityClass maps Galaxy security_classification
integer codes incorrectly. The Browser's switch arms are:
| Code | Browser output | Runtime SecurityMap / SecurityClassification enum |
|---|---|---|
| 0 | "FreeAccess" | FreeAccess ✓ |
| 1 | "Operate" | Operate ✓ |
| 2 | "Tune" | SecuredWrite ✗ |
| 3 | "Configure" | VerifiedWrite ✗ |
| 4 | "ViewOnly" | Tune ✗ |
| 5 | "Unknown(5)" | Configure ✗ |
| 6 | "Unknown(6)" | ViewOnly ✗ |
Codes 2–6 are all wrong. The AdminUI attribute side-panel labels "SecuredWrite" attributes
as "Tune" and "VerifiedWrite" attributes as "Configure", causing operators to misread
write-protection levels when selecting Galaxy tags. A tag the operator thinks is
"Tune"-restricted is actually read-only (SecuredWrite / VerifiedWrite → ViewOnly from
the OPC UA server's perspective). The correct mapping is already implemented in the
sibling Driver.Galaxy/Browse/SecurityMap.cs and matches the SecurityClassification
enum's integer assignments exactly.
Recommendation: Fix MapSecurityClass to match SecurityClassification enum ordinals:
0→FreeAccess, 1→Operate, 2→SecuredWrite, 3→VerifiedWrite, 4→Tune, 5→Configure,
6→ViewOnly; unknown → "Unknown({raw})". Add a unit test asserting each code.
Resolution: Fixed 2026-06-19. Corrected all seven code-to-label arms in
MapSecurityClass to match SecurityClassification enum ordinals. Regression tests
MapSecurityClass_maps_all_known_codes and
MapSecurityClass_unknown_code_returns_Unknown_label added.
Driver.Galaxy.Browser-002
| Field | Value |
|---|---|
| Severity | Medium |
| Category | Concurrency & thread safety |
| Location | src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy.Browser/GalaxyBrowseSession.cs:167 |
| Status | Resolved |
Description: DisposeAsync uses a non-atomic read-then-write pattern on the
_disposed volatile field:
if (_disposed) return;
_disposed = true;
_rootGate.Dispose();
Two concurrent callers can both observe _disposed == false, both set it to true, and
both call _rootGate.Dispose(). The second call throws ObjectDisposedException from
SemaphoreSlim.Dispose(), which propagates uncaught because the try/catch that
follows only wraps _client.DisposeAsync(). The BrowseSessionReaper (background
service) and a browser-side "Close" button can race in production.
Recommendation: Wrap _rootGate.Dispose() in a try/catch (ObjectDisposedException)
mirroring the existing client disposal pattern, or use an atomic int guard via
Interlocked.Exchange.
Resolution: Fixed 2026-06-19. Added try { _rootGate.Dispose(); } catch (ObjectDisposedException) { }
mirroring the existing pattern for _client.DisposeAsync(). Regression test
DisposeAsync_concurrent_calls_do_not_throw added.
Driver.Galaxy.Browser-003
| Field | Value |
|---|---|
| Severity | Low |
| Category | Code organization & conventions |
| Location | src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy.Browser/GalaxyDriverBrowser.cs:149 |
| Status | Open |
Description: GalaxyDriverBrowser.ResolveApiKey is a verbatim copy of
GalaxyDriver.ResolveApiKey. The comment acknowledges this and explains why the Browser
project intentionally does not reference Driver.Galaxy. However, Finding
Driver.Galaxy.Browser-001 (the MapSecurityClass drift) demonstrates that duplicated
logic diverges. If a new secret-ref prefix (e.g. vault:) is added to the runtime
resolver, the Browser version will silently fall through to the cleartext-literal arm
and emit a spurious warning.
Recommendation: Extract ResolveApiKey into the Driver.Galaxy.Contracts project,
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)
Driver.Galaxy.Browser-004
| Field | Value |
|---|---|
| Severity | Low |
| Category | Testing coverage |
| Location | tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy.Browser.Tests/GalaxyBrowseSessionTests.cs |
| Status | Resolved |
Description: GalaxyBrowseSession.MapSecurityClass (all seven codes plus the
Unknown(N) fallback) had zero unit-test coverage. The existing test comment correctly
notes that RootAsync/ExpandAsync/AttributesAsync traversal is blocked by the internal
transport seam, but MapSecurityClass is a pure static method with no gateway dependency
— it is entirely testable in the unit suite. Finding Driver.Galaxy.Browser-001 (wrong
mapping for codes 2–6) would have been caught immediately had these tests existed.
Recommendation: Add [Fact] tests for each of the seven known codes (0–6) and the
unknown-code fallback.
Resolution: Fixed 2026-06-19. Tests MapSecurityClass_maps_all_known_codes and
MapSecurityClass_unknown_code_returns_Unknown_label added, covering all codes 0–6
plus an out-of-range value.