Pipeline Validation At Startup

[iancooper]

We lack validation of our pipelines and the ability to discern what they do easily for diagnostics.

We need both static analysis, with the ability to request validation and receive a report, that follows a similar model to validate methods for ASP.NET or describe other .NET event processing frameworks, and dynamic analysis that extends our recent Roslyn Analyser additions.

This remains one of the weakest parts of our offering, leading to criticism that it is difficult to set Brighter up.

This will close #2176

[claude]

PR Review: Pipeline Validation At Startup (Draft)

This is a design-phase PR — spec files and ADR only, no implementation code yet. My review focuses on the architecture, design decisions, and potential issues to address before or during implementation.

---

Overall Assessment

This is a well-thought-out ADR addressing a real pain point for Brighter users. The layered architecture mirrors Brighter's own configuration paths, the opt-in approach preserves backward compatibility, and the use of the existing Specification<T> pattern is a natural fit. The requirements document is thorough with clear acceptance criteria.

---

Design Issues to Resolve Before Implementation

1. Generic ResolveMapperInfo<TRequest>() vs. Runtime Type — Design Tension

The ADR proposes MessageMapperRegistry.ResolveMapperInfo<TRequest>() as a generic method, but the validator operates on Type at runtime (it iterates IEnumerable<Type> from GetHandlerTypes()). Calling a generic method with a runtime type requires reflection — which undermines the clean design.

Suggestion: Make the methods accept Type directly and have the generic overload delegate:

public (Type? mapperType, bool isDefault) ResolveMapperInfo(Type requestType)
public (Type? mapperType, bool isDefault) ResolveAsyncMapperInfo(Type requestType)

This deserves explicit resolution in the ADR before the tasks file is written.

2. Describe() on a Generic Class Ignores Its Type Parameter

PipelineBuilder<TRequest>.Describe(Type requestType) "ignores TRequest" — this is awkward API design. If the method ignores the type parameter, it likely belongs on a separate non-generic interface or internal utility rather than on the generic builder. The new internal describe-only constructor hinted at in the ADR may warrant a dedicated type entirely.

3. Thread Safety of _allHandlerTypes Dictionary

The proposed SubscriberRegistry._allHandlerTypes is a plain Dictionary<Type, HashSet<Type>>. If registration is strictly single-threaded (DI registration phase only), this is fine — but this assumption should be documented. The existing static caches in PipelineBuilder use ConcurrentDictionary; a brief note explaining why _allHandlerTypes does not need the same treatment would help future maintainers.

4. Hosted Service Coordination Not Fully Specified

The mitigation for double-running — "Use a shared IAmAValidationState singleton that tracks whether validation has already run" — is mentioned but not designed. IHostedService startup order is deterministic (registration order), so consider whether a simpler ordering guarantee suffices: register BrighterValidationHostedService first and have ServiceActivatorHostedService check a shared flag before running. The chosen mechanism should be specified in the ADR before implementation.

5. AttributeAsyncConsistency Rule — Inconsistent Examples

The rule appears twice with inconsistent scope:

• Detailed HandlerPipelineValidationRules example: checks only d.BeforeSteps
• Validation rules summary section: checks d.BeforeSteps.Concat(d.AfterSteps)

After-steps can also be misconfigured. The broader form is more correct; the examples should be made consistent before the tasks file is written.

6. IsBackstopAttribute() / IsResilienceAttribute() Helpers Undefined

The ADR references these helpers without specifying them. This is a meaningful design decision:

• Hardcoded list of known Brighter attribute types?
• Marker interface on RequestHandlerAttribute subclasses?
• Convention-based (e.g. naming)?

Third-party resilience attributes (custom Polly wrappers) may need to be recognized. The approach should be decided and documented in the ADR.

7. PumpHandlerMatch Rule — Empty Collection Edge Case

MessagePumpType.Reactor => handlerTypes.All(t => typeof(IHandleRequests).IsAssignableFrom(t)),

Enumerable.All() returns true on an empty collection, so this rule vacuously passes when no handlers are registered. The HandlerRegistered rule catches that case — the interaction is logically correct but non-obvious. A comment in the implementation explaining the intentional dependency between these two rules would help future maintainers.

---

Minor Points

BackstopAttributeOrdering severity is Warning — given that a backstop placed after a resilience pipeline is almost certainly a bug, consider whether Error is more appropriate. At minimum, document why Warning was chosen over Error.

RequestTypeSubtype null guard — s.DataType == null || ... guards against null DataType, but the Subscription constructor already validates this. If kept, add a comment explaining the defensive intent.

Missing trailing newlines — both .agent_instructions/design_principles.md and specs/0023-Pipeline-Validation-At-Startup/requirements.md end with \ No newline at end of file. Minor, but worth fixing.

README status checklist out of sync — specs/0023-Pipeline-Validation-At-Startup/README.md shows Requirements and Design as unchecked, but .requirements-approved and .design-approved marker files are both present in this PR. The checklist should reflect the actual state.

---

Pre-existing Bug (Noted in ADR)

The double AddGlobalInboxAttributesAsync() call in PipelineBuilder.BuildAsyncPipeline() is correctly identified. Per the Tidy First principle, this should be committed separately from the validation feature — good that the ADR calls this out explicitly.

---

What Looks Good

