Phase 4 Wave 3: DI-resolved validator path + ISettingsCollection exposure (VAL-01 DI path, API-02)

.planning/STATE.md

@@ -5,15 +5,15 @@ milestone_name: milestone
 current_phase: 04
 current_phase_name: collection-validation-binding
 status: executing
-stopped_at: Completed 04-05-PLAN.md (VAL-02) — Phase 4 all 5 plans done
-last_updated: "2026-07-15T13:58:04.988Z"
+stopped_at: Completed 04-04-PLAN.md (VAL-01 DI path + API-02, Wave 3) — all 5 Phase 4 plans landed; phase verify + secure + mark-complete pending
+last_updated: "2026-07-15T18:00:00.000Z"
 last_activity: 2026-07-15
-last_activity_desc: Phase 04 execution started
+last_activity_desc: Phase 04 Plan 04 (Wave 3) executed + reviewed
 progress:
   total_phases: 6
   completed_phases: 2
   total_plans: 9
-  completed_plans: 8
+  completed_plans: 9
   percent: 33
 ---
 
@@ -28,10 +28,10 @@ See: .planning/PROJECT.md (updated 2026-07-13)
 
 ## Current Position
 
-Phase: 04 (collection-validation-binding) — EXECUTING
-Plan: 5 of 5
-Status: Ready to execute
-Last activity: 2026-07-15 — Phase 04 execution started
+Phase: 04 (collection-validation-binding) — EXECUTING (all 5 plans landed; verify + secure + mark-complete pending)
+Plan: 5 of 5 — complete
+Status: Wave 3 (04-04) executed + reviewed; ready for phase verify
+Last activity: 2026-07-15 — Phase 04 Plan 04 (Wave 3) executed + reviewed
 
 Progress: [███░░░░░░░] 33%
 
@@ -64,6 +64,7 @@ Progress: [███░░░░░░░] 33%
 | Phase 04 P02 | 12min | 2 tasks | 4 files |
 | Phase 04 P03 | 5min | 2 tasks | 10 files |
 | Phase 04 P05 | 3min | 1 tasks | 2 files |
+| Phase 04 P04 | ~35min | 2 tasks | 5 files |
 
 ## Accumulated Context
 
@@ -89,6 +90,7 @@ Recent decisions affecting current work:
 - [Phase ?]: HasValidators short-circuit on the cached plan gates the validation hook before any allocation (protects the B-2 benchmark allocation gate)
 - [Phase 04]: 04-05 VAL-02: reuse value-free SettingsPropertyNullException for empty/whitespace rejection — already excluded from the ValuesPopulator:122 redaction filter, so no filter change
 - [Phase 04]: 04-05 VAL-02: reject guard placed ahead of 04-01's Func<object> list null-result factory dispatch and gated on _throwOnNull; accept path and factory dispatch untouched
+- [Phase 04]: 04-04 VAL-01 DI path + API-02: ISettingsCollection exposed via a DI singleton + an AddSimpleSettings(out ISettingsCollection, Action?) overload; deferred opt-in IServiceProvider.ValidateSimpleSettings() runs DI-registered ISettingValidation<T> from a fresh scope (IServiceScopeFactory), dispatches via the DIM bridge (no reflection), and throws the same value-free SettingsValidationException as the core path via the shared ThrowIfAny. Runner is internal; DI path is additive (reads no attribute).
 
 ### Pending Todos
 

.planning/phases/04-collection-validation-binding/04-04-PLAN.md

@@ -28,7 +28,7 @@ must_haves:
     - "IntegrateSimpleSettings returns the built ISettingsCollection so both the DI-singleton registration and the out-overload surface the same instance."
     - "The DI runner reuses ONLY public types (ISettingValidation<T>, ValidationContext<T>, ValidationResult, ValidationError, SettingsValidationException) — no Core InternalsVisibleTo needed."
     - "The DI runner resolves ISettingValidation<T> from `using var scope = provider.CreateScope()` (scope.ServiceProvider), NOT the root provider (review S-1)."
-    - "Reflective validator dispatch selects the overload via GetMethod(\"Validate\", new[]{ closedContextType }) and builds ValidationContext<T> via MakeGenericType (avoids AmbiguousMatchException — review S-2)."
+    - "SUPERSEDED (architect+security review): validator dispatch uses the ISettingValidation<T> default-interface bridge — cast to ISettingsValidator and call Validate(new ValidationContext(instance)); NO reflection over Validate and no generic ValidationContext<T> construction. Mirrors the shipped core ValuesPopulator.InvokeValidator path. GetServices still uses MakeGenericType(ISettingValidation<>) to resolve the closed validator type."
     - "The deferred runner aggregates errors and throws through the SAME Core helper SettingsValidationException.ThrowIfAny(errors) as Plan 03's core path, so the exception is contract-identical (D-10/D-12 / review S-3)."
 ---
 
