C4: consolidate DI composition - single provider-factory owner, single observation-core owner - #248
Merged
Merged
Conversation
AddNexoAgents was the only registration of Nexo.Core.Application.Agents.IAgentFactory
anywhere in the tree, and nothing called it except the one test written to prove it
worked. The kernel never invokes it, so IAgentFactory resolves to null in every
deployment profile -- confirmed by probing AddNexo across all 14 flag configurations
before touching anything:
IAgentFactory = <null> in all of Full / Server / Edge / AirGapped / System,
with and without trust, adaptive, and ephemeral.
So this was not a service that happened to be unused; it was a registration helper
that had never been wired into composition at all. Deleting it changes no resolution
because there was none to change.
The IAgentFactory / AgentFactory types themselves are left in place. They are public
Core.Application surface and a host can still register them by hand; only the unused
helper goes. Nexo.Core.Application carries no PublicAPI.Shipped.txt and no approved
snapshot mentions AddNexoAgents, so nothing needs blessing.
The test went with it rather than being retargeted: it asserted only that the helper
registered the thing the helper registered.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ng (C4)
The chain was assembled in two assemblies at once. Paths A and C were built in
NexoKernelRegistrar Phase 15; Path B was built inside
Nexo.BackgroundAgents.AddTrustServices. Nothing enforced that exactly one of them
fired -- correctness rested on three booleans (useSanitizingProviderFactory,
skipProviderRegistration, useAdaptive) being passed across the assembly boundary in
a combination that happened to be mutually exclusive. Flip one and you get either
two IProviderFactory registrations racing on last-wins, or none at all.
Phase 15 now owns the wiring outright. The kernel always calls AddTrustServices with
skipProviderRegistration: true, so trust contributes taxonomy, sanitization proxy and
audit log and nothing else. The chain is then composed bottom-up from two independent
switches instead of three hand-maintained branches:
ProviderFactory -> [SanitizingProviderFactory] -> [AdaptiveProviderFactory]
which produces the same four shipped combinations by construction rather than by
three `if`s agreeing with each other.
AddTrustServices keeps its own provider branch. Nexo.BackgroundAgents is a packed
package (scripts/pack-nexo-hosting-graph.sh), so deleting that branch would be a
behaviour break for SDK consumers who call it directly rather than a kernel change.
It is now commented as a path the kernel never takes.
Two asymmetries were preserved deliberately rather than tidied away:
* Path C still does not register the CONCRETE ProviderFactory, only the interface.
A host resolving ProviderFactory gets null when neither trust nor adaptive is on,
exactly as before.
* The IEphemeralModelLifecycle probe is not uniform. The sanitizing path only asked
for it when ephemeral models were enabled; the adaptive and plain paths always
asked. Those are equivalent as the kernel ships -- the lifecycle is registered
only when ephemeral models are on -- but they diverge if a host pre-registers its
own, so each path keeps the probe it had (see CreateProviderFactory remarks).
Verified by building AddNexo under 14 flag configurations (Full/Server/Edge/AirGapped/
System x trust x adaptive x ephemeral x pattern-store variants) and diffing both the
resolved concrete types and the full ServiceDescriptor list against master:
resolved types + decorator chains + store paths : IDENTICAL
ServiceDescriptor list, 3806 descriptors : byte-identical, same order
The descriptors do not even move, because the provider block was already the last
thing AddTrustServices did. Chains were unwrapped reflectively rather than compared
by outermost type, since Adaptive-over-Sanitizing and Adaptive-over-bare both surface
as AdaptiveProviderFactory and the difference would otherwise be invisible.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…(C4) IPatternStore, IPatternProcessedStore and IContextAssembler were registered twice, by Phase 07 (adaptation) and Phase 12 (observation pipeline), both with AddSingleton. Phase 12 ran second and silently won on last-wins wherever both fired. The two were not equivalent, which is what makes this more than cosmetic. They disagree about where the store lives: Phase 07 new LiteDbPatternStore(patternStorePath) -- verbatim Phase 12 new LiteDbPatternStore(Combine(repoRoot, storePath)) -- repo-rooted With an absolute PatternStorePath the two strings coincide and the duplicate is invisible. With a RELATIVE path they differ, and only then does it become observable which registration won. Phase 12 also degrades to NoOpPatternStore under NEXO_OBSERVATION_DEGRADED_MODE; Phase 07 has no such fallback. The premise going in was that Phase 07's registration is dead. That is only half true, and the half that isn't matters: Full / Server (pipeline on) Phase 12 wins, Phase 07 dead AirGapped (pipeline off) Phase 07 is the ONLY registration -- load-bearing any profile + DisableObservationPipeline same, load-bearing AirGapped ships IncludeAdaptation: true with IncludeObservationPipeline: false, so deleting Phase 07's registration outright would have left adaptation resolving a missing IContextAssembler there. Ownership is therefore made conditional rather than removed: Phase 12 owns observation core when the pipeline is active, adaptation owns it otherwise, and the phase states which case it is in. AddAdaptationInfrastructure gains an overload rather than an optional parameter, so the change is additive for the packed Nexo.Infrastructure surface instead of binary-breaking. Verified with the same 14-configuration probe: resolved types + decorator chains + store paths : IDENTICAL to master ServiceDescriptor delta : 6 removals, 0 additions, 0 moves The 6 are exactly the three observation-core descriptors in the two configurations where both phases ran (full + pattern store, absolute and relative). Every configuration where Phase 07 is load-bearing is untouched, and the surviving store path in the relative case is still the repo-rooted one -- i.e. the registration that was already winning is the one that remains. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Comments only -- no registration is added, removed, or reordered. Three duplicates remain after the previous two commits. All three are deliberate, and the point of this commit is that a reader can now tell that from the code instead of having to reconstruct the ordering to find out. IRAGService (Phase 11 vs 13b). Phase 13b overrides Phase 11 on last-wins so the contract always resolves to the VectorData-backed adapter. Converting either side to TryAdd would INVERT this and hand IRAGService back to the legacy RAGService -- the opposite of the intent -- so it stays AddSingleton, now said out loud. Also recorded why it is not expressed as a Replace: nothing enumerates IEnumerable<IRAGService> today, but a Replace would quietly change that enumeration for anyone who starts. IResilientExecutor (Phase 15 vs AddTrustServices). Never actually contended: the kernel registers it first and TryAdd means first-wins, and after the previous commit the trust-side registration sits in a branch the kernel no longer takes at all. GenerativeArtifactBrick (Nexo.Authoring). Genuinely two objects, not two registrations of one: TryAddSingleton creates the DI singleton, while AddNexoBrick routes the TYPE through AdaptationBrickOptions to ActivatorUtilities.CreateInstance, which constructs a fresh instance when BrickRegistry is built. Left alone deliberately -- the brick is stateless so the duplication is harmless, and making the registry resolve from the container instead would change instance identity for every additional brick type rather than just this one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The existing KernelPhaseResolutionTests answer "does it resolve". That is exactly
the question this change could break without failing: a provider chain that lost its
sanitizing layer still resolves, still reports AdaptiveProviderFactory as its type,
and still passes every test in the suite — while sending unscrubbed prompts past the
trust boundary. Presence is not the property worth pinning here; composition is.
Ten tests, covering what the consolidation made true by construction rather than by
coincidence:
* The provider chain for all four flag combinations, asserted as an ORDERED list
unwrapped through the decorators — plain / sanitizing / adaptive / both. This is
the assertion that distinguishes Adaptive->Sanitizing->Provider from
Adaptive->Provider, which are indistinguishable by resolved type.
* The wrapper and the concrete factory are the SAME instance. Two would mean two
provider caches and two ephemeral-model lifecycles.
* The deliberate asymmetry that the plain path registers only the interface, so
"fixing" it later has to be a decision rather than a side effect.
* Observation core has exactly one registration — asserted on the descriptor list,
which is where a duplicate is visible; resolution only ever shows the winner.
* Which owner wins in each direction: the pipeline's repo-rooted path when active,
adaptation's verbatim path under AirGapped where the pipeline is off and
adaptation's registration is load-bearing.
The store-path tests use a RELATIVE path deliberately. With an absolute path the two
owners produce byte-identical strings and the test would pass no matter which one
registered — the same reason the duplicate went unnoticed.
Tests are async purely because xunit's Timeout only applies to async tests, matching
the convention in KernelPhaseResolutionTests next door. Env vars are saved and
restored per instance and the class joins the EnvironmentVariables collection, so it
cannot race the other hosting tests over NEXO_TRUST_ENABLED / NEXO_LOAD_PREFERENCE.
10/10 pass.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
…e them
The testing-strategy gate failed with "Production-wiring paths changed without
ProdStyle/virtual-host test updates" -- on a PR whose entire point is a new
production-wiring test suite.
scripts/ci/pr-testing-strategy-gate.sh detects that suite by FILENAME, matching a
list of markers (*ProdStyle*, *KernelPhaseResolution*, *HostingE2ESmoke*, ...). It
does not read [Trait("Category", "ProdStyle")]. The class already carried that trait
and already ran in the prod-style category; only its name failed to say so, so the
gate could not see what it was.
Renamed KernelDiCompositionPinningTests -> KernelDiCompositionProdStyleTests. Nothing
about the tests changes; 10/10 still pass.
Deliberately NOT fixed with [skip-prod-style]. That token exists for changes that
genuinely do not warrant wiring tests, and asserting it here would be false -- the
tests exist, they are exactly the wiring tests the gate is asking for. Making the
filename agree with the trait is the honest fix. Teaching the gate to read traits
instead of filenames would be the better one, but that is a change to shared CI
tooling and does not belong in a DI consolidation PR.
Co-Authored-By: Claude Opus 5 (1M context) <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
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.
C4 — DI composition cleanup
Consolidation item C4. The riskiest of the set: registration order in
NexoKernelRegistraris load-bearing, so the hard constraint was do not change observable DI resolution. Every service must still resolve to the same implementation, and registration-order semantics must be preserved.The 20-phase model is kept as-is. Duplicates were consolidated inside it; the phase composition is not restructured.
What was consolidated
1. The provider-factory chain now has a single owner (Phase 15).
The chain was assembled in two assemblies at once. Paths A and C were built in
NexoKernelRegistrar.Phases.cs; Path B was built insideNexo.BackgroundAgents.AddTrustServices. Nothing enforced that exactly one fired — correctness rested on three booleans (useSanitizingProviderFactory,skipProviderRegistration,useAdaptive) crossing the assembly boundary in a combination that happened to be mutually exclusive. Flip one and you get either twoIProviderFactoryregistrations racing on last-wins, or none.The kernel now always calls
AddTrustServiceswithskipProviderRegistration: true, so trust contributes taxonomy, sanitization proxy and audit log and nothing else. The chain is composed bottom-up from two independent switches instead of three hand-maintained branches:producing the same four shipped combinations by construction rather than by three branches agreeing with each other.
2. Observation core has a single owner per configuration.
IPatternStore,IPatternProcessedStoreandIContextAssemblerwere registered by both Phase 07 (adaptation) and Phase 12 (observation pipeline), both withAddSingleton, with Phase 12 silently winning on last-wins. The two are not equivalent — they disagree about where the store lives:new LiteDbPatternStore(patternStorePath)— verbatimnew LiteDbPatternStore(Combine(repoRoot, storePath))— repo-rootedOwnership is now explicit: the pipeline owns observation core when active, adaptation owns it otherwise.
3. Deleted the orphan
AddNexoAgents.Analysis correction — Phase 07 is not a dead orphan
The premise going in was that the Phase 07 observation closure is dead. That is only half true, and the half that is not matters:
DisableObservationPipelineAirGappedshipsIncludeAdaptation: truewithIncludeObservationPipeline: false. Deleting Phase 07's registration outright — the obvious reading of "remove the dead duplicate" — would have left adaptation resolving a missingIContextAssemblerthere. Ownership was therefore made conditional rather than removed.Two further corrections from the same investigation:
PatternStorePaththe two registrations produce byte-identical strings, becausePath.Combinediscards the root when the second argument is absolute. The duplicate is only observable with a relative path. That is why it went unnoticed, and why the pinning tests use a relative path deliberately.IAgentFactoryresolves to<null>in all 14 configurations.AddNexoAgentswas not an unused service — it was a registration helper never wired into composition at all.Behaviour-preservation proof
A probe builds the real container from
AddNexounder 14 flag configurations (Full / Server / Edge / AirGapped / System × trust × adaptive × ephemeral × pattern-store variants) and records both the fullServiceDescriptorlist in registration order and the concrete type each of 21 key services resolves to. Baseline captured on master before any edit, then diffed.ServiceDescriptorlistThe 6 removals are exactly the three observation-core descriptors in the two configurations where both phases ran. Every configuration where Phase 07 is load-bearing is untouched, and in the relative-path case the surviving path is still the repo-rooted one — the registration that was already winning is the one that remains.
Chains are compared unwrapped, not by resolved type.
Adaptive -> Sanitizing -> ProviderandAdaptive -> Providerboth surface asAdaptiveProviderFactory; a chain that lost its sanitizing layer still resolves, still reports the same type, and would have passed every pre-existing test while sending unscrubbed prompts past the trust boundary.Judgment calls, flagged
TryAddwas deliberately NOT applied toAddObservationCore/AddObservationPipeline. Both are public API on packed assemblies, andAddtoTryAddinverts precedence for any host that pre-registers its own store: today the kernel overrides them, withTryAddthey would win. That is an observable resolution change that cannot be proven safe, and it conflicts with the hard constraint. The last-wins accident is gone regardless — removed structurally by single ownership rather than by the mechanism.AddTrustServiceskeeps its provider branch.Nexo.BackgroundAgentsis packed (scripts/pack-nexo-hosting-graph.sh), so deleting it would be a behaviour break for SDK consumers who call it directly, not a kernel change. The kernel simply never takes it now; it is commented as such.IAgentFactory/AgentFactorytypes remain. Public Core.Application surface, still hand-registerable. Only the unused helper was deleted. NoPublicAPI.Shipped.txtor approved snapshot references it, so nothing needs blessing.GenerativeArtifactBrickis genuinely two objects, not two registrations of one:TryAddSingletoncreates the DI singleton whileAddNexoBrickroutes the type throughAdaptationBrickOptionstoActivatorUtilities.CreateInstance, which constructs a fresh instance. Left alone — the brick is stateless, and making the registry resolve from the container would change instance identity for every additional brick type.ProviderFactory(only the interface), and theIEphemeralModelLifecycleprobe is still non-uniform across paths — equivalent as the kernel ships, divergent only if a host pre-registers its own lifecycle.AddAdaptationInfrastructuregains an overload rather than an optional parameter, so the change is additive for the packedNexo.Infrastructuresurface instead of binary-breaking.Tests
KernelDiCompositionProdStyleTests(10 tests) pins what a presence check cannot see: the ordered provider chain for all four flag combinations, that wrapper and wrapped are the same instance, the deliberate concrete-ProviderFactoryasymmetry, that observation core has exactly one registration (asserted on the descriptor list — resolution only ever shows the winner), and which owner wins in each direction.Nexo.slnbuildNexo.Tests.ApplicationNexo.Tests.DomainApplication coverage moves 68.35% to 68.31%, a 0.04pp drop purely from deleting the covered orphan.
Pre-existing failure, not from this change
Three
RuntimeStudioBlackBoxSmokeTestsdaemon tests time out locally. Verified by checking out master, rebuilding and rerunning: identical 3 failures on master. They spawn the real CLI daemon; one of the three does not involve the observation pipeline at all.🤖 Generated with Claude Code