Add per-library code-review scaffolding for the ZB.MOM.WW.* shared libs
Adapts the code-reviews convention (process, README generator, template) from the ScadaBridge app model (per-src/-module, Akka conventions) to scadaproj's reality: six shared libraries reviewed against their components/ specs. - REVIEW-PROCESS.md: review unit is a library; library->component-spec mapping; checklist re-targeted for reusable .NET libs (public API/semver, packaging & dependency hygiene, spec/shared-contract adherence) instead of actor/supervision. - _template/findings.md: library/packages/component-spec/shared-contract header. - regen-readme.py: per-library prose, data-driven Summary, '-' for unreviewed. - Seed Auth/Theme/Health/Telemetry/Configuration/Audit findings stubs (0 findings). - README.md generated; --check passes.
This commit is contained in:
Joseph Doherty
@@ -0,0 +1,152 @@
|
||||
# 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](../components/README.md)). 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/`](../components/auth/) |
|
||||
| `Theme` | `ZB.MOM.WW.Theme/` | single RCL | [`components/ui-theme/`](../components/ui-theme/) |
|
||||
| `Health` | `ZB.MOM.WW.Health/` | `Health`, `.Akka`, `.EntityFrameworkCore` | [`components/health/`](../components/health/) |
|
||||
| `Telemetry` | `ZB.MOM.WW.Telemetry/` | `Telemetry`, `.Serilog` | [`components/observability/`](../components/observability/) |
|
||||
| `Configuration` | `ZB.MOM.WW.Configuration/` | single | [`components/configuration/`](../components/configuration/) |
|
||||
| `Audit` | `ZB.MOM.WW.Audit/` | single | [`components/audit/`](../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`](../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`](_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.
|
||||
Reference in New Issue
Block a user