Put an ILogger seam over the Serilog pipeline

[phmatray]

Summary

Puts Microsoft.Extensions.Logging.ILogger in front of Serilog, so framework code stops referencing Serilog directly. Serilog stays the provider and the pipeline in Logging.Configure is untouched — no behaviour change and no public-API change.

Part of #428 — first of that issue's four additive PRs. Theme decoupling, the Spectre presenter, the [Obsolete]/[Experimental] markers, and the breaking removals are all out of scope here.

File	Change
Directory.Packages.props	+ Serilog.Extensions.Logging, + Microsoft.Extensions.Logging.Abstractions
src/Fallout.Build/Fallout.Build.csproj	PackageReferences for those two plus Microsoft.Extensions.DependencyInjection
Logging.DependencyInjection.cs (new)	AddFalloutLogging — configures the Serilog pipeline, registers ILoggerFactory / ILogger<> / ILogger over it
Logging.cs	static façade: Factory, Logger, UseLoggerFactory
Execution/BuildManager.cs	per-run composition root replacing the bare Logging.Configure(build)
tests/…/LoggerBridgeSpecs.cs (new)	16 bridge specs

Decisions worth reviewing

Not services.AddLogging(...). That installs MEL's own filter pipeline, default minimum Information — a second level authority that would drop trace and debug records before Serilog saw them and displace Logging.LevelSwitch. Registering the Serilog factory directly leaves the level switch as the only gate. The_bridge_does_not_filter_below_information is the regression guard.

The factory is left unbound (SerilogLoggerFactory(logger: null, dispose: false)), because Log.Logger is not stable for the process lifetime — Configure installs it late and Host.WriteErrorsAndWarnings swaps it again for the end-of-build summary. Binding still happens once per logger rather than per write, since the category is attached as SourceContext at construction. Two consequences the code depends on, both documented at the call sites: AddFalloutLogging configures the pipeline before registering the factory, so a container-resolved logger can never bind a stale one; and Logging.Logger is deliberately uncached.

Internal, not public. This is framework foundation, not public surface yet — consistent with the "internal foundation" note in AGENTS.md. The root AssemblyInfo.cs already grants InternalsVisibleTo to Fallout.Cli and the spec assemblies, which covers PR 4's CLI wiring.

Façade over DI, per the issue's decision: the ~85 Log.* call sites and the static BuildManager.Execute<T> are unchanged.

Provider lifetime. The ServiceProvider is declared outside the try so it survives into finally (Finish() still writes the outcome summary), but constructed inside it so a configuration failure returns the same exit code as before.

Test plan

• dotnet build fallout.slnx — 0 errors; the 42 warnings are pre-existing and none are in touched files
• dotnet test fallout.slnx — 830 passed, 7 skipped, 0 failed
• 16 new bridge specs: level mapping (Trace→Verbose … Critical→Fatal), no sub-Information filtering, level-switch gating, message templates staying templates, exceptions reaching LogEvent.Exception, factory not pinned to one pipeline, façade fallback with no container, UseLoggerFactory restore
• Dogfood ./build.ps1 Compile — exit 0, file sinks and rolling cleanup still writing .fallout/temp/build.log, OnBuildFinished extensions still firing after the dispose reordering, console theming unchanged
• git diff reviewed — no public API member added or changed

The bridge specs write through the process-global Log.Logger, so every message carries a marker and collected events are filtered to it; without that, a concurrent spec class's warning lands in the sink and fails an assertion.

Note on labels

Labelled skip-changelog rather than a category: nothing here is consumer-facing. Happy to switch it to enhancement if you'd rather the #428 work show up in the notes as it lands.

Generated with Claude Code

[phmatray]

Couldn't apply labels from the fork (no write access on this repo). Per the PR-creation flow this needs target/vCurrent plus a changelog-category label — skip-changelog is my suggestion, see the note at the end of the description.

[ChrisonSimtian]

thanks for raising this PR, I'll have a look today. I applied those labels for you and approved the workflow run.
Welcome to fallout :-) and might I say, I like your Blazor form library. Helped me out a few times at a previous job ;-)

[ChrisonSimtian]

Good PR. The reasoning is sound, the commit message and description are better than most of what lands here, and the tests are pointed at the right things. Two shape issues and one repo-rule miss below — all small, and I'd like them settled here rather than in PR 2, because 2–4 build directly on this.

What I verified

