No source changes since d692232 — header bumped to track the latest
reviewed commit, all prior findings remain closed.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Append 5 new findings (Client.Java-032..036): README flags for new
alarm subcommands do not exist; StreamAlarmsCommand bounded queue
silently drops alarms instead of fail-fast; BatchCommand whitespace
tokenisation shreds quoted args; no library-side stream_alarms test;
MxGatewayAlarmFeedSubscription is the fourth duplicate subscription
class.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Append 5 new findings (Client.Python-022..026): README flags for new
alarm subcommands do not exist; Client.Python-013 regression — the
silent localhost auto-plaintext branch is still present (the prior
Resolution did not survive the rename); production batch path uses
the click.testing.CliRunner helper; no behavioural tests for new SDK
+ CLI; bench cleanup swallows exceptions.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Append 8 new findings (Client.Rust-022..029): MalformedReply path
absent from the new bulk SDK methods, hard-coded client_correlation_id
in new CLI commands, no tests for stream_alarms / bulk SDK / bench,
RustClientDesign.md silent on new surface, run_bench_read_bulk clones
tags inside the loop, .cargo/config.toml comment is wrong, run_batch
exits on blank line, and cargo clippy fails at HEAD with three
warnings the prior reviewer punted.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Append 6 new findings (Client.Go-022..027): regressions of Client.Go-015
(runWriteBulkVariant secured flag) and Client.Go-018 (bench loop
ignores ctx.Err()) reintroduced by the bulk port; no SDK tests for the
new WriteBulk/ReadBulk/StreamAlarms methods; bulk SDK accepts empty
slices; bufio.Scanner buffer + blank-line sentinel can abort the batch
session.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Append 4 new findings (Client.Dotnet-018..021): README flags for the
new stream-alarms/acknowledge-alarm subcommands cite options that do
not exist on the CLI; BenchReadBulkAsync reinstates the silent
register-handle fallback and swallows OperationCanceledException;
both new --timeout-ms consumers cast int32 to uint without bounds.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Append 1 new finding (IntegrationTests-025): the
ResolveRepositoryRoot_NoMarkers test walks up from Path.GetTempPath()
through unisolated ancestors; a redirected TMP or co-located checkout
at C:\src silently breaks the assertion.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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) <noreply@anthropic.com>
Append 5 new findings (Tests-027..031) covering the
StreamEvents_WhenEventIsWritten_RecordsSendDuration flake root cause
(shared MeterListener by meter name), missing kill-path coverage
(reason propagation + concurrent-kill double-count), asymmetric guard
coverage between Close and Kill, missing audit-failure-path coverage
for ApiKey Delete, and the DashboardSnapshotPublisher reconnect-window
timer sensitivity.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Append 7 new findings (Server-044..050) covering the destructive-action
wave: KillWorkerAsync metric/state leaks, ShutdownAsync kill-fallback
gauge leak, inconsistent ConfirmDialog cleanup across pages, missing
XML docs on the new DashboardSessionAdmin surface, and unhandled
RemoveSessionAsync exception paths.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Closes the 2026-05-24 resolution sweep at HEAD `83eba4b`. All 21 open
findings from the d692232 re-review are now Resolved:
Server 327e9c5 Server-031, -032, -038, -039, -040, -041, -042, -043
Contracts bd1d1f1 Contracts-016, -017
Tests d48099f Tests-025, -026
IntegrationTests 865c22a IntegrationTests-022, -023, -024
Client.Java 10bd0c0 Client.Java-027, -028, -029, -030, -031
Client.Rust 83eba4b Client.Rust-021
Also fixes stale "Open findings" header counts in Client.Java and
IntegrationTests findings.md that survived the resolve passes
(the agents updated each finding's Status but missed the header
sum). `regen-readme.py --check` is now green.
Module status: 11 / 11 reviewed, 0 / 276 total open.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Client.Rust-021 (Design adherence): RustClientDesign.md "Crate layout"
section now describes the actual flat workspace structure instead of
the aspirational nested form. The replacement text states that the
workspace root is clients/rust/, the top-level crate
zb-mom-ww-mxgateway-client is declared in clients/rust/Cargo.toml
directly, and crates/mxgw-cli/ is the sole [workspace.members] entry.
The accompanying tree lists the real files on disk (Cargo.toml,
Cargo.lock, build.rs, README.md, RustClientDesign.md,
src/{lib,client,session,galaxy,options,auth,error,value,version,generated}.rs
plus the src/generated/ tonic-build output dir, tests/, and
crates/mxgw-cli/).
Doc-only change. cargo build --workspace + cargo test --workspace clean.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Client.Java-027 (Documentation): Updated 17 Gradle task references in
clients/java/README.md (lines 37, 108-110, 160-161, 169-176, 186, 206,
221) and 3 in clients/java/JavaClientDesign.md from the retired short
subproject names to the canonical zb-mom-ww-mxgateway-client /
zb-mom-ww-mxgateway-cli names. Copy-pasting any documented command now
matches the subproject names declared in settings.gradle.
Client.Java-028 (Design adherence): Build-layout block in
JavaClientDesign.md lines 23-27 updated to show the actual package
paths com/zb/mom/ww/mxgateway/{client,cli}/ instead of the retired
com/dohertylan/mxgateway/{client,cli}/ paths.
Client.Java-029 (Documentation): README.md line 210 corrected from
"zb-mom-ww-mxgateway-cli/build/install/mxgateway-cli" to
"zb-mom-ww-mxgateway-cli/build/install/zb-mom-ww-mxgateway-cli" — Gradle
installDist produces a directory whose name matches the project name,
not the short suffix. The e2e script already used the correct path.
Client.Java-030 (Testing coverage): Added
queryActiveAlarmsForwardsRequestAndStreamsSnapshots to
MxGatewayClientSessionTests. The test pushes a QueryActiveAlarmsRequest
carrying session_id / client_correlation_id / alarm_filter_prefix
through an InProcessGateway + TestGatewayService and asserts the server
observed all three request fields, two ActiveAlarmSnapshots stream in
order, and onError is never called. TDD red→green confirmed via a
deliberately-wrong session_id assertion. The re-triage note in
Client.Java-030's resolution clarifies that the finding's reference to
"the existing acknowledgeAlarm test" was aspirational — the alarm RPC
surface had zero coverage before this commit.
Client.Java-031 (Conventions): README.md prose lines 17, 22, 26 updated
to use the canonical zb-mom-ww-mxgateway-client / zb-mom-ww-mxgateway-cli
names so the layout description matches Gradle / IDE project names.
Verification: gradle build BUILD SUCCESSFUL; all Java unit tests pass.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
IntegrationTests-022 (Conventions): ResolveRepositoryRoot now throws
InvalidOperationException when the walk exhausts without finding a root
marker, with a message naming the start directory, the expected markers
(src/, .git, *.sln, *.slnx), and the MXGATEWAY_LIVE_MXACCESS_WORKER_EXE
escape hatch. Replaces the silent fallback to
Directory.GetCurrentDirectory() that previously masked misconfiguration.
New regression test
ResolveRepositoryRoot_NoMarkers_ThrowsInvalidOperationExceptionNamingStartAndMarkers
in IntegrationTestEnvironmentTests asserts the throw and the message
contents. TDD red→green confirmed.
IntegrationTests-023 (Testing coverage): DashboardLdapLiveTests's
AuthenticateAsync_AdminInGwAdminGroup_Succeeds now asserts that the
authenticated principal carries a ClaimTypes.Role claim with value
DashboardRoles.Admin in addition to the existing LdapGroupClaimType
assertion. A regression in MapGroupsToRoles (returning an empty list or
missing the RDN fallback) would now surface here. Gated by
MXGATEWAY_RUN_LIVE_LDAP_TESTS.
IntegrationTests-024 (Conventions): Option (b) — extracted within
IntegrationTests. New file TestSupport/NullDashboardEventBroadcaster.cs
(public type, private ctor, singleton Instance). The inline class at
the bottom of WorkerLiveMxAccessSmokeTests is gone; the file now imports
the shared type. Matches the unit-test project's Tests-007 / Tests-021 /
Tests-025 pattern while keeping the two test projects independently
buildable (no shared test-helpers project crossing module boundaries).
Verification: dotnet build src/ZB.MOM.WW.MxGateway.IntegrationTests
clean; 19/19 integration tests passing (live MxAccess + LDAP + Galaxy).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Tests-025 (Conventions): Extracted the previously-duplicated
NullDashboardEventBroadcaster into TestSupport/NullDashboardEventBroadcaster.cs
(singleton Instance, private ctor). The two nested copies in
EventStreamServiceTests and GatewayEndToEndFakeWorkerSmokeTests were
removed; both files now use the shared type via
'using ZB.MOM.WW.MxGateway.Tests.TestSupport;'. The Server-041 regression
test's ThrowingDashboardEventBroadcaster is intentionally left nested —
single-file usage doesn't warrant promotion to TestSupport. The third
copy in IntegrationTests/WorkerLiveMxAccessSmokeTests was handled by
IntegrationTests-024 in its own commit.
Tests-026 (Testing coverage): Added a new RecordingDashboardEventBroadcaster
test double in TestSupport — a thread-safe (ConcurrentQueue<DashboardEventCapture>)
recorder. New fixture
StreamEventsAsync_PublishesEachEventToDashboardBroadcaster in
EventStreamServiceTests pushes two events through the fake session and
asserts the broadcaster received both with the correct sessionId and
WorkerSequence. TDD red→green confirmed: the deliberately-wrong
"Expected 3, Actual 2" red phase proved the recording fake was actually
invoked by the production code path.
Verification: 486/486 server tests passing (485 previous + 1 new).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contracts-016 (Conventions): QueryActiveAlarmsRequest.session_id header
replaced with the unambiguous "Clients may leave session_id empty; the
gateway currently ignores it and serves the session-less central-monitor
cache. A future version may use it to scope the snapshot to one
session." Removes the ambiguity that the prior "reserved for future
use" wording introduced.
Contracts-017 (Documentation): The rpc QueryActiveAlarms comment now
includes the alarm_filter_prefix description: "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."
Both are proto-comment-only changes — no wire-format impact, no field
renumbering, and the regenerated MxaccessGateway.cs / MxaccessGatewayGrpc.cs
carry only the doc-comment delta. Added the additive-only regression
guard QueryActiveAlarmsRequest_PinsFieldNumbersAndRoundTripsPrefixFilter
to ProtobufContractRoundTripTests — pins
session_id=1 / client_correlation_id=2 / alarm_filter_prefix=3 by
descriptor lookup and round-trips the message with and without the
filter populated.
Verification: dotnet build src/ZB.MOM.WW.MxGateway.slnx clean;
ProtobufContractRoundTripTests 40/40 passing.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Server-031: re-triaged. The recommended gateway-side
"skip-while-command-in-flight" guard is already in place at
WorkerClient.HeartbeatLoopAsync via WorkerClientOptions.HeartbeatStuckCeiling
(default 75s = 5× HeartbeatGrace). Two regression tests pin the
behaviour. Recommendation #1 (decouple worker-side _writeLock) is a
Worker-module concern (Worker-017 / Worker-023) and out of scope here.
Server-032: re-triaged. Recommendation #2 (rich diagnostic) is already
in EnqueueWorkerEventAsync, with #3 (overflow grace) absorbed by the
TryWrite → WriteAsync-with-timeout fall-through. Test
EnqueueWorkerEvent_WhenChannelFullPastTimeout_FaultsWithRichDiagnostic
pins the diagnostic string. Recommendation #1 (prose contract in
gateway.md / docs) is deferred — outside this pass's edit scope.
Server-038 (Security): EventsHub.SubscribeSession's missing per-session
ACL is documented with a TODO(per-session-acl) and a <remarks> block
explaining the v1 acceptance (any dashboard role can subscribe to any
session — non-secret metadata, redacted value logging). The per-session
ACL design lands in a follow-up once a session-scoped role exists.
Server-039 (Error handling): HubTokenService.Validate now rejects a
deserialized payload where both Name and NameIdentifier are null/empty.
New test file HubTokenServiceTests.cs covers the regression and five
sanity cases. TDD confirmed.
Server-040 (Conventions): MapGroupsToRoles gains a precedence comment
explaining "full literal match first, leading-RDN fallback;
OrdinalIgnoreCase via DashboardOptions.GroupToRole". Documentation-only.
Server-041 (Design adherence): EventStreamService.ProduceEventsAsync
wraps the broadcaster.Publish call in try/catch (Exception). The
producer loop and gRPC stream are no longer at the mercy of the
broadcaster's never-throw discipline. New regression test
StreamEventsAsync_WhenDashboardBroadcasterThrows_StillYieldsEventsAndDoesNotFaultSession.
Server-042 (Performance): DashboardSnapshotPublisher.ExecuteAsync now
mirrors AlarmsHubPublisher's reconnect loop — wraps the await foreach
in a while-not-cancelled, catches general exceptions, and Task.Delays
5s before retrying. An internal ctor accepts a shorter delay for the
test. New test file DashboardSnapshotPublisherTests.cs covers the
throw-then-yield reconnect path and the normal-completion case.
Server-043 (Documentation): HubTokenService class XML doc gains a
<remarks> describing the singleton lifetime, the two consumer scopes
(DashboardHubConnectionFactory scoped, HubTokenAuthenticationHandler
transient), and the thread-safety contract.
Verification: dotnet build src/ZB.MOM.WW.MxGateway.slnx clean
(0 warnings / 0 errors); src/ZB.MOM.WW.MxGateway.Tests 486/486 passing.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Restores the `code-reviews/` tree (was unwritten on this working copy)
and re-reviews every module per `REVIEW-PROCESS.md` against HEAD
`d692232`. The diff in scope is the five commits since the last sweep:
`dc9c0c9` (ZB.MOM.WW gateway-side rename + slnx migrate),
`397d3c5` (client SDK rename + the missing alarm-RPC proto types and
the .NET DiscoverHierarchyOptions POCO), `27ed651` (role-based LDAP
auth + HubToken bearer, drop PathBase), `6594359` (sidebar layout +
three SignalR push hubs), and `d692232` (EventsHub publisher + doc
refresh).
Module status
| Module | Open | Total | Delta this pass |
|---|---|---|---|
| Server | 8 | 43 | +6 |
| Contracts | 2 | 17 | +2 |
| Tests | 2 | 26 | +2 |
| IntegrationTests | 3 | 24 | +3 |
| Client.Java | 5 | 31 | +5 |
| Client.Rust | 1 | 21 | +1 |
| Worker | 0 | 25 | 0 (rename-only diff, clean) |
| Worker.Tests | 0 | 30 | 0 (rename-only diff, clean) |
| Client.Dotnet | 0 | 17 | 0 (rename + alarm-fix diff, clean) |
| Client.Python | 0 | 21 | 0 (rename + alarm-fix diff, clean) |
| Client.Go | 0 | 21 | 0 (rename + alarm-fix diff, clean) |
Total new findings: 19. Severity breakdown: 1 Medium-security
(Server-038), 4 Medium-documentation/coverage, 14 Low.
New findings
* Server-038 (Medium / Security) — EventsHub.SubscribeSession accepts
any session id from any Viewer; no per-session ACL guards the
EventsHub group fan-out.
* Server-039 (Low / Error handling) — HubTokenService.Validate
accepts a payload with null Name/NameIdentifier.
* Server-040 (Low / Conventions) — MapGroupsToRoles undocumented
full-vs-RDN lookup precedence.
* Server-041 (Low / Design adherence) — EventStreamService calls
IDashboardEventBroadcaster.Publish without a try/catch — fragile
seam relying on the never-throw contract.
* Server-042 (Low / Performance) — DashboardSnapshotPublisher tight
retry loop with no backoff (vs AlarmsHubPublisher 5s delay).
* Server-043 (Low / Documentation) — HubTokenService singleton
sharing across login + hub-token validation undocumented.
* Contracts-016 (Low / Conventions) — QueryActiveAlarmsRequest.session_id
reserved-for-future-use ambiguity.
* Contracts-017 (Low / Documentation) — rpc QueryActiveAlarms doc
omits the alarm_filter_prefix filter description.
* Tests-025 (Low / Conventions) — duplicate NullDashboardEventBroadcaster
fakes in EventStreamServiceTests and GatewayEndToEndFakeWorkerSmokeTests.
* Tests-026 (Medium / Testing coverage) — no test proves
EventStreamService actually calls IDashboardEventBroadcaster.Publish.
* IntegrationTests-022 (Low / Conventions) — ResolveRepositoryRoot
silent fallback to Directory.GetCurrentDirectory().
* IntegrationTests-023 (Low / Testing coverage) — DashboardLdapLiveTests
success-path asserts ldap_group but not the Role claim.
* IntegrationTests-024 (Low / Conventions) — inline
NullDashboardEventBroadcaster fake duplicates Tests-side copies.
* Client.Java-027 (Medium / Documentation) — README + JavaClientDesign
Gradle task names still use the old short project names.
* Client.Java-028 (Medium / Design adherence) — JavaClientDesign
build-layout shows the old `com/dohertylan/mxgateway/` package paths.
* Client.Java-029 (Low / Documentation) — README installDist path
cites the wrong directory.
* Client.Java-030 (Low / Testing coverage) — no Java test exercises
the regenerated QueryActiveAlarmsRequest RPC.
* Client.Java-031 (Low / Conventions) — README prose uses old short
project names instead of canonical prefixed ones.
* Client.Rust-021 (Low / Design adherence) — RustClientDesign.md
"Crate layout" shows an aspirational nested `crates/zb-mom-ww-mxgateway-client/`
that does not exist; actual layout is the flat top-level crate.
Two pre-existing pending findings (Server-031 lock-contention,
Server-032 bounded event channel) remain unchanged — neither was
touched by this wave of commits.
Process notes
- The `code-reviews/` tree was not in this working copy's git
history (the local extract pre-dates the divergent branch that
carried the reviews). Restored from `dd7ca16` via
`git checkout dd7ca16 -- code-reviews/` before the re-review.
- Some "Resolved" entries in the restored findings.md reference
fixes that landed on the divergent branch (the same one that
carried the reviews) and are not present on the current main
lineage. The re-review treats those statuses as historical;
the new pass only files findings against HEAD's actual state.
- `python code-reviews/regen-readme.py --check` is green.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Joseph Doherty