dohertj2/scadaproj
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Baseline code review of the six ZB.MOM.WW.* shared libraries

Browse Source 
All six libraries reviewed at commit 5f75cd4 against their components/ specs,
following code-reviews/REVIEW-PROCESS.md. 35 findings (0 Critical, 1 High,
9 Medium, 25 Low); none block adoption.

- Auth      0/0/3/3  (security core sound; startup-validation + key-verify contract gaps)
- Telemetry 0/1/2/5  (HIGH Telemetry-001: redactor 'remove' is a no-op -> secrets reach sinks)
- Health    0/0/2/4  (Akka checks throw instead of Degraded when cluster not yet up)
- Theme     0/0/1/5  (undocumented Bootstrap-collapse JS dep; token/CSS hygiene)
- Audit     0/0/1/4  (composite re-throws OCE vs never-throw writer contract)
- Configuration 0/0/0/4 (DI idempotency, port-parse strictness, packaging)

Cross-cutting: XML docs authored but GenerateDocumentationFile unset -> docs
not shipped in any nupkg (Auth/Health/Telemetry/Configuration/Audit).

README.md regenerated from the per-library findings; regen-readme.py --check passes.
This commit is contained in:
Joseph Doherty
2026-06-01 11:08:12 -04:00
parent 5f75cd4dab
commit 26ba1c7215
7 changed files with 1422 additions and 119 deletions
Download Patch File Download Diff File Expand all files Collapse all files
code-reviews/Configuration/findings.md
+167 -17
View File
@@ -6,31 +6,181 @@
| Packages | `ZB.MOM.WW.Configuration` |
| Component spec | `components/configuration/spec/SPEC.md` |
| Shared contract | `components/configuration/shared-contract/ZB.MOM.WW.Configuration.md` |
| Status | Not yet reviewed |
| Last reviewed | — |
| Reviewer | — |
| Commit reviewed | — |
| Open findings | 0 |
| Status | Reviewed |
| Last reviewed | 2026-06-01 |
| Reviewer | Claude (automated baseline) |
| Commit reviewed | `5f75cd4` |
| Open findings | 4 |
## Summary
_Not yet reviewed._
The library is small (five source files, ~230 LOC) and in good health. Its core promise —
**accumulate every failure, never short-circuit** — holds across all three entry points
(`ValidationBuilder`, `OptionsValidatorBase`, `ConfigPreflight`): the override never returns
early, each primitive appends to a `List<string>` and returns `this`, and the `Success`/`Fail`
decision is taken only after every rule has run. The boundary cases that matter are correct:
`Port` rejects 0 and 65536 and accepts 1/65535; `HostPort` correctly rejects bare hosts, empty
ports, out-of-range ports, and (deliberately) unbracketed IPv6; `Required` treats null / empty /
whitespace alike; `OneOf` is case-insensitive and treats null as a failure (documented);
`MinCount` treats null as zero. Null-guarding is consistent (`ArgumentNullException`/
`ArgumentException.ThrowIfNullOrWhiteSpace` on every public entry point). The
`ConfigPreflight.ThrowIfInvalid()` envelope is **byte-compatible** with the ScadaBridge
`StartupValidator` format the SPEC pins (`"Configuration validation failed:\n - <field> <reason>"`,
`\n`-joined), and the validator created by `OptionsValidatorBase` is stateless (fresh
`ValidationBuilder` per call), so the singleton-registration requirement in SPEC §3 is honoured.
Dependency footprint is minimal and exactly as documented (four `Microsoft.Extensions.*`
abstractions, no ASP.NET Core). The four findings are all **Low**: a non-idempotent DI
registration (`AddSingleton` vs `TryAddEnumerable`), a small wording inconsistency in the raw-port
check, over-permissive integer port parsing, and the XML-doc-not-shipped packaging gap (which is
fleet-wide, not specific to this library). Test coverage (27 tests across the four public surfaces)
is solid for the happy and primary-failure paths; a few edge cases noted below are untested.
## Checklist coverage
| # | Category | Examined | Notes |
|---|----------|----------|-------|
| 1 | Correctness & logic bugs | ☐ | |
| 2 | Public API surface & compatibility | ☐ | |
| 3 | Concurrency & thread safety | ☐ | |
| 4 | Error handling & resilience | ☐ | |
| 5 | Security & secret handling | ☐ | |
| 6 | Performance & resource management | ☐ | |
| 7 | Spec & shared-contract adherence | ☐ | |
| 8 | Packaging, dependencies & project layout | ☐ | |
| 9 | Testing coverage | ☐ | |
| 10 | Documentation & XML docs | ☐ | |
| 1 | Correctness & logic bugs | ☑ | Accumulation never short-circuits; port/host:port/duration/min-count boundaries correct. Port parsing over-permissive (003). |
| 2 | Public API surface & compatibility | ☑ | Surface matches the shared contract exactly; nullability annotations correct; `sealed`/`abstract` deliberate; internal `Checks` does not leak. No issues. |
| 3 | Concurrency & thread safety | ☑ | Validator is stateless (fresh builder per `Validate`); `ValidationBuilder`/`ConfigPreflight` are per-call locals. Safe as a singleton. No issues. |
| 4 | Error handling & resilience | ☑ | Guard clauses on every public entry; correct exception types (`OptionsValidationException` via `ValidateOnStart`, `InvalidOperationException` from `ConfigPreflight`); no swallowed exceptions. No issues. |
| 5 | Security & secret handling | ☑ | No secrets handled; failure messages echo config *values* (port/host/role), which are non-sensitive by design. No issues. |
| 6 | Performance & resource management | ☑ | Startup-only, no hot path, nothing disposable, no async. No issues. |
| 7 | Spec & shared-contract adherence | ☑ | `ConfigPreflight` envelope byte-compatible with `StartupValidator`; primitives match §2 table. Minor wording inconsistency in `PortValue` (002). |
| 8 | Packaging, dependencies & project layout | ☑ | Single package, minimal closure, central versions, `.gitignore` present, no tracked build output. XML docs / README not packaged (004). |
| 9 | Testing coverage | ☑ | 27 tests cover the four public surfaces + accumulation + byte-format. Untested: `AddValidatedOptions` null guards, double-registration, exact wording strings. |
| 10 | Documentation & XML docs | ☑ | Public XML docs present and accurate on every type/member. Not emitted into the nupkg (004). |
## Findings
_No findings recorded yet._
### Configuration-001 — `AddValidatedOptions` uses `AddSingleton`, so a double call registers (and runs) the validator twice
| | |
|--|--|
| Severity | Low |
| Category | Correctness & logic bugs |
| Status | Open |
| Location | `ZB.MOM.WW.Configuration/src/ZB.MOM.WW.Configuration/ServiceCollectionExtensions.cs:36` |
**Description**
`AddValidatedOptions` registers the validator with
`services.AddSingleton<IValidateOptions<TOptions>, TValidator>()`. `IValidateOptions<TOptions>`
is a multi-registration (enumerable) service — the options pipeline resolves *all* registered
`IValidateOptions<TOptions>` and runs each. Because `AddSingleton` appends unconditionally, calling
`AddValidatedOptions<TOptions, TValidator>` twice for the same `TOptions`/`TValidator` pair (e.g.
two modules both guarding the same section, or an accidental duplicate during refactoring)
registers the validator twice, so it executes twice and every accumulated failure is reported
twice in the resulting `OptionsValidationException`. The framework-idiomatic registration for
`IValidateOptions<T>` is `TryAddEnumerable(ServiceDescriptor.Singleton<IValidateOptions<TOptions>,
TValidator>())`, which is idempotent. Impact is limited (consumers normally call once per section,
and duplicate messages are cosmetic, not a correctness break of the accumulate-all contract), hence
Low — but the deviation from the .NET idiom is real and easy to fix. The SPEC §3 wording ("registers
the validator as a singleton") is satisfied either way.
**Recommendation**
Replace `services.AddSingleton<IValidateOptions<TOptions>, TValidator>()` with
`services.TryAddEnumerable(ServiceDescriptor.Singleton<IValidateOptions<TOptions>, TValidator>())`
(via `Microsoft.Extensions.DependencyInjection.Extensions`). Add a test asserting that calling
`AddValidatedOptions` twice yields a single set of failure messages.
**Resolution**
_Unresolved._
### Configuration-002 — `Checks.PortValue` quotes the raw value on a parse failure but not on a range failure
| | |
|--|--|
| Severity | Low |
| Category | Spec & shared-contract adherence |
| Status | Open |
| Location | `ZB.MOM.WW.Configuration/src/ZB.MOM.WW.Configuration/Checks.cs:21` |
**Description**
`PortValue` produces two different message shapes for the same field depending on *why* the port
is invalid. When the raw string parses but is out of range (e.g. `"0"` or `"70000"`), it delegates
to `Checks.Port(int, field)`, which renders `"<field> must be between 1 and 65535 (was 0)"` —
**unquoted**. When the raw string does not parse (e.g. `"notaport"`, `null`), it renders
`"<field> must be between 1 and 65535 (was 'notaport')"` — **quoted**. So two failures of the same
rule, from the same raw-config caller, read with inconsistent quoting. The shared contract
(`shared-contract/ZB.MOM.WW.Configuration.md`, "Internal `Checks` seam") states `Checks` is "the
single source of failure wording" so "a port failure reads the same whether it came from a bound
options object or a raw config key" — this inconsistency is in mild tension with that claim. It is
cosmetic (no consumer parses the text) and the overlapping integer case *does* match the bound-options
wording, so it is Low.
**Recommendation**
Make the two branches use a consistent quoting convention for the offending value — e.g. have the
range-failure branch also quote (`(was '0')`), or have the parse-failure branch route through a
shared formatter. Lock the exact strings down with a wording assertion test.
**Resolution**
_Unresolved._
### Configuration-003 — Port parsing accepts leading sign and surrounding whitespace and is culture-sensitive
| | |
|--|--|
| Severity | Low |
| Category | Correctness & logic bugs |
| Status | Open |
| Location | `ZB.MOM.WW.Configuration/src/ZB.MOM.WW.Configuration/Checks.cs:22`, `ZB.MOM.WW.Configuration/src/ZB.MOM.WW.Configuration/Checks.cs:36` |
**Description**
Both `PortValue` (`int.TryParse(raw, out var port)`) and `HostPort`
(`int.TryParse(value[(idx + 1)..], out var port)`) use the default `int.TryParse` overload, which
applies `NumberStyles.Integer` (`AllowLeadingWhite | AllowTrailingWhite | AllowLeadingSign`) and the
current culture. As a result strings the documentation describes as "integer TCP port" are accepted
more loosely than intended: `"+5000"`, `" 5000 "`, and `"host: 5000"` (space after the colon) all
parse and pass, and parsing is locale-dependent. A leading `-` parses to a negative number that the
subsequent range check rejects (so `"-1"` is correctly rejected), but the whitespace/leading-`+`
cases silently pass. This is a robustness nuance rather than a security or correctness break — a port
that survives the loose parse is still in `1..65535` — so it is Low.
**Recommendation**
Parse with an explicit, culture-invariant, strict style, e.g.
`int.TryParse(raw, NumberStyles.None, CultureInfo.InvariantCulture, out var port)` (rejects sign and
whitespace), in both `PortValue` and `HostPort`. Add `Theory` cases for `"+5000"`, `" 5000 "`, and a
space-after-colon endpoint to pin the behaviour.
**Resolution**
_Unresolved._
### Configuration-004 — XML documentation and README are not packaged into the nupkg
| | |
|--|--|
| Severity | Low |
| Category | Documentation & XML docs |
| Status | Open |
| Location | `ZB.MOM.WW.Configuration/src/ZB.MOM.WW.Configuration/ZB.MOM.WW.Configuration.csproj:1`, `ZB.MOM.WW.Configuration/Directory.Build.props:1` |
**Description**
Every public type and member carries accurate XML doc comments, but neither the project nor
`Directory.Build.props` sets `<GenerateDocumentationFile>true</GenerateDocumentationFile>`, so no
`.xml` doc file is produced or included in the `dotnet pack` output. Consumers of the package
therefore get **no IntelliSense documentation** for `OptionsValidatorBase`, `ValidationBuilder`,
`AddValidatedOptions`, or `ConfigPreflight`, despite the docs existing in source. The project also
does not set `PackageReadmeFile`, so the README is not embedded in the package for display on the
NuGet feed. Category 10 of the review process explicitly notes "these are libraries — public docs
matter," so the gap is worth recording. It is fleet-wide (the sibling `ZB.MOM.WW.Audit` library has
the same omission), not a Configuration-specific regression, hence Low.
**Recommendation**
Add `<GenerateDocumentationFile>true</GenerateDocumentationFile>` (ideally in
`Directory.Build.props` so the whole family inherits it) and consider `<PackageReadmeFile>README.md</PackageReadmeFile>`
plus packing the README. Treat as a shared-infra change across the six libraries rather than a
one-off.
**Resolution**
_Unresolved._