• The unbound-factory analysis is correct. SerilogLogger binds Log.Logger at construction when logger: null and a category is supplied, and Host.WriteErrorsAndWarnings (src/Fallout.Build/Host.cs:96) reassigns Log.Logger without restoring it — so leaving the factory unbound is genuinely necessary, not defensive.
• Avoiding services.AddLogging(...) is the right call, and The_bridge_does_not_filter_below_information is exactly the guard that keeps someone from "tidying" it back.
• DelegateDisposable.SetAndRestore(() => staticField, ...) matches the existing pattern in this same file (ExecutingTargetLogEventEnricher.SetTargetEventProperty) — idiomatic here.
• InternalsVisibleTo claim checks out: Fallout.Build, Fallout.Build.Specs, and Fallout.Cli are all in the root AssemblyInfo.cs.
• Dispose ordering in the finally is right — the scope is restored before the provider that owns the factory is disposed.
• Tests correctly join ProcessGlobalStateCollection and filter by marker, consistent with InMemorySinkSpecs and the other process-global specs.

One thing that is not yours: Log.CloseAndFlush() ends up closing the errors-and-warnings pipeline rather than the one holding the file sinks, because WriteErrorsAndWarnings swaps Log.Logger during Finish(). Pre-existing — it's what #454 (FT-9) is about. Called out only so it doesn't get attributed to this change later.

Also

docs/dependencies.md needs rows for the new packages — that file asks reviewers to call it out, so consider this the call-out. Microsoft.Extensions.DependencyInjection deserves a sentence of its own: Fallout.Build is consumer-facing, so every consumer now pulls the full container transitively. Sanctioned by #428 ("add refs to Fallout.Build"), just needs to be written down — the doc already makes the same complaint about the Azure packages.

Labels

Applied for you, and skip-changelog was the right instinct — nothing here is consumer-facing. When PR 3 lands the Spectre presenter, that one should be enhancement.

Happy to approve once 1 and 2 are addressed. Neither needs a redesign.

[ChrisonSimtian]

This is the one I'd like changed before merge. AddFalloutLogging mutates process-global state from inside a DI registration method.

TryAdd* is idempotent; Logging.Configure is not. It reassigns Log.Logger unconditionally, every call.

That's harmless today because BuildManager is the only caller. But #428's PR 4 plans to "wire the CLI container to the same registrations" — and Fallout.Cli calling AddFalloutLogging() at container-build time runs Logging.Configure(null), which per the build == null early-returns throughout Logging.cs installs a pipeline with no file sinks, no host sink, and no filter. Whatever pipeline was there gets clobbered, and the already-registered factory won't notice.

Suggest splitting the two concerns:

// registration only — safe to call from any container, any number of times
public static IServiceCollection AddFalloutLogging(this IServiceCollection services)

and leaving Logging.Configure(build) explicit at the BuildManager.Execute call site, immediately before the container is built. Same ordering guarantee you document on CreateSerilogLoggerFactory (pipeline configured before the factory is registered), but it survives a second caller.

[phmatray]

Done in 9dfdf33. AddFalloutLogging now registers only, and never touches the pipeline. BuildManager.Execute calls Logging.Configure(build) explicitly, immediately before it builds the provider:

Logging.Configure(build);
services = new ServiceCollection().AddFalloutLogging().BuildServiceProvider();

The IFalloutBuild parameter is gone from the signature, so PR 4 cannot reach the Configure(null) path you described by accident.

New spec Registering_the_seam_leaves_the_pipeline_alone installs a pipeline, calls AddFalloutLogging on a second container, then asserts Log.Logger is still the same instance. I checked it fails when the Configure call is put back inside the registration.

[ChrisonSimtian]

These two registrations are singletons, which quietly contradicts the invariant the rest of the PR is built on.

Logging.Logger is deliberately uncached — the XML doc explains at length that binding happens once per logger construction, so caching one would strand it on a stale pipeline. But Logger<T> resolves its inner logger once in its constructor, and the non-generic factory lambda runs once per container. Both pin to whatever Log.Logger was current at first resolution.

So a component that resolves ILogger<T> and holds it across the WriteErrorsAndWarnings swap writes into the pre-swap pipeline — the exact failure Logging.Logger is uncached to avoid.

Nothing resolves these yet, so it's latent. What makes it worth fixing now: PR 2 and 3 put IHostTheme and IHostOutput into this same container, and IHostOutput is the component that renders the end-of-build summary — i.e. the one holding an injected logger at precisely the moment the pipeline is swapped.

Either register them transient, or add a note here saying why the pinning is acceptable and what callers must not do. A spec pairing with The_facade_logger_tracks_the_current_pipeline — asserting the container-resolved logger behaves the same way — would lock in whichever answer you pick.

[phmatray]

Agreed, and fixed in 9dfdf33. Both are transient now:

services.TryAddTransient(typeof(ILogger<>), typeof(Logger<>));
services.TryAddTransient(sp => sp.GetRequiredService<ILoggerFactory>().CreateLogger(Logging.DefaultCategoryName));

