Harden code-review tooling and align REVIEW-PROCESS.md with mxaccessgw
- regen-readme.py: use `python` not the broken `python3` Store alias in the generated note and docstring; --check now also fails when a module header's "Open findings" count disagrees with finding statuses or a finding has an unrecognised Status (find_inconsistencies) - REVIEW-PROCESS.md: rewritten for mxaccessgw (was describing ScadaLink) — MxGateway.* modules, "mxaccessgw conventions" checklist category, gateway.md/docs/ design context, `python` command - scripts/check-code-reviews-readme.ps1: CI/pre-commit wrapper for regen-readme.py --check - code-reviews/test_regen_readme.py: dependency-free parser tests - code-reviews/README.md: regenerated Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
Joseph Doherty
+84
-59
@@ -1,67 +1,84 @@
|
||||
# Code Review Process
|
||||
|
||||
This document describes how to perform a comprehensive, per-module code review of
|
||||
the ScadaLink codebase and how to track findings to resolution.
|
||||
the `mxaccessgw` codebase and how to track findings to resolution.
|
||||
|
||||
A **module** is one buildable project under `src/` (e.g. `src/ScadaLink.TemplateEngine`).
|
||||
Each module has its own folder under `code-reviews/` containing a single `findings.md`.
|
||||
A **module** is one buildable project under `src/` (e.g. `src/MxGateway.Worker`).
|
||||
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 `ScadaLink.` prefix stripped.
|
||||
1. Pick the module to review. Its folder is `code-reviews/<Module>/` where
|
||||
`<Module>` is the project name with the `MxGateway.` prefix stripped — so
|
||||
`src/MxGateway.Server` is reviewed in `code-reviews/Server/`.
|
||||
2. Identify the design context for the module:
|
||||
- Its component design doc: `docs/requirements/Component-<Name>.md`.
|
||||
- The relevant **Key Design Decisions** in `CLAUDE.md`.
|
||||
- `docs/requirements/HighLevelReqs.md` for cross-cutting requirements.
|
||||
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.
|
||||
- `gateway.md` — top-level architecture, command/event surface, IPC envelope,
|
||||
STA thread model, fault handling.
|
||||
- The relevant component design docs under `docs/` (e.g.
|
||||
`docs/MxAccessWorkerInstanceDesign.md`, `docs/GatewayProcessDesign.md`,
|
||||
`docs/Sessions.md`, `docs/Authentication.md`, `docs/GalaxyRepository.md`).
|
||||
- `docs/DesignDecisions.md` for the v1 design choices.
|
||||
- The **Repository-Specific Conventions** and **Process / Platform Notes** in
|
||||
`CLAUDE.md`.
|
||||
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` and fill in the header table
|
||||
(reviewer, date, commit SHA).
|
||||
(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.
|
||||
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.
|
||||
2. **Akka.NET conventions** — supervision strategies (Resume for coordinators, Stop
|
||||
for short-lived actors), `Tell` for hot paths / `Ask` only at system boundaries,
|
||||
message immutability, no blocking on non-blocking dispatchers, no `sender`/`this`
|
||||
captured in closures (`PipeTo` instead), correlation IDs on request/response.
|
||||
3. **Concurrency & thread safety** — shared mutable state, actor state mutated only
|
||||
on the actor thread, race conditions, correct use of async/await.
|
||||
4. **Error handling & resilience** — exception paths, store-and-forward integration,
|
||||
reconnect/retry logic, failover behaviour, transient vs permanent error
|
||||
classification, graceful degradation.
|
||||
5. **Security** — authentication/authorization checks, input validation, the script
|
||||
trust model (forbidden APIs: `System.IO`, `Process`, `Threading`, `Reflection`,
|
||||
raw network), secret handling, SQL/LDAP injection, logging of sensitive data.
|
||||
6. **Performance & resource management** — `IDisposable` disposal, stream/connection
|
||||
lifetimes, buffering and back-pressure, unnecessary allocations, N+1 queries.
|
||||
7. **Design-document adherence** — does the code match `Component-<Name>.md` and the
|
||||
relevant CLAUDE.md decisions? Flag both code that drifts from the design and design
|
||||
docs that are now stale.
|
||||
8. **Code organization & conventions** — persistence-ignorant POCO entities in
|
||||
Commons, repository interfaces in Commons / implementations in ConfigurationDatabase,
|
||||
namespace hierarchy, Options pattern (options classes owned by component projects),
|
||||
additive-only message contract evolution.
|
||||
9. **Testing coverage** — are the module's behaviours covered by tests in `tests/`?
|
||||
Note untested critical paths and missing edge-case tests.
|
||||
1. **Correctness & logic bugs** — off-by-one, null handling, incorrect
|
||||
conditionals, misuse of APIs, broken edge cases.
|
||||
2. **mxaccessgw conventions** — the rules in `CLAUDE.md` and the style guides
|
||||
under `docs/style-guides/`: the gateway never instantiates MXAccess COM
|
||||
directly; all MXAccess COM calls run on the worker's dedicated STA thread and
|
||||
the STA loop pumps Windows messages; IPC uses one bidirectional named pipe per
|
||||
worker carrying length-prefixed `WorkerEnvelope` protobuf frames; MXAccess
|
||||
parity is the contract (don't "fix" surprising MXAccess behaviour, never
|
||||
synthesize events); one worker and one event subscriber per session; the
|
||||
gateway terminates orphan workers on startup and does not reattach; C# style
|
||||
(file-scoped namespaces, `sealed` by default, `Async` suffix, MXAccess-aligned
|
||||
names); no Blazor UI component libraries; no logging of secrets or full tag
|
||||
values; generated code is never hand-edited.
|
||||
3. **Concurrency & thread safety** — shared mutable state, STA affinity, race
|
||||
conditions, correct use of `async`/`await`, locking, disposal races.
|
||||
4. **Error handling & resilience** — exception paths, worker crash / reconnect
|
||||
handling, fail-fast event backpressure, transient vs permanent error
|
||||
classification, graceful degradation, correct gRPC status codes.
|
||||
5. **Security** — authentication/authorization checks, API-key scope enforcement,
|
||||
input validation, SQL injection in the Galaxy Repository RPCs, secret
|
||||
handling, the dashboard anonymous-localhost bypass, logging of sensitive data.
|
||||
6. **Performance & resource management** — `IDisposable` disposal, pipe / stream
|
||||
/ COM lifetimes, buffering and back-pressure, unnecessary allocations on hot
|
||||
paths, N+1 queries.
|
||||
7. **Design-document adherence** — does the code match `gateway.md`, the relevant
|
||||
`docs/` component designs, `docs/DesignDecisions.md`, and `CLAUDE.md`? 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, additive-only contract evolution.
|
||||
9. **Testing coverage** — are the module's behaviours covered by tests
|
||||
(`src/MxGateway.Tests`, `src/MxGateway.Worker.Tests`,
|
||||
`src/MxGateway.IntegrationTests`)? 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`](_template/findings.md).
|
||||
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. `TemplateEngine-001`). IDs are permanent even after resolution.
|
||||
- **Finding ID** — `<Module>-NNN`, numbered sequentially within the module and
|
||||
never reused (e.g. `Worker-001`). IDs are permanent even after resolution.
|
||||
- **Severity:**
|
||||
- **Critical** — data loss, security breach, crash/deadlock, or cluster-wide outage.
|
||||
- **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.
|
||||
@@ -70,44 +87,52 @@ using the entry format in [`_template/findings.md`](_template/findings.md).
|
||||
- **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 refresh the base README (step 5).
|
||||
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:
|
||||
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).
|
||||
- `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** and
|
||||
drop off the base README's pending list. `Open` and `In Progress` are **pending**.
|
||||
`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 (process overview, the
|
||||
Pending Findings tables, and the Module Status table). It is **generated** from the
|
||||
`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:
|
||||
|
||||
```
|
||||
python3 code-reviews/regen-readme.py
|
||||
python code-reviews/regen-readme.py
|
||||
```
|
||||
|
||||
`regen-readme.py --check` exits non-zero if `README.md` is stale, for use in CI.
|
||||
`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.
|
||||
|
||||
> The repo's installed `python` is the real interpreter; the bare `python3`
|
||||
> alias 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.
|
||||
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