From ddfd41b06d6972089d6baae250ab8b4ef5a40f2c Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 20 Aug 2026 09:14:40 -0400 Subject: [PATCH] docs(archreview): cycle-3 remediation plan + tracker (batches A-G, model assignments) --- .../2026-08-19/remediation/00-tracking.md | 77 +++++++++++++++++++ 1 file changed, 77 insertions(+) create mode 100644 archreview/2026-08-19/remediation/00-tracking.md diff --git a/archreview/2026-08-19/remediation/00-tracking.md b/archreview/2026-08-19/remediation/00-tracking.md new file mode 100644 index 0000000..1257225 --- /dev/null +++ b/archreview/2026-08-19/remediation/00-tracking.md @@ -0,0 +1,77 @@ +# MxAccessGateway — Remediation Plan & Tracking (2026-08-19 review) + +Source review: [`../00-overall.md`](../00-overall.md) (HEAD `8b3e3b3`, committed as `f4919ba`). Generated 2026-08-20. +Tracks the 34 cycle-3 IDs (GWC-31+, WRK-32+, IPC-33+, SEC-37+, CLI-46+, TST-31+) plus the tracker-housekeeping actions the overall asked for. Cycle-1/2 trackers remain the record for their own IDs. + +**Status legend:** `Not started` · `In progress` · `Done` · `Blocked (external)` · `Deferred (P2)` · `N/A`. + +## Execution model + +This plan is executed as **seven parallel batches**, each by one subagent working in an isolated git worktree on its own `fix/c3-` branch, merged back into `main` by the coordinator in the order listed (lowest conflict surface first). Every batch must: build the affected component(s) on macOS (`NonWindows.slnx` — the x86 worker cannot build here), run only the filtered tests it touches, update docs in the same commit, and cite finding IDs in commit messages (`fix(IPC-34): …`). Worker-side changes (batches E, F) are verified afterwards on windev via `windev-worker-ci.ps1 -Mode test`. + +**Model assignment rationale:** `opus` for batches that change concurrency/authorization logic or require reasoning across several interacting files with regression tests; `sonnet` for contained, well-specified edits with a clear recipe (version bumps, doc reconciliation, a two-line worker fix); `haiku` for none — every batch either touches code invariants or needs judgement about stale prose, where the cost of a wrong mechanical sweep exceeds the saving. + +| Batch | Branch | Findings | Model | Why this model | Status | +|---|---|---|---|---|---| +| A | `fix/c3-client-versions` | CLI-46 | sonnet | Mechanical five-client bump + README note + one script guard; recipe is fully specified | Not started | +| B | `fix/c3-glauth-doc` | TST-31 | sonnet | Reconcile a table against one TOML source of truth, re-derive one default, change an assert to a log | Not started | +| C | `fix/c3-tracker-hygiene` | GWC-31 + housekeeping (close GWC-16/IPC-08/IPC-31, refresh TST-06/TST-17, register WRK-31) | sonnet | Cross-file comment/ID sweep needs care about what each ID *means*, but no logic changes | Not started | +| D | `fix/c3-dashboard-acl` | SEC-40, SEC-41, GWC-38 | opus | Authorization semantics, fail-closed decisions, claims re-resolution; each needs a regression test | Not started | +| E | `fix/c3-ipc-boundaries` | IPC-33, IPC-39, IPC-34, IPC-35 | opus | Changes the WorkerClient command lifecycle, the options validator, and the worker drain budget; race-sensitive | Not started | +| F | `fix/c3-worker-residuals` | WRK-32, WRK-33 (+WRK-34 opportunistic) | sonnet | Small, precisely located worker edits + doc prose; cannot be built on macOS so the agent's job is a careful, minimal diff | Not started | +| G | `fix/c3-session-lifecycle` | GWC-32, GWC-34, GWC-37 | opus | Distributor/dispose lifecycle races and an alarm-monitor re-resolve path; needs lock-discipline reasoning | Not started | + +**Out of scope for this execution (recorded, not dropped):** + +| ID | Status | Why | +|---|---|---| +| TST-32 | Blocked (external) | Gitea runner registration-token reset is a web-UI action on the Gitea admin page; no repo change. Operator to-do. | +| SEC-37 | Blocked (external) | Family-wide: TLS on the shared GLAuth lives in `scadaproj/infra/glauth/` (separate repo + docker host). Plan separately; after it lands, move the deployed service to `Production`. | +| CLI-47/48/49/50, IPC-36, IPC-37, TST-33/34/35/36, GWC-33/35/36, SEC-38/39, IPC-38, WRK-34 | Deferred (P2) | Roadmap tier 10; WRK-34 may be picked up by batch F if trivial. | + +## Batch specifications + +### A — CLI-46 client release-train bump (sonnet) +- Bump .NET/Go/Python/Rust clients `0.2.0 → 0.2.1`, Java `0.2.1 → 0.2.2` in every version-bearing file (`pyproject.toml` + `version.py`, `Cargo.toml` workspace + any `version =` consumers, `Directory.Build.props`/csproj ``, Java `build.gradle`/`gradle.properties`, Go module is tag-driven — update any `Version` const if one exists). Follow `docs/ClientPackaging.md` for the authoritative file list. +- `clients/python/README.md` + `docs/ClientPackaging.md`: note that published pypi 0.2.0 has a broken `connect()` (event-loop binding, fixed by `a8f86b5`) and is superseded by 0.2.1. +- Add a PR-time check to `scripts/check-codegen.ps1` (or a new `scripts/check-client-versions.ps1` wired where the other checks run): fail if any `clients/` source differs from its last publish tag while the version constant still equals that tag's version. Document in `docs/ClientPackaging.md`. +- Verify: `dotnet build clients/dotnet/...slnx`; `cargo check --workspace`; `python -m pytest` (clients/python); `JAVA_HOME=/opt/homebrew/opt/openjdk@17 gradle test` then **revert** spurious generated-proto churn; `go build ./... && go test ./...`. + +### B — TST-31 glauth.md + live-test credential (sonnet) +- Reconcile `glauth.md` against `~/Desktop/scadaproj/infra/glauth/config.toml`: ReadOnly gid, the pre-provisioned users table (drop `readonly/readonly123`, fix `admin` password), the worked example at `:124`. Keep the OPC-UA taxonomy note. +- Re-derive `MXGATEWAY_LIVE_MXACCESS_WRITE_SECURED_PASSWORD` default in `docs/GatewayTesting.md` and the `?? "admin123"` literal in `WorkerLiveMxAccessSmokeTests.cs` from the config; make the test **log** (not assert) whether `AuthenticateUser` succeeded. +- Verify: `dotnet build src/ZB.MOM.WW.MxGateway.NonWindows.slnx` (IntegrationTests compiles on mac? if excluded, note it). + +### C — GWC-31 + tracker housekeeping (sonnet) +- Sweep code comments citing unregistered `GWC-30`/`GWC-31` (perf pass) in `WorkerClient.cs`, `GatewayMetrics.cs`, `WorkerClientTests.cs`, worker `WorkerFrameReader/Writer.cs` to `PERF-nn` keyed to task numbers in `docs/plans/2026-08-15-perf-review-remediation.md`; add a PERF-ID table to that plan. Do not touch the genuine GWC-30 (frame-reader prefix) citations if any exist. +- `archreview/REVIEW-PROCESS.md` (or wherever the ID rule lives): state that GWC/WRK/IPC/SEC/CLI/TST prefixes are reserved for archreview registers. +- Cycle-2 tracker (`archreview/2026-07-12/remediation/00-tracking.md` + domain docs): GWC-16 → Won't fix (rationale from cycle-3 10-gateway-core.md), IPC-08 → Done (`7c1ea12`, residual IPC-34), IPC-31 → Done (superseded by GWC-28), refresh TST-06/TST-17 rows, register WRK-31 (interim ID burned by `e913dab`) so it is never reused. +- Verify: `dotnet build src/ZB.MOM.WW.MxGateway.NonWindows.slnx`; `dotnet test --filter FullyQualifiedName~WorkerClientTests`. + +### D — SEC-40, SEC-41, GWC-38 (opus) +- SEC-41: `DashboardSessionAcl` re-resolves tags from `LdapGroupClaimType` claims via `DashboardGroupTagMapping.MapGroupsToTags` against current options (as `HubTokenService` does) instead of trusting login-stamped tag claims; keep stamped claims only as fallback when no group claims exist (document). Test: principal with stale tag claim + group whose grant was removed → denied. +- SEC-40: decide **tighten**: `AlarmsHub` gets a dedicated policy requiring a real role claim (anonymous-loopback/Disabled bypass excluded) — and document in `docs/GatewayDashboardDesign.md` ACL section + `docs/GatewayConfiguration.md` GroupToTag row that alarm metadata remains galaxy-wide for authenticated Viewers (central monitor, no per-session provenance). Test for the policy. +- GWC-38: `GatewayBrowseScopeProvider` throws `RpcException(PermissionDenied)` (and logs) when `Current` is null **and** API-key auth is enabled; unchanged when auth mode is Disabled. Test both branches. +- Verify: build Server; `dotnet test --filter "FullyQualifiedName~DashboardSessionAcl|FullyQualifiedName~AlarmsHub|FullyQualifiedName~BrowseScope"` (adjust to real class names). + +### E — IPC-33, IPC-39, IPC-34, IPC-35 (opus) +- IPC-33 option (a): `GatewayOptionsValidator` adds the upper coupling `Worker:MaxMessageBytes ≤ Protocol:MaxGrpcMessageBytes + 2×reserve`; worker `ResolveDrainReplyByteBudget` becomes `MaxMessageBytes − 2×DrainReplyFrameHeadroomBytes` (floor `MaxMessageBytes/2`); update `docs/GatewayConfiguration.md`, `docs/WorkerFrameProtocol.md`. Test: `GatewayOptionsTests` case for the new bound. +- IPC-39: pre-check compares against `MaxMessageBytes − MaxSequenceEncodingBytes (11)`; fix the comment direction. +- IPC-34: caller-cancellation arms call `TrySendCancelForAbandonedCommand` (renamed) gated on `removed`, skipped when `_stopCts` is the source; new `WorkerClientErrorCode.CallerCancelled` replacing the `GatewayShutdown` mislabel, mapped to `Cancelled` in the gRPC layer; update `docs/Grpc.md` mapping line, `gateway.md:913-919`, `docs/GatewayProcessDesign.md:596-602`. Test: twin of the timeout test driven by the caller token. +- IPC-35: `ReadHandshakeEnvelopeAsync` special-cases `WorkerFault` → `WorkerClientException(WorkerFaulted, CreateWorkerFaultMessage(...))`. Test: fake writes `WorkerFault{MxaccessCreationFailed}` instead of `WorkerReady`, category text surfaces. +- Verify: build Server; `dotnet test --filter "FullyQualifiedName~WorkerClientTests|FullyQualifiedName~GatewayOptions"`. Worker half → windev afterwards. + +### F — WRK-32, WRK-33 (sonnet) +- WRK-33: `ObserveFaultWhenever(ObserveCommandTaskAsync(commandTask));` at `WorkerPipeSession.cs:1235` (or catch-all-and-log in the observer). +- WRK-32: one `pumpStep()` before capturing the `CurrentVersion` baseline in `MxAccessCommandExecutor.cs`; rewrite the executor doc comment (`:996-1003`) to state the recency-based contract; add the same to `docs/MxAccessWorkerInstanceDesign.md` write-completion section and the reply-field comment in `mxaccess_gateway.proto` (**comment-only proto edit → regenerate all generated artifacts in the same commit**, per the repo rule; if regen tooling is unavailable on macOS, skip the proto comment and say so). +- WRK-34 if trivial: `CreateFault(StaHung, "Watchdog", …)` with the correlation id moved into the message. +- Cannot build on macOS: keep diff minimal, net48-safe (no init-only/positional records). Verified on windev afterwards. + +### G — GWC-32, GWC-34, GWC-37 (opus) +- GWC-32: `DisposeAsync` transitions `_state` to `Closed` under `_syncRoot` before completing channels (or `if (_disposed || IsTerminalState()) return;` in `StageWorkerEvent`); update the XML doc to name all three completion paths. Test: dispose a Ready client mid-dispatch → no `ProtocolViolation` fault/metric. +- GWC-34: `_eventDistributorDisposed` flag set under `_syncRoot` when nulling; `EnsureDistributorCreated` throws `SessionManagerException(SessionNotReady)` when set. Test: attach after dispose → throws, no silent hang. +- GWC-37: on `ApplyProviderModeChangeAsync(toMode == Subtag)` re-resolve the watch list and re-subscribe; additionally surface "zero targets resolved while mode can need them" on the provider status (`Degraded` + reason). Update `docs/` alarm-subtag section. Test with the fake worker: GR discovery fails at start, succeeds at failover → watch list non-empty after failover. +- Verify: build Server; `dotnet test --filter "FullyQualifiedName~WorkerClientTests|FullyQualifiedName~GatewaySession|FullyQualifiedName~AlarmMonitor|FullyQualifiedName~AlarmWatchList"`. + +## Merge order +C is merged **last** (its comment sweep touches the same files E and G edit). Order: A, B, F, D, G, E, C. After merge: full `NonWindows` build + one full gateway test run; then windev worker CI for E+F.