ILoggerFactory stays a singleton. It is left unbound, so it reads the ambient pipeline every time it creates a logger and pins nothing.

I took your first option and the second one, because transient alone does not close the hole you are pointing at. Each resolution now binds the current pipeline, but a component that resolves once and holds the logger across the WriteErrorsAndWarnings swap still writes into the old pipeline. That is inherent to constructor injection. So the residual constraint is written down at the registration and on CreateSerilogLoggerFactory: anything that outlives a swap must read Logging.Logger at the point of writing rather than hold an injected logger.

That is the rule IHostOutput will have to follow in PR 3. Happy to revisit if you would rather it get something swap-aware instead, but that felt like a design PR 2 and 3 should make, not this one.

Two specs, as you suggested, paired with The_facade_logger_tracks_the_current_pipeline:

• A_container_logger_binds_the_pipeline_current_at_resolution resolves, swaps Log.Logger, resolves again, and asserts the write lands in the new sink.
• The_container_hands_out_a_new_logger_per_resolution covers both registrations.

Both fail if the registrations go back to singleton. I checked.

[ChrisonSimtian]

New transitive dependency on the full MS.DI container for every Fallout.Build consumer. Fine by me — #428 calls for it explicitly — but it needs a row in docs/dependencies.md along with Serilog.Extensions.Logging and Microsoft.Extensions.Logging.Abstractions.

Worth a note on the DI row that the container currently serves one internal composition root, so the footprint is deliberate rather than accidental. The Logging section is the natural home for the two Serilog/MEL entries.

[phmatray]

Added in 9dfdf33. Serilog.Extensions.Logging and Microsoft.Extensions.Logging.Abstractions went into the Logging section, as you suggested.

Microsoft.Extensions.DependencyInjection went into the Microsoft / .NET BCL table, with the footprint note in the Used by column:

Fallout.Build is consumer-facing, so every consumer now pulls the container transitively. Deliberate, not accidental. It currently serves one internal composition root, and the plugin foundation (milestone #6) is what will use it more widely.

[ChrisonSimtian]

@phmatray just checking, did you intentionally open this PR and are you genuinely interested in contributing? Or was that your AI? Just wanna know if you'll actually read the code review or if we take it from here :-)

[phmatray]

@ChrisonSimtian Thanks for applying the labels and approving the workflow run, and glad FormCraft was useful to you. Happy it helped :-)

To answer directly: yes, I opened this PR intentionally and I'm genuinely in. I found Fallout while digging through NUKE issues (I use NUKE on nearly all my repos) and I'm curious to see where this fork goes. The MCP integration idea in particular appeals to me a lot.

It's driven through my own Claude skill kit, but I'm the one steering it. I'll read the code review and finish the work. Fire away.

[phmatray]

@ChrisonSimtian Thanks for the review. All three points are addressed in 9dfdf33, with replies in each thread. The branch is also synced with develop, which merged clean.

Point	What changed
1. AddFalloutLogging mutates global state	Split. It registers only. BuildManager.Execute calls Logging.Configure(build) explicitly before building the provider. The IFalloutBuild parameter is gone from the signature.
2. Singleton registrations pin the pipeline	ILogger<T> and ILogger are transient. ILoggerFactory stays a singleton, since it is unbound and pins nothing. The residual constraint is documented: a component that outlives a pipeline swap must read Logging.Logger at the point of writing.
3. docs/dependencies.md	Rows for all three packages, with the transitive-footprint note on the DI row.

Also took CodeRabbit's nitpick: StubLoggerFactory.CreateLogger returns NullLogger.Instance instead of throwing. AddProvider still throws, since the seam never calls it.

Verification

• dotnet build fallout.slnx: 0 errors, no warnings in the touched files.
• dotnet test fallout.slnx: 833 passed, 7 skipped, 0 failed.
• ./build.ps1 Compile: exit 0. File sinks still write to .fallout/temp/build.log, and the OnBuildFinished extensions still fire.

Three specs were added, one per behaviour. I checked each one fails when its change is reverted, so they guard rather than just pass:

• Registering_the_seam_leaves_the_pipeline_alone fails if Configure goes back inside the registration.
• A_container_logger_binds_the_pipeline_current_at_resolution and The_container_hands_out_a_new_logger_per_resolution fail if the registrations go back to singleton.

One thing worth your call

On point 2, transient fixes the resolution, not the holding. A component that resolves a logger once and keeps it across the WriteErrorsAndWarnings swap still writes into the old pipeline. I documented the rule instead of building an indirection this PR was not asked to build, but IHostOutput in PR 3 is exactly the component that hits it. Say the word if you would rather it get something swap-aware, and I will design that in PR 2 or 3 rather than here.

Noted on the Log.CloseAndFlush ordering being #454 and not mine, and on enhancement for PR 3's label.