scadaproj/code-reviews/REVIEW-PROCESS.md

Code Review Process

This document describes how to perform a comprehensive, per-library code review of the shared libraries hosted in scadaproj and how to track findings to resolution.

scadaproj is an umbrella workspace, but it hosts first-party source: the six ZB.MOM.WW.* shared libraries (the realized output of the component normalizations). Those libraries are the code that gets reviewed here. A library is one self-contained solution at the repo root (its own .slnx, with src/ and tests/) — e.g. ZB.MOM.WW.Auth/. Each library has its own folder under code-reviews/ containing a single findings.md.

The review unit is the library, not the individual NuGet package. A library that ships several packages (Auth → 4, Health → 3, Telemetry → 2) is reviewed as a whole; findings point at the specific package/file in their Location.

The six libraries and their design context

Each library is reviewed against its normalized component spec. The library folder name is the ZB.MOM.WW. prefix stripped; note the spec directory is not always the same word.

Library folder	Library root	Packages	Component spec
Auth	ZB.MOM.WW.Auth/	Abstractions, Ldap, ApiKeys, AspNetCore	components/auth/
Theme	ZB.MOM.WW.Theme/	single RCL	components/ui-theme/
Health	ZB.MOM.WW.Health/	Health, .Akka, .EntityFrameworkCore	components/health/
Telemetry	ZB.MOM.WW.Telemetry/	Telemetry, .Serilog	components/observability/
Configuration	ZB.MOM.WW.Configuration/	single	components/configuration/
Audit	ZB.MOM.WW.Audit/	single	components/audit/

1. Before you start

1. Pick the library to review. Its review folder is code-reviews/<Library>/ where <Library> is the table above (the ZB.MOM.WW. prefix stripped).
2. Identify the design context for the library:
   • Its component spec: components/<component>/spec/SPEC.md.
   • Its proposed shared contract: components/<component>/shared-contract/ZB.MOM.WW.<Library>.md.
   • The library's own README.md and the relevant row in the Component normalization table in the root CLAUDE.md.
   • The adoption backlog components/<component>/GAPS.md (what is deliberately deferred).
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. Build and test the library so findings are grounded in a green (or known-red) baseline: cd ZB.MOM.WW.<Library> && dotnet test.
5. Open code-reviews/<Library>/findings.md and fill in the header table (reviewer, date, commit SHA).

2. Review checklist

Work through every category below for the library. 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.

These libraries are reusable packages consumed by three separate apps (OtOpcUa, MxAccessGateway, ScadaBridge), so the bar is API stability and minimal blast radius: a defect here can break every consumer at once.

1. Correctness & logic bugs — off-by-one, null handling, incorrect conditionals, misuse of BCL/framework APIs, broken edge cases.
2. Public API surface & compatibility — the public surface is intentional and minimal; naming follows .NET conventions; nullable reference annotations are correct; sealed / abstract / virtual choices are deliberate; no internal types leak through public signatures; evolution is additive-only with semver discipline (libraries are at 0.1.0).
3. Concurrency & thread safety — these libraries run inside concurrent hosts; options and value types are immutable, singletons/statics are thread-safe, no shared mutable state, correct async/await, no sync-over-async.
4. Error handling & resilience — exceptions thrown to consumers are appropriate and documented; argument validation / guard clauses (ArgumentNullException etc.); no swallowed exceptions; fail-fast vs. graceful degradation is deliberate.
5. Security & secret handling — Auth peppered-HMAC API keys and LDAP-injection surface; redaction seams (ILogRedactor in Telemetry, IAuditRedactor in Audit); input validation; no secrets or PII in messages, logs, or exception text.
6. Performance & resource management — IDisposable correctness and disposal, allocations on hot paths, buffering/back-pressure, no unnecessary work in DI registration, async all the way down.
7. Spec & shared-contract adherence — does the code match the library's components/<component>/spec/SPEC.md and shared-contract/ZB.MOM.WW.<Library>.md? Flag both code that drifts from the normalized contract and spec/contract docs that are now stale relative to the code.
8. Packaging, dependencies & project layout — minimal dependency footprint (e.g. Audit's only non-BCL dependency is Microsoft.Extensions.DependencyInjection.Abstractions); correct target framework (.NET 10); package id/version metadata; dotnet pack produces the expected nupkg set; central versions in Directory.Packages.props; no public dependency leakage; Abstractions-vs-implementation split; Options pattern owned by the library; AddZb* DI registration ergonomics.
9. Testing coverage — are the library's behaviours covered by tests in its tests/ project? Note untested critical paths, missing edge-case tests, and whether the public contract (not just internals) is exercised.
10. Documentation & XML docs — XML doc accuracy and presence on the public API (these are libraries — public docs matter), library README.md correctness, misleading or stale comments, undocumented non-obvious behaviour.

3. Recording findings

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

• Finding ID — <Library>-NNN, numbered sequentially within the library and never reused (e.g. Auth-001, Telemetry-003). IDs are permanent even after resolution.
• Severity:
  • Critical — security breach, data loss, or a defect that crashes/corrupts or deadlocks every consuming app (e.g. a broken public contract, secret leakage).
  • High — incorrect behaviour with significant impact across consumers, or a public-API / back-compat break, with 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 library header table (status, open-finding count) and refresh 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, a GAPS.md item, or a later milestone).

Resolved, Won't Fix, and Deferred findings are all considered closed and drop off the base README's pending list. Open and In Progress are pending.

5. Updating the base README

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

After any review or status change, regenerate it:

python3 code-reviews/regen-readme.py

regen-readme.py --check exits non-zero if README.md is stale, for use in CI.

The per-library 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 library

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.