Joseph Doherty
d2d2e5f68f
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>
77 lines
4.0 KiB
Markdown
77 lines
4.0 KiB
Markdown
# 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 same
|
|
workflow already used to resolve the Critical dashboard finding and the
|
|
Client.Rust module (see git commits `a8aafdf`, `0d8a28d`, `9082e50`).
|
|
|
|
## 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/` — a `src/MxGateway.*` project or
|
|
a `clients/` language client. The module→source mapping and the per-module
|
|
build/test commands are in `CLAUDE.md` (the "Source Update Workflow" table and
|
|
the per-client 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.)
|
|
- Run that module's full build and test suite with the module-appropriate
|
|
toolchain and confirm it is green:
|
|
- `src/MxGateway.*` .NET projects — `dotnet build` + `dotnet test` for the
|
|
project; the Worker must build x86 (`-p:Platform=x86`).
|
|
- `clients/dotnet` — `dotnet build clients/dotnet/MxGateway.Client.sln` and its tests.
|
|
- `clients/go` — `gofmt`, `go build ./...`, `go test ./...`.
|
|
- `clients/rust` — `cargo fmt`, `cargo test --workspace`,
|
|
`cargo clippy --workspace --all-targets -- -D warnings`.
|
|
- `clients/python` — `python -m pytest`.
|
|
- `clients/java` — `gradle test`.
|
|
- A regression test for a gateway-server finding belongs in `src/MxGateway.Tests`;
|
|
for a worker finding, in `src/MxGateway.Worker.Tests`. Adding a test there is
|
|
permitted even though it is a different module's source tree.
|
|
- Update only that module's `code-reviews/<Module>/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 test project
|
|
— e.g. do not run the `Server` and `Contracts` agents together, since both add
|
|
regression tests under `src/MxGateway.Tests`.
|
|
|
|
## After each batch returns (orchestrator does this — keep your own context lean)
|
|
|
|
- Build and test every component the batch touched, using the `CLAUDE.md`
|
|
commands; confirm clean. For any .NET change, `dotnet build src/MxGateway.sln`.
|
|
- 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.
|