|
|
|
@@ -4,8 +4,8 @@
|
|
|
|
|
|---|---|
|
|
|
|
|
| Module | `src/ZB.MOM.WW.MxGateway.Contracts` |
|
|
|
|
|
| Reviewer | Claude Code |
|
|
|
|
|
| Review date | 2026-05-24 |
|
|
|
|
|
| Commit reviewed | `42b0037` |
|
|
|
|
|
| Review date | 2026-06-15 |
|
|
|
|
|
| Commit reviewed | `410acc9` |
|
|
|
|
|
| Status | Re-reviewed |
|
|
|
|
|
| Open findings | 0 |
|
|
|
|
|
|
|
|
|
@@ -50,6 +50,43 @@ Python and Go descriptors. No fields renumbered or repurposed.
|
|
|
|
|
| 9 | Testing coverage | No issues found — `ProtobufContractRoundTripTests` and `GatewayContractInfoTests` continue to pin the protocol version; new `QueryActiveAlarmsRequest` lacks a round-trip test but the RPC type is generated and exercised end-to-end by the gRPC client tests in each language. |
|
|
|
|
|
| 10 | Documentation & comments | Issues found: Contracts-017 (the `rpc QueryActiveAlarms` comment block does not mention the `alarm_filter_prefix` request field). |
|
|
|
|
|
|
|
|
|
|
#### 2026-06-15 re-review (commit 410acc9)
|
|
|
|
|
|
|
|
|
|
Re-review pass at `410acc9` scoped to the contract changes since `42b0037`
|
|
|
|
|
(`git diff 42b0037..HEAD -- src/ZB.MOM.WW.MxGateway.Contracts/`). The window
|
|
|
|
|
contains two unrelated additive contract features. The brief targets the
|
|
|
|
|
**alarm-provider fallback** surface in `mxaccess_gateway.proto`: the new
|
|
|
|
|
`AlarmProviderMode` enum (`UNSPECIFIED=0`/`ALARMMGR=1`/`SUBTAG=2`), the
|
|
|
|
|
`AlarmSubtagTarget` watch-list message, `AlarmFailoverConfig`, the three new
|
|
|
|
|
`SubscribeAlarmsCommand` fields (`forced_mode=2`, `watch_list=3`, `failover=4`),
|
|
|
|
|
the `OnAlarmProviderModeChangedEvent` (`MxEvent.body` oneof tag 25,
|
|
|
|
|
`MxEventFamily=6`), the `degraded=14`/`source_provider=15` provenance fields on
|
|
|
|
|
`OnAlarmTransitionEvent` **and** `ActiveAlarmSnapshot`, and the
|
|
|
|
|
`AlarmFeedMessage.provider_status=4` oneof case carrying `AlarmProviderStatus`.
|
|
|
|
|
The same window also adds the Galaxy `BrowseChildren` lazy-browse RPC
|
|
|
|
|
(`galaxy_repository.proto`) and three XML doc comments on `GatewayContractInfo`
|
|
|
|
|
constants — both outside the brief's alarm focus but checked for additive-only
|
|
|
|
|
hygiene (clean). `Generated/*.cs` is build output and was not reviewed as
|
|
|
|
|
hand-written. `mxaccess_worker.proto` is unchanged (the alarm additions live in
|
|
|
|
|
the gateway proto the worker imports — matches the design doc's Superseded note).
|
|
|
|
|
|
|
|
|
|
Verified against `docs/plans/2026-06-13-alarm-subtag-fallback-design.md`,
|
|
|
|
|
`docs/plans/2026-06-15-forced-subtag-mode-fix.md`, and the worker/gateway source
|
|
|
|
|
(`AlarmDispatcher.cs:213`, `MxAccessEventMapper.cs:151`, `GatewayAlarmMonitor.cs`).
|
|
|
|
|
|
|
|
|
|
| # | Category | Result |
|
|
|
|
|
|---|---|---|
|
|
|
|
|
| 1 | Correctness & logic bugs | No issues found. Field semantics are correct against source: `AlarmProviderStatus.degraded`/`OnAlarmTransitionEvent.degraded` track `mode == SUBTAG` (worker `AlarmDispatcher.cs:213` sets `SourceProvider = Degraded ? Subtag : Alarmmgr`; gateway `GatewayAlarmMonitor._providerDegraded = toMode == Subtag`). `OnAlarmProviderModeChangedEvent.hresult` "0 on failback" matches the Auto-mode failover/failback path that emits it; forced mode is seeded gateway-side and emits no worker event, so the comment is not contradicted. |
|
|
|
|
|
| 2 | mxaccessgw conventions | No issues found. The subtag fallback synthesizes events **inside the worker** and marks every synthesized transition `degraded`, satisfying the CLAUDE.md "gateway forwards only worker-emitted events; synthesizing is an explicit opt-in non-parity mode" rule. `snake_case` fields, `PascalCase` messages, the `ALARM_PROVIDER_MODE_`/`MX_EVENT_FAMILY_` enum-prefix discipline, and the top-of-file wire-compatibility policy block (Contracts-005) are all honoured. Generated code regenerated, not hand-edited. |
|
|
|
|
|
| 3 | Concurrency & thread safety | N/A — pure contract definitions plus a static constants class. |
|
|
|
|
|
| 4 | Error handling & resilience | No issues found. The degraded/provider-status surface lets clients distinguish the lower-fidelity subtag feed from the authoritative alarmmgr feed; `AlarmProviderStatus` is emitted on stream open and every switch so late joiners learn the mode. |
|
|
|
|
|
| 5 | Security | No issues found — none of the new fields carry credentials or secrets. `AlarmSubtagTarget` carries only item-address strings. |
|
|
|
|
|
| 6 | Performance & resource management | No issues found. `repeated AlarmSubtagTarget watch_list` is sent once at subscribe time, not per-event; provenance fields are scalars. No hot-path bloat. |
|
|
|
|
|
| 7 | Design-document adherence | No drift. The shipped contract matches `docs/plans/2026-06-13-alarm-subtag-fallback-design.md` (including its Superseded notes: additions in the gateway proto, not the worker proto). |
|
|
|
|
|
| 8 | Code organization & conventions | No issues found. Every addition uses a new, contiguous field number — `SubscribeAlarmsCommand` 2-4, `MxEvent.body` 25, `MxEventFamily` 6, `OnAlarmTransitionEvent`/`ActiveAlarmSnapshot` 14-15, `AlarmFeedMessage.payload` 4 — with no reuse, renumbering, or type narrowing of any existing field. Enum zero-values are `UNSPECIFIED`. Additive-only invariant preserved. |
|
|
|
|
|
| 9 | Testing coverage | Issues found: Contracts-018 — `ProtobufContractRoundTripTests` covers the new `AlarmProviderStatus` (via `AlarmFeedMessage`) and the `OnAlarmTransitionEvent` `degraded`/`source_provider` fields, but has no round-trip coverage for the `ActiveAlarmSnapshot` provenance fields, the `SubscribeAlarmsCommand` extensions (`forced_mode`/`watch_list`/`failover`), or `OnAlarmProviderModeChangedEvent`. |
|
|
|
|
|
| 10 | Documentation & comments | Issues found: Contracts-019 — the `ActiveAlarmSnapshot.degraded`/`source_provider` fields carry no in-proto comment while the byte-identical fields on `OnAlarmTransitionEvent` are documented; and the `AlarmProviderMode` enum doc explains `UNSPECIFIED` only for the `forced_mode` use, not for the provenance (`source_provider`) reuse. |
|
|
|
|
|
|
|
|
|
|
## Findings
|
|
|
|
|
|
|
|
|
|
### Contracts-001
|
|
|
|
@@ -341,3 +378,33 @@ additive-only with no reuse, renumbering, or type narrowing.
|
|
|
|
|
Re-review: no new findings. Open finding count remains 0. All seventeen
|
|
|
|
|
recorded Contracts findings (Contracts-001..017) remain closed
|
|
|
|
|
(Resolved / Won't Fix).
|
|
|
|
|
|
|
|
|
|
### Contracts-018
|
|
|
|
|
|
|
|
|
|
| Field | Value |
|
|
|
|
|
|---|---|
|
|
|
|
|
| Severity | Low |
|
|
|
|
|
| Category | Testing coverage |
|
|
|
|
|
| Location | `src/ZB.MOM.WW.MxGateway.Tests/Contracts/ProtobufContractRoundTripTests.cs:396` (`ActiveAlarmSnapshot_RoundTripsAllFields`) |
|
|
|
|
|
| Status | Resolved |
|
|
|
|
|
|
|
|
|
|
**Description:** The alarm-provider fallback feature added several new wire fields to `mxaccess_gateway.proto`. `ProtobufContractRoundTripTests` was extended with `AlarmFeedMessage_RoundTripsProviderStatus` (covers `AlarmProviderStatus` + the `provider_status` oneof case) and `Transition_RoundTripsDegradedProvenance` (covers `OnAlarmTransitionEvent.degraded`/`source_provider`), but three pieces of the new contract surface have no round-trip coverage: (a) `ActiveAlarmSnapshot.degraded` (14) / `source_provider` (15) — `ActiveAlarmSnapshot_RoundTripsAllFields` stops at `OperatorComment` (field 11) and never sets or asserts the two new provenance fields, so a future renumber/type change to them would not be caught; (b) the `SubscribeAlarmsCommand` extensions `forced_mode` (2), `watch_list` (3, `repeated AlarmSubtagTarget`), and `failover` (4, `AlarmFailoverConfig`) — no test exercises these, and the live `forced_mode` enum-drop concern that prompted the `2026-06-15-forced-subtag-mode-fix` investigation is exactly the kind of wire shape prior contract tests have been written to pin; (c) `OnAlarmProviderModeChangedEvent` (the `MxEvent.body` oneof tag 25 / `MxEventFamily=6` worker→gateway event). This is the same class of gap previously flagged for the bulk family (Contracts-007 / Contracts-010): new wire shapes shipped without round-trip pinning.
|
|
|
|
|
|
|
|
|
|
**Recommendation:** Extend `ActiveAlarmSnapshot_RoundTripsAllFields` (or add a focused test) to set and assert `degraded = true` + `source_provider = AlarmProviderMode.Subtag`; add a round-trip test for `SubscribeAlarmsCommand` populating `forced_mode`, a `watch_list` entry (all six `AlarmSubtagTarget` string fields), and a `failover` `AlarmFailoverConfig`; and add a round-trip / `MxEvent` oneof-case test for `OnAlarmProviderModeChangedEvent` pinning `MxEvent.BodyCase == OnAlarmProviderModeChanged` for `MxEventFamily.OnAlarmProviderModeChanged`.
|
|
|
|
|
|
|
|
|
|
**Resolution:** _(2026-06-15)_ Verified the three coverage gaps against the proto — `ActiveAlarmSnapshot.degraded`/`source_provider` (14/15), `SubscribeAlarmsCommand.forced_mode`/`watch_list`/`failover` (2/3/4), and the `MxEvent.body` oneof tag 25 / `MxEventFamily=6` `OnAlarmProviderModeChangedEvent` were all unpinned. Added three focused round-trip tests to `ProtobufContractRoundTripTests`: `ActiveAlarmSnapshot_RoundTripsDegradedProvenance` (sets/asserts `degraded = true` + `source_provider = AlarmProviderMode.Subtag`), `SubscribeAlarmsCommand_RoundTripsForcedModeWatchListAndFailover` (populates `forced_mode`, a `watch_list` entry with all six `AlarmSubtagTarget` string fields, and a `failover` `AlarmFailoverConfig`), and `MxEvent_RoundTripsOnAlarmProviderModeChangedBody` (pins `MxEvent.BodyCase == OnAlarmProviderModeChanged` + `Family == OnAlarmProviderModeChanged`). All fields round-trip — no contract bug found. The full `ProtobufContractRoundTrip` filter is 49/49 green.
|
|
|
|
|
|
|
|
|
|
### Contracts-019
|
|
|
|
|
|
|
|
|
|
| Field | Value |
|
|
|
|
|
|---|---|
|
|
|
|
|
| Severity | Low |
|
|
|
|
|
| Category | Documentation & comments |
|
|
|
|
|
| Location | `src/ZB.MOM.WW.MxGateway.Contracts/Protos/mxaccess_gateway.proto:850-851` (`ActiveAlarmSnapshot`), `:318-324` (`AlarmProviderMode`) |
|
|
|
|
|
| Status | Resolved |
|
|
|
|
|
|
|
|
|
|
**Description:** Two in-proto documentation gaps on the new alarm-provider surface. (1) `OnAlarmTransitionEvent.degraded` (line 805-808) and `source_provider` (809-810) carry clear comments ("True when this transition came from the subtag-monitoring fallback … synthesized from data changes, reduced fidelity"; "Which provider produced this transition."), but the byte-identical `ActiveAlarmSnapshot.degraded` (850) and `source_provider` (851) are declared bare with no comment. The two messages model the same provenance concept and a reader of `ActiveAlarmSnapshot` alone gets no signal that a non-`UNSPECIFIED` `source_provider` plus `degraded = true` means the snapshot came from the lower-fidelity subtag source. (2) The `AlarmProviderMode` enum comment (318-319) documents the zero value only for one use site — "UNSPECIFIED on a SubscribeAlarmsCommand means auto: alarmmgr primary with subtag fallback" — but the same enum is reused as a provenance field on `OnAlarmTransitionEvent.source_provider`, `ActiveAlarmSnapshot.source_provider`, `OnAlarmProviderModeChangedEvent.mode`, and `AlarmProviderStatus.mode`. The worker always sets `source_provider` to `ALARMMGR` or `SUBTAG` (never `UNSPECIFIED`; `MxAccessEventMapper.cs:151` defaults to `Alarmmgr`, `AlarmDispatcher.cs:213` picks `Subtag`/`Alarmmgr`), so `UNSPECIFIED` as a provenance value has no defined meaning and the comment does not say so. The ProtobufStyleGuide rule "comment fields carrying MXAccess parity / non-obvious semantics" applies — this is a non-parity provenance marker.
|
|
|
|
|
|
|
|
|
|
**Recommendation:** (1) Add comments to `ActiveAlarmSnapshot.degraded` / `source_provider` mirroring the wording already on `OnAlarmTransitionEvent` (or a one-line cross-reference). (2) Extend the `AlarmProviderMode` enum comment to note that as a `source_provider` / `mode` provenance value the field is always `ALARMMGR` or `SUBTAG` on the wire and `UNSPECIFIED` should be treated as "unknown / not yet determined", so the zero value is unambiguous at every use site. Comment-only changes; no wire-format impact.
|
|
|
|
|
|
|
|
|
|
**Resolution:** _(2026-06-15)_ Confirmed both gaps in `mxaccess_gateway.proto`: `ActiveAlarmSnapshot.degraded`/`source_provider` (14/15) were bare while the byte-identical `OnAlarmTransitionEvent` fields were documented, and the `AlarmProviderMode` enum comment only explained `UNSPECIFIED` for the `forced_mode` use. (1) Added comments to `ActiveAlarmSnapshot.degraded`/`source_provider` mirroring the `OnAlarmTransitionEvent` wording (subtag-fallback / reduced-fidelity, always ALARMMGR or SUBTAG, never UNSPECIFIED). (2) Extended the `AlarmProviderMode` enum comment to distinguish its two use sites: as `forced_mode`, `UNSPECIFIED` = auto; as a provenance value (`OnAlarmTransitionEvent.source_provider`, `ActiveAlarmSnapshot.source_provider`, `OnAlarmProviderModeChangedEvent.mode`, `AlarmProviderStatus.mode`) the worker always emits ALARMMGR/SUBTAG and `UNSPECIFIED` should be read as "unknown / not yet determined". Comment-only changes; no wire-format impact. NOTE: on this dev box the `csharp` protoc generator DOES emit proto leading comments into `Generated/MxaccessGateway.cs` `<summary>` XML doc (contrary to the brief's assumption), so the build regenerated `Generated/MxaccessGateway.cs` with the new doc comments only — diff is `///`-comment lines exclusively, zero code/wire/type changes. `dotnet build -f net10.0` succeeds with 0 warnings / 0 errors.
|
|
|
|
|
Joseph Doherty