feat(core): milestone 5 phase 0 - public provider extensibility engine wiring - #28
Merged
ncipollina merged 3 commits intoJul 31, 2026
Merged
Conversation
…e wiring Design docs for Milestone 5 (NSubstitute Integration): ADR-0024 (the core public provider extensibility model - how an integration package contributes open-ended, pattern-matching composition logic to pipeline stages 5/6, deliberately deferred by Milestone 3), ADR-0025 (Compono.NSubstitute's own package design, built on ADR-0024), and PLAN-0005 tracking the milestone's four phases. Both ADRs record an explicit Alpha Compatibility Policy - the contract is public but provisional during alpha, revisable when Milestone 6/7 or real usage justifies it, any such change recorded as a dated ADR Amendment rather than a silent edit. Implements PLAN-0005 Phase 0 against ADR-0024's Decision Outcome: - New public surface: ICompositionValueProvider (TryProvide), the decoupled CompositionProviderRequest/CompositionProviderResult value types, and CompositionBuilder.AddSemanticProvider/AddTestDoubleProvider - registering into pipeline stages 5/6, which have been wired end to end since Milestone 2 but never fed anything until now. - Compiled at CompositionBuilder.Build() time into an internal PublicProviderAdapter : ICompositionProvider, the same "public builder data compiles into an internal provider" shape ADR-0020's .For<T>() rules already use - no new pipeline mechanism, stage order/shared-scope reuse/recursion detection are all reused unchanged. - ICompositionProvider gains a ProviderType default-interface member so diagnostics keep naming the real, logical provider (e.g. a future NSubstituteProvider) through the adapter, never the adapter's own type. - Composer.Create<T>/CreateMany<T>/CreateRow (and the internal 3-arg test seam one level further down the constructor chain) now thread the configured semantic/test-double provider lists into CompositionContext. New CompositionValueProviderTests.cs covers this end to end through the real Composer API, in isolation from Compono.NSubstitute (which doesn't exist yet, per testing.md's "verify a new public entry point in isolation" rule): stage precedence, registration order, NotHandled fallthrough, diagnostics identity through the adapter, shared-scope reuse, and uncaught-provider-exception propagation. Full suite green: Compono.Tests 408/408 (204 x 2 TFMs), Compono.XunitV3.Tests 94/94, Compono.Generators.Tests 166/166 - dotnet build/dotnet test against the whole solution, zero warnings, zero regressions. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cee71a5787
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…-recursion PR #28 review (Codex, two P1 findings, same root cause): PublicProviderAdapter called a wrapped ICompositionValueProvider's TryProvide directly, bypassing CompositionContext.InvokeFactory entirely - the only thing that pushes a manual-resolve frame or guards against reentrance. 1. A provider calling the descriptor-less context.Resolve<T>() - exactly what ICompositionValueProvider's own XML doc promises works - threw InvalidOperationException unconditionally, since no manual-resolve frame was ever active. 2. A provider recursing on its own requested type (via the descriptor-based overload, which needs no frame) had no cycle protection and ran until StackOverflowException instead of producing a diagnosed CompositionException. Fixed with a new CompositionContext.InvokeProvider method, structurally parallel to InvokeFactory but distinct in two ways: reentrance is keyed on (provider instance, requested type), not delegate identity - TypeRuleProvider/ MemberRuleProvider get away with delegate-identity keying because each instance is compiled 1:1 for exactly one type, but one ICompositionValueProvider instance can legitimately handle many types (e.g. "any interface"), including composing a different type as a legitimate nested dependency, which a provider-only key would have wrongly blocked. And it never catches or wraps an exception TryProvide throws - InvokeFactory's blanket catch was deliberately not reused, since ADR-0024's Provider Failure Semantics commits to a provider's own thrown exception propagating uncaught; the reentrance guard's own diagnosed exception is a different concern from wrapping the provider's exception, and stays intact. PublicProviderAdapter now carries its own PipelineStage (set at construction in CompositionBuilder.Build) so InvokeProvider's reentrance-failure trace entry is recorded against the right stage. Recorded as ADR-0024 Amendment 1 (the ADR is Accepted, so this is a dated amendment, not a silent edit to its original Decision Outcome text) and in PLAN-0005's Phase 0 Notes. Two new regression tests (CompositionValueProviderTests.cs); the pre-existing ThrownException_FromAPublicProvider_PropagatesUncaught test continues to pass unchanged, confirming the fix didn't reverse the Provider Failure Semantics decision. Full suite green: 672/672 across the whole solution. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3cb7892b51
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…ry-only wording PR #28 review (Codex, two P2 findings): 1. docs/architecture.md/mvp.md/public-api.md still described the public provider extension point as "design only" and stages 5/6 as unconditionally "empty" - stale the moment the prior commit's InvokeProvider fix landed, since AddSemanticProvider/AddTestDoubleProvider are real, tested public API now. Updated the Resolution Pipeline table, the stages-4/5/6/7 summary paragraph, the Open Architectural Decisions entry, mvp.md's Milestone 5 section, and public-api.md's Provider Extensibility section to distinguish "ADR-0024's core mechanism: implemented" from "Compono.NSubstitute (ADR-0025): still doesn't exist" - pulled forward from PLAN-0005's own Phase 3 task list rather than left contradicting the code until that phase lands; Phase 3's checklist updated to reflect what was pulled forward versus what's still open. 2. ICompositionContext.Resolve<TValue>()'s public XML doc and its InvalidOperationException message still described a factory-only contract, not mentioning that a provider's TryProvide (via the new InvokeProvider method) is now also a valid caller. Updated both, plus the same factory-only framing on PathSegment.ManualResolve/ CompositionRequestKind.ManualResolve's XML docs for consistency. Full suite green: 672/672. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
ncipollina
added a commit
that referenced
this pull request
Jul 31, 2026
…nted docs/public-api.md and docs/mvp.md still said the package was "not yet implemented" after this phase's code landed - pulled the correction forward into this PR rather than deferring to Phase 3, per Codex review and matching the same fix pattern PR #28 established for Phase 0. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
ncipollina
added a commit
that referenced
this pull request
Jul 31, 2026
…29) * feat(nsubstitute): milestone 5 phase 1 - Compono.NSubstitute package NSubstituteProvider/NSubstituteOptions/UseNSubstitute per ADR-0025 (Amendment 1's corrected snapshot/delegate-matching shapes). Tests (Phase 2) and docs (Phase 3) tracked separately in PLAN-0005. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * docs(xunitv3): shorten PrivateAssets comment on Compono reference Trim wording that implied all transitive dependencies would otherwise disappear - it's specifically the analyzer asset that needs to flow through. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * docs(nsubstitute): stop describing Compono.NSubstitute as not implemented docs/public-api.md and docs/mvp.md still said the package was "not yet implemented" after this phase's code landed - pulled the correction forward into this PR rather than deferring to Phase 3, per Codex review and matching the same fix pattern PR #28 established for Phase 0. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
📋 Summary
Milestone 5 (NSubstitute Integration) design docs — ADR-0024 (public provider extensibility model), ADR-0025 (
Compono.NSubstitutepackage design), PLAN-0005 (four-phase implementation plan) — plus PLAN-0005 Phase 0: the core engine wiring both ADRs specify. Phase 0 fills in pipeline stages 5/6 (semantic value providers, test-double providers), which have been wired end-to-end since Milestone 2 but never fed anything until now, with a small public extension point decoupled from the engine's own internal request/provider types.📝 Changes
Design docs
ICompositionValueProviderinterface (not a bare delegate — a genuine fork raised and resolved during design review), a small decoupled request/result contract, deterministic stage placement/registration order, diagnostics identity, immutable/reusable registration,NotHandled/Handledsemantics. Includes an explicit Alpha Compatibility Policy: the contract is public but provisional during alpha, revisable when Milestone 6/7 or real usage justifies it, any such change recorded as a dated ADR Amendment rather than a silent edit.Compono.NSubstitute's own package design built on ADR-0024 (substitutable-shape rules for interfaces/delegates/abstract classes,NSubstituteOptions, diagnostics). Includes Amendment 1, recorded during pre-implementation review:NSubstituteProvidermust snapshotNSubstituteOptionsrather than retain the caller-owned mutable instance, and delegate matching must useIsSubclassOf(typeof(MulticastDelegate))rather thanDelegate.IsAssignableFrom(which wrongly matches the two framework base types themselves).Compono.NSubstitute; Phase 2: test suites/verification; Phase 3: docs), including a Breaking-Change Handling During Alpha section sourced from this repo's actual Release Drafter config (.github/release-drafter.yml'sbreaking/majorlabel → major version bump).Phase 0 implementation (
src/Compono)ICompositionValueProvider(TryProvide),CompositionProviderRequest/CompositionProviderResult,CompositionBuilder.AddSemanticProvider/AddTestDoubleProvider.CompositionBuilder.Build()time into an internalPublicProviderAdapter : ICompositionProvider— the same "public builder data compiles into an internal provider" shape ADR-0020's.For<T>()rules already use. No new pipeline mechanism: stage order, shared-scope reuse, and recursion detection are all reused unchanged.ICompositionProvidergains aProviderTypedefault-interface member so diagnostics keep naming the real, logical provider through the adapter (never the adapter's own type).Composer.Create<T>/CreateMany<T>/CreateRow(and one internal test seam one level further down the constructor chain) now thread the configured semantic/test-double provider lists intoCompositionContext.Tests
CompositionValueProviderTests.cs, exercising the publicComposerAPI in isolation fromCompono.NSubstitute(which doesn't exist yet —testing.md's "verify a new public entry point in isolation" rule): stage precedence (semantic before test-double), registration order,NotHandledfallthrough, diagnostics identity through the adapter, shared-scope reuse, uncaught-provider-exception propagation.🧪 Validation
dotnet build/dotnet testagainst the whole solution — 668/668 passing, 0 warnings. (Compono.Tests408/408 = 204 × 2 TFMs,Compono.XunitV3.Tests94/94,Compono.Generators.Tests166/166.)Compono.Testscases still pass unchanged); confirmeddocs/mvp.md/architecture.md/public-api.mdare deliberately left describing stages 5/6 as still empty — that update is explicitly PLAN-0005 Phase 3's job, matching PLAN-0004's precedent of deferring doc updates to the final phase.💬 Notes for Reviewers
Compono.NSubstituteitself (Phase 1) is a separate, later PR, per this repo's one-phase-per-PR rule.Accepted. ADR-0025's Amendment 1 was applied before anyCompono.NSubstitutecode exists, so there's no drift between the ADR and a future implementation to reconcile later.Co-Authored-By: Claude Sonnet 5 noreply@anthropic.com