• Layered architecture cleanly mirrors Brighter's three configuration paths
• Opt-in via IBrighterBuilder extension methods is the right integration point
• Reusing Specification<T> from the codebase is an elegant fit; moving it to Paramore.Brighter makes sense
• ValidationRule<T> cleanly separates predicate from reporting metadata
• Dry-run reusing the existing attribute caches is a smart performance choice
• PipelineValidationResult.ThrowIfInvalid() with aggregate reporting satisfies FR-10 cleanly
• Keeping consumer rules in Paramore.Brighter.ServiceActivator (not core) is the right layering decision
• Breaking-change risk for IAmASubscriberRegistry.GetHandlerTypes() is correctly identified with a concrete mitigation (default interface method or new inspector interface)
• Good prior art survey (ASP.NET Core, MassTransit, Wolverine, Rebus) grounding the design in ecosystem conventions

---

Since this is a draft, the design questions above are the right things to resolve before moving to the tasks phase. Happy to discuss any of these in more detail.

[iancooper]

@claude We have considered and rejected point 2: "Describe() on a Generic Class Ignores Its Type Parameter
PipelineBuilder.Describe(Type requestType)"

Two issues:

(1) Moving Describe to a separate class carries the risk of model drift between the two. This approach ensures that the path used to build pipelines matches the one we describe.
(2) We do not have an instance of requestType at the point at which we evaluate.

Both these lead us to prefer adding a non-generic method that takes the type

[claude]

Claude finished @iancooper's task in 32s —— View job

---

PR Review: Pipeline Validation At Startup (Draft)

• Analyze design feedback response from Ian Cooper
• Update review to reflect accepted/rejected decisions

---

Response on Point 2: Describe() Design

Acknowledged and the reasoning is sound. To summarise the accepted decision for the record:

Describe(Type requestType) stays on PipelineBuilder<TRequest> as a non-generic method that accepts Type. The two reasons for keeping it co-located rather than extracting to a separate class:

1. Drift prevention — the describe path and the build path share the same code, guaranteeing they stay in sync as the pipeline evolves.
2. No runtime instance — the validator operates on Type objects at registration time, not on request instances, so a generic constraint buys nothing here.

Adding a non-generic Describe(Type requestType) on the builder resolves the awkwardness I flagged — the generic TRequest is still used for the actual build path, while introspection uses the Type parameter. That's a clean resolution.

---

Remaining Open Design Questions

The other points from my original review that are still worth resolving before the tasks file is written:

1. ResolveMapperInfo<TRequest>() vs runtime Type — the generic-vs-Type tension in MessageMapperRegistry. Suggest ResolveMapperInfo(Type requestType) with the generic overload delegating.
2. Thread safety of _allHandlerTypes — document the single-threaded assumption (or use ConcurrentDictionary for consistency with existing static caches in PipelineBuilder).
3. Hosted service coordination — the IAmAValidationState singleton approach needs to be specified before implementation.
4. AttributeAsyncConsistency rule — inconsistent examples — BeforeSteps only vs BeforeSteps.Concat(AfterSteps).
5. IsBackstopAttribute() / IsResilienceAttribute() helpers — the recognition strategy (hardcoded list, marker interface, or convention-based) should be decided before implementation.

Minor items (trailing newlines, README checklist sync) are low priority and can be fixed as part of implementation.

---

[claude]

Code Review: Pipeline Validation At Startup (ADR-0053 + ADR-0054)

This is a design/specification PR (ADRs, requirements, spec tracking). No implementation code is included. Feedback is on the architectural design and completeness.

---

Overall Assessment

The design is well-reasoned and addresses a real pain point. The layered architecture mirrors Brighter's own three configuration paths, the Specification + ValidationRule pattern is idiomatic, and the opt-in integration via IBrighterBuilder is clean. The hosted-service coordination mechanism is elegant. Several design gaps and wording issues should be resolved before implementation begins.

---

Design Gaps

1. How does PipelineValidator enumerate all handler types? (Critical)

ADR-0053 shows:

foreach (var description in _pipelineBuilder.Describe())

But PipelineBuilder<TRequest> is a generic class — you need one instance per TRequest. The ADR doesn't explain how the validator gets descriptions for all registered request types. The subscriber registry knows which types are registered; the validator would need to iterate SubscriberRegistry and invoke Describe(Type requestType) per type, likely via a non-generic factory or aggregator. This needs to be spelled out — it affects the PipelineBuilder constructor signature, the Describe() signature, and the validator's constructor dependencies.

2. Sync/async attribute rule may over-fire on custom handlers

The rule:

d.BeforeSteps.Concat(d.AfterSteps).All(step =>
{
    var stepIsAsync = typeof(IHandleRequestsAsync).IsAssignableFrom(step.HandlerType);
    return d.IsAsync == stepIsAsync;
})

