# 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/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 `MxGateway.`
     prefix stripped — `src/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/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`](code-reviews/_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.