mxaccessgw/REVIEW-PROCESS.md

Code Review Process

This document describes how to perform a comprehensive, per-module code review of the mxaccessgw codebase and how to track findings to resolution.

A module is one buildable project under src/ (e.g. src/ZB.MOM.WW.MxGateway.Worker) or one language client under clients/ (e.g. clients/rust). 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>/:
   • For a src/ project, <Module> is the project name with the ZB.MOM.WW.MxGateway. prefix stripped — src/ZB.MOM.WW.MxGateway.Server is reviewed in code-reviews/Server/.
   • For a language client, <Module> is Client.<Lang> — clients/rust is reviewed in code-reviews/Client.Rust/.
2. Identify the design context for the module:
   • 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, 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.
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/ZB.MOM.WW.MxGateway.Tests, src/ZB.MOM.WW.MxGateway.Worker.Tests, src/ZB.MOM.WW.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.

• 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 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.

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.