diff --git a/REVIEW-PROCESS.md b/REVIEW-PROCESS.md new file mode 100644 index 0000000..cf0f923 --- /dev/null +++ b/REVIEW-PROCESS.md @@ -0,0 +1,162 @@ +# Code Review Process + +This document describes how to perform a comprehensive, per-module code review of +the `lmxopcua` codebase (the ZB.MOM.WW.OtOpcUa OPC UA server) and how to track +findings to resolution. + +A **module** is one buildable project under `src/` (e.g. +`src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy`) or one test project under `tests/` +(e.g. `tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy.Tests`). 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//`, where + `` is the project name with the `ZB.MOM.WW.OtOpcUa.` prefix stripped: + - `src/Server/ZB.MOM.WW.OtOpcUa.Server` is reviewed in `code-reviews/Server/`. + - `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy` → `code-reviews/Driver.Galaxy/`. + - `src/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions` → `code-reviews/Core.Abstractions/`. + - `tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy.Tests` → + `code-reviews/Driver.Galaxy.Tests/`. + + The solution `ZB.MOM.WW.OtOpcUa.slnx` enumerates every project; `src/` is + grouped into `Core/`, `Server/`, `Drivers/`, `Client/`, and `Tooling/`. +2. Identify the design context for the module: + - `CLAUDE.md` — project goal, the data-flow architecture, the contained-name + vs tag-name concept, and the **Library Preferences** / build & runtime + constraints. + - `StyleGuide.md` — repository code-style conventions. + - The relevant docs under `docs/` and `docs/v2/` — e.g. `docs/OpcUaServer.md`, + `docs/AddressSpace.md`, `docs/ReadWriteOperations.md`, `docs/security.md`, + `docs/Redundancy.md`, `docs/ScriptedAlarms.md`, `docs/AlarmTracking.md`, + `docs/ServiceHosting.md`, `docs/v2/plan.md`, `docs/v2/acl-design.md`, + `docs/v2/driver-specs.md`, `docs/v2/driver-stability.md`, the + `docs/v2/Galaxy.*.md` set, and the driver notes under `docs/drivers/`. + - The auto-memory index at + `~/.claude/projects/.../memory/MEMORY.md` records non-obvious project + decisions and is worth a scan before a review. +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//findings.md` (copy it from + [`code-reviews/_template/findings.md`](code-reviews/_template/findings.md) if it + does not exist yet) 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, wrong data-type mapping. +2. **OtOpcUa conventions** — the rules in `CLAUDE.md` and `StyleGuide.md`: Galaxy + access flows through the in-process `GalaxyDriver` over gRPC to the separately + installed `mxaccessgw` gateway — nothing in this repo loads MXAccess COM + directly; browse uses **contained names** and runtime read/write uses + **tag names** (`tag_name.AttributeName`); authorization decisions happen in + `DriverNodeManager` at the server layer, never in driver-level code — drivers + only report `SecurityClassification` as metadata; .NET 10 / AnyCPU; Serilog + with a rolling daily file sink; xUnit + Shouldly for unit tests; the .NET + generic host with `AddWindowsService` for the Server and Admin hosts; the OPC + Foundation UA .NET Standard stack for OPC UA; generated code is not + hand-edited. +3. **Concurrency & thread safety** — shared mutable state, race conditions, + correct use of `async`/`await`, locking, disposal races, background-loop and + reconnect-supervisor lifetimes. +4. **Error handling & resilience** — exception paths, driver/gateway reconnect + handling, transient vs permanent error classification, graceful degradation, + correct OPC UA `StatusCode`s, address-space rebuild on redeploy. +5. **Security** — OPC UA transport security profiles (`SecurityProfileResolver`), + LDAP bind authentication and the group→permission mapping + (`LdapUserAuthenticator`), ACL enforcement at the `DriverNodeManager` layer, + input validation, SQL injection in the `ConfigDb` / Galaxy Repository queries, + certificate handling, and secret handling (no logging of credentials, LDAP + service-account passwords, or API keys). +6. **Performance & resource management** — `IDisposable` disposal, gRPC channel / + stream / session lifetimes, buffering and back-pressure on event pumps, + unnecessary allocations on hot paths, N+1 queries. +7. **Design-document adherence** — does the code match `CLAUDE.md`, the relevant + `docs/` and `docs/v2/` designs? 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, the capability-interface seams + (`IReadable`, `IWritable`, `ISubscribable`, `IAlarmSource`, etc.). +9. **Testing coverage** — are the module's behaviours covered? Unit suites are + `*.Tests` (xUnit + Shouldly); integration suites are `*.IntegrationTests` and + need their Docker fixture up; DB-backed tests in `*.Configuration.Tests`, + `*.Admin.Tests`, and `*.Server.Tests` need the central SQL Server. 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** — `-NNN`, numbered sequentially within the module and + never reused (e.g. `Driver.Galaxy-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. `code-reviews/test_regen_readme.py` covers the +generator itself (`python code-reviews/test_regen_readme.py`). + +> The repo's installed `python` is the real interpreter; the bare `python3` +> alias on this box 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. diff --git a/code-reviews/.gitignore b/code-reviews/.gitignore new file mode 100644 index 0000000..0768482 --- /dev/null +++ b/code-reviews/.gitignore @@ -0,0 +1,2 @@ +# regen-readme.py / test_regen_readme.py bytecode cache +__pycache__/ diff --git a/code-reviews/README.md b/code-reviews/README.md new file mode 100644 index 0000000..b287336 --- /dev/null +++ b/code-reviews/README.md @@ -0,0 +1,25 @@ +# Code Reviews + + + +Cross-module code review index for the OtOpcUa server codebase (`lmxopcua`). The review process is defined in [../REVIEW-PROCESS.md](../REVIEW-PROCESS.md). + +Each module's `findings.md` is the source of truth; this file is generated from them by `regen-readme.py` and must not be edited by hand. + +## Module status + +| Module | Reviewer | Date | Commit | Status | Open | Total | +|---|---|---|---|---|---|---| +| _no modules reviewed yet_ | | | | | | | + +## Pending findings + +Findings with status `Open` or `In Progress`, ordered by severity. + +_No pending findings._ + +## Closed findings + +Findings with status `Resolved`, `Won't Fix`, or `Deferred`. + +_No closed findings._ diff --git a/code-reviews/_template/findings.md b/code-reviews/_template/findings.md new file mode 100644 index 0000000..7e5da07 --- /dev/null +++ b/code-reviews/_template/findings.md @@ -0,0 +1,53 @@ +# Code Review — <Module> + + + +| Field | Value | +|---|---| +| Module | `src//ZB.MOM.WW.OtOpcUa.` | +| Reviewer | | +| Review date | | +| Commit reviewed | `` | +| Status | Not started | +| Open findings | 0 | + +## Checklist coverage + +A comprehensive review completes every category, recording "No issues found" where +a category produced nothing rather than leaving it blank. + +| # | Category | Result | +|---|---|---| +| 1 | Correctness & logic bugs | _pending_ | +| 2 | OtOpcUa conventions | _pending_ | +| 3 | Concurrency & thread safety | _pending_ | +| 4 | Error handling & resilience | _pending_ | +| 5 | Security | _pending_ | +| 6 | Performance & resource management | _pending_ | +| 7 | Design-document adherence | _pending_ | +| 8 | Code organization & conventions | _pending_ | +| 9 | Testing coverage | _pending_ | +| 10 | Documentation & comments | _pending_ | + +## Findings + + + +### -001 + +| Field | Value | +|---|---| +| Severity | Critical / High / Medium / Low | +| Category | one of the 10 checklist categories | +| Location | `path/to/File.cs:NN` | +| Status | Open / In Progress / Resolved / Won't Fix / Deferred | + +**Description:** What is wrong and why it matters. + +**Recommendation:** Concrete suggested fix. + +**Resolution:** _(empty until closed; on close, record the fixing commit SHA, the date, and a one-line description of the fix)_ diff --git a/code-reviews/prompt.md b/code-reviews/prompt.md new file mode 100644 index 0000000..0c1fca7 --- /dev/null +++ b/code-reviews/prompt.md @@ -0,0 +1,78 @@ +# Prompt — resolve open code-review findings + +Reusable orchestration prompt for clearing the `code-reviews/` backlog. Paste it +to a fresh agent when you want the remaining findings worked through. + +--- + +Resolve all open code-review findings (every severity), following the workflow +in `REVIEW-PROCESS.md`. + +## Setup + +- Read `code-reviews/README.md` for the open findings and `REVIEW-PROCESS.md` + for the workflow. Group the open findings by module. +- A module is one folder under `code-reviews/` — one `src/` project or one + `tests/` project, named with the `ZB.MOM.WW.OtOpcUa.` prefix stripped. The + module→project mapping is in `REVIEW-PROCESS.md` section 1; the build/test + commands are in `CLAUDE.md` ("Build Commands"). + +## Dispatch — one general-purpose subagent per module, in batches of ~5 modules + +Each subagent, for every open finding in its assigned module, must: + +- Verify the finding's root cause against the actual source. Do NOT trust the + finding text — if it is wrong or misclassified, re-triage it (correct the + severity/description in that module's `findings.md`) instead of forcing a fix. +- Use real TDD: write the regression test FIRST and run it to confirm it fails, + THEN implement the root-cause fix, THEN confirm it passes. (Do not use + `git stash` — parallel agents would race on the shared stash stack.) +- The regression test belongs in the reviewed project's own test project — a + finding in `src/.../ZB.MOM.WW.OtOpcUa.Driver.Galaxy` gets its test in + `tests/.../ZB.MOM.WW.OtOpcUa.Driver.Galaxy.Tests`. +- Run that module's build and test suite and confirm it is green: + - Build + unit-test the affected project, e.g. + `dotnet build src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy/...` and + `dotnet test tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy.Tests/...`. + - A single test: `dotnet test --filter "FullyQualifiedName~MyClass.MyMethod"`. + - `*.IntegrationTests` need their Docker fixture up — bring it up with + `lmxopcua-fix up ` (see `CLAUDE.md` "Docker Workflow"). + DB-backed `*.Configuration.Tests`, `*.Admin.Tests`, and `*.Server.Tests` + need the central SQL Server. If a fixture/service is unavailable, document + why the suite was skipped rather than reporting it green. + - For a change that crosses project boundaries, build each affected project; + a whole-solution check is `dotnet build ZB.MOM.WW.OtOpcUa.slnx`. +- Update only that module's `code-reviews//findings.md`: set each + resolved finding's Status to `Resolved` with a Resolution note describing the + fix (the orchestrator appends the fixing commit SHA), and update the header + "Open findings" count. +- CONSTRAINTS: edit only the source and test files needed for the assigned + module's findings, plus that module's own `findings.md`. Do NOT edit + `code-reviews/README.md`. Do NOT commit. Do NOT touch another module's + `findings.md`. +- Report a summary: each finding — root-cause confirmation, the fix, test names, + and any re-triage. + +Batch so that no two subagents in the same batch write to the same project. In +particular do not review a `src/` project and its matching `*.Tests` project in +the same batch — a finding in the source project adds its regression test to +that test project. + +## After each batch returns (orchestrator does this — keep your own context lean) + +- Build and test every project the batch touched, using the `CLAUDE.md` + commands; confirm clean. For a wide change, `dotnet build ZB.MOM.WW.OtOpcUa.slnx`. +- Commit per module — one commit per module, message referencing the finding + IDs. Record the fixing commit SHA in each finding's Resolution. +- Regenerate the index: `python code-reviews/regen-readme.py`, then + `python code-reviews/regen-readme.py --check` to confirm it is consistent; + stage `code-reviews/README.md`. (Use `python` — the bare `python3` alias on + this box resolves to the Windows Store stub and fails.) You may stage + `README.md` with each module's commit, or commit it once per batch after the + script runs. +- Push. + +## Continue + +Continue batch by batch until all findings are Resolved or re-triaged. If a +finding needs a design decision, skip it and surface it rather than guessing. diff --git a/code-reviews/regen-readme.py b/code-reviews/regen-readme.py new file mode 100644 index 0000000..c409f71 --- /dev/null +++ b/code-reviews/regen-readme.py @@ -0,0 +1,241 @@ +#!/usr/bin/env python3 +"""Regenerate code-reviews/README.md from the per-module findings.md files. + +The per-module findings.md files are the source of truth. This script aggregates +them into the single cross-module README.md (module status + pending/closed +finding tables). + +Usage: + python code-reviews/regen-readme.py # rewrite README.md + python code-reviews/regen-readme.py --check # exit 1 if stale or inconsistent + +`--check` fails when README.md is out of date OR when a module's header +`Open findings` count disagrees with its finding statuses, or a finding +carries an unrecognised Status value. +""" +from __future__ import annotations + +import re +import sys +from pathlib import Path + +ROOT = Path(__file__).resolve().parent +README = ROOT / "README.md" + +PENDING_STATUSES = {"Open", "In Progress"} +KNOWN_STATUSES = {"Open", "In Progress", "Resolved", "Won't Fix", "Deferred"} +SEVERITY_ORDER = {"Critical": 0, "High": 1, "Medium": 2, "Low": 3} + +GENERATED_NOTE = ( + "" +) + + +def cell(value: str) -> str: + """Escape a value for safe inclusion in a markdown table cell.""" + return value.replace("|", "\\|").strip() + + +def summarize(value: str, limit: int = 240) -> str: + """Trim a long description to a single-cell-friendly summary.""" + value = value.strip() + if len(value) <= limit: + return value + return value[: limit - 1].rstrip() + "…" + + +def first_table(text: str) -> dict[str, str]: + """Parse the first contiguous block of '| key | value |' rows into a dict.""" + rows: dict[str, str] = {} + started = False + for line in text.splitlines(): + stripped = line.strip() + if stripped.startswith("|"): + started = True + cells = [c.strip() for c in stripped.strip("|").split("|")] + if len(cells) >= 2: + key, value = cells[0], cells[1] + if key and not set(key) <= {"-", ":"} and key != "Field": + rows[key] = value + elif started: + break + return rows + + +def parse_module(findings_path: Path) -> dict: + """Parse one module's findings.md into its header and finding list.""" + text = findings_path.read_text(encoding="utf-8") + module = findings_path.parent.name + parts = re.split(r"^##\s+Findings\s*$", text, maxsplit=1, flags=re.M) + header = first_table(parts[0]) + findings: list[dict] = [] + if len(parts) > 1: + for chunk in re.split(r"^###\s+", parts[1], flags=re.M)[1:]: + fid = chunk.splitlines()[0].strip() + tbl = first_table(chunk) + desc_m = re.search( + r"\*\*Description:\*\*\s*(.*?)(?=\n\*\*|\Z)", chunk, re.S + ) + desc = re.sub(r"\s+", " ", desc_m.group(1)).strip() if desc_m else "" + findings.append( + { + "id": fid, + "severity": tbl.get("Severity", ""), + "category": tbl.get("Category", ""), + "location": tbl.get("Location", ""), + "status": tbl.get("Status", ""), + "description": desc, + } + ) + return {"module": module, "header": header, "findings": findings} + + +def build_readme(modules: list[dict]) -> str: + modules = sorted(modules, key=lambda m: m["module"]) + all_findings = [ + dict(f, module=m["module"]) for m in modules for f in m["findings"] + ] + pending = [f for f in all_findings if f["status"] in PENDING_STATUSES] + closed = [ + f + for f in all_findings + if f["status"] and f["status"] not in PENDING_STATUSES + ] + + def sev_key(f: dict) -> tuple: + return (SEVERITY_ORDER.get(f["severity"], 9), f["id"]) + + pending.sort(key=sev_key) + closed.sort(key=sev_key) + + out: list[str] = [ + "# Code Reviews", + "", + GENERATED_NOTE, + "", + "Cross-module code review index for the OtOpcUa server codebase " + "(`lmxopcua`). The review process is defined in " + "[../REVIEW-PROCESS.md](../REVIEW-PROCESS.md).", + "", + "Each module's `findings.md` is the source of truth; this file is generated " + "from them by `regen-readme.py` and must not be edited by hand.", + "", + "## Module status", + "", + "| Module | Reviewer | Date | Commit | Status | Open | Total |", + "|---|---|---|---|---|---|---|", + ] + if not modules: + out.append( + "| _no modules reviewed yet_ | | | | | | |" + ) + for m in modules: + h = m["header"] + open_n = sum( + 1 for f in m["findings"] if f["status"] in PENDING_STATUSES + ) + out.append( + f"| [{m['module']}]({m['module']}/findings.md) " + f"| {cell(h.get('Reviewer', ''))} " + f"| {cell(h.get('Review date', ''))} " + f"| {cell(h.get('Commit reviewed', ''))} " + f"| {cell(h.get('Status', ''))} " + f"| {open_n} | {len(m['findings'])} |" + ) + + out += ["", "## Pending findings", ""] + out.append( + "Findings with status `Open` or `In Progress`, ordered by severity." + ) + out.append("") + if pending: + out.append("| ID | Severity | Category | Location | Description |") + out.append("|---|---|---|---|---|") + for f in pending: + out.append( + f"| {cell(f['id'])} | {cell(f['severity'])} " + f"| {cell(f['category'])} | {cell(f['location'])} " + f"| {cell(summarize(f['description']))} |" + ) + else: + out.append("_No pending findings._") + + out += ["", "## Closed findings", ""] + out.append("Findings with status `Resolved`, `Won't Fix`, or `Deferred`.") + out.append("") + if closed: + out.append("| ID | Severity | Status | Category | Location |") + out.append("|---|---|---|---|---|") + for f in closed: + out.append( + f"| {cell(f['id'])} | {cell(f['severity'])} " + f"| {cell(f['status'])} | {cell(f['category'])} " + f"| {cell(f['location'])} |" + ) + else: + out.append("_No closed findings._") + + return "\n".join(out) + "\n" + + +def find_inconsistencies(modules: list[dict]) -> list[str]: + """Return human-readable problems in the per-module findings.md files. + + Checks that each module header's `Open findings` count agrees with its + finding statuses, and that every finding carries a known Status value. + """ + issues: list[str] = [] + for m in modules: + open_n = sum( + 1 for f in m["findings"] if f["status"] in PENDING_STATUSES + ) + declared = m["header"].get("Open findings", "").strip() + if declared != str(open_n): + issues.append( + f"{m['module']}: header 'Open findings' = '{declared}' but " + f"{open_n} finding(s) are Open/In Progress" + ) + for f in m["findings"]: + if f["status"] not in KNOWN_STATUSES: + issues.append( + f"{m['module']}: finding {f['id']} has unrecognised " + f"Status '{f['status']}'" + ) + return issues + + +def main(argv: list[str]) -> int: + check = "--check" in argv[1:] + module_dirs = sorted( + d + for d in ROOT.iterdir() + if d.is_dir() and d.name != "_template" and (d / "findings.md").is_file() + ) + modules = [parse_module(d / "findings.md") for d in module_dirs] + content = build_readme(modules) + issues = find_inconsistencies(modules) + if check: + stale = ( + README.read_text(encoding="utf-8") if README.exists() else "" + ) != content + for issue in issues: + print(f"inconsistent: {issue}", file=sys.stderr) + if stale: + print( + "code-reviews/README.md is stale - run regen-readme.py", + file=sys.stderr, + ) + if stale or issues: + return 1 + print("code-reviews/README.md is up to date and consistent.") + return 0 + for issue in issues: + print(f"warning: {issue}", file=sys.stderr) + README.write_text(content, encoding="utf-8", newline="\n") + print(f"Wrote {README} ({len(modules)} modules).") + return 0 + + +if __name__ == "__main__": + raise SystemExit(main(sys.argv)) diff --git a/code-reviews/test_regen_readme.py b/code-reviews/test_regen_readme.py new file mode 100644 index 0000000..341c3ff --- /dev/null +++ b/code-reviews/test_regen_readme.py @@ -0,0 +1,165 @@ +#!/usr/bin/env python3 +"""Tests for regen-readme.py. + +Dependency-free: run with `python code-reviews/test_regen_readme.py`. +Exits 0 if all tests pass, 1 otherwise. +""" +from __future__ import annotations + +import importlib.util +import tempfile +import traceback +from pathlib import Path + +HERE = Path(__file__).resolve().parent + +# regen-readme.py is not an importable module name (hyphen), so load it by path. +_spec = importlib.util.spec_from_file_location("regen_readme", HERE / "regen-readme.py") +regen = importlib.util.module_from_spec(_spec) +_spec.loader.exec_module(regen) + +FIXTURE = """# Code Review — Demo + +| Field | Value | +|---|---| +| Module | `src/Demo` | +| Reviewer | Tester | +| Review date | 2026-05-18 | +| Commit reviewed | `abc1234` | +| Status | Reviewed | +| Open findings | 1 | + +## Findings + +### Demo-001 + +| Field | Value | +|---|---| +| Severity | High | +| Category | Security | +| Location | `src/Demo/File.cs:10` | +| Status | Open | + +**Description:** A first problem that matters. + +**Recommendation:** Fix it. + +**Resolution:** _(open)_ + +### Demo-002 + +| Field | Value | +|---|---| +| Severity | Low | +| Category | Documentation & comments | +| Location | `src/Demo/File.cs:20` | +| Status | Resolved | + +**Description:** A second, minor problem. + +**Recommendation:** Tidy it. + +**Resolution:** Fixed in def5678 on 2026-05-18. +""" + + +def _parse_fixture() -> dict: + """Write FIXTURE to a temp Demo/findings.md and parse it.""" + with tempfile.TemporaryDirectory() as tmp: + path = Path(tmp) / "Demo" / "findings.md" + path.parent.mkdir() + path.write_text(FIXTURE, encoding="utf-8") + return regen.parse_module(path) + + +def test_first_table_skips_separator_and_field_header(): + table = regen.first_table("| Field | Value |\n|---|---|\n| Severity | High |\n") + assert table == {"Severity": "High"}, table + + +def test_parse_module_header(): + m = _parse_fixture() + assert m["module"] == "Demo", m["module"] + assert m["header"]["Reviewer"] == "Tester" + assert m["header"]["Status"] == "Reviewed" + assert m["header"]["Open findings"] == "1" + + +def test_parse_module_findings(): + m = _parse_fixture() + assert len(m["findings"]) == 2, len(m["findings"]) + first = m["findings"][0] + assert first["id"] == "Demo-001" + assert first["severity"] == "High" + assert first["category"] == "Security" + assert first["location"] == "`src/Demo/File.cs:10`" + assert first["status"] == "Open" + assert first["description"] == "A first problem that matters." + assert m["findings"][1]["status"] == "Resolved" + + +def test_build_readme_splits_pending_and_closed(): + readme = regen.build_readme([_parse_fixture()]) + assert "## Pending findings" in readme + assert "## Closed findings" in readme + pending, closed = readme.split("## Closed findings", 1) + assert "Demo-001" in pending # Open -> pending + assert "Demo-001" not in closed + assert "Demo-002" in closed # Resolved -> closed + assert "_No pending findings._" not in pending + + +def test_build_readme_handles_no_modules(): + readme = regen.build_readme([]) + assert "no modules reviewed yet" in readme + assert "_No pending findings._" in readme + assert "_No closed findings._" in readme + + +def test_find_inconsistencies_clean_fixture(): + assert regen.find_inconsistencies([_parse_fixture()]) == [] + + +def test_find_inconsistencies_detects_wrong_open_count(): + m = _parse_fixture() + m["header"]["Open findings"] = "7" + issues = regen.find_inconsistencies([m]) + assert len(issues) == 1 and "Open findings" in issues[0], issues + + +def test_find_inconsistencies_detects_unknown_status(): + m = _parse_fixture() + m["findings"][0]["status"] = "Bogus" + issues = regen.find_inconsistencies([m]) + # Wrong status also shifts the open count, so expect the status issue present. + assert any("unrecognised Status" in i for i in issues), issues + + +def test_summarize_truncates_long_text(): + long = "x" * 500 + out = regen.summarize(long) + assert len(out) <= 240 and out.endswith("…"), len(out) + assert regen.summarize("short") == "short" + + +def main() -> int: + tests = sorted( + (name, fn) + for name, fn in globals().items() + if name.startswith("test_") and callable(fn) + ) + failed = 0 + for name, fn in tests: + try: + fn() + print(f"PASS {name}") + except Exception: # noqa: BLE001 - test runner reports all failures + failed += 1 + print(f"FAIL {name}") + traceback.print_exc() + print(f"\n{len(tests) - failed}/{len(tests)} passed.") + return 1 if failed else 0 + + +if __name__ == "__main__": + raise SystemExit(main()) diff --git a/scripts/check-code-reviews-readme.ps1 b/scripts/check-code-reviews-readme.ps1 new file mode 100644 index 0000000..78f5a65 --- /dev/null +++ b/scripts/check-code-reviews-readme.ps1 @@ -0,0 +1,20 @@ +# Verifies code-reviews/README.md is regenerated from, and consistent with, the +# per-module findings.md files. Intended as a CI / pre-commit gate. +# +# Exits non-zero when README.md is stale, when a module header's "Open findings" +# count disagrees with its finding statuses, or when a finding carries an +# unrecognised Status value. See REVIEW-PROCESS.md section 5. + +[CmdletBinding()] +param() + +Set-StrictMode -Version Latest +$ErrorActionPreference = "Stop" + +$repoRoot = Resolve-Path (Join-Path $PSScriptRoot "..") +$script = Join-Path $repoRoot "code-reviews/regen-readme.py" + +# The bare `python3` alias on this platform resolves to the Windows Store stub; +# `python` is the real interpreter. +& python $script --check +exit $LASTEXITCODE