For steps whose handler type implements neither IHandleRequests nor IHandleRequestsAsync (e.g. a custom cross-cutting handler that doesn't inherit from either base), stepIsAsync would be false and the rule would flag sync handlers as problematic. The rule should skip steps that implement neither interface rather than treating them as implicitly sync.

3. Backstop ordering rule message doesn't identify which attribute is misordered

The check fires if any backstop step > any resilience step but the error is reported at handler level. Consider a pipeline with backstops at steps 1 and 5, and resilience at step 3 — the step-5 backstop is misordered but step-1 is fine. The error Source and Message should identify which backstop is problematic (e.g. "Handler 'X' — 'RejectMessageOnErrorAsync' at step 5 is after 'UseResiliencePipelineAsync' at step 3"), which also requires the rule to yield multiple errors per handler rather than one.

4. Exception type for startup failure

ThrowIfInvalid() throws AggregateException wrapping ConfigurationException. Callers already catching ConfigurationException (the existing Brighter convention) won't catch an AggregateException. Consider a dedicated PipelineValidationException : ConfigurationException that holds the PipelineValidationResult directly:

// Callers can catch ConfigurationException and unwrap details directly
throw new PipelineValidationException(result);

5. Specification<T> move — breaking-change analysis missing

The ADR says the move "requires a using update" for existing consumers. But these are public NuGet types — any user code importing Paramore.Brighter.Mediator and referencing ISpecification<T> or Specification<T> will break at compile time if the namespace changes. The ADR should confirm: (a) the namespace stays the same and only the assembly moves, (b) type-forwarding shims are provided, or (c) this is acknowledged as a minor breaking change in V10.x.

6. BrighterValidationHostedService as a permanent no-op in full-messaging scenarios

When AddConsumers() is used, BrighterValidationHostedService is registered but resolves IDispatcher and exits — a permanently idle hosted service. Functionally correct, but potentially surprising during diagnostics. An alternative: AddConsumers() could suppress the registration of BrighterValidationHostedService at build time, or the hosted service could log a trace-level message explaining why it's a no-op.

---

Correctness Concerns

7. Validation rule message wording is ambiguous

The rule fires when backstop.Step > resilience.Step with message "so it will never execute on failure." Without knowing Brighter's step ordering convention (lower = outer vs lower = inner), this message could be read backwards. The message should make the ordering semantics explicit, e.g.: "In Brighter, lower Step values are outer wrappers. BackstopHandler at step {b.Step} is inner of ResilienceHandler at step {r.Step}, so exceptions are handled by resilience before the backstop can act."

8. FR-3 (missing mapper for published type) not covered in rule design

The requirements list FR-3: "No mapper registered for a published type" as a validation target. The ADR's ProducerValidationRules.Rules() doc comment only mentions FR-4: Publication.RequestType validation. The missing-mapper check doesn't appear in the rule design. Confirm this is intentionally deferred or add it to the rule set with the relevant FR reference.

---

Minor Issues

9. ValidationRule<T>.Evaluate() returns null on success

Returning null from Evaluate() when the rule passes leads to null-check boilerplate at every call site. Consider IEnumerable<PipelineValidationError> Evaluate(T entity) yielding zero or one errors — cleaner to compose with LINQ and removes null propagation.

10. ADR-0054 status inconsistency with PR scope

ADR-0053 is "Accepted" and ADR-0054 is "Proposed". If ADR-0054 is a follow-up, it should be in a separate PR to keep scope clear. If it is intended to be accepted here, its status should be updated before merge.

11. Spec README.md checklist not updated

specs/0023-Pipeline-Validation-At-Startup/README.md has all checklist items unchecked. Since requirements and design are complete, the checklist should reflect current state.

---

What's Well Done

• The layered design exactly mirrors the three configuration paths — clean and consistent with Brighter's existing structure.
• Marker interfaces (IAmABackstopHandler, IAmAResilienceHandler) as the extension point is idiomatic Brighter and avoids brittle hardcoded attribute name checks. Third-party attributes can participate automatically.
• DI-presence-based coordination between hosted services avoids shared mutable state — a genuinely elegant solution.
• DescribePipelines() at Information/Debug levels integrates with any logging provider and compares well to Wolverine/Rebus/MassTransit prior art.
• The getRequestType / MapRequestType suppression in BRT008 correctly handles dynamic dispatch without false positives.
• The test strategy in ADR-0054 covers the key edge cases thoroughly (dynamic pump type, both interfaces implemented, no handler found, static vs dynamic request type).
• The separation of runtime validation (ADR-0053) from static Roslyn analysis (ADR-0054) keeps each concern focused and allows them to be implemented independently.

[claude]

PR Review: Pipeline Validation At Startup (ADR-0053 + ADR-0054)

This is a design/documentation PR — two new ADRs plus the spec structure. No implementation code yet. Overall this is a well-thought-out design with good separation of concerns. A few issues worth addressing before or during implementation.

---

Strengths

• Layered composition mirrors Brighter's own three configuration paths cleanly — each layer validates its own concerns and they compose naturally.
• Opt-in design preserves full backward compatibility.
• Dry-run model as single source of truth — both validator and writer consume the same HandlerPipelineDescription, preventing drift between what's reported and what's built.
• Hosted service coordination via optional DI resolution is elegant and avoids shared mutable state.
• Moving Specification<T> to Paramore.Brighter core is a sound Tidy First move — it had no inherent dependency on Mediator.
• Pre-existing bug fix (AddGlobalInboxAttributesAsync called twice) is correctly identified and flagged as a separate tidy commit.
• Dynamic dispatch suppression in BRT008 (detecting getRequestType lambda to avoid false positives) is a thoughtful edge case.
• Excellent coverage of prior art (ASP.NET Core ValidateOnBuild, Wolverine, Rebus, MassTransit).

---

Issues

1. InvalidOperationException thrown inside a Specification<T> predicate (design correctness)

The AttributeAsyncConsistency rule throws from inside IsSatisfiedBy():

if (!isSync && !isAsync)
    throw new InvalidOperationException(
        $"Pipeline step handler type '{step.HandlerType.FullName}' implements neither...");

Specifications are expected to return a boolean, not throw. If a corrupted/unusual handler type reaches this check, startup crashes with an unhandled exception rather than returning a structured validation error. Either:

• Catch this in ValidationRule<T>.Evaluate() and return it as an Error-severity finding, or
• Add a guard rule that runs first and validates handler types are well-formed before this rule runs.

Throwing inside a predicate also makes the rule non-composable (you can't safely use And()/Or() around it).

2. ThrowIfInvalid() discards the Source field from the exception message

throw new AggregateException(
    "...",
    Errors.Select(e => new ConfigurationException(e.Message)));

e.Source (e.g. "Handler 'OrderCreatedHandler'") is not included in the ConfigurationException message. A developer reading the exception chain in logs would lose the handler/subscription context. Consider: new ConfigurationException($"[{e.Source}] {e.Message}").

3. Breaking interface change on IAmASubscriberRegistry — mitigation not decided

The ADR identifies the risk but leaves the resolution open:

Add it as a default interface method that returns empty, or add it to a new IAmASubscriberRegistryInspector interface.

This needs to be decided before implementation starts. A default interface method returning empty collections is the least-breaking option, but it means custom registry implementations silently skip validation without any indication. A separate opt-in interface with explicit DI registration is cleaner. Recommend deciding this in the ADR before marking it "Accepted."

4. Error message doesn't identify which attribute is mismatched

For AttributeAsyncConsistency, the message is:

"Async handler has sync pipeline attributes — they will be silently ignored"

When a handler has 5 attributes and one is wrong, the developer has to check all of them. The message should name the specific attribute type causing the mismatch, e.g.:

"Async handler 'OrderCreatedHandler' uses sync attribute '[RejectMessageOnError]' at step 2 — it will be silently ignored"

This is the same detail BRT007 provides in the Roslyn diagnostic (messageFormat includes the attribute name). The runtime validation message should match that quality.

5. Describe(Type requestType) ignores the class's TRequest parameter

The ADR justifies keeping it on PipelineBuilder<TRequest> for drift prevention and cache sharing, which are valid reasons. However Describe(Type requestType) is then a non-generic method on a generic class that accepts any Type, not just TRequest. This will confuse callers who instantiate PipelineBuilder<OrderCreated> and call Describe(typeof(PaymentReceived)).

Consider making the describe-only constructor produce an unbound PipelineBuilder<IRequest> or using a companion non-generic class that still delegates to the same caches. The ADR's rationale is sound, but this warrants an explicit code comment in the implementation explaining why requestType is not constrained to TRequest.

6. ResolveMapperInfo — MakeGenericType can throw

if (_defaultMessageMapper != null)
    return (_defaultMessageMapper.MakeGenericType(requestType), true);

MakeGenericType throws ArgumentException if _defaultMessageMapper is not a generic type definition, or InvalidOperationException if constraints are violated. The ADR doesn't mention error handling here. This should either be guarded with IsGenericTypeDefinition or exceptions should be caught and surfaced as validation errors rather than crashing startup.

7. Spec README checklist is stale

specs/0023-Pipeline-Validation-At-Startup/README.md has all items unchecked, but .requirements-approved and .design-approved files exist in the same directory. The checklist should reflect current status.

8. Consumer validation: mapper coverage is implicit, not listed as a rule

The validation rule details section (under "AddConsumers Rules") covers PumpHandlerMatch, HandlerRegistered, and RequestTypeSubtype, but does not include a mapper validation rule for subscriptions. The diagnostic report shows mapper info per subscription, implying the mapper should be validated too. If mapper validation is intentionally deferred, the ADR should say so explicitly and reference the relevant FR number.

---

Minor observations

• BrighterValidationHostedService is always registered when ValidatePipelines() is called, even in consumer apps where it becomes a guaranteed no-op. This is harmless but slightly wasteful. An alternative: don't register BrighterValidationHostedService at all in consumer apps (check for IDispatcher registration at ValidatePipelines() call time, not at startup). That said, the current approach is simpler and the startup cost is negligible.

• ValidationRule<T> recreates Specification<T> instances on every Rules() call (since it's a yield return static method). For startup-time-only validation this is fine, but worth noting if Rules() is ever called outside of startup.

• ADR-0054 BRT008 performance: The compilation-wide type search for handler types is noted as "bounded but not zero cost." Worth explicitly stating whether results are cached per compilation or re-computed for each subscription creation found — Roslyn analysis actions can be called many times during incremental compilation. If not cached, this could hurt IDE responsiveness in large projects.

• RequestTypeSubtype rule allows s.DataType == null to pass — this seems intentional (null is covered by HandlerRegistered), but a comment in the ADR would help future maintainers understand why null is not flagged here.

---

Summary

The design is solid and the ADR is thorough. Items 1 (exception in predicate), 2 (source lost in exception), and 3 (breaking interface mitigation) should be resolved before the implementation tasks are written. Items 4–6 are implementation-level details that can be addressed in code comments or during implementation. The rest are minor.

[claude]

PR Review: Pipeline Validation At Startup

This is a well-designed, spec-driven feature backed by ADRs 0053 and 0054. The architecture is clean, validation is genuinely opt-in, and the IAmA* naming convention is followed consistently. All 22 tasks in the spec are checked off and test coverage is broad. The notes below are intended to strengthen the implementation before merge.

---

🔴 High Priority

1. BrighterValidationHostedService — nullable constructor parameter breaks DI

When ValidatePipelines() is called without DescribePipelines(), IAmAPipelineDiagnosticWriter is never registered. Because BrighterValidationHostedService takes it as a constructor parameter (even though it is nullable), Microsoft.Extensions.DependencyInjection will throw at service provider build time — it does not support optional constructor injection.

ADR-0053 explicitly calls this out: "Microsoft.Extensions.DependencyInjection does not support optional constructor injection, so optional dependencies must be resolved explicitly from IServiceProvider." ServiceActivatorHostedService correctly follows this guidance. BrighterValidationHostedService does not.

Fix: Inject IServiceProvider and resolve IAmAPipelineDiagnosticWriter? via GetService<>() in StartAsync, matching the pattern used in ServiceActivatorHostedService.

---

2. Double Describe() call when both ValidatePipelines() and DescribePipelines() are used

When both extension methods are called (the common pattern shown in the samples), both BrighterValidationHostedService and BrighterDiagnosticHostedService are registered. BrighterValidationHostedService calls _diagnosticWriter?.Describe() — and so does BrighterDiagnosticHostedService for the non-consumer path (ConsumerOwnsValidation == false). The diagnostic report will be logged twice.

Fix: Remove the _diagnosticWriter?.Describe() call from BrighterValidationHostedService and let BrighterDiagnosticHostedService own all describe behaviour. Or guard BrighterDiagnosticHostedService so it is a no-op when BrighterValidationHostedService has already run.

---

🟡 Medium Priority

3. ValidatePipelines() hardcodes ServiceCollectionSubscriberRegistry

var subscriberRegistry = sp.GetRequiredService<ServiceCollectionSubscriberRegistry>();

This resolves the concrete class rather than an interface. Any application using a custom IAmASubscriberRegistry that also implements IAmASubscriberRegistryInspector will get a DI exception here. The ADR states custom registries must implement IAmASubscriberRegistryInspector, but the code does not try to resolve IAmASubscriberRegistryInspector directly — it requires the concrete type.

Fix: Resolve IAmASubscriberRegistryInspector first and fall back to ServiceCollectionSubscriberRegistry only if needed, or document clearly that custom registries must be registered both as themselves and as IAmASubscriberRegistryInspector.

---

4. MapperMethodDiscovery — SingleOrDefault! changes behaviour vs original Single()

All four methods in MapperMethodDiscovery apply the null-forgiving operator to SingleOrDefault(...):

return mapperType.GetMethods()
    ...
    .SingleOrDefault(...)!;

The original instance methods on TransformPipelineBuilder used Single(), which throws InvalidOperationException on no match. SingleOrDefault()! returns null (suppressing the compiler warning) and may produce a NullReferenceException at an unexpected call site. The callers in DescribeTransforms guard correctly, but callers in the existing Build path may now get NREs where they previously got a clear InvalidOperationException.

Fix: Either keep Single() for the build-path callers and use SingleOrDefault() (without !) for the describe-path callers, or handle null explicitly at all call sites.

---

🔵 Low Priority

5. BrighterPipelineValidationOptions — duplicated sentence in XML doc

/// Set automatically
/// Set automatically by <c>AddConsumers()</c>.

"Set automatically" appears twice. Minor copy-paste artifact.

---

6. AttributeAsyncConsistency uses base class check instead of interface check

The rule checks typeof(RequestHandler<>) and typeof(RequestHandlerAsync<>) rather than IHandleRequests / IHandleRequestsAsync as described in the ADR. Third-party pipeline steps that implement the handler interfaces without extending the base classes will not be correctly classified. Low practical impact today, but inconsistent with the ADR and less extensible.

---

7. IAmASubscriberRegistryInspector returns Type[]

Type[] GetHandlerTypes(Type requestType);
Type[] GetRegisteredRequestTypes();

Returning IReadOnlyCollection<Type> or IEnumerable<Type> would be more idiomatic for a public interface and give implementors more flexibility. Since this is new API surface, it is worth getting right before the public release.

---

8. MyCommandHandler visibility changed to public

MyCommandHandler in CommandProcessors/TestDoubles is changed from internal to public to avoid triggering the HandlerTypeVisibility rule in validation tests. A dedicated test double in the validation tests namespace would avoid modifying the shared command processor doubles and preserve test isolation.

---

ℹ️ Informational

9. ServiceCollectionBrighterBuilder — Brighter assembly scanning moved from constructor to factory method

The constructor previously scanned Brighter assemblies for built-in handlers/mappers/transforms. This is now moved to the BrighterHandlerBuilder() factory method. Code that constructs ServiceCollectionBrighterBuilder directly (e.g., in tests or custom integrations bypassing BrighterHandlerBuilder()) will no longer auto-register built-in handlers. This is a subtle behavioural change worth documenting in the PR notes or changelog.

---

Test Coverage Gaps

The validation unit tests are thorough. The following integration-level scenarios are not covered:

• Calling ValidatePipelines() without DescribePipelines() — would surface issue Support for multiple Application Layer Protocols in Task Queues #1 at test time rather than in production.
• Calling both ValidatePipelines() and DescribePipelines() together — would expose the double-describe issue (No API Documentation #2).
• The ConsumerOwnsValidation handoff between BrighterValidationHostedService and ServiceActivatorHostedService in a full hosted-service integration test.

---

Performance & Security

No concerns. Validation runs once at startup using reflection over type metadata only (no message payloads, no user input). The _lastResults caching in Specification<T> and the static memento caches in PipelineBuilder keep startup overhead minimal.

---

Overall this is a solid, well-thought-out feature. The two high-priority items (optional DI dependency and double-describe) are the main things to address before merge. The rest are quality improvements worth considering given that IAmASubscriberRegistryInspector is new public API.

[claude]

PR Review: Pipeline Validation At Startup - see full review below

[claude]

Review: Pipeline Validation At Startup

This is a well-designed feature addressing a real pain point. The layered architecture mirrors the three Brighter configuration paths, the ADR is excellent, and test coverage is comprehensive. Below are findings organised by severity.

Bugs and Correctness

1. Unused _serviceProvider in BrighterValidationHostedService

The field is injected and stored but never used in StartAsync. The constructor XML doc says optional dependencies (IAmAPipelineDiagnosticWriter) are resolved from IServiceProvider at startup but the code does not do this. Either remove the parameter or resolve and call the diagnostic writer here for the non-consumer path. Currently BrighterDiagnosticHostedService and BrighterValidationHostedService are fully independent — a user must call both DescribePipelines().ValidatePipelines() to get both behaviours.

2. Async detection inconsistency between validation rules

HandlerPipelineValidationRules.AttributeAsyncConsistency checks typeof(IHandleRequestsAsync).IsAssignableFrom(step.HandlerType), while ConsumerValidationRules.IsAsyncHandler walks the base type chain for RequestHandlerAsync<>. These may disagree for a handler that implements IHandleRequestsAsync<T> directly without inheriting from RequestHandlerAsync<T>. Recommend consolidating to HandlerMethodDiscovery.IsAsyncHandler() which is already used in PipelineBuilder.Describe().

3. BackstopAttributeOrdering warning message is inaccurate

The message "will never execute on failure" is not correct. When a backstop has a higher step number than a resilience handler, the backstop IS inner and DOES execute on failure — it fires on each individual attempt before resilience has exhausted its retries. The real problem is it acts too early. Suggest: [backstop] at step N is inside [resilience] at step M — the backstop will fire on each retry attempt rather than after resilience exhausts all retries.

Breaking Changes

4. ServiceActivatorHostedService constructor is a breaking change

The constructor went from 2 parameters (logger, dispatcher) to 4 (adding IServiceProvider and IOptions<BrighterPipelineValidationOptions>). Any code that manually constructs this class will fail to compile. The new test When_service_activator_single_constructor_should_resolve_optional_deps.cs tests the new signature but not backward compatibility. Consider a secondary constructor or a release notes callout.

Design Observations

5. BrighterPipelineValidationOptions leaks an internal coordination concern

ConsumerOwnsValidation is set exclusively by AddConsumers() with no legitimate reason for end users to configure it. Exposing it as a public options class leaks an implementation detail. Consider making the class internal.

6. Duplicate subscriber registry resolution pattern

The fallback cast via GetRequiredService<ServiceCollectionSubscriberRegistry>() appears in three places (ValidatePipelines, DescribePipelines, RegisterConsumerValidationSpecs). Since ServiceCollectionExtensions already registers IAmASubscriberRegistryInspector via TryAddSingleton, the fallback should never be reached. Either remove it or extract a helper.

7. PipelineBuilder<IRequest> as a describe-only builder is a type-parameter workaround

Using TRequest = IRequest in new PipelineBuilder<IRequest>(subscriberRegistry) is meaningless for description. A dedicated non-generic PipelineDescriptor class would make the intent clearer and avoid confusion about why Build() would fail on this variant.

8. AndNot/OrNot on Specification<T> bypass the visitor

These return lambda-only Specification<T> instances with no _errorFactory or _resultEvaluator, so the visitor returns empty results. If used in validation rule composition, errors will be silently swallowed. Worth documenting.

Missing Coverage

9. Mapper registration not validated

The ADR identifies "No mapper registered for a published type" as a startup-discoverable error (ArgumentOutOfRangeException at send time). ProducerValidationRules only validates RequestType set and implements-IRequest; mapper registration is not checked. A TODO comment in ProducerValidationRules would capture this as a known gap.

10. BackstopAttributeOrdering only checks BeforeSteps

Both backstops and resilience collections are drawn from d.BeforeSteps only. If intentional (different semantics for after-steps), a comment explaining the rationale would help.

Minor Notes

• Moving assembly scanning from the ServiceCollectionBrighterBuilder constructor to BrighterHandlerBuilder() is a good testability fix but ideally lives in a separate commit (tidy-first).
• IOptions<BrighterPipelineValidationOptions> resolution is safe in generic host apps but could surprise minimal test setups where AddOptions() has not been called transitively.

Summary

Priority items before merge:

1. Remove unused _serviceProvider from BrighterValidationHostedService or implement the optional describe call it was intended for
2. Address the breaking constructor change on ServiceActivatorHostedService
3. Fix the async detection inconsistency between handler and consumer validation rules
4. Correct the backstop ordering warning message

Excellent ADR documentation and test coverage throughout.

[claude]

PR Review: Pipeline Validation At Startup

This is a well-designed and well-tested feature. The architecture is sound, the ADR is thorough, and the test coverage is extensive. Below are observations ranging from bugs to minor nits.

---

Issues

1. PipelineDiagnosticWriter.LogSubscriptions is less detailed than the ADR specifies

The ADR design shows subscriptions should include handler type, pipeline chain, and mapper info. The actual LogSubscriptions only logs ChannelName and RoutingKey — handler, pipeline chain, and mapper info are absent. Since subscriptions are the most operationally useful part of the diagnostic output (pump/handler mismatch is the most common misconfiguration), this gap is worth tracking. If deferred intentionally, please add a TODO comment.

2. Mapper registration validation is absent

The ADR explicitly lists "No mapper registered for a published type (causes ArgumentOutOfRangeException at send time)" as a key problem to catch. ProducerValidationRules only validates RequestType null/type checks — there is no rule checking whether a mapper is actually registered for Publication.RequestType. The diagnostic writer can describe mapper resolution, but the validator never enforces it. Is this deferred intentionally?

3. AndNot loses error context from the negated specification

In AndSpecification<T>:

public ISpecification<T> AndNot(ISpecification<T> other)
    => new AndSpecification<T>(this, new Specification<T>(x => !other.IsSatisfiedBy(x)));

The negated arm wraps other.IsSatisfiedBy in a pure-predicate Specification<T> (no _errorFactory, no _resultEvaluator). When this compound spec fails and the visitor is called, the negated arm's LastResults is always [] — validation error detail from other is silently dropped. Contrast with Not(Func<T, ValidationError> errorFactory) which uses NotSpecification<T> and carries its own results. The current rules don't use AndNot, but this is a latent bug if the rule set expands.

---

Design Observations

4. Specification<T>._lastResults statefulness — future-proofing concern

The ADR notes this is safe for single-threaded startup validation, which is true today. _lastResults is a bare field with no volatile, no lock, and no warning. If specs are ever reused in a multi-threaded context (e.g. a rule shared across parallel health checks), the IsSatisfiedBy → Accept pair will race. A brief class-level comment noting "not thread-safe; intended for single-threaded startup validation only" would protect future maintainers.

5. RegisterConsumerValidationSpecs resolves IAmASubscriberRegistryInspector redundantly

The fallback cast pattern (IAmASubscriberRegistryInspector)sp.GetRequiredService<ServiceCollectionSubscriberRegistry>() appears three times across the codebase (RegisterConsumerValidationSpecs twice, BrighterPipelineValidationExtensions once). Since ServiceCollectionSubscriberRegistry now explicitly implements IAmASubscriberRegistryInspector, registering it as that interface directly in ServiceCollectionBrighterBuilder would eliminate all three fallbacks.

6. ValidationResultCollector<T> namespace mismatch

ValidationResultCollector<T> lives in Paramore.Brighter, while all the validation domain types (PipelineValidator, PipelineValidationResult, PipelineValidationException, ValidationError, ValidationSeverity) live in Paramore.Brighter.Validation. The collector is the glue between the spec pattern and the validation domain — it belongs with the other validation types in Paramore.Brighter.Validation.

7. Dual hosted-service registration with silent no-op deserves a comment

When AddConsumers() is called after ValidatePipelines(), both BrighterValidationHostedService and ServiceActivatorHostedService are registered. The former silently becomes a no-op because ConsumerOwnsValidation = true. A comment in BrighterValidationHostedService.StartAsync — e.g. // When ConsumerOwnsValidation=true, ServiceActivatorHostedService handles validation — see AddConsumers() — would make the intent clear to future readers.

---

Nits

• ServiceActivatorHostedService.cs lost its #region Licence header in this PR. All other new files include the standard MIT license header block.
• In ConsumerValidationRules.PumpHandlerMatch, the null-forgiving s.RequestType! in the error factory is correct (the predicate already guards null), but a brief inline comment would make it explicit.
• PipelineValidator.EvaluateSpecs creates one ValidationResultCollector<T> per call, but it's effectively stateless (delegates to each spec's LastResults). A comment clarifying this would explain why it's safe to share across specs in the loop.

---

What's working well

• Test coverage: 50+ focused test files, each testing exactly one behaviour, using well-named test doubles. SpyLogger and SpyPipeline* doubles are cleanly shared across both test projects.
• Exception hierarchy: PipelineValidationException : ConfigurationException ensures existing catch blocks for Brighter configuration errors also catch validation failures — a thoughtful backward-compatibility choice.
• Opt-in design: ValidatePipelines() / DescribePipelines() are fully additive and non-breaking. Existing users are unaffected.
• Specification pattern: Moving ISpecification<T> and Specification<T> to Paramore.Brighter and enhancing them with visitor-based error collection is the right call — the rule sets are independently testable and composable.
• Error aggregation: Errors from all three configuration paths are collected before throwing, so developers see everything wrong at once rather than fix-and-rediscover.
• PipelineBuilder.Describe() without instantiation: Describing the pipeline without constructing handlers avoids DI lifecycle complications — the right approach for startup validation.

[codescene-delta-analysis]

Code Health Improved (1 files improve in Code Health)

Gates Failed
Enforce critical code health rules (2 files with Deep, Nested Complexity, Bumpy Road Ahead)
Enforce advisory code health rules (3 files with Complex Method)

Gates Passed
2 Quality Gates Passed

See analysis details in CodeScene

Reason for failure

Enforce critical code health rules	Violations	Code Health Impact
PipelineDiagnosticWriter.cs	1 critical rule	9.10	Suppress
ServiceActivatorHostedService.cs	2 critical rules	10.00 → 9.24	Suppress

Enforce advisory code health rules	Violations	Code Health Impact
PipelineDiagnosticWriter.cs	1 advisory rule	9.10	Suppress
ConsumerValidationRules.cs	1 advisory rule	9.47	Suppress
ServiceCollectionBrighterBuilder.cs	1 advisory rule	9.10 → 8.82	Suppress

View Improvements

File	Code Health Impact	Categories Improved
TransformPipelineBuilder.cs	9.10 → 9.39	Code Duplication

Quality Gate Profile: Clean Code Collective
Install CodeScene MCP: safeguard and uplift AI-generated code. Catch issues early with our IDE extension and CLI tool.

[codescene-delta-analysis]

❌ New issue: Bumpy Road Ahead
StartAsync has 2 blocks with nested conditional logic. Any nesting of 2 or deeper is considered. Threshold is 2 blocks per function

_Suppress

[codescene-delta-analysis]

❌ New issue: Deep, Nested Complexity
StartAsync has a nested complexity depth of 4, threshold = 4

_Suppress

[claude]

PR Review: Pipeline Validation At Startup

This is a well-designed feature that addresses a real pain point. The layered architecture mirrors Brighter's own three configuration paths cleanly, the Specification pattern with visitor is elegant, and the test coverage is solid. The ADR documentation is thorough.

---

Potential Bugs / Issues

1. BackstopAttributeOrdering only checks BeforeSteps

In HandlerPipelineValidationRules.BackstopAttributeOrdering(), the rule inspects only d.BeforeSteps. AfterSteps are silently ignored. If a user inadvertently places a backstop attribute with HandlerTiming.After, the misordering is undetected. This may be intentional (backstops are always pre-handler), but there is no guard or comment explaining why AfterSteps is excluded.

2. OrNot/Not composition methods silently short-circuit

AndSpecification deliberately avoids short-circuit evaluation so the visitor can collect errors from both children — both Left and Right are always evaluated. But the OrNot and Not() helpers on Specification use short-circuit C# operators (|| and &&), meaning other._lastResults may be stale when the visitor runs. None of the current rule sets use OrNot, so this isn't currently a bug, but it creates a trap for future rule authors who compose specs with OrNot.

3. Redundant AddOptions call in ValidatePipelines()

In BrighterPipelineValidationExtensions.cs, both Configure(...) and AddOptions() are called. Configure calls AddOptions internally, so the explicit AddOptions call is redundant. Harmless but noisy.

---

Breaking Changes (worth noting in release notes)

4. ServiceActivatorHostedService constructor changed

The constructor gained two new required parameters (IServiceProvider and IOptions). DI resolution will still work because IOptions has a default even when unconfigured, and AddConsumers() registers BrighterPipelineValidationOptions unconditionally. However, any user instantiating ServiceActivatorHostedService directly (in test setup, or custom hosted service composition) will get a compile error. Since this is V10.X the break may be intentional, but it should be called out in the changelog.

5. Open generic handlers excluded from subscriber registry (behavior change)

Previously, open generic types like ExceptionPolicyHandler<> were added to the registry with TRequest as the registered request type — which was incorrect. The fix is right: open generics are now DI-registered without being added to the subscriber registry. But any project relying on the prior broken behavior will silently change. The assembly scanning move from the builder constructor to BrighterHandlerBuilder is a good structural improvement.

---

Design Observations / Minor Suggestions

6. Mapper registration not validated for publications

The ADR and PR description cite "No mapper registered for a published type" as an FR. ProducerValidationRules only checks RequestType presence and IRequest assignability — no mapper lookup is performed. This is likely deferred (mapper introspection without instantiation requires ResolveMapperInfo()), but the gap between the stated FR and what ships warrants a follow-up issue or a TODO in the code.

7. _lastResults statefulness should have an inline comment

The ADR acknowledges that _lastResults makes Specification stateful and safe only because startup is single-threaded. A one-line comment on the field — "Startup validation is single-threaded; this is set by IsSatisfiedBy and read by the visitor immediately after. Not safe for concurrent use." — would help future maintainers who encounter it in isolation.

8. No description output when using ValidatePipelines() without DescribePipelines()

BrighterValidationHostedService.StartAsync validates but never calls Describe(). For CQRS-only users (no ServiceActivatorHostedService), there is no automatic pipeline description at startup. When a validation error fires, there is no pipeline layout output to help diagnose the problem. This is arguably by design but worth documenting.

9. Visitor precondition (must follow IsSatisfiedBy) is undocumented

The flow in PipelineValidator.EvaluateSpecs is correct — IsSatisfiedBy must precede Accept to populate _lastResults on leaf nodes. If the contract were violated, the collector would silently return empty results. A note on ValidationResultCollector or ISpecificationVisitor that it must be used after IsSatisfiedBy would prevent future misuse.

---

What's Done Well

• Exception catching inside IsSatisfiedBy converts runtime errors into structured Error-severity findings — prevents startup crashes from buggy specs.
• AndSpecification correctly avoids short-circuit evaluation to collect all errors from both children.
• TryAddSingleton used throughout DI registration — correctly prevents double-registration.
• The IAmABackstopHandler / IAmAResilienceHandler marker interfaces are clean and extensible — third-party handlers can participate in the ordering check without any changes to Brighter.
• Test file naming consistently follows Brighter conventions (When_[condition]should[expected_behavior].cs).
• The open-generic handler exclusion fix is a correct necessary fix, not a workaround.

Overall this is ready for serious review by the core team. The items to resolve before merge are: item 1 (AfterSteps gap in backstop ordering), item 6 (mapper validation gap vs. stated FR), and item 4 (changelog entry for the constructor break). Items 2, 7, 8, and 9 are robustness suggestions, not blockers.