From ae0ccc9a3a22d9891a0d50e79caa07b339668f30 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Mon, 1 Jun 2026 11:22:37 -0400 Subject: [PATCH] Mark all baseline code-review findings resolved All 35 findings fixed in 544a6dd and marked Status: Resolved with resolution notes. README regenerated: 0 pending / 35 total across 6 libraries. --- code-reviews/Audit/findings.md | 22 ++++---- code-reviews/Auth/findings.md | 26 +++++----- code-reviews/Configuration/findings.md | 18 +++---- code-reviews/Health/findings.md | 26 +++++----- code-reviews/README.md | 72 ++++++-------------------- code-reviews/Telemetry/findings.md | 53 +++++++++++++------ code-reviews/Theme/findings.md | 26 +++++----- 7 files changed, 112 insertions(+), 131 deletions(-) diff --git a/code-reviews/Audit/findings.md b/code-reviews/Audit/findings.md index adbb946..bb91f23 100644 --- a/code-reviews/Audit/findings.md +++ b/code-reviews/Audit/findings.md @@ -10,7 +10,7 @@ | Last reviewed | 2026-06-01 | | Reviewer | Claude (automated baseline) | | Commit reviewed | `5f75cd4` | -| Open findings | 5 | +| Open findings | 0 | ## Summary @@ -54,7 +54,7 @@ family-wide baseline pattern), and a couple of missing edge-case tests (Audit-00 |--|--| | Severity | Medium | | Category | Error handling & resilience | -| Status | Open | +| Status | Resolved | | Location | `ZB.MOM.WW.Audit/src/ZB.MOM.WW.Audit/CompositeAuditWriter.cs:24` | **Description** @@ -92,7 +92,7 @@ surface. Option (a) is the lower-blast-radius choice and matches the seam's stat **Resolution** -_Unresolved._ +Resolved in `544a6dd`, 2026-06-01 — `CompositeAuditWriter.WriteAsync` now swallows `OperationCanceledException` like any other writer failure (single bare `catch`), so cancellation never surfaces to the caller; XML doc and the cancellation test updated to assert non-propagation. ### Audit-002 — `TruncatingAuditRedactor` over-redaction is partial: the catch path scrubs only `DetailsJson`, leaving `Target` unredacted @@ -100,7 +100,7 @@ _Unresolved._ |--|--| | Severity | Low | | Category | Error handling & resilience | -| Status | Open | +| Status | Resolved | | Location | `ZB.MOM.WW.Audit/src/ZB.MOM.WW.Audit/TruncatingAuditRedactor.cs:27-31`, `:34-40` | **Description** @@ -128,7 +128,7 @@ unexpected failures rather than a predictable negative-length misconfiguration. **Resolution** -_Unresolved._ +Resolved in `544a6dd`, 2026-06-01 — the over-redact catch now returns `rawEvent with { DetailsJson = null, Target = null }` (strictly safer), and `Truncate` clamps a negative `max` to 0 so a negative-length misconfiguration fails safe instead of throwing; new tests pin both behaviours. ### Audit-003 — `TruncatingAuditRedactorOptions` is a mutable class, not the immutable "options record" the contract describes @@ -136,7 +136,7 @@ _Unresolved._ |--|--| | Severity | Low | | Category | Public API surface & compatibility | -| Status | Open | +| Status | Resolved | | Location | `ZB.MOM.WW.Audit/src/ZB.MOM.WW.Audit/TruncatingAuditRedactorOptions.cs:4-12` | **Description** @@ -160,7 +160,7 @@ readonly fields at construction so post-construction mutation cannot affect it. **Resolution** -_Unresolved._ +Resolved in `544a6dd`, 2026-06-01 — `TruncatingAuditRedactorOptions` is now a `sealed record` with `init`-only properties, matching the contract's "options record" and removing the post-construction mutation footgun on the singleton redactor. ### Audit-004 — XML documentation is authored but not emitted, so IntelliSense docs do not ship to consumers @@ -168,7 +168,7 @@ _Unresolved._ |--|--| | Severity | Low | | Category | Documentation & XML docs | -| Status | Open | +| Status | Resolved | | Location | `ZB.MOM.WW.Audit/src/ZB.MOM.WW.Audit/ZB.MOM.WW.Audit.csproj:1-18`, `ZB.MOM.WW.Audit/Directory.Build.props:1-10` | **Description** @@ -189,7 +189,7 @@ documented, no CS1591 "missing XML comment" warnings should appear. **Resolution** -_Unresolved._ +Resolved in `544a6dd`, 2026-06-01 — set `true` in the packable `ZB.MOM.WW.Audit.csproj`; `ZB.MOM.WW.Audit.xml` now builds with zero CS1591 warnings and packs into the nupkg under `lib/net10.0/`. ### Audit-005 — Missing edge-case tests for the redactor never-throw/over-redact contract and composite null/empty handling @@ -197,7 +197,7 @@ _Unresolved._ |--|--| | Severity | Low | | Category | Testing coverage | -| Status | Open | +| Status | Resolved | | Location | `ZB.MOM.WW.Audit/tests/ZB.MOM.WW.Audit.Tests/TruncatingAuditRedactorTests.cs`, `.../CompositeAuditWriterTests.cs` | **Description** @@ -224,4 +224,4 @@ guarded — pair with the chosen fix for the null case). **Resolution** -_Unresolved._ +Resolved in `544a6dd`, 2026-06-01 — added four tests: redactor over-redact catch branch scrubs both fields, negative-max clamp, and `CompositeAuditWriter` empty-list no-op + null-writer-entry swallow (19 → 23 tests). diff --git a/code-reviews/Auth/findings.md b/code-reviews/Auth/findings.md index 0ad35cb..da3ba5c 100644 --- a/code-reviews/Auth/findings.md +++ b/code-reviews/Auth/findings.md @@ -10,7 +10,7 @@ | Last reviewed | 2026-06-01 | | Reviewer | Claude (automated baseline) | | Commit reviewed | `5f75cd4` | -| Open findings | 6 | +| Open findings | 0 | ## Summary @@ -52,7 +52,7 @@ hashing algorithm and the AspNetCore surface. The library is **not yet adopted** |--|--| | Severity | Medium | | Category | Error handling & resilience | -| Status | Open | +| Status | Resolved | | Location | `ZB.MOM.WW.Auth/src/ZB.MOM.WW.Auth.AspNetCore/ServiceCollectionExtensions.cs:40` | **Description** @@ -89,7 +89,7 @@ already run.) Add a test asserting that building + starting the host fails for a **Resolution** -_Unresolved._ +Resolved in `544a6dd`, 2026-06-01 — `AddZbLdapAuth` now binds via `AddOptions().Bind(...).ValidateOnStart()` so the validator runs at host start, not on first login (test: `ServiceCollectionExtensionsTests.AddZbLdapAuth_StartingHost_FailsForInsecureConfig`, plus a `...SucceedsForSecureConfig` companion). ### Auth-002 — A failed `MarkUsedAsync` write turns a valid API key into a thrown exception @@ -97,7 +97,7 @@ _Unresolved._ |--|--| | Severity | Medium | | Category | Error handling & resilience | -| Status | Open | +| Status | Resolved | | Location | `ZB.MOM.WW.Auth/src/ZB.MOM.WW.Auth.ApiKeys/ApiKeyVerifier.cs:66` | **Description** @@ -121,7 +121,7 @@ throws and assert the result is still `Succeeded == true`. **Resolution** -_Unresolved._ +Resolved in `544a6dd`, 2026-06-01 — `VerifyAsync` now wraps the best-effort `MarkUsedAsync` write in try/catch, re-throwing only `OperationCanceledException` and swallowing all other failures so a valid key still authenticates (tests: `ApiKeyVerifierTests.VerifyAsync_ValidKey_MarkUsedThrows_StillSucceeds` and `..._MarkUsedThrowsOperationCanceled_Propagates`). ### Auth-003 — Corrupt `scopes`/`constraints` column throws `JsonException` through the verifier @@ -129,7 +129,7 @@ _Unresolved._ |--|--| | Severity | Medium | | Category | Correctness & logic bugs | -| Status | Open | +| Status | Resolved | | Location | `ZB.MOM.WW.Auth/src/ZB.MOM.WW.Auth.ApiKeys/Sqlite/ScopeSerializer.cs:31`, `ZB.MOM.WW.Auth/src/ZB.MOM.WW.Auth.ApiKeys/Sqlite/SqliteApiKeyStore.cs:71` | **Description** @@ -153,7 +153,7 @@ documented exception) rather than an unhandled `JsonException`. **Resolution** -_Unresolved._ +Resolved in `544a6dd`, 2026-06-01 — `ScopeSerializer.Deserialize` now catches `JsonException` and fails closed to an empty scope set (policy (a)), so a corrupt `scopes`/`constraints` column degrades to a zero-scope identity instead of throwing through the store and verifier (tests: `SqliteApiKeyStoreTests.ScopeSerializer_DeserializeMalformed_ReturnsEmptySet_DoesNotThrow` (Theory) and `..._FindByKeyId_CorruptScopesColumn_ReturnsRecordWithEmptyScopes_DoesNotThrow`). ### Auth-004 — README misstates the hashing algorithm and the AspNetCore public surface @@ -161,7 +161,7 @@ _Unresolved._ |--|--| | Severity | Low | | Category | Documentation & XML docs | -| Status | Open | +| Status | Resolved | | Location | `ZB.MOM.WW.Auth/README.md:13`, `ZB.MOM.WW.Auth/README.md:14` | **Description** @@ -190,7 +190,7 @@ DI-method names with the shipped names. **Resolution** -_Unresolved._ +Resolved in `544a6dd`, 2026-06-01 — README package table corrected: "PBKDF2" → "pepper-keyed HMAC-SHA256" and the AspNetCore row rewritten to the real surface (`AddZbLdapAuth`, `ZbCookieDefaults.Apply`, `ZbClaimTypes`, LDAP-only, no API-key/cookie-middleware wiring), with the stale `ApiKeys` dependency dropped from that row to match the `.csproj` (test: doc-only). ### Auth-005 — `CreateKeyAsync` persists `KeyPrefix` as `prefix_keyId`, inconsistent with the read path @@ -198,7 +198,7 @@ _Unresolved._ |--|--| | Severity | Low | | Category | Correctness & logic bugs | -| Status | Open | +| Status | Resolved | | Location | `ZB.MOM.WW.Auth/src/ZB.MOM.WW.Auth.ApiKeys/Admin/ApiKeyAdminCommands.cs:104` | **Description** @@ -220,7 +220,7 @@ column, or document precisely what `KeyPrefix` is meant to contain on the `ApiKe **Resolution** -_Unresolved._ +Resolved in `544a6dd`, 2026-06-01 — `CreateKeyAsync` now persists `KeyPrefix = _options.TokenPrefix` (the bare prefix) instead of `prefix_keyId`, aligning the write path with the read/test paths (test: `ApiKeyAdminCommandsTests.CreateKey_PersistsBareTokenPrefix_NotPrefixUnderscoreKeyId`). ### Auth-006 — LDAP connection ignores `allowInsecure` and offers no TLS certificate-validation hook @@ -228,7 +228,7 @@ _Unresolved._ |--|--| | Severity | Low | | Category | Security & secret handling | -| Status | Open | +| Status | Resolved | | Location | `ZB.MOM.WW.Auth/src/ZB.MOM.WW.Auth.Ldap/Internal/NovellLdapConnection.cs:16` | **Description** @@ -251,4 +251,4 @@ beyond the OS default, and document the default validation behavior. **Resolution** -_Unresolved._ +Resolved in `544a6dd`, 2026-06-01 — added an optional `LdapOptions.ServerCertificateValidationCallback` (BCL `RemoteCertificateValidationCallback`, no new dependency) threaded through the `ILdapConnection.Connect` seam; `NovellLdapConnection` now builds `LdapConnectionOptions.ConfigureRemoteCertificateValidationCallback(...)` (a supplied callback wins; otherwise the previously-dead `allowInsecure` gates a dev-only accept-any path; otherwise OS-trust-store default), and the default validation behaviour is documented in XML/README (tests: `LdapAuthServiceTests.Connect_ReceivesConfiguredCertValidationCallback`, `..._Connect_ReceivesAllowInsecureFlag_FromOptions`, `..._Connect_NoCertCallbackConfigured_PassesNull`). diff --git a/code-reviews/Configuration/findings.md b/code-reviews/Configuration/findings.md index 840e833..2698613 100644 --- a/code-reviews/Configuration/findings.md +++ b/code-reviews/Configuration/findings.md @@ -10,7 +10,7 @@ | Last reviewed | 2026-06-01 | | Reviewer | Claude (automated baseline) | | Commit reviewed | `5f75cd4` | -| Open findings | 4 | +| Open findings | 0 | ## Summary @@ -58,7 +58,7 @@ is solid for the happy and primary-failure paths; a few edge cases noted below a |--|--| | Severity | Low | | Category | Correctness & logic bugs | -| Status | Open | +| Status | Resolved | | Location | `ZB.MOM.WW.Configuration/src/ZB.MOM.WW.Configuration/ServiceCollectionExtensions.cs:36` | **Description** @@ -86,7 +86,7 @@ Replace `services.AddSingleton, TValidator>()` with **Resolution** -_Unresolved._ +Resolved in `544a6dd`, 2026-06-01 — `AddValidatedOptions` now registers the validator via `TryAddEnumerable(ServiceDescriptor.Singleton<...>())`, so a double call is idempotent (test: `AddValidatedOptionsTests.Calling_twice_registers_validator_once`). ### Configuration-002 — `Checks.PortValue` quotes the raw value on a parse failure but not on a range failure @@ -94,7 +94,7 @@ _Unresolved._ |--|--| | Severity | Low | | Category | Spec & shared-contract adherence | -| Status | Open | +| Status | Resolved | | Location | `ZB.MOM.WW.Configuration/src/ZB.MOM.WW.Configuration/Checks.cs:21` | **Description** @@ -119,7 +119,7 @@ shared formatter. Lock the exact strings down with a wording assertion test. **Resolution** -_Unresolved._ +Resolved in `544a6dd`, 2026-06-01 — `Checks.PortValue` now quotes the offending raw value on both the parse-failure and range-failure branches (`(was '0')`), wording pinned by test (test: `ChecksWordingTests.PortValue_range_failure_quotes_the_value`). ### Configuration-003 — Port parsing accepts leading sign and surrounding whitespace and is culture-sensitive @@ -127,7 +127,7 @@ _Unresolved._ |--|--| | Severity | Low | | Category | Correctness & logic bugs | -| Status | Open | +| Status | Resolved | | 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** @@ -151,7 +151,7 @@ space-after-colon endpoint to pin the behaviour. **Resolution** -_Unresolved._ +Resolved in `544a6dd`, 2026-06-01 — both `PortValue` and `HostPort` now parse with `int.TryParse(s, NumberStyles.None, CultureInfo.InvariantCulture, ...)`, rejecting leading sign/whitespace and culture-dependent formats (test: `ChecksWordingTests.PortValue_rejects_loose_inputs`, `HostPort_rejects_loose_port_inputs`). ### Configuration-004 — XML documentation and README are not packaged into the nupkg @@ -159,7 +159,7 @@ _Unresolved._ |--|--| | Severity | Low | | Category | Documentation & XML docs | -| Status | Open | +| Status | Resolved | | 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** @@ -183,4 +183,4 @@ one-off. **Resolution** -_Unresolved._ +Resolved in `544a6dd`, 2026-06-01 — `GenerateDocumentationFile=true` added to `Directory.Build.props` (test project opts out) and `PackageReadmeFile`/README pack item added to the csproj, so `dotnet pack` now ships `ZB.MOM.WW.Configuration.xml` and `README.md` in the nupkg. diff --git a/code-reviews/Health/findings.md b/code-reviews/Health/findings.md index 0e4478c..cc6a6ba 100644 --- a/code-reviews/Health/findings.md +++ b/code-reviews/Health/findings.md @@ -10,7 +10,7 @@ | Last reviewed | 2026-06-01 | | Reviewer | Claude (automated baseline) | | Commit reviewed | `5f75cd4` | -| Open findings | 6 | +| Open findings | 0 | ## Summary @@ -57,7 +57,7 @@ and missing `GenerateDocumentationFile` so the (otherwise excellent) XML docs do |--|--| | Severity | Medium | | Category | Error handling & resilience | -| Status | Open | +| Status | Resolved | | Location | `ZB.MOM.WW.Health/src/ZB.MOM.WW.Health.Akka/AkkaClusterHealthCheck.cs:45`, `ZB.MOM.WW.Health/src/ZB.MOM.WW.Health.Akka/ActiveNodeHealthCheck.cs:106` | **Description** @@ -91,7 +91,7 @@ startup exceptions and mapping them to Degraded with a "cluster not yet ready" d **Resolution** -_Unresolved._ +Resolved in `544a6dd`, 2026-06-01 — wrapped the `Cluster.Get(system)`/`SelfMember`/leader reads in `AkkaClusterHealthCheck` and `ActiveNodeHealthCheck` in `try/catch (Exception when not OCE)` returning `Degraded("Akka cluster state not yet accessible.")`; tests add a plain non-clustered ActorSystem to assert Degraded instead of the escaping `ConfigurationException`. ### Health-002 — `GrpcDependencyHealthCheck` lets non-`RpcException`/non-`OperationCanceledException` errors escape @@ -99,7 +99,7 @@ _Unresolved._ |--|--| | Severity | Medium | | Category | Error handling & resilience | -| Status | Open | +| Status | Resolved | | Location | `ZB.MOM.WW.Health/src/ZB.MOM.WW.Health/GrpcDependencyHealthCheck.cs:54` | **Description** @@ -130,7 +130,7 @@ This brings the gRPC probe in line with `DatabaseHealthCheck` and the documented **Resolution** -_Unresolved._ +Resolved in `544a6dd`, 2026-06-01 — added a trailing `catch (Exception ex)` to `GrpcDependencyHealthCheck.CheckHealthAsync` returning `Unhealthy("{name} probe failed: {ex.Message}", ex)` after the OCE/Rpc external-cancellation handlers; new test asserts a probe throwing `InvalidOperationException` maps to Unhealthy. ### Health-003 — Null `description` is omitted from the JSON body instead of emitted as `null` @@ -138,7 +138,7 @@ _Unresolved._ |--|--| | Severity | Low | | Category | Spec & shared-contract adherence | -| Status | Open | +| Status | Resolved | | Location | `ZB.MOM.WW.Health/src/ZB.MOM.WW.Health/ZbHealthWriter.cs:32` | **Description** @@ -163,7 +163,7 @@ descriptions are omitted, and add a test pinning the chosen behaviour. **Resolution** -_Unresolved._ +Resolved in `544a6dd`, 2026-06-01 — removed `DefaultIgnoreCondition = WhenWritingNull` from `ZbHealthWriter.SerializerOptions` so a null `description` renders as `"description": null` (matching the spec example / UI-client shape); writer XML doc now states the key is always present, and a new `ResponseWriterTests` case pins present-and-null. ### Health-004 — XML docs recommend the `active` tag for ready-tier probes, contradicting the spec @@ -171,7 +171,7 @@ _Unresolved._ |--|--| | Severity | Low | | Category | Documentation & XML docs | -| Status | Open | +| Status | Resolved | | Location | `ZB.MOM.WW.Health/src/ZB.MOM.WW.Health.Akka/AkkaClusterHealthCheck.cs:11`, `ZB.MOM.WW.Health/src/ZB.MOM.WW.Health/GrpcDependencyHealthCheck.cs:21` | **Description** @@ -192,7 +192,7 @@ back into `SPEC.md` §2.2/§2.4 so code and spec agree. **Resolution** -_Unresolved._ +Resolved in `544a6dd`, 2026-06-01 — updated the XML summaries on `AkkaClusterHealthCheck` and `GrpcDependencyHealthCheck` to recommend `ZbHealthTags.Ready` only (dropped the `[ready, active]` suggestion), aligning the docs with SPEC §2.2/§2.4. ### Health-005 — `MapZbHealth` returns only the readiness builder, silently dropping conventions for the active/live tiers @@ -200,7 +200,7 @@ _Unresolved._ |--|--| | Severity | Low | | Category | Public API surface & compatibility | -| Status | Open | +| Status | Resolved | | Location | `ZB.MOM.WW.Health/src/ZB.MOM.WW.Health/ZbHealthEndpointExtensions.cs:64` | **Description** @@ -222,7 +222,7 @@ note it in the README's `MapZbHealth` example. **Resolution** -_Unresolved._ +Resolved in `544a6dd`, 2026-06-01 — `MapZbHealth` now returns a private `CompositeEndpointConventionBuilder` that fans `Add`/`Finally` to all three endpoint builders (readiness, active, liveness); `` docs updated, and a new `TierMappingTests` case proves `.RequireHost(...)` gates all three tiers. ### Health-006 — XML documentation is not emitted into the packed nupkgs @@ -230,7 +230,7 @@ _Unresolved._ |--|--| | Severity | Low | | Category | Packaging, dependencies & project layout | -| Status | Open | +| Status | Resolved | | Location | `ZB.MOM.WW.Health/Directory.Build.props:3`, `ZB.MOM.WW.Health/src/ZB.MOM.WW.Health/ZB.MOM.WW.Health.csproj:3` | **Description** @@ -252,4 +252,4 @@ CS1591 warnings surface on currently-undocumented members. **Resolution** -_Unresolved._ +Resolved in `544a6dd`, 2026-06-01 — set `true` (plus `NoWarn=CS1591` for test/non-packed members) in `Directory.Build.props`; verified `dotnet pack -c Release` now ships `lib/net10.0/*.xml` in all three nupkgs. diff --git a/code-reviews/README.md b/code-reviews/README.md index 713ebcc..c9fa1c4 100644 --- a/code-reviews/README.md +++ b/code-reviews/README.md @@ -35,26 +35,26 @@ code-reviews/ ## Summary -6 of 6 libraries reviewed. 35 pending findings across all libraries. +6 of 6 libraries reviewed. 0 pending findings across all libraries. | Severity | Open findings | |----------|---------------| | Critical | 0 | -| High | 1 | -| Medium | 9 | -| Low | 25 | -| **Total** | **35** | +| High | 0 | +| Medium | 0 | +| Low | 0 | +| **Total** | **0** | ## Library Status | Library | Last reviewed | Commit | Open (C/H/M/L) | Open | Total | |---------|---------------|--------|----------------|------|-------| -| [Audit](Audit/findings.md) | 2026-06-01 | `5f75cd4` | 0/0/1/4 | 5 | 5 | -| [Auth](Auth/findings.md) | 2026-06-01 | `5f75cd4` | 0/0/3/3 | 6 | 6 | -| [Configuration](Configuration/findings.md) | 2026-06-01 | `5f75cd4` | 0/0/0/4 | 4 | 4 | -| [Health](Health/findings.md) | 2026-06-01 | `5f75cd4` | 0/0/2/4 | 6 | 6 | -| [Telemetry](Telemetry/findings.md) | 2026-06-01 | `5f75cd4` | 0/1/2/5 | 8 | 8 | -| [Theme](Theme/findings.md) | 2026-06-01 | `5f75cd4` | 0/0/1/5 | 6 | 6 | +| [Audit](Audit/findings.md) | 2026-06-01 | `5f75cd4` | 0/0/0/0 | 0 | 5 | +| [Auth](Auth/findings.md) | 2026-06-01 | `5f75cd4` | 0/0/0/0 | 0 | 6 | +| [Configuration](Configuration/findings.md) | 2026-06-01 | `5f75cd4` | 0/0/0/0 | 0 | 4 | +| [Health](Health/findings.md) | 2026-06-01 | `5f75cd4` | 0/0/0/0 | 0 | 6 | +| [Telemetry](Telemetry/findings.md) | 2026-06-01 | `5f75cd4` | 0/0/0/0 | 0 | 8 | +| [Theme](Theme/findings.md) | 2026-06-01 | `5f75cd4` | 0/0/0/0 | 0 | 6 | ## Pending Findings @@ -67,52 +67,14 @@ description, location, recommendation — lives in the library's `findings.md`. _None open._ -### High (1) +### High (0) -| ID | Library | Title | -|----|---------|-------| -| Telemetry-001 | [Telemetry](Telemetry/findings.md) | `RedactionEnricher` ignores property removal, leaving secrets in the event | +_None open._ -### Medium (9) +### Medium (0) -| ID | Library | Title | -|----|---------|-------| -| Audit-001 | [Audit](Audit/findings.md) | `CompositeAuditWriter` re-throws `OperationCanceledException` to the caller, contradicting the "must not throw" writer contract | -| Auth-001 | [Auth](Auth/findings.md) | LDAP options validator is registered but never runs at startup | -| Auth-002 | [Auth](Auth/findings.md) | A failed `MarkUsedAsync` write turns a valid API key into a thrown exception | -| Auth-003 | [Auth](Auth/findings.md) | Corrupt `scopes`/`constraints` column throws `JsonException` through the verifier | -| Health-001 | [Health](Health/findings.md) | Akka health checks throw (instead of returning Degraded) when cluster state is inaccessible | -| Health-002 | [Health](Health/findings.md) | `GrpcDependencyHealthCheck` lets non-`RpcException`/non-`OperationCanceledException` errors escape | -| Telemetry-002 | [Telemetry](Telemetry/findings.md) | Redactor cannot inspect or scrub destructured/structured property values | -| Telemetry-003 | [Telemetry](Telemetry/findings.md) | No tests for redactor removal or structured-value redaction | -| Theme-001 | [Theme](Theme/findings.md) | Mobile hamburger toggle silently depends on Bootstrap collapse JS | +_None open._ -### Low (25) +### Low (0) -| ID | Library | Title | -|----|---------|-------| -| Audit-002 | [Audit](Audit/findings.md) | `TruncatingAuditRedactor` over-redaction is partial: the catch path scrubs only `DetailsJson`, leaving `Target` unredacted | -| Audit-003 | [Audit](Audit/findings.md) | `TruncatingAuditRedactorOptions` is a mutable class, not the immutable "options record" the contract describes | -| Audit-004 | [Audit](Audit/findings.md) | XML documentation is authored but not emitted, so IntelliSense docs do not ship to consumers | -| Audit-005 | [Audit](Audit/findings.md) | Missing edge-case tests for the redactor never-throw/over-redact contract and composite null/empty handling | -| Auth-004 | [Auth](Auth/findings.md) | README misstates the hashing algorithm and the AspNetCore public surface | -| Auth-005 | [Auth](Auth/findings.md) | `CreateKeyAsync` persists `KeyPrefix` as `prefix_keyId`, inconsistent with the read path | -| Auth-006 | [Auth](Auth/findings.md) | LDAP connection ignores `allowInsecure` and offers no TLS certificate-validation hook | -| Configuration-001 | [Configuration](Configuration/findings.md) | `AddValidatedOptions` uses `AddSingleton`, so a double call registers (and runs) the validator twice | -| Configuration-002 | [Configuration](Configuration/findings.md) | `Checks.PortValue` quotes the raw value on a parse failure but not on a range failure | -| Configuration-003 | [Configuration](Configuration/findings.md) | Port parsing accepts leading sign and surrounding whitespace and is culture-sensitive | -| Configuration-004 | [Configuration](Configuration/findings.md) | XML documentation and README are not packaged into the nupkg | -| Health-003 | [Health](Health/findings.md) | Null `description` is omitted from the JSON body instead of emitted as `null` | -| Health-004 | [Health](Health/findings.md) | XML docs recommend the `active` tag for ready-tier probes, contradicting the spec | -| Health-005 | [Health](Health/findings.md) | `MapZbHealth` returns only the readiness builder, silently dropping conventions for the active/live tiers | -| Health-006 | [Health](Health/findings.md) | XML documentation is not emitted into the packed nupkgs | -| Telemetry-004 | [Telemetry](Telemetry/findings.md) | `service.instance.id` Resource attribute is undocumented in spec and contract | -| Telemetry-005 | [Telemetry](Telemetry/findings.md) | Two hand-maintained Resource-attribute builders can silently drift | -| Telemetry-006 | [Telemetry](Telemetry/findings.md) | Malformed `OtlpEndpoint` throws `UriFormatException` late, with no context | -| Telemetry-007 | [Telemetry](Telemetry/findings.md) | Redaction snapshot allocates a dictionary on every log event | -| Telemetry-008 | [Telemetry](Telemetry/findings.md) | `MapZbMetrics` XML doc claims it is "only valid when Exporter = Prometheus" — stale | -| Theme-002 | [Theme](Theme/findings.md) | `.chip-idle` foreground diverges from the documented token pairing | -| Theme-003 | [Theme](Theme/findings.md) | Hardcoded hex values in CSS contradict the "no hardcoded hex" rule | -| Theme-004 | [Theme](Theme/findings.md) | `NavRailItem` emits a `.rail-ico` span that no stylesheet defines | -| Theme-005 | [Theme](Theme/findings.md) | Orphan and unstyled nav CSS classes in `layout.css` | -| Theme-006 | [Theme](Theme/findings.md) | Public component/parameter surface lacks XML documentation | +_None open._ diff --git a/code-reviews/Telemetry/findings.md b/code-reviews/Telemetry/findings.md index 8566ea9..88dfbe3 100644 --- a/code-reviews/Telemetry/findings.md +++ b/code-reviews/Telemetry/findings.md @@ -10,7 +10,7 @@ | Last reviewed | 2026-06-01 | | Reviewer | Claude (automated baseline) | | Commit reviewed | `5f75cd4` | -| Open findings | 8 | +| Open findings | 0 | ## Summary @@ -55,7 +55,7 @@ happy paths but have no coverage for redactor removal or structured-value redact |--|--| | Severity | High | | Category | Security & secret handling | -| Status | Open | +| Status | Resolved | | Location | `ZB.MOM.WW.Telemetry/src/ZB.MOM.WW.Telemetry.Serilog/RedactionEnricher.cs:49-67` | **Description** @@ -90,7 +90,9 @@ diff.) Add a test asserting a removing redactor scrubs the property (see Telemet **Resolution** -_Unresolved._ +Resolved in `544a6dd`, 2026-06-01 — `RedactionEnricher` now captures the original property +key set and calls `RemovePropertyIfPresent` for any key the redactor dropped from the snapshot, +so a removing redactor scrubs the property; covered by a new removing-redactor test. ### Telemetry-002 — Redactor cannot inspect or scrub destructured/structured property values @@ -98,7 +100,7 @@ _Unresolved._ |--|--| | Severity | Medium | | Category | Security & secret handling | -| Status | Open | +| Status | Resolved | | Location | `ZB.MOM.WW.Telemetry/src/ZB.MOM.WW.Telemetry.Serilog/RedactionEnricher.cs:49-55` | **Description** @@ -129,7 +131,10 @@ payloads are scrubbed when they are not. **Resolution** -_Unresolved._ +Resolved in `544a6dd`, 2026-06-01 — documented the seam's reach (scalar top-level properties +only; structured values exposed as their raw Serilog wrapper, redactable whole-property only) on +the `ILogRedactor` XML doc, the shared contract, and the README; pinned by a destructured-object +test. Nested-field redaction was deliberately not implemented (too invasive for v0.1.0). ### Telemetry-003 — No tests for redactor removal or structured-value redaction @@ -137,7 +142,7 @@ _Unresolved._ |--|--| | Severity | Medium | | Category | Testing coverage | -| Status | Open | +| Status | Resolved | | Location | `ZB.MOM.WW.Telemetry/tests/ZB.MOM.WW.Telemetry.Serilog.Tests/RedactionTests.cs:33-69` | **Description** @@ -158,7 +163,9 @@ takes). These should fail today and pin the fixes. **Resolution** -_Unresolved._ +Resolved in `544a6dd`, 2026-06-01 — added `Removing_redactor_scrubs_the_property_from_the_event` +(red→green for Telemetry-001) and `Redactor_cannot_reach_a_field_inside_a_destructured_object` +(pins the documented Telemetry-002 limitation), plus a Resource-attribute parity test. ### Telemetry-004 — `service.instance.id` Resource attribute is undocumented in spec and contract @@ -166,7 +173,7 @@ _Unresolved._ |--|--| | Severity | Low | | Category | Spec & shared-contract adherence | -| Status | Open | +| Status | Resolved | | Location | `ZB.MOM.WW.Telemetry/src/ZB.MOM.WW.Telemetry/ZbResource.cs:19-45` | **Description** @@ -190,7 +197,10 @@ member in the shared contract, so the normalized spec and the code agree. **Resolution** -_Unresolved._ +Resolved in `544a6dd`, 2026-06-01 — kept the attribute (documented the +`MachineName:ProcessId` rationale) and added `service.instance.id` to the Resource tables in +`SPEC.md` §2 and `METRIC-CONVENTIONS.md` §4, plus the `ZbResource.InstanceId` member to the shared +contract; spec and code now agree. ### Telemetry-005 — Two hand-maintained Resource-attribute builders can silently drift @@ -198,7 +208,7 @@ _Unresolved._ |--|--| | Severity | Low | | Category | Spec & shared-contract adherence | -| Status | Open | +| Status | Resolved | | Location | `ZB.MOM.WW.Telemetry/src/ZB.MOM.WW.Telemetry/ZbResource.cs:38-64`, `ZB.MOM.WW.Telemetry/src/ZB.MOM.WW.Telemetry.Serilog/ZbSerilogConfig.cs:125-151` | **Description** @@ -221,7 +231,9 @@ key-for-key identical for a representative options object. **Resolution** -_Unresolved._ +Resolved in `544a6dd`, 2026-06-01 — introduced `ZbResource.BuildAttributes` as the single +source of truth; `ZbResource.Configure` (OTel SDK) and `ZbSerilogConfig.BuildResourceAttributes` +(OTLP log sink) now both derive from it, and a parity test asserts the two sets are identical. ### Telemetry-006 — Malformed `OtlpEndpoint` throws `UriFormatException` late, with no context @@ -229,7 +241,7 @@ _Unresolved._ |--|--| | Severity | Low | | Category | Error handling & resilience | -| Status | Open | +| Status | Resolved | | Location | `ZB.MOM.WW.Telemetry/src/ZB.MOM.WW.Telemetry/ZbTelemetryExtensions.cs:127-135` | **Description** @@ -252,7 +264,10 @@ option (consistent with the existing `ServiceName` guard) rather than letting a **Resolution** -_Unresolved._ +Resolved in `544a6dd`, 2026-06-01 — added `ZbTelemetryOptionsValidator.Validate`, called from +both `BuildOptions` and `AddZbSerilog`: when `Exporter == Otlp` it requires a non-empty, +well-formed absolute `OtlpEndpoint` and throws a named `ArgumentException` (no-op for Prometheus); +covered by three new tests. ### Telemetry-007 — Redaction snapshot allocates a dictionary on every log event @@ -260,7 +275,7 @@ _Unresolved._ |--|--| | Severity | Low | | Category | Performance & resource management | -| Status | Open | +| Status | Resolved | | Location | `ZB.MOM.WW.Telemetry/src/ZB.MOM.WW.Telemetry.Serilog/RedactionEnricher.cs:49-67` | **Description** @@ -282,7 +297,9 @@ redaction on very hot loggers. Acceptable as-is given redaction is opt-in and se **Resolution** -_Unresolved._ +Resolved in `544a6dd`, 2026-06-01 — `Enrich` now short-circuits before any snapshot allocation +when the event has no properties (and still early-returns when no redactor is registered), so the +per-event dictionary copy is only paid when there is actually something to redact. ### Telemetry-008 — `MapZbMetrics` XML doc claims it is "only valid when Exporter = Prometheus" — stale @@ -290,7 +307,7 @@ _Unresolved._ |--|--| | Severity | Low | | Category | Documentation & XML docs | -| Status | Open | +| Status | Resolved | | Location | `ZB.MOM.WW.Telemetry/src/ZB.MOM.WW.Telemetry/ZbMetricsEndpointExtensions.cs:11-14` | **Description** @@ -314,4 +331,6 @@ overlay). Align the shared-contract summary for `MapZbMetrics` to match. **Resolution** -_Unresolved._ +Resolved in `544a6dd`, 2026-06-01 — rewrote the `MapZbMetrics` XML doc to state it is valid +under any `Exporter` value (Prometheus always-on; OTLP additive overlay) and aligned the matching +shared-contract summary. diff --git a/code-reviews/Theme/findings.md b/code-reviews/Theme/findings.md index 8193ca3..bc3c27e 100644 --- a/code-reviews/Theme/findings.md +++ b/code-reviews/Theme/findings.md @@ -10,7 +10,7 @@ | Last reviewed | 2026-06-01 | | Reviewer | Claude (automated baseline) | | Commit reviewed | `5f75cd4` | -| Open findings | 6 | +| Open findings | 0 | ## Summary @@ -52,7 +52,7 @@ absence of XML docs on the public parameter surface. |--|--| | Severity | Medium | | Category | Spec & shared-contract adherence | -| Status | Open | +| Status | Resolved | | Location | `ZB.MOM.WW.Theme/src/ZB.MOM.WW.Theme/Components/ThemeShell.razor:5` | **Description** @@ -81,7 +81,7 @@ accurate. **Resolution** -_Unresolved._ +Resolved in `544a6dd`, 2026-06-01 — removed the Bootstrap-collapse JS dependency: the shell is now a native CSS-only `
`/`` disclosure (no `data-bs-*`), force-shown on lg+; bUnit test asserts no Bootstrap collapse hooks. ### Theme-002 — `.chip-idle` foreground diverges from the documented token pairing @@ -89,7 +89,7 @@ _Unresolved._ |--|--| | Severity | Low | | Category | Spec & shared-contract adherence | -| Status | Open | +| Status | Resolved | | Location | `ZB.MOM.WW.Theme/src/ZB.MOM.WW.Theme/wwwroot/css/theme.css:177` | **Description** @@ -117,7 +117,7 @@ Either change `.chip-idle` to `color: var(--idle)` to honor the documented pairi **Resolution** -_Unresolved._ +Resolved in `544a6dd`, 2026-06-01 — changed `.chip-idle` foreground from `var(--ink-soft)` to `var(--idle)`, honoring the DESIGN-TOKENS pairing rule; static-asset test asserts the pairing. ### Theme-003 — Hardcoded hex values in CSS contradict the "no hardcoded hex" rule @@ -125,7 +125,7 @@ _Unresolved._ |--|--| | Severity | Low | | Category | Spec & shared-contract adherence | -| Status | Open | +| Status | Resolved | | Location | `ZB.MOM.WW.Theme/src/ZB.MOM.WW.Theme/wwwroot/css/theme.css:174` (and theme.css:96-98,145-150,175-176,251-254,281,290-291,335,348,368-369,377; layout.css:123,128,184) | **Description** @@ -150,7 +150,7 @@ honest. **Resolution** -_Unresolved._ +Resolved in `544a6dd`, 2026-06-01 — promoted every derived border/ink/wash shade to named `:root` tokens (`--ok-border`, `--warn-ink`, `--info-bg`, `--hover-bg`, `--active-bg`, `--zebra-bg`, …) and referenced them; no hex literal remains outside `:root` in either stylesheet, asserted by a new test. ### Theme-004 — `NavRailItem` emits a `.rail-ico` span that no stylesheet defines @@ -158,7 +158,7 @@ _Unresolved._ |--|--| | Severity | Low | | Category | Correctness & logic bugs | -| Status | Open | +| Status | Resolved | | Location | `ZB.MOM.WW.Theme/src/ZB.MOM.WW.Theme/Components/NavRailItem.razor:4` | **Description** @@ -178,7 +178,7 @@ color:var(--ink-faint);`) alongside the existing `.rail-link` rules, and add a b **Resolution** -_Unresolved._ +Resolved in `544a6dd`, 2026-06-01 — added a `.rail-ico` rule to `layout.css` (inline-flex, sized, gapped, `--ink-faint`); bUnit tests assert the span is emitted when `Icon` is supplied and omitted otherwise. ### Theme-005 — Orphan and unstyled nav CSS classes in `layout.css` @@ -186,7 +186,7 @@ _Unresolved._ |--|--| | Severity | Low | | Category | Documentation & XML docs | -| Status | Open | +| Status | Resolved | | Location | `ZB.MOM.WW.Theme/src/ZB.MOM.WW.Theme/wwwroot/css/layout.css:75` (`.rail-eyebrow`), `:103` (`.rail-eyebrow-chevron`); `ZB.MOM.WW.Theme/src/ZB.MOM.WW.Theme/Components/NavRailSection.razor:5` (`.rail-eyebrow-label`) | **Description** @@ -208,7 +208,7 @@ rule or drop the now-redundant wrapper span. Keeping one chevron mechanism remov **Resolution** -_Unresolved._ +Resolved in `544a6dd`, 2026-06-01 — removed the dead `.rail-eyebrow` and `.rail-eyebrow-chevron` rules and dropped the redundant `.rail-eyebrow-label` wrapper span in `NavRailSection`, leaving one chevron mechanism (`summary::before`). ### Theme-006 — Public component/parameter surface lacks XML documentation @@ -216,7 +216,7 @@ _Unresolved._ |--|--| | Severity | Low | | Category | Documentation & XML docs | -| Status | Open | +| Status | Resolved | | Location | `ZB.MOM.WW.Theme/src/ZB.MOM.WW.Theme/Components/ThemeShell.razor:23` (and `BrandBar.razor`, `NavRailItem.razor`, `NavRailSection.razor`, `StatusPill.razor`, `TechButton.razor`, `TechCard.razor`, `TechField.razor`, `ThemeHead.razor`, `ButtonVariant.cs`, `StatusState.cs`) | **Description** @@ -239,4 +239,4 @@ minimum the non-obvious ones (`ThemeShell.Accent`, `NavRailItem.Match`, `TechBut **Resolution** -_Unresolved._ +Resolved in `544a6dd`, 2026-06-01 — added `///` summaries to all nine remaining components, every public `[Parameter]`, and both enums (`ButtonVariant`, `StatusState`); enabled `GenerateDocumentationFile` (CS1591 left off the error set for Razor-generated members) so docs build under `TreatWarningsAsErrors` with 0 warnings.