@@ -177,12 +177,14 @@ New symbols introduced by THIS plan (exclude from drift/orphan checks):
     and resolve all registered validators for that type via
     `scope.ServiceProvider.GetServices(typeof(ISettingValidation<>).MakeGenericType(type))`.
 
-    REFLECTIVE DISPATCH (review S-2, architect A3): because the settings type is a runtime `Type`, both the
-    context construction and the `Validate` call are reflective. `ISettingValidation<T> : ISettingsValidator`
-    declares TWO `Validate` overloads, so select the method explicitly with
-    `closedValidationType.GetMethod("Validate", new[] { closedContextType })` — NEVER the parameterless
-    `GetMethod("Validate")`, which throws AmbiguousMatchException. Build the context with
-    `Activator.CreateInstance(typeof(ValidationContext<>).MakeGenericType(type), instance)`.
+    DISPATCH — DIM BRIDGE, NOT REFLECTION (architect + security review; SUPERSEDES the earlier S-2 text
+    below): resolve the validators for the runtime `Type` via
+    `scope.ServiceProvider.GetServices(typeof(ISettingValidation<>).MakeGenericType(type))`, then dispatch each
+    through the default-interface bridge — `((ISettingsValidator)validator).Validate(new ValidationContext(instance))`.
+    `ISettingValidation<T>` default-implements the base `Validate`, so there is NO reflection over the `Validate`
+    methods (`GetMethod`/`MethodInfo.Invoke`) and NO generic `ValidationContext<T>` construction. This mirrors the
+    shipped core `ValuesPopulator.InvokeValidator` path exactly and structurally avoids the
+    `TargetInvocationException` inner-chaining leak vector.
 
     Collect every `ValidationError` across all resolved validators and finish by calling the SHARED Core helper
     `SettingsValidationException.ThrowIfAny(allErrors)` (review S-3 / D-10) — the SAME helper Plan 03's core path
@@ -202,7 +204,7 @@ New symbols introduced by THIS plan (exclude from drift/orphan checks):
     - `ISettingsValidationRunner` + `SettingsValidationRunner` exist in GenericHost; `ValidateSimpleSettings(this IServiceProvider)` is public.
     - A failing DI-registered ISettingValidation<T> throws SettingsValidationException only when ValidateSimpleSettings() is called (not during AddSimpleSettings).
     - S-1: the runner resolves validators from `provider.CreateScope().ServiceProvider` (a fresh scope), and a validator with a SCOPED injected dependency runs successfully under `BuildServiceProvider(validateScopes: true)`.
-    - S-2: reflective dispatch selects the overload via `GetMethod("Validate", new[]{ closedContextType })` (no AmbiguousMatchException) and builds `ValidationContext<T>` via `MakeGenericType`.
+    - S-2 (SUPERSEDED): dispatch is the `((ISettingsValidator)validator).Validate(new ValidationContext(instance))` default-interface bridge — no reflection over `Validate`, no `MakeGenericType` on the context.
     - S-3: the runner aggregates and throws via `SettingsValidationException.ThrowIfAny(errors)` (the shared Core helper from Plan 03), so the DI-path exception type + Errors shape match the core-path contract; no bound value is present in ToString().
     - The runner uses only public validation types (no new InternalsVisibleTo to Core).
     - AddSimpleSettingsIntegrationTests green on net10; dotnet-architect + security-auditor sign-off recorded.

.planning/phases/04-collection-validation-binding/04-04-REVIEW.md

