From 2f8404d2efde0c192a24102d3773174aeb39722f Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Sun, 24 May 2026 08:28:25 -0400 Subject: [PATCH] code-reviews: re-review Contracts at 42b0037 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit No new findings — the only Contracts commit in window (bd1d1f1) is a comment-only proto edit; field numbering remains additive with no reuse, renumbering, or type narrowing. Co-Authored-By: Claude Opus 4.7 (1M context) --- code-reviews/Contracts/findings.md | 39 ++++++++++++++++++++++++++++-- 1 file changed, 37 insertions(+), 2 deletions(-) diff --git a/code-reviews/Contracts/findings.md b/code-reviews/Contracts/findings.md index 5353879..df10bc8 100644 --- a/code-reviews/Contracts/findings.md +++ b/code-reviews/Contracts/findings.md @@ -5,8 +5,8 @@ | Module | `src/ZB.MOM.WW.MxGateway.Contracts` | | Reviewer | Claude Code | | Review date | 2026-05-24 | -| Commit reviewed | `d692232` | -| Status | Reviewed | +| Commit reviewed | `42b0037` | +| Status | Re-reviewed | | Open findings | 0 | ## Checklist coverage @@ -306,3 +306,38 @@ Python and Go descriptors. No fields renumbered or repurposed. **Recommendation:** Extend the RPC comment with one line: "`QueryActiveAlarmsRequest.alarm_filter_prefix` optionally narrows the snapshot to alarms whose `alarm_full_reference` starts with the given prefix; an empty prefix returns the full set." Comment-only. **Resolution:** _(2026-05-24)_ Confirmed against `mxaccess_gateway.proto` at `d2d2e5f`: lines 29-34 of the `rpc QueryActiveAlarms` block describe the snapshot order and session-less origin but never mention `alarm_filter_prefix`, which is only documented on the request message itself two lines below — a client author reading the RPC comment alone cannot discover the filter capability. Extended the RPC comment with the finding's recommended one-line addition verbatim: "`QueryActiveAlarmsRequest.alarm_filter_prefix` optionally narrows the snapshot to alarms whose `alarm_full_reference` starts with the given prefix; an empty prefix returns the full set." Comment-only change; the RPC signature, request/reply types, and field numbers are unchanged. The same regression test added under Contracts-016 (`ProtobufContractRoundTripTests.QueryActiveAlarmsRequest_PinsFieldNumbersAndRoundTripsPrefixFilter`) also exercises a round-trip of `QueryActiveAlarmsRequest` with `alarm_filter_prefix` populated and pins `QueryActiveAlarms` on the public `MxAccessGateway` service surface, so the RPC + filter field combination the new comment documents is wire-stable. `dotnet build src/ZB.MOM.WW.MxGateway.slnx` is green (0 warnings, 0 errors). + +#### 2026-05-24 re-review (commit 42b0037) + +Re-review pass at `42b0037` scoped to the contract changes since `d692232`. +The window contains exactly one Contracts commit (`bd1d1f1`, "Resolve +Contracts-016..017"), which is a comment-only edit to +`mxaccess_gateway.proto`: the `rpc QueryActiveAlarms` block gains the +`alarm_filter_prefix` description sentence (Contracts-017 resolution) and +the `QueryActiveAlarmsRequest` header comment replaces the misleading +"reserved" wording with the unambiguous "Clients may leave `session_id` +empty; the gateway currently ignores it…" phrasing (Contracts-016 +resolution). The two `Generated/*.cs` edits in the same window are pure +regeneration of those comments into XML doc — no wire-format, +field-number, or type changes. The `StreamAlarms` / `AcknowledgeAlarm` +alarm surface mentioned in the re-review brief was last touched at +`397d3c5` (already covered in the `d692232` pass) and is unchanged in this +window; field numbering across all three protos remains +additive-only with no reuse, renumbering, or type narrowing. + +| # | Category | Result | +|---|---|---| +| 1 | Correctness & logic bugs | No issues found. The two comment changes are factually correct against the source (`MxAccessGatewayService.QueryActiveAlarms` does ignore `session_id` and does apply the `alarm_filter_prefix` `StartsWith` match). | +| 2 | mxaccessgw conventions | No issues found. The wire-compatibility policy comment block at the top of `mxaccess_gateway.proto` (Contracts-005 resolution) remains intact; the edits are additive comment-only changes consistent with the additive-only contract evolution rule. Generated code under `Generated/` regenerated cleanly from the source comments, not hand-edited. | +| 3 | Concurrency & thread safety | N/A — pure contract definitions and a static constants class. | +| 4 | Error handling & resilience | No issues found. | +| 5 | Security | No issues found — no new credential-bearing fields in the window. | +| 6 | Performance & resource management | No issues found. | +| 7 | Design-document adherence | No drift. `docs/AlarmClientDiscovery.md` already documents the `alarm_filter_prefix` filter and the session-less central-monitor cache; the new RPC-comment line restates what the doc already says, in line with the CLAUDE.md "Update docs in the same change as the source" rule (docs were already current at `d692232`, source caught up at `bd1d1f1`). | +| 8 | Code organization & conventions | No issues found. Field numbers and type names unchanged; additive-only invariant preserved. | +| 9 | Testing coverage | No issues found. `ProtobufContractRoundTripTests.QueryActiveAlarmsRequest_PinsFieldNumbersAndRoundTripsPrefixFilter` (added under Contracts-016) pins the three `QueryActiveAlarmsRequest` field numbers and round-trips an `alarm_filter_prefix`-bearing payload; comment-only proto changes do not need additional test coverage. | +| 10 | Documentation & comments | No issues found. The two edits are themselves documentation fixes that close prior Contracts-016 and Contracts-017 findings. | + +Re-review: no new findings. Open finding count remains 0. All seventeen +recorded Contracts findings (Contracts-001..017) remain closed +(Resolved / Won't Fix).