lmxopcua/REVIEW-PROCESS.md

Code Review Process

This document describes how to perform a comprehensive, per-module code review of the lmxopcua codebase (the ZB.MOM.WW.OtOpcUa OPC UA server) and how to track findings to resolution.

A module is one buildable project under src/ (e.g. src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy) or one test project under tests/ (e.g. tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy.Tests). Each module has its own folder under code-reviews/ containing a single findings.md.

1. Before you start

1. Pick the module to review. Its folder is code-reviews/<Module>/, where <Module> is the project name with the ZB.MOM.WW.OtOpcUa. prefix stripped:

   • src/Server/ZB.MOM.WW.OtOpcUa.Server is reviewed in code-reviews/Server/.
   • src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy → code-reviews/Driver.Galaxy/.
   • src/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions → code-reviews/Core.Abstractions/.
   • tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy.Tests → code-reviews/Driver.Galaxy.Tests/.

   The solution ZB.MOM.WW.OtOpcUa.slnx enumerates every project; src/ is grouped into Core/, Server/, Drivers/, Client/, and Tooling/.

2. Identify the design context for the module:

   • CLAUDE.md — project goal, the data-flow architecture, the contained-name vs tag-name concept, and the Library Preferences / build & runtime constraints.
   • StyleGuide.md — repository code-style conventions.
   • The relevant docs under docs/ and docs/v2/ — e.g. docs/OpcUaServer.md, docs/AddressSpace.md, docs/ReadWriteOperations.md, docs/security.md, docs/Redundancy.md, docs/ScriptedAlarms.md, docs/AlarmTracking.md, docs/ServiceHosting.md, docs/v2/plan.md, docs/v2/acl-design.md, docs/v2/driver-specs.md, docs/v2/driver-stability.md, the docs/v2/Galaxy.*.md set, and the driver notes under docs/drivers/.
   • The auto-memory index at ~/.claude/projects/.../memory/MEMORY.md records non-obvious project decisions and is worth a scan before a review.

3. Record the exact commit being reviewed: git rev-parse --short HEAD. Every review is a snapshot — a finding only means something relative to a known commit.

4. Open code-reviews/<Module>/findings.md (copy it from code-reviews/_template/findings.md if it does not exist yet) and fill in the header table (reviewer, date, commit SHA, status).

2. Review checklist

Work through every category below for the module. A comprehensive review means the checklist is completed even where it produces no findings — record "No issues found" for a category rather than leaving it ambiguous.

1. Correctness & logic bugs — off-by-one, null handling, incorrect conditionals, misuse of APIs, broken edge cases, wrong data-type mapping.
2. OtOpcUa conventions — the rules in CLAUDE.md and StyleGuide.md: Galaxy access flows through the in-process GalaxyDriver over gRPC to the separately installed mxaccessgw gateway — nothing in this repo loads MXAccess COM directly; browse uses contained names and runtime read/write uses tag names (tag_name.AttributeName); authorization decisions happen in DriverNodeManager at the server layer, never in driver-level code — drivers only report SecurityClassification as metadata; .NET 10 / AnyCPU; Serilog with a rolling daily file sink; xUnit + Shouldly for unit tests; the .NET generic host with AddWindowsService for the Server and Admin hosts; the OPC Foundation UA .NET Standard stack for OPC UA; generated code is not hand-edited.
3. Concurrency & thread safety — shared mutable state, race conditions, correct use of async/await, locking, disposal races, background-loop and reconnect-supervisor lifetimes.
4. Error handling & resilience — exception paths, driver/gateway reconnect handling, transient vs permanent error classification, graceful degradation, correct OPC UA StatusCodes, address-space rebuild on redeploy.
5. Security — OPC UA transport security profiles (SecurityProfileResolver), LDAP bind authentication and the group→permission mapping (LdapUserAuthenticator), ACL enforcement at the DriverNodeManager layer, input validation, SQL injection in the ConfigDb / Galaxy Repository queries, certificate handling, and secret handling (no logging of credentials, LDAP service-account passwords, or API keys).
6. Performance & resource management — IDisposable disposal, gRPC channel / stream / session lifetimes, buffering and back-pressure on event pumps, unnecessary allocations on hot paths, N+1 queries.
7. Design-document adherence — does the code match CLAUDE.md, the relevant docs/ and docs/v2/ designs? Flag both code that drifts from the design and design docs that are now stale.
8. Code organization & conventions — namespace hierarchy, project layout, the Options pattern, separation of concerns, the capability-interface seams (IReadable, IWritable, ISubscribable, IAlarmSource, etc.).
9. Testing coverage — are the module's behaviours covered? Unit suites are *.Tests (xUnit + Shouldly); integration suites are *.IntegrationTests and need their Docker fixture up; DB-backed tests in *.Configuration.Tests, *.Admin.Tests, and *.Server.Tests need the central SQL Server. Note untested critical paths and missing edge-case tests.
10. Documentation & comments — XML doc accuracy, misleading or stale comments, undocumented non-obvious behaviour.

3. Recording findings

Add one entry per finding to the ## Findings section of the module's findings.md, using the entry format in _template/findings.md.

• Finding ID — <Module>-NNN, numbered sequentially within the module and never reused (e.g. Driver.Galaxy-001). IDs are permanent even after resolution.
• Severity:
  • Critical — data loss, security breach, crash/deadlock, or outage.
  • High — incorrect behaviour with significant impact; no safe workaround.
  • Medium — incorrect or risky behaviour with limited impact or a workaround.
  • Low — minor issues, style, maintainability, documentation.
• Category — one of the 10 checklist categories above.
• Location — file:line (clickable), or a list of locations.
• Description — what is wrong and why it matters.
• Recommendation — concrete suggested fix.

After recording findings, update the module header table (status, open-finding count) and regenerate the base README (step 5).

4. Marking an item resolved

Findings are never deleted — they are an audit trail. To close one, change its Status and complete the Resolution field:

• Open — newly recorded, not yet addressed.
• In Progress — a fix is actively being worked on.
• Resolved — fixed. The Resolution field must state the fixing commit SHA, the date, and a one-line description of the fix.
• Won't Fix — intentionally not fixed. The Resolution field must justify why.
• Deferred — valid but postponed. The Resolution field must say what it is waiting on (e.g. a tracked issue or a later milestone).

Resolved, Won't Fix, and Deferred findings are all considered closed. Open and In Progress are pending and appear in the base README's Pending Findings table.

5. Updating the base README

code-reviews/README.md holds the single cross-module view (the Module Status table and the Pending / Closed Findings tables). It is generated from the per-module findings.md files — do not edit it by hand.

After any review or status change, regenerate it:

python code-reviews/regen-readme.py

regen-readme.py --check exits non-zero if README.md is stale, if a module header's Open findings count disagrees with its finding statuses, or if a finding carries an unrecognised Status value. The PowerShell wrapper scripts/check-code-reviews-readme.ps1 runs that check and is the intended hook for CI or a pre-commit step. code-reviews/test_regen_readme.py covers the generator itself (python code-reviews/test_regen_readme.py).

The repo's installed python is the real interpreter; the bare python3 alias on this box resolves to the Windows Store stub and fails. Use python.

The per-module findings.md files are the source of truth; README.md is the aggregated index and must always agree with them — which the script guarantees.

6. Re-reviewing a module

Re-reviews append to the same findings.md. Update the header to the new commit and date, continue the finding numbering from the last used ID, and leave prior findings (including closed ones) in place as history.