chore: add per-module code review process and tracking infra
Adapts the code-review procedure, folder layout, template, and tooling from the sibling mxaccessgw repo to lmxopcua. - REVIEW-PROCESS.md: per-module review workflow — a module is one src/ or tests/ project (ZB.MOM.WW.OtOpcUa. prefix stripped); 10-category checklist; finding IDs/severities/statuses; re-review rules. - code-reviews/_template/findings.md: per-module findings template. - code-reviews/regen-readme.py: generates the cross-module README.md index from the per-module findings.md files; --check gates staleness and consistency. - code-reviews/test_regen_readme.py: dependency-free generator tests. - code-reviews/prompt.md: orchestration prompt for clearing the backlog. - code-reviews/README.md: generated index (no modules reviewed yet). - scripts/check-code-reviews-readme.ps1: CI / pre-commit check wrapper. Adapted to this repo: ZB.MOM.WW.OtOpcUa module naming, OtOpcUa conventions checklist (in-process GalaxyDriver + mxaccessgw, contained-name vs tag-name, ACL at DriverNodeManager), single .NET solution build/test commands, and the lmxopcua design docs. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
Joseph Doherty
162
REVIEW-PROCESS.md
Normal file
162
REVIEW-PROCESS.md
Normal file
@@ -0,0 +1,162 @@
|
||||
# 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`](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 `StatusCode`s, 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`](code-reviews/_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.
|
||||
Reference in New Issue
Block a user