diff --git a/FIX-PLAN.md b/FIX-PLAN.md index d442520..d736bac 100644 --- a/FIX-PLAN.md +++ b/FIX-PLAN.md @@ -9,9 +9,10 @@ _Derived from the 2026-07-10 three-part review (architecture · tests · perform - **P4 merged (#25)** — de-reflect + DRY the array/enumerable converters: shared `CollectionTypeConverter` (`Array.CreateInstance` + indexed fill, manual `LinkedList` walk); `CreateNullResult` de-reflected. Gated `ConvertArrayBenchmark`: **1.33 KB→688 B (−49%), 5.7×**. Suite 68/TFM. Included a user modernization pass (collection expressions across ~19 files). - **P5 merged (#26)** — resolve config section once per type: `ConfigurationBinder` caches the `IConfigurationSection` per section name (`ConcurrentDictionary`, zero-capture `GetOrAdd`). Plan reviewed by architect+perf+security (chose internal cache over a contract change — layering); code reviewed via `/code-review`. Gated `ConfigBinderBenchmark`: **BindNoRoot 80→40 B (−50%), BindWithRoot 144→56 B (−61%)**. Suite **71/TFM**. Also gated P4's `ConvertArrayBenchmark` (was never in the CI filter). **Security review surfaced a pre-existing leak → new item S1.** - **Perf track P0–P5 = COMPLETE + merged.** `master` @ `498fc81`. -- **S1 shipped** (branch `security/s1-redact-exception-value`, PR open) — **full fix (Rec 1)**: conversion-failure exceptions no longer carry the bound value **or** chain the value-bearing framework inner (only the failure's type name is surfaced); the value-free "required value missing" case split into its own `SettingsPropertyNullException` so its useful message survives. Three-specialist plan review (security ENDORSE-WITH-CHANGES; architect/perf in-context — kit agents misfired) + `/code-review` (clean, 0 antipatterns). Suite **76 net10** (was 71; +5 redaction tests). Detail in §S1 below. -- **In flight:** S1 PR open (branch `security/s1-redact-exception-value`) — carries this fix-plan + handoff refresh. -- **Next:** engine tests (T4 `ValuesPopulator` / T5 `TypeConverter` / T7 generator concurrency) **or** batch the breaking cleanups (C2 public exception base / A5 / C1 / A6 / A3 / A4) while still pre-stable · optional P3b (tiered/lazy compiled setter, only if set *time* shows in a profile). +- **S1 shipped + merged (#27).** Conversion-failure exceptions no longer carry the bound value **or** chain the value-bearing framework inner; the value-free "required value missing" case split into its own `SettingsPropertyNullException`. `master` @ `5277c60`. Detail in §S1 below. +- **C2 shipped** (branch `refactor/c2-exception-hierarchy`, PR open) — **public exception hierarchy**: `SimpleSettingsException` base, reparent all 10, promote the 4 escapees to public, flatten 3 to root namespace, leak-safe structured properties, retype the `TypeIsNotInterface` throw. Plan reviewed by `security-auditor` + `dotnet-architect` (both ENDORSE-WITH-CHANGES, both fired cleanly) + perf in-context; code by `/code-review`. Suite **82 net10** (was 76; +6). Detail in §C2 below. +- **In flight:** C2 PR open (branch `refactor/c2-exception-hierarchy`) — carries this fix-plan + handoff refresh. +- **Next:** engine tests (T4 `ValuesPopulator` / T5 `TypeConverter` / T7 generator concurrency race) **or** continue the pre-stable breaking cleanups (A5 make `SettingsHolder` internal / C1 `List` support / A6 command-line quoting / A3 `Core.AspNet` / A4 dependency floor) · **A1 (HIGH)** AOT/trim story · optional P3b. - **C3 — DECIDED (option 2):** cache in the provider only; Core `SettingsBuilder.GetSettings` unchanged; no reload. See #17. - **Held — do NOT delete (feature work coming):** D1 Validations (reconcile with the `validate-settings` branch) · D2 EqualityCompererCreator. - Running status lives in `SESSION-HANDOFF.md`. @@ -44,7 +45,7 @@ _Derived from the 2026-07-10 three-part review (architecture · tests · perform **Phase 3 — Correctness/feature (needs design)** - [ ] C1 · Decide: support `List`/`IList`/`ICollection` or document the `IEnumerable`-only limit · Sev Med · Eff M -- [ ] C2 · Introduce a public `SimpleSettingsException` base; make boundary-crossing exceptions public + structured · Sev Med · Eff M · **Break** +- [x] C2 · Introduce a public `SimpleSettingsException` base; reparent all 10 exceptions, promote the 4 build-path escapees to public, flatten 3 to the root namespace, add leak-safe structured properties, retype the `TypeIsNotInterface` throw · Sev Med · Eff M · **Break** - [x] C3 · Decide reload / provider-vs-singleton semantics (see also P1) · Sev High · Eff M–L · **DECIDED: option 2, provider-level cache (#17)** **Phase 4 — Tests (write-first shortlist, then broaden)** @@ -150,10 +151,14 @@ if (settingsOptions.AttributeType != null && **Fix — decide:** (a) document the limitation and throw a clear error, or (b) broaden `EnumerableTypeConverter.CanConvert` to handle assignable collection interfaces/`List` and return a compatible instance. Coordinate with P4. **Verify:** T6 doc test, upgraded to a positive test if (b). -### C2 · Public exception base + structured data — Sev Med · Eff M · **Break** -**Problem:** 9 exception types derive straight from `Exception` (no common base — can’t `catch (SimpleSettingsException)`); some are `internal` yet escape the public build path (uncatchable by type); context (binder/section/key/type) lives only in message strings; `TypeConverter.cs:62` throws a bare `Exception`. -**Fix:** add `public abstract class SimpleSettingsException : Exception`; reparent all library exceptions; make boundary-crossing ones public; expose context as properties; replace the bare `Exception` (ties into D-work). Add the standard ctor set. -**Verify:** builds; suite green; add a `catch (SimpleSettingsException)` test. +### C2 · Public exception base + structured data — Sev Med · Eff M · **Break** · **DONE (branch `refactor/c2-exception-hierarchy`)** +**Problem:** 10 exception types derived straight from `Exception` (no common base — couldn’t `catch (SimpleSettingsException)`); 4 were `internal` yet escaped the public build path (uncatchable by type); context (binder/section/key/type) lived only in message strings; the reachable not-an-interface guard threw an untyped `InvalidOperationException` (`SettingsCollection.cs:21,31`, `SettingsBuilder.cs:71`). + +**Resolved (this PR).** Added `public abstract class SimpleSettingsException : Exception` (protected `(message)` + `(message, inner)` ctors; no parameterless/`[Serializable]` ctor — BinaryFormatter is obsolete on net8/10). Reparented all 10; promoted the 4 escapees (`SettingsPropertyValueException`, `SettingsPropertyNullException`, `TypeGenerationException`, `SettingsPropertyExtractionException`) to public; **flattened 3 mis-namespaced types** (`SettingsExtractionException` from `.Core`; `TypeGenerationException` + `SettingsPropertyExtractionException` from `.Core.Reflection`) to the root namespace so the public surface is coherent. Exposed leak-safe structured properties (`SettingsBindingException.{BinderType,Section,Key}`, `SettingsPropertyValueException.{SettingsType,PropertyName,TargetType,ConversionErrorType}`, `SettingsType`/`OptionType`/`ArgumentName` on the rest). New `SettingsTypeNotInterfaceException : SimpleSettingsException` replaces the 3 `InvalidOperationException(TypeIsNotInterface)` throws (the one real behavior break — flag in release notes). The two documented-**unreachable** "No converter found" `InvalidOperationException`s (`TypeConverter.cs:57`, `CollectionTypeConverter.cs:68`) were **left** as internal invariant guards. + +**S1 preserved structurally:** `SettingsPropertyValueException`'s ctor now takes the failure's **`Type`** (not the `Exception`), so no value-bearing object crosses its boundary; `SettingsBindingException` stores primitives and **does not retain the `BindingContext`** (which holds the bound value). `InnerException == null` is asserted by test. + +**Reviewed:** plan by `security-auditor` (ENDORSE-WITH-CHANGES — structural S1 hardening) + `dotnet-architect` (ENDORSE-WITH-CHANGES — namespace flattening, `Section`/`ConversionErrorType` naming, "now public" test must use reflection since IVT masks it); code by `/code-review` (high) + Roslyn `detect_antipatterns` (0). **Tests +6** (`SimpleSettings/ExceptionHierarchyTests.cs`): base is public+abstract; a reflection invariant that every library exception derives from the base; the 4 promotions are public (reflection); not-interface→typed+catchable; conversion→structured metadata + `InnerException == null`; binder-throws→context. Suite **82 net10** (was 76). ### C3 · Reload / provider-vs-singleton semantics — Sev High · Eff M–L **Problem:** DI registers startup-built **singletons** (`ServicesSettingsBuilderExtensions.cs:37-42`), but `ISettingsProvider.GetSettings` **re-binds a fresh instance every call** (`SettingsBuilder.cs:99-108`) — so `GetService()` and `provider.GetSettings()` can return different objects/values. No `IOptionsMonitor`-style reload exists (acknowledged by the `// replace this…` TODO at `ServicesSettingsBuilderExtensions.cs:35`). diff --git a/SESSION-HANDOFF.md b/SESSION-HANDOFF.md index 98de55b..41328d7 100644 --- a/SESSION-HANDOFF.md +++ b/SESSION-HANDOFF.md @@ -3,58 +3,59 @@ _Last updated: 2026-07-13 · owner: Guy Ludvig (guy@frontegg.com)_ ## TL;DR -We're working the three-specialist review fix plan (**`FIX-PLAN.md`**, repo root — per-item file:line detail). The perf track **P0–P5 is merged**; **S1 (secret redaction) is now done and in an open PR** on branch **`security/s1-redact-exception-value`** (run `gh pr list` for the number). `master` is still @ `498fc81`. Suite **76 tests net10** (was 71; +5 redaction tests). CI runs net8 + net10. +We're working the three-specialist review fix plan (**`FIX-PLAN.md`**, repo root — per-item file:line detail). Perf track **P0–P5 merged**; **S1 (secret redaction) merged (#27)**; **C2 (public exception hierarchy) is now done and in an open PR** on branch **`refactor/c2-exception-hierarchy`** (run `gh pr list` for the number). `master` @ `5277c60`. Suite **82 tests net10** (CI runs net8 + net10). -**S1 — full fix (Rec 1):** conversion-failure exceptions no longer carry the bound value **or** chain the value-bearing framework inner exception; only the failure's *type name* is surfaced. The value-free "required value missing" case was split into its own `SettingsPropertyNullException` so its useful message survives. The three-specialist plan review picked Rec 1 and **rejected Rec 2** (an opt-in flag to restore value/inner) as insecure-by-configuration. Both changed exceptions are `internal` → non-breaking. +**C2 — what it did:** added `public abstract class SimpleSettingsException : Exception` and reparented all 10 library exceptions (so consumers can `catch (SimpleSettingsException)`); promoted the 4 build-path escapees to public; flattened 3 mis-namespaced types to the root namespace; exposed leak-safe structured properties; and replaced the untyped `InvalidOperationException(TypeIsNotInterface)` with a typed `SettingsTypeNotInterfaceException` (the one real behavior break). **S1's redaction is now structural** — `SettingsPropertyValueException` takes the failure's `Type`, not the `Exception`, so a value-bearing object can't cross its boundary. -Still **pre-stable** (no `v*` tag; only auto-alphas published), so breaking changes remain free — a good window to batch the breaking cleanups (C2 / A5 / C1 / A6 / A3 / A4) before the first `v2.0.0-beta`. +Still **pre-stable** (no `v*` tag; only auto-alphas), so breaking changes remain free — keep batching the breaking cleanups (A5 / C1 / A6 / A3 / A4) before the first `v2.0.0-beta`. ## Do this first (new session) -1. **Verify git state** (`git log`, `gh pr list`) — expect `master` @ `498fc81`, branch **`security/s1-redact-exception-value`** pushed with the S1 commit, and **its PR open**. If the PR already merged, `master` will have advanced — reconcile. -2. **If the S1 PR is still open:** check CI (build+test net8/net10, and the **benchmark** gate — S1 is off the hot path, so allocations shouldn't move). Merge when green (squash), or address review. S1 carries this handoff + `FIX-PLAN.md` refresh in the same commit, so they merge with it. -3. **Then pick next work** (ranked below): engine tests (T4/T5/T7) **or** batch the breaking cleanups (C2 public exception base / A5 / C1 / A6 / A3 / A4) while pre-stable. Per the workflow (project memory `[[dotnet-review-workflow]]`): plan → review the plan with `dotnet-architect`/`performance-analyst`/`security-auditor` → implement → review the diff with the **`/code-review` skill**. ⚠️ **See the sub-agent gotcha** — this session the `dotnet-architect` and `performance-analyst` agents ALSO misfired (0 tool calls, leaked preamble); only `security-auditor` worked. Have the in-context fallback ready. +1. **Verify git state** (`git log`, `gh pr list`) — expect `master` @ `5277c60` (S1 #27 merged), branch **`refactor/c2-exception-hierarchy`** pushed with the C2 commit, and **its PR open**. If C2 already merged, `master` advanced — reconcile. +2. **If the C2 PR is open:** check CI (build+test net8/net10 + the benchmark allocation gate — C2 is off the hot path, so nothing should move). Merge when green (squash, via `guy-lud`). C2 carries this handoff + `FIX-PLAN.md` refresh in the same commit. +3. **Then pick next work** (ranked below). Per the workflow (project memory `[[dotnet-review-workflow]]`): plan → review the plan with `dotnet-architect`/`performance-analyst`/`security-auditor` → implement → review the diff with the **`/code-review` skill**. See the sub-agent gotcha — kit plan-agents are **intermittently** flaky (misfired on S1, fired cleanly on C2); keep the in-context fallback ready. ## Current state -- On branch **`security/s1-redact-exception-value`** with the S1 commit (code + tests + this docs refresh). **PR open** (see `gh pr list`). `master` @ `498fc81` (P5, #26). -- **Perf track P0–P5 complete + merged.** Build clean (0 warnings, both TFMs). Suite **76 net10**. -- A **`gh-pages`** branch holds benchmark data (`dev/bench/`); do **not** delete it — the allocation baseline lives there. The remote also still has merged `perf/p4-*`/`perf/p5-*` branches (optional cleanup) plus legacy/held branches (`validate-settings`, `version-7.x`, pre-#8). Deleting remote branches needs the `guy-lud` push identity. +- On branch **`refactor/c2-exception-hierarchy`** with the C2 commit (code + tests + this docs refresh). **PR open** (see `gh pr list`). `master` @ `5277c60`. +- **Perf track P0–P5, S1, C2 all complete.** Build clean (0 warnings, both TFMs). Suite **82 net10**. +- A **`gh-pages`** branch holds benchmark data (`dev/bench/`); do **not** delete it — the allocation baseline lives there. The remote also still has merged `perf/*`/`security/s1-*` branches (optional cleanup) plus legacy/held branches. Deleting remote branches needs the `guy-lud` push identity. ## What shipped (recent → older) -- **S1 — redact secret values from exception messages (branch `security/s1-redact-exception-value`, PR open).** A bound config value that fails type conversion used to reach logs two ways: (1) our own `SettingsPropertyValueException` message interpolated `[{value}]`, and (2) the failing converter's framework inner exception (`FormatException`/`ArgumentException` for int/enum/DateTime) embeds the raw input and was chained in. **Fix (Rec 1, full):** `Resources.PropertySetterExceptionMessage` rewritten to report property name + **target type** + the inner's **exception type name** (no value, typo `to to` fixed); `SettingsPropertyValueException` ctor drops the `value` param and **no longer chains** the inner — uniform invariant *"never carries a value, never chains an inner"* → auditably leak-proof; both `ValuesPopulator` throw sites updated (`:104` plan-build, `:133` convert). New **`SettingsPropertyNullException`** (internal): the "AllowEmpty=false, no value" path is value-free, so it keeps its full message and `ConvertPropertyValue` rethrows it unredacted (security-review required change — else it degraded to `[Exception]`). `ISectionBinder` doc note added (its wrapper `SettingsBindingException` is `public` and chains the binder's inner → custom binders must not throw value-bearing messages; the 4 built-ins don't). **Tests +5** (`Conversion/ExceptionRedactionTests.cs`): sentinel secret bound to int/enum/DateTime/Uri/hostile-custom-converter, each asserting the secret is absent from the whole `ex.ToString()` chain while property + target type remain; existing null test updated to the new type. **Note:** on modern .NET `UriFormatException` is generic (no URI), so the Uri leak was via *our* message — the Uri test targets that. **Accepted tradeoff:** non-secret misconfigs lose the framework message/stack (get property + target + failure type instead) — flag in release notes. **Reviewed:** plan by the 3 specialists (security ENDORSE-WITH-CHANGES + 3 required changes, all folded in; architect+perf **misfired** → done in-context); code by `/code-review` high effort + Roslyn MCP (0 antipatterns, blast radius = only the 2 throw sites + tests). -- **P5 — resolve config section once per type (merged, #26).** `ConfigurationBinder` caches the resolved `IConfigurationSection` per section name (`ConcurrentDictionary`, zero-capture `GetOrAdd(..., static (name, self) => …, this)`). Internal cache, not a contract change (threading `IConfigurationSection` through Core's `ISectionBinder` would be a layering violation; single-implementer). Reload-safe (live view). **Gated `ConfigBinderBenchmark`: BindNoRoot 80→40 B (−50%), BindWithRoot 144→56 B (−61%).** Also wired P4's `ConvertArrayBenchmark` into the CI filter. **Security review of P5 surfaced the leak → S1.** -- **P4 — de-reflect + DRY the array/enumerable converters (merged, #25).** Shared `Conversion/CollectionTypeConverter` (`Array.CreateInstance` + indexed fill, manual `LinkedList` walk — no boxed enumerator/closure); `ArrayTypeConverter`/`EnumerableTypeConverter` are thin subclasses, both return `T[]`; `CreateNullResult` de-reflected. **Gated `ConvertArrayBenchmark`: 1.33 KB→688 B (−49%), 5.7×.** 12 parity tests. -- **P3 — cached "settings plan" (#24, `faa48d9`).** Per-type `SettingsPlan` cached on the populator: section name once+lazy, `[SettingsProperty]` read once, per-property `readonly struct` `PropertyPlan`/`PropertyConversion` with precomputed converter. **Warm re-populate −55–61%**; gated `ScanBenchmark` ≈flat. Emitted/compiled setter built but **reverted** (regressed the gated cold scan for no warm gain — net10 `SetValue` is alloc-free). Follow-ups: P3b (tiered setter, only if set *time* matters) + binder `CreateKey` string-concat. -- **#23** session-wrap docs · **#22** benchmark-tracking CI (gates PRs on allocation regressions via github-action-benchmark on `gh-pages`) · **#21** perf quick wins Q1–Q4 + M1 collision fix + micro-benchmarks (Q1 2.7× / Q3 2.65× / Q4 32×) · **#20** docs tutorials refresh · **#18** P2 memoize extraction · **#17** P1 provider cache + C3 · **#16** P0 benchmark harness · earlier #8/#10–#15. +- **C2 — public exception hierarchy (branch `refactor/c2-exception-hierarchy`, PR open).** New `public abstract class SimpleSettingsException : Exception` (protected `(message)` + `(message, inner)` ctors; no parameterless/`[Serializable]` ctor). Reparented all 10 exceptions; promoted the 4 escapees (`SettingsPropertyValueException`, `SettingsPropertyNullException`, `TypeGenerationException`, `SettingsPropertyExtractionException`) to **public**; **flattened 3 to root ns** (`SettingsExtractionException` was `.Core`; `TypeGenerationException` + `SettingsPropertyExtractionException` were `.Core.Reflection`). Added leak-safe structured props (`SettingsBindingException.{BinderType,Section,Key}`; `SettingsPropertyValueException.{SettingsType,PropertyName,TargetType,ConversionErrorType}`; `SettingsType`/`OptionType`/`ArgumentName` on the rest). New `SettingsTypeNotInterfaceException` replaces the 3 `InvalidOperationException(TypeIsNotInterface)` throws (the one behavior break → release notes). **Left** the 2 unreachable "No converter found" `InvalidOperationException`s as invariant guards. **S1 made structural:** `SettingsPropertyValueException` ctor takes the failure `Type` (not the `Exception`); `SettingsBindingException` stores primitives, does not retain `BindingContext` (which holds the bound value). **Reviewed:** plan by `security-auditor` + `dotnet-architect` (both ENDORSE-WITH-CHANGES, both fired cleanly this time; perf in-context) → adopted: pass `Type` not `Exception`, flatten namespaces, `Section`/`ConversionErrorType` naming, reflection-based "is public" tests (IVT masks accessibility from a plain reference). Code by `/code-review` (high) + Roslyn `detect_antipatterns` (0). **Tests +6** (`SimpleSettings/ExceptionHierarchyTests.cs`): base public+abstract; reflection invariant that every library exception derives from the base; the 4 promotions are public; not-interface→typed+catchable; conversion→structured metadata + `InnerException == null`; binder-throws→context. +- **S1 — redact secret values from conversion-failure exceptions (merged #27).** Two leak vectors closed: our message no longer interpolates the value, and the value-bearing framework inner is no longer chained. New value-free `SettingsPropertyNullException` for the "AllowEmpty=false, no value" path. `ISectionBinder` doc note (custom binders mustn't throw value-bearing messages). +5 redaction tests. (C2 later made the no-chain guarantee structural.) +- **P5 — resolve config section once per type (merged, #26).** `ConfigurationBinder` caches the `IConfigurationSection` per section name (`ConcurrentDictionary`, zero-capture `GetOrAdd`). **BindNoRoot 80→40 B (−50%), BindWithRoot 144→56 B (−61%).** The P5 security review surfaced S1. +- **P4 — de-reflect + DRY the array/enumerable converters (merged, #25).** Shared `CollectionTypeConverter` (`Array.CreateInstance` + indexed fill, manual `LinkedList` walk). **1.33 KB→688 B (−49%), 5.7×.** +- **P3 — cached "settings plan" (#24).** Per-type `SettingsPlan`; warm re-populate **−55–61%**; gated `ScanBenchmark` ≈flat. Compiled setter reverted (net10 `SetValue` is alloc-free). Follow-up P3b (only if set *time* matters). +- **#23** wrap docs · **#22** benchmark-tracking CI (gates PRs on allocation regressions via `gh-pages`) · **#21** perf quick wins Q1–Q4 + M1 + micro-benchmarks · **#20** docs tutorials · **#18** P2 · **#17** P1+C3 · **#16** P0 harness · earlier #8/#10–#15. ## Key decisions & context (carry forward) -- **Benchmark tracking gates on ALLOCATIONS, not time.** Allocated bytes are deterministic → safe to fail a build on. `gh-pages` (`dev/bench/`) holds the baseline. -- **S1 exception invariant.** `SettingsPropertyValueException` deliberately **never carries the bound value and never chains an inner** (both can embed secrets). `SettingsPropertyNullException` is the distinct value-free "required value missing" case and keeps its full message. Don't reintroduce a chained inner or a value param — that reopens the leak. A future opt-in "full diagnostics" knob was explicitly rejected (insecure-by-configuration). -- **C2 (future) will make exceptions public + structured** (a `SimpleSettingsException` base, reparent all, expose context as properties). S1 stayed minimal on purpose; when C2 lands, give `SettingsPropertyValueException`/`SettingsPropertyNullException` structured props (PropertyName/TargetType/FailureType) and reparent them. -- **M1 / generated names.** The generated impl type name (in `SettingsClassGenerator`) is namespace-qualified and must stay **separate** from `GetNormalizeInterfaceName`, which drives the default config section name. Don't merge them. -- **C3 resolved — option 2 (provider-level cache).** Reload / `IOptionsMonitor` is the future "option 3". +- **Benchmark tracking gates on ALLOCATIONS, not time.** `gh-pages` (`dev/bench/`) holds the baseline. +- **Exception-redaction invariant (S1+C2).** `SettingsPropertyValueException` **never carries the bound value and never chains an inner** — its ctor takes the failure `Type`, not the `Exception`, so this is now structural. `SettingsBindingException` stores primitives (`BinderType`/`Section`/`Key`), never the `BindingContext` (holds `CurrentValue`). `SettingsPropertyNullException` is the distinct value-free "required value missing" case. Don't add a `Value` property, re-chain an inner, or retain a value-bearing object — that reopens the leak. +- **Exception hierarchy (C2 done).** All library exceptions derive from `public abstract SimpleSettingsException` and live in the **root** namespace. A reflection invariant test enforces the "all derive from base" rule — if you add a new exception, derive it from `SimpleSettingsException` or that test fails. The two "No converter found" `InvalidOperationException`s are deliberate unreachable guards (not part of the family). +- **M1 / generated names.** The generated impl type name (in `SettingsClassGenerator`) is namespace-qualified and must stay **separate** from `GetNormalizeInterfaceName` (drives the default config section name). Don't merge. +- **C3 resolved — option 2 (provider-level cache).** Reload/`IOptionsMonitor` is the future "option 3". - **Validations (D1) — HELD, do NOT delete.** Public `Validations/*` + `SettingsPropertyAttribute.ValidatorType` are dead but intended for a feature; reconcile with the `validate-settings` branch. - **`EqualityCompererCreator` (D2) — HELD** (internal, dead, latent invalid-IL bug at `EqualityCompererCreator.cs:38`). - **Pre-stable window:** no `v*` stable tag. Breaking changes free until the first `v2.0.0-beta`. ## Next priorities (ranked — detail in FIX-PLAN.md) -1. **Merge the S1 PR** once CI is green (see Do this first). +1. **Merge the C2 PR** once CI is green (see Do this first). 2. **Engine tests:** T4 `ValuesPopulator` (precedence + exception wrappers), T5 `TypeConverter` (null/nullable/empty-enumerable/attribute), T7 generator concurrency stress — the unsynchronized check-then-`DefineType` in `GenerateType` is **still open** (Q4's `ConcurrentDictionary` made the cache thread-safe but did not close that race). T6 converters largely done across P4+P5. -3. **Breaking cleanups (batch while pre-stable):** C2 (public `SimpleSettingsException` base + structured exceptions — pairs naturally with S1's new types), A5 (make `SettingsHolder` internal), C1 (`List`/`IList` support), A6 (command-line quoted-arg parsing), A3 (`Core.AspNet` public type or drop the package), A4 (float `Microsoft.Extensions.*` floor per-TFM). -4. **A1 (HIGH):** AOT/trim annotations for the `Reflection.Emit` engine (or plan a source generator); at minimum document the limitation before stable. -5. **README** — may still have stale `existall/SimpleConfig` links (the `docs/` tutorials were done in #20). -6. **D1 validations feature** — owner-driven; reconcile the `validate-settings` branch. Optional **P3b** compiled setter (only if a profile shows set *time* matters). +3. **Breaking cleanups (batch while pre-stable):** A5 (make `SettingsHolder`/`ISettingsHolder` internal — never in a public signature), C1 (`List`/`IList`/`ICollection` support or a documented limit), A6 (command-line quoted-arg parsing + skip exe path), A3 (`Core.AspNet` — make `Environments` public or drop the package), A4 (float `Microsoft.Extensions.*` floor per-TFM so net8 consumers aren't pulled to 10.x). +4. **A1 (HIGH):** AOT/trim annotations for the `Reflection.Emit` engine (or plan a source generator); at minimum document the limitation before stable. C2's get-only `Type` props added no trim surface, so no rework needed there. +5. **README** — may still have stale `existall/SimpleConfig` links. Optional **P3b** compiled setter (only if a profile shows set *time* matters). +6. **D1 validations feature** — owner-driven; reconcile the `validate-settings` branch. ## How releasing works (unchanged — durable) -- **`ci.yml`** — on PRs to `master`: build + test (net8.0 + net10.0). **`release.yml`**: push to `master` → auto-publishes a MinVer height-based `-alpha` to nuget.org; manual **Release** (`workflow_dispatch`, `channel` beta/rc/stable + `bump` patch/minor/major) computes the next version, tags `v*`, publishes, creates a GitHub Release (`dry_run: true` previews). +- **`ci.yml`** — on PRs to `master`: build + test (net8.0 + net10.0). **`release.yml`**: push to `master` → auto-publishes a MinVer height-based `-alpha`; manual **Release** (`workflow_dispatch`, `channel` beta/rc/stable + `bump` patch/minor/major) computes the next version, tags `v*`, publishes, creates a GitHub Release (`dry_run: true` previews). - **`benchmark.yml`** — on push to `master` + PRs: runs BDN, gates PRs on allocation regressions. - **Versioning = MinVer**, tag prefix `v`, baseline **2.0.0**, keyless publish via NuGet Trusted Publishing (OIDC). First real release: Actions → Release → `channel: beta` (→ `v2.0.0-beta.1`); use `dry_run` first. - Workflows invoke the solution through the `SOLUTION` env var (**`SimpleSettings.slnx`**). **Any push to `master` publishes an alpha** — so everything goes through PRs. ## Gotchas a new session MUST know -- **Pushing / PRs:** the active `git`/`gh` identity (`guy-frontegg`) is **read-only** on this repo; push/PR/merge via the **`guy-lud`** account. `origin` already uses the SSH alias **`github-guy-lud`** (→ `~/.ssh/guy-lud-account`), so **`git push` already uses guy-lud** — no change needed. For `gh` writes: `gh auth switch --user guy-lud`, then switch back to `guy-frontegg` after. Full recipe in the assistant's private project memory (`simplesettings-push-access`). -- **Run `dotnet` from `src/`** (global.json opts into Microsoft.Testing.Platform for TUnit). Only the net10 runtime is installed locally → net8 is **build-only** locally; CI runs both. Do NOT prefix `cd ` before `dotnet`. Run a single test project on net10 with `dotnet test --framework net10.0`. +- **Pushing / PRs:** the active `git`/`gh` identity (`guy-frontegg`) is **read-only** on this repo; push/PR/merge via **`guy-lud`**. `origin` already uses the SSH alias **`github-guy-lud`** (→ `~/.ssh/guy-lud-account`), so **`git push` already uses guy-lud** — no change needed. For `gh` writes: `gh auth switch --user guy-lud`, then switch back to `guy-frontegg` after. Full recipe in the assistant's private project memory (`simplesettings-push-access`). +- **Run `dotnet` from `src/`** (global.json opts into Microsoft.Testing.Platform for TUnit). Only the net10 runtime is installed locally → net8 is **build-only** locally; CI runs both. Do NOT prefix `cd ` before `dotnet`. Run one test project on net10 with `dotnet test --framework net10.0 --no-build` (build first). - **Benchmarks:** run from `src/` — `dotnet run -c Release --project performance/ExistForAll.SimpleSettings.Benchmark -- --filter --job short`. Output dir (`BenchmarkDotNet.Artifacts/`) is gitignored. - **`FIX-PLAN.md`** (repo root) is the full, prioritized plan with per-item file:line detail — open it explicitly; it is not auto-injected. -- **Wrap ritual — handoff branch rule:** refresh this file so it rides the session's real PR (the current work branch). If there is **no** open work branch at wrap (everything merged), **leave the refresh uncommitted** so the *next* session's first branch carries it. Do **not** commit docs to `master`, and do **not** create a dedicated docs branch/PR: `release.yml` fires on *every* `master` push with **no `paths` filter**, so a doc-only push burns a throwaway `-alpha`. (This session S1 was the work branch, so the docs rode it.) -- **Sub-agent flakiness (dotnet-claude-kit) — WORSENING:** the kit agents intermittently misfire — returning a leaked skill/role preamble with **0 tool calls** instead of doing the work. Previously only `code-reviewer`; **this session `dotnet-architect` AND `performance-analyst` also misfired** on the S1 plan review (only `security-auditor` worked, first try). Mitigation that held: **use the `/code-review` skill for the code-review step** (in-context, reliable), and **do the architect/perf plan-review lenses in-context** if their agents misfire (perf here was trivially "failure-path only, no impact"; architect was an exception-design judgment). `security-auditor` is still worth spawning — it did a thorough, source-verified pass. Kit is at latest (0.10.0). For perf specifically, `dotnet-diag:analyzing-dotnet-performance` (Microsoft-maintained) is a fallback. +- **Wrap ritual — handoff branch rule:** refresh this file so it rides the session's real PR (the current work branch). If there is **no** open work branch at wrap (everything merged), **leave the refresh uncommitted** so the *next* session's first branch carries it. Do **not** commit docs to `master`, and do **not** create a dedicated docs branch/PR: `release.yml` fires on *every* `master` push with **no `paths` filter**, so a doc-only push burns a throwaway `-alpha`. +- **Sub-agent flakiness (dotnet-claude-kit) — INTERMITTENT.** Kit agents sometimes misfire — returning a leaked skill/role preamble with **0 tool calls** instead of working. It's inconsistent: on S1 the `dotnet-architect` + `performance-analyst` misfired (only `security-auditor` worked); on C2 **both `security-auditor` and `dotnet-architect` fired cleanly**. So: spawn them, but **check each result has real tool calls / substance** — if one misfired, retry once or do that lens **in-context** (the reliable fallback). Always use the **`/code-review` skill** (not the `code-reviewer` agent) for the code-review step. Kit is at latest (0.10.0). For perf, `dotnet-diag:analyzing-dotnet-performance` (Microsoft-maintained) is a fallback. - Commits/PRs here **omit** the Co-Authored-By / Generated-with trailer (project preference). diff --git a/src/Core/ExistForAll.SimpleSettings/Core/Reflection/SettingsPropertyExtractionException.cs b/src/Core/ExistForAll.SimpleSettings/Core/Reflection/SettingsPropertyExtractionException.cs deleted file mode 100644 index 5a6153f..0000000 --- a/src/Core/ExistForAll.SimpleSettings/Core/Reflection/SettingsPropertyExtractionException.cs +++ /dev/null @@ -1,13 +0,0 @@ -using System; - -namespace ExistForAll.SimpleSettings.Core.Reflection -{ - internal class SettingsPropertyExtractionException : Exception - { - public SettingsPropertyExtractionException(Type type, Exception exception) : - base(Resources.SettingsPropertiesExtractionMessage(type), exception) - { - - } - } -} \ No newline at end of file diff --git a/src/Core/ExistForAll.SimpleSettings/Core/Reflection/TypeGenerationException.cs b/src/Core/ExistForAll.SimpleSettings/Core/Reflection/TypeGenerationException.cs deleted file mode 100644 index 82d8848..0000000 --- a/src/Core/ExistForAll.SimpleSettings/Core/Reflection/TypeGenerationException.cs +++ /dev/null @@ -1,13 +0,0 @@ -using System; - -namespace ExistForAll.SimpleSettings.Core.Reflection -{ - internal class TypeGenerationException : Exception - { - public TypeGenerationException(Type type, Exception e) : - base(Resources.SettingsClassGenerationException(type),e) - { - - } - } -} \ No newline at end of file diff --git a/src/Core/ExistForAll.SimpleSettings/Core/SettingsExtractionException.cs b/src/Core/ExistForAll.SimpleSettings/Core/SettingsExtractionException.cs deleted file mode 100644 index 2ba1e6c..0000000 --- a/src/Core/ExistForAll.SimpleSettings/Core/SettingsExtractionException.cs +++ /dev/null @@ -1,13 +0,0 @@ -using System; - -namespace ExistForAll.SimpleSettings.Core -{ - public class SettingsExtractionException : Exception - { - public SettingsExtractionException(Type type, Exception e) - : base(Resources.SettingsExtractionsExceptionMessage(type),e) - { - - } - } -} \ No newline at end of file diff --git a/src/Core/ExistForAll.SimpleSettings/SettingsBindingException.cs b/src/Core/ExistForAll.SimpleSettings/SettingsBindingException.cs index e5a3c08..0d9b223 100644 --- a/src/Core/ExistForAll.SimpleSettings/SettingsBindingException.cs +++ b/src/Core/ExistForAll.SimpleSettings/SettingsBindingException.cs @@ -1,12 +1,26 @@ -using System; - -namespace ExistForAll.SimpleSettings -{ - public class SettingsBindingException : Exception - { - public SettingsBindingException(ISectionBinder binder, BindingContext conextBindingContext, Exception e) - : base(Resources.SettingsBindingExceptionMessage(binder, conextBindingContext.Section, conextBindingContext.Key),e) - { - } - } -} \ No newline at end of file +using System; + +namespace ExistForAll.SimpleSettings +{ + // Raised when an ISectionBinder throws while binding a property. It carries the binder type, section, and + // key as primitives and deliberately does NOT retain the BindingContext — the context holds the bound value + // (context.CurrentValue), so keeping only primitives guarantees no property can ever surface that value + // (the S1 secret-redaction invariant). The binder's own exception is chained; the built-in binders never + // put a value in their messages, and a custom ISectionBinder must not either (see ISectionBinder). + public class SettingsBindingException : SimpleSettingsException + { + public SettingsBindingException(ISectionBinder binder, BindingContext context, Exception innerException) + : base(Resources.SettingsBindingExceptionMessage(binder, context.Section, context.Key), innerException) + { + BinderType = binder.GetType(); + Section = context.Section; + Key = context.Key; + } + + public Type BinderType { get; } + + public string Section { get; } + + public string Key { get; } + } +} diff --git a/src/Core/ExistForAll.SimpleSettings/SettingsBuilder.cs b/src/Core/ExistForAll.SimpleSettings/SettingsBuilder.cs index 6acf3ae..e5dbfdd 100644 --- a/src/Core/ExistForAll.SimpleSettings/SettingsBuilder.cs +++ b/src/Core/ExistForAll.SimpleSettings/SettingsBuilder.cs @@ -68,7 +68,7 @@ public object GetSettings(Type settingsType) if (!settingsType.GetTypeInfo().IsInterface) { - throw new InvalidOperationException(Resources.TypeIsNotInterface(settingsType.Name)); + throw new SettingsTypeNotInterfaceException(settingsType); } return InnerBuild(settingsType); diff --git a/src/Core/ExistForAll.SimpleSettings/SettingsCollection.cs b/src/Core/ExistForAll.SimpleSettings/SettingsCollection.cs index c2575cf..276225f 100644 --- a/src/Core/ExistForAll.SimpleSettings/SettingsCollection.cs +++ b/src/Core/ExistForAll.SimpleSettings/SettingsCollection.cs @@ -18,7 +18,7 @@ public object GetSettings(Type type) { if (!type.GetTypeInfo().IsInterface) { - throw new InvalidOperationException(Resources.TypeIsNotInterface(type.Name)); + throw new SettingsTypeNotInterfaceException(type); } return _settingsHolders.TryGetValue(type, out var holder) ? holder.SettingsImplementation : throw new SettingsTypeNotFoundException(type); @@ -28,7 +28,7 @@ public bool TryGetSettings(Type type, out object? settings) { if (!type.GetTypeInfo().IsInterface) { - throw new InvalidOperationException(Resources.TypeIsNotInterface(type.Name)); + throw new SettingsTypeNotInterfaceException(type); } if(_settingsHolders.TryGetValue(type, out var holder)) diff --git a/src/Core/ExistForAll.SimpleSettings/SettingsExtractionException.cs b/src/Core/ExistForAll.SimpleSettings/SettingsExtractionException.cs new file mode 100644 index 0000000..ee83fae --- /dev/null +++ b/src/Core/ExistForAll.SimpleSettings/SettingsExtractionException.cs @@ -0,0 +1,17 @@ +using System; + +namespace ExistForAll.SimpleSettings +{ + // Moved to the root namespace (was ExistForAll.SimpleSettings.Core) so the public exception surface is + // coherent — consumers catch it without a using for an internal-looking sub-namespace. See C2. + public class SettingsExtractionException : SimpleSettingsException + { + public SettingsExtractionException(Type settingsType, Exception innerException) + : base(Resources.SettingsExtractionsExceptionMessage(settingsType), innerException) + { + SettingsType = settingsType; + } + + public Type SettingsType { get; } + } +} diff --git a/src/Core/ExistForAll.SimpleSettings/SettingsOptionNonAttributeException.cs b/src/Core/ExistForAll.SimpleSettings/SettingsOptionNonAttributeException.cs index 20dc93b..5cf54dc 100644 --- a/src/Core/ExistForAll.SimpleSettings/SettingsOptionNonAttributeException.cs +++ b/src/Core/ExistForAll.SimpleSettings/SettingsOptionNonAttributeException.cs @@ -1,12 +1,15 @@ -using System; - -namespace ExistForAll.SimpleSettings -{ - public class SettingsOptionNonAttributeException : Exception - { - public SettingsOptionNonAttributeException(Type type) - : base(Resources.SettingsOptionAttributeTypeMessage(type)) - { - } - } -} \ No newline at end of file +using System; + +namespace ExistForAll.SimpleSettings +{ + public class SettingsOptionNonAttributeException : SimpleSettingsException + { + public SettingsOptionNonAttributeException(Type optionType) + : base(Resources.SettingsOptionAttributeTypeMessage(optionType)) + { + OptionType = optionType; + } + + public Type OptionType { get; } + } +} diff --git a/src/Core/ExistForAll.SimpleSettings/SettingsOptionsArgumentMissingException.cs b/src/Core/ExistForAll.SimpleSettings/SettingsOptionsArgumentMissingException.cs index 8c2bc29..38ccd7f 100644 --- a/src/Core/ExistForAll.SimpleSettings/SettingsOptionsArgumentMissingException.cs +++ b/src/Core/ExistForAll.SimpleSettings/SettingsOptionsArgumentMissingException.cs @@ -1,12 +1,15 @@ -using System; - -namespace ExistForAll.SimpleSettings -{ - public class SettingsOptionsArgumentMissingException : Exception - { - public SettingsOptionsArgumentMissingException(string argumentName) - : base(Resources.SettingsOptionsArgumentMissingMessage(argumentName)) - { - } - } -} \ No newline at end of file +using System; + +namespace ExistForAll.SimpleSettings +{ + public class SettingsOptionsArgumentMissingException : SimpleSettingsException + { + public SettingsOptionsArgumentMissingException(string argumentName) + : base(Resources.SettingsOptionsArgumentMissingMessage(argumentName)) + { + ArgumentName = argumentName; + } + + public string ArgumentName { get; } + } +} diff --git a/src/Core/ExistForAll.SimpleSettings/SettingsOptionsArgumentNullException.cs b/src/Core/ExistForAll.SimpleSettings/SettingsOptionsArgumentNullException.cs index 80156ea..e6175df 100644 --- a/src/Core/ExistForAll.SimpleSettings/SettingsOptionsArgumentNullException.cs +++ b/src/Core/ExistForAll.SimpleSettings/SettingsOptionsArgumentNullException.cs @@ -1,11 +1,12 @@ -using System; - -namespace ExistForAll.SimpleSettings -{ - public class SettingsOptionsArgumentNullException : Exception - { - public SettingsOptionsArgumentNullException() : base(Resources.SettingsOptionsArgumentNullMessage) - { - } - } -} \ No newline at end of file +using System; + +namespace ExistForAll.SimpleSettings +{ + public class SettingsOptionsArgumentNullException : SimpleSettingsException + { + public SettingsOptionsArgumentNullException() + : base(Resources.SettingsOptionsArgumentNullMessage) + { + } + } +} diff --git a/src/Core/ExistForAll.SimpleSettings/SettingsPropertyExtractionException.cs b/src/Core/ExistForAll.SimpleSettings/SettingsPropertyExtractionException.cs new file mode 100644 index 0000000..a20bb78 --- /dev/null +++ b/src/Core/ExistForAll.SimpleSettings/SettingsPropertyExtractionException.cs @@ -0,0 +1,17 @@ +using System; + +namespace ExistForAll.SimpleSettings +{ + // Promoted to public and moved to the root namespace (was internal in ExistForAll.SimpleSettings.Core.Reflection): + // it escapes the build path, so consumers must be able to catch it by type. See C2. + public class SettingsPropertyExtractionException : SimpleSettingsException + { + public SettingsPropertyExtractionException(Type settingsType, Exception innerException) + : base(Resources.SettingsPropertiesExtractionMessage(settingsType), innerException) + { + SettingsType = settingsType; + } + + public Type SettingsType { get; } + } +} diff --git a/src/Core/ExistForAll.SimpleSettings/SettingsPropertyNullException.cs b/src/Core/ExistForAll.SimpleSettings/SettingsPropertyNullException.cs index 4a3aef0..8d5ec97 100644 --- a/src/Core/ExistForAll.SimpleSettings/SettingsPropertyNullException.cs +++ b/src/Core/ExistForAll.SimpleSettings/SettingsPropertyNullException.cs @@ -2,14 +2,18 @@ namespace ExistForAll.SimpleSettings { - // Raised when a property marked [SettingsProperty(AllowEmpty = false)] resolves to no value. This is - // distinct from SettingsPropertyValueException (a value that failed conversion): here there is no bound - // value at all, so the message (property name only) carries nothing sensitive and is surfaced in full — - // it is a common misconfiguration and the detail aids diagnosis. See S1 in FIX-PLAN.md. - internal class SettingsPropertyNullException : Exception + // Raised when a property marked [SettingsProperty(AllowEmpty = false)] resolves to no value. Distinct from + // SettingsPropertyValueException (a value that failed conversion): here there is no bound value at all, so + // the message (property name only) carries nothing sensitive and is surfaced in full — it is a common + // misconfiguration and the detail aids diagnosis. See S1/C2 in FIX-PLAN.md. + public class SettingsPropertyNullException : SimpleSettingsException { public SettingsPropertyNullException(string propertyName) : base(Resources.PropertyNotAllowNullMessage(propertyName)) - {} + { + PropertyName = propertyName; + } + + public string PropertyName { get; } } } diff --git a/src/Core/ExistForAll.SimpleSettings/SettingsPropertyValueException.cs b/src/Core/ExistForAll.SimpleSettings/SettingsPropertyValueException.cs index 02fdf57..7a469af 100644 --- a/src/Core/ExistForAll.SimpleSettings/SettingsPropertyValueException.cs +++ b/src/Core/ExistForAll.SimpleSettings/SettingsPropertyValueException.cs @@ -1,21 +1,35 @@ -using System; -using System.Reflection; - -namespace ExistForAll.SimpleSettings -{ - // Raised when a bound configuration value fails type conversion. The bound value is deliberately NOT put - // in the message, and the converter's inner exception is NOT chained: both can embed the raw value (e.g. a - // FormatException reads "The input string 'SECRET' was not in a correct format."), which would then reach - // logs via Exception.ToString()/ILogger. Only the failure's exception type name is surfaced — that is a - // compile-time identifier and cannot carry a secret. See S1 in FIX-PLAN.md. For the distinct - // "required value missing" case (no value at all, so nothing to leak) see SettingsPropertyNullException. - internal class SettingsPropertyValueException : Exception - { - public SettingsPropertyValueException( - Type interfaceType, - PropertyInfo property, - Exception conversionError) - : base(Resources.PropertySetterExceptionMessage(interfaceType, property, conversionError.GetType().Name)) - {} - } -} \ No newline at end of file +using System; +using System.Reflection; + +namespace ExistForAll.SimpleSettings +{ + // Raised when a bound configuration value fails type conversion. The bound value is deliberately NOT put in + // the message, and the converter's inner exception is NOT chained — both can embed the raw value (e.g. a + // FormatException reads "The input string 'SECRET' was not in a correct format."), which would then reach + // logs via Exception.ToString()/ILogger. Only leak-safe metadata is exposed: the settings type, property + // name, target type, and the CLR type of the converter's failure. The ctor takes that failure's Type (not + // the Exception object) so a value-bearing exception cannot cross this type's boundary — the S1 guarantee is + // structural, not conventional. See S1/C2 in FIX-PLAN.md. For the "required value missing" case (no value at + // all, so nothing to leak) see SettingsPropertyNullException. + public class SettingsPropertyValueException : SimpleSettingsException + { + public SettingsPropertyValueException(Type settingsType, PropertyInfo property, Type conversionErrorType) + : base(Resources.PropertySetterExceptionMessage(settingsType, property, conversionErrorType.Name)) + { + SettingsType = settingsType; + PropertyName = property.Name; + TargetType = property.PropertyType; + ConversionErrorType = conversionErrorType; + } + + public Type SettingsType { get; } + + public string PropertyName { get; } + + public Type TargetType { get; } + + // The CLR type of the exception the converter threw (e.g. typeof(FormatException)) — a type identity, + // never the failing value or the exception instance. + public Type ConversionErrorType { get; } + } +} diff --git a/src/Core/ExistForAll.SimpleSettings/SettingsTypeNotFoundException.cs b/src/Core/ExistForAll.SimpleSettings/SettingsTypeNotFoundException.cs index 278bfaa..3e8a715 100644 --- a/src/Core/ExistForAll.SimpleSettings/SettingsTypeNotFoundException.cs +++ b/src/Core/ExistForAll.SimpleSettings/SettingsTypeNotFoundException.cs @@ -1,12 +1,15 @@ -using System; - -namespace ExistForAll.SimpleSettings -{ - public class SettingsTypeNotFoundException : Exception - { - public SettingsTypeNotFoundException(Type settingsType) - :base(Resources.GetSettingsNotFoundMessageFormatMessage(settingsType)) - { - } - } -} \ No newline at end of file +using System; + +namespace ExistForAll.SimpleSettings +{ + public class SettingsTypeNotFoundException : SimpleSettingsException + { + public SettingsTypeNotFoundException(Type settingsType) + : base(Resources.GetSettingsNotFoundMessageFormatMessage(settingsType)) + { + SettingsType = settingsType; + } + + public Type SettingsType { get; } + } +} diff --git a/src/Core/ExistForAll.SimpleSettings/SettingsTypeNotInterfaceException.cs b/src/Core/ExistForAll.SimpleSettings/SettingsTypeNotInterfaceException.cs new file mode 100644 index 0000000..1d728ce --- /dev/null +++ b/src/Core/ExistForAll.SimpleSettings/SettingsTypeNotInterfaceException.cs @@ -0,0 +1,18 @@ +using System; + +namespace ExistForAll.SimpleSettings +{ + // Raised when a type handed to the builder/collection is not an interface (SimpleSettings generates its + // implementations from interfaces). Replaces the untyped InvalidOperationException that used to escape, so + // it is catchable as part of the SimpleSettingsException family. See C2 in FIX-PLAN.md. + public class SettingsTypeNotInterfaceException : SimpleSettingsException + { + public SettingsTypeNotInterfaceException(Type settingsType) + : base(Resources.TypeIsNotInterface(settingsType.Name)) + { + SettingsType = settingsType; + } + + public Type SettingsType { get; } + } +} diff --git a/src/Core/ExistForAll.SimpleSettings/SimpleSettingsException.cs b/src/Core/ExistForAll.SimpleSettings/SimpleSettingsException.cs new file mode 100644 index 0000000..6deea51 --- /dev/null +++ b/src/Core/ExistForAll.SimpleSettings/SimpleSettingsException.cs @@ -0,0 +1,22 @@ +using System; + +namespace ExistForAll.SimpleSettings +{ + // Common base for every exception the library throws, so consumers can catch the whole family with a single + // catch (SimpleSettingsException). Abstract: it is a grouping/marker base — always throw a specific subtype. + // Only the two forwarding ctors are needed; no parameterless ctor (every subtype carries required context) + // and no [Serializable]/SerializationInfo ctor (BinaryFormatter is obsolete on the net8/net10 targets). + // See C2 in FIX-PLAN.md. + public abstract class SimpleSettingsException : Exception + { + protected SimpleSettingsException(string message) + : base(message) + { + } + + protected SimpleSettingsException(string message, Exception innerException) + : base(message, innerException) + { + } + } +} diff --git a/src/Core/ExistForAll.SimpleSettings/TypeGenerationException.cs b/src/Core/ExistForAll.SimpleSettings/TypeGenerationException.cs new file mode 100644 index 0000000..c099ac6 --- /dev/null +++ b/src/Core/ExistForAll.SimpleSettings/TypeGenerationException.cs @@ -0,0 +1,17 @@ +using System; + +namespace ExistForAll.SimpleSettings +{ + // Promoted to public and moved to the root namespace (was internal in ExistForAll.SimpleSettings.Core.Reflection): + // it escapes the build path, so consumers must be able to catch it by type. See C2. + public class TypeGenerationException : SimpleSettingsException + { + public TypeGenerationException(Type settingsType, Exception innerException) + : base(Resources.SettingsClassGenerationException(settingsType), innerException) + { + SettingsType = settingsType; + } + + public Type SettingsType { get; } + } +} diff --git a/src/Core/ExistForAll.SimpleSettings/ValuesPopulator.cs b/src/Core/ExistForAll.SimpleSettings/ValuesPopulator.cs index 7cece38..4408de7 100644 --- a/src/Core/ExistForAll.SimpleSettings/ValuesPopulator.cs +++ b/src/Core/ExistForAll.SimpleSettings/ValuesPopulator.cs @@ -101,7 +101,7 @@ private SettingsPlan GetOrBuildPlan(Type settings, SettingsOptions options) // the per-populate convert try as SettingsPropertyValueException. No bound value exists at // plan build, and setup failures describe types/converters, not a value — so nothing sensitive // is dropped by the redacting exception (which never carries the value or chains the inner). - throw new SettingsPropertyValueException(settings, property, e); + throw new SettingsPropertyValueException(settings, property, e.GetType()); } } @@ -126,7 +126,7 @@ private SettingsPlan GetOrBuildPlan(Type settings, SettingsOptions options) { // e (and its message) may embed the raw bound value, which could be a secret — never chain it // or put the value in the message. Only the failure's type name is surfaced. See S1. - throw new SettingsPropertyValueException(settingsType, propertyPlan.Property, e); + throw new SettingsPropertyValueException(settingsType, propertyPlan.Property, e.GetType()); } } } diff --git a/src/Tests/ExistForAll.SimpleSettings.UnitTests/SimpleSettings/ExceptionHierarchyTests.cs b/src/Tests/ExistForAll.SimpleSettings.UnitTests/SimpleSettings/ExceptionHierarchyTests.cs new file mode 100644 index 0000000..b7f35b4 --- /dev/null +++ b/src/Tests/ExistForAll.SimpleSettings.UnitTests/SimpleSettings/ExceptionHierarchyTests.cs @@ -0,0 +1,122 @@ +using System; +using System.Linq; +using ExistForAll.SimpleSettings.Binder; + +namespace ExistForAll.SimpleSettings.UnitTests.SimpleSettings +{ + // C2: every library exception derives from the public abstract SimpleSettingsException, the escaping types + // are public, and each carries its context as structured (leak-safe) properties. See C2 in FIX-PLAN.md. + public class ExceptionHierarchyTests + { + [Test] + public async Task Base_IsPublicAndAbstract() + { + await Assert.That(typeof(SimpleSettingsException).IsPublic).IsTrue(); + await Assert.That(typeof(SimpleSettingsException).IsAbstract).IsTrue(); + } + + [Test] + public async Task AllLibraryExceptions_DeriveFromSimpleSettingsException() + { + var baseType = typeof(SimpleSettingsException); + + // Any exception type in the library that does NOT derive from the common base is an offender — + // this fails if a future exception is added without reparenting. + var offenders = baseType.Assembly.GetTypes() + .Where(t => typeof(Exception).IsAssignableFrom(t) + && t != baseType + && t.Namespace != null + && t.Namespace.StartsWith("ExistForAll.SimpleSettings")) + .Where(t => !baseType.IsAssignableFrom(t)) + .Select(t => t.FullName) + .ToArray(); + + await Assert.That(offenders.Length).IsEqualTo(0); + } + + [Test] + public async Task ExceptionsThatEscapeTheBuild_ArePublic() + { + // InternalsVisibleTo lets this test project see internal types, so a plain reference can't prove + // `public` — assert accessibility via reflection instead. + await Assert.That(typeof(SettingsPropertyValueException).IsPublic).IsTrue(); + await Assert.That(typeof(SettingsPropertyNullException).IsPublic).IsTrue(); + await Assert.That(typeof(TypeGenerationException).IsPublic).IsTrue(); + await Assert.That(typeof(SettingsPropertyExtractionException).IsPublic).IsTrue(); + } + + [Test] + public async Task NonInterfaceType_ThrowsTypedException_CatchableAsBase() + { + var builder = SettingsBuilder.CreateBuilder(); + + var caught = CatchBase(() => builder.GetSettings(typeof(NotAnInterface))); + + await Assert.That(caught is SettingsTypeNotInterfaceException).IsTrue(); + await Assert.That(((SettingsTypeNotInterfaceException)caught!).SettingsType).IsEqualTo(typeof(NotAnInterface)); + } + + [Test] + public async Task ConversionFailure_ExposesSafeStructuredMetadata_AndNoChainedInner() + { + var collection = new InMemoryCollection(); + collection.Add("IntSettings", nameof(IIntSettings.Value), "not-a-number"); + var builder = SettingsBuilder.CreateBuilder(x => x.AddSectionBinder(new InMemoryBinder(collection))); + + var caught = CatchBase(() => builder.GetSettings()); + + await Assert.That(caught is SettingsPropertyValueException).IsTrue(); + var ex = (SettingsPropertyValueException)caught!; + await Assert.That(ex.SettingsType).IsEqualTo(typeof(IIntSettings)); + await Assert.That(ex.PropertyName).IsEqualTo(nameof(IIntSettings.Value)); + await Assert.That(ex.TargetType).IsEqualTo(typeof(int)); + await Assert.That(ex.ConversionErrorType).IsEqualTo(typeof(FormatException)); + // The load-bearing S1 invariant: no value-bearing inner is chained. + await Assert.That(ex.InnerException).IsNull(); + } + + [Test] + public async Task BinderThrows_ExposesBinderContext() + { + var builder = SettingsBuilder.CreateBuilder(x => x.AddSectionBinder(new ThrowingBinder())); + + var caught = CatchBase(() => builder.GetSettings()); + + await Assert.That(caught is SettingsBindingException).IsTrue(); + var ex = (SettingsBindingException)caught!; + await Assert.That(ex.BinderType).IsEqualTo(typeof(ThrowingBinder)); + await Assert.That(ex.Section).IsEqualTo("IntSettings"); + await Assert.That(ex.Key).IsEqualTo(nameof(IIntSettings.Value)); + } + + private static SimpleSettingsException? CatchBase(Action act) + { + try + { + act(); + } + catch (SimpleSettingsException e) + { + return e; + } + + return null; + } + + public interface IIntSettings + { + int Value { get; set; } + } + + public class NotAnInterface + { + } + + // Throws on every bind so ValuesPopulator wraps it in a SettingsBindingException. + private class ThrowingBinder : ISectionBinder + { + public void BindPropertySettings(BindingContext context) + => throw new InvalidOperationException("binder failed"); + } + } +} diff --git a/src/Tests/ExistForAll.SimpleSettings.UnitTests/SimpleSettings/SettingsCollectionTests.cs b/src/Tests/ExistForAll.SimpleSettings.UnitTests/SimpleSettings/SettingsCollectionTests.cs index f5bf819..abb00fb 100644 --- a/src/Tests/ExistForAll.SimpleSettings.UnitTests/SimpleSettings/SettingsCollectionTests.cs +++ b/src/Tests/ExistForAll.SimpleSettings.UnitTests/SimpleSettings/SettingsCollectionTests.cs @@ -11,7 +11,9 @@ public async Task GetSettings_WhenTypeIsNotInterface_ShouldThrowException() { var sut = SettingsBuilder.CreateBuilder(); - await Assert.That(() => sut.GetSettings()).Throws(); + // C2: a non-interface type now throws the typed SettingsTypeNotInterfaceException (was an untyped + // InvalidOperationException), so it's catchable as part of the SimpleSettingsException family. + await Assert.That(() => sut.GetSettings()).Throws(); } [Test]