From 4bae833667b02c173bbf7e308a380c691eccbea4 Mon Sep 17 00:00:00 2001 From: guy-lud Date: Wed, 15 Jul 2026 21:44:30 +0300 Subject: [PATCH 1/5] docs: refresh session handoff (Phase 4 Waves 1-2 + comment #3 merged; Wave 3 next) --- SESSION-HANDOFF.md | 105 +++++++++++++++++++++++++-------------------- 1 file changed, 58 insertions(+), 47 deletions(-) diff --git a/SESSION-HANDOFF.md b/SESSION-HANDOFF.md index 73ecfff..69a1611 100644 --- a/SESSION-HANDOFF.md +++ b/SESSION-HANDOFF.md @@ -1,62 +1,73 @@ # SESSION HANDOFF — SimpleSettings -_Last updated: 2026-07-14 · owner: Guy Ludvig (guy@frontegg.com)_ +_Last updated: 2026-07-15 (reconcile + wrap session) · owner: Guy Ludvig (guy@frontegg.com)_ ## TL;DR -**GSD is the source of truth** (`.planning/`); `FIX-PLAN.md` is frozen historical reference only. -Phases **1 & 2 merged** (PR #30 → `master` @ `67aa72f`; `master` now @ **`8750359`**). **Phase 3 — Public Surface, Packaging & Binder Cleanup — is COMPLETE and in open PR #31** (`https://github.com/existall/SimpleSettings/pull/31`, base `master`, head `gsd/phase-3-public-surface-packaging-binder-cleanup` @ `cf69197`). Verified **4/4 must-haves**, review-clean, suite **208/208** (net8+net10), build 0 warnings. - -**Next milestone work is decided:** a **new engine phase goes in BEFORE the beta** to satisfy client requirements (approved 2026-07-14). Currently on a LOCAL staging branch **`gsd/phase-4-collection-validation-binding`** (forked off the Phase 3 tip `cf69197`, NOT pushed). The roadmap has NOT been restructured yet. +**GSD is the source of truth** (`.planning/`); `FIX-PLAN.md` frozen historical reference only. +**Phases 1–3 + Phase 4 Waves 1–2 (incl. review comment #3) are SHIPPED to `master`** (@ `1388c5c`). Phase 4 **Wave 3 (plan 04-04) is NOT started**; phase verify + security gate + "mark complete" also pending. +This session: resolved PR #33 review comment #3, answered all 4 review comments, then **merged PR #33** (Waves 1–2, `52a2c6f`) and **PR #34** (Phase-4 `.planning` docs + a cosmetic `[]` tweak, `1388c5c`) to master. **Two `2.0.0-alpha.0.*` alphas auto-published** (one per merge) — add to the unlist list. ## Do this first (new session) -1. **Verify git state** (`git log -3`, `git branch --show-current`, `gh pr list`): expect current branch **`gsd/phase-4-collection-validation-binding`** (local), PR **#31 open** (Phase 3), PR #30 merged. `master` @ `8750359`. -2. **Decide the Phase-3 PR #31 disposition:** merge it to `master` via `guy-lud` (publishes a throwaway alpha — expected pre-stable), or leave open while stacking Phase 4. If #31 merges, **rebase `gsd/phase-4-collection-validation-binding` onto updated `master`** (it's currently stacked on the unmerged Phase 3 tip). -3. **Formalize the new engine phase** (client reqs — full detail in `.planning/backlog/client-requirements-pre-beta.md`): - - Add 5 requirements to `REQUIREMENTS.md`: **COLL-02** (empty-collection default), **COLL-03** (YAML-sequence / indexed-child binding), **VAL-01** (wire `ISettingValidation`/`ValidatorType`), **VAL-02** (tighter `AllowEmpty`), **API-02** (`AddSimpleSettings` exposes `ISettingsCollection`). - - Insert them as **new Phase 4 "Collection & Validation Binding"** via `/gsd-phase` (use it for safe renumbering): current **Phase 4 (AOT/Trim & Docs) → Phase 5**, current **Phase 5 (beta) → Phase 6**. Beta stays last. - - Then **`/gsd-discuss-phase 4`** → plan → execute (fresh session per phase; `/clear` between). - -## Phase 3 — SHIPPED (in PR #31, awaiting merge) -- **API-01** `SettingsHolder` → `internal sealed`. **PKG-01** `Core.AspNet` package removed (+ dead test ref). **PKG-02** per-TFM `Microsoft.Extensions.*` floors (net8: `Configuration` 8.0.0, `DependencyInjection.Abstractions` 8.0.2; net10 10.0.9) + restore-time NU1901–1904 audit gate. **SRC-02** CLI binder: space-separated `--k v` + quoted-value binding, `SkipFirstArgument` (default **false**), `AddCommandLine()` → `GetCommandLineArgs()` owns exe-skip. -- **D-05 was refined mid-planning (owner-approved):** the exe-skip is split by entry point (NOT a shared default-true) — see `03-CONTEXT.md` D-05. -- **Owner follow-up (not automated):** unlist the published `Core.AspNet` `2.0.0-alpha.0.*` prereleases on NuGet.org (guy-lud account). - -## New engine phase — client requirements (APPROVED; to formalize) -Source detail + provisional IDs + file locations + load-bearing constraints: **`.planning/backlog/client-requirements-pre-beta.md`**. Summary: -- **COLL-02 / COLL-03** pull deferred **COLL-01** forward and expand it (empty-collection default; sequence binding). **VAL-01** is the held **D1 Validations**. -- **Load-bearing constraints to preserve when planning:** (a) comma-scalar collection binding MUST keep working (prod env-var overrides `MultiHost__CommonHosts` depend on it); (b) **COLL-03 changes `ConfigurationBinder.BindPropertySettings`** — the redaction-critical method Phase 3 deliberately left untouched — so the **S1/SEC-01 secret-redaction invariant MUST be re-verified** when it lands; (c) collection converter chain must still run per element (int[]/enum arrays). -- Engine files in scope (different from Phase 3's CLI files): `TypeConverter.ConvertValue` / `ValidateNullAcceptance`, `ConfigurationBinder.BindPropertySettings`, `SettingsPropertyAttribute`/`ISettingValidation`, the `AddSimpleSettings` DI extension. - -## Remaining roadmap (after the insert/renumber) -- **Phase 4 (NEW, engine):** COLL-02, COLL-03, VAL-01, VAL-02, API-02. -- **Phase 5 (was 4):** AOT-01 (reflection/AOT-trim annotations), DOC-01 (README refresh — MUST add the review's "spaced secrets now bind via `AddCommandLine`" migration note + the Phase-3 breaking-change list). -- **Phase 6 (was 5):** REL-01 (cut the first `v2.0.0-beta`; suite green net8 + net10). -- **Still held:** D2 EqualityComparerCreator. +1. **Verify git state:** on branch **`gsd/phase-4-wave-3`** (fresh off master, local-only, holds this handoff); `master` @ `1388c5c`; `git status` clean; `gh pr list` shows no open Phase-4 PRs (#33/#34 merged). +2. **Next work = Wave 3 / plan 04-04** (below), then verify → secure → mark Phase 4 complete. +3. **Push/PR only via `guy-lud`** (`gh auth switch --user guy-lud` … then switch back to `guy-frontegg`). `git push` uses the `github-guy-lud` SSH alias. See `[[simplesettings-push-access]]`. + +## What shipped this session (on master @ 1388c5c) +- **Review comment #3 RESOLVED:** the object-level validator now rides on **`SettingsSectionAttribute.ValidatorType`** (`SettingsValidatorAttribute` deleted — it was unreleased). `ValuesPopulator.GetOrBuildPlan` reads it there; dispatch + redaction unchanged. + - **Design consequence (now a fact):** `[SettingsSection]` is also the scan-discovery marker, so **attaching an object validator makes a type scan-discoverable** (validate ⇒ discoverable). The deliberately-failing validation fixtures therefore moved to a **new never-scanned test library `ExistForAll.SimpleSettings.UnitTests.Fixtures`** (public types; UnitTests references it but never passes it to `ScanAssemblies`). A regression test proves a `[SettingsSection(ValidatorType=…)]` type is validated under the scan path. + - **Why the fixtures are public, not internal:** the Reflection.Emit proxy generator emits into a **random-GUID dynamic assembly that cannot implement an internal interface** (`TypeLoadException`) — settings interfaces resolved through the library MUST be public. Empirically proven. See `[[simplesettings-proxy-internal-interface]]`. +- **All 4 PR #33 review comments answered** (ConfigurationBinder `TrySetChildSequence` rationale; PropertyConversion double-`if`; attribute reuse; ValuesPopulator cast). +- **Waves 1–2** (COLL-01/02/03, VAL-01 core, VAL-02) all on master. Full suite **143/143** on net10; build clean net8 + net10. + +## Wave 3 — plan 04-04 (NEXT code work) — VAL-01 DI path + API-02 +Delivers `ISettingsCollection` DI singleton + `out`-overload of `AddSimpleSettings`; a deferred DI validator runner (`ValidateSimpleSettings()`, fresh `CreateScope()` per S-1); shared `ThrowIfAny` (S-3). +- ⚠ **04-04-PLAN is DOUBLY superseded — do NOT follow it literally:** + 1. It describes **reflective** dispatch (`GetMethod("Validate")` + `MakeGenericType`). Use the **DIM bridge** instead: `((ISettingsValidator)validator).Validate(new ValidationContext(instance))`, wrapped in the MED-3 value-free guard (`SettingsValidatorInvocationException`) — same as the core path. + 2. It predates comment #3: the object validator is now read from **`SettingsSectionAttribute.ValidatorType`**, not a separate attribute. The DI runner must read it the same way. + Keep the fresh-scope resolution + the shared `ThrowIfAny`. +- **New cadence for landing it:** this branch is a clean descendant of master (which now has `.planning`), so the **Wave-3 PR includes src + `.planning` together** — the old src-only cherry-pick dance is retired (it was only needed while `.planning` wasn't on master). After merge: phase **verify** (`gsd-verifier`), **security gate** (`/gsd-secure-phase 4`, D-06 sign-off), mark complete. + +## Cadence / mechanics (updated) +- **PR #33 was merged mid-phase** (owner shipped Waves 1–2 standalone). The old "keep ONE evolving draft PR, stop after each wave" cadence is **retired**. Going forward: **fresh branch off master per chunk → PR (src + .planning) → merge.** Each master merge burns an alpha (pre-stable, acceptable). +- **The handoff lives on the work branch, not master** — a handoff-only master push would burn a doc-only alpha. It rides to master with the next code PR. This wrap committed it on `gsd/phase-4-wave-3`. See `[[simplesettings-handoff-workflow]]`. +- **Subagents run async in the background** (notify on completion); executors run sequentially on the branch (no worktree isolation in this harness). Spot-check commits + SUMMARY + clean tree + build/test before re-dispatching — stalls can be cosmetic. + +## STANDING review rule (memory `[[dotnet-review-workflow]]`) +Before EACH PR: review code with **both** the `gsd-code-review` skill AND `dotnet-claude-kit:code-review`, and review tests with the **`dotnet-claude-kit:test-engineer`** agent. Plan-review trio (architect/perf/security) up front before writing code. Be proportional — skip for trivial docs/cosmetic PRs (e.g. #34). + +## Remaining roadmap +- **Phase 4:** Wave 3 (04-04) + verify + secure + complete. +- **Phase 5:** AOT-01; **DOC-01 (README)** — MUST add: Phase-3 breaking-change list, "spaced secrets bind via `AddCommandLine`", VAL-01 validator-author-must-not-echo-secrets, DI-path `ValidateSimpleSettings()` is opt-in, **and NEW: the validate ⇒ discoverable coupling** (validating via `[SettingsSection(ValidatorType=…)]` also makes the type scan-discovered). Fold in **validator-dispatch caching** (code-review M2, deferred perf). +- **Phase 6:** REL-01 (first `v2.0.0-beta`; suite green net8 + net10). +- **Held:** EQ-01 (D2 EqualityComparerCreator). **Deferred:** PERF-03 (compiled setter); `${ENV:-}` placeholder detection (VAL-02, D-13). ## How releasing works (durable) -- **`ci.yml`** — PRs to `master`: build + test (net8.0 + net10.0). **`release.yml`**: push to `master` → auto-publishes a MinVer height-based `-alpha` (NO `paths` filter — every push, incl. docs, publishes). Manual **Release** (`workflow_dispatch`, channel beta/rc/stable + bump) tags `v*`. -- **`benchmark.yml`** — push to `master` + PRs: BDN, gates PRs on allocation regressions (baseline in `gh-pages`). -- **Versioning = MinVer**, tag prefix `v`, baseline **2.0.0**, keyless NuGet Trusted Publishing (OIDC). Workflows use `SOLUTION=SimpleSettings.slnx`. **Everything goes through PRs.** +- **`ci.yml`** — PRs to `master`: build + test (net8.0 + net10.0). **`release.yml`**: push to `master` → auto-publishes a MinVer height-based `-alpha` (NO `paths` filter — **every push publishes an alpha**). Manual **Release** (`workflow_dispatch`) tags `v*`. +- **`benchmark.yml`** — push to `master` + PRs: BDN, gates PRs on **allocation** regressions (baseline in `gh-pages`). Green through #34. +- **Versioning = MinVer**, tag prefix `v`, baseline **2.0.0**, keyless NuGet Trusted Publishing (OIDC). `SOLUTION=SimpleSettings.slnx`. **Everything through PRs; never commit to `master` directly.** ## Gotchas a new session MUST know -- **TUnit test filtering:** `dotnet test --filter "*Name*"` is **rejected** by Microsoft.Testing.Platform/TUnit — exits **5**, zero tests (looks green at a glance). Use `--treenode-filter "/*/*/ClassNameTests/*"` or run unfiltered. See `[[simplesettings-test-stack]]`. -- **Run `dotnet` from `src/`** (global.json → Microsoft.Testing.Platform). net10 runtime only locally (both TFM test DLLs run on it); net8 runs natively in CI. Don't `cd ` before `dotnet`. -- **Pushing / PRs:** active `git`/`gh` identity (`guy-frontegg`) is **read-only** here; push/PR/merge via **`guy-lud`**. `origin` uses SSH alias **`github-guy-lud`** → `git push` already uses guy-lud. For `gh` writes: `gh auth switch --user guy-lud`, do the write, then `gh auth switch --user guy-frontegg`. See `[[simplesettings-push-access]]`. -- **Never commit docs to `master`** (release.yml has no `paths` filter → burns an alpha). Wrap ritual: refresh THIS file on the current work branch (now committed on `gsd/phase-4-…`). See `[[simplesettings-handoff-workflow]]`. -- **GSD branching:** `config.json` `branching_strategy: none` → `/gsd-execute-phase` commits on the *current* branch. Keep each phase on its own feature branch; never execute on `master`. See `[[simplesettings-gsd-source-of-truth]]`. -- **Review workflow (`[[dotnet-review-workflow]]`):** plan → review the plan with `dotnet-architect`/`performance-analyst`/`security-auditor` → implement → review finished code with `code-reviewer`. Phase 3 ran all of these; the architect caught a HIGH false-green (a vacuous verify), so **take the specialist reviews seriously — they earn their cost.** -- **VALIDATION.md gotcha (`[[gsd-plan-phase-validation-md]]`):** with Nyquist on, `/gsd-plan-phase` §5.5 writes only VALIDATION.md frontmatter → the plan-checker BLOCKS on the stub. Populate its body from RESEARCH's "## Validation Architecture" before the checker runs. -- Commits/PRs here **omit** the Co-Authored-By / Generated-with trailer (project preference). +- **TUnit test filtering:** `dotnet test --filter "*Name*"` exits **5** with zero tests (looks green). Use `--treenode-filter "/*/*/ClassNameTests/*"` or run unfiltered. See `[[simplesettings-test-stack]]`. +- **Run `dotnet` from `src/`** (global.json → Microsoft.Testing.Platform). net10 runtime only locally; net8 in CI. Build first, then `dotnet test SimpleSettings.slnx -c Release -f net10.0 --no-build`. +- **Solution-wide `-f net8.0` build trips `NETSDK1005`** on the net10-only `Benchmark`. Use `dotnet build SimpleSettings.slnx -c Release` (no `-f`). +- **Pushing/PRs:** active `guy-frontegg` is **read-only**; push/PR/merge via **`guy-lud`**. See `[[simplesettings-push-access]]`. +- **Never commit to `master` directly.** Wrap ritual: refresh THIS file on the current work branch. See `[[simplesettings-handoff-workflow]]`. +- **GSD branching:** `branching_strategy: none` → executors commit on the *current* branch. Stay on `gsd/phase-4-wave-3`. See `[[simplesettings-gsd-source-of-truth]]`. +- **`pre-bash-guard` silently blocks `git reset --hard`.** Use temp-branch + merge / `-s ours` / `--ff-only`. See `[[simplesettings-bash-guard]]`. +- **Settings interfaces MUST be public** — the proxy generator can't implement internal interfaces. See `[[simplesettings-proxy-internal-interface]]`. +- Commits/PRs **omit** the Co-Authored-By / Generated-with trailer (project preference). ## Key decisions & context (carry forward) -- **Exception-redaction invariant (S1+C2, locked).** `SettingsPropertyValueException` never carries the bound value / never chains an inner; `SettingsBindingException` stores primitives; `SettingsPropertyNullException` = value-free "required missing". Don't weaken. **Re-verify for COLL-03 (touches `BindPropertySettings`).** -- **Generator concurrency (ENG-01/T7, #29).** One `_generationGate` over all generation (double-checked lock; warm path lock-free). Don't switch to `Lazy`-per-type. -- **Benchmark tracking gates on ALLOCATIONS, not time** (`gh-pages` baseline). The CLI binder is NOT in the benchmark set. +- **Object validator = `[SettingsSection].ValidatorType`** (this session, merged). One attribute; validate ⇒ discoverable. Core reads it in `ValuesPopulator.GetOrBuildPlan`. +- **Exception-redaction invariant (S1+C2, locked).** `SettingsValidatorInvocationException` = value-free Type-only wrap for a throwing validator; `SettingsValidationException` composes only author `ValidationError` text; null/property exceptions value-free; no inner chaining of anything that saw a value. D-06 approved for the COLL-03 sequence path. Don't weaken. +- **VAL-01 dispatch = DIM bridge, no reflection.** `ISettingValidation` default-implements the base `Validate`. Core + (upcoming) DI runner dispatch via the `ISettingsValidator` cast. +- **`SettingsPlan.HasValidators` zero-alloc short-circuit** protects the validator-free warm path (benchmark gate). Don't add per-populate allocation before it. +- **Generator concurrency (T7, #29):** one `_generationGate` (double-checked lock; warm path lock-free). Don't switch to `Lazy`-per-type. - **Pre-stable window:** no `v*` tag; breaking changes free until the first `v2.0.0-beta`. ## Minor tracked follow-ups (non-blocking) -- **`.claude/`/`.codex/` tracked on `master`** (`8750359`, "remove later"): add to `.gitignore` + `git rm --cached` on a **branch/PR** (never on `master`). -- **REQUIREMENTS.md traceability:** 13 brownfield baseline IDs (`BIND-01…NAME-01`) are in the body but not the traceability table (pre-existing; surfaces in `/gsd-progress` and on `phase.complete`). -- **Phase 2 cosmetic NITs** in `Core/TypeConverterTests.cs`: add `using System;`; optional test rename. -- **Stale remote branch** `origin/chore/gsd-ownership-cutover` (merged) — safe to delete via `guy-lud`. -- **Codebase-map drift:** root files (README, LICENSE, SESSION-HANDOFF.md, …) predate the map — refresh via `/gsd-map-codebase` when convenient (non-blocking). +- **Owner:** unlist published `2.0.0-alpha.0.*` prereleases on NuGet.org (guy-lud) — **grew by 2 this session** (#33, #34 merges). +- **Old branch retired:** `gsd/phase-4-collection-validation-binding` (+ `…-pr`) deleted this wrap — fully superseded by master. If any lingers on origin, delete it. +- **`.claude/`/`.codex/` tracked on `master`**: add to `.gitignore` + `git rm --cached` on a branch/PR (never on `master`). +- **REQUIREMENTS.md traceability:** 13 brownfield baseline IDs (`BIND-01…NAME-01`) in the body but not the traceability table (pre-existing). +- **Codebase-map drift:** root files predate the map — refresh via `/gsd-map-codebase` when convenient. From 08a9c33fe8c7a4340fe353a3d44bb53fdfa4b615 Mon Sep 17 00:00:00 2001 From: guy-lud Date: Wed, 15 Jul 2026 22:07:13 +0300 Subject: [PATCH 2/5] feat: expose ISettingsCollection from AddSimpleSettings (API-02/D-15) Register the built ISettingsCollection as a DI singleton and add an AddSimpleSettings(out ISettingsCollection, Action?) overload that surfaces the same instance while preserving the IServiceCollection fluent chain. --- .../ServicesSettingsBuilderExtensions.cs | 13 +++++- .../AddSimpleSettingsIntegrationTests.cs | 42 +++++++++++++++++++ 2 files changed, 54 insertions(+), 1 deletion(-) diff --git a/src/Core/ExistForAll.SimpleSettings.Extensions.GenericHost/ServicesSettingsBuilderExtensions.cs b/src/Core/ExistForAll.SimpleSettings.Extensions.GenericHost/ServicesSettingsBuilderExtensions.cs index d938ebe..81d6648 100644 --- a/src/Core/ExistForAll.SimpleSettings.Extensions.GenericHost/ServicesSettingsBuilderExtensions.cs +++ b/src/Core/ExistForAll.SimpleSettings.Extensions.GenericHost/ServicesSettingsBuilderExtensions.cs @@ -18,7 +18,15 @@ public static IServiceCollection AddSimpleSettings(this IServiceCollection servi return services; } - private static void IntegrateSimpleSettings(this IServiceCollection services, + public static IServiceCollection AddSimpleSettings(this IServiceCollection services, + out ISettingsCollection settings, + Action? action = null) + { + settings = IntegrateSimpleSettings(services, action); + return services; + } + + private static ISettingsCollection IntegrateSimpleSettings(this IServiceCollection services, Action? action = null) { SettingsBuilderOptions? options = null; @@ -40,7 +48,10 @@ private static void IntegrateSimpleSettings(this IServiceCollection services, services.AddSingleton(settings.Key, settings.Value); } + services.AddSingleton(settingsCollection); services.AddSingleton(new SettingsProvider(settingsCollection, settingsBuilder)); + + return settingsCollection; } } } \ No newline at end of file diff --git a/src/Tests/ExistForAll.SimpleSettings.UnitTests/DependencyInjection/AddSimpleSettingsIntegrationTests.cs b/src/Tests/ExistForAll.SimpleSettings.UnitTests/DependencyInjection/AddSimpleSettingsIntegrationTests.cs index 0847d80..6fb44fe 100644 --- a/src/Tests/ExistForAll.SimpleSettings.UnitTests/DependencyInjection/AddSimpleSettingsIntegrationTests.cs +++ b/src/Tests/ExistForAll.SimpleSettings.UnitTests/DependencyInjection/AddSimpleSettingsIntegrationTests.cs @@ -90,6 +90,48 @@ public async Task AddSimpleSettings_NullAction_Throws() await Assert.That(() => services.AddSimpleSettings(null!)).Throws(); } + [Test] + public async Task AddSimpleSettings_RegistersSettingsCollection_ServingTheContainerInstance() + { + var services = new ServiceCollection(); + services.AddSimpleSettings(o => o.AddAssemblies([typeof(IDiExampleSettings).Assembly])); + + var provider = services.BuildServiceProvider(); + var collection = provider.GetRequiredService(); + + // The collection serves the same startup-built instance the container resolves for the interface. + await Assert.That(ReferenceEquals(collection.GetSettings(), + provider.GetRequiredService())).IsTrue(); + } + + [Test] + public async Task AddSimpleSettings_OutOverload_SurfacesBoundCollection_AndPreservesChain() + { + var collection = new InMemoryCollection(); + collection.Add(Section, nameof(IDiExampleSettings.Name), "out-value"); + + var services = new ServiceCollection(); + var returned = services.AddSimpleSettings(out var settings, o => + { + o.AddAssemblies([typeof(IDiExampleSettings).Assembly]); + o.AddSectionBinder(new InMemoryBinder(collection)); + }); + + await Assert.That(ReferenceEquals(returned, services)).IsTrue(); + await Assert.That(settings.GetSettings().Name).IsEqualTo("out-value"); + } + + [Test] + public async Task AddSimpleSettings_OutOverload_SurfacesSameInstanceAsResolvedSingleton() + { + var services = new ServiceCollection(); + services.AddSimpleSettings(out var settings, o => o.AddAssemblies([typeof(IDiExampleSettings).Assembly])); + + var provider = services.BuildServiceProvider(); + + await Assert.That(ReferenceEquals(settings, provider.GetRequiredService())).IsTrue(); + } + public interface IDiExampleSettings { string Name { get; set; } From 3eb67f5385a2e233d0b7d9887ceaa54e702f0aaf Mon Sep 17 00:00:00 2001 From: guy-lud Date: Wed, 15 Jul 2026 22:11:17 +0300 Subject: [PATCH 3/5] feat: DI-resolved settings validator path (VAL-01 DI path) Add a deferred, opt-in ISettingsValidationRunner resolved via IServiceProvider.ValidateSimpleSettings(). It resolves DI-registered ISettingValidation from a fresh scope (so scoped-dependency validators work), dispatches through the ISettingsValidator cast (the default-interface bridge, no reflection), and aggregates via the shared SettingsValidationException.ThrowIfAny so the thrown contract is identical to the core populate path. A throwing validator surfaces value-free as SettingsValidatorInvocationException (type-only, no bound value, no inner). --- .../ISettingsValidationRunner.cs | 7 + .../ServiceProviderValidationExtensions.cs | 28 ++++ .../ServicesSettingsBuilderExtensions.cs | 1 + .../SettingsValidationRunner.cs | 59 +++++++ .../AddSimpleSettingsIntegrationTests.cs | 145 ++++++++++++++++++ 5 files changed, 240 insertions(+) create mode 100644 src/Core/ExistForAll.SimpleSettings.Extensions.GenericHost/ISettingsValidationRunner.cs create mode 100644 src/Core/ExistForAll.SimpleSettings.Extensions.GenericHost/ServiceProviderValidationExtensions.cs create mode 100644 src/Core/ExistForAll.SimpleSettings.Extensions.GenericHost/SettingsValidationRunner.cs diff --git a/src/Core/ExistForAll.SimpleSettings.Extensions.GenericHost/ISettingsValidationRunner.cs b/src/Core/ExistForAll.SimpleSettings.Extensions.GenericHost/ISettingsValidationRunner.cs new file mode 100644 index 0000000..394db68 --- /dev/null +++ b/src/Core/ExistForAll.SimpleSettings.Extensions.GenericHost/ISettingsValidationRunner.cs @@ -0,0 +1,7 @@ +namespace ExistForAll.SimpleSettings.Extensions.GenericHost +{ + internal interface ISettingsValidationRunner + { + void Validate(); + } +} diff --git a/src/Core/ExistForAll.SimpleSettings.Extensions.GenericHost/ServiceProviderValidationExtensions.cs b/src/Core/ExistForAll.SimpleSettings.Extensions.GenericHost/ServiceProviderValidationExtensions.cs new file mode 100644 index 0000000..5bf7029 --- /dev/null +++ b/src/Core/ExistForAll.SimpleSettings.Extensions.GenericHost/ServiceProviderValidationExtensions.cs @@ -0,0 +1,28 @@ +using Microsoft.Extensions.DependencyInjection; + +namespace ExistForAll.SimpleSettings.Extensions.GenericHost +{ + public static class ServiceProviderValidationExtensions + { + /// + /// Runs every DI-registered against its scanned settings + /// instance and throws a single aggregated if any validator + /// reports an error. + /// + /// + /// This is opt-in and deferred: DI-resolved validators cannot run during AddSimpleSettings because + /// the container is not built yet, so the host must call this explicitly after + /// BuildServiceProvider() (typically at startup). Attribute-declared validators + /// ([SettingsSection(ValidatorType = ...)] and [SettingsProperty(ValidatorType = ...)]) run + /// automatically during binding and do not require this call. Validators are resolved from a fresh scope, + /// so validators with scoped dependencies are supported. + /// + public static IServiceProvider ValidateSimpleSettings(this IServiceProvider provider) + { + if (provider == null) throw new ArgumentNullException(nameof(provider)); + + provider.GetRequiredService().Validate(); + return provider; + } + } +} diff --git a/src/Core/ExistForAll.SimpleSettings.Extensions.GenericHost/ServicesSettingsBuilderExtensions.cs b/src/Core/ExistForAll.SimpleSettings.Extensions.GenericHost/ServicesSettingsBuilderExtensions.cs index 81d6648..7884ea9 100644 --- a/src/Core/ExistForAll.SimpleSettings.Extensions.GenericHost/ServicesSettingsBuilderExtensions.cs +++ b/src/Core/ExistForAll.SimpleSettings.Extensions.GenericHost/ServicesSettingsBuilderExtensions.cs @@ -50,6 +50,7 @@ private static ISettingsCollection IntegrateSimpleSettings(this IServiceCollecti services.AddSingleton(settingsCollection); services.AddSingleton(new SettingsProvider(settingsCollection, settingsBuilder)); + services.AddSingleton(); return settingsCollection; } diff --git a/src/Core/ExistForAll.SimpleSettings.Extensions.GenericHost/SettingsValidationRunner.cs b/src/Core/ExistForAll.SimpleSettings.Extensions.GenericHost/SettingsValidationRunner.cs new file mode 100644 index 0000000..7994da9 --- /dev/null +++ b/src/Core/ExistForAll.SimpleSettings.Extensions.GenericHost/SettingsValidationRunner.cs @@ -0,0 +1,59 @@ +using ExistForAll.SimpleSettings.Validations; +using Microsoft.Extensions.DependencyInjection; + +namespace ExistForAll.SimpleSettings.Extensions.GenericHost +{ + internal sealed class SettingsValidationRunner : ISettingsValidationRunner + { + private readonly ISettingsCollection _settings; + private readonly IServiceScopeFactory _scopeFactory; + + public SettingsValidationRunner(ISettingsCollection settings, IServiceScopeFactory scopeFactory) + { + _settings = settings; + _scopeFactory = scopeFactory; + } + + public void Validate() + { + // Resolve validators from a fresh scope so a validator (or an injected dependency) that is scoped + // does not trip the "cannot resolve scoped service from root provider" guard under scope validation. + using var scope = _scopeFactory.CreateScope(); + var provider = scope.ServiceProvider; + + var errors = new List(); + + foreach (var pair in _settings) + { + var validatorType = typeof(ISettingValidation<>).MakeGenericType(pair.Key); + + foreach (var validator in provider.GetServices(validatorType)) + { + if (validator is null) + continue; + + ValidationResult result; + try + { + // Dispatch through the non-generic ISettingsValidator; ISettingValidation's default + // implementation forwards to the author's generic overload, so no reflection is needed. + result = ((ISettingsValidator)validator).Validate(new ValidationContext(pair.Value)); + } + catch (Exception e) + { + // 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. See S1. + throw new SettingsValidatorInvocationException(validator.GetType(), e.GetType()); + } + + foreach (var error in result.Errors) + errors.Add(error); + } + } + + // Aggregate and throw through the same shared helper the core populate path uses, so the DI-path + // exception is contract-identical (type + Errors + value-free message). + SettingsValidationException.ThrowIfAny(errors); + } + } +} diff --git a/src/Tests/ExistForAll.SimpleSettings.UnitTests/DependencyInjection/AddSimpleSettingsIntegrationTests.cs b/src/Tests/ExistForAll.SimpleSettings.UnitTests/DependencyInjection/AddSimpleSettingsIntegrationTests.cs index 6fb44fe..224f271 100644 --- a/src/Tests/ExistForAll.SimpleSettings.UnitTests/DependencyInjection/AddSimpleSettingsIntegrationTests.cs +++ b/src/Tests/ExistForAll.SimpleSettings.UnitTests/DependencyInjection/AddSimpleSettingsIntegrationTests.cs @@ -1,5 +1,6 @@ using ExistForAll.SimpleSettings.Binder; using ExistForAll.SimpleSettings.Extensions.GenericHost; +using ExistForAll.SimpleSettings.Validations; using Microsoft.Extensions.DependencyInjection; namespace ExistForAll.SimpleSettings.UnitTests.DependencyInjection @@ -132,9 +133,153 @@ public async Task AddSimpleSettings_OutOverload_SurfacesSameInstanceAsResolvedSi await Assert.That(ReferenceEquals(settings, provider.GetRequiredService())).IsTrue(); } + [Test] + public async Task ValidateSimpleSettings_WhenDiValidatorFails_ThrowsAggregatedException() + { + var services = new ServiceCollection(); + services.AddSimpleSettings(o => o.AddAssemblies([typeof(IDiValidatedSettings).Assembly])); + services.AddSingleton(); + services.AddSingleton, FailingDiValidator>(); + + var provider = services.BuildServiceProvider(); + + var exception = await Assert.That(() => provider.ValidateSimpleSettings()) + .Throws(); + + // Same contract as the core path: an aggregated SettingsValidationException carrying the errors. + await Assert.That(exception!.Errors.Count).IsEqualTo(1); + await Assert.That(exception.Errors[0].ErrorMessage).IsEqualTo("di validation failed"); + } + + [Test] + public async Task ValidateSimpleSettings_WhenDiValidatorPasses_DoesNotThrow_AndReturnsProvider() + { + var services = new ServiceCollection(); + services.AddSimpleSettings(o => o.AddAssemblies([typeof(IDiValidatedSettings).Assembly])); + services.AddSingleton, PassingDiValidator>(); + + var provider = services.BuildServiceProvider(); + + var result = provider.ValidateSimpleSettings(); + + await Assert.That(ReferenceEquals(result, provider)).IsTrue(); + } + + [Test] + public async Task AddSimpleSettings_DoesNotRunDiValidators_UntilValidateIsCalled() + { + var services = new ServiceCollection(); + services.AddSimpleSettings(o => o.AddAssemblies([typeof(IDiValidatedSettings).Assembly])); + services.AddSingleton(); + services.AddSingleton, FailingDiValidator>(); + + // AddSimpleSettings and BuildServiceProvider must complete without running the DI validator. + var provider = services.BuildServiceProvider(); + + await Assert.That(() => provider.ValidateSimpleSettings()).Throws(); + } + + [Test] + public async Task ValidateSimpleSettings_ResolvesValidatorFromFreshScope_ForScopedDependencies() + { + var services = new ServiceCollection(); + services.AddSimpleSettings(o => o.AddAssemblies([typeof(IScopedValidatedSettings).Assembly])); + services.AddScoped(); + services.AddScoped, ScopedDependentValidator>(); + + // Under scope validation, resolving the scoped validator from the root provider would throw; the + // runner resolves it from a fresh scope, so this succeeds. + var provider = services.BuildServiceProvider(validateScopes: true); + + var result = provider.ValidateSimpleSettings(); + + await Assert.That(ReferenceEquals(result, provider)).IsTrue(); + } + + [Test] + public async Task ValidateSimpleSettings_WhenValidatorThrows_RedactsBoundValue() + { + const string secret = "REDACTION-SENTINEL-8F3A2C"; + var config = new InMemoryCollection(); + config.Add(nameof(IDiSecretSettings).Substring(1), nameof(IDiSecretSettings.ApiKey), secret); + + var services = new ServiceCollection(); + services.AddSimpleSettings(o => + { + o.AddAssemblies([typeof(IDiSecretSettings).Assembly]); + o.AddSectionBinder(new InMemoryBinder(config)); + }); + services.AddSingleton, ThrowingDiValidator>(); + + var provider = services.BuildServiceProvider(); + + // A throwing validator surfaces value-free: the invocation exception carries only type names. + var exception = await Assert.That(() => provider.ValidateSimpleSettings()) + .Throws(); + + await Assert.That(exception!.ToString().Contains(secret)).IsFalse(); + } + public interface IDiExampleSettings { string Name { get; set; } } + + public interface IDiValidatedSettings + { + string Name { get; set; } + } + + public interface IScopedValidatedSettings + { + string Name { get; set; } + } + + public interface IDiSecretSettings + { + string ApiKey { get; set; } + } + + public sealed class ValidatorDependency + { + } + + public sealed class ScopedDependency + { + } + + public sealed class FailingDiValidator : ISettingValidation + { + public FailingDiValidator(ValidatorDependency dependency) + { + } + + public ValidationResult Validate(ValidationContext context) + { + var result = new ValidationResult(); + result.AddError(new ValidationError(nameof(IDiValidatedSettings.Name), "di validation failed")); + return result; + } + } + + public sealed class PassingDiValidator : ISettingValidation + { + public ValidationResult Validate(ValidationContext context) => new(); + } + + public sealed class ScopedDependentValidator : ISettingValidation + { + public ScopedDependentValidator(ScopedDependency dependency) + { + } + + public ValidationResult Validate(ValidationContext context) => new(); + } + + public sealed class ThrowingDiValidator : ISettingValidation + { + public ValidationResult Validate(ValidationContext context) + => throw new InvalidOperationException($"boom with {context.Settings!.ApiKey}"); + } } } From c2b4f2ced3f00e6918903452f79b404900902bab Mon Sep 17 00:00:00 2001 From: guy-lud Date: Wed, 15 Jul 2026 22:25:08 +0300 Subject: [PATCH 4/5] refactor: address Wave 3 review findings - ValidateSimpleSettings surfaces a caller-facing error when AddSimpleSettings was not called (internal runner type no longer leaks into the message). - Trim runner comments to one line each; drop internal planning-ID reference. - Harden tests: deferral test is now an invocation-counter timing probe (0 runs at build, 1 on the explicit call); scope test asserts the scoped validator actually executed; redaction test pins that the secret is bound; add no-op (no DI validators) and misuse coverage. --- .../ServiceProviderValidationExtensions.cs | 6 +- .../SettingsValidationRunner.cs | 12 ++-- .../AddSimpleSettingsIntegrationTests.cs | 70 ++++++++++++++++--- 3 files changed, 70 insertions(+), 18 deletions(-) diff --git a/src/Core/ExistForAll.SimpleSettings.Extensions.GenericHost/ServiceProviderValidationExtensions.cs b/src/Core/ExistForAll.SimpleSettings.Extensions.GenericHost/ServiceProviderValidationExtensions.cs index 5bf7029..a9c25fa 100644 --- a/src/Core/ExistForAll.SimpleSettings.Extensions.GenericHost/ServiceProviderValidationExtensions.cs +++ b/src/Core/ExistForAll.SimpleSettings.Extensions.GenericHost/ServiceProviderValidationExtensions.cs @@ -21,7 +21,11 @@ public static IServiceProvider ValidateSimpleSettings(this IServiceProvider prov { if (provider == null) throw new ArgumentNullException(nameof(provider)); - provider.GetRequiredService().Validate(); + var runner = provider.GetService() + ?? throw new InvalidOperationException( + "ValidateSimpleSettings requires AddSimpleSettings to have been called during service registration."); + + runner.Validate(); return provider; } } diff --git a/src/Core/ExistForAll.SimpleSettings.Extensions.GenericHost/SettingsValidationRunner.cs b/src/Core/ExistForAll.SimpleSettings.Extensions.GenericHost/SettingsValidationRunner.cs index 7994da9..a5b7135 100644 --- a/src/Core/ExistForAll.SimpleSettings.Extensions.GenericHost/SettingsValidationRunner.cs +++ b/src/Core/ExistForAll.SimpleSettings.Extensions.GenericHost/SettingsValidationRunner.cs @@ -16,8 +16,7 @@ public SettingsValidationRunner(ISettingsCollection settings, IServiceScopeFacto public void Validate() { - // Resolve validators from a fresh scope so a validator (or an injected dependency) that is scoped - // does not trip the "cannot resolve scoped service from root provider" guard under scope validation. + // Resolve from a fresh scope so validators (or their dependencies) that are scoped resolve correctly. using var scope = _scopeFactory.CreateScope(); var provider = scope.ServiceProvider; @@ -35,14 +34,12 @@ public void Validate() ValidationResult result; try { - // Dispatch through the non-generic ISettingsValidator; ISettingValidation's default - // implementation forwards to the author's generic overload, so no reflection is needed. + // Dispatch through ISettingsValidator; its default implementation forwards to the typed overload. result = ((ISettingsValidator)validator).Validate(new ValidationContext(pair.Value)); } catch (Exception e) { - // 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. See S1. + // A validator threw: surface value-free (types only) — the inner may embed a secret, never chain it. throw new SettingsValidatorInvocationException(validator.GetType(), e.GetType()); } @@ -51,8 +48,7 @@ public void Validate() } } - // Aggregate and throw through the same shared helper the core populate path uses, so the DI-path - // exception is contract-identical (type + Errors + value-free message). + // Aggregate through the shared helper so the DI-path exception is contract-identical to the core path. SettingsValidationException.ThrowIfAny(errors); } } diff --git a/src/Tests/ExistForAll.SimpleSettings.UnitTests/DependencyInjection/AddSimpleSettingsIntegrationTests.cs b/src/Tests/ExistForAll.SimpleSettings.UnitTests/DependencyInjection/AddSimpleSettingsIntegrationTests.cs index 224f271..0f1c355 100644 --- a/src/Tests/ExistForAll.SimpleSettings.UnitTests/DependencyInjection/AddSimpleSettingsIntegrationTests.cs +++ b/src/Tests/ExistForAll.SimpleSettings.UnitTests/DependencyInjection/AddSimpleSettingsIntegrationTests.cs @@ -168,32 +168,41 @@ public async Task ValidateSimpleSettings_WhenDiValidatorPasses_DoesNotThrow_AndR [Test] public async Task AddSimpleSettings_DoesNotRunDiValidators_UntilValidateIsCalled() { + var counter = new InvocationCounter(); var services = new ServiceCollection(); services.AddSimpleSettings(o => o.AddAssemblies([typeof(IDiValidatedSettings).Assembly])); - services.AddSingleton(); - services.AddSingleton, FailingDiValidator>(); + services.AddSingleton(counter); + services.AddSingleton, CountingDiValidator>(); - // AddSimpleSettings and BuildServiceProvider must complete without running the DI validator. var provider = services.BuildServiceProvider(); - await Assert.That(() => provider.ValidateSimpleSettings()).Throws(); + // AddSimpleSettings and BuildServiceProvider must complete without running the DI validator. + await Assert.That(counter.Count).IsEqualTo(0); + + provider.ValidateSimpleSettings(); + + // The explicit call runs it exactly once. + await Assert.That(counter.Count).IsEqualTo(1); } [Test] public async Task ValidateSimpleSettings_ResolvesValidatorFromFreshScope_ForScopedDependencies() { + var counter = new InvocationCounter(); var services = new ServiceCollection(); services.AddSimpleSettings(o => o.AddAssemblies([typeof(IScopedValidatedSettings).Assembly])); + services.AddSingleton(counter); services.AddScoped(); services.AddScoped, ScopedDependentValidator>(); - // Under scope validation, resolving the scoped validator from the root provider would throw; the - // runner resolves it from a fresh scope, so this succeeds. + // Resolving the scoped validator from the root provider would throw under scope validation; the runner + // resolves it from a fresh scope, so this succeeds and the scoped validator actually runs. var provider = services.BuildServiceProvider(validateScopes: true); var result = provider.ValidateSimpleSettings(); await Assert.That(ReferenceEquals(result, provider)).IsTrue(); + await Assert.That(counter.Count).IsEqualTo(1); } [Test] @@ -213,6 +222,9 @@ public async Task ValidateSimpleSettings_WhenValidatorThrows_RedactsBoundValue() var provider = services.BuildServiceProvider(); + // Guard against a vacuous pass: the secret must actually be bound onto the instance the validator reads. + await Assert.That(provider.GetRequiredService().ApiKey).IsEqualTo(secret); + // A throwing validator surfaces value-free: the invocation exception carries only type names. var exception = await Assert.That(() => provider.ValidateSimpleSettings()) .Throws(); @@ -220,6 +232,24 @@ public async Task ValidateSimpleSettings_WhenValidatorThrows_RedactsBoundValue() await Assert.That(exception!.ToString().Contains(secret)).IsFalse(); } + [Test] + public async Task ValidateSimpleSettings_WithNoDiValidators_IsNoOp_AndReturnsProvider() + { + var services = new ServiceCollection(); + services.AddSimpleSettings(o => o.AddAssemblies([typeof(IDiExampleSettings).Assembly])); + var provider = services.BuildServiceProvider(); + + await Assert.That(ReferenceEquals(provider.ValidateSimpleSettings(), provider)).IsTrue(); + } + + [Test] + public async Task ValidateSimpleSettings_WithoutAddSimpleSettings_Throws() + { + var provider = new ServiceCollection().BuildServiceProvider(); + + await Assert.That(() => provider.ValidateSimpleSettings()).Throws(); + } + public interface IDiExampleSettings { string Name { get; set; } @@ -248,6 +278,11 @@ public sealed class ScopedDependency { } + public sealed class InvocationCounter + { + public int Count { get; set; } + } + public sealed class FailingDiValidator : ISettingValidation { public FailingDiValidator(ValidatorDependency dependency) @@ -267,13 +302,30 @@ public sealed class PassingDiValidator : ISettingValidation context) => new(); } - public sealed class ScopedDependentValidator : ISettingValidation + public sealed class CountingDiValidator : ISettingValidation { - public ScopedDependentValidator(ScopedDependency dependency) + private readonly InvocationCounter _counter; + + public CountingDiValidator(InvocationCounter counter) => _counter = counter; + + public ValidationResult Validate(ValidationContext context) { + _counter.Count++; + return new(); } + } + + public sealed class ScopedDependentValidator : ISettingValidation + { + private readonly InvocationCounter _counter; + + public ScopedDependentValidator(ScopedDependency dependency, InvocationCounter counter) => _counter = counter; - public ValidationResult Validate(ValidationContext context) => new(); + public ValidationResult Validate(ValidationContext context) + { + _counter.Count++; + return new(); + } } public sealed class ThrowingDiValidator : ISettingValidation From 11fc7980b16bab1daccbfd77884560d67d609542 Mon Sep 17 00:00:00 2001 From: guy-lud Date: Wed, 15 Jul 2026 22:28:58 +0300 Subject: [PATCH 5/5] docs(planning): Phase 4 Wave 3 (04-04) summary, review, plan patch, state Record the VAL-01 DI path + API-02 execution: 04-04-SUMMARY.md, the gsd-code-reviewer 04-04-REVIEW.md, STATE.md (all 5 Phase 4 plans landed; verify+secure pending), and patch the plan's superseded reflective-dispatch text to the shipped default-interface-bridge dispatch. --- .planning/STATE.md | 18 +- .../04-04-PLAN.md | 18 +- .../04-04-REVIEW.md | 98 +++++++++ .../04-04-SUMMARY.md | 191 ++++++++++++++++++ 4 files changed, 309 insertions(+), 16 deletions(-) create mode 100644 .planning/phases/04-collection-validation-binding/04-04-REVIEW.md create mode 100644 .planning/phases/04-collection-validation-binding/04-04-SUMMARY.md diff --git a/.planning/STATE.md b/.planning/STATE.md index 08997a2..2e0c50a 100644 --- a/.planning/STATE.md +++ b/.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 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 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 diff --git a/.planning/phases/04-collection-validation-binding/04-04-PLAN.md b/.planning/phases/04-collection-validation-binding/04-04-PLAN.md index 911dd84..ea7d483 100644 --- a/.planning/phases/04-collection-validation-binding/04-04-PLAN.md +++ b/.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, ValidationContext, ValidationResult, ValidationError, SettingsValidationException) — no Core InternalsVisibleTo needed." - "The DI runner resolves ISettingValidation 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 via MakeGenericType (avoids AmbiguousMatchException — review S-2)." + - "SUPERSEDED (architect+security review): validator dispatch uses the ISettingValidation default-interface bridge — cast to ISettingsValidator and call Validate(new ValidationContext(instance)); NO reflection over Validate and no generic ValidationContext 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 : 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` default-implements the base `Validate`, so there is NO reflection over the `Validate` + methods (`GetMethod`/`MethodInfo.Invoke`) and NO generic `ValidationContext` 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 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` 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. diff --git a/.planning/phases/04-collection-validation-binding/04-04-REVIEW.md b/.planning/phases/04-collection-validation-binding/04-04-REVIEW.md new file mode 100644 index 0000000..d41bdd2 --- /dev/null +++ b/.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` 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` default-interface bridge forwards to the author overload. Cast is + provably safe (services resolved for `ISettingValidation` are `ISettingsValidator`). +- **Non-null aggregate:** `errors` is eagerly `new List()`, 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`). + +## 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_ diff --git a/.planning/phases/04-collection-validation-binding/04-04-SUMMARY.md b/.planning/phases/04-collection-validation-binding/04-04-SUMMARY.md new file mode 100644 index 0000000..5c054ed --- /dev/null +++ b/.planning/phases/04-collection-validation-binding/04-04-SUMMARY.md @@ -0,0 +1,191 @@ +--- +phase: 04-collection-validation-binding +plan: 04 +subsystem: dependency-injection +tags: [dependency-injection, validation, di-scope, exception-redaction, dotnet, tunit, api-surface] + +# Dependency graph +requires: + - phase: 04-collection-validation-binding + provides: "Plan 03 sync validation contracts (ISettingValidation DIM bridge, ValidationContext, SettingsValidationException.ThrowIfAny, SettingsValidatorInvocationException)" +provides: + - "ISettingsCollection exposed from AddSimpleSettings: registered as a DI singleton AND surfaced via an AddSimpleSettings(out ISettingsCollection, Action?) overload (API-02/D-15)" + - "Deferred, opt-in DI-resolved validator path: IServiceProvider.ValidateSimpleSettings() runs DI-registered ISettingValidation from a fresh scope and throws the same aggregated SettingsValidationException as the core path (VAL-01 DI path / D-11)" +affects: [05-aot-docs, DOC-01] + +# Tech tracking +tech-stack: + added: [] + patterns: + - "DI validator dispatch via the ISettingValidation default-interface bridge (cast to ISettingsValidator) — no reflection over Validate; mirrors core ValuesPopulator.InvokeValidator" + - "Fresh-scope resolution via IServiceScopeFactory.CreateScope() so scoped-dependency validators resolve under validateScopes:true (review S-1)" + - "Shared SettingsValidationException.ThrowIfAny so the DI-path thrown contract is identical to the core path (review S-3)" + - "Deferred opt-in runner (resolvable + IServiceProvider extension) — Abstractions-only, no IHostedService/Options dependency (Q3)" + +key-files: + created: + - src/Core/ExistForAll.SimpleSettings.Extensions.GenericHost/ISettingsValidationRunner.cs + - src/Core/ExistForAll.SimpleSettings.Extensions.GenericHost/SettingsValidationRunner.cs + - src/Core/ExistForAll.SimpleSettings.Extensions.GenericHost/ServiceProviderValidationExtensions.cs + modified: + - src/Core/ExistForAll.SimpleSettings.Extensions.GenericHost/ServicesSettingsBuilderExtensions.cs + - src/Tests/ExistForAll.SimpleSettings.UnitTests/DependencyInjection/AddSimpleSettingsIntegrationTests.cs + +key-decisions: + - "Dispatch uses the DIM bridge, NOT reflection (supersedes the plan's S-2 text): the runner casts each resolved validator to ISettingsValidator and calls Validate(new ValidationContext(instance)); GetServices resolves via MakeGenericType(ISettingValidation<>). Byte-identical to the shipped core InvokeValidator." + - "The DI runner reads NO attribute: attribute-declared object/property validators ([SettingsSection].ValidatorType, [SettingsProperty].ValidatorType) are the core path and already run inline during AddSimpleSettings/ScanAssemblies. The DI path is additive (D-11): it only resolves DI-registered ISettingValidation." + - "ISettingsValidationRunner + impl are internal (architect nice-to-have): the only consumer entry point is the public ValidateSimpleSettings(); trims public surface pre-beta. Runner registered/resolved within GenericHost, no InternalsVisibleTo needed." + - "Runner injects IServiceScopeFactory (not IServiceProvider) — states the create-scopes intent, avoids the service-locator smell, still Abstractions-only (architect nice-to-have)." + - "Error accumulator is eagerly allocated (not the core path's lazy-null) and passed non-null to ThrowIfAny, which ArgumentNullExceptions on null; the deferred path is not hot so the lazy optimization is unneeded (architect required)." + - "ValidateSimpleSettings resolves the runner via GetService + a caller-facing InvalidOperationException when AddSimpleSettings was not called (dotnet-kit review S1) — the internal runner type no longer leaks into the misuse message." + +patterns-established: + - "Deferred opt-in validation entry point on IServiceProvider (post-BuildServiceProvider); attribute validators stay inline, DI validators run on the explicit call" + - "Per-test DI validators over plain suffix-discovered fixtures in the UnitTests assembly (never attribute-discovered, so they never run during a sibling test's ScanAssemblies)" + +requirements-completed: [VAL-01, API-02] + +coverage: + - id: A1 + description: "ISettingsCollection is resolvable via GetRequiredService and serves the same instance the container resolves for the interface (API-02/D-15 mechanism 1)" + requirement: "API-02" + verification: + - kind: unit + ref: "src/Tests/ExistForAll.SimpleSettings.UnitTests/DependencyInjection/AddSimpleSettingsIntegrationTests.cs#AddSimpleSettings_RegistersSettingsCollection_ServingTheContainerInstance" + status: pass + human_judgment: false + - id: A2 + description: "AddSimpleSettings(out ISettingsCollection, Action?) surfaces the built collection with bound values and preserves the IServiceCollection fluent chain (API-02/D-15 mechanism 2)" + requirement: "API-02" + verification: + - kind: unit + ref: "src/Tests/ExistForAll.SimpleSettings.UnitTests/DependencyInjection/AddSimpleSettingsIntegrationTests.cs#AddSimpleSettings_OutOverload_SurfacesBoundCollection_AndPreservesChain" + status: pass + - kind: unit + ref: "src/Tests/ExistForAll.SimpleSettings.UnitTests/DependencyInjection/AddSimpleSettingsIntegrationTests.cs#AddSimpleSettings_OutOverload_SurfacesSameInstanceAsResolvedSingleton" + status: pass + human_judgment: false + - id: A3 + description: "A DI-registered failing ISettingValidation (constructor-injected) throws an aggregated SettingsValidationException carrying the error; a passing one does not throw" + requirement: "VAL-01" + verification: + - kind: unit + ref: "src/Tests/ExistForAll.SimpleSettings.UnitTests/DependencyInjection/AddSimpleSettingsIntegrationTests.cs#ValidateSimpleSettings_WhenDiValidatorFails_ThrowsAggregatedException" + status: pass + - kind: unit + ref: "src/Tests/ExistForAll.SimpleSettings.UnitTests/DependencyInjection/AddSimpleSettingsIntegrationTests.cs#ValidateSimpleSettings_WhenDiValidatorPasses_DoesNotThrow_AndReturnsProvider" + status: pass + human_judgment: false + - id: A4 + description: "DI validators do NOT run during AddSimpleSettings/BuildServiceProvider (counter == 0) and run exactly once on the explicit ValidateSimpleSettings() call (counter == 1) — deferred timing (Pitfall 4)" + requirement: "VAL-01" + verification: + - kind: unit + ref: "src/Tests/ExistForAll.SimpleSettings.UnitTests/DependencyInjection/AddSimpleSettingsIntegrationTests.cs#AddSimpleSettings_DoesNotRunDiValidators_UntilValidateIsCalled" + status: pass + human_judgment: false + - id: A5 + description: "A validator whose constructor injects a SCOPED dependency resolves and executes under BuildServiceProvider(validateScopes:true) — proving fresh-scope resolution (review S-1)" + requirement: "VAL-01" + verification: + - kind: unit + ref: "src/Tests/ExistForAll.SimpleSettings.UnitTests/DependencyInjection/AddSimpleSettingsIntegrationTests.cs#ValidateSimpleSettings_ResolvesValidatorFromFreshScope_ForScopedDependencies" + status: pass + human_judgment: false + - id: A6 + description: "A throwing DI validator whose message embeds a bound secret surfaces as value-free SettingsValidatorInvocationException whose full ToString() omits the secret (D-12/T-04-VAL); the secret is asserted bound first so the test is non-vacuous" + requirement: "VAL-01" + verification: + - kind: unit + ref: "src/Tests/ExistForAll.SimpleSettings.UnitTests/DependencyInjection/AddSimpleSettingsIntegrationTests.cs#ValidateSimpleSettings_WhenValidatorThrows_RedactsBoundValue" + status: pass + human_judgment: false + - id: A7 + description: "ValidateSimpleSettings is a no-op returning the provider when no DI validators are registered; throws an informative error when AddSimpleSettings was never called" + requirement: "VAL-01" + verification: + - kind: unit + ref: "src/Tests/ExistForAll.SimpleSettings.UnitTests/DependencyInjection/AddSimpleSettingsIntegrationTests.cs#ValidateSimpleSettings_WithNoDiValidators_IsNoOp_AndReturnsProvider" + status: pass + - kind: unit + ref: "src/Tests/ExistForAll.SimpleSettings.UnitTests/DependencyInjection/AddSimpleSettingsIntegrationTests.cs#ValidateSimpleSettings_WithoutAddSimpleSettings_Throws" + status: pass + human_judgment: false + +# Metrics +duration: ~35min (incl. plan-review trio + code/test review) +completed: 2026-07-15 +status: complete +--- + +# Phase 04 Plan 04: DI-Resolved Validator Path + ISettingsCollection Exposure + +**`AddSimpleSettings` now hands back the built `ISettingsCollection` (resolvable singleton + `out`-overload), and a deferred, opt-in `IServiceProvider.ValidateSimpleSettings()` runs DI-registered `ISettingValidation` from a fresh scope, throwing the same value-free aggregated `SettingsValidationException` as the core path — completing VAL-01 (DI path) and API-02.** + +## Accomplishments +- **API-02 (D-15):** `IntegrateSimpleSettings` returns the built `ISettingsCollection`; it is registered as `AddSingleton(collection)` and surfaced by a new `AddSimpleSettings(out ISettingsCollection, Action? = null)` overload that preserves the `IServiceCollection` fluent chain. The out param and the DI singleton are the one instance the collection built. +- **VAL-01 DI path (D-11):** a new internal `ISettingsValidationRunner`/`SettingsValidationRunner` and the public `IServiceProvider.ValidateSimpleSettings()` extension run DI-registered `ISettingValidation` in a deferred, opt-in post-provider-build step (the container is not built during `AddSimpleSettings` — Pitfall 4). +- **Fresh-scope resolution (S-1):** the runner resolves validators from `IServiceScopeFactory.CreateScope()`, so a validator with a scoped injected dependency resolves and executes under `BuildServiceProvider(validateScopes: true)`. +- **Contract-identical failure (S-3/D-10/D-12):** the runner dispatches via the `ISettingValidation` default-interface bridge (no reflection), wraps a throwing validator value-free as `SettingsValidatorInvocationException(type, type)` (no bound value, no chained inner), and aggregates through the shared `SettingsValidationException.ThrowIfAny` — identical to the core `ValuesPopulator.InvokeValidator` by construction. + +## Task Commits +1. **Task 1 — expose ISettingsCollection (API-02/D-15)** — `08a9c33` (feat) +2. **Task 2 — DI-resolved validator path (VAL-01 DI path)** — `3eb67f5` (feat) +3. **Review fixes (GSD + dotnet-kit + test-engineer findings)** — `c2b4f2c` (refactor) + +## Files Created/Modified +- `ISettingsValidationRunner.cs` (created) — internal interface, `void Validate()`. +- `SettingsValidationRunner.cs` (created) — internal sealed impl; fresh scope, DIM-bridge dispatch, value-free invocation guard, shared `ThrowIfAny`. +- `ServiceProviderValidationExtensions.cs` (created) — public `ValidateSimpleSettings(this IServiceProvider)` with XML-doc documenting the opt-in/deferred contract; caller-facing error on misuse. +- `ServicesSettingsBuilderExtensions.cs` (modified) — `IntegrateSimpleSettings` returns `ISettingsCollection`; registers `AddSingleton` + `AddSingleton`; new `out`-overload. +- `AddSimpleSettingsIntegrationTests.cs` (modified) — 10 new tests + fixtures (3 API-02, 7 VAL-01 DI path incl. redaction + timing-counter + fresh-scope-execution + no-op + misuse). + +## Decisions Made / Deviations from Plan +The written 04-04-PLAN.md was **superseded in two ways** (documented in the handoff and confirmed by the plan-review trio); the plan text was patched to match what shipped: +1. **Dispatch = DIM bridge, not reflection.** The plan's S-2 text called for `GetMethod("Validate", ...)` + `MakeGenericType` on the context. Instead, the runner casts to `ISettingsValidator` and calls `Validate(new ValidationContext(instance))` — `ISettingValidation` default-implements the base `Validate`, so no reflection over `Validate` is needed (only `MakeGenericType(ISettingValidation<>)` for `GetServices`). This mirrors the post-comment-#3 core path and structurally removes the `TargetInvocationException` inner-chaining leak vector (former REVIEW MED-3). +2. **Object validator lives on `[SettingsSection].ValidatorType` (comment #3).** The DI runner reads no attribute; attribute-declared validators are the core path and already run inline during `ScanAssemblies`. D-11: the DI path is additive. + +Additional refinements adopted from the plan-review trio (all architect/security nice-to-haves or required items): +- `ISettingsValidationRunner` + impl are **internal** (public surface trimmed pre-beta; only `ValidateSimpleSettings()` is public). +- Runner injects **`IServiceScopeFactory`**, not `IServiceProvider`. +- Error accumulator **eagerly allocated** and passed non-null to `ThrowIfAny` (it `ArgumentNullException`s on null). +- `ValidateSimpleSettings` throws a **caller-facing** `InvalidOperationException` (not the raw `GetRequiredService` message naming the internal runner) when `AddSimpleSettings` was not called. + +## Reviews +- **Plan-review trio (up front, on the corrected design):** dotnet-architect — APPROVE-WITH-CHANGES (scope factory, internal, eager list, doc the opt-in, patch the plan); security-auditor — APPROVE-WITH-CHANGES, **T-04-VAL satisfied by construction**, confirmed the exact guard boundary (resolve outside the try; build context + Validate inside; capture `e.GetType()` only, never chain `e`), required a gating DI-path redaction test; performance-analyst — APPROVE, no benchmark-gate risk (warm populate path untouched). +- **Post-implementation review (on the Wave 3 diff):** gsd-code-reviewer — no BLOCKER/HIGH/MEDIUM, 3 LOW (fixed the internal-ID comment + trimmed comments; multi-call last-wins is pre-existing); dotnet-kit code-reviewer — no critical, adopted S1 (caller-facing error) + S2 (pin redaction precondition); dotnet-kit test-engineer — coverage adequate, adopted the invocation-counter timing probe, the scope-execution assertion, the redaction precondition, and the no-op test. Report: `04-04-REVIEW.md`. + +## Carry-forward for Phase 5 / DOC-01 +- **README/API docs MUST document the opt-in caveat:** DI-registered validators run **only** on the explicit `ValidateSimpleSettings()` call after `BuildServiceProvider()`; attribute-declared validators run automatically during binding. (XML-doc is already on `ValidateSimpleSettings`.) +- **Advisory to document (security):** validator **constructors** must not echo injected secrets — the D-12 echo-responsibility extends to the DI ctor surface (resolution runs outside the value-free guard). +- **Validate ⇒ discoverable coupling** (comment #3) still applies to the DI path: a type is validated by the runner only if it is scan-discovered into the `ISettingsCollection`. + +## Deviations from Plan +Dispatch mechanism, runner visibility, scope-factory injection, and eager-list allocation deviate from the literal plan text as described above (superseded; plan text patched). No scope changes; both requirements (VAL-01 DI path, API-02) delivered. + +## Issues Encountered +- Solution-wide `dotnet build -c Release -f net8.0` still reports NETSDK1005 on the net10-only Benchmark (pre-existing, out of scope). Authoritative `dotnet build SimpleSettings.slnx -c Release` (no `-f`) succeeds for net8.0 + net10.0 with 0 warnings. + +## Verification +- `cd src && dotnet build SimpleSettings.slnx -c Release` → 0 warnings, 0 errors (net8.0 + net10.0). +- Full UnitTests suite (net10) → **153/153 pass** (was 143 on master; +10 this plan). net8 runs in CI. +- `AddSimpleSettingsIntegrationTests` (net10) → 15/15 (5 pre-existing + 10 new). +- Redaction (T-04-VAL): `ValidateSimpleSettings_WhenValidatorThrows_RedactsBoundValue` binds a sentinel secret, asserts it is bound, then asserts the thrown `SettingsValidatorInvocationException.ToString()` omits it. +- Fresh scope (S-1): the scoped-dependency validator resolves and runs (counter == 1) under `validateScopes: true`. +- No new PackageReference in GenericHost (Abstractions-only preserved). + +## Next Phase Readiness +- Phase 4 code is complete (all 5 plans landed). Remaining Phase 4 steps: phase verify (gsd-verifier), security gate (/gsd-secure-phase 4, D-06 sign-off), mark complete. +- DOC-01 (Phase 5) carry-forward items listed above. +- No blockers. + +## Self-Check: PASSED +- FOUND: src/Core/ExistForAll.SimpleSettings.Extensions.GenericHost/ISettingsValidationRunner.cs +- FOUND: src/Core/ExistForAll.SimpleSettings.Extensions.GenericHost/SettingsValidationRunner.cs +- FOUND: src/Core/ExistForAll.SimpleSettings.Extensions.GenericHost/ServiceProviderValidationExtensions.cs +- FOUND: .planning/phases/04-collection-validation-binding/04-04-SUMMARY.md +- FOUND commits: 08a9c33, 3eb67f5, c2b4f2c + +--- +*Phase: 04-collection-validation-binding* +*Completed: 2026-07-15*