Files

T

Joseph Doherty 14f0cae7d9 docs(archreview): mark guards 07/C-1, 07/S-1,S-2,S-4 + #9 failover done; record findings #10 , #11

2026-07-08 18:27:16 -04:00

29 KiB

Raw Blame History

Plan 07 — Client Tooling, Analyzers & Cross-Cutting Engineering System

Status (2026-07-08): ✅ C-1 DONE — fix/archreview-c1-wire-analyzer f0082af5 (OTOPCUA0001 wired tree-wide + ~280 hits triaged; negative-control-verified live; surfaced the RESILIENCE-DISPATCH-GAP, task #10). ✅ S-1/S-2/S-4 DONE — fix/archreview-s1-s2-ci-coverage 10b89830 (whole-solution unit leg + scripts/ci/assert-not-all-skipped.sh fail-on-skip gate + CLI sleep→readiness deflake; skip-gate proven locally, CI-run verification pending a push). Remaining in this domain: S-3 (nightly E2E no-op), S-7 (global.json), U-3/U-4/U-5/U-6 hygiene, C-2..C-6. See STATUS.md.


Source report	`archreview/07-client-tooling-engineering.md`
Overall context	`archreview/00-OVERALL.md` (theme #4 "verification system", action #6/#11)
Base commit	`9cad9ed0` (master); verified against current working tree 2026-07-08
Domain	Client.CLI/Shared/UI, Analyzers, build/CPM, CI, test architecture, repo hygiene

Verification summary

26 findings in the report (S-1..S-8, P-1..P-4, C-1..C-6, U-1..U-8). Every actionable finding was re-verified against the live tree; all confirmed, none stale. Four items are explicitly "no-action / positive" in the report (P-3, P-4, C-3, U-1) and are recorded but not planned. Key verifications performed:

C-1 CONFIRMED — repo-wide grep for ZB.MOM.WW.OtOpcUa.Analyzers in .csproj/.props/.targets returns only the analyzer's own csproj and its test project's ProjectReference. Zero OutputItemType="Analyzer" anywhere in the tree (the only hit is inside the review markdown).
S-1/S-2 CONFIRMED — v2-ci.yml:47-52 gates exactly 5 unit projects; slnx contains 47 *.Tests.csproj (10 of them *.IntegrationTests). Integration job (:61-76) runs fixture-less on ubuntu-latest with no services:. Category traits are declared via [Trait("Category", "…")], so a Category!=E2E&Category!=LiveIntegration filter is valid.
S-3 CONFIRMED — grep for Category…E2E across tests/**/*.cs returns zero matches; the nightly workflow's own header admits the no-op.
S-7 CONFIRMED — no global.json at root (nor anywhere); v2-ci.yml:10-12 claims one exists.
U-3 CONFIRMED — docs/Client.CLI.md has zero sections for confirm, shelve, disable (and only substring hits for ack/enable); all 5 alarm commands ship as command files.
U-4 CONFIRMED — git ls-files shows pending.md, current.md, looseends.md, stillpending.md, HISTORIAN-GATEWAY-INTEGRATION-ISSUES.md all tracked; pending.md:3 itself declares "HARD RULE: never stage pending.md / current.md".
U-5 CONFIRMED — git ls-files lib/ lists 7 tracked proprietary AVEVA DLLs; zero HintPath hits reference lib/ anywhere.
U-6 CONFIRMED — sql_login.txt is .gitignore:47, git check-ignore matches, git log empty (never committed). The retired Driver.Historian.Wonderware* dirs under src/+tests/ are untracked bin/obj husks (NOT tracked projects; not in slnx). Tracked Wonderware residue is only docs/drivers/Historian.Wonderware.md (an intentional retired-stub) and 3 code-reviews/Driver.Historian.Wonderware*/findings.md.
C-2 CONFIRMED — none of the 4 client/tooling src csprojs set TreatWarningsAsErrors.
C-5 CONFIRMED — no .editorconfig; StyleGuide.md:3 says "for all ScadaBridge documentation".
S-5/S-6/U-8 CONFIRMED — OpcUaClientService.cs lock gaps at cited lines; CommandBase.cs:123 Log.CloseAndFlush() + :133 global Log.Logger swap; CommandBase.cs:91 AutoAcceptCertificates = true.

Priority ordering

Tier A — restore meaning to green CI (do first): C-1, S-1, S-2, S-3, S-7. Tier B — repo hygiene: U-5, U-4, U-6, C-5 (StyleGuide title). Tier C — docs: U-3. Tier D — engineering debt (as capacity allows): C-2, P-1, U-2, S-5, S-6, S-4, C-4, C-6, S-8, U-8, P-2. No-action (recorded only): P-3, P-4, C-3, U-1, U-7.

TIER A — Restore meaning to green CI

C-1 (High) — Wire the OTOPCUA0001 analyzer into every project

Restatement: the sole custom analyzer enforces its CapabilityInvoker-wrapping rule against nothing but its own 31 unit tests; no consuming project references it.

Verification: confirmed — only self + test ProjectReference; zero OutputItemType="Analyzer".

Root cause: the "built-but-never-wired" house failure mode (theme #1). The analyzer project was authored and unit-tested but the DI/build-graph wiring step was never done.

Design: inject the analyzer as an analyzer-type ProjectReference from Directory.Build.props, conditioned to exclude the analyzer project itself (and its own build, to avoid a self-reference cycle). Analyzer projects target netstandard2.0, so ReferenceOutputAssembly="false" + OutputItemType="Analyzer" is the correct incantation — the analyzer DLL is loaded into the compiler host, not linked. This makes OTOPCUA0001 run on every src/ and tests/ compilation.

Add to Directory.Build.props (after the existing ItemGroup):

<ItemGroup Condition="'$(MSBuildProjectName)' != 'ZB.MOM.WW.OtOpcUa.Analyzers'
                       and '$(MSBuildProjectName)' != 'ZB.MOM.WW.OtOpcUa.Analyzers.Tests'">
  <ProjectReference Include="$(MSBuildThisFileDirectory)src/Tooling/ZB.MOM.WW.OtOpcUa.Analyzers/ZB.MOM.WW.OtOpcUa.Analyzers.csproj"
                    OutputItemType="Analyzer"
                    ReferenceOutputAssembly="false"
                    PrivateAssets="all" />
</ItemGroup>

(The .Analyzers.Tests exclusion is belt-and-suspenders: it already ProjectReferences the analyzer directly; letting Directory.Build.props add a second analyzer-typed reference is harmless but noisy.)

Triage sub-step (required, do NOT skip): OTOPCUA0001 is a diagnostic. Its default severity must be checked in UnwrappedCapabilityCallAnalyzer — if it is Warning and a src project has TWE on, the wired-in analyzer can break the build on the first unwrapped call it finds. So the wiring must be done in this order:

Wire the ProjectReference (above).
dotnet build ZB.MOM.WW.OtOpcUa.slnx and capture every OTOPCUA0001 hit.
For each hit, decide: real violation → route the call through CapabilityInvoker; intentional (test doubles, the invoker's own internals) → add a scoped #pragma warning disable OTOPCUA0001 or a per-project <NoWarn>OTOPCUA0001</NoWarn> with a comment. The analyzer's own tests demonstrate the intended pattern.
Only after the tree is clean should any project promote OTOPCUA0001 to error (out of scope here).

Files to touch: Directory.Build.props (add ItemGroup); plus any .csproj needing NoWarn and any source files with genuine unwrapped calls surfaced by step 2.

Verify: dotnet build ZB.MOM.WW.OtOpcUa.slnx shows OTOPCUA0001 diagnostics on real code (proving it is live); a deliberately-unwrapped call in a scratch file trips it; the analyzer's 31 tests still pass. Confirm the build graph does not regress (dotnet build clean after triage).

Effort: M (wiring is S; the triage wave is the unknown — could be S or M depending on how many unwrapped calls exist). Risk: Medium — a Warning-severity analyzer on TWE projects can fail the build; the ordered triage above contains it. Blast radius is every compilation, so build once locally before pushing.

S-1 (High) — CI gates ~15% of the test matrix

Restatement: v2-ci.yml runs 5 of 47 unit-test projects; Client (388 tests), Analyzers (31), all driver and most Core suites never run in CI.

Verification: confirmed (5 enumerated, 47 total).

Root cause: hand-maintained matrix predates most projects and was never widened; new projects drift out of coverage silently.

Design: replace the hand-maintained unit-tests matrix with a single solution-wide leg that excludes only the env-gated tiers. The skip-gated *.IntegrationTests fixtures already tolerate unreachable endpoints (they Assert.Skip), so running them here is safe; but to keep the unit leg fast and deterministic, split responsibilities:

unit-tests job: one step — dotnet test ZB.MOM.WW.OtOpcUa.slnx --configuration Release --no-restore --filter "Category!=E2E&Category!=LiveIntegration" --logger "trx;LogFileName=unit.trx" after an explicit dotnet restore + dotnet build --no-restore (share build via --no-build where possible; see P-1). This runs all 47 projects in one invocation — no matrix to drift.

Rationale for the whole-solution filter over regenerating a matrix from the slnx: it is self-maintaining (new project = automatically covered), matches CLAUDE.md's own dotnet test ZB.MOM.WW.OtOpcUa.slnx guidance, and the trx output feeds S-2's skip-gate. The integration split (Host.IntegrationTests / OpcUaServer.IntegrationTests as a distinct matrix leg) stays, but is folded into the fail-on-skip work below (S-2).

Files to touch: .github/workflows/v2-ci.yml — replace the unit-tests job body.

Verify: push to a branch; the unit-tests job log shows Client.CLI/Shared/UI + Analyzers + driver + Core suites executing (not skipped); wall-clock is acceptable (pair with P-1 caching).

Effort: S. Risk: Medium — first full-solution CI run will surface latent failures / flakes that were never gated (esp. the fixed-sleep timing tests, S-4). That is the point, but budget time to stabilize. Blast radius: CI only.

S-2 (High) — "green" integration CI means "skipped"; skip is indistinguishable from pass

Restatement: the integration job runs fixture-less against 10.100.0.35 defaults → probe fails → Assert.Skip → job green. A real fixture outage cannot be told from a real pass.

Verification: confirmed — no services: in the integration job; fixtures hard-default to 10.100.0.35 (Modbus/AbServer/Snap7/OpcPlc fixtures + DriverTestConnectE2eTests).

Root cause: the probe-once skip pattern is correct for dev boxes, but CI has no signal that the tier ran nothing.

Design — two complementary changes:

Make skips visible/failing. Emit --logger "trx" from the integration leg and add a post-test step that parses the trx and fails the job when skipped-count exceeds a threshold (e.g. any integration test skipped when CI was supposed to have fixtures, or > N total). A small inline shell or a tiny scripts/ci/assert-not-all-skipped.ps1/.sh reading the <Results>/outcome="NotExecuted" counts. This is the minimal, always-applicable guard.
(Optional, higher value) Actually start the reachable fixtures as workflow services:. The Modbus sim and opc-plc images run on any hosted runner; add them as services: and set MODBUS_SIM_ENDPOINT=localhost:5020 / OPCUA_SIM_ENDPOINT=opc.tcp://localhost:50000 so those probes connect and the tests actually run. S7/AbCip/Galaxy fixtures have no hosted-runner image and stay legitimately skipped (guard #1 excludes them from the threshold, or scopes the threshold to the modbus/opc-plc subset).

Recommend shipping guard #1 first (cheap, universal), then #2 as a follow-up.

Files to touch: .github/workflows/v2-ci.yml (integration job: add trx logger, post-step, and optionally services: + endpoint env); optionally a new scripts/ci/assert-not-all-skipped.*.

Verify: deliberately point an endpoint at an unreachable host in a branch → integration job goes red (not green). With services up, the modbus/opc-plc tests show as passed, not skipped.

Effort: S (guard #1) / M (with services). Risk: Low-Medium — a too-tight threshold could red-flag legitimately-unavailable fixtures; scope the threshold to the images CI can actually run.

S-3 (Medium) — nightly E2E workflow is a permanent green no-op

Restatement: v2-e2e.yml filters Category=E2E, which matches zero tests; it boots the full docker-dev fleet for nothing and trains people to ignore a nightly green.

Verification: confirmed — zero Category=E2E in the tree; the workflow header admits it.

Root cause: the E2E project (tests/Server/ZB.MOM.WW.OtOpcUa.E2ETests) was scoped but never landed.

Design (pick one):

(a) Disable the schedule until the project exists — comment out the schedule: trigger, keep workflow_dispatch, and add a guard step that fails if Category=E2E matches zero tests (so the no-op can't silently return). Lowest effort; removes the false-green.
(b) Land a minimal E2E round-trip — a single [Trait("Category","E2E")] test that, against the booted docker-dev fleet, connects via Client.Shared/Client.CLI to opc.tcp://localhost:4840, browses to a known seeded node, reads a value, and asserts Good. The scripts/e2e/test-*.ps1 harnesses show exactly what to assert. This gives the nightly job real meaning and exercises the otherwise-uncovered Client stack end-to-end.

Recommend (a) now (removes the false signal immediately) with (b) as the tracked follow-on — option (a)'s zero-match guard also protects (b) from regressing back to a no-op.

Files to touch: .github/workflows/v2-e2e.yml; for (b) a new tests/Server/…E2ETests project + slnx entry.

Verify: (a) manual dispatch fails-fast on zero E2E tests; (b) the round-trip test passes against the fleet and fails if the seeded node is absent.

Effort: S (a) / M-L (b). Risk: Low.

S-7 (Low) — no `global.json` though the build depends on an exact SDK band

Restatement: v2-ci.yml:10-12 claims a repo-root global.json pins the SDK; none exists. Meanwhile Directory.Packages.props:44-49 pins Roslyn 5.0.0 to match SDK 10.0.105's compiler.

Verification: confirmed — no global.json; the CI comment is false.

Root cause: the pin comment describes intended state that was never created; the Roslyn pin note implicitly depends on SDK version stability the repo doesn't enforce.

Design: add a root global.json:

{
  "sdk": {
    "version": "10.0.105",
    "rollForward": "latestFeature"
  }
}

latestFeature allows patch/feature SDK rolls within the 10.0.1xx band but keeps the major/minor stable, so the Roslyn 5.0.0 pin's stated condition ("until the SDK rolls to 10.0.110+") becomes a deliberate, visible decision rather than a silent drift. Cross-check the exact installed SDK version before committing the version value. Update the Roslyn-pin comment in Directory.Packages.props to reference global.json as the coupling point.

Files to touch: new global.json; comment tweak in Directory.Packages.props.

Verify: dotnet --version in-repo resolves within the band; CI setup-dotnet honors it (the workflow comment becomes true). Build + tests unchanged.

Effort: S. Risk: Low — a too-strict version/rollForward could fail a runner lacking that exact SDK; latestFeature mitigates.

TIER B — Repo hygiene

U-5 (Medium) — orphaned proprietary AVEVA DLLs tracked in `lib/`

Restatement: 7 committed vendor binaries in lib/, referenced by nothing — a redistribution/licence risk in every clone.

Verification: confirmed — git ls-files lib/ lists all 7; zero HintPath hits repo-wide.

Root cause: leftovers from the retired in-process MXAccess / Wonderware sidecar era; the bitness/COM story now lives entirely in the mxaccessgw repo (per CLAUDE.md).

Design: git rm -r lib/. Before deleting, re-confirm nothing references them:

grep -rn "aahClient\|ArchestrA\|Historian.CBE\|Historian.DPAPI\|HintPath.*lib" \
  --include='*.csproj' --include='*.props' --include='*.targets' .

(expected: no hits outside the review docs). Optionally add lib/ to .gitignore to prevent re-introduction, and note in the commit that the DLLs remain retrievable from history / the AVEVA SDK if ever needed. Purging them from history is a separate, heavier decision (BFG/filter-repo) — out of scope; the immediate licence-hygiene win is removing them from the tip.

Files to touch: delete lib/* (7 files); optional .gitignore entry.

Verify: git status clean; dotnet build ZB.MOM.WW.OtOpcUa.slnx still succeeds (proves nothing linked them); git ls-files lib/ empty.

Effort: S. Risk: Low — verified zero references; blast radius nil.

U-4 (Medium) — root planning-file sprawl contradicts its own "never stage" rule

Restatement: pending.md, current.md, looseends.md, stillpending.md, HISTORIAN-GATEWAY-INTEGRATION-ISSUES.md are committed at root; pending.md:3 itself says never stage pending.md/current.md. Content is stale snapshots now contradicting docs/ + CLAUDE.md.

Verification: confirmed — all 5 tracked; the hard-rule text present.

Root cause: working-notes files that were staged despite the rule, and never cleaned up as the backlog shipped (~95% per the memory index).

Design — make the file's own rule true:

pending.md + current.md — untrack but keep locally (they're active working notes): git rm --cached pending.md current.md, then add both to .gitignore. This exactly satisfies the "never stage" rule.
looseends.md, stillpending.md, HISTORIAN-GATEWAY-INTEGRATION-ISSUES.md — these are point-in-time snapshots of shipped work. Preferred: move to docs/plans/ with dated names (the established convention), e.g. docs/plans/2026-05-18-looseends.md, so the history is preserved as an archived plan rather than a live-looking root file. Acceptable alternative: git rm them outright since the memory index records the backlog as shipped and docs/+CLAUDE.md now carry the authoritative state. Recommend archive-move for stillpending.md/HISTORIAN-GATEWAY-… (they have referenced history) and delete for looseends.md if fully superseded — confirm with a quick read before choosing per-file.

Files to touch: .gitignore (+2 entries); git rm --cached pending/current; move-or-delete the 3 snapshots; docs/plans/ if archiving.

Verify: git ls-files | grep -E '^(pending|current|looseends|stillpending|HISTORIAN)' returns only any intentionally-archived docs/plans/* paths; git status shows pending.md/current.md now untracked (ignored). pending.md's hard-rule is now consistent with reality.

Effort: S. Risk: Low — pure VCS hygiene; keep local copies (--cached) so no working data is lost.

U-6 (Low) — secrets hygiene + retired-dir husks

Restatement: sql_login.txt is NOT committed (gitignored) but is a plaintext credential on Desktop; retired Driver.Historian.Wonderware* dirs linger on disk as bin/obj husks.

Verification: confirmed — sql_login.txt .gitignore:47, git log empty; the 8 Driver.Historian.Wonderware* dirs under src/+tests/ are untracked (only docs/ stub + 3 code-reviews/…/findings.md are tracked residue).

Root cause: local dev debris; the credential file is a convenience left in the repo dir.

Design:

sql_login.txt — no tracking action needed (already ignored). Recommend moving the credential to dotnet user-secrets or an env file outside the repo directory, and audit export-clean-copy.bat at root to confirm it excludes sql_login.txt (and lib/, pki, planning files) from any export bundle. Low-priority, local-only.
Wonderware husks — delete the untracked src/Drivers/…Wonderware* and tests/Drivers/…Wonderware* directories from disk (rm -rf, they're pure bin/obj). Decide separately whether to also remove the 3 tracked code-reviews/Driver.Historian.Wonderware*/ findings and keep docs/drivers/Historian.Wonderware.md as the intentional retired stub (the doc is deliberately retained per CLAUDE.md; leave it).

Files to touch: none tracked for the credential; rm -rf the untracked husks; audit export-clean-copy.bat.

Verify: find src tests -type d -iname '*Wonderware*' returns nothing; git status unchanged by the husk deletion (they were untracked); export-clean-copy.bat excludes the sensitive files.

Effort: S. Risk: Low.

C-5 (Low) — no `.editorconfig`; StyleGuide.md mis-titled for a sister repo

Restatement: no mechanical code-style enforcement; StyleGuide.md:3 still says "for all ScadaBridge documentation" (copy-paste from the sibling repo).

Verification: confirmed — no .editorconfig; StyleGuide.md:3 wording present.

Root cause: style held by review culture only; StyleGuide copied from ScadaBridge without re-titling.

Design — two independent, small changes:

StyleGuide fix (trivial, do now): replace "ScadaBridge" with "OtOpcUa" in StyleGuide.md:3 (and scan the file for other ScadaBridge leftovers).
.editorconfig (optional): add a root .editorconfig codifying the already-consistent conventions — file-scoped namespaces, 4-space indent, _camelCase private fields, var usage — set as suggestion/warning (not error) to avoid a big-bang reformat. This is documentation-of- convention, not a reformatting mandate; a dotnet format CI step is a further follow-on (U-7), deliberately deferred so it doesn't churn the tree.

Files to touch: StyleGuide.md; optional new .editorconfig.

Verify: StyleGuide title reads OtOpcUa; .editorconfig doesn't flip existing warnings to errors (build unchanged).

Effort: S. Risk: Low (keep severities non-error).

TIER C — Documentation

U-3 (Medium) — `docs/Client.CLI.md` documents 8 of 14 commands

Restatement: ack/acknowledge, confirm, shelve, enable, disable have no doc sections, though all ship and CLAUDE.md points operators at this doc.

Verification: confirmed — confirm/shelve/disable = 0 mentions; ack/enable only substring hits; command files exist for all 5.

Root cause: the inbound operator ack/shelve feature shipped its runtime + CLI but the CLI doc was never extended.

Design: add five command sections to docs/Client.CLI.md mirroring the existing section shape (synopsis, options, example invocation, expected output). Source the exact options/args from the command classes: AcknowledgeCommand.cs, ConfirmCommand.cs, ShelveCommand.cs, EnableCommand.cs, DisableCommand.cs (and their shared CommandBase connection options). Group them under an "Alarm operator commands" heading since they are the operator-facing ack/shelve workflow. Cross-check against docs/ScriptedAlarms.md §"Inbound operator ack/shelve" for terminology consistency.

Files to touch: docs/Client.CLI.md.

Verify: each of the 5 commands has a section; a -h/--help run of each command matches the documented options. Optionally add a CI docs-coverage check (U-7 follow-on) asserting every Commands/*Command.cs has a doc section.

Effort: S. Risk: Low.

TIER D — Engineering debt (as capacity allows)

These are real but lower-leverage than restoring CI meaning. Batched with condensed treatment.

C-2 (Medium) — TWE never reached the Client/Tooling slice

Add <TreatWarningsAsErrors>true</TreatWarningsAsErrors> to Client.CLI.csproj, Client.Shared.csproj, Client.UI.csproj, Analyzers.csproj (v2-era code; the legacy xUnit1051 blocker doesn't apply). Do this AFTER C-1's triage — with the analyzer wired in, a Warning-level OTOPCUA0001 would now fail these builds, so sequence C-1 triage → C-2. Verify each builds clean with TWE on. Effort S; Risk Low (build once locally first). Burn down the 38 test-project TWE gaps per module as a tracked follow-on.

P-1 (Medium) — CI does 7 uncached restore+build passes

Add actions/cache for ~/.nuget/packages keyed on a hash of Directory.Packages.props + NuGet.config; do one explicit restore, then build --no-restore, then test --no-build in the unit leg (dovetails with S-1's single-leg design). Effort S; Risk Low; biggest CI wall-clock win.

U-2 (Medium) — add `Host.Tests` (the one real coverage hole)

Create tests/Server/ZB.MOM.WW.OtOpcUa.Host.Tests asserting DI composition — e.g. "when ServerHistorian:Enabled, IHistorianProvisioning resolves to GatewayTagProvisioner and the applier receives it" — the exact class of dormant-wiring bug (PR #423, DeferredAddressSpaceSink) that unit tests would have caught. Add to slnx so S-1's whole-solution CI picks it up. Effort M; Risk Low.

S-5 (Medium) — Client.Shared subscription lock-discipline gaps

In OpcUaClientService.cs: (a) move the _dataSubscription null-check/create inside an async gate mirroring _alarmSubscribeSemaphore (line ~262); (b) take _subscriptionLock in RunFailoverAsync when nulling _dataSubscription/_alarmSubscription (lines ~695-696); (c) route UnsubscribeAlarmsAsync (line ~335) through _alarmSubscribeSemaphore; (d) observe exceptions from the fire-and-forget keep-alive failover. Near-unhittable for the session-per-command CLI; real for the long-lived Client.UI service. Effort M; Risk Medium (concurrency code — add targeted tests). Do before Client.UI grows multi-threaded usage.

S-6 (Medium) — global `Log.Logger` swap per CLI command

CommandBase.ConfigureLogging() (:120-134) does Log.CloseAndFlush() + replaces static Log.Logger per command — a race under xunit.v3's parallel collections and hostile to embedding. Give each command an instance ILogger from a per-execution LoggerConfiguration.CreateLogger(). The existing LoggerLifecycleTests polices this. Effort M; Risk Low-Medium (touches every command's logging).

S-4 (Medium) — fixed-sleep timing tests

Replace start-up await Task.Delay(100) sleeps in Client.CLI.Tests (SubscribeCommandTests, AlarmsCommandTests, EventHandlerLifecycleTests) with a readiness signal — a TaskCompletionSource completed on first SubscribeAsync exposed by FakeOpcUaClientService. Currently masked because CI never runs them (S-1); will flake once S-1 lands, so pair with S-1. Effort M; Risk Low.

C-4 (Low) — finish xunit.v3 migration

Migrate the 3 holdouts (AdminUI.Tests, ControlPlane.Tests, Runtime.Tests) off xunit 2.9.2, then drop the xunit/xunit.runner.visualstudio 2.x pins from Directory.Packages.props:108-109. Optionally relocate GatewayLiveIntegrationTests to a *.IntegrationTests sibling. Effort M; Risk Low.

C-6 (Low) — csproj boilerplate duplicates Directory.Build.props

Strip redundant TargetFramework/Nullable/ImplicitUsings from the ~70 csprojs on next sweep so an SDK bump touches one file. Effort M (mechanical); Risk Low. Low priority.

S-8 (Low) — first-caller interval wins for shared data subscription

Document the shared-publish-interval behavior in docs/Client.CLI.md/docs/Client.UI.md, or create per-interval subscriptions if UI users mix cadences. Effort S; Risk Low.

U-8 (Low) — CLI security posture is dev-tool-grade

Document that AutoAcceptCertificates = true (CommandBase.cs:91) means --security signandencrypt encrypts but does not authenticate the server (MITM-able), and that -P is process-visible. Optionally add a --strict-certs opt-in. Separately, note the Galaxy-specific alarm attribute names baked into the generic OpcUaClientService.cs:851-871 as a documented layering compromise. Effort S; Risk Low.

P-2 (Low) — recursive browse N+1

BrowseAsync issues an extra HasChildrenAsync per Object child (:230-231) and SubscribeCommand.CollectVariablesAsync walks serially. Batch/bulk-read references only if subscribe -r start-up becomes a soak-test bottleneck. Effort M; Risk Low. Defer.

U-7 (Low) — missing CI stages inventory

Tracking item: analyzer enforcement (C-1), all-suite coverage (S-1), dotnet format/style step, explicit failing NuGet-audit step, running scripts/check-code-reviews-readme.ps1 in CI, and a docs freshness/link check. Also the ci/ab-server.lock.json references a non-existent "GitHub Actions step" in docs/v2/test-data-sources.md — either land the AB fixture download step or fix the doc. Address incrementally as the above land.

No-action (verified positive / accurate — recorded only)

P-3 — probe-once collection-fixture pattern is well-engineered; the issue is what CI does with it (S-2), not the fixtures.
P-4 — CLI subscription output uses a channel-serialised writer; allocation-sane. A bounded DropOldest channel is a cosmetic future option.
C-3 — CPM discipline exemplary; the NuGetAuditSuppress carve-out and the two transitive CVE pins are still valid and accurately scoped. Keep the removal reminders alive.
U-1 — Client.UI is a genuine, adequately-tested product (not vestigial); "maturing", not "underdeveloped". Its only gap is the same CI/E2E gap (S-1/S-3).

Suggested execution sequence

C-1 (wire analyzer) + its triage — establishes the enforcement mechanism; must precede C-2.
S-1 + P-1 (whole-solution CI + caching) in one workflow edit — biggest coverage win.
S-2 (fail-on-skip guard, then optional services:) — closes the false-green.
S-7 (global.json) + S-3 (disable E2E schedule / zero-match guard) — small CI truth fixes.
U-5, U-4, U-6, C-5 (hygiene batch) — one commit; verified zero-reference deletions.
U-3 (Client.CLI doc) — five sections.
C-2 (TWE, after C-1 triage), then Tier-D items as capacity allows, pairing S-4 with S-1.

Each of steps 1-4 must be validated by an actual CI run on a branch before merge — per the report's own theme #4, green CI here has been meaningless, so the fixes must be observed to change the badge's behavior (a broken build/deliberate skip must now go red).

29 KiB Raw Blame History