@@ -0,0 +1,98 @@
+---
+phase: 04-collection-validation-binding (Wave 3)
+reviewed: 2026-07-15T00:00:00Z
+depth: deep
+diff_range: 4bae833..HEAD
+files_reviewed: 4
+files_reviewed_list:
+  - src/Core/ExistForAll.SimpleSettings.Extensions.GenericHost/ISettingsValidationRunner.cs
+  - src/Core/ExistForAll.SimpleSettings.Extensions.GenericHost/ServiceProviderValidationExtensions.cs
+  - src/Core/ExistForAll.SimpleSettings.Extensions.GenericHost/ServicesSettingsBuilderExtensions.cs
+  - src/Core/ExistForAll.SimpleSettings.Extensions.GenericHost/SettingsValidationRunner.cs
+findings:
+  blocker: 0
+  high: 0
+  medium: 0
+  low: 3
+  total: 3
+status: issues_found
+---
+
+# Phase 04 Wave 3: Code Review Report
+
+**Depth:** deep (cross-file: verified DI runner against core `ValuesPopulator.InvokeValidator`, `SettingsValidationException.ThrowIfAny`, `ISettingValidation<T>` default-interface bridge, `SettingsValidatorInvocationException`)
+**Status:** issues_found (LOW only — nothing material)
+
+## Summary
+
+The DI-resolved validator path is a faithful mirror of the core populate path. All security-sensitive
+invariants hold:
+
+- **Redaction (LOCKED):** on a validator throw the runner rethrows value-free as
+  `new SettingsValidatorInvocationException(validator.GetType(), e.GetType())` — no chained inner, no
+  settings value. The bound `pair.Value` never reaches any library exception's message/ToString. Verified
+  against the dedicated redaction test (`ThrowingDiValidator` embeds a sentinel secret; the surfaced
+  exception is asserted clean).
+- **No reflection:** dispatch is the `((ISettingsValidator)validator).Validate(new ValidationContext(pair.Value))`
+  cast; the `ISettingValidation<T>` default-interface bridge forwards to the author overload. Cast is
+  provably safe (services resolved for `ISettingValidation<pair.Key>` are `ISettingsValidator`).
+- **Non-null aggregate:** `errors` is eagerly `new List<ValidationError>()`, so `ThrowIfAny(errors)` is
+  never handed null. Empty list → no-op, matching core.
+- **Fresh scope:** `IServiceScopeFactory.CreateScope()` with `using`; disposed even when `ThrowIfAny`
+  throws. Singleton runner injecting singleton `ISettingsCollection` + `IServiceScopeFactory` — no captive
+  dependency, no async/lifetime issue.
+- **out-overload + registration:** `AddSimpleSettings(out ISettingsCollection, Action?)` returns the built
+  collection and `services`; no overload ambiguity with the existing 2-arg `Action` overload (differs by
+  the `out` parameter). `ISettingsCollection` + `ISettingsValidationRunner` registered as singletons.
+
+No BLOCKER / HIGH / MEDIUM findings. Three LOW items below.
+
+## Low
+
+### LOW-1: Internal planning-ID reference ("See S1") in a comment — reintroduces a prior review finding
+
+**File:** `src/Core/ExistForAll.SimpleSettings.Extensions.GenericHost/SettingsValidationRunner.cs:41`
+**Issue:** The catch-block comment ends with `See S1.` — an internal planning-ID reference. This is
+exactly the class of comment a prior review flagged (LOW-1) and the phase convention explicitly prohibits.
+(The core files carry the same references, but this diff adds a new occurrence.)
+**Fix:** Drop the planning-ID tail:
+```csharp
+// A validator threw: surface value-free (the inner may embed a secret it read) — only the
+// validator and failure types, never the instance and never a chained inner.
+```
+
+### LOW-2: Multi-line rationale comments exceed the org "one line max" guideline
+
+**File:** `src/Core/ExistForAll.SimpleSettings.Extensions.GenericHost/SettingsValidationRunner.cs:19-20, 34-35, 40-42, 53-54`
+**Issue:** Four two-line comments. Borderline: this matches the established heavily-commented style of the
+core files (`ValuesPopulator`, the exception family), so it is arguably an accepted project convention that
+overrides the org default. Noted for consistency only; not actionable if the project style is intentional.
+**Fix:** Optionally collapse each to a single line, or leave as-is to match core style.
+
+### LOW-3: Repeated `AddSimpleSettings` calls silently validate only the last collection
+
+**File:** `src/Core/ExistForAll.SimpleSettings.Extensions.GenericHost/ServicesSettingsBuilderExtensions.cs:51-53`
+**Issue:** Each call adds another `ISettingsCollection` and `ISettingsValidationRunner` singleton. DI
+last-wins, so a second `AddSimpleSettings` (different assembly set) means the runner's injected
+`ISettingsCollection` is the last one — the first collection's settings are never checked by
+`ValidateSimpleSettings()`. Pre-existing last-wins pattern (same as `ISettingsProvider`), and multi-call is
+likely unsupported, so this is an edge-case gap, not a regression.
+**Fix:** If multi-call is out of scope, no change. If it should be supported, have the runner enumerate all
+registered `ISettingsCollection` instances (inject `IEnumerable<ISettingsCollection>`).
+
+## Non-findings verified (parity preserved, intentionally not flagged)
+
+- A validator returning `null` `ValidationResult` would NRE at `result.Errors` (outside the try). This is
+  **identical** to core `InvokeValidator` (`ValuesPopulator.cs:117`); fixing only the DI side would break
+  the "identical contract" guarantee. Left as-is intentionally.
+- `provider.GetServices(...)` activation failures propagate raw (outside the try). Not a library exception
+  and cannot carry a bound settings value — no redaction concern.
+- Author-supplied `ValidationError.ErrorMessage` may reach `SettingsValidationException`'s message — by
+  design (author text, not a bound value); matches core.
+- `SettingsValidatorInvocationException` uses `validator.GetType()` (runtime) vs core's `validatorType`
+  (declared) — per the approved constraint; both surface the concrete validator type. Not a divergence.
+
+---
+
+_Reviewer: Claude (adversarial code review)_
+_Depth: deep_