From 2e96b2d91718d7ba41386385415e1763126e6ca2 Mon Sep 17 00:00:00 2001 From: guy-lud Date: Wed, 15 Jul 2026 20:58:44 +0300 Subject: [PATCH 1/2] just using [] instead of new --- .../ConfigurationBinder.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Core/ExistForAll.SimpleSettings.Extensions.Binders/ConfigurationBinder.cs b/src/Core/ExistForAll.SimpleSettings.Extensions.Binders/ConfigurationBinder.cs index 88d9a6a..49e6b4c 100644 --- a/src/Core/ExistForAll.SimpleSettings.Extensions.Binders/ConfigurationBinder.cs +++ b/src/Core/ExistForAll.SimpleSettings.Extensions.Binders/ConfigurationBinder.cs @@ -64,7 +64,7 @@ private static bool TrySetChildSequence(IConfigurationSection section, BindingCo { var childValue = child.Value; if (!string.IsNullOrEmpty(childValue)) - (values ??= new List()).Add(childValue); + (values ??= []).Add(childValue); } if (values is null) From 095992ec459c28c973528f2152f4ab2630ea3280 Mon Sep 17 00:00:00 2001 From: guy-lud Date: Wed, 15 Jul 2026 21:23:21 +0300 Subject: [PATCH 2/2] docs(04): land Phase 4 planning + execution records on master --- .planning/REQUIREMENTS.md | 28 +- .planning/ROADMAP.md | 57 +- .planning/STATE.md | 57 +- .planning/config.json | 3 +- .../04-01-PLAN.md | 239 ++++++++ .../04-01-SUMMARY.md | 163 ++++++ .../04-02-PLAN.md | 230 ++++++++ .../04-02-SUMMARY.md | 161 ++++++ .../04-03-PLAN.md | 279 +++++++++ .../04-03-SUMMARY.md | 197 +++++++ .../04-04-PLAN.md | 249 ++++++++ .../04-05-PLAN.md | 157 ++++++ .../04-05-SUMMARY.md | 143 +++++ .../04-CONTEXT.md | 227 ++++++++ .../04-DISCUSSION-LOG.md | 99 ++++ .../04-PATTERNS.md | 273 +++++++++ .../04-RESEARCH.md | 530 ++++++++++++++++++ .../04-REVIEW.md | 213 +++++++ .../04-VALIDATION.md | 101 ++++ 19 files changed, 3364 insertions(+), 42 deletions(-) create mode 100644 .planning/phases/04-collection-validation-binding/04-01-PLAN.md create mode 100644 .planning/phases/04-collection-validation-binding/04-01-SUMMARY.md create mode 100644 .planning/phases/04-collection-validation-binding/04-02-PLAN.md create mode 100644 .planning/phases/04-collection-validation-binding/04-02-SUMMARY.md create mode 100644 .planning/phases/04-collection-validation-binding/04-03-PLAN.md create mode 100644 .planning/phases/04-collection-validation-binding/04-03-SUMMARY.md create mode 100644 .planning/phases/04-collection-validation-binding/04-04-PLAN.md create mode 100644 .planning/phases/04-collection-validation-binding/04-05-PLAN.md create mode 100644 .planning/phases/04-collection-validation-binding/04-05-SUMMARY.md create mode 100644 .planning/phases/04-collection-validation-binding/04-CONTEXT.md create mode 100644 .planning/phases/04-collection-validation-binding/04-DISCUSSION-LOG.md create mode 100644 .planning/phases/04-collection-validation-binding/04-PATTERNS.md create mode 100644 .planning/phases/04-collection-validation-binding/04-RESEARCH.md create mode 100644 .planning/phases/04-collection-validation-binding/04-REVIEW.md create mode 100644 .planning/phases/04-collection-validation-binding/04-VALIDATION.md diff --git a/.planning/REQUIREMENTS.md b/.planning/REQUIREMENTS.md index 710d36f..8724f0d 100644 --- a/.planning/REQUIREMENTS.md +++ b/.planning/REQUIREMENTS.md @@ -57,6 +57,14 @@ Remaining open work (from `FIX-PLAN.md`), batched toward the first `v2.0.0-beta` - [x] **PKG-02**: Float `Microsoft.Extensions.*` floor per-TFM (`8.0.x` for net8) or justify the pin (A4) - [x] **SRC-02**: Command-line binder parses quoted values with spaces correctly and skips `arg[0]` (A6) +### Collection & Validation Binding + +- [x] **COLL-02**: Unset `T[]`/`List` bind an empty collection, never `null` (only `IEnumerable` does today); fix in `TypeConverter.ConvertValue` null branch (client pre-beta #1) +- [x] **COLL-03**: Bind collections from YAML/child-section sequences via `GetChildren()` in `ConfigurationBinder.BindPropertySettings`; comma-scalar MUST still bind (prod `MultiHost__CommonHosts`); children win over scalar; empty/whitespace/empty-sequence → empty (never `[""]`); each element flows the inner converter chain; re-verify S1/SEC-01 redaction (client pre-beta #2) +- [x] **VAL-01**: Wire `ISettingValidation` + `SettingsPropertyAttribute.ValidatorType` into the populate path so a settings object validates itself incl. cross-property rules (promoted from Held D1; reconcile with the `validate-settings` branch) (client pre-beta #3) +- [x] **VAL-02**: `[SettingsProperty(AllowEmpty=false)]` rejects `""`/whitespace/unsubstituted `${ENV:-}` at bind, not just `null`; `TypeConverter.ValidateNullAcceptance` (naturally rides on VAL-01) (client pre-beta) +- [ ] **API-02**: `AddSimpleSettings(...)` exposes the `ISettingsCollection` (return value or resolvable service — shape TBD in planning) (client pre-beta #4) + ### AOT/Trim & Documentation - [ ] **AOT-01**: Annotate reflection entry points (`[RequiresDynamicCode]`/`[RequiresUnreferencedCode]`) and/or document the AOT/trim limitation before stable (A1) @@ -72,7 +80,6 @@ Deferred / held. Tracked but not in the current roadmap. ### Held (do NOT delete) -- **VAL-01**: Wire the `Validations/*` API + `SettingsPropertyAttribute.ValidatorType` into the populate path (reconcile with the `validate-settings` branch) — D1, HELD - **EQ-01**: Delete or fix+wire `EqualityCompererCreator` (has a latent invalid-IL bug) for value-equality on generated types — D2, HELD ### Deferred @@ -104,18 +111,23 @@ Deferred / held. Tracked but not in the current roadmap. | PKG-01 | Phase 3 | Complete | | PKG-02 | Phase 3 | Complete | | SRC-02 | Phase 3 | Complete | -| AOT-01 | Phase 4 | Pending | -| DOC-01 | Phase 4 | Pending | -| REL-01 | Phase 5 | Pending | +| COLL-02 | Phase 4 | Complete | +| COLL-03 | Phase 4 | Complete | +| VAL-01 | Phase 4 | Complete | +| VAL-02 | Phase 4 | Complete | +| API-02 | Phase 4 | Pending | +| AOT-01 | Phase 5 | Pending | +| DOC-01 | Phase 5 | Pending | +| REL-01 | Phase 6 | Pending | **Coverage:** -- v1 requirements: 15 total -- Mapped to phases: 15 +- v1 requirements: 20 total (added Phase 4 engine reqs COLL-02/COLL-03/VAL-02/API-02 + promoted VAL-01 from Held) +- Mapped to phases: 20 - Unmapped: 0 ✓ -- Complete: 4 (Phase 1 SEC-01/SEC-02/EXC-01 #27/#28 + ENG-01/T7 #29); Pending: 11 +- Complete: 12 (Phase 1 SEC-01/SEC-02/EXC-01 #27/#28; Phase 2 COLL-01/TEST-01/TEST-02/TEST-03 + ENG-01/T7 #29; Phase 3 API-01/PKG-01/PKG-02/SRC-02 #31); Pending: 8 (Phase 4 COLL-02/COLL-03/VAL-01/VAL-02/API-02; Phase 5 AOT-01/DOC-01; Phase 6 REL-01) - Validated (shipped, no phase): 13 --- *Requirements defined: 2026-07-13* -*Last updated: 2026-07-14 — ENG-01/T7 marked complete (#29). GSD is now the source of truth; FIX-PLAN.md frozen as a historical reference. Reconciled from session handoff + git.* +*Last updated: 2026-07-14 — Phase 4 "Collection & Validation Binding" formalized (COLL-02/COLL-03/VAL-02/API-02 added; VAL-01 promoted from Held D1); AOT/Docs → Phase 5, beta → Phase 6. ENG-01/T7 complete (#29). GSD is the source of truth; FIX-PLAN.md frozen as historical reference.* diff --git a/.planning/ROADMAP.md b/.planning/ROADMAP.md index 374a2cd..524650c 100644 --- a/.planning/ROADMAP.md +++ b/.planning/ROADMAP.md @@ -2,7 +2,7 @@ ## Overview -**Status (2026-07-14):** Phase 1 shipped (S1 #27, C2 #28); ENG-01/T7 also merged (#29). `master` @ `10f9275`. Active phase is **Phase 2** — ENG-01 done; COLL-01 (deferred) + engine tests (TEST-01/02/03) remain. +**Status (2026-07-14):** Phases 1–3 complete (Phase 1 #27/#28, Phase 2 #30, Phase 3 #31; ENG-01/T7 #29). `master` @ `7f9e17c`. Active phase is **Phase 4 — Collection & Validation Binding** (new engine phase for the client pre-beta requirements); AOT/Trim & Docs renumbered to Phase 5, first beta to Phase 6. The binding engine already ships and works. This milestone is a hardening + pre-stable cleanup pass that batches every remaining breaking change and safety fix before cutting the @@ -10,8 +10,9 @@ first `v2.0.0-beta`. It starts by locking the secret-safe exception story and gi consumers one catchable, structured exception base (Phase 1), proves binding correctness across collection/nullable/converter shapes and closes the generator concurrency race with tests (Phase 2), trims and corrects the public surface, packaging, and command-line binder -(Phase 3), tells consumers the truth about AOT/trim and refreshes the docs (Phase 4), and -finally publishes the batched result as the first beta (Phase 5). Everything serves the core +(Phase 3), binds collections and settings validation correctly across empty/sequence/validator +shapes (Phase 4), tells consumers the truth about AOT/trim and refreshes the docs (Phase 5), and +finally publishes the batched result as the first beta (Phase 6). Everything serves the core value: config → typed settings maps accurately, and never leaks a secret doing it. ## Phases @@ -24,8 +25,9 @@ value: config → typed settings maps accurately, and never leaks a secret doing - [x] **Phase 1: Exception Safety & Public Hierarchy** — ✓ COMPLETE (S1 #27, C2 #28 merged 2026-07-14) - No secret leaks; one catchable, structured `SimpleSettingsException` base - [x] **Phase 2: Binding Correctness & Engine Test Hardening** - Collections/nullable/converters verified; generator race closed by tests *(ENG-01/T7 done #29; COLL-01 + TEST-01/02/03 remain)* (completed 2026-07-14) - [x] **Phase 3: Public Surface, Packaging & Binder Cleanup** - Meaningful public surface; per-TFM deps; correct command-line parsing (completed 2026-07-14) -- [ ] **Phase 4: AOT/Trim Honesty & Documentation** - Honest AOT/trim signals; canonically-named docs -- [ ] **Phase 5: First v2.0.0-beta Release** - Batched breaking changes ship as an installable pre-release +- [ ] **Phase 4: Collection & Validation Binding** - Empty/sequence collection binding, working settings validation, and DI collection surface (client pre-beta engine requirements) +- [ ] **Phase 5: AOT/Trim Honesty & Documentation** - Honest AOT/trim signals; canonically-named docs +- [ ] **Phase 6: First v2.0.0-beta Release** - Batched breaking changes ship as an installable pre-release ## Phase Details @@ -85,10 +87,40 @@ value: config → typed settings maps accurately, and never leaks a secret doing - [x] 03-02-PLAN.md — Command-line binder cleanup: SkipFirstArgument option + space-separated `--k v` lookahead + arg[0] skip + AddCommandLine tokenization (SRC-02) [Wave 2, depends on 03-01] -### Phase 4: AOT/Trim Honesty & Documentation +### Phase 4: Collection & Validation Binding -**Goal**: Consumers get honest signals about AOT/trim support and accurate, canonically-named documentation. +**Goal**: Collections bind correctly across empty, comma-scalar, and YAML-sequence shapes; declared settings validation actually runs; and the DI extension exposes the settings collection — the client-requested engine features batched before beta. **Depends on**: Phase 3 +**Requirements**: COLL-02, COLL-03, VAL-01, VAL-02, API-02 +**Success Criteria** (what must be TRUE): + + 1. An unset `T[]` / `List` / `IEnumerable` binds to an empty collection, never `null` (COLL-02). + 2. A collection binds from a YAML/child-section sequence (`- a` / `- b`); the comma-scalar form still binds (prod `MultiHost__CommonHosts` unaffected); children win when both exist; each element still flows the inner converter chain (COLL-03). + 3. A settings object's declared `ISettingValidation` / `[SettingsProperty(ValidatorType=...)]` is invoked in the bind pipeline, including cross-property rules (VAL-01). + 4. `[SettingsProperty(AllowEmpty=false)]` rejects empty / whitespace values at bind, not just `null` (VAL-02). *(Unsubstituted `${ENV:-}` placeholder detection deferred per CONTEXT D-13 — revisit post-beta.)* + 5. `AddSimpleSettings(...)` exposes the `ISettingsCollection` (return value or resolvable service) (API-02). + 6. After COLL-03 edits `ConfigurationBinder.BindPropertySettings`, the S1/SEC-01 secret-redaction invariant is re-verified; suite green on net8 + net10. + +**Plans**: 4/5 plans executed + +**Wave 1** + +- [x] 04-01-PLAN.md — List family conversion (COLL-01/D-01) + empty-not-null default for array/List/IEnumerable (COLL-02/D-02) [Wave 1] + +**Wave 2** *(blocked on Wave 1 completion)* + +- [x] 04-02-PLAN.md — Config child-sequence binding via GetChildren() (COLL-03/D-04, D-05) + S1 sequence-element redaction regression (D-06 security gate) [Wave 2, depends on 04-01] +- [x] 04-03-PLAN.md — VAL-01 core path: sync validator contracts + SettingsValidationException + [SettingsValidator]/ValidatorType wired into the populate pipeline incl. cross-property (VAL-01) [Wave 2, depends on 04-01] +- [x] 04-05-PLAN.md — VAL-02: AllowEmpty=false rejects empty/whitespace at bind, value-free (VAL-02/D-13, D-14) [Wave 2, depends on 04-01] + +**Wave 3** *(blocked on Wave 2 completion)* + +- [ ] 04-04-PLAN.md — API-02 ISettingsCollection exposure (DI singleton + out-overload, D-15) + VAL-01 deferred DI-resolved validator runner (D-11/Q3) [Wave 3, depends on 04-03] + +### Phase 5: AOT/Trim Honesty & Documentation + +**Goal**: Consumers get honest signals about AOT/trim support and accurate, canonically-named documentation. +**Depends on**: Phase 4 **Requirements**: AOT-01, DOC-01 **Success Criteria** (what must be TRUE): @@ -98,10 +130,10 @@ value: config → typed settings maps accurately, and never leaks a secret doing **Plans**: TBD -### Phase 5: First v2.0.0-beta Release +### Phase 6: First v2.0.0-beta Release **Goal**: All batched breaking changes and hardening ship as the first pre-release beta consumers can install. -**Depends on**: Phases 1–4 (release gate — all breaking + hardening work complete) +**Depends on**: Phases 1–5 (release gate — all breaking + hardening work complete) **Requirements**: REL-01 **Success Criteria** (what must be TRUE): @@ -115,12 +147,13 @@ value: config → typed settings maps accurately, and never leaks a secret doing ## Progress **Execution Order:** -Phases execute in numeric order: 1 → 2 → 3 → 4 → 5 +Phases execute in numeric order: 1 → 2 → 3 → 4 → 5 → 6 | Phase | Plans Complete | Status | Completed | |-------|----------------|--------|-----------| | 1. Exception Safety & Public Hierarchy | n/a (shipped) | ✓ Complete | 2026-07-14 (#27/#28) | | 2. Binding Correctness & Engine Test Hardening | 2/2 | Complete | 2026-07-14 | | 3. Public Surface, Packaging & Binder Cleanup | 2/2 | Complete | 2026-07-14 | -| 4. AOT/Trim Honesty & Documentation | 0/TBD | Not started | - | -| 5. First v2.0.0-beta Release | 0/TBD | Not started | - | +| 4. Collection & Validation Binding | 4/5 | In Progress| | +| 5. AOT/Trim Honesty & Documentation | 0/TBD | Not started | - | +| 6. First v2.0.0-beta Release | 0/TBD | Not started | - | diff --git a/.planning/STATE.md b/.planning/STATE.md index 173183d..08997a2 100644 --- a/.planning/STATE.md +++ b/.planning/STATE.md @@ -2,19 +2,19 @@ gsd_state_version: 1.0 milestone: v2.0.0 milestone_name: milestone -current_phase: 4 -current_phase_name: AOT/Trim Honesty & Documentation -status: verifying -stopped_at: Phase 3 context gathered -last_updated: "2026-07-14T13:07:28.003Z" -last_activity: 2026-07-14 -last_activity_desc: Phase 03 complete, transitioned to Phase 4 +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" +last_activity: 2026-07-15 +last_activity_desc: Phase 04 execution started progress: - total_phases: 5 + total_phases: 6 completed_phases: 2 - total_plans: 4 - completed_plans: 4 - percent: 40 + total_plans: 9 + completed_plans: 8 + percent: 33 --- # Project State @@ -24,16 +24,16 @@ progress: See: .planning/PROJECT.md (updated 2026-07-13) **Core value:** Correctness of binding — config → strongly-typed settings maps accurately across every supported shape (sections, arrays/enumerables, defaults, nullable, custom converters). -**Current focus:** Phase 03 — public-surface-packaging-binder-cleanup +**Current focus:** Phase 04 — collection-validation-binding ## Current Position -Phase: 4 — AOT/Trim Honesty & Documentation -Plan: Not started -Status: Phase complete — ready for verification -Last activity: 2026-07-14 — Phase 03 complete, transitioned to Phase 4 +Phase: 04 (collection-validation-binding) — EXECUTING +Plan: 5 of 5 +Status: Ready to execute +Last activity: 2026-07-15 — Phase 04 execution started -Progress: [████░░░░░░] 40% +Progress: [███░░░░░░░] 33% ## Performance Metrics @@ -60,6 +60,10 @@ Progress: [████░░░░░░] 40% | Phase 02 P02 | 3min | 1 tasks | 1 files | | Phase 03 P01 | 3min | 3 tasks | 4 files | | Phase 03 P02 | 4min | 2 tasks | 4 files | +| Phase 04 P01 | 6min | 2 tasks | 8 files | +| Phase 04 P02 | 12min | 2 tasks | 4 files | +| Phase 04 P03 | 5min | 2 tasks | 10 files | +| Phase 04 P05 | 3min | 1 tasks | 2 files | ## Accumulated Context @@ -78,6 +82,13 @@ Recent decisions affecting current work: - [Phase 02]: TUnit invocation uses --treenode-filter (Microsoft.Testing.Platform); legacy --filter returns zero tests / exit 5. - [Phase 02]: TEST-03 (Plan 02) scalar Uri/DateTime conversion locked via ScalarConversionTests (Uri->new Uri, DateTime->ParseExact yyyy-MM-dd, one format-mismatch negative asserting exception type only); no array-of-* or redaction duplication. Phase 2 success criterion #5 scalar coverage met on net10 (net8 via CI). - [Phase 03]: SRC-02 (Plan 02): CLI binder lookahead + split-by-entry-point exe skip. SkipFirstArgument default false (AddArguments binds arg[0]); AddCommandLine sources GetCommandLineArgs() and sets SkipFirstArgument=true internally. Prefixed next-token = new key (diverges from Microsoft). Empty-safe zero-alloc prefix detection; CLI-path secret redaction (S1) held via store-only BindPropertySettings + regression test. +- [Phase ?]: [Phase 04]: COLL-01/COLL-02 (Plan 01): ListTypeConverter handles the List family (List/IList/ICollection/IReadOnlyList/IReadOnlyCollection) via a cached per-element Func factory — no MakeGenericType/Activator on the warm path (S-4/A1). IsListLike is a NEW disjoint predicate (IsEnumerable unchanged, Pitfall 1); IsCollectionShape is the shared shape check COLL-03 reuses. +- [Phase ?]: [Phase 04]: COLL-02 (Plan 01): unbound array/List/IEnumerable bind an empty collection not null; the List null-result is fresh-per-bind (baked Func factory in the existing _nullResult slot, so PropertyPlan[] layout is unchanged) while arrays/IEnumerable keep the shared cached empty array (review B-3). +- [Phase ?]: 04-02: ConfigurationBinder binds collections from child-section sequences (children win, comma-scalar preserved, whitespace/empty -> empty); Binders reuses Core internal IsCollectionShape via InternalsVisibleTo +- [Phase ?]: VAL-01 core path: object + property validators run post-populate, aggregated via shared SettingsValidationException.ThrowIfAny (reused by Plan 04 DI path) +- [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 ### Pending Todos @@ -87,18 +98,22 @@ None yet. - None blocking. (T7/ENG-01 `SettingsClassGenerator` concurrency race **closed** — shipped pre-GSD via #29: double-checked locking, warm path lock-free, same/distinct-interface `Barrier` stress tests.) +### Roadmap Evolution + +- Phase 4 inserted: Phase 4 Collection & Validation Binding formalized (COLL-02/COLL-03/VAL-01/VAL-02/API-02); AOT/Docs renumbered to Phase 5, beta to Phase 6 + ## Deferred Items Items acknowledged and carried forward: | Category | Item | Status | Deferred At | |----------|------|--------|-------------| -| Held feature | VAL-01 Validations API (D1) | Held (owner-driven) | 2026-07-13 | +| Held feature | VAL-01 Validations API (D1) | Promoted → Phase 4 (2026-07-14) | 2026-07-13 | | Held feature | EQ-01 EqualityCompererCreator (D2) | Held | 2026-07-13 | | Perf | PERF-03 compiled setter (P3b) | Deferred (profile-gated) | 2026-07-13 | ## Session Continuity -Last session: 2026-07-14T12:55:01.741Z -Stopped at: Phase 3 context gathered -Resume file: .planning/phases/03-public-surface-packaging-binder-cleanup/03-CONTEXT.md +Last session: 2026-07-15T13:58:04.982Z +Stopped at: Completed 04-05-PLAN.md (VAL-02) — Phase 4 all 5 plans done +Resume file: None diff --git a/.planning/config.json b/.planning/config.json index 32e9fb0..b0ed171 100644 --- a/.planning/config.json +++ b/.planning/config.json @@ -45,7 +45,8 @@ "post_planning_gaps": true, "security_enforcement": true, "security_asvs_level": 1, - "security_block_on": "high" + "security_block_on": "high", + "_auto_chain_active": false }, "ship": { "pr_body_sections": [ diff --git a/.planning/phases/04-collection-validation-binding/04-01-PLAN.md b/.planning/phases/04-collection-validation-binding/04-01-PLAN.md new file mode 100644 index 0000000..7e02d9d --- /dev/null +++ b/.planning/phases/04-collection-validation-binding/04-01-PLAN.md @@ -0,0 +1,239 @@ +--- +phase: 04-collection-validation-binding +plan: 1 +type: execute +wave: 1 +depends_on: [] +files_modified: + - src/Core/ExistForAll.SimpleSettings/Core/Reflection/TypeExtensions.cs + - src/Core/ExistForAll.SimpleSettings/Conversion/ListTypeConverter.cs + - src/Core/ExistForAll.SimpleSettings/Conversion/CollectionTypeConverter.cs + - src/Core/ExistForAll.SimpleSettings/Conversion/TypeConvertersCollections.cs + - src/Core/ExistForAll.SimpleSettings/Core/Reflection/TypeConverter.cs + - src/Core/ExistForAll.SimpleSettings/Conversion/PropertyConversion.cs + - src/Tests/ExistForAll.SimpleSettings.UnitTests/Conversion/CollectionConversionTests.cs + - src/Tests/ExistForAll.SimpleSettings.UnitTests/Core/TypeConverterTests.cs +autonomous: true +requirements: [COLL-01, COLL-02] + +must_haves: + truths: + - "A settings interface with List/IList/ICollection/IReadOnlyList/IReadOnlyCollection bound from a delimited scalar materializes a List with the parsed elements (COLL-01/D-01)." + - "An IEnumerable property still materializes a T[] (no regression) — the pinned enumerable-returns-array test stays green (D-01)." + - "An unbound int[] binds an empty array, never null (COLL-02/D-02)." + - "An unbound List binds an empty List, never null (COLL-02/D-02)." + - "An unbound IEnumerable still binds an empty T[] (unchanged)." + - "Two resolves of an unset List property return reference-distinct empty lists — mutating one never affects another (the list null-result is fresh per bind, not a shared cached instance) (COLL-02/D-02 correctness, review B-3)." + artifacts: + - "src/Core/ExistForAll.SimpleSettings/Conversion/ListTypeConverter.cs" + - "src/Core/ExistForAll.SimpleSettings/Core/Reflection/TypeExtensions.cs (IsListLike + IsCollectionShape)" + - "src/Core/ExistForAll.SimpleSettings/Core/Reflection/TypeConverter.cs (CreateNullResult shape branch)" + - "src/Core/ExistForAll.SimpleSettings/Conversion/PropertyConversion.cs (fresh-per-bind list null-result; shared cached empty-array path unchanged)" + key_links: + - "ListTypeConverter.CanConvert uses the NEW IsListLike predicate — disjoint from IsArray and IsEnumerable — so it never claims arrays or the open IEnumerable<>." + - "ListTypeConverter is registered AFTER EnumerableTypeConverter and BEFORE EnumTypeConverter in TypeConvertersCollections; existing entries are NOT reordered." + - "CreateNullResult branches on IsArray -> IsListLike -> IsEnumerable -> scalar so each shape yields its correct empty default." + - "For list shapes the null-result is a fresh-per-bind factory (built once at plan build, invoked in PropertyConversion) — arrays/IEnumerable keep returning the shared cached empty array; no warm-path reflection, no shared-mutable-list aliasing (review B-3 + S-4/A1)." +--- + + +Land full `List`-family conversion support (COLL-01/D-01) and the empty-not-null default for every +collection shape (COLL-02/D-02). This is the foundation the rest of the phase builds on: COLL-03's +"empty-sequence -> empty" behavior leans on COLL-02's empty default (D-03), and VAL-01 validators that +inspect collection/list properties need those properties to bind correctly first. + +Purpose: A populated `List` currently falls through to `DefaultTypeConverter` and throws, and an +unbound `T[]`/`List` currently binds `null` — both are named directly in the ROADMAP success criteria +and the client pre-beta doc. +Output: A new `ListTypeConverter`, two disjoint shape predicates (`IsListLike`, `IsCollectionShape`), a +shape-aware `CreateNullResult`, and extended converter/type-converter tests. + +Skills: invoke `dotnet-claude-kit:modern-csharp` for the new converter and predicate idioms (C# 12-safe +only — no `field` keyword / extension members, which are net10/C# 14-only and break the net8 build), and +`dotnet-claude-kit:testing` for the TUnit test extensions. + + + +@/Users/guyludvig/frontegg/development/open-source/SimpleSettings/.claude/gsd-core/workflows/execute-plan.md +@/Users/guyludvig/frontegg/development/open-source/SimpleSettings/.claude/gsd-core/templates/summary.md + + + +@.planning/PROJECT.md +@.planning/ROADMAP.md +@.planning/STATE.md +@.planning/phases/04-collection-validation-binding/04-CONTEXT.md +@.planning/phases/04-collection-validation-binding/04-RESEARCH.md +@.planning/phases/04-collection-validation-binding/04-PATTERNS.md +@.planning/phases/04-collection-validation-binding/04-VALIDATION.md +@.planning/codebase/CONVENTIONS.md + +@src/Core/ExistForAll.SimpleSettings/Conversion/CollectionTypeConverter.cs +@src/Core/ExistForAll.SimpleSettings/Conversion/ArrayTypeConverter.cs +@src/Core/ExistForAll.SimpleSettings/Conversion/EnumerableTypeConverter.cs +@src/Core/ExistForAll.SimpleSettings/Core/Reflection/TypeExtensions.cs +@src/Core/ExistForAll.SimpleSettings/Core/Reflection/TypeConverter.cs +@src/Core/ExistForAll.SimpleSettings/Conversion/TypeConvertersCollections.cs + + + +New symbols introduced by THIS plan (exclude from drift/orphan checks — they are net-new): +- `ExistForAll.SimpleSettings.Conversion.ListTypeConverter` (internal converter) +- `TypeExtensions.IsListLike(this Type)` (internal extension) +- `TypeExtensions.IsCollectionShape(this Type)` (internal extension) +- `CollectionTypeConverter.Convert` promoted to `virtual` (signature unchanged) +- A cached element-type -> `List` materialization factory (used by `ListTypeConverter.Convert` and by the list null-result) replacing the `Activator.CreateInstance(MakeGenericType(...))` idiom on the warm path (review S-4/A1) +- `PropertyConversion` list null-result handled via a fresh-per-bind factory in the existing `_nullResult` slot (no new struct field, so `PropertyPlan[]` layout / plan-build byte-cost is unchanged; review B-3) +- New test cases/fixture interfaces in `CollectionConversionTests` (IIntList, IIntIList, IIntICollection, IIntReadOnlyList, IIntReadOnlyCollection or equivalent) and `TypeConverterTests` + + + + + + Task 1: List<T> family converter + disjoint shape predicates (COLL-01/D-01) + + - src/Core/ExistForAll.SimpleSettings/Conversion/CollectionTypeConverter.cs (the base to subclass; `Convert` at :25-38 must become `virtual`; `AsArray` :43-51 already passes an Array through) + - src/Core/ExistForAll.SimpleSettings/Conversion/ArrayTypeConverter.cs (analog: ~20-line subclass overriding CanConvert + GetElementType) + - src/Core/ExistForAll.SimpleSettings/Conversion/EnumerableTypeConverter.cs (analog: generic-arg element type via GetTypeInfo().GetGenericArguments()[0]) + - src/Core/ExistForAll.SimpleSettings/Core/Reflection/TypeExtensions.cs (IsEnumerable :21-26 — the predicate shape to mirror; do NOT widen it) + - src/Core/ExistForAll.SimpleSettings/Conversion/TypeConvertersCollections.cs (registration chain :7-12 — insert, do not reorder) + - 04-PATTERNS.md § "Conversion/ListTypeConverter.cs" and § "Core/Reflection/TypeExtensions.cs" + - 04-RESEARCH.md Pattern 1 + Pitfall 1 (why a separate predicate; converter ordering) + + src/Core/ExistForAll.SimpleSettings/Core/Reflection/TypeExtensions.cs, src/Core/ExistForAll.SimpleSettings/Conversion/ListTypeConverter.cs, src/Core/ExistForAll.SimpleSettings/Conversion/CollectionTypeConverter.cs, src/Core/ExistForAll.SimpleSettings/Conversion/TypeConvertersCollections.cs, src/Tests/ExistForAll.SimpleSettings.UnitTests/Conversion/CollectionConversionTests.cs + + - Test: List<int> bound from "1,2,3" -> List<int> equal to [1,2,3]. + - Test: IList<int>, ICollection<int>, IReadOnlyList<int>, IReadOnlyCollection<int> each bound from a delimited scalar -> a List<int> instance with the parsed elements. + - Test: IEnumerable<int> bound from "5,6,7" still returns a T[] (pinned Convert_ToIntEnumerable_MaterializesAnArray stays green). + - Test: each list element still flows the element converter chain (e.g. an enum or DateTime list parses per element). + + + Add `IsListLike(this Type)` to `TypeExtensions` mirroring the `IsEnumerable` idiom + (`GetTypeInfo().IsGenericType` + `GetGenericTypeDefinition()` compared against the set + `List<>`, `IList<>`, `ICollection<>`, `IReadOnlyList<>`, `IReadOnlyCollection<>`). Add + `IsCollectionShape(this Type)` returning `type.IsArray || type.IsEnumerable() || type.IsListLike()` + (the single source of truth COLL-03 will reuse). Do NOT widen `IsEnumerable` — it feeds both + `EnumerableTypeConverter.CanConvert` and `TypeConverter.CreateNullResult`; widening makes + `EnumerableTypeConverter` claim `List` and return a `T[]` unassignable to a `List` property + (Pitfall 1). Per D-01, create `internal class ListTypeConverter : CollectionTypeConverter` copying the + `ArrayTypeConverter` constructor-forwarding shape: override `CanConvert` to use `IsListLike`, override + `GetElementType` to return the first generic argument (as `EnumerableTypeConverter` does), and override + `Convert` to call `base.Convert(value, settingsType)` (the shared array-build path) then wrap the built + array into a `List` via a CACHED per-element-type factory delegate (perf finding S-4/A1) — do NOT use + `Activator.CreateInstance(typeof(List<>).MakeGenericType(elementType), builtArray)`, which reintroduces the + reflection idiom P4 deleted plus a transient `Type[1]`/`object[1]`/`Invoke` on the per-populate warm path. + Cache a `Func` keyed by element type (e.g. a `ConcurrentDictionary>` + whose factory is built once via a generic helper that calls `new List(array)`), so per-populate + materialization is a plain delegate call with zero reflection. Note the corrected BCL semantics: because the + built array is `ICollection`, `new List(array)` COPIES the elements into a fresh right-sized backing + buffer — it does NOT alias the built array (two n-sized buffers exist transiently). This is correct and keeps + the array/IEnumerable hot paths byte-unchanged (the wrap only runs for previously-throwing list targets). + Promote `CollectionTypeConverter.Convert` to `virtual` (mechanical, internal, non-breaking) so the + override compiles; leave `AsArray`/`GetElementConverter` and every existing converter untouched. Register + `new ListTypeConverter(settingsOptions, this)` in `TypeConvertersCollections` via `AddLast` positioned + AFTER `EnumerableTypeConverter` and BEFORE `EnumTypeConverter`; the predicate is disjoint so ordering is + safe — do NOT reorder existing entries (user-first / DefaultTypeConverter-last is load-bearing). Extend + `CollectionConversionTests.cs` with nested (non-settings-indicated) fixture interfaces for the five list + shapes and the behavior tests above; keep the pinned `Convert_ToIntEnumerable_MaterializesAnArray` + unchanged and green. Follow `modern-csharp` (C# 12-safe), block-scoped namespaces, tabs to match the + surrounding Conversion files. + + + cd src && dotnet test SimpleSettings.slnx -c Release -f net10.0 --treenode-filter "/*/*/CollectionConversionTests/*" + + + - `grep -n "IsListLike" src/Core/ExistForAll.SimpleSettings/Core/Reflection/TypeExtensions.cs` shows a new predicate; `IsEnumerable` body is unchanged (still compares only against `typeof(IEnumerable<>)`). + - `ListTypeConverter.cs` exists, is `internal`, extends `CollectionTypeConverter`, and `CanConvert` calls `IsListLike`. + - `TypeConvertersCollections.cs` registers `ListTypeConverter` after `EnumerableTypeConverter` and before `EnumTypeConverter`; the DateTime/Uri/Array/Enumerable/Enum/Default relative order is preserved. + - `ListTypeConverter.Convert` materializes the `List` via a cached factory delegate; no `Activator.CreateInstance`/`MakeGenericType`/`Invoke` executes on the per-populate path (perf S-4/A1). + - The CollectionConversionTests run is green including all five list-shape cases and the pinned `Convert_ToIntEnumerable_MaterializesAnArray` (IEnumerable still returns int[]). + + List<T>/IList<T>/ICollection<T>/IReadOnlyList<T>/IReadOnlyCollection<T> all materialize a List<T> from a delimited scalar; IEnumerable<T> still returns a T[]; CollectionConversionTests green on net10. + + + + Task 2: Empty-not-null default for every collection shape (COLL-02/D-02) + + - src/Core/ExistForAll.SimpleSettings/Core/Reflection/TypeConverter.cs (`CreateNullResult` :27-36 — the method to extend; today arrays hit the `!IsEnumerable()` branch and return null because an array is a reference type; `CreateConversion` :12-23 passes the null-result into `PropertyConversion`) + - src/Core/ExistForAll.SimpleSettings/Conversion/PropertyConversion.cs (`Convert` :29-40 consumes `_nullResult` in the `value == null` branch — EDIT here per review B-3 so a list null-result is fresh per bind, not a shared cached instance) + - src/Tests/ExistForAll.SimpleSettings.UnitTests/Core/TypeConverterTests.cs (the ResolveConversion seam + conventions to extend) + - 04-PATTERNS.md § "Core/Reflection/TypeConverter.cs — CreateNullResult" + - 04-RESEARCH.md Pattern 1 (COLL-02 empty-not-null) + State of the Art (Array.CreateInstance, not Enumerable.Empty reflection) + + src/Core/ExistForAll.SimpleSettings/Core/Reflection/TypeConverter.cs, src/Core/ExistForAll.SimpleSettings/Conversion/PropertyConversion.cs, src/Tests/ExistForAll.SimpleSettings.UnitTests/Core/TypeConverterTests.cs + + - Test: PropertyConversion for an unbound int[] property, Convert(null) returns an empty int[] (Length 0), not null. + - Test: PropertyConversion for an unbound List<int> property, Convert(null) returns an empty List<int> (Count 0), not null. + - Test (regression): PropertyConversion for an unbound IEnumerable<int> property, Convert(null) still returns an empty int[]. + - Test (B-3 correctness): two separate Convert(null) calls for an unbound List<int> property return reference-distinct List<int> instances; Add-ing to one leaves the other empty. + + + Extend `CreateNullResult` (per D-02) so the shape branch is: if `propertyType.IsArray` return the cached + shared empty array `Array.CreateInstance(propertyType.GetElementType()!, 0)`; else if `propertyType.IsEnumerable()` + return the cached shared empty array `Array.CreateInstance(elementType, 0)` (existing behavior, unchanged); + else if `propertyType.IsListLike()` return a FRESH-PER-BIND empty `List` — NOT a shared cached instance + (review B-3: an empty array is safe to share because it is effectively immutable, but a mutable empty + `List` is not — a consumer mutating "their" empty list would corrupt every later null-bind of that + property). Mechanism: resolve the element type from the first generic argument, then have `CreateNullResult` + return a `Func` factory that produces a fresh `List` on each call. Store that factory in the + EXISTING `_nullResult` slot (its type is already `object?`), which avoids adding a field to the readonly + `PropertyConversion` struct and therefore keeps the `PropertyPlan[]` layout and plan-build byte-cost + unchanged (protects the B-2 benchmark gate). Build the factory ONCE here at plan build (cold reflection via a + generic helper is fine and expected — keep CreateNullResult's cold reflection as-is per S-4; the delegate it + returns must do a plain `new List()` with NO per-call reflection). In `PropertyConversion.Convert`, + in the `value == null && !_throwOnNull` path, return `factory()` when `_nullResult` is a `Func`, + otherwise return `_nullResult` exactly as today (arrays / value-type defaults / IEnumerable stay on the + shared-instance path, so their behavior and warm-path cost are byte-identical). For the scalar/value-type + fallback keep the existing value-type-default / null branch (`IsValueType ? Activator.CreateInstance(propertyType) : null`). + Do not reintroduce `Enumerable.Empty` reflection. Extend `Core/TypeConverterTests.cs` via its + `ResolveConversion` helper with the four behavior tests above, adding nested fixture interfaces (e.g. an + int[] property, a List<int> property) alongside the existing `ISample`; the B-3 test resolves the null + result twice and asserts reference inequality plus mutation isolation. Do NOT re-assert the AllowEmpty path + (owned by VAL-02 / SettingsPropertyTests). Follow `modern-csharp` + `testing`. + + + cd src && dotnet test SimpleSettings.slnx -c Release -f net10.0 --treenode-filter "/*/*/TypeConverterTests/*" + + + - `CreateNullResult` in TypeConverter.cs branches on `IsArray`, `IsListLike`, and `IsEnumerable` before the value-type/null fallback. + - TypeConverterTests run is green: unbound int[] -> empty int[] (not null); unbound List<int> -> empty List<int> (not null); unbound IEnumerable<int> -> empty int[]. + - B-3: two resolves of an unset List<int> property return reference-distinct empty lists and mutating one does not affect the other; the array/IEnumerable null-result stays the shared cached empty instance. + - No `Enumerable.Empty` reflection is introduced (`grep -n "Enumerable\).GetMethod\|\"Empty\"" src/Core/ExistForAll.SimpleSettings/Core/Reflection/TypeConverter.cs` returns nothing). + + Unbound array, List-family, and IEnumerable collection properties all bind an empty collection instead of null; each unset List<T> bind yields a fresh (reference-distinct) empty list; arrays/IEnumerable keep the shared cached empty instance; TypeConverterTests green on net10. + + + + + +## Trust Boundaries + +| Boundary | Description | +|----------|-------------| +| config source -> converter chain | Untrusted config values (possibly secrets) enter the collection converters and are split/parsed per element | +| conversion failure -> host logs | A per-element convert failure surfaces as an exception whose ToString() reaches host logs | + +## STRIDE Threat Register + +| Threat ID | Category | Component | Severity | Disposition | Mitigation Plan | +|-----------|----------|-----------|----------|-------------|-----------------| +| T-04-01 | Information Disclosure | ListTypeConverter element convert failure | medium | mitigate | Element convert failures still flow through the existing `ValuesPopulator.ConvertPropertyValue` value-free `SettingsPropertyValueException` wrapper (unchanged this plan); the new converter adds no value to any message. Redaction on the sequence path is re-verified in Plan 02 (D-06). | +| T-04-02 | Tampering | Converter chain ordering | low | mitigate | New converter inserted with a disjoint predicate after EnumerableTypeConverter; existing user-first/Default-last order preserved (acceptance criterion). | +| T-04-SC | Supply Chain | package installs | n/a | accept | No npm/pip/cargo and no new `PackageVersion` entries this phase (04-RESEARCH.md Package Legitimacy Audit: N/A). No install task, no checkpoint. | + + + +- `cd src && dotnet build SimpleSettings.slnx -c Release` succeeds for net8.0 AND net10.0 (no C# 14-only syntax leaked into shared code). +- CollectionConversionTests + TypeConverterTests green on net10 locally (net8 in CI). +- Pinned `Convert_ToIntEnumerable_MaterializesAnArray` still green (no IEnumerable regression). +- `benchmark.yml` allocation gate stays green — the List wrap (cached factory, no warm-path reflection) and the fresh-per-bind empty list only run for list-shaped targets that previously threw or bound null; array/IEnumerable/scalar hot paths and the plan-build `PropertyPlan[]` allocation are byte-unchanged (RESEARCH Pitfall 5; reviews S-4/A1 + B-3; the `_nullResult` slot is reused so the struct does not grow). + + + +ROADMAP success criterion 1 (COLL-02: unset T[]/List/IEnumerable bind an empty collection, never null) +is met, and COLL-01/D-01 full List-family conversion is delivered, both proven by green tests on net10. + + + +Create `.planning/phases/04-collection-validation-binding/04-01-SUMMARY.md` when done. + diff --git a/.planning/phases/04-collection-validation-binding/04-01-SUMMARY.md b/.planning/phases/04-collection-validation-binding/04-01-SUMMARY.md new file mode 100644 index 0000000..b57e935 --- /dev/null +++ b/.planning/phases/04-collection-validation-binding/04-01-SUMMARY.md @@ -0,0 +1,163 @@ +--- +phase: 04-collection-validation-binding +plan: 01 +subsystem: conversion +tags: [type-conversion, collections, generics, reflection-elision, dotnet, tunit] + +# Dependency graph +requires: + - phase: 02-engine-core-tests + provides: CollectionTypeConverter de-reflection base + PropertyConversion null-result seam + - phase: 03-sources-cli + provides: converter-chain ordering conventions (user-first / Default-last) +provides: + - "ListTypeConverter: List/IList/ICollection/IReadOnlyList/IReadOnlyCollection materialize a List from a delimited scalar (COLL-01/D-01)" + - "IsListLike + IsCollectionShape shape predicates (disjoint from IsArray/IsEnumerable) — IsCollectionShape is the single source of truth COLL-03 will reuse" + - "Shape-aware CreateNullResult: unbound array/List/IEnumerable bind an empty collection, never null (COLL-02/D-02)" + - "Fresh-per-bind empty List null-result (reference-distinct per bind) via a baked factory delegate in the existing _nullResult slot" +affects: [collection-source-binding, validation, COLL-03, VAL-01] + +# Tech tracking +tech-stack: + added: [] + patterns: + - "Cached per-element-type Func materialization delegate (built once via generic-method CreateDelegate) instead of Activator.CreateInstance(MakeGenericType(...)) on the warm path" + - "Factory-delegate null-result reusing the object? _nullResult slot to give mutable defaults fresh-per-bind semantics without growing the readonly struct" + +key-files: + created: + - src/Core/ExistForAll.SimpleSettings/Conversion/ListTypeConverter.cs + modified: + - src/Core/ExistForAll.SimpleSettings/Core/Reflection/TypeExtensions.cs + - src/Core/ExistForAll.SimpleSettings/Conversion/CollectionTypeConverter.cs + - src/Core/ExistForAll.SimpleSettings/Conversion/TypeConvertersCollections.cs + - src/Core/ExistForAll.SimpleSettings/Core/Reflection/TypeConverter.cs + - src/Core/ExistForAll.SimpleSettings/Conversion/PropertyConversion.cs + - src/Tests/ExistForAll.SimpleSettings.UnitTests/Conversion/CollectionConversionTests.cs + - src/Tests/ExistForAll.SimpleSettings.UnitTests/Core/TypeConverterTests.cs + +key-decisions: + - "ListTypeConverter subclasses CollectionTypeConverter and reuses base.Convert's array-build path, then copies into List via a cached factory delegate — zero warm-path reflection (review S-4/A1)" + - "IsEnumerable left unchanged (still only typeof(IEnumerable<>)); a separate disjoint IsListLike avoids EnumerableTypeConverter claiming List and returning an unassignable T[] (Pitfall 1)" + - "List null-result is a fresh-per-bind factory, not a shared instance — a shared mutable empty List would be corrupted by a consumer mutating 'their' empty list (review B-3); arrays/IEnumerable keep the shared cached empty array (effectively immutable)" + +patterns-established: + - "Warm-path generic materialization via cached CreateDelegate factory keyed by element type" + - "Mutable-default null-results carried as Func in the existing _nullResult slot so PropertyPlan[] layout/byte-cost is unchanged (protects the B-2 benchmark gate)" + +requirements-completed: [COLL-01, COLL-02] + +coverage: + - id: D1 + description: "List/IList/ICollection/IReadOnlyList/IReadOnlyCollection bound from a delimited scalar materialize a List with parsed elements (COLL-01/D-01)" + requirement: "COLL-01" + verification: + - kind: unit + ref: "src/Tests/ExistForAll.SimpleSettings.UnitTests/Conversion/CollectionConversionTests.cs#Convert_DelimitedString_ToIntList_MaterializesList (and IntIList/IntICollection/IntReadOnlyList/IntReadOnlyCollection/DayOfWeekList)" + status: pass + human_judgment: false + - id: D2 + description: "IEnumerable still materializes a T[] — no regression (pinned Convert_ToIntEnumerable_MaterializesAnArray)" + requirement: "COLL-01" + verification: + - kind: unit + ref: "src/Tests/ExistForAll.SimpleSettings.UnitTests/Conversion/CollectionConversionTests.cs#Convert_ToIntEnumerable_MaterializesAnArray" + status: pass + human_judgment: false + - id: D3 + description: "Unbound array/List/IEnumerable collection properties bind an empty collection instead of null (COLL-02/D-02)" + requirement: "COLL-02" + verification: + - kind: unit + ref: "src/Tests/ExistForAll.SimpleSettings.UnitTests/Core/TypeConverterTests.cs#Convert_NullForArrayProperty_ReturnsEmptyArray, #Convert_NullForListProperty_ReturnsEmptyList, #Convert_NullForEnumerableProperty_ReturnsEmptyArray" + status: pass + human_judgment: false + - id: D4 + description: "Each unset List bind yields a fresh reference-distinct empty list; mutating one never affects another (review B-3)" + requirement: "COLL-02" + verification: + - kind: unit + ref: "src/Tests/ExistForAll.SimpleSettings.UnitTests/Core/TypeConverterTests.cs#Convert_NullForListProperty_YieldsFreshInstancePerBind" + status: pass + human_judgment: false + +# Metrics +duration: 6min +completed: 2026-07-15 +status: complete +--- + +# Phase 04 Plan 01: Collection Conversion Foundation Summary + +**Full List-family conversion (List/IList/ICollection/IReadOnlyList/IReadOnlyCollection) plus an empty-not-null default for every collection shape, both with zero warm-path reflection and B-3 fresh-per-bind list semantics.** + +## Performance + +- **Duration:** ~6 min +- **Started:** 2026-07-15T14:24:00Z +- **Completed:** 2026-07-15T14:30:06Z +- **Tasks:** 2 (both TDD) +- **Files modified:** 8 (1 created, 7 modified) + +## Accomplishments +- `ListTypeConverter` materializes a `List` for the whole List family from a delimited scalar; the element converter chain still runs per element (enum/DateTime/Uri lists parse correctly). +- `IEnumerable` still returns a `T[]` — the pinned `Convert_ToIntEnumerable_MaterializesAnArray` stays green (no regression). +- Unbound `int[]`, `List`, and `IEnumerable` now bind an empty collection rather than `null`. +- Two null-binds of an unset `List` return reference-distinct, mutation-isolated empty lists (review B-3), while arrays/IEnumerable keep the shared cached empty array. +- No `Activator.CreateInstance`/`MakeGenericType`/`Invoke` on the per-populate warm path — materialization is a cached `Func` delegate keyed by element type (review S-4/A1). + +## Task Commits + +Each task was committed atomically (TDD RED → GREEN): + +1. **Task 1: List-family converter + disjoint shape predicates** — `7a91d1e` (test) → `5c2dab0` (feat) +2. **Task 2: Empty-not-null default for every collection shape** — `44b33d9` (test) → `1dfd464` (feat) + +**Plan metadata:** _(final docs commit — see git log)_ + +## Files Created/Modified +- `Conversion/ListTypeConverter.cs` (created) — List-family converter; cached per-element `Func` factory built via generic-method `CreateDelegate`. +- `Core/Reflection/TypeExtensions.cs` — added `IsListLike` (disjoint) and `IsCollectionShape`; `IsEnumerable` unchanged. +- `Conversion/CollectionTypeConverter.cs` — `Convert` promoted to `virtual` (mechanical, internal, non-breaking). +- `Conversion/TypeConvertersCollections.cs` — registered `ListTypeConverter` after `EnumerableTypeConverter`, before `EnumTypeConverter`; existing order preserved. +- `Core/Reflection/TypeConverter.cs` — `CreateNullResult` branches `IsArray → IsEnumerable → IsListLike → value-type/null`; list branch bakes an empty-list factory delegate. +- `Conversion/PropertyConversion.cs` — `Convert` invokes the factory when `_nullResult` is `Func`; all other shapes on the shared-instance path unchanged. +- `Tests/.../Conversion/CollectionConversionTests.cs` — 6 new tests + 6 fixture interfaces for the List family and per-element enum list. +- `Tests/.../Core/TypeConverterTests.cs` — 4 new null-result tests + 3 fixture interfaces (incl. the B-3 fresh-per-bind assertion). + +## Decisions Made +None beyond the plan — implemented exactly as specified. The plan's `` blocks superseded the PATTERNS.md `Activator.CreateInstance(MakeGenericType(...))` sketch with the cached-factory approach (review S-4/A1), and that plan-level guidance was followed. + +## Deviations from Plan + +None - plan executed exactly as written. + +## Issues Encountered +- A solution-wide `dotnet build SimpleSettings.slnx -c Release -f net8.0` reports `NETSDK1005` on `ExistForAll.SimpleSettings.Benchmark` because that project is single-TFM `net10.0`. This is a pre-existing, out-of-scope condition of the benchmark project, not a result of these changes. The plan's authoritative command (`dotnet build SimpleSettings.slnx -c Release`, no `-f`) succeeds with 0 warnings/0 errors, and building the Core library directly with `-f net8.0` succeeds with 0 errors — confirming the shared code is C# 12-safe for net8. + +## Verification +- `cd src && dotnet build SimpleSettings.slnx -c Release` → 0 warnings, 0 errors (net8.0 + net10.0 per-project TFMs). +- Core library `-f net8.0` build → 0 errors (C# 12-safe shared code confirmed). +- `CollectionConversionTests` → 18/18 pass (incl. all 5 list shapes, per-element enum list, and pinned enumerable-array test). +- `TypeConverterTests` → 8/8 pass (incl. array/list/enumerable empty defaults and B-3 fresh-per-bind). +- Full suite net10 → 114/114 pass. +- Acceptance greps: `IsEnumerable` body unchanged; no `Enumerable.Empty` reflection in `TypeConverter.cs`; no `Activator.CreateInstance`/`MakeGenericType` in `ListTypeConverter.cs` code; registration order preserved. + +## User Setup Required +None - no external service configuration required. + +## Next Phase Readiness +- `IsCollectionShape` is available as the single shape predicate COLL-03's source binder will reuse. +- COLL-02's empty default is in place, which COLL-03's empty-sequence→empty behavior (D-03) builds on. +- List-family and array/enumerable properties now bind correctly, unblocking VAL-01 validators that inspect collection properties. +- No blockers. + +## Self-Check: PASSED + +- FOUND: src/Core/ExistForAll.SimpleSettings/Conversion/ListTypeConverter.cs +- FOUND: .planning/phases/04-collection-validation-binding/04-01-SUMMARY.md +- FOUND commits: 7a91d1e, 5c2dab0, 44b33d9, 1dfd464 + +--- +*Phase: 04-collection-validation-binding* +*Completed: 2026-07-15* diff --git a/.planning/phases/04-collection-validation-binding/04-02-PLAN.md b/.planning/phases/04-collection-validation-binding/04-02-PLAN.md new file mode 100644 index 0000000..1650f63 --- /dev/null +++ b/.planning/phases/04-collection-validation-binding/04-02-PLAN.md @@ -0,0 +1,230 @@ +--- +phase: 04-collection-validation-binding +plan: 2 +type: execute +wave: 2 +depends_on: [04-01] +files_modified: + - src/Core/ExistForAll.SimpleSettings.Extensions.Binders/ConfigurationBinder.cs + - src/Core/ExistForAll.SimpleSettings/Info.cs + - src/Tests/ExistForAll.SimpleSettings.UnitTests/ConfigBuilderConfigurationBinderIntegrationTests.cs + - src/Tests/ExistForAll.SimpleSettings.UnitTests/Conversion/ExceptionRedactionTests.cs +autonomous: true +requirements: [COLL-03] + +must_haves: + truths: + - "A collection property binds from an IConfiguration child-section sequence (Section:Key:0, Section:Key:1, ...) as a string[] flowing the element converter chain (COLL-03/D-04)." + - "When both a scalar and children exist for the key, the children win (D-05)." + - "The comma-scalar form still binds unchanged (prod MultiHost__CommonHosts override preserved) (D-05)." + - "Empty / whitespace scalar and empty child-sequence -> empty collection, never [\"\"] (D-05, leaning on COLL-02's empty default)." + - "[SettingsSection]/root prefix is honored on the sequence path exactly as on the scalar path (D-05)." + - "A secret carried in a sequence element is absent from the entire ex.ToString() chain when an element fails to convert (D-06 / S1)." + artifacts: + - "src/Core/ExistForAll.SimpleSettings.Extensions.Binders/ConfigurationBinder.cs (GetChildren() branch)" + - "src/Core/ExistForAll.SimpleSettings/Info.cs (InternalsVisibleTo grant to the Binders package)" + key_links: + - "ConfigurationBinder reuses Core's IsCollectionShape predicate (from Plan 01) via a new InternalsVisibleTo grant — one source of truth for shape detection, no drift with the converter chain." + - "CollectionTypeConverter.AsArray passes an incoming string[] straight through -> no converter change needed for COLL-03." + - "The whitespace guard (IsNullOrWhiteSpace) skips SetNewValue so COLL-02's empty-not-null nullResult produces the empty collection instead of a [\" \"] element." + - "The GetChildren() edit runs inside ValuesPopulator's wrapping catch (SettingsBindingException) — no element value may enter any exception message (D-06)." +--- + + +Bind collections from IConfiguration child-section sequences (COLL-03/D-04, D-05) in +`ConfigurationBinder.BindPropertySettings`, and re-verify the S1/SEC-01 secret-redaction invariant on the +new sequence path (D-06 — the phase's mandatory security gate). Depends on Plan 01 for the +`IsCollectionShape` predicate and COLL-02's empty-not-null default. + +Purpose: The binder reads a single scalar today; YAML/child-section sequences (`- a` / `- b`) never bind. +The client pre-beta doc requires sequence binding while keeping the prod comma-scalar override working. +Output: A `GetChildren()` branch producing a `string[]` (children win; whitespace/empty -> empty; prefix +honored), an `InternalsVisibleTo` grant so the Binders package reuses Core's shape predicate, and a new +redaction regression test proving no secret leaks from a sequence element. + +Skills: invoke `dotnet-claude-kit:modern-csharp` (C# 12-safe LINQ/collection idioms) for the binder edit +and `dotnet-claude-kit:testing` for the integration + redaction test extensions. + + + +@/Users/guyludvig/frontegg/development/open-source/SimpleSettings/.claude/gsd-core/workflows/execute-plan.md +@/Users/guyludvig/frontegg/development/open-source/SimpleSettings/.claude/gsd-core/templates/summary.md + + + +@.planning/PROJECT.md +@.planning/ROADMAP.md +@.planning/STATE.md +@.planning/phases/04-collection-validation-binding/04-CONTEXT.md +@.planning/phases/04-collection-validation-binding/04-RESEARCH.md +@.planning/phases/04-collection-validation-binding/04-PATTERNS.md +@.planning/phases/04-collection-validation-binding/04-VALIDATION.md +@.planning/codebase/CONVENTIONS.md + +@src/Core/ExistForAll.SimpleSettings.Extensions.Binders/ConfigurationBinder.cs +@src/Core/ExistForAll.SimpleSettings/BindingContext.cs +@src/Core/ExistForAll.SimpleSettings/Conversion/CollectionTypeConverter.cs +@src/Core/ExistForAll.SimpleSettings/ValuesPopulator.cs +@src/Core/ExistForAll.SimpleSettings/SettingsBindingException.cs +@src/Core/ExistForAll.SimpleSettings/Info.cs +@src/Tests/ExistForAll.SimpleSettings.UnitTests/Conversion/ExceptionRedactionTests.cs +@src/Tests/ExistForAll.SimpleSettings.UnitTests/ConfigBuilderConfigurationBinderIntegrationTests.cs + + + +New symbols introduced by THIS plan (exclude from drift/orphan checks): +- A new `InternalsVisibleTo("ExistForAll.SimpleSettings.Binders")` attribute in Info.cs +- New test methods + fixture interfaces in `SettingsBuilderConfigurationBinderIntegrationTests` (sequence bind, children-win, comma-scalar guard, empty->empty, prefix) +- A new `Convert_SecretInSequenceElement_DoesNotLeakValue` test (+ its fixture) in `ExceptionRedactionTests` +- Consumes (does not create): `TypeExtensions.IsCollectionShape` from Plan 01 + + + + + + Task 1: GetChildren() child-sequence branch in ConfigurationBinder (COLL-03/D-04, D-05) + + - src/Core/ExistForAll.SimpleSettings.Extensions.Binders/ConfigurationBinder.cs (the method to modify :26-36; keep the static-factory GetOrAdd :30 and ResolveSection root prefixing :38-41) + - src/Core/ExistForAll.SimpleSettings/BindingContext.cs (context.PropertyType :13,35 is public; SetNewValue :39-43) + - src/Core/ExistForAll.SimpleSettings/Conversion/CollectionTypeConverter.cs (AsArray :43-51 passes an Array through; RemoveEmptyEntries drops empty but KEEPS whitespace tokens -> the [" "] pitfall) + - src/Core/ExistForAll.SimpleSettings/Core/Reflection/TypeExtensions.cs (IsCollectionShape added in Plan 01 — the predicate to reuse) + - src/Core/ExistForAll.SimpleSettings/Info.cs (add the Binders InternalsVisibleTo grant here) + - 04-PATTERNS.md § "Extensions.Binders/ConfigurationBinder.cs" and § "Core/Reflection/TypeExtensions.cs" (IsCollectionShape reuse) + - 04-RESEARCH.md Pattern 2 (mechanism + locked sub-rules) + Pitfall 2 (whitespace -> [" "]) + + src/Core/ExistForAll.SimpleSettings.Extensions.Binders/ConfigurationBinder.cs, src/Core/ExistForAll.SimpleSettings/Info.cs, src/Tests/ExistForAll.SimpleSettings.UnitTests/ConfigBuilderConfigurationBinderIntegrationTests.cs + + - Test: child-section sequence (Section:Values:0=a, :1=b, :2=c) binds string[]/List<string> = [a,b,c]; an int-sequence binds int[]; each element flows the element converter chain. + - Test: when a scalar Section:Values="x" AND children Section:Values:0=a,:1=b both exist, the bound value is [a,b] (children win). + - Test: comma-scalar Section:Values="a,b,c" (no children) still binds [a,b,c] (regression guard for prod MultiHost__CommonHosts). + - Test: whitespace scalar Section:Values=" " and empty child-sequence both bind an empty collection with Count 0 (never a single blank element). + - Test: with RootSection/[SettingsSection] prefix, the sequence resolves under the prefixed section. + + + Add `[assembly:InternalsVisibleTo("ExistForAll.SimpleSettings.Binders")]` to Info.cs so the + Binders package can call Core's internal `IsCollectionShape` (one source of truth for shape detection; + duplicating the predicate risks drift with the converter chain). CRITICAL (review B-1): the grant target is + the Binders project's assembly SIMPLE NAME `ExistForAll.SimpleSettings.Binders` — its csproj is + `ExistForAll.SimpleSettings.Binders.csproj` with no `` override (the existing + `...UnitTests`/`...Benchmark` grants in Info.cs confirm this simple-name convention). Do NOT use the + directory name `ExistForAll.SimpleSettings.Extensions.Binders` — that assembly does not exist, Core's + internal predicate would stay invisible, and `ConfigurationBinder` would fail to compile with CS0122. + In `BindPropertySettings`, keep the + existing `_sections.GetOrAdd(context.Section, static (name, self) => self.ResolveSection(name), this)` + resolve. Per D-04/D-05, BEFORE the scalar read, when `context.PropertyType.IsCollectionShape()`: + materialize the children of `section.GetSection(context.Key).GetChildren()` into a `string[]` using a MANUAL + right-sized loop over the child sections — do NOT use `.Select(...).Where(...).ToArray()` (review S-5: LINQ + projection allocates iterators/closures on the collection-bind path; match the codebase's manual-walk + convention in `CollectionTypeConverter`/`TypeConverter`). Count the children (or the non-null `.Value`s), + allocate one `string[]` of that size, and fill it; if that array is non-empty call `context.SetNewValue(values)` + and return (children win). Otherwise fall through to the scalar `section[context.Key]`; for collection-shape targets, + when the scalar `IsNullOrWhiteSpace`, do NOT call `SetNewValue` (let COLL-02's empty-not-null nullResult + apply — avoids the `[" "]` whitespace-token pitfall). For non-collection targets keep the exact existing + scalar behavior (`if (value != null) context.SetNewValue(value)`). No converter change: `AsArray` passes + the produced `string[]` straight through the element chain. Prefixing is inherited for free because + `GetSection(key)` is a child of the already root-resolved section. Do NOT put any element value into an + exception message (D-06 — this runs inside ValuesPopulator's SettingsBindingException catch). Extend + `ConfigBuilderConfigurationBinderIntegrationTests.cs` with the five behavior tests, building the + IConfiguration via `new ConfigurationBuilder().AddInMemoryCollection(...)` with indexed keys + (`Section:Values:0`, `Section:Values:1`) for the sequence cases; add nested settings fixtures as needed. + Follow `modern-csharp` + `testing`; preserve the static-factory GetOrAdd (no per-call closure). + + + cd src && dotnet test SimpleSettings.slnx -c Release -f net10.0 --treenode-filter "/*/*/SettingsBuilderConfigurationBinderIntegrationTests/*" + + + - Info.cs contains `InternalsVisibleTo("ExistForAll.SimpleSettings.Binders")` (the Binders project's assembly SIMPLE NAME — csproj is `ExistForAll.SimpleSettings.Binders.csproj` with no `` override — NOT the directory name `...Extensions.Binders`; review B-1). + - PRIMARY B-1 GATE (cannot false-pass on the string): `cd src && dotnet build SimpleSettings.slnx -c Release` SUCCEEDS for net8.0 AND net10.0, proving `ConfigurationBinder` in the Binders assembly actually compiles against Core's `internal` `IsCollectionShape`/`IsListLike` shape predicate. A wrong IVT target (e.g. the directory name) leaves the internal invisible and the build fails with CS0122 even though Info.cs "contains an InternalsVisibleTo string". + - `ConfigurationBinder.BindPropertySettings` calls `GetSection(context.Key).GetChildren()` guarded by `IsCollectionShape` and returns early when children exist (children win). + - The comma-scalar regression test binds [a,b,c] (scalar path preserved for prod override). + - The whitespace/empty-sequence tests yield Count 0, never a one-element collection. + - The prefix test binds under the RootSection/[SettingsSection]-prefixed section. + - The GetChildren() -> `string[]` projection is built with a manual right-sized loop (no LINQ Select/Where/ToArray on the bind path; review S-5). + - The integration test run is green on net10. + + Child-section sequences bind (children win); comma-scalar still binds; empty/whitespace -> empty; prefix honored; each element flows the converter chain; integration tests green on net10. + + + + Task 2: S1 sequence-element redaction regression (COLL-03/D-06 — SECURITY GATE, blocking high) + + - src/Tests/ExistForAll.SimpleSettings.UnitTests/Conversion/ExceptionRedactionTests.cs (the Secret sentinel + AssertRedacted helper asserting absence from full ToString(); the pattern to extend) + - src/Core/ExistForAll.SimpleSettings/ValuesPopulator.cs (:114-128 ConvertPropertyValue value-free wrapper; :50-60 SettingsBindingException wrapping catch) + - src/Core/ExistForAll.SimpleSettings/SettingsBindingException.cs (retains only binder type/section/key primitives, never the BindingContext/value) + - src/Core/ExistForAll.SimpleSettings.Extensions.Binders/ConfigurationBinder.cs (the sequence path from Task 1 — the code under test) + - 04-RESEARCH.md § Security Domain (D-06 gate, blocking severity high) + Known Threat Patterns + - 04-VALIDATION.md row "COLL-03 / S1" (threat ref T-04-S1) + + src/Tests/ExistForAll.SimpleSettings.UnitTests/Conversion/ExceptionRedactionTests.cs + + - Test: an int[] property is bound via a ConfigurationBinder from a child-sequence whose SECOND element is the secret sentinel (Section:Values:0="1", Section:Values:1=SECRET); building the settings throws SettingsPropertyValueException; the secret sentinel appears NOWHERE in the thrown exception's full ToString() chain, while the property name and target type name are present. + - Test (S-6, first-element case): the same int[] path with the secret in the FIRST element (Section:Values:0=SECRET) is equally redacted — the throw site is the per-element convert, so element position must not matter. + - Test (S-6, List<int> variant): a List<int> property bound from a child-sequence whose element is the secret sentinel is redacted identically — the `ListTypeConverter` wraps into a `List` only AFTER the element-convert throw site, so the redaction path is the shared `CollectionTypeConverter` element convert; pin the guarantee across BOTH converter shapes (array + list). + + + Extend `ExceptionRedactionTests.cs` with `Convert_SecretInSequenceElement_DoesNotLeakValue` per D-06. + Reuse the existing `Secret` constant and `AssertRedacted` helper. Bind a collection-typed property + (e.g. an int[] fixture) through a `Microsoft.Extensions.Configuration` `ConfigurationBinder` built from + `new ConfigurationBuilder().AddInMemoryCollection(...)` with a child sequence where one element is the + secret sentinel that fails element conversion. Capture the thrown `SettingsPropertyValueException` (the + element convert failure is wrapped value-free by `ValuesPopulator.ConvertPropertyValue`) and assert via + `AssertRedacted` that the full `ToString()` excludes the sentinel and includes the property name + target + type name. Per review S-6, cover THREE cases (data-driven or three methods): (a) int[] with the secret in a + later element, (b) int[] with the secret in the FIRST element, (c) a `List` fixture with the secret + element. Rationale to preserve in the test intent: the throw site is the shared per-element convert inside + `CollectionTypeConverter`, and `ListTypeConverter` wraps into a `List` only AFTER that throw site — so the + redaction contract is identical across element position and across the array/list converter shapes; pinning + all three prevents a future converter-shape change from silently opening a leak. These tests exercise the NEW + GetChildren() sequence path specifically — do NOT reuse the existing scalar InMemoryBinder fixtures. This is + the D-06 security gate owned by `security-auditor`: surface it as a blocking-human checkpoint for review + sign-off before the plan is considered complete. Follow `dotnet-claude-kit:testing`. + + + cd src && dotnet test SimpleSettings.slnx -c Release -f net10.0 --treenode-filter "/*/*/ExceptionRedactionTests/*" + + + - A test named `Convert_SecretInSequenceElement_DoesNotLeakValue` exists in ExceptionRedactionTests and drives the ConfigurationBinder GetChildren() sequence path (not the scalar InMemoryBinder path). + - The test asserts the secret sentinel is absent from the entire `ex.ToString()` chain and that the property name + target type name are present. + - S-6 coverage: the guarantee is pinned for int[] with the secret in the first element AND in a later element, AND for a `List` fixture (both converter shapes), not just a single array/later-element case. + - The full ExceptionRedactionTests run (including the five pre-existing redaction tests) is green on net10. + - security-auditor sign-off recorded (blocking, severity high). + + A secret in a YAML/child sequence element is provably absent from the whole exception ToString() on convert failure; ExceptionRedactionTests green on net10; security-auditor approved. + + + + + +## Trust Boundaries + +| Boundary | Description | +|----------|-------------| +| config provider -> binder | Untrusted config sequences (possibly containing secrets) are read via GetChildren() and set onto the binding context | +| bind/convert failure -> host logs | SettingsBindingException / SettingsPropertyValueException ToString() reaches host logs | + +## STRIDE Threat Register + +| Threat ID | Category | Component | Severity | Disposition | Mitigation Plan | +|-----------|----------|-----------|----------|-------------|-----------------| +| T-04-S1 | Information Disclosure | ConfigurationBinder GetChildren() sequence path | high | mitigate | Convert failures on a sequence element surface as the value-free `SettingsPropertyValueException` (property/target/failure-type only); `SettingsBindingException` retains only binder type/section/key. New regression tests (Task 2) prove the sentinel is absent from the whole ToString() across element position (first + later) and both converter shapes (int[] + List<int>) per S-6. Owned by security-auditor, blocking. | +| T-04-02 | Tampering | children-win precedence | medium | mitigate | Children are read first and win over the scalar deterministically; comma-scalar regression test guards the prod override; no reorder of the binder chain. | +| T-04-03 | Denial of Service | large child sequence | low | accept | Sequence length is bounded by operator-controlled config; no untrusted remote input; consistent with existing scalar binding exposure. | +| T-04-SC | Supply Chain | package installs | n/a | accept | No package installs this phase (04-RESEARCH.md Package Legitimacy Audit: N/A). | + + + +- `cd src && dotnet build SimpleSettings.slnx -c Release` succeeds for net8.0 AND net10.0 — this is the B-1 gate: it proves the `InternalsVisibleTo("ExistForAll.SimpleSettings.Binders")` grant actually exposes Core's internal `IsCollectionShape`/`IsListLike` to `ConfigurationBinder` (a wrong assembly-name target fails with CS0122). +- SettingsBuilderConfigurationBinderIntegrationTests + ExceptionRedactionTests green on net10 (net8 in CI). +- The five pre-existing ExceptionRedactionTests remain green (no S1 regression from the binder edit — ROADMAP criterion 6). +- Full suite green on net10 locally after this wave; net8 + net10 in CI. + + + +ROADMAP success criterion 2 (COLL-03 sequence bind; comma-scalar preserved; children win; element chain) and +the sequence-path portion of criterion 6 (S1/SEC-01 re-verified after the BindPropertySettings edit) are met, +proven by green integration + redaction tests on net10. + + + +Create `.planning/phases/04-collection-validation-binding/04-02-SUMMARY.md` when done. + diff --git a/.planning/phases/04-collection-validation-binding/04-02-SUMMARY.md b/.planning/phases/04-collection-validation-binding/04-02-SUMMARY.md new file mode 100644 index 0000000..896c7fb --- /dev/null +++ b/.planning/phases/04-collection-validation-binding/04-02-SUMMARY.md @@ -0,0 +1,161 @@ +--- +phase: 04-collection-validation-binding +plan: 02 +subsystem: binders +tags: [configuration-binder, collections, sequence-binding, internals-visible-to, redaction, security, dotnet, tunit] + +# Dependency graph +requires: + - phase: 04-collection-validation-binding + provides: "IsCollectionShape/IsListLike shape predicates + COLL-02 empty-not-null collection default (Plan 01)" +provides: + - "ConfigurationBinder binds collection properties from IConfiguration child-section sequences (Section:Key:0, :1, ...) as a string[] flowing the element converter chain (COLL-03/D-04)" + - "Children-win precedence over a coexisting scalar; comma-scalar override preserved; whitespace/empty -> empty collection (D-05)" + - "InternalsVisibleTo grant to the Binders assembly so Core's IsCollectionShape is the single source of shape truth" + - "S1/SEC-01 secret-redaction invariant re-verified on the new sequence path across element position and both array/list converter shapes (D-06)" +affects: [validation, VAL-01, source-binding] + +# Tech tracking +tech-stack: + added: [] + patterns: + - "Manual two-pass right-sized string[] projection over IConfigurationSection.GetChildren() (count then fill) instead of LINQ Select/Where/ToArray on the bind path (review S-5)" + - "Cross-assembly reuse of Core's internal shape predicate via InternalsVisibleTo rather than duplicating the predicate in the Binders package (no drift with the converter chain)" + +key-files: + created: [] + modified: + - src/Core/ExistForAll.SimpleSettings/Info.cs + - src/Core/ExistForAll.SimpleSettings.Extensions.Binders/ConfigurationBinder.cs + - src/Tests/ExistForAll.SimpleSettings.UnitTests/ConfigBuilderConfigurationBinderIntegrationTests.cs + - src/Tests/ExistForAll.SimpleSettings.UnitTests/Conversion/ExceptionRedactionTests.cs + +key-decisions: + - "IVT grant targets the Binders project assembly SIMPLE NAME `ExistForAll.SimpleSettings.Binders` (its csproj has no override), NOT the directory name `...Extensions.Binders` — a wrong target leaves Core's internal invisible and ConfigurationBinder fails with CS0122 (review B-1)" + - "GetChildren() -> string[] built with a manual count-then-fill loop, filtering to non-null .Value leaves, so the element converter chain runs per element with no LINQ allocation on the bind path (review S-5)" + - "For collection-shape targets the scalar fall-through skips SetNewValue when IsNullOrWhiteSpace so COLL-02's empty-not-null default applies (avoids the [\" \"] whitespace-token pitfall); non-collection scalar behavior is byte-for-byte unchanged" + +patterns-established: + - "Collection binder branch: children win, empty sequence falls through to scalar, whitespace scalar yields the empty default" + +requirements-completed: [COLL-03] + +coverage: + - id: D04 + description: "A collection property binds from a child-section sequence (Section:Key:0,:1,...) as a string[] flowing the element converter chain (string[]/int[]/List)" + requirement: "COLL-03" + verification: + - kind: integration + ref: "src/Tests/ExistForAll.SimpleSettings.UnitTests/ConfigBuilderConfigurationBinderIntegrationTests.cs#Bind_ChildSequence_ToStringArray_BindsEachElement (+ ToIntArray, ToStringList)" + status: pass + human_judgment: false + - id: D05a + description: "When both a scalar and children exist for the key, children win" + requirement: "COLL-03" + verification: + - kind: integration + ref: "src/Tests/ExistForAll.SimpleSettings.UnitTests/ConfigBuilderConfigurationBinderIntegrationTests.cs#Bind_ScalarAndChildren_ChildrenWin" + status: pass + human_judgment: false + - id: D05b + description: "The comma-scalar form still binds unchanged (prod MultiHost__CommonHosts override preserved)" + requirement: "COLL-03" + verification: + - kind: integration + ref: "src/Tests/ExistForAll.SimpleSettings.UnitTests/ConfigBuilderConfigurationBinderIntegrationTests.cs#Bind_CommaScalar_NoChildren_StillBinds" + status: pass + human_judgment: false + - id: D05c + description: "Whitespace scalar and empty child-sequence -> empty collection (Count 0), never a single blank element" + requirement: "COLL-03" + verification: + - kind: integration + ref: "src/Tests/ExistForAll.SimpleSettings.UnitTests/ConfigBuilderConfigurationBinderIntegrationTests.cs#Bind_WhitespaceScalar_ToStringArray_BindsEmpty, #Bind_EmptySequence_BindsEmpty" + status: pass + human_judgment: false + - id: D05d + description: "RootSection/[SettingsSection] prefix honored on the sequence path exactly as on the scalar path" + requirement: "COLL-03" + verification: + - kind: integration + ref: "src/Tests/ExistForAll.SimpleSettings.UnitTests/ConfigBuilderConfigurationBinderIntegrationTests.cs#Bind_ChildSequence_WithRootSection_ResolvesUnderPrefix" + status: pass + human_judgment: false + - id: D06 + description: "A secret carried in a sequence element is absent from the entire ex.ToString() chain on convert failure — across element position (first + later) and both array/list converter shapes (S1/T-04-S1)" + requirement: "COLL-03" + verification: + - kind: unit + ref: "src/Tests/ExistForAll.SimpleSettings.UnitTests/Conversion/ExceptionRedactionTests.cs#Convert_SecretInSequenceElement_DoesNotLeakValue, #Convert_SecretInFirstSequenceElement_DoesNotLeakValue, #Convert_SecretInListSequenceElement_DoesNotLeakValue" + status: pass + human_judgment: true + +# Metrics +duration: 12min +completed: 2026-07-15 +status: complete +--- + +# Phase 04 Plan 02: ConfigurationBinder Child-Section Sequence Binding Summary + +**ConfigurationBinder now binds collection properties from IConfiguration child-section sequences (children win, comma-scalar preserved, whitespace/empty -> empty), reusing Core's internal IsCollectionShape via an InternalsVisibleTo grant, with the S1 secret-redaction invariant re-proven on the new path across element position and both array/list converter shapes.** + +## Performance + +- **Duration:** ~12 min +- **Completed:** 2026-07-15 +- **Tasks:** 2 (both TDD) +- **Files modified:** 4 (0 created, 4 modified) + +## Accomplishments +- A collection property expressed as an indexed sub-section (`Section:Key:0`, `:1`, ...) binds as a `string[]` flowing the existing element converter chain; `string[]`, `int[]`, and `List` all bind correctly (COLL-03/D-04). +- Children win over a coexisting scalar; the comma-scalar override still binds (prod `MultiHost__CommonHosts` regression guard preserved); whitespace scalar and empty sequence both yield an empty collection (Count 0), never a `[" "]` blank element; the RootSection prefix is inherited on the sequence path (D-05). +- The Binders assembly reuses Core's `internal` `IsCollectionShape` predicate via a new `InternalsVisibleTo("ExistForAll.SimpleSettings.Binders")` grant — one source of shape truth, no drift with the converter chain. +- The GetChildren() -> `string[]` projection uses a manual two-pass count-then-fill loop (no LINQ on the bind path; review S-5). +- D-06 security gate: the S1 secret-redaction invariant is re-proven on the new sequence path — a secret element that fails to convert is absent from the whole `SettingsPropertyValueException.ToString()` chain, pinned across the first-element and later-element positions and across the `int[]` and `List` converter shapes (S-6). + +## Task Commits + +Each task was committed atomically (TDD RED -> GREEN): + +1. **Task 1: GetChildren() child-sequence branch (COLL-03/D-04, D-05)** — `af63c18` (test) -> `26446f2` (feat) +2. **Task 2: D-06 sequence-element redaction regression (SECURITY GATE)** — `3402ac6` (test; regression pin on existing value-free wrapper, no production change needed) + +**Plan metadata:** _(final docs commit — see git log)_ + +## Files Created/Modified +- `Core/ExistForAll.SimpleSettings/Info.cs` — added `InternalsVisibleTo("ExistForAll.SimpleSettings.Binders")` (assembly simple name; B-1). +- `Core/ExistForAll.SimpleSettings.Extensions.Binders/ConfigurationBinder.cs` — added the `IsCollectionShape`-guarded `TrySetChildSequence` branch (manual two-pass right-sized `string[]`), children-win early return, and the collection-target whitespace/empty scalar fall-through that defers to COLL-02's empty default; non-collection scalar path unchanged. +- `Tests/.../ConfigBuilderConfigurationBinderIntegrationTests.cs` — 8 new behavior tests + 3 fixture interfaces (sequence bind for string[]/int[]/List, children-win, comma-scalar guard, whitespace->empty, empty-sequence->empty, prefix). +- `Tests/.../Conversion/ExceptionRedactionTests.cs` — 3 new D-06 redaction tests + a `CaptureSequenceConversionFailure` helper + 2 fixture interfaces (int[] later/first element, List). + +## Decisions Made +None beyond the plan — implemented exactly as specified. The IVT target was verified against the Binders csproj (`ExistForAll.SimpleSettings.Binders.csproj`, no `` override) so the simple name `ExistForAll.SimpleSettings.Binders` is correct, and the B-1 build gate proved the internal predicate is actually visible. + +## Deviations from Plan + +None - plan executed exactly as written. + +## Security Gate (D-06) + +- The D-06 secret-redaction regression test was implemented fully and autonomously (writing a test is implementation, not a human decision): 3 cases covering int[] secret-in-later-element, int[] secret-in-first-element, and List secret-element. All assert the sentinel is absent from the entire `ex.ToString()` chain while the property name and target-type token are present. The S1 invariant (no config/element value enters any exception ToString()) is not weakened. +- **The formal security-auditor SIGN-OFF for D-06 (blocking, severity high) is PENDING the phase-level security gate** — the orchestrator runs `/gsd-secure-phase` after the phase. The `human_judgment: true` marker on coverage row D06 reflects that pending sign-off. + +## Issues Encountered +- A solution-wide `dotnet build -f net8.0` still trips `NETSDK1005` on the net10-only Benchmark project (pre-existing, out of scope, documented in Plan 01). The authoritative `dotnet build SimpleSettings.slnx -c Release` (no `-f`) builds both net8.0 and net10.0 per-project TFMs with 0 warnings/0 errors — this is the B-1 gate and it passed, proving the IVT grant exposes Core's internal shape predicate to the Binders assembly. + +## Verification +- **B-1 gate:** `cd src && dotnet build SimpleSettings.slnx -c Release` -> 0 warnings, 0 errors; `ExistForAll.SimpleSettings.Binders.dll` built for BOTH net8.0 and net10.0 (compiles against Core's `internal` `IsCollectionShape`). +- `SettingsBuilderConfigurationBinderIntegrationTests` -> 13/13 pass on net10 (5 pre-existing + 8 new). +- `ExceptionRedactionTests` -> 8/8 pass on net10 (5 pre-existing redaction tests + 3 new D-06 cases). +- Full suite net10 -> 125/125 pass. + +## User Setup Required +None - no external service configuration required. + +## Next Phase Readiness +- Collection source binding (COLL-03) is complete; validation (VAL-01) can now inspect collection-typed properties bound from either scalar or sequence sources. +- D-06 security-auditor sign-off is queued for the phase-level `/gsd-secure-phase` gate. +- No blockers. + +## Self-Check: PASSED diff --git a/.planning/phases/04-collection-validation-binding/04-03-PLAN.md b/.planning/phases/04-collection-validation-binding/04-03-PLAN.md new file mode 100644 index 0000000..6c17056 --- /dev/null +++ b/.planning/phases/04-collection-validation-binding/04-03-PLAN.md @@ -0,0 +1,279 @@ +--- +phase: 04-collection-validation-binding +plan: 3 +type: execute +wave: 2 +depends_on: [04-01] +files_modified: + - src/Core/ExistForAll.SimpleSettings/Validations/ISettingsValidator.cs + - src/Core/ExistForAll.SimpleSettings/Validations/ISettingValidation.cs + - src/Core/ExistForAll.SimpleSettings/Validations/ValidationContext.cs + - src/Core/ExistForAll.SimpleSettings/Validations/ValidationContextOfT.cs + - src/Core/ExistForAll.SimpleSettings/SettingsValidatorAttribute.cs + - src/Core/ExistForAll.SimpleSettings/SettingsValidationException.cs + - src/Core/ExistForAll.SimpleSettings/Resources.cs + - src/Core/ExistForAll.SimpleSettings/SettingsPlan.cs + - src/Core/ExistForAll.SimpleSettings/ValuesPopulator.cs + - src/Tests/ExistForAll.SimpleSettings.UnitTests/SimpleSettings/SettingsValidationTests.cs +autonomous: true +requirements: [VAL-01] + +must_haves: + truths: + - "An object-level [SettingsValidator(typeof(X))] on a settings interface is instantiated (Activator, parameterless ctor) and invoked against the fully-populated instance; a failing validator throws SettingsValidationException (D-11/D-10)." + - "A property-level [SettingsProperty(ValidatorType=typeof(X))] validator runs on that property's value (D-11)." + - "Multiple validation failures aggregate into ONE SettingsValidationException carrying the full ValidationError list (D-10)." + - "A cross-property rule validates against the fully-populated instance (validators run AFTER the property-set loop) (D-09)." + - "SettingsValidationException.ToString() contains only author-supplied ValidationError messages — the exception itself adds no bound values (D-12/S1)." + - "SettingsValidationException is catchable as SimpleSettingsException (hierarchy invariant)." + - "When a settings type declares NO validators, the post-populate hook allocates ZERO new bytes — SettingsPlan.HasValidators short-circuits before any allocation, so the validator-free warm path (gated PlanPopulate/Resolve/Shape/ConvertArray/ConfigBinder benchmarks) stays byte-identical to master (review B-2)." + - "Both the core path (this plan) and the DI path (Plan 04) aggregate-and-throw through ONE shared Core helper SettingsValidationException.ThrowIfAny(errors), so the exception contract is structurally identical across the two paths (review S-3 / D-10)." + artifacts: + - "src/Core/ExistForAll.SimpleSettings/SettingsValidationException.cs (aggregate exception : SimpleSettingsException + static ThrowIfAny(IReadOnlyList) helper)" + - "src/Core/ExistForAll.SimpleSettings/SettingsValidatorAttribute.cs (object-level attribute)" + - "src/Core/ExistForAll.SimpleSettings/Validations/*.cs (sync interfaces + context ctors)" + - "src/Core/ExistForAll.SimpleSettings/SettingsPlan.cs (SettingsPlan.HasValidators + object-validator type; PropertyPlan.ValidatorType — computed once at plan build)" + key_links: + - "Validators are gathered at plan build (attribute reads threaded into SettingsPlan/PropertyPlan) — not on the hot populate path." + - "SettingsPlan.HasValidators (objectValidatorType is not null || any PropertyPlan.ValidatorType is not null) gates the hook: `if (!plan.HasValidators) return;` runs before any allocation; the List accumulator is allocated lazily only after a validator actually produces an error (review B-2)." + - "The validation hook runs AFTER the property-set loop in ValuesPopulator.PopulateInstanceWithValues so cross-property rules see every property." + - "Reflective ISettingValidation.Validate dispatch selects the overload explicitly via GetMethod(\"Validate\", new[]{ closedContextType }) (avoids AmbiguousMatchException) and constructs ValidationContext via MakeGenericType (review S-2)." + - "SettingsValidationException derives SimpleSettingsException with a Resources factory message; it never embeds a bound value (D-12); the aggregate-and-throw is centralized in SettingsValidationException.ThrowIfAny for both paths (S-3)." +--- + + +Wire declared settings validation into the core populate path (VAL-01 core path): redesign the held +validator interfaces to synchronous (D-07), give the validation contexts a constructor (D-08), add an +object-level `[SettingsValidator]` attribute + a property-level `ValidatorType` hook (D-11 core path), run +all applicable validators after the property-set loop (D-09), aggregate every error, and throw one +`SettingsValidationException : SimpleSettingsException` (D-10) that leaks no bound values (D-12). Depends on +Plan 01 so validators can inspect correctly-bound collection/list properties. The DI-resolved validator +path + API-02 exposure are Plan 04. + +Purpose: The held `Validations/*` scaffolding is dead (never invoked). The client pre-beta batch requires a +settings object to validate itself, including cross-property rules, in the bind pipeline. +Output: Sync validator interfaces, constructed contexts, a new object-level attribute, a new aggregate +exception + Resources message, plan-build attribute threading, the post-populate hook, and a new +`SettingsValidationTests` suite. + +Skills: invoke `dotnet-claude-kit:modern-csharp` (C# 12-safe records/pattern matching, no C# 14-only +features) for the new types and `dotnet-claude-kit:testing` for the TUnit suite. + +Architecture note: this plan establishes the aggregate + redaction contract that Plan 04's DI-resolved path +must reproduce byte-identically. dotnet-architect + security-auditor confirm the contract shape. + +Note (review N-3, OPTIONAL — NOT adopted here): a default-interface-method bridge on `ISettingValidation` +(implementing the base `ISettingsValidator.Validate(ValidationContext)` by forwarding to the generic overload, +net8+/C# 8) would let authors implement only the generic `Validate` AND would let both runners dispatch via +the single non-generic `ISettingsValidator.Validate` — dissolving S-2's AmbiguousMatchException and removing +the `MakeGenericType` context construction. This is a larger contract change than a targeted revision warrants, +so it is deliberately deferred: S-2's explicit-overload-selection fix stands as the authoritative resolution. +If a later on-branch pass adopts the DIM bridge (reversal is free pre-beta), reconcile S-2 accordingly in both +this plan and Plan 04. + + + +@/Users/guyludvig/frontegg/development/open-source/SimpleSettings/.claude/gsd-core/workflows/execute-plan.md +@/Users/guyludvig/frontegg/development/open-source/SimpleSettings/.claude/gsd-core/templates/summary.md + + + +@.planning/PROJECT.md +@.planning/ROADMAP.md +@.planning/STATE.md +@.planning/phases/04-collection-validation-binding/04-CONTEXT.md +@.planning/phases/04-collection-validation-binding/04-RESEARCH.md +@.planning/phases/04-collection-validation-binding/04-PATTERNS.md +@.planning/phases/04-collection-validation-binding/04-VALIDATION.md +@.planning/codebase/CONVENTIONS.md + +@src/Core/ExistForAll.SimpleSettings/Validations/ISettingValidation.cs +@src/Core/ExistForAll.SimpleSettings/Validations/ISettingsValidator.cs +@src/Core/ExistForAll.SimpleSettings/Validations/ValidationContext.cs +@src/Core/ExistForAll.SimpleSettings/Validations/ValidationContextOfT.cs +@src/Core/ExistForAll.SimpleSettings/Validations/ValidationResult.cs +@src/Core/ExistForAll.SimpleSettings/Validations/ValidationError.cs +@src/Core/ExistForAll.SimpleSettings/SettingsPropertyAttribute.cs +@src/Core/ExistForAll.SimpleSettings/SimpleSettingsException.cs +@src/Core/ExistForAll.SimpleSettings/SettingsPropertyValueException.cs +@src/Core/ExistForAll.SimpleSettings/Resources.cs +@src/Core/ExistForAll.SimpleSettings/ValuesPopulator.cs +@src/Core/ExistForAll.SimpleSettings/SettingsPlan.cs +@src/Tests/ExistForAll.SimpleSettings.UnitTests/SimpleSettings/ExceptionHierarchyTests.cs + + + +New symbols introduced by THIS plan (exclude from drift/orphan checks): +- `ExistForAll.SimpleSettings.SettingsValidationException` (public : SimpleSettingsException, carries IReadOnlyList Errors) +- `ExistForAll.SimpleSettings.SettingsValidationException.ThrowIfAny(IReadOnlyList errors)` (public static — aggregate-and-throw helper reused by Plan 04's DI runner; review S-3) +- `ExistForAll.SimpleSettings.SettingsValidatorAttribute` (public, AttributeTargets.Interface, Type ValidatorType) +- `Resources.SettingsValidationExceptionMessage(...)` +- Sync `ISettingsValidator.Validate` / `ISettingValidation.Validate` (signature change: drop Task<>) +- `ValidationContext(object settings)` + `ValidationContext(T settings)` constructors +- New validator fields on `SettingsPlan` / `PropertyPlan` (object-level validator type + per-property ValidatorType) plus `SettingsPlan.HasValidators` computed at plan build (review B-2) +- New test class `SettingsValidationTests` + its fixtures/validators + + + + + + Task 1: Validation contracts — sync interfaces, context ctors, attribute, aggregate exception (D-07/D-08/D-10/D-11/D-12) + + - src/Core/ExistForAll.SimpleSettings/Validations/ISettingValidation.cs + ISettingsValidator.cs (currently return Task; dead scaffolding -> make sync) + - src/Core/ExistForAll.SimpleSettings/Validations/ValidationContext.cs + ValidationContextOfT.cs (getter-only, no ctor -> add ctors) + - src/Core/ExistForAll.SimpleSettings/Validations/ValidationResult.cs + ValidationError.cs (already complete: AddError/Errors/IsValid; ValidationError(settingsName, errorMessage) — do NOT change) + - src/Core/ExistForAll.SimpleSettings/SettingsPropertyAttribute.cs (analog for the new attribute shape; AttributeTargets differs) + - src/Core/ExistForAll.SimpleSettings/SimpleSettingsException.cs + SettingsPropertyValueException.cs (base ctors + value-free message via Resources) + - src/Core/ExistForAll.SimpleSettings/Resources.cs (static factory-message convention :35-43) + - src/Tests/ExistForAll.SimpleSettings.UnitTests/SimpleSettings/ExceptionHierarchyTests.cs (reflection invariant every exception derives SimpleSettingsException — auto-covers the new type) + - 04-PATTERNS.md § "SettingsValidationException.cs", § "SettingsValidatorAttribute.cs", § "Validations/*" + + src/Core/ExistForAll.SimpleSettings/Validations/ISettingsValidator.cs, src/Core/ExistForAll.SimpleSettings/Validations/ISettingValidation.cs, src/Core/ExistForAll.SimpleSettings/Validations/ValidationContext.cs, src/Core/ExistForAll.SimpleSettings/Validations/ValidationContextOfT.cs, src/Core/ExistForAll.SimpleSettings/SettingsValidatorAttribute.cs, src/Core/ExistForAll.SimpleSettings/SettingsValidationException.cs, src/Core/ExistForAll.SimpleSettings/Resources.cs + + Per D-07, change `ISettingsValidator.Validate` to return `ValidationResult` (drop `Task<>`) and + `ISettingValidation.Validate(ValidationContext)` to return `ValidationResult`; the interfaces are + never invoked today so the change is free. Per D-08, add `ValidationContext(object settings)` assigning + the base `Settings`, and `ValidationContext(T settings)` calling `base(settings)` and setting the + shadowed `T? Settings` (follow the `ValidationError` ctor-assigns-getter shape). Create + `SettingsValidatorAttribute` (public, `[AttributeUsage(AttributeTargets.Interface)]`, `Type ValidatorType`, + ctor `SettingsValidatorAttribute(Type validatorType)`) per D-11 — analog `SettingsPropertyAttribute` but + Interface-targeted, one public type per file. Create `SettingsValidationException : SimpleSettingsException` + (public) per D-10/D-12: ctor takes `IReadOnlyList`, exposes `IReadOnlyList Errors`, + and builds its message via a NEW `Resources.SettingsValidationExceptionMessage(...)` factory composed ONLY + from author-supplied `ValidationError.ErrorMessage`/`SettingsName` — embed no bound values (mirror the S1 + comment intent of SettingsPropertyValueException explaining why no value is present). Add the Resources + factory following the existing static-method convention. Also add a public `static void ThrowIfAny(IReadOnlyList errors)` + on `SettingsValidationException` (review S-3): it throws ONE aggregated `SettingsValidationException(errors)` + when the list is non-empty and returns otherwise. This is the SINGLE aggregate-and-throw entry point that + BOTH the core path (this plan, Task 2) and the DI-resolved runner (Plan 04) call, so the thrown contract + (type + Errors + value-free message) cannot drift between the two paths. No behavior wiring in this task. Follow + `modern-csharp`; block-scoped namespaces; tabs to match surrounding Core files; exceptions/attributes in + the root `ExistForAll.SimpleSettings` namespace per convention. + + + cd src && dotnet build SimpleSettings.slnx -c Release -f net10.0 && dotnet test SimpleSettings.slnx -c Release -f net10.0 --treenode-filter "/*/*/ExceptionHierarchyTests/*" + + + - `ISettingsValidator.Validate` and `ISettingValidation.Validate` return `ValidationResult` (no `Task` in the Validations interface files). + - `ValidationContext` and `ValidationContext` each expose a constructor that carries the settings instance. + - `SettingsValidatorAttribute` exists (AttributeTargets.Interface, Type ValidatorType) and `SettingsValidationException : SimpleSettingsException` exposes `Errors`. + - `SettingsValidationException.ThrowIfAny(IReadOnlyList)` exists (public static): throws one aggregated exception when the list is non-empty, returns when empty (review S-3). + - Solution builds for net8.0 AND net10.0; ExceptionHierarchyTests green on net10 (proves the new exception joins the catchable family). + - `SettingsValidationException`/its Resources message contain no parameter carrying a bound property value. + + Sync validator contracts, constructed contexts, the object-level attribute, and the aggregate exception (+ Resources message) exist; build green both TFMs; ExceptionHierarchyTests green. + + + + Task 2: Core-path validation execution in ValuesPopulator + plan-build threading + tests (D-09/D-10/D-11) + + - src/Core/ExistForAll.SimpleSettings/ValuesPopulator.cs (:29-65 populate loop — hook AFTER the foreach; :76-103 GetOrBuildPlan where [SettingsProperty] is read at :84 — thread ValidatorType here) + - src/Core/ExistForAll.SimpleSettings/SettingsPlan.cs (SettingsPlan + PropertyPlan readonly struct — add validator fields resolved at plan build) + - src/Core/ExistForAll.SimpleSettings/Core/Reflection/TypeExtensions.cs (GetTypeInfo().GetCustomAttribute(true) idiom :14 for reading the object-level attribute) + - src/Core/ExistForAll.SimpleSettings/Core/Reflection/TypeConverter.cs (Activator.CreateInstance(attribute.X) idiom :43 for instantiating validators) + - src/Core/ExistForAll.SimpleSettings/Validations/ISettingValidation.cs + ISettingsValidator.cs (NOTE for review S-2: `ISettingValidation : ISettingsValidator` exposes TWO `Validate` overloads — a plain `GetMethod("Validate")` on the closed generic throws AmbiguousMatchException) + - src/Tests/ExistForAll.SimpleSettings.UnitTests/SimpleSettings/SettingsPropertyTests.cs + DependencyInjection/AddSimpleSettingsIntegrationTests.cs (TUnit conventions, nested non-indicated fixtures, GetSettings() usage) + - 04-PATTERNS.md § "ValuesPopulator.cs — validation hook" + § "Attribute read at plan build" + § "Activator-based instantiation" + § "Perf-conscious iteration" + - 04-RESEARCH.md Pattern 3 (hook point, discovery, aggregate) + Open Questions Q2 (property-level contract shape) + + src/Core/ExistForAll.SimpleSettings/SettingsPlan.cs, src/Core/ExistForAll.SimpleSettings/ValuesPopulator.cs, src/Tests/ExistForAll.SimpleSettings.UnitTests/SimpleSettings/SettingsValidationTests.cs + + - Test: an interface with [SettingsValidator(typeof(FailingValidator))] whose validator adds an error throws SettingsValidationException; a passing validator does not throw. + - Test: a property with [SettingsProperty(ValidatorType=typeof(PropValidator))] runs PropValidator against that property's value (ValidationContext carries the property value per Q2 recommendation). + - Test: a validator that adds two errors (or two validators each adding one) yields ONE SettingsValidationException whose Errors has 2 entries. + - Test: a cross-property validator reading two properties of the fully-populated instance fires only after both are set. + - Test (D-12): SettingsValidationException.ToString() contains the author-supplied error message text and NOT any injected bound value. + - Test (B-2 fast path): a settings type declaring NO validators (HasValidators == false) populates and returns without throwing and without entering the gather path; the zero-allocation guarantee itself is enforced by the benchmark allocation gate (see verification), not a unit assertion. + + + At plan build (`GetOrBuildPlan`), per D-11 core path: read the object-level `SettingsValidatorAttribute` + off the settings type via `GetTypeInfo().GetCustomAttribute(true)` and store its + `ValidatorType` on `SettingsPlan`; thread each property's `[SettingsProperty(ValidatorType)]` (already read + at :84) into the `PropertyPlan` (add a field). Resolve validators at build, not per populate. + + ZERO-ALLOCATION SHORT-CIRCUIT (review B-2 — allocation-gate blocker; the gated benchmark fixtures are all + validator-free on the warm populate path, so ANY per-resolve allocation added after the property loop + regresses the byte gate). Compute a `bool HasValidators` ONCE at plan build and store it on the cached + `SettingsPlan`: `HasValidators = objectValidatorType is not null || any PropertyPlan has a non-null + ValidatorType`. The post-populate hook MUST start with `if (!plan.HasValidators) return;` executed BEFORE + allocating anything — no accumulator list, no "applicable validators" list, no enumerator that allocates. + Only AFTER a validator actually produces at least one error may the `List` be lazily + allocated (start it null; create it on first error). This keeps the validator-free warm path byte-identical + to master. + + After the `foreach (var propertyPlan in plan.Properties)` loop in `PopulateInstanceWithValues` (D-09 — the + instance is now fully populated so cross-property rules see everything) and past the HasValidators guard, + gather the applicable core-path validators: instantiate the object-level `ValidatorType` via + `Activator.CreateInstance` (parameterless ctor) and invoke its `ISettingValidation.Validate` with + a `ValidationContext(instance)`; for each property-level `ValidatorType`, instantiate via + `Activator` and invoke `ISettingValidation.Validate` with a `ValidationContext(propertyValue)` + per Q2 (the context carries the property value). Because `TSettings`/`TProperty` are runtime `Type`s, this + dispatch is reflective — and `ISettingValidation : ISettingsValidator` declares TWO `Validate` overloads, + so (review S-2) resolve the method explicitly with `closedType.GetMethod("Validate", new[] { closedContextType })` + (NEVER the parameterless `GetMethod("Validate")`, which throws AmbiguousMatchException), and construct the + closed context with `Activator.CreateInstance(typeof(ValidationContext<>).MakeGenericType(t), instanceOrValue)`. + Collect every `ValidationError` from each returned `ValidationResult`; finish by calling the shared Core + helper `SettingsValidationException.ThrowIfAny(errors)` (review S-3 / D-10) rather than throwing inline — the + identical helper is reused by Plan 04's DI runner so the aggregated exception contract cannot drift. Put the + gather+aggregate in a small internal helper so both paths share it. Use manual iteration over concrete + collections on the populate-frequency path (no LINQ First/closure) per the perf convention; the validator + gather may build lists since it only runs when `HasValidators` is true. Create + `SimpleSettings/SettingsValidationTests.cs` with the behavior tests; fixtures MUST be nested, + NON-settings-indicated interfaces (no "Settings" suffix, no [SettingsSection]/ISettingsSection) resolved + via `GetSettings()` so a deliberately-failing validator never trips unrelated ScanAssemblies-based + tests. Follow `modern-csharp` + `testing`. + + + cd src && dotnet test SimpleSettings.slnx -c Release -f net10.0 --treenode-filter "/*/*/SettingsValidationTests/*" + + + - Object-level [SettingsValidator] and property-level [SettingsProperty(ValidatorType)] are both read at plan build and invoked post-populate. + - A failing validator throws SettingsValidationException; two failures aggregate into one exception with Errors.Count == 2. + - B-2: `SettingsPlan.HasValidators` is computed at plan build; the hook returns immediately (`if (!plan.HasValidators) return;`) before any allocation for validator-free types, and the `benchmark.yml` allocated-byte gate stays byte-identical to master for the PlanPopulate/Resolve/Shape/ConvertArray/ConfigBinder benchmarks. + - S-2: reflective validator dispatch selects the overload via `GetMethod("Validate", new[] { closedContextType })` (no AmbiguousMatchException) and builds `ValidationContext` via `MakeGenericType`. + - S-3: the core path aggregates and throws through `SettingsValidationException.ThrowIfAny(errors)` (the same helper Plan 04's DI runner uses), not an inline throw. + - A cross-property validator observes both properties set (runs after the property loop). + - SettingsValidationException.ToString() contains the author error text and excludes any injected bound value (D-12). + - Fixtures are non-settings-indicated (a full `ScanAssemblies(GetType().Assembly)`-based test elsewhere still passes — verified by the full suite staying green in this wave). + - SettingsValidationTests green on net10. + + Declared object- and property-level validators run in the populate pipeline including cross-property rules, aggregate into one value-free SettingsValidationException via the shared ThrowIfAny helper; validator-free types short-circuit with zero allocation (HasValidators); reflective dispatch is AmbiguousMatchException-free; SettingsValidationTests green on net10; benchmark allocation gate byte-identical; full suite still green. + + + + + +## Trust Boundaries + +| Boundary | Description | +|----------|-------------| +| validator author code -> exception message | Author-supplied ValidationError messages are composed into SettingsValidationException.ToString() which reaches host logs | +| settings instance -> validator | The fully-populated settings instance (possibly holding secret values) is handed to validators via ValidationContext | + +## STRIDE Threat Register + +| Threat ID | Category | Component | Severity | Disposition | Mitigation Plan | +|-----------|----------|-----------|----------|-------------|-----------------| +| T-04-VAL | Information Disclosure | SettingsValidationException message | high | mitigate | The exception composes its message ONLY from author-supplied ValidationError text and adds no bound value (D-12); a test asserts ToString() carries only author messages. Validator-author responsibility to not echo secrets is documented. security-auditor confirms. | +| T-04-04 | Tampering | validator instantiation via Activator | low | mitigate | Only parameterless core-path validators are Activator-instantiated; type comes from a compile-time attribute, not untrusted input. | +| T-04-05 | Denial of Service | validator throws/loops | low | accept | Validators are operator-authored startup code; failures fail-fast at build/populate — acceptable startup behavior. | +| T-04-SC | Supply Chain | package installs | n/a | accept | No package installs this phase (04-RESEARCH.md Package Legitimacy Audit: N/A). | + + + +- `cd src && dotnet build SimpleSettings.slnx -c Release` succeeds for net8.0 AND net10.0. +- SettingsValidationTests + ExceptionHierarchyTests green on net10 (net8 in CI). +- `benchmark.yml` allocation gate stays byte-identical to master for the validator-free PlanPopulate/Resolve/Shape/ConvertArray/ConfigBinder benchmarks — the HasValidators short-circuit adds no allocation on the warm path (review B-2). +- Full suite green on net10 after this wave — no auto-discovered fixture regresses existing ScanAssemblies tests. +- No bound value appears in SettingsValidationException.ToString() (D-12 / S1 re-affirmed for the validation path). + + + +ROADMAP success criterion 3 (VAL-01: declared ISettingValidation / [SettingsProperty(ValidatorType)] +invoked in the bind pipeline, including cross-property rules) is met for the core path, with aggregation and +redaction proven by green tests on net10. The DI-resolved path completes in Plan 04. + + + +Create `.planning/phases/04-collection-validation-binding/04-03-SUMMARY.md` when done. + diff --git a/.planning/phases/04-collection-validation-binding/04-03-SUMMARY.md b/.planning/phases/04-collection-validation-binding/04-03-SUMMARY.md new file mode 100644 index 0000000..db4dcae --- /dev/null +++ b/.planning/phases/04-collection-validation-binding/04-03-SUMMARY.md @@ -0,0 +1,197 @@ +--- +phase: 04-collection-validation-binding +plan: 03 +subsystem: validation +tags: [validation, reflection, attributes, exception-redaction, dotnet, tunit, zero-allocation] + +# Dependency graph +requires: + - phase: 04-collection-validation-binding + provides: "Plan 01 collection/List binding so validators inspect correctly-bound collection properties" +provides: + - "Sync ISettingsValidator/ISettingValidation.Validate contracts (Task<> dropped) + constructed ValidationContext/ValidationContext" + - "[SettingsValidator(typeof(X))] object-level attribute (AttributeTargets.Interface)" + - "SettingsValidationException : SimpleSettingsException carrying IReadOnlyList Errors, value-free message" + - "SettingsValidationException.ThrowIfAny(IReadOnlyList) shared aggregate-and-throw helper (reused by Plan 04 DI runner)" + - "Core-path validation execution in ValuesPopulator: object + property validators run post-populate, aggregated into one exception" + - "SettingsPlan.HasValidators/ObjectValidatorType + PropertyPlan.ValidatorType resolved once at plan build" +affects: [04-04, validation, DI-validator-path, API-02] + +# Tech tracking +tech-stack: + added: [] + patterns: + - "HasValidators short-circuit: single field read guards the post-populate hook before any allocation (review B-2)" + - "Reflective validator dispatch via GetMethod(name, new[]{closedContextType}) + Activator.CreateInstance(MakeGenericType) — avoids AmbiguousMatchException on the two-overload interface (review S-2)" + - "Centralized aggregate-and-throw (ThrowIfAny) so core and DI paths share one thrown contract (review S-3)" + - "Lazy error-list allocation: List created only on first produced error" + +key-files: + created: + - src/Core/ExistForAll.SimpleSettings/SettingsValidatorAttribute.cs + - src/Core/ExistForAll.SimpleSettings/SettingsValidationException.cs + - src/Tests/ExistForAll.SimpleSettings.UnitTests/SimpleSettings/SettingsValidationTests.cs + modified: + - src/Core/ExistForAll.SimpleSettings/Validations/ISettingsValidator.cs + - src/Core/ExistForAll.SimpleSettings/Validations/ISettingValidation.cs + - src/Core/ExistForAll.SimpleSettings/Validations/ValidationContext.cs + - src/Core/ExistForAll.SimpleSettings/Validations/ValidationContextOfT.cs + - src/Core/ExistForAll.SimpleSettings/Resources.cs + - src/Core/ExistForAll.SimpleSettings/SettingsPlan.cs + - src/Core/ExistForAll.SimpleSettings/ValuesPopulator.cs + +key-decisions: + - "Validator's declared T is read from its implemented ISettingValidation<> interface to build ValidationContext — uniform for object-level (T=settings interface) and property-level (T=property type)" + - "RunValidators routes through SettingsValidationException.ThrowIfAny only when the lazy error list is non-null, keeping the null (no-error) path allocation-free while still using the shared throw helper for the error case" + - "ValidationContext gained a required (object settings) ctor; the previously implicit parameterless ctor is removed — safe because the interfaces were dead scaffolding with no callers" + +patterns-established: + - "Plan-build attribute threading for validators (object-level + per-property), never on the hot populate path" + - "Zero-allocation warm-path guard via a precomputed bool flag on the cached plan" + - "Non-settings-indicated nested fixtures resolved via GetSettings() so failing validators never trip ScanAssemblies tests" + +requirements-completed: [VAL-01] + +coverage: + - id: D1 + description: "Object-level [SettingsValidator(typeof(X))] runs against the fully-populated instance; a failing validator throws SettingsValidationException, a passing one does not" + requirement: "VAL-01" + verification: + - kind: unit + ref: "src/Tests/ExistForAll.SimpleSettings.UnitTests/SimpleSettings/SettingsValidationTests.cs#ObjectValidator_WhenItAddsError_ThrowsSettingsValidationException" + status: pass + - kind: unit + ref: "src/Tests/ExistForAll.SimpleSettings.UnitTests/SimpleSettings/SettingsValidationTests.cs#ObjectValidator_WhenValid_DoesNotThrow" + status: pass + human_judgment: false + - id: D2 + description: "Property-level [SettingsProperty(ValidatorType=typeof(X))] runs against that property's value via ValidationContext" + requirement: "VAL-01" + verification: + - kind: unit + ref: "src/Tests/ExistForAll.SimpleSettings.UnitTests/SimpleSettings/SettingsValidationTests.cs#PropertyValidator_RunsAgainstThatPropertyValue" + status: pass + human_judgment: false + - id: D3 + description: "Multiple failures aggregate into ONE SettingsValidationException carrying the full ValidationError list (Errors.Count == 2)" + requirement: "VAL-01" + verification: + - kind: unit + ref: "src/Tests/ExistForAll.SimpleSettings.UnitTests/SimpleSettings/SettingsValidationTests.cs#MultipleErrors_AggregateIntoOneExceptionWithAllErrors" + status: pass + human_judgment: false + - id: D4 + description: "Cross-property validator observes every property set (runs after the property-set loop) — valid pair does not throw, violated pair throws" + requirement: "VAL-01" + verification: + - kind: unit + ref: "src/Tests/ExistForAll.SimpleSettings.UnitTests/SimpleSettings/SettingsValidationTests.cs#CrossPropertyValidator_ObservesEveryPropertySet" + status: pass + - kind: unit + ref: "src/Tests/ExistForAll.SimpleSettings.UnitTests/SimpleSettings/SettingsValidationTests.cs#CrossPropertyValidator_WhenRuleViolated_Throws" + status: pass + human_judgment: false + - id: D5 + description: "SettingsValidationException.ToString() carries only author-supplied ValidationError text and no injected bound value (D-12); catchable as SimpleSettingsException (ExceptionHierarchytests)" + requirement: "VAL-01" + verification: + - kind: unit + ref: "src/Tests/ExistForAll.SimpleSettings.UnitTests/SimpleSettings/SettingsValidationTests.cs#Exception_CarriesAuthorMessage_AndNoBoundValue" + status: pass + - kind: unit + ref: "src/Tests/ExistForAll.SimpleSettings.UnitTests/SimpleSettings/ExceptionHierarchyTests.cs#AllLibraryExceptions_DeriveFromSimpleSettingsException" + status: pass + human_judgment: false + - id: D6 + description: "Validator-free type short-circuits (HasValidators == false) and populates without throwing or entering the gather path; zero-allocation guarantee enforced by the benchmark allocation gate in CI" + requirement: "VAL-01" + verification: + - kind: unit + ref: "src/Tests/ExistForAll.SimpleSettings.UnitTests/SimpleSettings/SettingsValidationTests.cs#NoValidators_PopulatesAndReturnsWithoutThrowing" + status: pass + human_judgment: false + +# Metrics +duration: 5min +completed: 2026-07-15 +status: complete +--- + +# Phase 04 Plan 03: Core-Path Settings Validation Summary + +**Declared object- and property-level validators now run in the core populate pipeline (VAL-01 core path): sync contracts, constructed contexts, a `[SettingsValidator]` attribute, and a value-free aggregate `SettingsValidationException` thrown through a shared `ThrowIfAny` helper — with a `HasValidators` zero-allocation short-circuit protecting the benchmark gate.** + +## Performance + +- **Duration:** ~5 min +- **Started:** 2026-07-15T13:44:44Z +- **Completed:** 2026-07-15T13:49:58Z +- **Tasks:** 2 (Task 2 TDD: RED -> GREEN) +- **Files modified:** 10 (3 created, 7 modified) + +## Accomplishments +- The dead `Validations/*` scaffolding is now live: `ISettingsValidator`/`ISettingValidation.Validate` return `ValidationResult` (Task<> dropped, D-07), and `ValidationContext`/`ValidationContext` carry the settings instance via constructors (D-08). +- `[SettingsValidator(typeof(X))]` (object-level, `AttributeTargets.Interface`) and `[SettingsProperty(ValidatorType=typeof(X))]` (property-level) are both read once at plan build and invoked after the property-set loop, so cross-property rules see the fully-populated instance (D-09/D-11). +- Every produced `ValidationError` aggregates into one `SettingsValidationException : SimpleSettingsException` (D-10) whose message is composed solely from author-supplied `ValidationError` text — no bound value is embedded (D-12), verified against a sentinel-secret binding. +- `SettingsValidationException.ThrowIfAny(errors)` is the single aggregate-and-throw entry point the core path calls and Plan 04's DI runner will reuse, so the thrown contract cannot drift (review S-3). +- `SettingsPlan.HasValidators` is computed once at build; the post-populate hook starts with `if (!plan.HasValidators) return;` before any allocation, and the error list is allocated lazily on first error only — the validator-free warm path stays byte-identical to master (review B-2). +- Reflective dispatch selects the correct `Validate` overload via `GetMethod(name, new[]{ closedContextType })` and builds the context with `MakeGenericType`, avoiding the `AmbiguousMatchException` the two-overload interface would otherwise raise (review S-2). + +## Task Commits + +Each task was committed atomically: + +1. **Task 1: Validation contracts (sync interfaces, context ctors, attribute, aggregate exception + ThrowIfAny)** - `f039dc7` (feat) +2. **Task 2: Core-path validation execution in ValuesPopulator + plan-build threading + tests** - `ee7f481` (test / RED) -> `a6b290e` (feat / GREEN) + +**Plan metadata:** _(final docs commit — see git log)_ + +## Files Created/Modified +- `SettingsValidatorAttribute.cs` (created) — object-level attribute, `Type ValidatorType`, `AttributeTargets.Interface`. +- `SettingsValidationException.cs` (created) — aggregate exception `: SimpleSettingsException` exposing `Errors`; static `ThrowIfAny` shared helper. +- `Validations/ISettingsValidator.cs` / `ISettingValidation.cs` — `Validate` returns `ValidationResult` (Task<> removed). +- `Validations/ValidationContext.cs` / `ValidationContextOfT.cs` — added `(object settings)` / `(T settings)` constructors. +- `Resources.cs` — `SettingsValidationExceptionMessage(IReadOnlyList)` composed only from author text. +- `SettingsPlan.cs` — `SettingsPlan.ObjectValidatorType` + `HasValidators` (computed at build); `PropertyPlan.ValidatorType`. +- `ValuesPopulator.cs` — plan-build attribute threading, `RunValidators` post-populate hook (HasValidators guard, lazy list, ThrowIfAny), reflective `InvokeValidator`/`ResolveContextType`. +- `Tests/.../SimpleSettings/SettingsValidationTests.cs` (created) — 8 tests + non-indicated nested fixtures/validators. + +## Decisions Made +- Derive each validator's `T` from its implemented `ISettingValidation<>` interface to build `ValidationContext`, uniform across object-level (T = settings interface) and property-level (T = property type) validators. This keeps `InvokeValidator` a single reflective path. +- `RunValidators` calls `ThrowIfAny` only when the lazily-allocated error list is non-null; the shared helper still owns the throw for the error case, while the no-error path never allocates. +- Removing the implicit parameterless `ValidationContext` ctor (superseded by the required `(object settings)` ctor) is safe — the interfaces had no callers (confirmed by grep). + +## Deviations from Plan + +None - plan executed exactly as written. + +## Issues Encountered +- Solution-wide `dotnet build -c Release -f net8.0` reports NETSDK1005 on the net10-only Benchmark project (pre-existing, out of scope per plan). The authoritative `dotnet build SimpleSettings.slnx -c Release` (no `-f`) succeeds for net8.0 AND net10.0 with 0 warnings / 0 errors. + +## Verification +- `cd src && dotnet build SimpleSettings.slnx -c Release` -> 0 warnings, 0 errors (net8.0 + net10.0 per-project TFMs). +- `SettingsValidationTests` (net10) -> 8/8 pass. +- `ExceptionHierarchyTests` (net10) -> 6/6 pass (new exception joins the catchable family). +- Full suite (net10) -> 133/133 pass — no auto-discovered fixture regressed the ScanAssemblies-based tests. +- B-2 allocation gate: enforced by design (single-field `HasValidators` guard before any allocation on the validator-free warm path; error list allocated lazily). The BenchmarkDotNet allocation gate runs in CI and is expected byte-identical to master for the validator-free PlanPopulate/Resolve/Shape/ConvertArray/ConfigBinder benchmarks — not run locally (long-running). +- D-12: `Exception_CarriesAuthorMessage_AndNoBoundValue` asserts `ToString()` contains the author message and excludes the sentinel secret bound to the property. + +## User Setup Required +None - no external service configuration required. + +## Next Phase Readiness +- Plan 04 (DI-resolved validator path + API-02) reuses `SettingsValidationException.ThrowIfAny` and the same aggregate + redaction contract established here — the shared helper guarantees the thrown contract stays byte-identical across the two paths. +- Sync validator contracts and constructed contexts are in place for the DI runner. +- No blockers. + +## Self-Check: PASSED + +- FOUND: src/Core/ExistForAll.SimpleSettings/SettingsValidatorAttribute.cs +- FOUND: src/Core/ExistForAll.SimpleSettings/SettingsValidationException.cs +- FOUND: src/Tests/ExistForAll.SimpleSettings.UnitTests/SimpleSettings/SettingsValidationTests.cs +- FOUND: .planning/phases/04-collection-validation-binding/04-03-SUMMARY.md +- FOUND commits: f039dc7, ee7f481, a6b290e + +--- +*Phase: 04-collection-validation-binding* +*Completed: 2026-07-15* diff --git a/.planning/phases/04-collection-validation-binding/04-04-PLAN.md b/.planning/phases/04-collection-validation-binding/04-04-PLAN.md new file mode 100644 index 0000000..911dd84 --- /dev/null +++ b/.planning/phases/04-collection-validation-binding/04-04-PLAN.md @@ -0,0 +1,249 @@ +--- +phase: 04-collection-validation-binding +plan: 4 +type: execute +wave: 3 +depends_on: [04-03] +files_modified: + - src/Core/ExistForAll.SimpleSettings.Extensions.GenericHost/ServicesSettingsBuilderExtensions.cs + - src/Core/ExistForAll.SimpleSettings.Extensions.GenericHost/ISettingsValidationRunner.cs + - src/Core/ExistForAll.SimpleSettings.Extensions.GenericHost/SettingsValidationRunner.cs + - src/Core/ExistForAll.SimpleSettings.Extensions.GenericHost/ServiceProviderValidationExtensions.cs + - src/Tests/ExistForAll.SimpleSettings.UnitTests/DependencyInjection/AddSimpleSettingsIntegrationTests.cs +autonomous: true +requirements: [VAL-01, API-02] + +must_haves: + truths: + - "After AddSimpleSettings, ISettingsCollection is resolvable via GetRequiredService() (D-15 mechanism 1)." + - "An AddSimpleSettings overload surfaces the built ISettingsCollection (out parameter) while the IServiceCollection fluent chain is preserved (D-15 mechanism 2)." + - "A DI-registered ISettingValidation is invoked against the scanned settings instance in a deferred post-provider-build step and, on failure, throws the SAME SettingsValidationException contract as the core path (D-11 DI path / Q3)." + - "DI-resolved validators do NOT run inline during AddSimpleSettings (the container is not built at registration time — Pitfall 4)." + - "The DI runner resolves validators from a FRESH scope (provider.CreateScope()), so a validator with a SCOPED injected dependency resolves without the 'Cannot resolve scoped service from root provider' failure under scope validation (Development default) (review S-1)." + artifacts: + - "src/Core/ExistForAll.SimpleSettings.Extensions.GenericHost/ISettingsValidationRunner.cs + SettingsValidationRunner.cs" + - "src/Core/ExistForAll.SimpleSettings.Extensions.GenericHost/ServiceProviderValidationExtensions.cs (ValidateSimpleSettings)" + - "AddSingleton(settingsCollection) + AddSimpleSettings(out ISettingsCollection) overload" + key_links: + - "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)." + - "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)." +--- + + +Expose `ISettingsCollection` from the DI extension (API-02/D-15) and complete VAL-01's DI-resolved validator +path (D-11 DI path). Because `IntegrateSimpleSettings` builds settings at registration — before the +container exists (Pitfall 4 / Open Question Q3) — DI-resolved `ISettingValidation` cannot run inline; run +them in a deferred post-provider-build step that throws the SAME aggregated `SettingsValidationException` as +the core path. Depends on Plan 03 for the sync interfaces, contexts, and the aggregate exception. + +Purpose: The client pre-beta batch requires `AddSimpleSettings` to hand back the settings collection and to +support validators that need injected services. +Output: `AddSingleton` + an `out ISettingsCollection` overload, an +`ISettingsValidationRunner` + impl, and a public `IServiceProvider.ValidateSimpleSettings()` extension, plus +DI integration tests. + +Skills: invoke `dotnet-claude-kit:modern-csharp` (C# 12-safe) and `dotnet-claude-kit:testing` (DI integration +tests with ServiceCollection + BuildServiceProvider). + +ARCHITECTURE DECISION (gated): the deferred-post-build mechanism (a resolvable runner invoked via +`ValidateSimpleSettings()` rather than an IHostedService, because the GenericHost package references only +`Microsoft.Extensions.DependencyInjection.Abstractions` and adding a hosting dependency is out of scope this +phase) is the RESEARCH-recommended Q3 fallback. dotnet-architect + security-auditor MUST confirm this shape +and that the DI-path failure is byte-identical to the core-path SettingsValidationException. + +Notes (review N-2, architect A1/A5 — surface, do not over-engineer): +- DI-path fail-fast is OPT-IN: it only fires when the consumer explicitly calls `provider.ValidateSimpleSettings()` + after `BuildServiceProvider()` (unlike the core path, which runs inline during populate). This is a real + discoverability caveat — it MUST be documented in the XML-doc on `ValidateSimpleSettings` and in the README. + README/API docs land in Phase 5 (DOC-01); do NOT drop this — carry it forward as a DOC-01 input. +- Cross-path aggregation is PER-PATH: a settings type carrying BOTH an attribute-declared validator (core path, + Plan 03) and a DI-registered validator will fail at TWO lifecycle moments (populate, then ValidateSimpleSettings) + rather than in one combined exception. This is acceptable for the phase. OPTIONALLY, the runner MAY de-duplicate + a validator that is both attribute-referenced and DI-registered (skip re-running the same concrete type) — a + nice-to-have, not required; do not add complexity beyond a type-identity skip if adopted. + + + +@/Users/guyludvig/frontegg/development/open-source/SimpleSettings/.claude/gsd-core/workflows/execute-plan.md +@/Users/guyludvig/frontegg/development/open-source/SimpleSettings/.claude/gsd-core/templates/summary.md + + + +@.planning/PROJECT.md +@.planning/ROADMAP.md +@.planning/STATE.md +@.planning/phases/04-collection-validation-binding/04-CONTEXT.md +@.planning/phases/04-collection-validation-binding/04-RESEARCH.md +@.planning/phases/04-collection-validation-binding/04-PATTERNS.md +@.planning/phases/04-collection-validation-binding/04-VALIDATION.md +@.planning/codebase/CONVENTIONS.md + +@src/Core/ExistForAll.SimpleSettings.Extensions.GenericHost/ServicesSettingsBuilderExtensions.cs +@src/Core/ExistForAll.SimpleSettings.Extensions.GenericHost/SettingsProvider.cs +@src/Core/ExistForAll.SimpleSettings/ISettingsCollection.cs +@src/Core/ExistForAll.SimpleSettings/SettingsCollection.cs +@src/Core/ExistForAll.SimpleSettings/SettingsCollectionExtensions.cs +@src/Core/ExistForAll.SimpleSettings/SettingsBuilder.cs +@src/Tests/ExistForAll.SimpleSettings.UnitTests/DependencyInjection/AddSimpleSettingsIntegrationTests.cs + + + +New symbols introduced by THIS plan (exclude from drift/orphan checks): +- `AddSimpleSettings(this IServiceCollection, out ISettingsCollection, Action? = null)` overload +- `services.AddSingleton(settingsCollection)` registration inside IntegrateSimpleSettings +- `IntegrateSimpleSettings` return type changed void -> ISettingsCollection +- `ExistForAll.SimpleSettings.Extensions.GenericHost.ISettingsValidationRunner` (public) + `SettingsValidationRunner` (internal impl — resolves validators from a fresh scope per review S-1, aggregates via `SettingsValidationException.ThrowIfAny` per review S-3) +- `ServiceProviderValidationExtensions.ValidateSimpleSettings(this IServiceProvider)` (public extension — deferred, OPT-IN DI-validation entry point the consumer must call after BuildServiceProvider) +- New DI integration test methods + fixtures/validators +- Consumes (does not create): `SettingsValidationException`, `ISettingValidation`, `ValidationContext` from Plan 03; `ISettingsCollection` (already public) + + + + + + Task 1: Expose ISettingsCollection from AddSimpleSettings (API-02/D-15) + + - src/Core/ExistForAll.SimpleSettings.Extensions.GenericHost/ServicesSettingsBuilderExtensions.cs (:7-44; IntegrateSimpleSettings is void and builds the collection at :32; two existing IServiceCollection-returning overloads to keep) + - src/Core/ExistForAll.SimpleSettings/ISettingsCollection.cs (already public) + SettingsCollection.cs (internal — DI uses the interface, no visibility change) + - src/Tests/ExistForAll.SimpleSettings.UnitTests/DependencyInjection/AddSimpleSettingsIntegrationTests.cs (ServiceCollection -> AddSimpleSettings(o => ...) -> BuildServiceProvider().GetRequiredService() convention; InMemoryBinder fake) + - 04-PATTERNS.md § "Extensions.GenericHost/ServicesSettingsBuilderExtensions.cs" (API-02) + - 04-RESEARCH.md Pattern 5 + Open Question Q4 (cannot overload by return type; use out param, keep chaining) + + src/Core/ExistForAll.SimpleSettings.Extensions.GenericHost/ServicesSettingsBuilderExtensions.cs, src/Tests/ExistForAll.SimpleSettings.UnitTests/DependencyInjection/AddSimpleSettingsIntegrationTests.cs + + - Test: after AddSimpleSettings, provider.GetRequiredService() returns a collection whose GetSettings() yields the same instance as GetRequiredService(). + - Test: the new AddSimpleSettings(out ISettingsCollection settings, ...) overload sets `settings` to the built collection and still returns IServiceCollection so the fluent chain is preserved. + - Test (regression): the existing AddSimpleSettings overloads and ISettingsProvider behavior are unchanged (existing DI tests stay green). + + + Per D-15, refactor `IntegrateSimpleSettings` to RETURN the built `ISettingsCollection` (currently void) + and, inside it, register `services.AddSingleton(settingsCollection)` (instance + registration — the collection is fully built at :32; `ISettingsCollection` is public, `SettingsCollection` + stays internal). Keep the two existing `IServiceCollection`-returning overloads intact for the fluent + chain. Add a new overload `AddSimpleSettings(this IServiceCollection services, out ISettingsCollection settings, Action? action = null)` + that assigns `settings` from `IntegrateSimpleSettings(...)` and returns `services` (you cannot overload by + return type alone — the `out` param is the distinct signature, Q4). Preserve the null-action guard on the + action-taking path. Extend `AddSimpleSettingsIntegrationTests.cs` with the three behavior tests. Follow + `modern-csharp` + `testing`. + + + cd src && dotnet test SimpleSettings.slnx -c Release -f net10.0 --treenode-filter "/*/*/AddSimpleSettingsIntegrationTests/*" + + + - `IntegrateSimpleSettings` returns `ISettingsCollection` and registers `AddSingleton(...)`. + - `GetRequiredService()` resolves and its GetSettings() equals GetRequiredService() (same instance). + - The `out ISettingsCollection` overload sets the collection and returns IServiceCollection (chaining preserved); existing overloads unchanged. + - All AddSimpleSettingsIntegrationTests (including the four pre-existing tests) green on net10. + + ISettingsCollection is resolvable from DI and returnable via an out-overload; fluent chain preserved; DI integration tests green on net10. + + + + Task 2: Deferred DI-resolved validator runner (VAL-01 DI path / D-11 / Q3 — architecture-gated) + + - src/Core/ExistForAll.SimpleSettings.Extensions.GenericHost/ServicesSettingsBuilderExtensions.cs (registration site; ScanAssemblies runs eagerly at :32 before the container exists) + - src/Core/ExistForAll.SimpleSettings.Extensions.GenericHost/SettingsProvider.cs (analog for an internal GenericHost service consuming ISettingsCollection) + - src/Core/ExistForAll.SimpleSettings/Validations/ISettingValidation.cs + ValidationContextOfT.cs + ValidationResult.cs + ValidationError.cs (public contracts from Plan 03) + - src/Core/ExistForAll.SimpleSettings/SettingsValidationException.cs (public aggregate + static `ThrowIfAny(IReadOnlyList)` from Plan 03 — CALL ThrowIfAny, do not re-implement the aggregate/throw; review S-3) + - src/Core/ExistForAll.SimpleSettings/ISettingsCollection.cs (IEnumerable> to iterate scanned type+instance) + - 04-RESEARCH.md Open Question Q3 (deferred-post-build recommendation) + Pitfall 4 (container-not-built) + Architectural Responsibility Map (VAL-01 DI-resolved row) + - 04-PATTERNS.md § "Extensions.GenericHost" (DI validators, defer to post-provider-build) + + src/Core/ExistForAll.SimpleSettings.Extensions.GenericHost/ISettingsValidationRunner.cs, src/Core/ExistForAll.SimpleSettings.Extensions.GenericHost/SettingsValidationRunner.cs, src/Core/ExistForAll.SimpleSettings.Extensions.GenericHost/ServiceProviderValidationExtensions.cs, src/Core/ExistForAll.SimpleSettings.Extensions.GenericHost/ServicesSettingsBuilderExtensions.cs, src/Tests/ExistForAll.SimpleSettings.UnitTests/DependencyInjection/AddSimpleSettingsIntegrationTests.cs + + - Test: a DI-registered ISettingValidation (constructor-injected with a service) that adds an error causes provider.ValidateSimpleSettings() to throw SettingsValidationException carrying that error. + - Test: a passing DI-registered validator makes ValidateSimpleSettings() complete without throwing. + - Test: DI-resolved validators do NOT run during AddSimpleSettings itself (registering a failing DI validator does not throw until ValidateSimpleSettings() is called). + - Test (S-1, scoped dependency): a DI-registered ISettingValidation whose constructor injects a SCOPED service resolves and runs successfully under a provider built with scope validation enabled (BuildServiceProvider(validateScopes: true)) — proving the runner resolves from a fresh scope, not the root provider. + - Test: the thrown exception type + Errors shape match the core-path SettingsValidationException (same contract). + + + Per D-11 DI path / Q3, add `ISettingsValidationRunner` (public) with a method that runs DI-resolved + validation and an internal `SettingsValidationRunner` impl. Register the runner during + `IntegrateSimpleSettings` (it needs the built `ISettingsCollection` + the `IServiceProvider`). Add a public + `ServiceProviderValidationExtensions.ValidateSimpleSettings(this IServiceProvider provider)` that resolves + the runner and executes it — this is the DEFERRED post-provider-build step the consumer calls after + `BuildServiceProvider()` (DI validators cannot run at registration — the container is not built yet, + Pitfall 4). + + FRESH SCOPE (review S-1, architect A2): the runner MUST resolve validators from a fresh scope, not the root + provider — `using var scope = provider.CreateScope();` and resolve every `ISettingValidation` from + `scope.ServiceProvider`. Resolving `GetServices(...)` off the root provider throws "Cannot resolve scoped + service from root provider" whenever a validator (or one of its injected dependencies) is scoped, and scope + validation is on by default in Development. Iterate the `ISettingsCollection` (each `KeyValuePair`) + 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)`. + + 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 + calls — so the DI-path aggregated exception is contract-identical (type + Errors + value-free message) by + construction, not by duplication. Use only the public types from Plan 03, so no Core InternalsVisibleTo is + needed; add no bound values to the exception. Extend `AddSimpleSettingsIntegrationTests.cs` with the five + behavior tests; register the validator and its injected dependency (including the scoped-dependency case) on + the same ServiceCollection; use a settings fixture scoped so it does not disturb sibling assembly-scan tests. + This mechanism is the architecture-gated decision — record dotnet-architect + security-auditor sign-off that + the deferred shape, the fresh-scope resolution, and the shared aggregate/redaction contract are correct. + Follow `modern-csharp` + `testing`. + + + cd src && dotnet test SimpleSettings.slnx -c Release -f net10.0 --treenode-filter "/*/*/AddSimpleSettingsIntegrationTests/*" + + + - `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-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. + + DI-resolved ISettingValidation validators run in a deferred post-provider-build step, resolved from a fresh scope (scoped-dependency validators work), and throw the same SettingsValidationException as the core path via the shared ThrowIfAny helper; not run inline at registration; DI tests green on net10 (incl. the scope-validation case); architecture sign-off recorded. + + + + + +## Trust Boundaries + +| Boundary | Description | +|----------|-------------| +| DI container -> validator resolution | Service-dependent validators are resolved from the container and run against populated settings instances | +| validator message -> host logs | DI-path SettingsValidationException.ToString() reaches host logs | + +## STRIDE Threat Register + +| Threat ID | Category | Component | Severity | Disposition | Mitigation Plan | +|-----------|----------|-----------|----------|-------------|-----------------| +| T-04-VAL | Information Disclosure | DI-path SettingsValidationException | high | mitigate | The DI runner reproduces the core-path contract: message from author ValidationError text only, no bound value (D-12). Test asserts ToString() carries only author messages. security-auditor confirms parity with the core path. | +| T-04-06 | Elevation of Privilege | deferred validator timing | medium | mitigate | DI validators run only after BuildServiceProvider via an explicit ValidateSimpleSettings() call; they never run against a half-built graph at registration (Pitfall 4). Architecture-gated by dotnet-architect. | +| T-04-07 | Spoofing | reflective validator resolution | low | accept | Validator types resolve from the DI container the consumer configured; no untrusted type input. | +| T-04-SC | Supply Chain | package installs | n/a | accept | No package installs this phase; GenericHost keeps its single Microsoft.Extensions.DependencyInjection.Abstractions reference (no hosting dependency added). | + + + +- `cd src && dotnet build SimpleSettings.slnx -c Release` succeeds for net8.0 AND net10.0. +- AddSimpleSettingsIntegrationTests green on net10 (net8 in CI); the four pre-existing DI tests remain green. +- The scoped-dependency validator test passes under `BuildServiceProvider(validateScopes: true)` — proving fresh-scope resolution (review S-1). +- No new PackageReference/PackageVersion added to GenericHost (dependency-free — RESEARCH Standard Stack). +- DI-path and core-path SettingsValidationException are contract-identical BY CONSTRUCTION — both call `SettingsValidationException.ThrowIfAny` (type + Errors + no bound value; review S-3). + + + +ROADMAP success criteria 3 (VAL-01 DI-resolved path) and 5 (API-02: AddSimpleSettings exposes +ISettingsCollection via resolvable service AND return-value overload) are met, proven by green DI integration +tests on net10 with architecture + security sign-off on the deferred DI-validation mechanism. + + + +Create `.planning/phases/04-collection-validation-binding/04-04-SUMMARY.md` when done. + diff --git a/.planning/phases/04-collection-validation-binding/04-05-PLAN.md b/.planning/phases/04-collection-validation-binding/04-05-PLAN.md new file mode 100644 index 0000000..34aca92 --- /dev/null +++ b/.planning/phases/04-collection-validation-binding/04-05-PLAN.md @@ -0,0 +1,157 @@ +--- +phase: 04-collection-validation-binding +plan: 5 +type: execute +wave: 2 +depends_on: [04-01] +files_modified: + - src/Core/ExistForAll.SimpleSettings/Conversion/PropertyConversion.cs + - src/Tests/ExistForAll.SimpleSettings.UnitTests/SimpleSettings/SettingsPropertyTests.cs +autonomous: true +requirements: [VAL-02] + +must_haves: + truths: + - "[SettingsProperty(AllowEmpty=false)] rejects an empty string and a whitespace-only string at bind, not just null (VAL-02/D-13)." + - "The rejection surfaces a value-free exception in the SimpleSettingsException family (D-14)." + - "[SettingsProperty(AllowEmpty=true)] (the default) still accepts empty and whitespace strings (no regression)." + artifacts: + - "src/Core/ExistForAll.SimpleSettings/Conversion/PropertyConversion.cs (extended Convert null/empty branch)" + key_links: + - "VAL-02 reuses SettingsPropertyNullException (value-free) which is ALREADY excluded from the ValuesPopulator.cs:122 redaction filter — so NO exception-filter change is needed and the empty diagnostic is never redacted." + - "The empty/whitespace check only applies when _throwOnNull (AllowEmpty=false); a whitespace value with AllowEmpty=true falls through to normal conversion." +--- + + +Tighten `[SettingsProperty(AllowEmpty=false)]` so it rejects empty and whitespace strings at bind, not just +null (VAL-02/D-13), inline at the conversion layer (D-14) in `PropertyConversion.Convert`. Depends on Plan +01, which owns the collection null-result shape that `PropertyConversion` consumes in the same null/empty +branch — landing VAL-02 after COLL-02 keeps the empty-collection default and empty-string rejection semantics +reconciled and tested together. + +Purpose: The client pre-beta batch requires empty/whitespace values to be rejected (a common misconfig), +value-free, without a hot-path scan. `${ENV:-}` placeholder detection is explicitly DEFERRED (D-13 — out of +scope this phase). +Output: An extended `PropertyConversion.Convert` guard and new AllowEmpty tests. + +Decision (planner, per D-14): REUSE `SettingsPropertyNullException` (value-free) rather than introduce a new +empty-specific type. It is already excluded from the `ValuesPopulator.ConvertPropertyValue` redaction filter +(`ValuesPopulator.cs:122`), so NO filter change is required and the "empty not allowed" diagnostic is never +redacted into a generic conversion error (Pitfall 3 avoided by construction). + +Skills: invoke `dotnet-claude-kit:modern-csharp` (C# 12-safe pattern matching for the guard) and +`dotnet-claude-kit:testing` for the TUnit tests. + + + +@/Users/guyludvig/frontegg/development/open-source/SimpleSettings/.claude/gsd-core/workflows/execute-plan.md +@/Users/guyludvig/frontegg/development/open-source/SimpleSettings/.claude/gsd-core/templates/summary.md + + + +@.planning/PROJECT.md +@.planning/ROADMAP.md +@.planning/STATE.md +@.planning/phases/04-collection-validation-binding/04-CONTEXT.md +@.planning/phases/04-collection-validation-binding/04-RESEARCH.md +@.planning/phases/04-collection-validation-binding/04-PATTERNS.md +@.planning/phases/04-collection-validation-binding/04-VALIDATION.md +@.planning/codebase/CONVENTIONS.md + +@src/Core/ExistForAll.SimpleSettings/Conversion/PropertyConversion.cs +@src/Core/ExistForAll.SimpleSettings/Core/Reflection/TypeConverter.cs +@src/Core/ExistForAll.SimpleSettings/SettingsPropertyNullException.cs +@src/Core/ExistForAll.SimpleSettings/ValuesPopulator.cs +@src/Core/ExistForAll.SimpleSettings/SettingsPropertyAttribute.cs +@src/Tests/ExistForAll.SimpleSettings.UnitTests/SimpleSettings/SettingsPropertyTests.cs + + + +New symbols introduced by THIS plan (exclude from drift/orphan checks): +- Extended `PropertyConversion.Convert` null/empty guard (no new public symbol; signature unchanged) +- New test methods + fixture interfaces in `SettingsPropertyTests` (AllowEmpty=false rejects ""/whitespace; AllowEmpty=true accepts them) +- Consumes (does not create): `SettingsPropertyNullException` (existing), `_throwOnNull` resolved from `AllowEmpty` at TypeConverter.cs:16 + + + + + + Task 1: Reject empty/whitespace strings for AllowEmpty=false (VAL-02/D-13, D-14) + + - src/Core/ExistForAll.SimpleSettings/Conversion/PropertyConversion.cs (:29-40 Convert — the exact null-branch to extend; _throwOnNull is the AllowEmpty=false flag; _nullResult is COLL-02's empty default). NOTE: Plan 01 (dependency, B-3) already extended this same `value == null` branch so that a list `_nullResult` held as a `Func` factory returns a fresh list per bind — extend that branch for empty/whitespace strings WITHOUT disturbing the factory dispatch (both live in the same guard). + - src/Core/ExistForAll.SimpleSettings/Core/Reflection/TypeConverter.cs (:16 `throwOnNull = attribute is { AllowEmpty: false }` — where the flag is resolved at plan build) + - src/Core/ExistForAll.SimpleSettings/SettingsPropertyNullException.cs (value-free message: property name only — the type to reuse) + - src/Core/ExistForAll.SimpleSettings/ValuesPopulator.cs (:114-128 ConvertPropertyValue — the redaction filter at :122 already excludes SettingsPropertyNullException; do NOT edit it) + - src/Tests/ExistForAll.SimpleSettings.UnitTests/SimpleSettings/SettingsPropertyTests.cs (the existing AllowEmpty=false test + conventions to extend) + - 04-PATTERNS.md § "Conversion/PropertyConversion.cs — Convert" + § "TUnit test conventions" + - 04-RESEARCH.md Pattern 4 + Pitfall 3 (reuse SettingsPropertyNullException to avoid the redaction-filter widening) + + src/Core/ExistForAll.SimpleSettings/Conversion/PropertyConversion.cs, src/Tests/ExistForAll.SimpleSettings.UnitTests/SimpleSettings/SettingsPropertyTests.cs + + - Test: a string property with [SettingsProperty(AllowEmpty=false)] bound to "" throws SettingsPropertyNullException. + - Test: the same property bound to a whitespace-only value (" ") throws SettingsPropertyNullException. + - Test: the thrown exception's message carries the property name and no bound value (value-free). + - Test: a string property with the default AllowEmpty (true) bound to "" or " " does NOT throw and binds the value as-is. + + + Per D-13/D-14, extend the `Convert` guard in `PropertyConversion` so the existing null branch also handles + a null-or-whitespace string: when the value is null OR a `string` that is `string.IsNullOrWhiteSpace`, and + `_throwOnNull` is set, throw `new SettingsPropertyNullException(_propertyName)` (REUSE — value-free and + already excluded from the ValuesPopulator redaction filter, so no filter edit and no redaction of the + diagnostic). When `_throwOnNull` is NOT set: a null value returns `_nullResult` as today, and a + whitespace/empty string with AllowEmpty=true falls through to normal `_converter.Convert` (unchanged + behavior for the accepted case). Do not put the value into the exception. Keep the change inside + `PropertyConversion.Convert`; do NOT modify `ValuesPopulator.ConvertPropertyValue` or its exception filter. + Renaming `_throwOnNull` is optional cleanup and NOT required. Extend `SettingsPropertyTests.cs` with the + four behavior tests using nested fixture interfaces (a string property with AllowEmpty=false and one with + default AllowEmpty), bound via `InMemoryCollection`/`InMemoryBinder` + `SettingsBuilder`. Follow + `modern-csharp` (C# 12-safe pattern `value is string s && string.IsNullOrWhiteSpace(s)`) + `testing`; + tabs to match surrounding files. + + + cd src && dotnet test SimpleSettings.slnx -c Release -f net10.0 --treenode-filter "/*/*/SettingsPropertyTests/*" + + + - `PropertyConversion.Convert` rejects null-or-whitespace strings only when `_throwOnNull` is set, throwing `SettingsPropertyNullException`. + - VAL-02 does NOT modify or widen the `ValuesPopulator.ConvertPropertyValue` `:122` redaction filter or the binder catch (the filter already excludes `SettingsPropertyNullException`, so reusing that type needs no filter change). NOTE (review N-1): assert this about THIS plan's own diff — do NOT assert a byte-pristine `ValuesPopulator.cs`, because Plan 03 (VAL-01, same wave) legitimately edits `ValuesPopulator.cs` for the validation hook; a cumulative `git diff` will show that file changed by 03. Scope the check to VAL-02's change set (PropertyConversion.cs), e.g. confirm this plan's commit touches only PropertyConversion.cs + the test file. + - AllowEmpty=false bound to "" and to " " each throw SettingsPropertyNullException with a value-free message containing the property name. + - AllowEmpty=true bound to "" / " " does not throw and binds the value. + - SettingsPropertyTests green on net10, including the pre-existing AllowEmpty=false test. + + AllowEmpty=false rejects empty and whitespace (value-free SettingsPropertyNullException); AllowEmpty=true still accepts them; no redaction-filter change; SettingsPropertyTests green on net10. + + + + + +## Trust Boundaries + +| Boundary | Description | +|----------|-------------| +| config value -> conversion | An empty/whitespace config value crosses into PropertyConversion where AllowEmpty=false rejects it | +| rejection exception -> host logs | SettingsPropertyNullException.ToString() reaches host logs | + +## STRIDE Threat Register + +| Threat ID | Category | Component | Severity | Disposition | Mitigation Plan | +|-----------|----------|-----------|----------|-------------|-----------------| +| T-04-08 | Information Disclosure | AllowEmpty rejection exception | low | mitigate | Reuses value-free SettingsPropertyNullException (property name only); an empty/whitespace value is not itself sensitive, and the exception is value-free regardless (RESEARCH Assumption A5). | +| T-04-09 | Tampering | redaction filter bypass | medium | mitigate | Reusing SettingsPropertyNullException means it stays excluded from the ValuesPopulator:122 filter by construction — the empty diagnostic is never redacted and no new type slips through unlisted (Pitfall 3). | +| T-04-SC | Supply Chain | package installs | n/a | accept | No package installs this phase (04-RESEARCH.md Package Legitimacy Audit: N/A). | + + + +- `cd src && dotnet build SimpleSettings.slnx -c Release` succeeds for net8.0 AND net10.0. +- SettingsPropertyTests green on net10 (net8 in CI), including the pre-existing AllowEmpty=false test. +- VAL-02's own change set is confined to PropertyConversion.cs (+ its test); it does not touch the `ValuesPopulator.cs:122` redaction filter or the binder catch (review N-1 — Plan 03 edits ValuesPopulator.cs in the same wave, so do NOT assert the whole file is byte-pristine; assert only that VAL-02 leaves the filter/catch untouched). +- Full suite green on net10 after this wave. + + + +ROADMAP success criterion 4 (VAL-02: [SettingsProperty(AllowEmpty=false)] rejects empty/whitespace at bind, +not just null; `${ENV:-}` placeholder detection deferred per D-13) is met, proven by green tests on net10. + + + +Create `.planning/phases/04-collection-validation-binding/04-05-SUMMARY.md` when done. + diff --git a/.planning/phases/04-collection-validation-binding/04-05-SUMMARY.md b/.planning/phases/04-collection-validation-binding/04-05-SUMMARY.md new file mode 100644 index 0000000..73bbd9e --- /dev/null +++ b/.planning/phases/04-collection-validation-binding/04-05-SUMMARY.md @@ -0,0 +1,143 @@ +--- +phase: 04-collection-validation-binding +plan: 05 +subsystem: conversion +tags: [validation, allow-empty, string, dotnet, tunit, redaction] + +# Dependency graph +requires: + - phase: 04-collection-validation-binding + provides: "04-01 PropertyConversion.Convert null-branch factory dispatch (Func list null-result) that VAL-02 extends in place" +provides: + - "[SettingsProperty(AllowEmpty=false)] rejects null AND null-or-whitespace strings at bind (VAL-02/D-13), value-free via SettingsPropertyNullException (D-14)" + - "AllowEmpty=true (default) still binds empty and whitespace strings as-is (no regression)" +affects: [validation] + +# Tech tracking +tech-stack: + added: [] + patterns: + - "Single _throwOnNull guard covering both null and C#-12-safe null-or-whitespace string (value is string s && string.IsNullOrWhiteSpace(s)) ahead of the 04-01 factory dispatch, so the reject path short-circuits before the shared/factory null-result branch" + +key-files: + created: [] + modified: + - src/Core/ExistForAll.SimpleSettings/Conversion/PropertyConversion.cs + - src/Tests/ExistForAll.SimpleSettings.UnitTests/SimpleSettings/SettingsPropertyTests.cs + +key-decisions: + - "REUSE value-free SettingsPropertyNullException (property name only) rather than a new empty-specific type — it is already excluded from the ValuesPopulator.cs:122 redaction filter, so no filter change and the empty diagnostic is never redacted (Pitfall 3 avoided by construction)" + - "Rejection guard placed BEFORE the 04-01 Func factory dispatch and gated on _throwOnNull, leaving the AllowEmpty=true accept path (empty/whitespace -> _converter.Convert) and the null-result factory dispatch untouched" + +patterns-established: + - "AllowEmpty=false extends its reject predicate to null-or-whitespace strings inline at the conversion layer (D-14), no hot-path scan for the accepted case" + +requirements-completed: [VAL-02] + +coverage: + - id: D1 + description: "[SettingsProperty(AllowEmpty=false)] bound to an empty string throws value-free SettingsPropertyNullException" + requirement: "VAL-02" + verification: + - kind: unit + ref: "src/Tests/ExistForAll.SimpleSettings.UnitTests/SimpleSettings/SettingsPropertyTests.cs#Build_WhenAllowEmptyIsFalse_EmptyString_ShouldThrowNullException" + status: pass + human_judgment: false + - id: D2 + description: "[SettingsProperty(AllowEmpty=false)] bound to a whitespace-only string throws value-free SettingsPropertyNullException" + requirement: "VAL-02" + verification: + - kind: unit + ref: "src/Tests/ExistForAll.SimpleSettings.UnitTests/SimpleSettings/SettingsPropertyTests.cs#Build_WhenAllowEmptyIsFalse_WhitespaceString_ShouldThrowNullException" + status: pass + human_judgment: false + - id: D3 + description: "The rejection exception message carries the property name and no bound value (value-free)" + requirement: "VAL-02" + verification: + - kind: unit + ref: "src/Tests/ExistForAll.SimpleSettings.UnitTests/SimpleSettings/SettingsPropertyTests.cs#Build_WhenAllowEmptyIsFalse_EmptyString_MessageIsValueFreeWithPropertyName" + status: pass + human_judgment: false + - id: D4 + description: "AllowEmpty=true (default) bound to empty or whitespace does not throw and binds the value as-is" + requirement: "VAL-02" + verification: + - kind: unit + ref: "src/Tests/ExistForAll.SimpleSettings.UnitTests/SimpleSettings/SettingsPropertyTests.cs#Build_WhenAllowEmptyIsTrue_EmptyString_BindsValue, #Build_WhenAllowEmptyIsTrue_WhitespaceString_BindsValue" + status: pass + human_judgment: false + +# Metrics +duration: 3min +completed: 2026-07-15 +status: complete +--- + +# Phase 04 Plan 05: AllowEmpty String Rejection Summary + +**[SettingsProperty(AllowEmpty=false)] now rejects empty and whitespace strings at bind (not just null) via the value-free SettingsPropertyNullException, reusing the existing type so the ValuesPopulator redaction filter needs no change.** + +## Performance + +- **Duration:** ~3 min +- **Started:** 2026-07-15T13:53:15Z +- **Completed:** 2026-07-15T13:56:13Z +- **Tasks:** 1 (TDD) +- **Files modified:** 2 + +## Accomplishments +- `PropertyConversion.Convert` rejects a null-or-whitespace string (in addition to null) when `_throwOnNull` (AllowEmpty=false), throwing the value-free `SettingsPropertyNullException` (property name only). +- The reject guard sits ahead of 04-01's `Func` list null-result factory dispatch and preserves it — the null path still returns a fresh-per-bind list, arrays/IEnumerable keep the shared empty instance. +- AllowEmpty=true (default) is unchanged: an empty or whitespace string falls through to the normal converter and binds as-is. +- No change to the `ValuesPopulator.cs:122` redaction filter or the binder catch — `SettingsPropertyNullException` is already excluded, so the empty diagnostic is never redacted (Pitfall 3). + +## Task Commits + +Each task was committed atomically (TDD RED -> GREEN): + +1. **Task 1: Reject empty/whitespace strings for AllowEmpty=false (VAL-02/D-13, D-14)** — `b675dd3` (test) -> `795dd93` (feat) + +**Plan metadata:** _(final docs commit — see git log)_ + +_No REFACTOR commit: the GREEN change was a single guard line; nothing to clean up._ + +## Files Created/Modified +- `Conversion/PropertyConversion.cs` — `Convert` gains a leading `_throwOnNull && (value == null || value is string s && string.IsNullOrWhiteSpace(s))` reject guard (C# 12-safe); the null-result factory dispatch and accept path are unchanged. +- `Tests/.../SimpleSettings/SettingsPropertyTests.cs` — 5 new tests + 2 fixture interfaces (`IRejectsEmpty` with AllowEmpty=false, `IAllowsEmpty` default) bound via `InMemoryCollection`/`InMemoryBinder`; the pre-existing int AllowEmpty=false test is retained. + +## Decisions Made +None beyond the plan — implemented exactly as specified. Reused `SettingsPropertyNullException` (value-free) per D-14; placed the guard ahead of the 04-01 factory dispatch and gated it on `_throwOnNull` so both the accept path and the list null-result factory stay intact. The optional `_throwOnNull` rename was not performed (explicitly optional in the plan). + +## Deviations from Plan + +None - plan executed exactly as written. + +## Issues Encountered +None. RED showed exactly the 3 AllowEmpty=false empty/whitespace tests failing while the two AllowEmpty=true accept-tests and the pre-existing int test passed, confirming the guard was the only missing behavior. + +## Scope Verification +- `git diff HEAD~2 HEAD --name-only` after the feat commit lists only `PropertyConversion.cs` + the test file — VAL-02's own change set does not touch `ValuesPopulator.cs` (the `:122` redaction filter and binder catch are untouched by this plan, consistent with review N-1: Plan 03 legitimately edited ValuesPopulator.cs earlier in the wave). + +## Verification +- `cd src && dotnet build SimpleSettings.slnx -c Release` -> Build succeeded, 0 Warnings, 0 Errors (net8.0 + net10.0 per-project TFMs). +- `SettingsPropertyTests` on net10 -> 6/6 pass (incl. the pre-existing AllowEmpty=false int test). +- Full suite on net10 -> 138/138 pass. + +## User Setup Required +None - no external service configuration required. + +## Next Phase Readiness +- VAL-02 is complete; ROADMAP success criterion 4 (empty/whitespace rejection at bind, `${ENV:-}` placeholder detection deferred per D-13) is met and proven green on net10. +- No blockers. + +## Self-Check: PASSED + +- FOUND: src/Core/ExistForAll.SimpleSettings/Conversion/PropertyConversion.cs +- FOUND: src/Tests/ExistForAll.SimpleSettings.UnitTests/SimpleSettings/SettingsPropertyTests.cs +- FOUND: .planning/phases/04-collection-validation-binding/04-05-SUMMARY.md +- FOUND commits: b675dd3 (test), 795dd93 (feat) + +--- +*Phase: 04-collection-validation-binding* +*Completed: 2026-07-15* diff --git a/.planning/phases/04-collection-validation-binding/04-CONTEXT.md b/.planning/phases/04-collection-validation-binding/04-CONTEXT.md new file mode 100644 index 0000000..9682b05 --- /dev/null +++ b/.planning/phases/04-collection-validation-binding/04-CONTEXT.md @@ -0,0 +1,227 @@ +# Phase 4: Collection & Validation Binding - Context + +**Gathered:** 2026-07-15 +**Status:** Ready for planning + + +## Phase Boundary + +Make collection binding correct across every shape (empty, comma-scalar, YAML/child-section +sequence), make declared settings-validation actually **run** in the bind pipeline, and expose the +settings collection from the DI extension — the client's pre-beta engine batch. Requirements +**COLL-02, COLL-03, VAL-01, VAL-02, API-02** are locked by ROADMAP; this discussion settled **HOW**. + +**Explicitly pulled into scope:** full `List`/`IList`/`ICollection` support (the long-deferred +**COLL-01**) — COLL-02/COLL-03's `List` behavior is incoherent without it. + +**Not in scope:** AOT-01 / DOC-01 (Phase 5); REL-01 (Phase 6); held D2 `EqualityCompererCreator`; +`${ENV:-}` placeholder detection (deferred — see Deferred Ideas); `IOptionsMonitor`-style reload. + + + +## Implementation Decisions + +### Collection scope — absorb COLL-01 (full `List` family) +- **D-01:** Land **full `List` family support** this phase. One converter materializes a `List` + and satisfies `List`, `IList`, `ICollection`, `IReadOnlyList`, `IReadOnlyCollection`; + arrays stay on `ArrayTypeConverter`. Broaden `TypeExtensions.IsEnumerable` + (`Core/Reflection/TypeExtensions.cs:21-26`, matches only the open `IEnumerable<>` today) and the + element-type detection so these shapes are recognized for both conversion and the null/empty default. + Reuse the existing `CollectionTypeConverter` array-build path (`Conversion/CollectionTypeConverter.cs`), + then wrap into a `List` when the target is a list shape. Rationale: only `ArrayTypeConverter` + (`IsArray`) and `EnumerableTypeConverter` (open `IEnumerable<>`) exist today, so a populated `List` + currently falls to `DefaultTypeConverter` and throws — the ROADMAP success criteria and client doc + both name `List`, so the empty/sequence fixes require real conversion support. + +### COLL-02 — empty-collection default +- **D-02:** Extend `TypeConverter.CreateNullResult` (`Core/Reflection/TypeConverter.cs:27-36`, the null + branch) so an unset `T[]`, `List` family, and `IEnumerable` all bind an **empty collection, + never `null`**. Today only `IEnumerable` does (via `IsEnumerable()`); arrays and lists fall to the + value-type/`null` branch. Materialize `Array.CreateInstance(elementType, 0)` for arrays and a new + empty `List` for the list shapes. +- **D-03:** **Sequencing — COLL-02 before COLL-03.** COLL-03's "empty / whitespace / empty-sequence → + empty" behavior leans on COLL-02's empty-not-null default already being in place. + +### COLL-03 — YAML / child-section sequence binding (redaction-critical) +- **D-04:** `ConfigurationBinder.BindPropertySettings` + (`Extensions.Binders/ConfigurationBinder.cs:26-36`) currently reads a single scalar `section[key]`. + For **collection-typed targets**, enumerate `GetChildren()` and set the sequence (as an array / + `string[]`) onto `context`. The existing `CollectionTypeConverter.AsArray` + (`Conversion/CollectionTypeConverter.cs:43-51`) already passes an incoming `Array` straight through, + so a binder that sets an array `NewValue` flows through the inner element-converter chain unchanged. +- **D-05:** Locked sub-rules (from client doc, non-negotiable): **children win** when both a scalar and + children exist; **comma-scalar MUST still bind** (prod env override `MultiHost__CommonHosts` depends + on it); empty / whitespace / empty-sequence → **empty, never `[""]`**; each element still flows the + inner converter chain (so `int[]` / enum arrays keep working); honor `[SettingsSection]` / root + prefixing exactly as the scalar path does today. +- **D-06 (SECURITY GATE, locked):** COLL-03 edits `BindPropertySettings`, which runs inside + `ValuesPopulator`'s wrapping catch (`ValuesPopulator.cs:50-60` → `SettingsBindingException`). The + **S1/SEC-01 secret-redaction invariant MUST be re-verified**: add a regression test proving a secret + value carried in a YAML sequence element is absent from the whole `ex.ToString()` chain on a + bind/convert failure. `security-auditor` owns this in the plan-review panel. + +### VAL-01 — settings validation wired into the pipeline +- **D-07:** **Redesign the held validator interfaces to synchronous.** Change + `ISettingsValidator.Validate` / `ISettingValidation.Validate` to return `ValidationResult` (drop + `Task<>`). The populate/build path is synchronous `void` and runs at startup; a sync validator hooks in + with no sync-over-async hazard. The interfaces are dead scaffolding (never invoked), so the signature + change is free pre-beta. +- **D-08:** **Complete the held scaffolding.** `ValidationContext` (`Validations/ValidationContext.cs`) + and `ValidationContext` are getter-only with no constructor — they can't currently carry a + `Settings` value. Add construction so the populated instance is passed in. +- **D-09:** **Timing — validation runs at build/populate (startup).** Hook after the property-set loop + in `ValuesPopulator.PopulateInstanceWithValues` (`ValuesPopulator.cs:29-65`), i.e. once the instance is + fully populated (cross-property rules need all properties set). +- **D-10:** **Failure — aggregate, throw once.** Run all applicable validators for the type, collect + every `ValidationError`, and throw ONE new **`SettingsValidationException : SimpleSettingsException`** + carrying the full error list. Fail-fast at startup; operator sees every problem at once; stays in the + one catchable exception family. +- **D-11:** **Discovery — BOTH mechanisms.** + - *Core path (always):* property-level `[SettingsProperty(ValidatorType=typeof(X))]` + (`SettingsPropertyAttribute.cs:14`, runs on that property's value) **and** an object-level + interface-attribute (e.g. `[SettingsValidator(typeof(X))]`) on the settings interface, instantiated + via `Activator` (parameterless ctor). Works for both `SettingsBuilder` and DI paths. + - *DI path (when a container is present, `AddSimpleSettings`):* also resolve registered + `ISettingValidation` from the container — supports validators needing injected services (closest + to replacing the gateway's interim FluentValidation boot-hook). +- **D-12:** **Redaction.** The thrown `SettingsValidationException` must add no bound values itself. + `ValidationError` messages are validator-author-supplied → the author owns not echoing secrets; + document this. Re-verify no value leak (ties to S1/SEC-01). + +### VAL-02 — tighter `AllowEmpty` +- **D-13:** `[SettingsProperty(AllowEmpty=false)]` rejects **null + `""` + whitespace** + (`string.IsNullOrWhiteSpace`). `${ENV:-}` placeholder detection is **deferred** (client marked it + lowest-urgency; pattern-matching `${...}` risks false positives, and the library only sees whatever the + config provider left — it does no substitution itself). +- **D-14:** **Inline at the conversion layer.** Extend `PropertyConversion.Convert`'s existing + `_throwOnNull` branch (`Conversion/PropertyConversion.cs:29-40`; `throwOnNull` is resolved at plan-build + in `TypeConverter.CreateConversion:16`) to also reject empty/whitespace strings. Keep it a binding-layer + concern (not a VAL-01 validator). Throw a **value-free** exception in the `SimpleSettingsException` + family; exact type is the planner's call (reuse `SettingsPropertyNullException` or a dedicated + empty-specific sibling). NOTE: the client doc's pointer `TypeConverter.ValidateNullAcceptance` is + **stale** — that method no longer exists; `PropertyConversion` is the real hook. + +### API-02 — expose `ISettingsCollection` +- **D-15:** **Implement both** exposure mechanisms, then decide the final shape at review on the branch + (reversal is free pre-beta): + 1. Register `ISettingsCollection` as a resolvable DI singleton + (`services.AddSingleton(settingsCollection)` in + `ServicesSettingsBuilderExtensions.IntegrateSimpleSettings`; the collection already exists at + `:32`). + 2. Add an overload of `AddSimpleSettings` that returns the `ISettingsCollection`. + Keep the existing `IServiceCollection`-returning overloads intact so the fluent chain still works. + `ISettingsCollection` is public; `SettingsCollection` impl stays internal. + +### Claude's Discretion +- Exact new type/method names (`SettingsValidationException`, `[SettingsValidator]`, the VAL-02 exception + type, the API-02 return overload signature), test file placement under + `src/Tests/ExistForAll.SimpleSettings.UnitTests/`, and the precise `List`-materialization mechanism + (wrap the built array vs. build a `List` directly). All breaking/behavior changes are expected and + batched pre-beta. + + + +## Canonical References + +**Downstream agents MUST read these before planning or implementing.** + +### Requirement / roadmap / client source +- `.planning/ROADMAP.md` — Phase 4 goal + 6 success criteria +- `.planning/REQUIREMENTS.md` — COLL-02 / COLL-03 / VAL-01 / VAL-02 / API-02 + traceability +- `.planning/backlog/client-requirements-pre-beta.md` — the client's original per-item asks and rationale + (⚠ its `TypeConverter.ValidateNullAcceptance` pointer is stale; trust current source) + +### COLL-01 scope + COLL-02 (empty default) +- `src/Core/ExistForAll.SimpleSettings/Core/Reflection/TypeConverter.cs:27-36` — `CreateNullResult` null branch +- `src/Core/ExistForAll.SimpleSettings/Core/Reflection/TypeExtensions.cs:21-26` — `IsEnumerable` (broaden here) +- `src/Core/ExistForAll.SimpleSettings/Conversion/CollectionTypeConverter.cs` — shared array-build path (`AsArray:43-51`) +- `src/Core/ExistForAll.SimpleSettings/Conversion/ArrayTypeConverter.cs` — `IsArray` target +- `src/Core/ExistForAll.SimpleSettings/Conversion/EnumerableTypeConverter.cs` — open `IEnumerable<>` target +- `src/Core/ExistForAll.SimpleSettings/Conversion/TypeConvertersCollections.cs` — the converter `LinkedList` + +### COLL-03 (sequence binding + redaction gate) +- `src/Core/ExistForAll.SimpleSettings.Extensions.Binders/ConfigurationBinder.cs:26-36` — `BindPropertySettings` (add `GetChildren()`) +- `src/Core/ExistForAll.SimpleSettings/ValuesPopulator.cs:50-60` — the `SettingsBindingException` wrapping catch (redaction context) +- Exception contract (S1/SEC-01): `SettingsBindingException`, `SettingsPropertyValueException`, `SettingsPropertyNullException`, `SimpleSettingsException` + +### VAL-01 / VAL-02 (validation + empty) +- `src/Core/ExistForAll.SimpleSettings/Validations/` — held scaffolding: `ISettingValidation.cs`, `ISettingsValidator.cs`, `ValidationContext.cs` (needs ctor), `ValidationContextOfT.cs`, `ValidationResult.cs`, `ValidationError.cs` +- `src/Core/ExistForAll.SimpleSettings/SettingsPropertyAttribute.cs:14` — `ValidatorType` (VAL-01), `AllowEmpty:12` (VAL-02) +- `src/Core/ExistForAll.SimpleSettings/ValuesPopulator.cs:29-65` — populate path (VAL-01 hook point, after the property loop) +- `src/Core/ExistForAll.SimpleSettings/Conversion/PropertyConversion.cs:29-40` — `_throwOnNull` branch (VAL-02 hook) +- `src/Core/ExistForAll.SimpleSettings.Extensions.GenericHost/ServicesSettingsBuilderExtensions.cs:21-44` — DI-path validator resolution site + +### API-02 +- `src/Core/ExistForAll.SimpleSettings.Extensions.GenericHost/ServicesSettingsBuilderExtensions.cs:7-44` — `AddSimpleSettings` / `IntegrateSimpleSettings` (collection built at `:32`) +- `src/Core/ExistForAll.SimpleSettings/ISettingsCollection.cs` — the public interface to expose +- `src/Core/ExistForAll.SimpleSettings/SettingsCollection.cs` — internal impl; `SettingsCollectionExtensions.cs` — `GetSettings` helper + +### Test conventions +- `.planning/codebase/TESTING.md` — TUnit conventions (⚠ `--filter` returns 0 tests / exit 5; use `--treenode-filter` or run unfiltered; build/test from `src/`) +- `src/Tests/ExistForAll.SimpleSettings.UnitTests/Conversion/CollectionConversionTests.cs`, `Core/TypeConverterTests.cs`, `Core/ValuesPopulatorTests.cs` — existing coverage to extend/dedupe against + + + +## Existing Code Insights + +### Reusable Assets +- **`CollectionTypeConverter` array-build path** — `Array.CreateInstance` + indexed element-convert; the + `List` shapes can reuse this and wrap the result. `AsArray` already passes an `Array` through, so a + COLL-03 binder that sets an array `NewValue` needs no converter change. +- **Per-property `PropertyConversion` struct + plan cache** — `AllowEmpty`/`throwOnNull` and the null-result + are resolved once at plan-build; VAL-02 extends the existing branch, no hot-path scan added. +- **Held `Validations/*` types** — `ValidationResult`/`ValidationError` are complete; the interfaces + + `ValidationContext` need the sync + ctor changes only. +- **`InternalsVisibleTo`** (`Info.cs`) — keeps internal types reachable from UnitTests/Benchmark. + +### Established Patterns +- One public type per file; `I`-prefixed interfaces; block-scoped namespaces; `net8.0;net10.0` only; + central package management. Modern C# (skill `modern-csharp`), TUnit tests (skill `testing`) — cite in the plan. +- Ordered converter chain (user first, `DefaultTypeConverter` last) and last-writer-wins binder precedence + are load-bearing — new collection/validation code must not reorder them. +- Breaking changes are free pre-stable and batched before `v2.0.0-beta`. + +### Integration Points +- `TypeConverter.CreateNullResult` / `TypeExtensions.IsEnumerable` — collection shape detection (COLL-01/02). +- `ConfigurationBinder.BindPropertySettings` — sequence reading (COLL-03). +- `ValuesPopulator.PopulateInstanceWithValues` — post-populate validation hook (VAL-01). +- `PropertyConversion.Convert` — empty rejection (VAL-02). +- `ServicesSettingsBuilderExtensions` — DI validator resolution + `ISettingsCollection` exposure (VAL-01/API-02). + +### Research flags (for gsd-phase-researcher / plan-review) +- **DI-path validator timing/ordering** — `AddSimpleSettings` builds instances at registration; DI-resolved + validators may depend on services not yet registered. Resolve the ordering and keep the aggregate+redaction + contract identical across the core path and the DI path. +- **`List` materialization** — confirm the cheapest path (wrap the existing built array vs. build the list). + + + +## Specific Ideas +- **`origin/validate-settings` is a pre-modernization fossil** (all old `SimpleConfig`→`SimpleSettings` + rename commits, ~188k deletions, dead `build.cake`/`appveyor.yml`). It carries an old + `Core/SettingsOptionsValidator` not on `master`, but nothing reconcilable. **VAL-01 is greenfield** — + ignore the REQUIREMENTS "reconcile with the validate-settings branch" note as effectively moot (glance at + the old `SettingsOptionsValidator` only for original design intent, do not port). +- **dotnet-kit layering:** plan-review panel up front (`dotnet-architect` + `performance-analyst` + + `security-auditor`, the latter owns the D-06 redaction gate); executor uses `modern-csharp` + `testing` + skills; `code-reviewer` on the finished diff. +- Ship on branch `gsd/phase-4-collection-validation-binding` → PR via `guy-lud`. Never commit to `master` + (every push publishes a throwaway alpha via `release.yml`). + + + +## Deferred Ideas +- **`${ENV:-}` unsubstituted-placeholder detection** (VAL-02) — deferred; revisit if a real case appears + (needs a precise pattern to avoid false positives; library does no env-substitution itself). +- **AOT-01 / DOC-01** — Phase 5. **REL-01** — Phase 6. +- **D2 `EqualityCompererCreator`** — HELD; out of scope. + +None of the above block Phase 4. + + +--- + +*Phase: 04-collection-validation-binding* +*Context gathered: 2026-07-15* + + diff --git a/.planning/phases/04-collection-validation-binding/04-DISCUSSION-LOG.md b/.planning/phases/04-collection-validation-binding/04-DISCUSSION-LOG.md new file mode 100644 index 0000000..5ac9269 --- /dev/null +++ b/.planning/phases/04-collection-validation-binding/04-DISCUSSION-LOG.md @@ -0,0 +1,99 @@ +# Phase 4: Collection & Validation Binding - Discussion Log + +> **Audit trail only.** Do not use as input to planning, research, or execution agents. +> Decisions are captured in CONTEXT.md — this log preserves the alternatives considered. + +**Date:** 2026-07-15 +**Phase:** 04-collection-validation-binding +**Areas discussed:** Collection scope (List), VAL-01 validation model, VAL-02 empty semantics, API-02 exposure shape + +--- + +## Collection scope — is `List` in? + +| Option | Description | Selected | +|--------|-------------|----------| +| Full: add List family | Absorb COLL-01: one converter materializes List and satisfies List/IList/ICollection/IReadOnlyList/IReadOnlyCollection; broaden IsEnumerable/element detection. Meets ROADMAP + client doc which both name List. | ✓ | +| Arrays + IEnumerable only | Cheapest; List stays deferred; requires walking back the List wording and leaves an incoherent "empty List" half-state. | | + +**User's choice:** Full: add List family +**Notes:** Confirmed no converter matches List today (only ArrayTypeConverter/EnumerableTypeConverter), so COLL-02/COLL-03's List behavior requires real conversion support. COLL-01 (owner-deferred since Phase 2) is pulled into Phase 4. + +--- + +## VAL-01 — validation model + +### Q1: async vs sync signature + +| Option | Description | Selected | +|--------|-------------|----------| +| Redesign to sync | `ValidationResult Validate(...)`; hooks cleanly into the sync startup populate path, no sync-over-async hazard; free pre-beta (interfaces are dead). | ✓ | +| Keep async | Preserve Task; supports I/O validators but forces sync-over-async or an async pipeline ripple. | | + +**User's choice:** Redesign to sync + +### Q2: failure behavior + +| Option | Description | Selected | +|--------|-------------|----------| +| Aggregate, throw one exception | Collect every ValidationError, throw one SettingsValidationException : SimpleSettingsException; operator sees all problems at once. | ✓ | +| Fail-fast on first error | Throw on first invalid; simplest but slower multi-field debug loop. | | + +**User's choice:** Aggregate, throw one exception + +### Q3: object-level validator discovery + +| Option | Description | Selected | +|--------|-------------|----------| +| Attribute + Activator | `[SettingsValidator(typeof(X))]` on the interface, Activator-instantiated; works in both paths; no injected deps. | | +| DI-resolved | Container-registered ISettingValidation; supports injected deps but DI-path only. | | +| Both: attribute + optional DI | Attribute validators always run in the core path; DI-registered validators also resolve when a container is present. Most flexible. | ✓ | + +**User's choice:** Both: attribute + optional DI +**Notes:** Research flag captured — DI-path validator timing/ordering (validators with service deps) and keeping aggregate+redaction identical across both sites. + +--- + +## VAL-02 — what counts as "empty" + +### Q1: scope + +| Option | Description | Selected | +|--------|-------------|----------| +| "" + whitespace only | Reject null + empty + whitespace; skip the ${ENV:-} placeholder heuristic (lowest-urgency, false-positive risk). | ✓ | +| + unsubstituted ${ENV:-} | Also reject unresolved ${VAR}/${VAR:-}; meets full written ask but needs a precise heuristic. | | + +**User's choice:** "" + whitespace only + +### Q2: placement + +| Option | Description | Selected | +|--------|-------------|----------| +| Inline at conversion layer | Extend PropertyConversion.Convert's _throwOnNull branch; local, trivial, value-free exception at conversion. | ✓ | +| Built-in validator (rides VAL-01) | Express as an ISettingValidation flowing through the aggregate exception; more indirection for a one-line check. | | + +**User's choice:** Inline at conversion layer + +--- + +## API-02 — exposing `ISettingsCollection` + +| Option | Description | Selected | +|--------|-------------|----------| +| Resolvable DI singleton | AddSingleton; non-breaking, keeps fluent chain. | (part of choice) | +| Return value from the method | Hand back the collection directly; breaks IServiceCollection chaining or needs an overload. | (part of choice) | +| Both DI + provider surface | Register DI singleton + expose on ISettingsProvider; widest reach, largest surface. | | + +**User's choice:** Free-text — "we need to try them both and decide" → resolved to: implement both the DI singleton and a return-value overload, keep existing IServiceCollection overloads, decide the final shape at review on the branch (path A over a separate spike). +**Notes:** Reversal is free pre-beta, so building both and choosing empirically is low-risk. + +## Claude's Discretion + +- Exact new type/method names (SettingsValidationException, [SettingsValidator], VAL-02 exception type, API-02 return overload signature). +- Test file placement; the precise List-materialization mechanism (wrap array vs. build list directly). + +## Deferred Ideas + +- `${ENV:-}` unsubstituted-placeholder detection (VAL-02) — deferred. +- AOT-01 / DOC-01 — Phase 5; REL-01 — Phase 6; D2 EqualityCompererCreator — HELD. + diff --git a/.planning/phases/04-collection-validation-binding/04-PATTERNS.md b/.planning/phases/04-collection-validation-binding/04-PATTERNS.md new file mode 100644 index 0000000..d02d446 --- /dev/null +++ b/.planning/phases/04-collection-validation-binding/04-PATTERNS.md @@ -0,0 +1,273 @@ +# Phase 4: Collection & Validation Binding - Pattern Map + +**Mapped:** 2026-07-15 +**Files analyzed:** 17 (11 source new/modified + 6 test new/extend) +**Analogs found:** 17 / 17 (every file has an in-repo analog; this is a codebase-internal phase) + +## File Classification + +| New/Modified File | Role | Data Flow | Closest Analog | Match Quality | +|-------------------|------|-----------|----------------|---------------| +| NEW `Conversion/ListTypeConverter.cs` | converter | transform | `Conversion/ArrayTypeConverter.cs` + `Conversion/EnumerableTypeConverter.cs` | exact (both extend `CollectionTypeConverter`) | +| MOD `Core/Reflection/TypeExtensions.cs` (add `IsListLike`) | utility | transform | `IsEnumerable` in same file (lines 21-26) | exact | +| MOD `Core/Reflection/TypeConverter.cs` `CreateNullResult` | service | transform | the method itself (lines 27-36) | self | +| MOD `Conversion/TypeConvertersCollections.cs` (register `ListTypeConverter`) | config | event-driven (registration) | existing `AddLast(...)` chain (lines 7-12) | self | +| MOD `Conversion/PropertyConversion.cs` `Convert` (VAL-02) | service | transform | the method itself (lines 29-40) | self | +| MOD `Extensions.Binders/ConfigurationBinder.cs` `BindPropertySettings` (COLL-03) | binder | request-response (config read) | the method itself (lines 26-36) | self | +| NEW `SettingsValidationException.cs` (VAL-01) | model (exception) | — | `SettingsPropertyValueException.cs` / `SettingsBindingException.cs` (aggregate-carrying variant) | role-match | +| NEW `SettingsValidatorAttribute.cs` (VAL-01, name = discretion) | model (attribute) | — | `SettingsPropertyAttribute.cs` | role-match (different `AttributeTargets`) | +| MOD `Validations/ISettingsValidator.cs` + `ISettingValidation.cs` (async→sync) | model (interface) | — | the files themselves | self | +| MOD `Validations/ValidationContext.cs` + `ValidationContextOfT.cs` (add ctor) | model | — | `ValidationError.cs` ctor shape (lines 8-12) | role-match | +| MOD `ValuesPopulator.cs` `PopulateInstanceWithValues` (VAL-01 hook) + `ConvertPropertyValue` filter (VAL-02) | service | event-driven (post-populate) | the method itself (lines 29-65, 114-128) | self | +| MOD `Extensions.GenericHost/ServicesSettingsBuilderExtensions.cs` (API-02 + DI validators) | config (DI ext) | event-driven (registration) | the method itself (lines 7-44) | self | +| EXT `Conversion/CollectionConversionTests.cs` | test | — | itself (TUnit) | self | +| EXT `Core/TypeConverterTests.cs` | test | — | `CollectionConversionTests.cs` conventions | role-match | +| EXT `ConfigBuilderConfigurationBinderIntegrationTests.cs` | test | — | `AddSimpleSettingsIntegrationTests.cs` | role-match | +| EXT `Conversion/ExceptionRedactionTests.cs` | test | — | itself | self | +| EXT `DependencyInjection/AddSimpleSettingsIntegrationTests.cs` + NEW `SimpleSettings/SettingsValidationTests.cs` | test | — | `AddSimpleSettingsIntegrationTests.cs` | self / role-match | + +**Cross-cutting conventions (all source files):** one public type per file; `I`-prefixed interfaces; block-scoped namespaces (`namespace X { ... }`, NOT file-scoped — verified across every source file read); internal types by default, `public` only on the intended surface; no `[Serializable]`/serialization ctors; comments explain the *why* of a non-obvious performance/security choice, sparingly. `net8.0;net10.0` — no `field` keyword / extension members (C# 14 / net10-only) in shared library code. + +## Pattern Assignments + +### `Conversion/ListTypeConverter.cs` (NEW — converter, transform) + +**Analog:** `Conversion/ArrayTypeConverter.cs` (whole file) and `Conversion/EnumerableTypeConverter.cs` (whole file). Both are ~20-line `internal` subclasses of `CollectionTypeConverter` overriding only `CanConvert` + `GetElementType`. + +**Shape to copy** — `ArrayTypeConverter.cs:3-19` (constructor forwarding + two overrides): +```csharp +internal class ArrayTypeConverter : CollectionTypeConverter +{ + public ArrayTypeConverter(SettingsOptions settingsOptions, TypeConvertersCollections converters) + : base(settingsOptions, converters) { } + + public override bool CanConvert(Type settingsType) => settingsType.IsArray; + protected override Type GetElementType(Type settingsType) => settingsType.GetElementType()!; +} +``` + +**Generic-arg element type** — copy from `EnumerableTypeConverter.cs:18-21`: +```csharp +protected override Type GetElementType(Type settingsType) + => settingsType.GetTypeInfo().GetGenericArguments()[0]; // needs `using System.Reflection;` +``` + +**The wrap to `List`** — the base `CollectionTypeConverter.Convert` (`CollectionTypeConverter.cs:25-38`) builds a typed `Array`. To wrap it, the planner must make that method `virtual` (or extract a `protected` array-build helper). `CanConvert` uses the NEW `IsListLike` predicate. Materialize via `Activator.CreateInstance(typeof(List<>).MakeGenericType(elementType), builtArray)!`. Do NOT widen `IsEnumerable` (Pitfall 1). + +--- + +### `Core/Reflection/TypeExtensions.cs` (MODIFY — add `IsListLike`) + +**Analog:** the existing `IsEnumerable` (lines 21-26) — the exact predicate shape to mirror: +```csharp +public static bool IsEnumerable(this Type type) +{ + var info = type.GetTypeInfo(); + return info.IsGenericType && info.GetGenericTypeDefinition() == typeof(IEnumerable<>); +} +``` +`IsListLike` must be **disjoint** from `IsEnumerable` and `IsArray`: match generic definitions `List<>`, `IList<>`, `ICollection<>`, `IReadOnlyList<>`, `IReadOnlyCollection<>`. Same `GetTypeInfo().IsGenericType && GetGenericTypeDefinition() == ...` idiom, checked against the set. COLL-03's binder also needs a "collection shape" check (array OR list OR enumerable) — either add a combined `IsCollectionShape` helper here or compose the three predicates. + +--- + +### `Core/Reflection/TypeConverter.cs` — `CreateNullResult` (MODIFY, COLL-02) + +**Analog:** the method itself (lines 27-36). Today it returns `null` for reference types that are not the open `IEnumerable<>`: +```csharp +private static object? CreateNullResult(Type propertyType) +{ + if (!propertyType.IsEnumerable()) + return propertyType.GetTypeInfo().IsValueType ? Activator.CreateInstance(propertyType) : null; + var elementType = propertyType.GetTypeInfo().GetGenericArguments()[0]; + return Array.CreateInstance(elementType, 0); +} +``` +Extend the shape branch: array target → `Array.CreateInstance(elementType, 0)` (element type via `GetElementType()` / `GetElementType()!`); list family → empty `List` (`Activator.CreateInstance(typeof(List<>).MakeGenericType(elementType))!`); open `IEnumerable` → unchanged empty array; else → existing value-type/null branch. Reuse the same `Array.CreateInstance` idiom already in the file (do not reintroduce `Enumerable.Empty` reflection — see State of the Art). + +--- + +### `Conversion/TypeConvertersCollections.cs` (MODIFY — register `ListTypeConverter`) + +**Analog:** the ctor itself (lines 5-13): +```csharp +AddLast(new DateTimeTypeConverter(settingsOptions)); +AddLast(new UriTypeConvertor()); +AddLast(new ArrayTypeConverter(settingsOptions, this)); +AddLast(new EnumerableTypeConverter(settingsOptions, this)); +AddLast(new EnumTypeConverter()); +AddLast(new DefaultTypeConverter()); +``` +Insert `AddLast(new ListTypeConverter(settingsOptions, this));` after `EnumerableTypeConverter` and before `EnumTypeConverter`. Predicate is disjoint, so order is safe. **Do NOT reorder** existing entries (load-bearing user-first / `DefaultTypeConverter`-last). + +--- + +### `Conversion/PropertyConversion.cs` — `Convert` (MODIFY, VAL-02) + +**Analog:** the method itself (lines 29-40): +```csharp +public object? Convert(object? value) +{ + if (value == null) + { + if (_throwOnNull) + throw new SettingsPropertyNullException(_propertyName); + return _nullResult; + } + return _converter.Convert(value, _strippedType); +} +``` +Extend the guard so `_throwOnNull` also rejects null-or-whitespace strings (`value is string s && string.IsNullOrWhiteSpace(s)`). Throw a **value-free** `SimpleSettingsException` subtype — reuse `SettingsPropertyNullException` (already excluded from the redaction filter, simplest) OR a new sibling. If a NEW type is introduced, the `ValuesPopulator.ConvertPropertyValue` filter at line 122 MUST be widened (see below) — Pitfall 3. `_throwOnNull` resolved at `TypeConverter.cs:16` (`attribute is { AllowEmpty: false }`). + +--- + +### `Extensions.Binders/ConfigurationBinder.cs` — `BindPropertySettings` (MODIFY, COLL-03) + +**Analog:** the method itself (lines 26-36): +```csharp +public void BindPropertySettings(BindingContext context) +{ + var section = _sections.GetOrAdd(context.Section, static (name, self) => self.ResolveSection(name), this); + var value = section[context.Key]; + if (value != null) + context.SetNewValue(value); +} +``` +Add a collection-shape branch BEFORE the scalar read: `section.GetSection(context.Key).GetChildren()` (`using Microsoft.Extensions.Configuration;` already present); if children exist, `context.SetNewValue(childValues.ToArray())` (a `string[]`) and return (children win). Fall through to the scalar otherwise; for collection targets, skip `SetNewValue` when the scalar is null/whitespace (empty→empty; COLL-02 default applies). Use `context.PropertyType` + the new `IsCollectionShape`/`IsListLike|IsArray|IsEnumerable` predicate. Preserve the `static` factory `GetOrAdd` (no per-call closure) and the `ResolveSection` root/`[SettingsSection]` prefixing (lines 38-41) — inherited for free by `GetSection(key)`. `CollectionTypeConverter.AsArray` (lines 43-51) passes an incoming `Array` through unchanged, so no converter change. + +**Redaction (D-06 gate):** this method runs inside `ValuesPopulator`'s catch → `SettingsBindingException` (which retains only binder type/section/key primitives, never the value — see `SettingsBindingException.cs:8-16`). Do not put any element value into an exception message. + +--- + +### `SettingsValidationException.cs` (NEW — VAL-01 aggregate exception) + +**Analog:** `SettingsPropertyValueException.cs` (lines 13-33) for the property-carrying shape, and `SettingsBindingException.cs` for the "carries typed context via base ctor + exposes read-only props" pattern. All derive `SimpleSettingsException` (message-only or message+inner base ctors — `SimpleSettingsException.cs:10-18`; no serialization ctor). + +**Shape to copy** (from `SettingsPropertyValueException.cs`): +```csharp +public class SettingsPropertyValueException : SimpleSettingsException +{ + public SettingsPropertyValueException(Type settingsType, PropertyInfo property, Type conversionErrorType) + : base(Resources.PropertySetterExceptionMessage(settingsType, property, conversionErrorType.Name)) + { ... SettingsType = settingsType; ... } + public Type SettingsType { get; } +} +``` +For `SettingsValidationException`: ctor takes `IReadOnlyList` (from `Validations/ValidationError.cs` — has `SettingsName`, `ErrorMessage`), exposes `Errors`, and builds its message via a NEW `Resources` factory. **Redaction (D-12):** message composed ONLY from author-supplied `ValidationError.ErrorMessage` — add no bound values. Follow the `Resources.cs` static-factory-method convention (lines 35-43) for the message text; mirror the S1 comment style of `SettingsPropertyValueException` explaining why no value is embedded. + +--- + +### `SettingsValidatorAttribute.cs` (NEW — VAL-01 object-level, name = discretion) + +**Analog:** `SettingsPropertyAttribute.cs` (whole file): +```csharp +[AttributeUsage(AttributeTargets.Property)] +public class SettingsPropertyAttribute : Attribute +{ + public Type? ConverterType { get; set; } + public Type? ValidatorType { get; set; } + public SettingsPropertyAttribute(string name) { Name = name; } + public SettingsPropertyAttribute() { } +} +``` +New attribute uses `[AttributeUsage(AttributeTargets.Interface)]`, carries `Type ValidatorType` (the `ISettingValidation` impl), ctor `SettingsValidatorAttribute(Type validatorType)`. Read at plan build via `GetTypeInfo().GetCustomAttribute<...>(true)` (same idiom as `TypeExtensions.GetSectionName` line 14 and `ValuesPopulator.cs:84`). Instantiate the validator via `Activator.CreateInstance` (parameterless ctor) — same pattern as `TypeConverter.GetConverter` line 43. + +--- + +### `Validations/*` (MODIFY — sync signatures + ctor, VAL-01) + +**Interfaces** (`ISettingsValidator.cs:3-6`, `ISettingValidation.cs:3-6`) — drop `Task<>`: +```csharp +public interface ISettingsValidator { ValidationResult Validate(ValidationContext context); } +public interface ISettingValidation : ISettingsValidator { ValidationResult Validate(ValidationContext context); } +``` + +**Context ctors** — analog is `ValidationError.cs:8-12` (plain ctor-assigns-getter pattern). `ValidationContext.cs:5` and `ValidationContextOfT.cs:5` are getter-only: +```csharp +public class ValidationContext { public object? Settings { get; } } +public class ValidationContext : ValidationContext { public new T? Settings { get; } } +``` +Add `ValidationContext(object settings)` and `ValidationContext(T settings)` (the derived ctor calls `base(settings)` and sets the shadowed `T? Settings`). `ValidationResult` (`AddError`/`Errors`/`IsValid`) and `ValidationError` are already complete — do not change. + +--- + +### `ValuesPopulator.cs` — validation hook + filter (MODIFY, VAL-01 + VAL-02) + +**Analog:** the method itself. **VAL-01 hook (D-09):** after the `foreach (var propertyPlan in plan.Properties)` loop (`ValuesPopulator.cs:38-64`), once the instance is fully populated: gather core-path validators (object-level `[SettingsValidator]` on the type via `Activator`; property-level `[SettingsProperty(ValidatorType)]` threaded through the `PropertyPlan` built at lines 90-93), run all, aggregate `ValidationError`s, throw ONE `SettingsValidationException`. + +**VAL-02 filter** — `ConvertPropertyValue` (lines 114-128), the exception filter at line 122: +```csharp +catch (Exception e) when (e is not SettingsPropertyNullException) +{ + throw new SettingsPropertyValueException(settingsType, propertyPlan.Property, e.GetType()); +} +``` +If VAL-02 uses a NEW empty-exception type, widen to `when e is not (SettingsPropertyNullException or SettingsPropertyEmptyException)`. Preserve the S1 comment intent (value never chained/embedded). + +--- + +### `Extensions.GenericHost/ServicesSettingsBuilderExtensions.cs` (MODIFY, API-02 + VAL-01 DI) + +**Analog:** the method itself (lines 7-44). `IntegrateSimpleSettings` currently returns `void` and builds the collection at line 32: +```csharp +var settingsCollection = settingsBuilder.ScanAssemblies(options!.Assemblies); +foreach (var settings in settingsCollection) + services.AddSingleton(settings.Key, settings.Value); +services.AddSingleton(new SettingsProvider(settingsCollection, settingsBuilder)); +``` +API-02: add `services.AddSingleton(settingsCollection)` (instance registration; collection is fully built — `ISettingsCollection` public, `SettingsCollection` internal), refactor `IntegrateSimpleSettings` to **return** the `ISettingsCollection`, add a new public entry point that surfaces it (cannot overload by return type — use an `out ISettingsCollection` overload or a distinctly-named method; keep existing `IServiceCollection`-returning overloads at lines 7-19). VAL-01 DI path: DI-resolved `ISettingValidation` cannot run inline (container not built at registration — Pitfall 4); defer to a post-provider-build step, keeping the aggregate + `SettingsValidationException` contract identical to the core path (planner decision, gated by architect + security-auditor per Q3). + +--- + +## Shared Patterns + +### Exception family (S1 redaction) +**Source:** `SimpleSettingsException.cs:8-19` (abstract base, two forwarding ctors, no serialization ctor); `SettingsPropertyValueException.cs` (value-free — takes a failure `Type`, never the value/inner); `SettingsBindingException.cs` (primitives only, never `BindingContext`). +**Apply to:** `SettingsValidationException` and any VAL-02 exception — derive `SimpleSettingsException`, embed no bound values, message via a `Resources` factory. +```csharp +public abstract class SimpleSettingsException : Exception +{ + protected SimpleSettingsException(string message) : base(message) { } + protected SimpleSettingsException(string message, Exception innerException) : base(message, innerException) { } +} +``` + +### Resource message factories +**Source:** `Resources.cs:35-43` (static methods returning interpolated strings; the value deliberately omitted for S1). +**Apply to:** every new exception message. Add a `SettingsValidationExceptionMessage(...)` (and, if a new VAL-02 type, its message) here rather than inlining strings. + +### Attribute read at plan build (no per-populate cost) +**Source:** `ValuesPopulator.cs:84` (`GetCustomAttribute(inherit: true)` read once per property into the plan) and `TypeExtensions.cs:14` (`GetTypeInfo().GetCustomAttribute(true)`). +**Apply to:** reading `[SettingsValidator]` (type-level) and `ValidatorType` (property-level) — resolve at plan build, thread into the `PropertyPlan`/plan, not on the hot populate path. + +### Activator-based instantiation +**Source:** `TypeConverter.cs:43` (`(ISettingsTypeConverter)Activator.CreateInstance(attribute.ConverterType)!`). +**Apply to:** core-path validator instantiation (parameterless ctor). + +### Perf-conscious iteration +**Source:** `CollectionTypeConverter.cs:53-67` and `TypeConverter.cs:45-56` — manual `foreach` over the concrete `LinkedList` (no LINQ `First`/`Where`, no boxed enumerator/closure) on populate-frequency paths; `static` factory + factory-argument `GetOrAdd` in `ConfigurationBinder.cs:30`. +**Apply to:** any new per-populate loop (validator gathering that runs per populate should prefer this; per-plan-build work may use LINQ). + +### TUnit test conventions +**Source:** `Conversion/CollectionConversionTests.cs` (public class, `[Test]` methods `async Task`, `await Assert.That(...).IsTrue()/.IsEqualTo()/.Contains()`, private `Build(...)` helper); `Conversion/ExceptionRedactionTests.cs` (sentinel-secret + `AssertRedacted` helper asserting absence from full `ToString()`); `DependencyInjection/AddSimpleSettingsIntegrationTests.cs` (`ServiceCollection` → `AddSimpleSettings(o => ...)` → `BuildServiceProvider().GetRequiredService()`, in-memory `InMemoryCollection`/`InMemoryBinder` fakes). +**Apply to:** all Wave 0 test files. Method naming `Method_State_Expected`. `--treenode-filter` (NOT `--filter`); build/test from `src/`. Co-locate new fixture interfaces (`[SettingsValidator]` / `ValidatorType` / list-shaped + sequence-backed) per existing convention. +```csharp +[Test] +public async Task Convert_ToIntEnumerable_MaterializesAnArray() // pinned — keep green (COLL-01 regression guard) +{ + var result = Build("IntEnumerable", nameof(IIntEnumerable.Values), "5,6,7"); + await Assert.That(result.Values is int[]).IsTrue(); +} +``` + +## No Analog Found + +None. Every file maps to an in-repo analog or is a self-modification. The only genuinely novel design decision — DI-resolved validator timing (Q3) — is an architecture choice, not a missing pattern; the aggregate-exception + redaction contract it must satisfy is fully modeled by the exception-family analogs above. + +## Metadata + +**Analog search scope:** `src/Core/ExistForAll.SimpleSettings/` (Conversion, Core/Reflection, Validations, root exceptions/attributes), `src/Core/ExistForAll.SimpleSettings.Extensions.Binders/`, `src/Core/ExistForAll.SimpleSettings.Extensions.GenericHost/`, `src/Tests/ExistForAll.SimpleSettings.UnitTests/`. +**Files scanned:** 21 source + 3 test files read at file:line. +**Project conventions source:** no `CLAUDE.md` / `.claude/skills` in-repo (research cites external dotnet-kit `modern-csharp` + `testing` skills); conventions inferred from read source + CONTEXT §Established Patterns. +**Pattern extraction date:** 2026-07-15 + + diff --git a/.planning/phases/04-collection-validation-binding/04-RESEARCH.md b/.planning/phases/04-collection-validation-binding/04-RESEARCH.md new file mode 100644 index 0000000..eeb043c --- /dev/null +++ b/.planning/phases/04-collection-validation-binding/04-RESEARCH.md @@ -0,0 +1,530 @@ +# Phase 4: Collection & Validation Binding - Research + +**Researched:** 2026-07-15 +**Domain:** SimpleSettings binding engine (internal C# library) — collection conversion, config-sequence binding, settings validation, DI surface +**Confidence:** HIGH (codebase-internal; every claim below is grounded in read source with file:line) + + +## User Constraints (from CONTEXT.md) + +### Locked Decisions +- **D-01:** Land **full `List` family support** this phase. One converter materializes a `List` and satisfies `List`, `IList`, `ICollection`, `IReadOnlyList`, `IReadOnlyCollection`; arrays stay on `ArrayTypeConverter`. Broaden `TypeExtensions.IsEnumerable` (`Core/Reflection/TypeExtensions.cs:21-26`) and element-type detection. Reuse the existing `CollectionTypeConverter` array-build path (`Conversion/CollectionTypeConverter.cs`), then wrap into a `List`. +- **D-02:** Extend `TypeConverter.CreateNullResult` (`Core/Reflection/TypeConverter.cs:27-36`) so an unset `T[]`, `List` family, and `IEnumerable` all bind an **empty collection, never `null`**. Materialize `Array.CreateInstance(elementType, 0)` for arrays and a new empty `List` for the list shapes. +- **D-03:** Sequencing — **COLL-02 before COLL-03** (COLL-03's empty behavior leans on COLL-02's empty-not-null default). +- **D-04:** `ConfigurationBinder.BindPropertySettings` (`Extensions.Binders/ConfigurationBinder.cs:26-36`) reads a single scalar today. For collection-typed targets, enumerate `GetChildren()` and set the sequence (as an array / `string[]`) onto `context`. `CollectionTypeConverter.AsArray` (`:43-51`) passes an incoming `Array` straight through. +- **D-05:** Locked sub-rules: **children win** over scalar; **comma-scalar MUST still bind** (prod `MultiHost__CommonHosts`); empty / whitespace / empty-sequence → **empty, never `[""]`**; each element flows the inner converter chain; honor `[SettingsSection]` / root prefixing. +- **D-06 (SECURITY GATE):** COLL-03 edits run inside `ValuesPopulator`'s wrapping catch (`ValuesPopulator.cs:50-60`). **S1/SEC-01 secret-redaction MUST be re-verified** with a regression test proving a secret in a YAML sequence element is absent from the whole `ex.ToString()` chain on a bind/convert failure. `security-auditor` owns this. +- **D-07:** Redesign the held validator interfaces to **synchronous** — `ISettingsValidator.Validate` / `ISettingValidation.Validate` return `ValidationResult` (drop `Task<>`). +- **D-08:** Complete the held scaffolding — `ValidationContext` / `ValidationContext` need a constructor so the populated `Settings` can be carried. +- **D-09:** Timing — validation runs at build/populate (startup), hooked after the property-set loop in `ValuesPopulator.PopulateInstanceWithValues` (`ValuesPopulator.cs:29-65`). +- **D-10:** Failure — aggregate, throw once. New `SettingsValidationException : SimpleSettingsException` carrying the full `ValidationError` list. +- **D-11:** Discovery — **BOTH** mechanisms. *Core path (always):* property-level `[SettingsProperty(ValidatorType=typeof(X))]` **and** an object-level interface attribute (e.g. `[SettingsValidator(typeof(X))]`), instantiated via `Activator` (parameterless ctor). *DI path (`AddSimpleSettings`):* also resolve registered `ISettingValidation` from the container. +- **D-12:** Redaction — `SettingsValidationException` adds no bound values itself; `ValidationError` messages are validator-author-supplied (author owns not echoing secrets; document this). Re-verify no leak. +- **D-13:** `[SettingsProperty(AllowEmpty=false)]` rejects **null + `""` + whitespace** (`string.IsNullOrWhiteSpace`). `${ENV:-}` detection **deferred**. +- **D-14:** Inline at the conversion layer — extend `PropertyConversion.Convert`'s `_throwOnNull` branch (`Conversion/PropertyConversion.cs:29-40`). Throw a **value-free** exception in the `SimpleSettingsException` family; exact type is the planner's call. NOTE: client doc's `TypeConverter.ValidateNullAcceptance` pointer is **stale** — `PropertyConversion` is the real hook. +- **D-15:** Implement **both** API-02 exposure mechanisms: (1) `services.AddSingleton(settingsCollection)`; (2) an `AddSimpleSettings` variant that returns `ISettingsCollection`. Keep the existing `IServiceCollection`-returning overloads. `ISettingsCollection` public; `SettingsCollection` internal. + +### Claude's Discretion +- Exact new type/method names (`SettingsValidationException`, `[SettingsValidator]`, the VAL-02 exception type, the API-02 return-overload signature), test file placement under `src/Tests/ExistForAll.SimpleSettings.UnitTests/`, and the precise `List`-materialization mechanism (wrap the built array vs. build the list directly). All breaking/behavior changes are expected and batched pre-beta. + +### Deferred Ideas (OUT OF SCOPE) +- `${ENV:-}` unsubstituted-placeholder detection (VAL-02) — deferred. +- AOT-01 / DOC-01 — Phase 5. REL-01 — Phase 6. +- D2 `EqualityCompererCreator` (EQ-01) — HELD, out of scope. +- `IOptionsMonitor`-style reload. + + + +## Phase Requirements + +| ID | Description | Research Support | +|----|-------------|------------------| +| COLL-02 | Unset `T[]`/`List` bind an empty collection, never `null` | `CreateNullResult` shape detection + empty-materialization; §Standard Stack / Pattern 1 | +| COLL-03 | Bind collections from config child-section sequences via `GetChildren()`; comma-scalar still binds; children win; empty→empty; elements flow the converter chain; re-verify redaction | `ConfigurationBinder.BindPropertySettings` sequence read + `AsArray` passthrough; §Pattern 2; §Security Domain | +| VAL-01 | Wire `ISettingValidation` + `[SettingsProperty(ValidatorType)]` into the populate path incl. cross-property rules; BOTH discovery mechanisms; aggregate-throw | Sync interface redesign + `ValidationContext` ctor + post-populate hook + `SettingsValidationException`; §Pattern 3; §Open Questions (DI timing) | +| VAL-02 | `[SettingsProperty(AllowEmpty=false)]` rejects `""`/whitespace at bind, value-free | `PropertyConversion.Convert` `_throwOnNull` branch extension + exception-filter update; §Pattern 4 | +| API-02 | `AddSimpleSettings(...)` exposes `ISettingsCollection` (DI singleton + return-value) | `IntegrateSimpleSettings` refactor; §Pattern 5; §Open Questions (overload shape) | + + +## Summary + +This is an entirely codebase-internal phase: no new external dependencies, no new NuGet packages. Every capability slots into an existing, well-factored seam — the converter chain (`TypeConvertersCollections`), the per-type `SettingsPlan` cache, the `ISectionBinder` chain, and the `AddSimpleSettings` DI extension. The engineering risk is not "how does technology X work" but "how do these five edits interact with the load-bearing invariants already in place": converter ordering, last-writer-wins binder precedence, the S1 secret-redaction contract, and the per-type plan cache. + +The five changes decompose cleanly. **COLL-01/02** broaden collection-shape detection in one reflection helper and add a `List`-materializing converter that reuses the shared array-build path. **COLL-03** adds a `GetChildren()` branch to one binder method; because `CollectionTypeConverter.AsArray` already passes an `Array` straight through the element-converter chain, the binder only has to produce a `string[]` — no converter change. **VAL-01** completes dead scaffolding (sync signature + a constructor) and adds one post-populate hook plus a new aggregate exception; its only genuine unknown is DI-resolved-validator timing (settings are built at *registration*, before the container exists). **VAL-02** extends one `if` branch and — critically — must be reflected in the redaction exception-filter. **API-02** is a small refactor of `IntegrateSimpleSettings` plus new overloads, with one real C# constraint (you cannot overload by return type alone). + +**Primary recommendation:** Implement in the locked order (COLL-02 → COLL-03 → VAL-01 → VAL-02 → API-02). Keep `IsEnumerable` narrow and introduce a *separate* list-shape predicate + `ListTypeConverter` (broadening `IsEnumerable` in place would make `EnumerableTypeConverter` claim `List` and return an array, which cannot be assigned to a `List` property). Treat DI-path validator timing as an explicit planner decision gated by the architect + security-auditor. + +## Architectural Responsibility Map + +| Capability | Primary Tier | Secondary Tier | Rationale | +|------------|-------------|----------------|-----------| +| `List` family conversion (COLL-01) | Core library — `Conversion/*TypeConverter` + `TypeConvertersCollections` | Core — `Reflection/TypeExtensions` (shape detection) | Conversion is owned by the converter chain; shape detection is a reflection concern shared with the null-result path | +| Empty-not-null default (COLL-02) | Core library — `Reflection/TypeConverter.CreateNullResult` | — | The null/empty outcome is resolved once at plan build; it is a conversion-plan concern, not a binder concern | +| Config child-sequence read (COLL-03) | Binders extension — `Extensions.Binders/ConfigurationBinder` | Core — `CollectionTypeConverter.AsArray` (passthrough) | Reading `IConfiguration.GetChildren()` is a config-provider concern owned by the binder; the converter just consumes the produced `string[]` | +| Settings validation execution (VAL-01, core path) | Core library — `ValuesPopulator.PopulateInstanceWithValues` | Core — `Validations/*`, `Activator` | Validation runs once the instance is fully populated; the populate loop is the only place with the finished instance | +| Settings validation (VAL-01, DI-resolved) | GenericHost extension — `ServicesSettingsBuilderExtensions` | Core — shared aggregate/exception contract | Only the DI extension has a container; service-dependent validators must resolve there | +| Empty-string rejection (VAL-02) | Core library — `Conversion/PropertyConversion.Convert` | Core — exception-filter in `ValuesPopulator.ConvertPropertyValue` | Empty rejection is a per-property binding-layer concern resolved at plan build (`throwOnNull`), not a VAL-01 validator | +| `ISettingsCollection` exposure (API-02) | GenericHost extension — `ServicesSettingsBuilderExtensions` | Core — `ISettingsCollection` (already public) | The collection is built in the DI integration path; exposure is a DI-surface concern | + +## Standard Stack + +No new libraries. This phase uses only what is already referenced and pinned. + +### Core +| Library | Version | Purpose | Why Standard | +|---------|---------|---------|--------------| +| .NET BCL (`System.*`) | net8.0 / net10.0 | `List`, `Array.CreateInstance`, reflection, `Activator` | Target runtimes; no dependency added `[VERIFIED: dotnet --list-sdks → 10.0.301]` | +| `Microsoft.Extensions.Configuration` | 8.0.0 (net8) / 10.0.9 (net10) | `IConfigurationSection.GetChildren()` for COLL-03 | Already referenced by the Binders package `[VERIFIED: src/Directory.Packages.props]` | +| `Microsoft.Extensions.DependencyInjection(.Abstractions)` | 8.0.x / 10.0.9 | `AddSingleton`, DI-resolved validators (API-02 / VAL-01 DI path) | Already referenced by the GenericHost package `[VERIFIED: src/Directory.Packages.props]` | + +### Supporting (test only) +| Library | Version | Purpose | When to Use | +|---------|---------|---------|-------------| +| TUnit | 1.58.0 | Test framework on Microsoft.Testing.Platform | All new tests `[VERIFIED: src/Directory.Packages.props + global.json runner]` | + +### Alternatives Considered +| Instead of | Could Use | Tradeoff | +|------------|-----------|----------| +| Reusing `CollectionTypeConverter` array path + wrap to `List` | Build `List` directly element-by-element in a bespoke converter | Direct build avoids one array→list copy but duplicates the split/convert/passthrough logic already de-reflected in P4; wrap reuses the shared path (D-01 favors reuse). See Open Questions Q1 | +| `AddSingleton` instance registration | Factory registration `AddSingleton(sp => ...)` | Instance registration is correct here — the collection is fully built at registration time; a factory adds nothing | + +**Installation:** None. `dotnet restore` uses the existing central package versions. + +## Package Legitimacy Audit + +Not applicable — this phase installs **no external packages**. All dependencies (`Microsoft.Extensions.*`, TUnit) are already present and pinned via central package management in `src/Directory.Packages.props`. No `npm`/`pip`/`cargo` and no new `PackageVersion` entries are introduced. + +## Architecture Patterns + +### System Architecture Diagram + +``` + SettingsBuilder.CreateBuilder / AddSimpleSettings + │ + ▼ + ScanAssemblies / GetSettings(type) + │ + ▼ + ValuesPopulator.PopulateInstanceWithValues + │ + ┌──────────────────────────┼───────────────────────────┐ + ▼ ▼ ▼ + GetOrBuildPlan (cached) per-property binder loop [VAL-01 NEW hook] + • key / default (last-writer-wins) run validators AFTER + • PropertyConversion │ the property loop + - throwOnNull ◄─ VAL-02 ▼ │ + - nullResult ◄─ COLL-02 ISectionBinder chain ▼ + • ConfigurationBinder aggregate ValidationError[] + ├─ scalar section[key] throw SettingsValidationException + └─ [COLL-03 NEW] (core: Activator; + GetSection(key) DI: resolved ISettingValidation) + .GetChildren() → string[] + │ + ▼ + tempValue (string | string[]) + │ + ▼ + PropertyConversion.Convert(value) + • null/empty → nullResult OR throw (VAL-02) + • else → converter.Convert + │ + ┌─────────────────────────┼──────────────────────────┐ + ▼ ▼ ▼ + ArrayTypeConverter [ListTypeConverter NEW] EnumerableTypeConverter + (IsArray) (List/IList/...) (open IEnumerable<>) + └──────────── CollectionTypeConverter.AsArray + element chain ───────┘ + │ + ▼ + redaction boundary: convert failure → + SettingsPropertyValueException (value-free) [S1 / COLL-03 gate] + + API-02: IntegrateSimpleSettings also does AddSingleton(collection) + and returns the collection through a new AddSimpleSettings overload shape. +``` + +### Recommended Project Structure + +No new folders. Files land in existing locations: +``` +src/Core/ExistForAll.SimpleSettings/ +├── Core/Reflection/TypeExtensions.cs # broaden shape detection (COLL-01/02) +├── Core/Reflection/TypeConverter.cs # CreateNullResult empty-not-null (COLL-02) +├── Conversion/ListTypeConverter.cs # NEW converter (COLL-01) +├── Conversion/TypeConvertersCollections.cs # register ListTypeConverter (order matters) +├── Conversion/PropertyConversion.cs # empty rejection (VAL-02) +├── ValuesPopulator.cs # validation hook + exception-filter update +├── Validations/*.cs # sync signatures + ctor (VAL-01) +├── SettingsValidationException.cs # NEW aggregate exception (VAL-01) +├── SettingsValidatorAttribute.cs # NEW object-level attribute (VAL-01) [name = discretion] +└── (VAL-02 exception: reuse SettingsPropertyNullException OR new sibling) +src/Core/ExistForAll.SimpleSettings.Extensions.Binders/ +└── ConfigurationBinder.cs # GetChildren() sequence read (COLL-03) +src/Core/ExistForAll.SimpleSettings.Extensions.GenericHost/ +└── ServicesSettingsBuilderExtensions.cs # ISettingsCollection exposure + DI validators (API-02/VAL-01) +src/Tests/ExistForAll.SimpleSettings.UnitTests/ # extend Conversion/, Core/, DependencyInjection/, SimpleSettings/ +``` + +### Pattern 1: `List` family — separate predicate + wrap the built array (COLL-01/02) + +**What:** A new `ListTypeConverter : CollectionTypeConverter` that reuses the base array-build path (`CollectionTypeConverter.Convert` → `Array.CreateInstance` + element convert) and wraps the result into a `List`. Shape detection lives in a *new* predicate, not by widening `IsEnumerable`. + +**When to use:** Target property is `List`, `IList`, `ICollection`, `IReadOnlyList`, or `IReadOnlyCollection`. + +**Why a separate predicate (critical):** `IsEnumerable` (`TypeExtensions.cs:21-26`) is consumed in **two** places — `EnumerableTypeConverter.CanConvert` (`EnumerableTypeConverter.cs:13`) and `TypeConverter.CreateNullResult` (`TypeConverter.cs:29`). If you widen `IsEnumerable` to also match the list shapes, `EnumerableTypeConverter` will claim `List` and return a `T[]`; `T[]` is **not** assignable to a `List` property, so `PropertyInfo.SetValue` throws. The pinned test `Convert_ToIntEnumerable_MaterializesAnArray` (`CollectionConversionTests.cs:38-46`) also requires `IEnumerable` to keep returning an array. `[VERIFIED: codebase grep — IsEnumerable used at TypeExtensions.cs:21, EnumerableTypeConverter.cs:13, TypeConverter.cs:29]` + +**Converter ordering:** current chain (`TypeConvertersCollections.cs:7-12`) is `DateTime, Uri, Array, Enumerable, Enum, Default`. Insert `ListTypeConverter` so its predicate is **disjoint** from `ArrayTypeConverter` (`IsArray`) and `EnumerableTypeConverter` (`IsEnumerable`, i.e. exactly the open `IEnumerable<>`). A `List` is neither `IsArray` nor the `IEnumerable<>` definition, so it currently falls through to `DefaultTypeConverter` and throws via `Convert.ChangeType`. Placement after `EnumerableTypeConverter` and before `EnumTypeConverter`/`DefaultTypeConverter` is safe because the predicates do not overlap. **Do not reorder** the existing converters (load-bearing per CONVENTIONS / CONTEXT). `[VERIFIED: TypeConvertersCollections.cs:7-12; ArrayTypeConverter.cs:10-13; EnumerableTypeConverter.cs:13-15]` + +**Cheapest materialization:** `new List(array)` where `array` is the base-built `Array`. **Correction (review S-4/A1):** `new List(array)` does NOT alias the array — because the built `T[]` is an `ICollection`, the `List(ICollection)` constructor allocates a fresh right-sized backing buffer and `CopyTo`s the elements into it, so the wrap transiently holds two n-sized buffers (an element copy, not a cheap "reference-copy" as previously stated). This satisfies all five shapes because `List` implements `IList`, `ICollection`, `IReadOnlyList`, `IReadOnlyCollection`. **Do NOT materialize via `Activator.CreateInstance(typeof(List<>).MakeGenericType(elementType), builtArray)` on the per-populate path** — that reintroduces the reflection idiom P4 deleted plus transient `Type[1]`/`object[1]`/`Invoke`. Use a cached per-element-type `Func` factory (built once via a generic helper doing `new List(array)`), so per-populate materialization is a plain delegate call. `CreateNullResult`'s cold plan-build reflection stays as-is (the empty-`List` null-result must additionally be produced fresh per bind — a shared mutable empty list is a correctness bug, review B-3). The array build is unchanged, so the array/enumerable hot paths (benchmark-gated) are untouched; the list wrap only runs for previously-throwing list targets. `[ASSUMED — .NET BCL semantics; verify no benchmark regression per PERF-01/PERF-02 gate]` + +**Example (illustrative shape only — planner owns final form):** +```csharp +// Source: derived from CollectionTypeConverter.cs:25-38 (existing array-build path) +internal class ListTypeConverter : CollectionTypeConverter +{ + public ListTypeConverter(SettingsOptions settingsOptions, TypeConvertersCollections converters) + : base(settingsOptions, converters) { } + + public override bool CanConvert(Type settingsType) => settingsType.IsListLike(); + + protected override Type GetElementType(Type settingsType) + => settingsType.GetTypeInfo().GetGenericArguments()[0]; + + // wrap the base-built typed array into a List + public override object Convert(object value, Type settingsType) + { + var built = (Array)base.Convert(value, settingsType); + var elementType = GetElementType(settingsType); + var listType = typeof(List<>).MakeGenericType(elementType); + return Activator.CreateInstance(listType, built)!; + } +} +``` +Note: `CollectionTypeConverter.Convert` is currently a non-virtual public method (`CollectionTypeConverter.cs:25`). To override it, the planner makes it `virtual` (or extracts the array-build into a protected helper the list converter calls). Either is a mechanical, non-breaking internal change. + +**COLL-02 empty-not-null:** `CreateNullResult` (`TypeConverter.cs:27-36`) must branch on shape: +- array target → `Array.CreateInstance(elementType, 0)` (element type via `GetElementType()`), +- list family → empty `List`, +- open `IEnumerable` → `Array.CreateInstance(elementType, 0)` (unchanged), +- else → value-type default / null (unchanged). + +Today arrays hit the `!IsEnumerable()` branch and return `null` because an array is a reference type — so unbound `int[]` currently binds `null`, which COLL-02 fixes. `[VERIFIED: TypeConverter.cs:27-36 — array is reference type, IsValueType false → returns null]` + +### Pattern 2: Config child-sequence binding (COLL-03) + +**What:** In `ConfigurationBinder.BindPropertySettings` (`ConfigurationBinder.cs:26-36`), when the target property is collection-typed, read `section.GetSection(context.Key).GetChildren()` and, if any children exist, set a `string[]` of their `.Value` onto the context (children win over the scalar). Otherwise fall back to the existing scalar `section[context.Key]`. + +**When to use:** `context.PropertyType` is array / list-family / `IEnumerable` (same shape predicate as Pattern 1). Reuse the shape helper so scalars are untouched. + +**Why it flows unchanged:** `CollectionTypeConverter.AsArray` (`CollectionTypeConverter.cs:43-51`) already does `value as Array ?? new[]{value}` — an incoming `string[]` passes straight through, then each element goes through `GetElementConverter` (int/enum/DateTime/Uri all keep working). So the binder only needs to produce a `string[]`; **no converter change** for COLL-03. `[VERIFIED: CollectionTypeConverter.cs:43-51]` + +**Locked sub-rules mapped to mechanism (D-05):** +- **Children win:** check `GetChildren()` first; only fall back to `section[key]` when there are no children. +- **Comma-scalar still binds:** when no children, `section[context.Key]` returns `"a,b,c"` exactly as today → `AsArray` splits on `ArraySplitDelimiter` (`SettingsOptions.cs:13`, default `","`). Prod `MultiHost__CommonHosts` env override unaffected. `[VERIFIED: SettingsOptions.cs:13; CollectionTypeConverter.cs:45-48]` +- **Empty / whitespace / empty-sequence → empty, never `[""]`:** if `GetChildren()` is empty AND the scalar is `null`/whitespace, **do not** call `SetNewValue`; `tempValue` stays at the default and COLL-02's empty-not-null `nullResult` produces the empty collection. This avoids the `AsArray` split of a whitespace string yielding a single non-empty `[" "]` element (`Split(..., RemoveEmptyEntries)` does not drop a whitespace-only token). Guard with `string.IsNullOrWhiteSpace`. `[VERIFIED: CollectionTypeConverter.cs:47 uses RemoveEmptyEntries but whitespace token survives]` +- **Root / `[SettingsSection]` prefixing honored:** the section is resolved through the existing `_sections.GetOrAdd(...ResolveSection...)` path (`ConfigurationBinder.cs:30,38-41`), which already applies `RootSection`. `GetSection(key).GetChildren()` is a child of that resolved section, so prefixing is inherited for free. `[VERIFIED: ConfigurationBinder.cs:38-41]` + +**Example (illustrative):** +```csharp +// Source: derived from ConfigurationBinder.cs:26-36 (existing scalar read) +public void BindPropertySettings(BindingContext context) +{ + var section = _sections.GetOrAdd(context.Section, + static (name, self) => self.ResolveSection(name), this); + + if (context.PropertyType.IsCollectionShape()) + { + var children = section.GetSection(context.Key).GetChildren(); + var values = children.Select(c => c.Value).Where(v => v != null).ToArray(); + if (values.Length > 0) { context.SetNewValue(values); return; } + // fall through to scalar; if scalar is null/whitespace, do NOT set (COLL-02 empty result) + } + + var value = section[context.Key]; + if (context.PropertyType.IsCollectionShape() && string.IsNullOrWhiteSpace(value)) return; + if (value != null) context.SetNewValue(value); +} +``` +`context.PropertyType` is available on `BindingContext` (`BindingContext.cs:13,35`). `[VERIFIED: BindingContext.cs:13,35]` + +### Pattern 3: Synchronous validation pipeline (VAL-01) + +**What:** Complete the held `Validations/*` scaffolding and run validators after the populate loop. + +**Interface redesign (D-07):** +```csharp +// ISettingsValidator.cs / ISettingValidation.cs — drop Task<> +public interface ISettingsValidator { ValidationResult Validate(ValidationContext context); } +public interface ISettingValidation : ISettingsValidator { ValidationResult Validate(ValidationContext context); } +``` +Current signatures return `Task` and are never invoked (`ISettingValidation.cs:5`, `ISettingsValidator.cs:5`) — dead scaffolding, so the signature change is free pre-beta. `[VERIFIED: ISettingValidation.cs:3-6, ISettingsValidator.cs:3-6]` + +**Context ctor (D-08):** `ValidationContext.Settings` is getter-only with no ctor (`ValidationContext.cs:5`); `ValidationContext` shadows it with `new T? Settings` (`ValidationContextOfT.cs:5`) — neither can carry a value. Add constructors so the populated instance is passed in (e.g. `ValidationContext(object settings)` and `ValidationContext(T settings)`). `ValidationResult` / `ValidationError` are already complete (`ValidationResult.cs`, `ValidationError.cs`). `[VERIFIED: ValidationContext.cs:1-7, ValidationContextOfT.cs:1-6, ValidationResult.cs, ValidationError.cs]` + +**Hook point (D-09):** after the `foreach (var propertyPlan in plan.Properties)` loop in `ValuesPopulator.PopulateInstanceWithValues` (`ValuesPopulator.cs:38-64`), i.e. once every property is set (cross-property rules need the full instance). Collect validators, run all, aggregate errors, throw once. `[VERIFIED: ValuesPopulator.cs:29-65]` + +**Discovery — core path (always, via `Activator`, D-11):** +- **Object-level:** a NEW attribute (e.g. `[SettingsValidator(typeof(X))]`, `AttributeTargets.Interface`) on the settings interface; `X` implements `ISettingValidation`; instantiate via `Activator.CreateInstance` (parameterless ctor); pass `new ValidationContext(instance)`. +- **Property-level:** `[SettingsProperty(ValidatorType=typeof(X))]` (`SettingsPropertyAttribute.cs:14`) — runs on **that property's value**. This is a real contract ambiguity: the held interfaces are typed to the *settings* type, not the property value. See Open Questions Q2 — the planner must define whether a property-level validator implements `ISettingValidation` (receiving the property value) or validates the settings object. Both attribute hooks already have a natural read point: the object-level attribute at the type, the property-level attribute is already read per property at plan build (`ValuesPopulator.cs:84`) and can be threaded into the `PropertyPlan`. + +**Discovery — DI path (`AddSimpleSettings`, D-11):** additionally resolve registered `ISettingValidation` from the container. **This is the phase's one genuine unknown — see Open Questions Q3.** + +**Aggregate exception (D-10):** new `SettingsValidationException : SimpleSettingsException` carrying the full `IReadOnlyList`. Follow the existing base-ctor pattern (`SimpleSettingsException.cs:10-18` — message + optional inner; no serialization ctor). Message built via a `Resources` factory (`Resources.cs` convention). `[VERIFIED: SimpleSettingsException.cs:8-19; Resources.cs pattern]` + +**Redaction (D-12):** `SettingsValidationException` composes its message only from author-supplied `ValidationError.ErrorMessage`; it adds no bound values. Document that validator authors own not echoing secrets. A regression test asserts the aggregate `ToString()` contains only the author messages (no injected values). + +### Pattern 4: Empty-string rejection at conversion (VAL-02) + +**What:** Extend `PropertyConversion.Convert` (`PropertyConversion.cs:29-40`). Today it throws `SettingsPropertyNullException` only when `value == null` and `_throwOnNull`. Add: when `_throwOnNull` and the value is a null-or-whitespace string, throw the same value-free exception before converting. + +```csharp +// Source: PropertyConversion.cs:29-40 (extended) +public object? Convert(object? value) +{ + if (value is null || (value is string s && string.IsNullOrWhiteSpace(s))) + { + if (_throwOnNull) throw new SettingsPropertyNullException(_propertyName); // or new empty-specific sibling + if (value is null) return _nullResult; + // whitespace string with AllowEmpty=true: fall through to normal conversion + } + return _converter.Convert(value, _strippedType); +} +``` +`_throwOnNull` is resolved at plan build from `AllowEmpty` (`TypeConverter.cs:16` — `attribute is { AllowEmpty: false }`). `[VERIFIED: PropertyConversion.cs:29-40; TypeConverter.cs:16]` + +**CRITICAL integration detail — the redaction exception-filter:** `ValuesPopulator.ConvertPropertyValue` wraps conversion failures in the redacting `SettingsPropertyValueException`, **except** `SettingsPropertyNullException`, via the filter `catch (Exception e) when (e is not SettingsPropertyNullException)` (`ValuesPopulator.cs:122`). If VAL-02 reuses `SettingsPropertyNullException`, it propagates cleanly. **If the planner introduces a new `SettingsPropertyEmptyException`, the filter at `ValuesPopulator.cs:122` MUST be widened** to `when e is not (SettingsPropertyNullException or SettingsPropertyEmptyException)` — otherwise the clear empty-value message gets redacted into a generic conversion error. An empty/whitespace value is not itself sensitive, but keep the exception value-free regardless. `[VERIFIED: ValuesPopulator.cs:114-128]` + +**Naming note:** `_throwOnNull` becomes a slight misnomer (now also throws-on-empty). Renaming to e.g. `_rejectEmpty` is optional cleanup (internal struct field; no public surface). `[VERIFIED: PropertyConversion.cs:12]` + +### Pattern 5: Expose `ISettingsCollection` (API-02) + +**What:** In `IntegrateSimpleSettings` (`ServicesSettingsBuilderExtensions.cs:21-44`), add `services.AddSingleton(settingsCollection)` (the collection already exists at `:32`), and add a public entry point that returns the collection. + +**Real C# constraint:** you **cannot** overload `AddSimpleSettings` by return type alone. The two existing overloads differ by parameters (`(services)` vs `(services, action)`) and both return `IServiceCollection` (`:7-19`). A return-`ISettingsCollection` variant needs a distinct signature. Options (planner picks, D-15 says decide on-branch): +1. **`out` parameter overload** — `AddSimpleSettings(this IServiceCollection services, out ISettingsCollection settings, Action<...>? action = null)` returning `IServiceCollection` (keeps chaining). +2. **Distinctly-named method** — e.g. `AddSimpleSettingsCollection(...)` returning `ISettingsCollection`. +Refactor `IntegrateSimpleSettings` to **return** the built `ISettingsCollection` (currently `void`, `:21`) so both public shapes can choose what to surface. Keep the existing `IServiceCollection`-returning overloads intact for the fluent chain. `[VERIFIED: ServicesSettingsBuilderExtensions.cs:7-44]` + +`ISettingsCollection` is already `public` (`ISettingsCollection.cs:3`); `SettingsCollection` is `internal` (`SettingsCollection.cs:6`) and stays internal — DI registration uses the interface, so no visibility change needed. `[VERIFIED: ISettingsCollection.cs:3, SettingsCollection.cs:6]` + +### Anti-Patterns to Avoid +- **Widening `IsEnumerable` in place** to cover `List` — breaks `EnumerableTypeConverter` (returns array, unassignable to `List`) and the pinned enumerable-returns-array test. Use a separate predicate. +- **Reordering the converter chain** — user-first / `DefaultTypeConverter`-last ordering is load-bearing (CONTEXT §Established Patterns). Insert the new converter with a disjoint predicate; never move existing entries. +- **Running DI-resolved validators inside `PopulateInstanceWithValues` during `ScanAssemblies`** — the container does not exist yet at registration time (see Q3). +- **Letting a new VAL-02 exception fall through the redaction filter unlisted** — silently redacts the empty-value diagnostic. +- **Putting any bound value into `SettingsValidationException` or a COLL-03 exception message** — violates S1. + +## Don't Hand-Roll + +| Problem | Don't Build | Use Instead | Why | +|---------|-------------|-------------|-----| +| Split delimited scalar → typed array | A new split/convert loop | Existing `CollectionTypeConverter` array-build path (`:25-38`) | Already de-reflected + benchmark-tuned in P4; reuse via the `List` wrap | +| Read config sequence | Manual index probing (`section[key:0]`, `key:1`...) | `IConfigurationSection.GetChildren()` | Standard config API; handles arbitrary length + provider layering | +| Empty collection instance | `new T[0]` literals scattered | `Array.CreateInstance(elementType, 0)` (already the P4 pattern in `CreateNullResult`) | One materialization site, element-type driven | +| Aggregate multiple validation failures | Ad-hoc `List` | `ValidationResult` (already has `AddError`/`Errors`/`IsValid`) + one `SettingsValidationException` | Scaffolding is complete; only interface async→sync + a ctor remain | +| Catchable error family | `throw new Exception(...)` | `SimpleSettingsException` subtype | Enforced by the existing hierarchy invariant test (`ExceptionHierarchyTests.cs`) | + +**Key insight:** Nearly every "new" capability is a small edit at an existing seam plus reuse of a P4-optimized path. The failure mode in this phase is not missing infrastructure but breaking one of the four invariants (converter order, binder precedence, S1 redaction, plan cache) while wiring the edits. + +## Runtime State Inventory + +Not a rename/refactor/migration phase — greenfield feature edits to a library with no stored/registered runtime state. Omitted per the section trigger. + +## Common Pitfalls + +### Pitfall 1: `List` claimed by the wrong converter +**What goes wrong:** After adding list support, `List` binds to a `T[]` and `SetValue` throws, or `IEnumerable` starts returning a `List` and breaks the pinned test. +**Why it happens:** Widening the shared `IsEnumerable` predicate instead of adding a disjoint one; or placing `ListTypeConverter` where its predicate overlaps `EnumerableTypeConverter`. +**How to avoid:** Separate `IsListLike` predicate; keep `IsEnumerable` matching only the open `IEnumerable<>`; verify the three collection tests plus a new `IEnumerable`-still-returns-array assertion. +**Warning signs:** `ArgumentException: object of type 'T[]' cannot be converted to type 'List`1'` at `PropertyInfo.SetValue`. + +### Pitfall 2: Whitespace scalar produces `[" "]` instead of empty +**What goes wrong:** A collection property bound to a whitespace-only scalar yields a one-element collection, violating D-05 "whitespace → empty". +**Why it happens:** `AsArray` splits with `RemoveEmptyEntries`, which drops empty tokens but keeps whitespace tokens. +**How to avoid:** Guard `string.IsNullOrWhiteSpace(value)` in the binder for collection targets and skip `SetNewValue`, letting COLL-02's empty `nullResult` apply. +**Warning signs:** A collection with `Count == 1` and a blank element in the empty-input test. + +### Pitfall 3: VAL-02 exception silently redacted +**What goes wrong:** A dedicated empty-value exception is caught by the redaction wrapper and re-thrown as a generic `SettingsPropertyValueException`, losing the "empty not allowed" diagnostic. +**Why it happens:** The exception-filter at `ValuesPopulator.cs:122` only excludes `SettingsPropertyNullException`. +**How to avoid:** Either reuse `SettingsPropertyNullException` (already excluded) or widen the filter to include the new type. +**Warning signs:** Test expecting the empty-specific exception type catches `SettingsPropertyValueException` instead. + +### Pitfall 4: DI validators run before their dependencies are registered +**What goes wrong:** A DI-resolved `ISettingValidation` that injects an app service throws or resolves a half-built graph, because `AddSimpleSettings` builds and validates settings at *registration* time. +**Why it happens:** `IntegrateSimpleSettings` calls `ScanAssemblies` → `PopulateInstanceWithValues` eagerly at `:32`, before `BuildServiceProvider` and before the consumer finishes registering services. +**How to avoid:** Defer DI-resolved validators to a post-provider-build step (see Q3). Core-path (`Activator`, parameterless) validators are safe to run inline. +**Warning signs:** `InvalidOperationException: no service for type ...` during `AddSimpleSettings`, or validators that never fire. + +### Pitfall 5: Benchmark regression on the collection hot path +**What goes wrong:** The `List` wrap adds an allocation that trips the BenchmarkDotNet allocation gate (PERF-02). +**Why it happens:** `new List(array)` copies the backing store. +**How to avoid:** Keep array/`IEnumerable` paths byte-for-byte unchanged (the wrap only runs for list-shaped targets, which previously *threw*). Confirm the benchmark job stays green; a net-new allocation for a previously-failing path is acceptable, a regression on existing paths is not. +**Warning signs:** `benchmark.yml` flags allocated-bytes increase on `ValuesPopulator`/array converter benchmarks. + +## Code Examples + +All illustrative examples are inlined in Patterns 1–5 above, each cited to the exact source lines they derive from. No external documentation examples are needed for this phase. + +## State of the Art + +| Old Approach | Current Approach | When Changed | Impact | +|--------------|------------------|--------------|--------| +| `Enumerable.Empty()` via reflected `GetMethod("Empty")` | `Array.CreateInstance(elementType, 0)` | P4 | COLL-02 follows the same pattern for arrays/lists | +| Per-converter `List` + reflected `ToArray` | Shared `CollectionTypeConverter` array build | P4 | COLL-01 wraps the shared array; no per-converter duplication | +| Value-bearing conversion exceptions | Value-free `SettingsPropertyValueException` (takes failure `Type`, no inner) | S1/C2 (#27/#28) | COLL-03 + VAL-01/02 must preserve this; do not chain value-bearing inners | +| Held async validator scaffolding (`Task`) | Synchronous `ValidationResult` (this phase) | Phase 4 (D-07) | Free change — interfaces never invoked | + +**Deprecated/outdated:** +- `origin/validate-settings` branch and its `Core/SettingsOptionsValidator` — pre-modernization fossil; **do not port** (CONTEXT §specifics). VAL-01 is greenfield. +- Client doc pointer `TypeConverter.ValidateNullAcceptance` — **stale**, method no longer exists; VAL-02's hook is `PropertyConversion.Convert`. + +## Assumptions Log + +| # | Claim | Section | Risk if Wrong | +|---|-------|---------|---------------| +| A1 | `new List(builtArray)` is the cheapest correct materialization satisfying all five list shapes | Pattern 1 | If a benchmark regresses, planner switches to direct list build; behavior identical either way | +| A2 | Making `CollectionTypeConverter.Convert` virtual (or extracting a protected helper) is a clean internal change | Pattern 1 | Low — internal type, `InternalsVisibleTo` covers tests | +| A3 | Property-level `[SettingsProperty(ValidatorType)]` validates the property value (not the whole settings object) | Pattern 3 / Q2 | Medium — wrong contract shape means rework of the property-validator path; needs planner decision | +| A4 | Deferring DI-resolved validators to a post-build step preserves the aggregate + redaction contract | Pattern 3 / Q3 | High — this is the phase's critical unknown; architect + security-auditor must sign off | +| A5 | An empty/whitespace config value is not itself a secret, so VAL-02's exception may name the property | Pattern 4 | Low — exception is kept value-free regardless | + +## Open Questions (RESOLVED) + +> All four resolved during planning (plan set `6db217a`): Q1 → 04-01 (wrap the array-build path), Q2 → 04-03 (`ISettingValidation` for property-level), Q3 → 04-04 (deferred post-provider-build DI-validator runner), Q4 → 04-04/04-05 (`out ISettingsCollection` overload). The recommendations below are now locked by the cited plans. + +1. **`List` materialization: wrap vs. direct build (CONTEXT research flag).** — **RESOLVED → 04-01 (wrap).** + - What we know: reuse-the-array wrap is simplest and honors D-01's "reuse the array-build path"; direct build saves one copy. + - What's unclear: whether the wrap trips PERF-02 for list-shaped targets. + - Recommendation: implement the wrap; run `benchmark.yml` locally/CI; only switch to direct build if a *regression on existing paths* appears (a new allocation on a previously-throwing path is expected, not a regression). + +2. **Property-level validator contract (VAL-01, D-11).** + - What we know: `[SettingsProperty(ValidatorType=...)]` "runs on that property's value"; the held interfaces are typed to the settings `T`. + - What's unclear: does a property-level validator implement `ISettingValidation` (context carries the property value) or validate the whole settings object post-populate? + - Recommendation: property-level `ValidatorType` implements `ISettingValidation` and receives `ValidationContext` carrying the property value; object-level `[SettingsValidator]` implements `ISettingValidation`. Planner to lock this shape. + +3. **DI-resolved validator timing/ordering (CONTEXT CRITICAL UNKNOWN, D-11).** + - What we know: `IntegrateSimpleSettings` builds + would-validate settings at registration (`:32`), before `BuildServiceProvider` and before consumer services are all registered. Core-path (`Activator`, parameterless) validators are safe inline; DI-resolved validators are not. + - What's unclear: the mechanism to run service-dependent validators after the container exists while keeping the aggregate + redaction contract byte-identical to the core path. + - Recommendation (for planner + architect + security-auditor): run **core-path validators inline** in `ValuesPopulator` (covers `SettingsBuilder` and DI). For **DI-resolved** validators, defer to a post-provider-build step registered by `AddSimpleSettings` — e.g. a small `IStartupValidator`/`IHostedService` that resolves `ISettingValidation` per settings type and throws the **same** `SettingsValidationException`. Verify the GenericHost package may reference the needed hosting abstraction; if not, fall back to a resolvable `ISimpleSettingsValidationRunner` the host invokes, or restrict DI-resolved validators to dependency-free ones. Gate at plan-review. + +4. **API-02 return-overload shape (D-15).** + - What we know: cannot overload by return type; two candidate shapes (`out` param vs. distinct method name). + - Recommendation: implement the DI-singleton registration unconditionally; add an `out ISettingsCollection` overload (keeps chaining) as the default return-value mechanism; final shape decided on-branch (reversal free pre-beta). + +## Environment Availability + +| Dependency | Required By | Available | Version | Fallback | +|------------|------------|-----------|---------|----------| +| .NET SDK (net10) | build + test (net10 runtime locally) | ✓ | 10.0.301 | — | +| .NET 8 runtime | net8 test execution | ✗ locally (build-only) | — | CI runs net8; build both TFMs locally, run net8 in CI | +| TUnit / Microsoft.Testing.Platform | all tests | ✓ | 1.58.0 | — | +| `Microsoft.Extensions.Configuration` | COLL-03 `GetChildren()` | ✓ | 8.0.0 / 10.0.9 | — | +| `Microsoft.Extensions.DependencyInjection` | API-02 / VAL-01 DI path | ✓ | 8.0.x / 10.0.9 | — | + +**Missing dependencies with no fallback:** none. +**Missing dependencies with fallback:** net8 runtime not installed locally — build targets both TFMs (`net8.0;net10.0`), tests run on net10 locally and net8 in CI. `[VERIFIED: dotnet --list-sdks → only 10.0.301; TESTING.md CI matrix]` + +**TUnit invocation gotcha (VERIFIED via STATE.md + TESTING.md):** `dotnet test --filter` returns 0 tests / exit 5 on Microsoft.Testing.Platform. Use `--treenode-filter "/*/*/ClassNameTests/*"` or run unfiltered. Build + test from `src/`. + +## Validation Architecture + +Nyquist validation is **enabled** (`config.json → workflow.nyquist_validation: true`). Test framework is established; every requirement maps to automatable TUnit tests extending existing files. + +### Test Framework +| Property | Value | +|----------|-------| +| Framework | TUnit 1.58.0 on Microsoft.Testing.Platform | +| Config file | `src/global.json` (`"test": { "runner": "Microsoft.Testing.Platform" }`) | +| Quick run command | `cd src && dotnet test "$SOLUTION" -c Release --no-build --treenode-filter "/*/*/CollectionConversionTests/*"` | +| Full suite command | `cd src && dotnet restore && dotnet build -c Release --no-restore && dotnet test -c Release --no-build` | + +### Phase Requirements → Test Map +| Req ID | Behavior | Test Type | Automated Command | File Exists? | +|--------|----------|-----------|-------------------|-------------| +| COLL-01 | Delimited scalar → `List`/`IList`/`ICollection`/`IReadOnlyList`/`IReadOnlyCollection` all materialize a `List` | unit | `--treenode-filter "/*/*/CollectionConversionTests/*"` | ✅ extend `Conversion/CollectionConversionTests.cs` | +| COLL-01 | `IEnumerable` still returns a `T[]` (no regression) | unit | same | ✅ already pinned (`Convert_ToIntEnumerable_MaterializesAnArray`) — keep green | +| COLL-02 | Unbound `int[]` → empty array, not null | unit | `--treenode-filter "/*/*/TypeConverterTests/*"` | ✅ extend `Core/TypeConverterTests.cs` | +| COLL-02 | Unbound `List` → empty list, not null | unit | same | ❌ Wave 0 (add cases) | +| COLL-03 | Child-section sequence binds to `string[]`/`int[]`/`List` | integration | `--treenode-filter "/*/*/SettingsBuilderConfigurationBinderIntegrationTests/*"` | ✅ extend `ConfigBuilderConfigurationBinderIntegrationTests.cs` | +| COLL-03 | Children win when scalar + children both present | integration | same | ❌ Wave 0 | +| COLL-03 | Comma-scalar still binds (regression guard) | integration | same | ✅ existing comma path — add explicit assertion | +| COLL-03 | Empty / whitespace / empty-sequence → empty, never `[""]` | integration | same | ❌ Wave 0 | +| COLL-03 | `[SettingsSection]` / root prefix honored on sequence | integration | same | ❌ Wave 0 | +| **COLL-03 / S1** | **Secret in a sequence element absent from full `ex.ToString()` on convert failure** | unit | `--treenode-filter "/*/*/ExceptionRedactionTests/*"` | ✅ extend `Conversion/ExceptionRedactionTests.cs` | +| VAL-01 | Object-level `[SettingsValidator]` runs; failing validator throws `SettingsValidationException` | unit | `--treenode-filter "/*/*/*Validation*/*"` | ❌ Wave 0 (new test class) | +| VAL-01 | Property-level `ValidatorType` runs on the property value | unit | same | ❌ Wave 0 | +| VAL-01 | Multiple failures aggregate into one exception's error list | unit | same | ❌ Wave 0 | +| VAL-01 | Cross-property rule validates against the fully-populated instance | unit | same | ❌ Wave 0 | +| VAL-01 | DI-resolved `ISettingValidation` runs (per Q3 mechanism) | integration | `--treenode-filter "/*/*/AddSimpleSettingsIntegrationTests/*"` | ❌ Wave 0 (extend DI tests) | +| VAL-01 / D-12 | `SettingsValidationException.ToString()` contains only author messages, no injected values | unit | same as VAL-01 | ❌ Wave 0 | +| VAL-02 | `AllowEmpty=false` rejects `""` and whitespace (not just null); value-free exception | unit | `--treenode-filter "/*/*/TypeConverterTests/*"` or `SettingsPropertyTests` | ✅ extend `Core/TypeConverterTests.cs` / `SimpleSettings/SettingsPropertyTests.cs` | +| VAL-02 | `AllowEmpty=true` still accepts empty/whitespace | unit | same | ❌ Wave 0 | +| API-02 | `ISettingsCollection` resolvable via `GetRequiredService` after `AddSimpleSettings` | integration | `--treenode-filter "/*/*/AddSimpleSettingsIntegrationTests/*"` | ✅ extend `DependencyInjection/AddSimpleSettingsIntegrationTests.cs` | +| API-02 | Return-value overload yields the same collection; `IServiceCollection` chaining preserved | integration | same | ❌ Wave 0 | + +### Sampling Rate +- **Per task commit:** the `--treenode-filter` quick run for the touched area. +- **Per wave merge:** full suite on net10 locally (`dotnet test -c Release --no-build`). +- **Phase gate:** full suite green on net8 + net10 (CI) plus `benchmark.yml` allocation gate green before `/gsd-verify-work`. + +### Wave 0 Gaps +- [ ] `Conversion/CollectionConversionTests.cs` — add `List` family + `IEnumerable`-still-array cases (COLL-01) +- [ ] `Core/TypeConverterTests.cs` — empty-not-null for array + `List` (COLL-02); empty/whitespace rejection (VAL-02) +- [ ] `ConfigBuilderConfigurationBinderIntegrationTests.cs` — sequence bind, children-win, comma-scalar guard, empty→empty, prefix (COLL-03) +- [ ] `Conversion/ExceptionRedactionTests.cs` — **S1 sequence-element redaction** regression (COLL-03 gate) +- [ ] New `SimpleSettings/SettingsValidationTests.cs` (or similar) — object-level, property-level, aggregate, cross-property, redaction (VAL-01) +- [ ] `DependencyInjection/AddSimpleSettingsIntegrationTests.cs` — DI-resolved validator (Q3), `ISettingsCollection` resolution + return overload (VAL-01/API-02) +- [ ] Test fixtures: new settings interfaces with `[SettingsValidator]` / `ValidatorType` / list-shaped + sequence-backed properties (co-locate per TESTING.md conventions) + +## Security Domain + +`security_enforcement: true`, `security_asvs_level: 1`, `security_block_on: high`. + +### Applicable ASVS Categories +| ASVS Category | Applies | Standard Control | +|---------------|---------|-----------------| +| V2 Authentication | no | Library binds config; no auth surface | +| V3 Session Management | no | — | +| V4 Access Control | no | — | +| V5 Input Validation | yes | VAL-01 (settings validation) + VAL-02 (empty rejection); config values validated at bind via the converter chain and validators | +| V6 Cryptography | no | No crypto; never hand-rolled | +| V7 Error Handling & Logging | **yes (primary)** | S1 secret-redaction invariant — exceptions carry no bound values, chain no value-bearing inners; `SettingsPropertyValueException` takes a failure `Type`, not the exception | +| V8 Data Protection | yes | Secret values in config must not leak into logs via `Exception.ToString()`/`ILogger` (the reason the library logs nothing itself) | + +### Known Threat Patterns for this stack +| Pattern | STRIDE | Standard Mitigation | +|---------|--------|---------------------| +| Secret in a YAML/child-sequence element leaks through a bind/convert failure (COLL-03) | Information Disclosure | Convert failures surface as value-free `SettingsPropertyValueException` (`ValuesPopulator.cs:114-128`); `SettingsBindingException` retains only primitives (binder type/section/key), never `BindingContext` (`SettingsBindingException.cs:5-16`). New regression test proves the sentinel is absent from the whole `ex.ToString()` chain. | +| Validator error message echoes a secret value (VAL-01) | Information Disclosure | `SettingsValidationException` adds no bound values; document validator-author responsibility (D-12); test asserts only author messages appear. | +| VAL-02 empty-value exception embeds the value | Information Disclosure | Exception is value-free (property name only), reusing `SettingsPropertyNullException`'s value-free message contract. | +| Custom `ISectionBinder`/`ISettingsTypeConverter` leaks a value in its own message | Information Disclosure | Wrapper drops the inner and keeps only its `Type` name; already covered by `ExceptionRedactionTests` (`Convert_CustomConverterLeakingValue_IsStillRedacted`) — extend to the sequence path. | + +**Security gate (D-06):** the S1 sequence-element redaction regression test is mandatory and owned by `security-auditor` in the plan-review panel. Blocking severity is `high`. + +## Sources + +### Primary (HIGH confidence) +- Codebase source (read this session, cited inline with file:line): `TypeConverter.cs`, `TypeExtensions.cs`, `CollectionTypeConverter.cs`, `ArrayTypeConverter.cs`, `EnumerableTypeConverter.cs`, `TypeConvertersCollections.cs`, `DefaultTypeConverter.cs`, `ConfigurationBinder.cs`, `ValuesPopulator.cs`, `PropertyConversion.cs`, `SettingsPropertyAttribute.cs`, `Validations/*`, `ServicesSettingsBuilderExtensions.cs`, `ISettingsCollection.cs`, `SettingsCollection.cs`, `BindingContext.cs`, `SettingsBuilder.cs`, `SettingsPlan.cs`, `SettingsOptions.cs`, `SimpleSettingsException.cs`, `SettingsPropertyValueException.cs`, `SettingsPropertyNullException.cs`, `SettingsBindingException.cs`, `Resources.cs`, `Info.cs`. +- Existing tests (patterns to extend): `Conversion/CollectionConversionTests.cs`, `Conversion/ExceptionRedactionTests.cs`, `DependencyInjection/AddSimpleSettingsIntegrationTests.cs`, `ConfigBuilderConfigurationBinderIntegrationTests.cs`. +- Planning artifacts: `04-CONTEXT.md` (D-01..D-15), `REQUIREMENTS.md`, `STATE.md`, `.planning/codebase/CONVENTIONS.md`, `.planning/codebase/TESTING.md`, `.planning/config.json`. +- Environment: `dotnet --list-sdks` → 10.0.301; `src/Directory.Packages.props` (pinned versions). + +### Secondary (MEDIUM confidence) +- dotnet-claude-kit `modern-csharp` skill — C# 14 idioms (primary ctors, collection expressions, pattern matching). **Constraint applied:** `field` keyword and extension members are C# 14 (net10) only; `LangVersion` is unset per-TFM (net8 → C# 12), so those two features MUST NOT appear in shared library code. Collection expressions `[]` and primary constructors are C# 12 and safe on net8. +- dotnet-claude-kit `testing` skill — AAA, test-behavior-not-implementation, `Method_State_Expected` naming, test-data builders. Adapted to this project's TUnit + hand-written in-memory fakes (no xUnit/WebApplicationFactory/Testcontainers here). + +### Tertiary (LOW confidence) +- None. This phase required no web research; all findings are codebase-verified. + +## Metadata + +**Confidence breakdown:** +- Standard stack: HIGH — no new packages; all versions verified against the pinned central props. +- Architecture / integration points: HIGH — every seam read at file:line; invariants confirmed. +- Pitfalls: HIGH — derived directly from the read code (converter predicates, exception filter, DI timing). +- DI-validator timing (Q3): MEDIUM — mechanism is a planner decision; the constraint (build-at-registration) is verified, the resolution is recommended not locked. + +**Research date:** 2026-07-15 +**Valid until:** 2026-08-14 (stable internal domain; re-check only if the converter chain, `ValuesPopulator`, or the DI extension is refactored before planning) + + diff --git a/.planning/phases/04-collection-validation-binding/04-REVIEW.md b/.planning/phases/04-collection-validation-binding/04-REVIEW.md new file mode 100644 index 0000000..10e2c7a --- /dev/null +++ b/.planning/phases/04-collection-validation-binding/04-REVIEW.md @@ -0,0 +1,213 @@ +--- +phase: 04-collection-validation-binding +reviewed: 2026-07-15T00:00:00Z +depth: deep +scope: PR #33 (Waves 1-2) production source on gsd/phase-4-collection-validation-binding +files_reviewed: 16 +files_reviewed_list: + - src/Core/ExistForAll.SimpleSettings.Extensions.Binders/ConfigurationBinder.cs + - src/Core/ExistForAll.SimpleSettings/Conversion/ListTypeConverter.cs + - src/Core/ExistForAll.SimpleSettings/Conversion/CollectionTypeConverter.cs + - src/Core/ExistForAll.SimpleSettings/Conversion/PropertyConversion.cs + - src/Core/ExistForAll.SimpleSettings/Conversion/TypeConvertersCollections.cs + - src/Core/ExistForAll.SimpleSettings/Core/Reflection/TypeConverter.cs + - src/Core/ExistForAll.SimpleSettings/Core/Reflection/TypeExtensions.cs + - src/Core/ExistForAll.SimpleSettings/Info.cs + - src/Core/ExistForAll.SimpleSettings/Resources.cs + - src/Core/ExistForAll.SimpleSettings/SettingsPlan.cs + - src/Core/ExistForAll.SimpleSettings/SettingsValidationException.cs + - src/Core/ExistForAll.SimpleSettings/SettingsValidatorAttribute.cs + - src/Core/ExistForAll.SimpleSettings/Validations/ISettingValidation.cs + - src/Core/ExistForAll.SimpleSettings/Validations/ISettingsValidator.cs + - src/Core/ExistForAll.SimpleSettings/Validations/ValidationContext.cs + - src/Core/ExistForAll.SimpleSettings/Validations/ValidationContextOfT.cs + - src/Core/ExistForAll.SimpleSettings/ValuesPopulator.cs +findings: + blocker: 0 + high: 0 + medium: 3 + low: 4 + total: 7 +status: issues_found +--- + +# Phase 4: Collection & Validation Binding — Code Review Report + +**Reviewed:** 2026-07-15 +**Depth:** deep (cross-file: binder → converter chain → populate → exception family) +**Files Reviewed:** 16 production files (tests read for coverage confirmation, not re-reviewed) +**Status:** issues_found + +## Summary + +The phase-4 batch is, on the whole, solid and correct on the happy path. The central library +invariant — config values must never leak into an exception `ToString()` that reaches logs — is +structurally preserved and *well* tested: `SettingsPropertyValueException` still drops both the value +and the inner exception, and `ExceptionRedactionTests` now proves this for the new array **and** +`List` sequence element paths (D-06 security gate). The `List` family conversion, the +empty-not-null defaults (COLL-02), the child-sequence binding (COLL-03), and the validation pipeline +(VAL-01) all work as specified and are covered by focused TUnit tests. The converter chain is disjoint +(`IsEnumerable` = open `IEnumerable<>` only; `IsListLike` = List/IList/ICollection/IReadOnly* families; +arrays separate), so there are no converter-order conflicts. + +**No blockers.** The redaction gate holds. The findings below are latent-correctness and +quality/robustness issues that should be addressed before beta but do not block merge on their own. + +The two findings worth the reviewer's attention: (MED-1) the COLL-03 child-sequence walk enumerates a +**live** config view twice, which is a TOCTOU crash under reload; and (MED-2) empty-string sequence +elements diverge from the comma-scalar path, contradicting D-05. + +## Narrative Findings (AI reviewer) + +### MEDIUM + +#### MED-1: Child-sequence double-enumeration over a live config view is a TOCTOU crash + +**File:** `src/Core/ExistForAll.SimpleSettings.Extensions.Binders/ConfigurationBinder.cs:57-81` +**Issue:** `TrySetChildSequence` walks `childSection.GetChildren()` twice — a count pass (62-66) and a +fill pass (73-77). Per this class's own documented contract (lines 13-18), the cached section is a +**live view**: "indexing the cached section re-reads the providers on each access — a config reload is +still reflected." Each `GetChildren()` call therefore re-queries the providers. If the provider set +changes between the two passes (a reload on another thread — file watcher, Azure App Config, etc.): +- **more** non-null children in pass 2 → `values[index++]` overruns the `count`-sized array → + `IndexOutOfRangeException`; +- **fewer** children in pass 2 → trailing `null` slots remain in `values`, later dereferenced by the + element converter at `CollectionTypeConverter.cs:34` (`source.GetValue(i)!`) as a null element → + `NullReferenceException` / spurious conversion failure. + +Either way the failure is a non-`SimpleSettingsException` crash. Trigger window is narrow (populate is +synchronous startup), but the class explicitly advertises reload-safety, so it is a real latent bug. +**Fix:** enumerate the children **once** into a temporary buffer, then size/fill from that snapshot: +```csharp +private static bool TrySetChildSequence(IConfigurationSection section, BindingContext context) +{ + List? values = null; + foreach (var child in section.GetSection(context.Key).GetChildren()) + { + if (string.IsNullOrEmpty(child.Value)) + continue; + (values ??= new List()).Add(child.Value); + } + + if (values is null) + return false; + + context.SetNewValue(values.ToArray()); + return true; +} +``` + +#### MED-2: Empty-string sequence elements create phantom entries and diverge from the comma-scalar path + +**File:** `src/Core/ExistForAll.SimpleSettings.Extensions.Binders/ConfigurationBinder.cs:64,75` +**Issue:** The element filter is `child.Value != null`, which **keeps** empty-string (`""`) elements. +So the child sequence `[ "1", "", "3" ]` binds to `["1","","3"]`, whereas the equivalent comma scalar +`"1,,3"` binds to `["1","3"]` — `CollectionTypeConverter.AsArray` splits with +`StringSplitOptions.RemoveEmptyEntries` (`CollectionTypeConverter.cs:47`). For a typed collection +(`int[]`, `List`) the phantom `""` element then fails conversion and throws +`SettingsPropertyValueException`, so two representations of the same data behave differently, and a +blank YAML list item becomes a startup crash. This also contradicts D-05 ("empty / whitespace / +empty-sequence → empty, never `[""]`"). +**Fix:** match the scalar path's empty-entry semantics by skipping empty children: +`if (string.IsNullOrEmpty(child.Value)) continue;` (use `IsNullOrWhiteSpace` if the D-05 "whitespace → +empty" rule should also apply to individual elements). Folded into the MED-1 rewrite above. + +#### MED-3: Reflective validator invocation has no failure handling — misuse escapes the exception family and softens redaction + +**File:** `src/Core/ExistForAll.SimpleSettings/ValuesPopulator.cs:99-121` +**Issue:** `InvokeValidator` performs three unguarded reflection steps: +- `Activator.CreateInstance(validatorType)` (101) throws a raw `MissingMethodException` if the declared + validator has no public parameterless constructor. +- `validatorType.GetMethod("Validate", new[] { closedContextType })` (110) returns `null` when the + validator implements `Validate` **explicitly** (explicit interface impls are private) → the `!` + yields a `NullReferenceException` at line 112. +- `validateMethod.Invoke(...)` (112) wraps *any* exception the validator throws in + `TargetInvocationException`. That is (a) **not** a `SimpleSettingsException`, breaking the D-10 + "one catchable family" contract, and (b) `TargetInvocationException.ToString()` chains the inner + exception — if a throwing validator's message echoes `context.Settings` (which may be a secret) it + reaches logs. D-12 assigns echo-responsibility to the validator author, but a value-bearing + exception crossing the library boundary un-redacted is exactly the S1/SEC-01 vector the rest of the + codebase is engineered to close (compare `SettingsPropertyValueException`, which surfaces only the + failure's *type name*). + +**Fix:** wrap the create/resolve/invoke in a try/catch, unwrap `TargetInvocationException` +(`ex.InnerException ?? ex`), guard the null `validateMethod`, and rethrow as a value-free +`SimpleSettingsException` subtype carrying only the validator type name — mirroring +`SettingsPropertyValueException`'s "type identity, never the instance" pattern so the redaction +guarantee stays structural rather than conventional. + +### LOW + +#### LOW-1: New comments violate the project's one-line / no-internal-reference convention + +**File:** multiple — e.g. `Core/Reflection/TypeConverter.cs:28-31`, `ValuesPopulator.cs:71-73,107-109`, +`Conversion/PropertyConversion.cs:37-38`, `Conversion/ListTypeConverter.cs:7-10`, +`Extensions.Binders/ConfigurationBinder.cs:52-56`, `SettingsPlan.cs:47-48` +**Issue:** The batch adds many multi-line comments that reference internal planning IDs meaningless to +an open-source consumer reading the source ("review B-3", "review S-2", "review S-3", "S-6", "B-2", +"S-4/A1", "P2"/"P3"). Project/org convention: "Avoid comments unless necessary, and limit to a maximum +of one line." These are dangling references to artifacts that ship only in `.planning/`. +**Fix:** trim each to a single explanatory line and drop the `review …`/letter-number IDs (keep the +*why*, lose the cross-reference). + +#### LOW-2: VAL-02 whitespace rejection throws `SettingsPropertyNullException` with a "null" message + +**File:** `src/Core/ExistForAll.SimpleSettings/Conversion/PropertyConversion.cs:32-33` +**Issue:** A non-null, whitespace-only string with `AllowEmpty=false` throws +`SettingsPropertyNullException`, whose message (`Resources.PropertyNotAllowNullMessage`) states the +property "does not allow null" — misleading when the value was actually `" "`. D-14 explicitly +permitted a dedicated empty-specific sibling for exactly this reason. +**Fix:** broaden the (value-free) message to cover "null or empty/whitespace", or introduce the +empty-specific exception D-14 allowed for. + +#### LOW-3: Object/nested-element collections silently bind to empty with no diagnostic + +**File:** `src/Core/ExistForAll.SimpleSettings.Extensions.Binders/ConfigurationBinder.cs:57-81` +**Issue:** The walk counts only children whose `Value` is non-null. A `List` (whose +children are sub-sections with `Value == null`) yields count 0 → falls through to the scalar path → +empty collection, with no error. If binding object-typed sequences is out of scope this is acceptable, +but the silent empty result can mask a real misconfiguration. +**Fix:** document the scalar/delimited-only element contract, or detect object-shaped children and +throw a value-free "unsupported element shape" exception rather than binding empty. + +#### LOW-4: Partly dead defensive code in the aggregate-throw path + +**File:** `src/Core/ExistForAll.SimpleSettings/SettingsValidationException.cs:26-33` and +`src/Core/ExistForAll.SimpleSettings/ValuesPopulator.cs:95-96` +**Issue:** The only in-repo caller (`RunValidators`) already null-checks (`if (errors is not null)`) +and reaches `ThrowIfAny` only when at least one error was added, so `ThrowIfAny`'s `errors == null` +guard and `errors.Count == 0` early-return are both unreachable from that path. Harmless — `ThrowIfAny` +is a public entry point intended to be shared with the not-yet-landed DI runner (Plan 04) — but the +redundant caller guard is noise. +**Fix:** drop the `if (errors is not null)` guard in `RunValidators` and call +`SettingsValidationException.ThrowIfAny(errors ?? Array.Empty())` unconditionally, or +leave a one-line note that the guard is a warm-path allocation-avoider (it is not — `errors` is already +non-null here). + +## Verified-safe (adversarial checks that passed) + +- **Redaction (S1/SEC-01/D-06):** `SettingsPropertyValueException` surfaces only type/property/target + metadata and the failure's `Type` (never the instance or value); `ConvertPropertyValue` + (`ValuesPopulator.cs:191-205`) does not chain the inner. `SettingsBindingException` retains only + primitives, not the `BindingContext`. `ExceptionRedactionTests` proves the sentinel is absent from + the full `ToString()` for scalars, custom converters, and first/last elements of both `int[]` and + `List` sequences. Solid. +- **`List` materialization:** `base.Convert` builds an `elementType[]` and `ListTypeConverter` + casts `(TElement[])` where `TElement == elementType` (factory keyed on `elementType`) — cast is + always valid; `new List(source)` copies rather than aliases. Correct for all list-like shapes + (`IList`/`ICollection`/`IReadOnly*` all satisfied by the returned `List`). +- **Empty defaults (COLL-02):** arrays/`IEnumerable` share an immutable length-0 array; the List + family returns a fresh `List` per bind via a baked factory (`_nullResult is Func`) — the + "mutable default must not be shared" concern (B-3) is correctly handled. +- **Ambiguous `Validate` overload (S-2):** `GetMethod("Validate", new[] { closedContextType })` + correctly disambiguates the two overloads `ISettingValidation` forces the author to implement; + `Activator.CreateInstance(closedContextType, target)` correctly wraps the single arg (incl. `null`) + as a one-element params array, not a null args array. +- **`InternalsVisibleTo("…Binders")`:** required and correct — `ConfigurationBinder` now consumes the + internal `IsCollectionShape` extension. + +--- + +_Reviewed: 2026-07-15_ +_Reviewer: Claude (gsd-code-reviewer)_ +_Depth: deep_ diff --git a/.planning/phases/04-collection-validation-binding/04-VALIDATION.md b/.planning/phases/04-collection-validation-binding/04-VALIDATION.md new file mode 100644 index 0000000..c8a136f --- /dev/null +++ b/.planning/phases/04-collection-validation-binding/04-VALIDATION.md @@ -0,0 +1,101 @@ +--- +phase: 4 +slug: collection-validation-binding +status: ready +nyquist_compliant: true +wave_0_complete: false +created: 2026-07-15 +--- + +# Phase 4 — Validation Strategy + +> Per-phase validation contract for feedback sampling during execution. +> Body sourced from `04-RESEARCH.md` § Validation Architecture (lines 427-500). + +--- + +## Test Infrastructure + +| Property | Value | +|----------|-------| +| **Framework** | TUnit 1.58.0 on Microsoft.Testing.Platform | +| **Config file** | `src/global.json` (`"test": { "runner": "Microsoft.Testing.Platform" }`) | +| **Quick run command** | `cd src && dotnet test "$SOLUTION" -c Release --no-build --treenode-filter "/*/*//*"` | +| **Full suite command** | `cd src && dotnet restore && dotnet build -c Release --no-restore && dotnet test -c Release --no-build` | +| **Estimated runtime** | ~30-60 seconds (net10 locally; net8 via CI) | + +> ⚠ TUnit gotcha: `dotnet test --filter "*X*"` exits 5 / zero tests. Use `--treenode-filter` or run unfiltered. Run from `src/`. + +--- + +## Sampling Rate + +- **After every task commit:** the `--treenode-filter` quick run for the touched test class. +- **After every plan wave:** full suite on net10 locally (`dotnet test -c Release --no-build`). +- **Before `/gsd-verify-work`:** full suite green on net8 + net10 (CI) **plus** `benchmark.yml` allocation gate green. +- **Max feedback latency:** ~60 seconds. + +--- + +## Per-Requirement Verification Map + +Task IDs are assigned by the planner; each task's `` verify must map to a row below. + +| Requirement | Behavior | Threat Ref | Test Type | Automated Command (treenode-filter) | File Exists | +|-------------|----------|------------|-----------|-------------------------------------|-------------| +| COLL-01 | Delimited scalar → `List`/`IList`/`ICollection`/`IReadOnlyList`/`IReadOnlyCollection` all materialize a `List` | — | unit | `/*/*/CollectionConversionTests/*` | ✅ extend `Conversion/CollectionConversionTests.cs` | +| COLL-01 | `IEnumerable` still returns a `T[]` (no regression) | — | unit | `/*/*/CollectionConversionTests/*` | ✅ pinned `Convert_ToIntEnumerable_MaterializesAnArray` — keep green | +| COLL-02 | Unbound `int[]` → empty array, not null | — | unit | `/*/*/TypeConverterTests/*` | ✅ extend `Core/TypeConverterTests.cs` | +| COLL-02 | Unbound `List` → empty list, not null | — | unit | `/*/*/TypeConverterTests/*` | ❌ W0 | +| COLL-03 | Child-section sequence binds to `string[]`/`int[]`/`List` | — | integration | `/*/*/SettingsBuilderConfigurationBinderIntegrationTests/*` | ✅ extend `ConfigBuilderConfigurationBinderIntegrationTests.cs` | +| COLL-03 | Children win when scalar + children both present | — | integration | same | ❌ W0 | +| COLL-03 | Comma-scalar still binds (regression guard, prod `MultiHost__CommonHosts`) | — | integration | same | ✅ add explicit assertion | +| COLL-03 | Empty / whitespace / empty-sequence → empty, never `[""]` | — | integration | same | ❌ W0 | +| COLL-03 | `[SettingsSection]` / root prefix honored on sequence | — | integration | same | ❌ W0 | +| **COLL-03 / S1** | **Secret in a sequence element absent from full `ex.ToString()` on convert failure** | **T-04-S1 (D-06)** | unit | `/*/*/ExceptionRedactionTests/*` | ✅ extend `Conversion/ExceptionRedactionTests.cs` | +| VAL-01 | Object-level `[SettingsValidator]` runs; failing validator throws `SettingsValidationException` | — | unit | `/*/*/*Validation*/*` | ❌ W0 (new test class) | +| VAL-01 | Property-level `ValidatorType` runs on the property value | — | unit | same | ❌ W0 | +| VAL-01 | Multiple failures aggregate into one exception's error list | — | unit | same | ❌ W0 | +| VAL-01 | Cross-property rule validates against the fully-populated instance | — | unit | same | ❌ W0 | +| VAL-01 | DI-resolved `ISettingValidation` runs (per Q3 deferred-post-build mechanism) | — | integration | `/*/*/AddSimpleSettingsIntegrationTests/*` | ❌ W0 | +| VAL-01 / D-12 | `SettingsValidationException.ToString()` contains only author messages, no injected values | T-04-VAL (D-12) | unit | `/*/*/*Validation*/*` | ❌ W0 | +| VAL-02 | `AllowEmpty=false` rejects `""` and whitespace (not just null); value-free exception | — | unit | `/*/*/TypeConverterTests/*` or `/*/*/SettingsPropertyTests/*` | ✅ extend `Core/TypeConverterTests.cs` | +| VAL-02 | `AllowEmpty=true` still accepts empty/whitespace | — | unit | same | ❌ W0 | +| API-02 | `ISettingsCollection` resolvable via `GetRequiredService` after `AddSimpleSettings` | — | integration | `/*/*/AddSimpleSettingsIntegrationTests/*` | ✅ extend `DependencyInjection/AddSimpleSettingsIntegrationTests.cs` | +| API-02 | Return-value overload yields the same collection; `IServiceCollection` chaining preserved | — | integration | same | ❌ W0 | + +*Status per task: ⬜ pending · ✅ green · ❌ red · ⚠️ flaky (tracked in each plan's tasks)* + +--- + +## Wave 0 Requirements + +- [ ] `Conversion/CollectionConversionTests.cs` — add `List` family + `IEnumerable`-still-array cases (COLL-01) +- [ ] `Core/TypeConverterTests.cs` — empty-not-null for array + `List` (COLL-02); empty/whitespace rejection (VAL-02) +- [ ] `ConfigBuilderConfigurationBinderIntegrationTests.cs` — sequence bind, children-win, comma-scalar guard, empty→empty, prefix (COLL-03) +- [ ] `Conversion/ExceptionRedactionTests.cs` — **S1 sequence-element redaction** regression (COLL-03 / D-06 gate) +- [ ] New `SimpleSettings/SettingsValidationTests.cs` — object-level, property-level, aggregate, cross-property, redaction (VAL-01) +- [ ] `DependencyInjection/AddSimpleSettingsIntegrationTests.cs` — DI-resolved validator (Q3), `ISettingsCollection` resolution + return overload (VAL-01/API-02) +- [ ] Test fixtures: settings interfaces with `[SettingsValidator]` / `ValidatorType` / list-shaped + sequence-backed properties (co-locate per TESTING.md) + +--- + +## Manual-Only Verifications + +All phase behaviors have automated verification. + +*(The two on-branch decisions — `List` wrap-vs-direct-build and API-02 return-overload shape — are design choices resolved at code-review, not manual test verifications.)* + +--- + +## Validation Sign-Off + +- [x] All tasks have `` verify or Wave 0 dependencies +- [x] Sampling continuity: no 3 consecutive tasks without automated verify +- [x] Wave 0 covers all MISSING (❌) references +- [x] No watch-mode flags +- [x] Feedback latency < 60s +- [x] `nyquist_compliant: true` set in frontmatter +- [ ] `wave_0_complete: true` — flips when the Wave-0 test stubs are authored during execution + +**Approval:** approved 2026-07-15 (validation strategy; `wave_0_complete` flips during execution)