From ac29599406f9ca4d998e15baf6098edf00c531b7 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Sun, 6 Sep 2026 14:01:57 -0400 Subject: [PATCH] =?UTF-8?q?docs(plans):=20Flow=20Debugger=20architectural/?= =?UTF-8?q?code=20review=20=E2=80=94=204=20Medium=20+=209=20Low=20findings?= =?UTF-8?q?,=20test=20evidence?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Full review of the Flow Debugger commit range (v1.0, v1.1 outbound calls, v1.2 follow-ups, outcome-merge fix, two perf commits). Also records the exact 11 role-negative Playwright tests that cannot pass on the login-disabled rig in the cluster-ops skill. Claude-Session: https://claude.ai/code/session_01NcMG6xXdsJbmD2wYAgkK29 --- .../skills/scadabridge-cluster-ops/SKILL.md | 2 +- .../2026-09-06-flow-debugger-arch-review.md | 330 ++++++++++++++++++ 2 files changed, 331 insertions(+), 1 deletion(-) create mode 100644 docs/plans/2026-09-06-flow-debugger-arch-review.md diff --git a/.claude/skills/scadabridge-cluster-ops/SKILL.md b/.claude/skills/scadabridge-cluster-ops/SKILL.md index 7c8deb24..d3334558 100644 --- a/.claude/skills/scadabridge-cluster-ops/SKILL.md +++ b/.claude/skills/scadabridge-cluster-ops/SKILL.md @@ -6,7 +6,7 @@ description: How to drive the local ScadaBridge docker cluster — CLI managemen # ScadaBridge Cluster Ops (Docker / OrbStack) - **Management URL**: `http://localhost:9000` — the CLI connects via the Traefik load balancer, which routes to the active central node. Direct access: central-a on port 9001, central-b on port 9002. -- **Central UI login is DISABLED on the rig (both central nodes, since 2026-09-06)** — `ScadaBridge:Security:Auth:DisableLogin=true` in `docker/central-node-{a,b}/appsettings.Central.json`, so `http://localhost:9000` (and 9001/9002) opens the UI as `multi-role` with every role, no LDAP login. The CLI/management API still takes `--username/--password` (Basic auth is a separate path), and `POST /auth/login` still answers 302, so the Playwright fixture login keeps working; only `LoginTests` (expects a `/login` redirect + 401 on bad credentials) fails against this rig. Flip the two keys back to `false` and recreate the node for real-login checks. +- **Central UI login is DISABLED on the rig (both central nodes, since 2026-09-06)** — `ScadaBridge:Security:Auth:DisableLogin=true` in `docker/central-node-{a,b}/appsettings.Central.json`, so `http://localhost:9000` (and 9001/9002) opens the UI as `multi-role` with every role, no LDAP login. The CLI/management API still takes `--username/--password` (Basic auth is a separate path), and `POST /auth/login` still answers 302, so the Playwright fixture login keeps working. Expected Playwright failures on this rig are exactly the 11 role-NEGATIVE tests (full run 2026-09-06: 164/175) — `RoleNavigationTests.*_DoesNotSee*`, `AuditLogPageTests.PermissionGating_DesignerWithoutOperationalAudit_IsDeniedAccess`, `LoginTests.UnauthenticatedUser_RedirectsToLogin` — because every browser session carries every role; any other Playwright failure is real. Flip the two keys back to `false` and recreate the node for real-login checks. - **Test user**: `--username multi-role --password password` — has Admin, Design, and Deployment roles. The `admin` user only has the Admin role and cannot create templates, data connections, or deploy. - **Config file**: `~/.scadabridge/config.json` — stores `managementUrl` and default format. See `docker/README.md` for a ready-to-use test config. - **Rebuild cluster**: `bash docker/deploy.sh` — builds the `scadabridge:latest` image and recreates all containers. Run this after code changes to ManagementActor, Host, or any server-side component. diff --git a/docs/plans/2026-09-06-flow-debugger-arch-review.md b/docs/plans/2026-09-06-flow-debugger-arch-review.md new file mode 100644 index 00000000..8cf46ee7 --- /dev/null +++ b/docs/plans/2026-09-06-flow-debugger-arch-review.md @@ -0,0 +1,330 @@ +# Flow Debugger — architectural / code review + +**Date:** 2026-09-06 · **Status:** REVIEW REPORT (no code changed by this review) +**Scope:** every commit from `7fa916e2` (Flow Debugger design) to `0ccbe140` (cached-call forwarder +enqueue-and-observe) on `main` — Flow Debugger v1.0, v1.1 outbound calls, v1.2 follow-ups, the +outcome-merge fix and the two same-day performance commits. 165 files, +20 228 / −778 lines. +**Method:** read of every non-test source file in the range against the three design/plan documents +(`2026-09-05-flow-debugger-design.md`, `2026-09-06-flow-debugger-outbound-calls.md`, +`2026-09-06-flow-debugger-followups.md`) plus `2026-09-06-tracking-store-async-writes-design.md`; +targeted verification of the claims the code comments make (where redaction runs, how the +`ManagementActor` dispatches, proto field numbering, timestamp storage format, page gates); full +solution test run (§6). + +## 1. Verdict + +The feature is architecturally sound and lands where the design said it would: trace steps are +audit events, every contract change is additive, the site never depends on central for tracing, +the run path is one management command shared by UI and CLI, and the per-node actor is not blocked +by a long run. Test coverage is broad (≈ 6 000 lines of new tests across every layer). + +Four findings warrant a fix before the feature is relied on outside the rig, all Medium, none +requiring a redesign: + +| # | Severity | Finding | Owner component | +|---|---|---|---| +| F1 | **Medium** | The inline trace copy (routed reply + Live result) is the **pre-redaction, un-capped** event — per-target SQL parameter redaction and the 8 KB / 64 KB payload caps only apply to the stored row; the wire copy is bounded by count, not bytes | Site Runtime, Inbound API, Management | +| F2 | **Medium** | The **method-entry Live run has no site-scope check**; only the value-change entry enforces `PermittedSiteIds` | Management, Inbound API | +| F3 | **Medium** | **Replay is Deployer-gated**; the design (D6, §9) says `OperationalAudit`, and the Audit Log / Execution Chain drill-ins invite auditors onto a page they cannot open | Central UI, Management | +| F4 | **Medium** | **Three unrelated run timeouts**: server = method timeout + 10 s; UI Ask = fixed 2 min; CLI = fixed 2 min. A method with `TimeoutSeconds > ~110` makes both clients give up while the run continues | Central UI, CLI | + +Then nine Low findings (F5–F13) — ordering under clock skew, a trace-arm false positive, silent +shedding of traced runs, continuation-scheduler consistency, a duplicated chain walker, a +kind-only prediction overlay, always-on `RouteCall` vs `SkipBodyCapture`, dead stream fields, and +doc/implementation drift — and a list of what was checked and found correct (§4). + +## 2. Findings + +Severity: **High** = data loss / security boundary / outage; **Medium** = wrong behaviour an operator +will hit, or a documented property that does not hold; **Low** = quality, consistency, or a +narrow edge. Each finding names the file, states what the code does, why it matters, and the fix. + +### F1 — Inline trace copy bypasses redaction and payload caps `[Medium]` + +**Where.** `SiteRuntime/Scripts/FlowTraceCollector.cs`; `ScriptRuntimeContext.Collect` / +`EmitTrace`; `ScriptRunLauncher.EmitScriptRun`; `InboundAPI/RouteHelper.cs` `EmitRouteCall`; +`InboundAPI/InboundDatabaseHelper.cs` `EmitAudit`; `ManagementService/FlowDebug/FlowDebugRunner.cs`. + +**What.** Every emit site deliberately hands the collector / trace sink the *same* `AuditEvent` +instance it then hands the writer, "collector first". But redaction and capping live inside the +writers — `FallbackAuditWriter.WriteAsync` (`_redactor.Apply(evt)`, line 88) on the site and +`CentralAuditWriter.WriteAsync` (line 113) on central — and both return a *new* filtered record. +The collector therefore holds the raw event: no `PerTargetOverrides` SQL-parameter redaction, no +header redaction, no `DefaultCapBytes` (8 KB) / `ErrorCapBytes` (64 KB) truncation. That raw list +is what `ScriptCallResult.TraceEvents` → `RouteToCallResponse.TraceEvents` carries over gRPC, what +`FlowDebugRunResult.Events` returns to the page and the CLI (`--format json` prints it verbatim), +and what the page renders in a Live run. Replay reads the stored, redacted rows — so Live and +Replay of the same execution can show different payloads. + +**Why it matters.** +- Design §9 states "Trace payloads pass through the existing redaction policy (auth headers + redacted, 8 KB cap, 64 KB on error rows)". It does not hold for the Live path. +- A per-target redaction override exists precisely to keep specific SQL parameter values out of + audit; a Deployer running the Flow Debugger sees them anyway. +- Size: `FlowTraceCollector` caps at **500 events, not bytes**. A traced run whose DB/API rows + carry large request/response summaries (a 64 KB error detail × a few hundred rows) exceeds the + gRPC default 4 MB receive limit on central's `GrpcSiteTransport` channel (no + `MaxReceiveMessageSize` is set anywhere in `src/`). The routed call then fails with + `ResourceExhausted` — and because the same reply carries the script's *return value*, a method + flagged `TraceAllCalls` would fail a **production** call on payload size alone. + +**Fix.** Apply the redactor at the collector boundary, not the writer: inject `IAuditRedactor` +into `ScriptRuntimeContext`'s `Collect` (site) and into `FlowDebugRunner`'s sink (central) so the +collector stores `redactor.Apply(evt)`; the writers keep their own call (idempotent). Add a byte +budget to `FlowTraceCollector` (e.g. 1 MB, counted on the redacted `DetailsJson` length) alongside +the 500-event cap, setting `Truncated` when either trips. Tests: a per-target SQL redaction +override is honoured on `RouteToCallResponse.TraceEvents`; a collector over budget truncates. +Effort: small (≈ 60 lines + tests). + +### F2 — Method-entry Live run has no site-scope check `[Medium]` + +**Where.** `FlowDebugRunner.RunMethodAsync` vs `RunValueChangeAsync` (`EnforceSiteScope`). + +**What.** The value-change entry refuses with `SiteScopeViolationException` when the instance's +site is outside `user.PermittedSiteIds`. The method entry has no equivalent: a Deployer whose role +is scoped to site A can pick any method whose script routes to site B and run it Live — real +`Route.To().Call`, `SetAttributes`, `Database.*`, `Notify.Send` against site B, with the operator's +username on the inbound row. Design §9 only mentions the value-change entry, so this is an +omission in the design carried into the code, not a deviation. + +**Why it matters.** Site-scoped Deployer is the project's operator-of-one-plant model. The HTTP +inbound API does not weaken it (API keys are a separate credential); the Flow Debugger does. + +**Fix.** Cheapest and complete: give `RouteHelper` a site-scope predicate (`WithSiteScope( +IReadOnlySet permittedSiteIds)`), checked in `RouteTarget` right after `_instanceLocator` +resolves the target's site id, throwing `SiteScopeViolationException` (already mapped to +`ManagementUnauthorized`). `FlowDebugRunner.RunMethodAsync` sets it from `user`; the HTTP endpoint +passes nothing (unchanged). The static prediction already resolves constant route targets, so the +page can also warn before the run. Effort: small. + +### F3 — Replay requires Deployer; the design says OperationalAudit `[Medium]` + +**Where.** `ManagementActor.GetRequiredRoles` (`FlowDebugReplayCommand => DeployerOnly`); +`FlowDebugger.razor` (`[Authorize(Policy = RequireDeployment)]`); drill-in buttons in +`AuditEventDetail.razor` and `ExecutionTreePage.razor`. + +**What.** Design D6 / §9: "Live run is a Deployer act … Replay needs `OperationalAudit`". The +implementation gates the page and all three commands on Deployer. The Audit Log and Execution +Chain pages — whose own gate is `OperationalAudit` — now show an *Open in Flow Debugger* button +that an auditor without Deployer cannot use. + +**Fix.** Either amend D6 (and hide the drill-in for non-Deployers), or implement the split: page +policy "OperationalAudit or Deployment", `FlowDebugReplayCommand` accepts either role, Run button +and the two run commands stay Deployer-only. The split is ≈ 20 lines plus a +`RequiredRoleMatrixTests` row; it is the better answer because the audit drill-in is the feature's +main entry for a *past* incident. + +### F4 — Three unrelated Live-run timeouts `[Medium]` + +**Where.** `FlowDebugRunner.RunMethodAsync` (`timeout + RunGrace`, `CancellationToken.None` from +the actor); `CentralUI/Services/FlowDebugService.cs` (`RunAskTimeout = 2 min`); +`CLI/Commands/FlowDebugCommands.cs` (`RunTimeout = 2 min`); `ManagementEndpoints` long-running +list (server side is correctly exempted from the ordinary Ask timeout). + +**What.** The server bounds a run by the method's `TimeoutSeconds` (unbounded positive int) plus +10 s. Both clients wait a fixed 2 minutes. `FlowDebugService`'s comment asserts "both fit inside +two minutes" — nothing enforces it. A method with `TimeoutSeconds = 180` (legal; long integration +handshakes exist) makes the page show `TRANSPORT_ERROR` and the CLI a 504 at 120 s while the run +continues to completion on central, writes its audit rows, and cannot be cancelled (the handler +runs on `CancellationToken.None`). The value-change entry has the same shape (`WaitSeconds` is +user-supplied and unbounded; UI sends the default 45). + +**Fix.** Derive the client wait from the request: UI = `method.TimeoutSeconds + RunGrace + 15 s` +(the page already has the method list) and `WaitSeconds + 15 s`; CLI = same after a `GetApiMethod` +read, or a `--timeout` option. Optionally clamp `TimeoutSeconds` for Live runs and say so in the +error. Effort: small. + +### F5 — Step ordering trusts cross-node timestamps `[Low]` + +**Where.** `FlowDebugRunner.RunMethodAsync` (`events.OrderBy(e => e.OccurredAtUtc)`), +`FlowGraphBuilder.Build` (global sort by `OccurredAtUtc`, spawner search +`p.OccurredAtUtc <= first.OccurredAtUtc`). + +**What.** Central rows carry central's clock, site rows the site's. The collector preserved +emission order, and the runner discards it by re-sorting. With a site clock 1–2 s ahead, site steps +sort after the Response node in `Steps` (the CLI table and the detail list) and a trigger cascade's +spawner search falls back to `parentChain[0]`. Edges are id-based so the graph stays connected; +this is display order only. Route calls are unaffected (linked by `ChildExecutionId`). + +**Fix.** Keep a sequence number per event on the inline path (the collector's index) and sort by +`(OccurredAtUtc within an execution, sequence)`; or simply preserve the collector's order and +merge Replay rows behind it. Low. + +### F6 — Trace arm can stamp an unrelated device update `[Low]` + +**Where.** `InstanceActor.HandleSetDataAttribute` (arms before the DCL write completes), +`HandleTagValueUpdate` (consumes the arm "whatever the value"). + +**What.** The arm is recorded when the traced write is *issued*, not when the DCL acknowledges it. +If the write fails (bad quality, device rejects) the arm stays for 30 s; the next unrelated +subscription update on that attribute is stamped with the debugger's execution id and spawns a +traced, parented run — the debugger then shows a "device echo" of a write that never landed. +Design §5.3 accepts "whatever the value", which covers clamping, not failure. + +**Fix.** Clear the arm in the `WriteTagResponse` failure continuation (the actor already pipes the +response back; a `Tell` to self with the attribute name is enough). Low. + +### F7 — A shed or rate-limited traced run is invisible `[Low]` + +**Where.** `ScriptActor.TrySpawnExecution` (`MinTimeBetweenRuns` suppression) and +`SpawnExecution` (`MaxConcurrentRunsPerScript` → `ShedRun`). + +**What.** A traced value-change run dropped by either guard emits no `ScriptTrace` row; the +debugger reports `NO_TRIGGER_WITHIN_WINDOW` with the message "no value-change trigger on this +attribute, or no device echo" — both wrong in that case, and the operator is sent to check the +wrong things. + +**Fix.** When `trace` is set, emit a `ScriptRun` row with `Failed` and an error of +`shed: MaxConcurrentRunsPerScript` / `suppressed: MinTimeBetweenRuns` (the launcher's +`EmitScriptRun` can be reused with `elapsedMs = 0`). Low. + +### F8 — Fault continuations: scheduler and logging inconsistent `[Low]` + +**Where.** `ScriptRuntimeContext.EmitTrace`, `ScriptRunLauncher.EmitScriptRun`, +`RouteTarget.EmitRouteCall`, `InboundDatabaseHelper.EmitAudit`. + +**What.** All four use `ContinueWith(..., TaskContinuationOptions.OnlyOnFaulted)` with no +scheduler argument, so the continuation inherits `TaskScheduler.Current` — on a script thread that +is the blocking-I/O `ScriptExecutionScheduler`. `CachedCallTelemetryForwarder` (same commit range) +passes `TaskScheduler.Default` explicitly. The two central emitters also discard the fault +(`static _ => { }`) with no log line, so a central audit outage is silent on those rows. + +**Fix.** Pass `CancellationToken.None, OnlyOnFaulted | ExecuteSynchronously, TaskScheduler.Default` +everywhere, and log at Warning in the two central emitters (a logger is one constructor parameter +away in both). Low. + +### F9 — `GetTraceEventsAsync` duplicates the execution-tree walk `[Low]` + +**Where.** `ConfigurationDatabase/Repositories/AuditLogRepository.cs`. + +**What.** The new method re-implements `GetExecutionTreeAsync`'s three-statement walk (up to +root, root window, recursive `Edges`/`Chain` CTE with `MAXRECURSION` and the 7-day window) as a +second copy, then filters channels with EF. Two walkers that must stay in step (window slack, +depth, the DISTINCT-edges trick). + +**Fix.** Extract `ResolveChainExecutionIdsAsync(executionId)` and have both call it. Low. + +### F10 — Prediction overlay matches real steps by kind only `[Low]` + +**Where.** `FlowGraphView.ComputeLayout` (`unmatched.FirstOrDefault(p => p.Kind == s.Kind)`). + +**What.** A method with two route calls (`A.X` then `B.Y`) that only executes the second one +matches the real `B.Y` step to the predicted `A.X` node; the ghost shown is `B.Y`. Wrong greying +for multi-route or branching methods. + +**Fix.** Match by `(Kind, Title)` first, then fall back to kind. Low, UI only. + +### F11 — Always-on `RouteCall` row ignores the method's `SkipBodyCapture` `[Low]` + +**Where.** `RouteTarget.EmitRouteCall` (`requestSummary: TraceJson(args)`), vs +`AuditWriteMiddleware` (honours `PerTargetOverrides[methodName].SkipBodyCapture` on the inbound +row). + +**What.** An operator who configured `SkipBodyCapture` for a method to keep its body out of the +audit log now gets the routed *arguments* — derived from that body — on every call's `RouteCall` +row, keyed by `{instance}.{script}`, which the per-method override does not match. Also: every +routed call is now one extra central MS SQL insert (fire-and-forget); at inbound-driven rates that +doubles the per-call central audit write, which the design accepted (D5) but should be stated in +the volume section. + +**Fix.** Thread the method's `SkipBodyCapture` into `RouteHelper` (the executor has the method) +and emit the row without summaries when set; add the sentence to design §10. Low. + +### F12 — Shipped but unused: `AttributeValueUpdate.source_execution_id` / `trace` `[Low]` + +**Where.** `sitestream.proto` fields 7/8, `StreamRelayActor`, `SiteStreamGrpcClient`. + +**What.** Design §5.4 planned the value-change entry to subscribe to the debug stream and take +the spawned id from the stamped `AttributeValueChanged`. The implementation polls +`PullTraceEvents` by `ParentExecutionId` instead (simpler, and correct). The stream fields are +therefore populated on every change and consumed by nothing. Not harmful — additive, one string +per update — but the design should record the switch and the fields should be marked "reserved +for the debug-stream path" or dropped from the client mapping. + +### F13 — Documentation drift to reconcile `[Low]` + +- Design §5.4 (debug-stream subscription) vs the polling implementation — see F12. +- Design D6 / §9 (Replay = OperationalAudit) vs Deployer everywhere — see F3. +- Design §9 (redaction applies to trace payloads) vs F1. +- `ApiMethodForm` / CLI help say "~7 audit rows per call"; the outbound-calls addendum makes a + call with one cached DB write and one notification **10 rows** (3 trace-only). Say "7–10". +- `FlowDebugRunner.RunMethodAsync` writes the inbound row *after* the run, so a Replay taken during + a long Live run shows only the `RouteCall`. Fine, but not stated anywhere. +- `SchemaRefResolver` became `public` for the runner; note it in Component-InboundAPI or move the + validation into `InboundScriptExecutor`. + +## 3. Performance commits (tracking store + forwarder) + +`OperationTrackingStore` single-writer queue (`d91135f9`) — reviewed line by line, **no findings**: +`_pending` is balanced on every path (enqueue failure, batch complete, per-row fallback, pump +teardown); the read barrier is skipped only when nothing is queued and otherwise rides the same +FIFO, so read-your-writes holds; the per-row fallback keeps good rows; `DisposeAsync` drains and +sync `Dispose()` fails queued items as documented; timestamps are stamped at record time. The one +trade — a queued-but-unapplied row lost on a hard crash — is stated in the file and the design memo. + +`CachedCallTelemetryForwarder` enqueue-and-observe (`0ccbe140`) — correct, and the design memo's +"correction" section is the right record: the first attribution (SQLite commits) was wrong, the +measurement said so, and the doc says so. The measured result (inbound 194 → 25 ms, script run +176 → 17 ms) stands. + +## 4. Verified correct (not findings) + +- **Additive contracts throughout.** Every record gains trailing optional parameters; proto fields + are new numbers (`RouteToCallRequestDto.trace = 7`, `RouteToCallResponseDto` 6/7, + `RouteToSetAttributesRequestDto.trace = 6`, `AttributeValueUpdate` 7/8, new `PullTraceEvents` + RPC); `AuditChannel.ScriptTrace` and the four kinds are appended last; `UpdateApiMethodCommand. + TraceAllCalls` is `bool?` so an older CLI never resets it; proto3 defaults are the negotiation, + as for stream batching. The `SiteCommandDtoMapper` maps an empty `trace_events` to `null`. +- **The `ManagementActor` is not blocked by a Live run** — `HandleEnvelope` runs `ProcessCommand` + and `PipeTo`s the result, so a 5-minute run does not stall other management commands. The + standby refusal (`IActiveNodeGate`, `STANDBY_NODE`) and the new `ManagementCommandException( + message, code)` path are correct and backwards compatible. +- **`FlowTraceCollector.Complete()`** closes the sink at the reply snapshot; later lifecycle rows + go to the writer only; `GuardTraceSink` means a throwing observer can neither fail a call nor skip + the durable write (pinned by tests). +- **`PullTraceEvents`** sits behind the existing `/sitestream.SiteStreamService/` PSK interceptor + prefix; caps ids at 64 and rows at 2000 on both ends; moves no forward-state cursor; the new + `IX_audit_event_occurred` index bounds the `json_extract` scan and the timestamp text format + matches the stored `"o"` form. +- **Migration** `AddApiMethodTraceAllCalls` + manual SQL twin with the `QUOTED_IDENTIFIER` header. +- **Central `Database.*` audit** (`InboundDatabaseHelper`): one `DbOutbound/DbWrite` row per call, + vocabulary-locked with the site's `AuditingDbCommand`; failure inside the audited span; result or + exception always reaches the script. +- **Graph builder**: fold by `(kind, CorrelationId)`, `LifecycleRank` tie-break, `IsAsync` rule, + leading-async linking from the spawner, stable synthesized Response id — all pinned by + `FlowGraphBuilderTests`. +- **Page**: outcome refresh merges by `EventId`; the pending-refresh loop is bounded (5 s / 2 min), + cancelled on mode switch, new run and dispose; Retry/Discard re-check site scope and reuse the + report pages' relay and wording; the report pages are `RequireDeployment` too, so the debugger's + actions add no privilege. The prediction circuit deadlock is fixed with a resolve-and-rewalk loop + and pinned by a single-thread-pump test. +- **Site-side gating**: untraced runs allocate no collector and emit nothing; only traced + `SetStaticAttributeCommand` writes arm; `TraceArmWindowSeconds` is validated `> 0`. +- **Rig proof**: `verify-delmia-notifier.sh` steps H and I (live + replay, outbound nodes) passed + on the rig per the commit ledger. + +## 5. Recommended order + +1. F1 (redact + byte-cap the inline copy) and F2 (route-time site scope) — same session, both + small, both close a stated security property. +2. F3 (Replay role split) and F4 (derive client waits) — small, operator-visible. +3. F6, F7, F8 — site-runtime polish, one commit. +4. F5, F9, F10, F11 — quality; can ride the next Flow Debugger change. +5. F12, F13 — docs; do with (1) so the design stops disagreeing with the code. + +Register rows: F5, F10, F11, F12 are candidates for the deferred-work register if not fixed in the +next pass; F1–F4 should not be deferred. + +## 6. Test evidence + +Full `dotnet test ZB.MOM.WW.ScadaBridge.slnx` at review time (2026-09-06, `main` @ `0ccbe140`): +**29 of 30 projects green, 9 297 tests passed.** The one red project is +`CentralUI.PlaywrightTests` (164 / 175): its 11 failures are the role-negative browser checks +(`RoleNavigationTests.*_DoesNotSee*`, `AuditLogPageTests.PermissionGating_…_IsDeniedAccess`, +`LoginTests.UnauthenticatedUser_RedirectsToLogin`) — every one asserts that a *lesser* role is +denied something, which cannot hold on the rig since 2026-09-06 (`2b80e1d8`) because both central +nodes run `DisableLogin=true` and every browser session is the all-roles `multi-role` user. None +touches the Flow Debugger; `Deployment/FlowDebuggerTests` (Live run + localStorage restore) passed. +Consequence worth recording alongside the login-disabled decision: the rig can no longer prove +role gating in the browser, so the Playwright suite's role-negative tests need either a second, +login-enabled central to target or a fixture that overrides the identity per test.