feat: source generator for Brighter handler/mapper/transform registration

[slang25]

Description

Adds Paramore.Brighter.SourceGenerators: a Roslyn incremental source generator that emits handler / message-mapper / transform registrations at compile time, as an alternative to runtime AutoFromAssemblies reflection scanning. The generator follows the recommended "read → intermediate model → write" structure and runs as a properly incremental pipeline (verified via tests on IncrementalStepRunReason).

Consumers can either:

• Add the package to the top-level project — an internal static class BrighterAssemblyRegistrations is auto-generated with an AddFromThisAssembly() extension on IBrighterBuilder. The opt-in flows via a build/ props file so it applies only to direct PackageReferences, not transitive ones. Override per-project with <BrighterAutoRegistration>false</BrighterAutoRegistration>.
• Or hand-write a static partial method marked with [BrighterRegistrations] and the generator fills in the body.

[ExcludeFromBrighterRegistration] opts a single type out either way. A new IBrighterBuilder.Transforms(...) callback on ServiceCollectionBrighterBuilder is added so transforms can be registered explicitly (symmetric with Handlers / MapperRegistry). The HelloWorld sample exercises both: the auto-generated path plus a NoOpTransformer for transform discovery.

Related Issues

Type of Change

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)
• Documentation update

Checklist

• I have read the Contributing Guide
• I have checked the documentation for relevant guidance
• I have added/updated XML documentation for any public API changes
• I have added/updated tests as appropriate
• My changes follow the existing code style and conventions

Additional Notes

Replaces #4127 (which was opened from my fork).

Architecture follows the Kathleen Dollard incremental-generator pattern: SemanticModelReader is the only place that touches Roslyn symbols and projects everything to Roslyn-free records; RegistrationWriter is a pure RegistrationModel → string function and is exhaustively unit-tested without a Compilation. Diagnostics are carried through the pipeline as DiagnosticInfo + LocationInfo (value-equatable) and rebuilt at source-output time.

Pipeline incrementality is verified, not just structural: IncrementalCachingTests drives CSharpGeneratorDriver with trackIncrementalGeneratorSteps: true and asserts on IncrementalStepRunReason — trailing-comment edits and unrelated class additions yield only Cached / Unchanged outputs; adding a real handler yields exactly one Modified source output containing the new handler.

---

Design update (2026-07-12)

API exploration (prompted by the multi-assembly / domain-project problem with AddFromThisAssembly, and informed by NServiceBus 10.2's source-generated registration) has landed on a revised design, now recorded in ADR 0062:

• RegistrationCatalog — an inert data type in core Paramore.Brighter, so handler-owning assemblies need no DI/Polly reference. The generator emits catalog data only; all application logic lives in the library.
• AddRegistrations(params RegistrationCatalog[]) — one registration verb in the DI package, applying with set semantics (exact duplicates union as no-ops).
• Declared holder — [BrighterRegistrations] public static partial class OrdersRegistrations;; the zero-config auto path synthesises a default holder instead of the AddFromThisAssembly() sugar (dropped: see ADR Alternative 7).
• Phase 2 (follow-up PR) — [RegistrationGroup] named convention scoops and opt-in GenerateBuilderExtensions fluent sugar.

⚠️ The code on this branch still implements the superseded imperative design (ADR Alternative 5); migrating the writer/pipeline to catalog emission is the outstanding work. samples/PROTOTYPE-RegistrationCatalog/ is a clearly-marked throwaway two-project sample that fakes the generated output so the proposed API can be felt at real call sites — it will be reverted off the branch before merge.

[claude]

Review: Source generator for Brighter handler/mapper/transform registration

Overall this is a well-structured generator — the strict separation of SemanticModelReader (Roslyn-touching) from the value-equatable model and the pure RegistrationWriter is exactly the recommended Kathleen Dollard pattern, and the verified incremental tests (IncrementalStepRunReason assertions) are a nice step above the usual "structural" caching claims. Below is feedback grouped roughly by priority.

1. Breaking change to IBrighterBuilder interface (likely needs flagging)

IBrighterBuilder.cs adds a new abstract member:

IBrighterBuilder Transforms(Action<ServiceCollectionTransformerRegistry> registerTransforms);

Adding a non-default member to a public interface is binary- and source-breaking for any external implementer of IBrighterBuilder. The PR is labelled "non-breaking change which adds functionality" but this isn't accurate by strict semver rules. Options:

• Make it a default interface method (C# 8 / netstandard2.1) — Brighter already targets recent runtimes so this should work.
• Or document this explicitly as a breaking change in release notes.

It's also worth asking whether the new method is needed on the interface at all, since the generated code only ever uses it on the concrete ServiceCollectionBrighterBuilder flow. An extension method that internally casts (or that's exposed on the concrete type) would avoid the breakage entirely.

2. Generated code couples to a specific IBrighterBuilder implementation

RegistrationWriter.cs:111:

sb.AppendLine("                var registry = (global::Paramore.Brighter.Extensions.DependencyInjection.ServiceCollectionSubscriberRegistry)r;");

The Handlers(Action<IAmASubscriberRegistry>) callback receives an IAmASubscriberRegistry, but the generator unconditionally casts to the concrete ServiceCollectionSubscriberRegistry so it can call EnsureHandlerIsRegistered. If anyone implements an alternative IBrighterBuilder (or the registry interface), generated code will throw InvalidCastException at registration time — a very surprising failure mode at runtime when the generator otherwise looks "framework-aware".

Consider either (a) lifting EnsureHandlerIsRegistered onto IAmASubscriberRegistry, (b) splitting open-generic registration into a separate callback method, or (c) at minimum, documenting this coupling.

3. Test coverage gaps

The generator-internal logic has solid RegistrationWriter unit tests, but the end-to-end behaviour is under-covered:

• Diagnostics BRGEN002 (non-static), BRGEN003 (wrong return type), BRGEN004 (wrong signature) — no end-to-end tests. Only BRGEN001 is exercised.
• Mappers / async mappers / transforms — RegistrationWriterTests covers the emitter, but there are no CSharpSourceGeneratorTest runs that feed real IAmAMessageMapper<> / IAmAMessageMapperAsync<> / IAmAMessageTransform types through the full pipeline. Given the discovery logic in ClassifyEntries / TryClassifyInterface is non-trivial, an end-to-end test for each kind would buy a lot of safety.
• IsPrimaryDeclaration — partial-class deduplication is logic-heavy and untested. A user with partial class Handler { ... } split across two files would otherwise be a regression risk.
• Abstract / nested / non-public-enclosed types — the IsClassifiable and IsReachableFromGeneratedCode filters have no tests.
• IAmAMessageTransform discovered on a generic class — silently skipped (see point 5 below); no test asserts or documents this.

Per the project's CLAUDE.md TDD policy, these gaps would normally need test-first coverage before merging.

4. BuildHintName collision risk

SemanticModelReader.cs:224-231 sanitises the type's display string by replacing every non-[A-Za-z0-9_] character with _:

foreach (var ch in raw)
    sanitized.Append(char.IsLetterOrDigit(ch) || ch == '_' ? ch : '_');

Two distinct types — e.g. Foo.Bar and Foo_Bar (yes, contrived, but possible in generated code or auto-naming conventions) — collapse to the same hint name and collide. Mitigations: include arity, append a short stable hash (e.g. FNV-1a of the original display string), or prefix with the assembly name.

5. Asymmetry: generic mappers and generic transforms are silently dropped

In SemanticModelReader.TryClassifyInterface (line 173, 175) and ClassifyEntries (line 147), mappers and transforms emit nothing when type.IsGenericType. Handlers, however, take the open-generic branch in MakeHandlerEntry and call EnsureHandlerIsRegistered. This is inconsistent and the user gets no warning when their MyMapper<T> : IAmAMessageMapper<MyEvent> is silently ignored. Either emit a diagnostic ("not supported") or document the limitation.

6. EquatableArray<T>.Equals will NRE on null items

Model/EquatableArray.cs:55:

if (!_items[i].Equals(other._items[i])) return false;

If T is a reference type and any element is null, this throws. The constraint where T : IEquatable<T> doesn't preclude nulls. Today no caller passes null, but the type is public and intended to be reused — defensive EqualityComparer<T>.Default.Equals(...) would harden this for free.

7. IsPrimaryDeclaration relies on undocumented Roslyn ordering

SemanticModelReader.cs:197-204:

// "Roslyn orders DeclaringSyntaxReferences deterministically."
var primary = refs[0];
return primary.SyntaxTree == cls.SyntaxTree && primary.Span == cls.Span;

This is observed behaviour, not a contract — but it's fine in practice. Consider ordering explicitly (refs.OrderBy(r => r.SyntaxTree.FilePath, StringComparer.Ordinal).ThenBy(r => r.Span.Start).First()) so the dedup is self-evidently stable.

8. Minor / nits

• HasExcludeAttribute does an independent GetTypeByMetadataName lookup on every classifiable class (SemanticModelReader.cs:208). MarkerSymbols already resolves marker symbols once — fold this attribute symbol into MarkerSymbols so it's resolved once per compilation.
• BrighterRegistrationsGenerator.DescriptorFor swallows unknown ids by defaulting to MustBePartial (line 203). This will produce a misleading diagnostic if a new id is ever added to Diagnostics.cs and DescriptorFor isn't updated. throw new InvalidOperationException (or Debug.Fail) on unknown ids would catch this at test time.
• CompilationProvider re-runs on every compilation change (BrighterRegistrationsGenerator.cs:147). This is the standard pattern when you need a compilation-level check, and the projection to bool keeps downstream cached — just confirming the team understands the trade-off.
• AutoRegistrationTests doesn't assert "no diagnostics". A regression that adds a spurious diagnostic to the auto path would be missed.
• The PR description claims internal static class BrighterAssemblyRegistrations but the test (AutoRegistrationTests.PropertyTrue_GeneratesBrighterAssemblyRegistrations) only checks Contains("internal static class BrighterAssemblyRegistrations") — fine; just noting AddFromThisAssembly itself is emitted as public static, not internal. Confirm that's intentional given the containing class is internal (a public member of an internal class is effectively internal — but it's slightly noisy).

What's particularly good

• Pipeline incrementality is verified with real IncrementalStepRunReason assertions — this is rare and excellent.
• The MethodTarget / RegistrationModel / DiscoveredEntry value-records cleanly separate Roslyn from emit logic, making the writer trivially unit-testable.
• Diagnostics are carried as DiagnosticInfo + LocationInfo and rehydrated at source-output time — exactly the right approach for cacheable pipelines.
• Shipping the BrighterAutoRegistration opt-in via build/ (not buildTransitive/) is a thoughtful choice that prevents the auto-class from spraying into libraries that pick the package up transitively.
• [ExcludeFromBrighterRegistration] gives users a fine-grained escape hatch.

Suggested actions

In rough priority order:

1. Resolve the IBrighterBuilder.Transforms interface-addition story (default impl, concrete-only, or breaking-change note).
2. Decide on the ServiceCollectionSubscriberRegistry coupling — either widen the interface or document it loudly.
3. Add end-to-end tests for BRGEN002/003/004, mapper/async-mapper/transform discovery, and the partial-class dedup.
4. Harden EquatableArray.Equals and BuildHintName.
5. Decide whether generic mappers/transforms should diagnose or stay silent.

Happy to dig further on any of these.

[claude]

Code Review: Source generator for Brighter handler/mapper/transform registration

Solid contribution overall — the architecture is exactly what you want from an incremental generator (Roslyn types confined to SemanticModelReader, pure-data records flow through the pipeline, RegistrationWriter is unit-testable without a Compilation). The verified incremental caching tests via IncrementalStepRunReason are a nice touch and rare to see. Below are the issues worth addressing before this leaves draft.

Correctness / behavioural concerns

1. Ambiguous-method risk with AddFromThisAssembly — both the auto-generated BrighterAssemblyRegistrations.AddFromThisAssembly(this IBrighterBuilder) (in Paramore.Brighter.Extensions.DependencyInjection) and a user-declared [BrighterRegistrations]-attributed extension method with the same conventional name can co-exist. If a consumer has both namespaces in scope, builder.AddFromThisAssembly() becomes ambiguous and won't compile. Worth either: (a) detecting the user-defined method during auto-pipeline gating and skipping auto-class emit, or (b) calling this out in the post-init attribute XML doc so the failure mode is discoverable.

2. Open-generic handler cast can throw InvalidCastException at runtime — RegistrationWriter.WriteHandlers (RegistrationWriter.cs:135) emits an unconditional cast (global::Paramore.Brighter.Extensions.DependencyInjection.ServiceCollectionSubscriberRegistry)r;. The newly added BrighterBuilderExtensions.Transforms explicitly handles the "not the concrete builder" case with a clear InvalidOperationException. Consider doing the same here — emit if (r is not ServiceCollectionSubscriberRegistry registry) throw new InvalidOperationException(...) so misuse fails with a useful message rather than a bare InvalidCastException.

3. AutoFromAssemblies is not a drop-in replacement. The PR description (and the sample swap in samples/CommandProcessor/HelloWorld/Program.cs) reads as if AddFromThisAssembly() and AutoFromAssemblies() are equivalents, but they aren't: AutoFromAssemblies accepts/scans referenced assemblies, while the generator only sees the current compilation. Users with multi-project handler organisations who migrate will silently lose registrations from referenced libraries. Worth documenting prominently — and consider whether the per-assembly approach (one [BrighterRegistrations] method per assembly) is the migration pattern you want to recommend.

4. AccessibilityModifier silently falls back to "internal" for Accessibility.NotApplicable and any other unhandled values (SemanticModelReader.cs:298). For a generator that emits source the user will read, a hidden default could be confusing if a method legitimately has unusual visibility. Either throw on unknown accessibilities (the validator should have caught it earlier) or fall back to private to fail more loudly.

5. IsClassifiable filters out abstract classes but not types containing abstract handler logic that gets inherited. AllInterfaces on a concrete subclass picks up IHandleRequests<T> correctly, so this is right — but AbstractAndPrivateNestedHandlers_AreFilteredOut is the only test that exercises this. Worth a positive test where Concrete : AbstractBase<Cmd> is registered while the abstract base is not, just to lock the behaviour in.

Code quality

6. MarkerSymbols.cs uses private set mutability for what are effectively init-only properties. Since you already polyfill IsExternalInit, switching to { get; init; } would make the intent clearer (and prevent accidental re-assignment within the assembly).

7. MarkerSymbols.Resolve is invoked per syntax-node in the discovery transform (SemanticModelReader.cs:88 inside ReadClass, called for every class with a base list). Each call does ~8 GetTypeByMetadataName lookups. It's fast but not free, and runs on every ReadClass invocation across every incremental edit. Compilation.GetTypeByMetadataName is internally cached, so this is probably acceptable, but worth profiling on a large solution before declaring victory.

8. Diagnostics and MarkerSymbols are public in a source-generator assembly that has IsPackable=false. They're only externally consumed by the test project. internal + InternalsVisibleTo("Paramore.Brighter.SourceGenerators.Tests") would be more honest about the surface area, but public is a defensible shortcut.

9. IsExternalInit.cs is missing the MIT licence header that every other file in the PR carries. Easy fix.

10. DescriptorFor switch (BrighterRegistrationsGenerator.cs:213) throws on unknown IDs, which is right, but the throw will manifest as a generator crash with no source line attribution. Since the IDs are constants under the generator's control, consider a Debug.Assert instead — or keep the throw and accept that any unknown id is genuinely a programming error.

Tests

11. Coverage is good — end-to-end Roslyn testing, pure writer unit tests, incremental caching via IncrementalStepRunReason, and all four validation diagnostics. Two gaps worth filling:

    • Open-generic handler emit is tested in RegistrationWriterTests.OpenGenericHandler_EmitsEnsureHandlerIsRegistered but only at the writer level. No end-to-end test drives an actual public class PolicyHandler<T> : RequestHandler<T> where T : class, IRequest through the full generator. Worth adding so the IsOpenGeneric/UnboundGenericName logic in SemanticModelReader is covered by an end-to-end test.
    • No test for a class implementing both IAmAMessageTransform and IAmAMessageTransformAsync — the reader code in ClassifyEntries adds a single Transform entry in that case, but the behaviour isn't pinned.

12. The diagnostic span tests (BrighterRegistrationsGeneratorTests.NonStaticMethod_ReportsBRGEN002 etc.) use absolute column positions like .WithSpan(9, 37, 9, 56). These are brittle to source edits but standard for analyzer testing. Just be aware.

Security

No notable concerns. The generator reads only the user's compilation; emitted code uses fully-qualified global:: prefixes and typeof(...) references with no string interpolation of user data into executable forms.

Performance

The incremental pipeline is well-structured. The discoveryBatches.Collect().Select(FlattenAndSort) step is the one place where the entire discovery set is re-sorted on any change, which is unavoidable for stable emit order. Once the array equality check downstream sees no change, the source-output is cached. The Where(static batch => !batch.IsEmpty) filter before Collect() is the right defensive move to keep unrelated class additions from flapping the discovery vector.

Nitpicks

• The DiagnosticInfo record has a LocationInfo? Location property that shadows the Roslyn Location type when reading the code — Diagnostic.Create(... info.Location?.ToLocation() ...). Renaming the property to LocationInfo or Site would make the conversion read more naturally.
• The post-init source declares both BrighterRegistrationsAttribute and ExcludeFromBrighterRegistrationAttribute as internal sealed. That's correct, but every consumer assembly will now get its own copy. With multiple Brighter-using assemblies in one process this is harmless (different identities, no runtime conflict), but worth noting in case future work wants to share them via the core Paramore.Brighter assembly.

Nice work overall. The "verified incrementality" via IncrementalStepRunReason and the clean reader/writer split are the kind of details I'd love to see more often in generator PRs.

🤖 Generated with Claude Code

[claude]

Code Review — Source generator for handler/mapper/transform registration

Overall this is a high-quality, well-architected PR. The "read → intermediate model → write" split is followed faithfully: SemanticModelReader is the only place that touches Roslyn symbols, RegistrationWriter is a pure RegistrationModel → string function, and diagnostics/locations are carried as value-equatable records and rebuilt at output time. The incremental pipeline is genuinely verified (asserting on IncrementalStepRunReason), not just structured to look incremental — that is the right bar and rarely done. Nice work.

Feedback below, ordered by impact.

🔴 Default-on auto-registration collides with the documented manual AddFromThisAssembly pattern

build/Paramore.Brighter.SourceGenerators.props defaults BrighterAutoRegistration to true, so the auto path emits an internal static BrighterAssemblyRegistrations.AddFromThisAssembly(this IBrighterBuilder) extension. But the manual path — exercised in the sample, all the e2e tests, and the attribute's own XML doc — teaches users to hand-write a [BrighterRegistrations] static partial IBrighterBuilder AddFromThisAssembly(this IBrighterBuilder builder). The manual pipeline runs regardless of BrighterAutoRegistration. So a consumer who follows the documented manual pattern with the package's default props ends up with two AddFromThisAssembly(this IBrighterBuilder) extension methods in scope, giving CS0121 ambiguous invocation at builder.AddFromThisAssembly(). Because the defaults actively steer users into this, it is the highest-impact issue.

Suggestion: suppress the auto class when the compilation already contains a [BrighterRegistrations] method, or give the auto method a distinct name, or at minimum document the conflict and the <BrighterAutoRegistration>false</BrighterAutoRegistration> escape hatch prominently. There is currently no test for the both-present scenario.

🟠 Accessibility divergence between the generator and AutoFromAssemblies

The ADR frames the two mechanisms as additive/equivalent, but they do not register the same set:

• Reflection scanner (RegisterHandlersFromAssembly) only registers handlers where ti.IsPublic || ti.IsNestedPublic.
• The generator's IsReachableFromGeneratedCode accepts internal (and internal-nested) types too.

So the generator picks up internal handlers that AutoFromAssemblies silently skips. This may be intended (generated code lives in-assembly, so internal is genuinely reachable and arguably better), but it is a behavioral asymmetry worth documenting so users switching mechanisms are not surprised.

🟠 Duplicate registrations via handler inheritance

ClassifyEntries iterates type.AllInterfaces. For class B : A where A : RequestHandler<Cmd>, both A and B are concrete + classifiable and both report IHandleRequests<Cmd>, so you emit Register<Cmd, A>() and Register<Cmd, B>() — two handlers for one request type. Reflection scanning has a similar quirk, but the generator makes it a compile-time emission. Worth deciding whether to register only the most-derived, or leave as-is and document.

🟡 MarkerSymbols.Resolve runs per-node instead of once

Resolve does 8 GetTypeByMetadataName lookups and is called inside both the per-class ReadClass and per-method ReadMethod transforms. The canonical pattern resolves markers once via CompilationProvider and Combines them in. Node-level caching bounds the cost, but this is the one spot that departs from the otherwise-careful incremental design — for a large compilation with many base-listed classes it is 8×N metadata lookups on the cold path.

🟡 Smaller items

• Packaging path is unexercised. IsPackable=false, and the sample wires the generator via ProjectReference ... OutputItemType="Analyzer" with a hand-set <BrighterAutoRegistration>/<CompilerVisibleProperty>. The "ships as a package, build/ props apply only to direct PackageReferences" story (the whole transitive-isolation argument) is therefore not actually validated by anything in the build yet.
• bool.TryParse on the MSBuild property rejects 1/0, which some users use for boolean MSBuild props. Only true/false (case-insensitive) work. Minor, but easy to mis-set silently.
• RS2008 suppressed as "not needed for prototype" in the csproj — this generator defines 5 stable diagnostic IDs (BRGEN001–005). For a shipping generator, analyzer release tracking (AnalyzerReleases.Shipped/Unshipped.md) is the recommended practice rather than suppression, and "prototype" reads oddly in a csproj headed for master.
• Two public Transforms APIs now exist (the instance method on ServiceCollectionBrighterBuilder and the new BrighterBuilderExtensions.Transforms extension) doing the same thing. The extension's keep-it-off-the-interface rationale is sound; just flagging the redundancy.
• Diagnostic emission order is not sorted (unlike FlattenAndSort for entries), so discoveryDiagnostics order tracks batch order. Cosmetic only.

✅ Test coverage

Strong where it counts: incremental caching verified against real IncrementalStepRunReason, exhaustive writer unit tests, e2e generator tests with expected-source comparison, all 5 diagnostics, exclude attribute, partial-class dedup, abstract/private filtering. Gaps I would close:

• Open-generic handler discovery is only tested at the writer level (hand-built model). The SemanticModelReader to model path (IsOpenGeneric, UnboundGenericName, the EnsureHandlerIsRegistered cast) has no reader/e2e test that an actual Foo<T> : RequestHandler<...> is discovered and emitted correctly.
• No test for the auto + manual collision (the first issue above).
• No test for a handler implementing both sync and async, or multiple request types.
• No guard-clause tests for BrighterBuilderExtensions.Transforms (the InvalidOperationException for a non-ServiceCollectionBrighterBuilder, and the ArgumentNull paths).
• No automated test that generated code compiles into and registers correctly in a real CommandProcessor — the sample exercises it but asserts nothing.

Security

Nothing of concern — compile-time generator, no untrusted input; the FNV-1a hash is purely for hint-name disambiguation.

---

Note: I reviewed this statically — I was unable to run dotnet build/test in this environment, so I could not confirm the suite is green.

🤖 Generated with Claude Code

[claude]

Review: source generator for handler/mapper/transform registration

Overall this is a high-quality, well-structured contribution. The Kathleen Dollard "read → model → write" split is followed cleanly, the model layer is genuinely Roslyn-free and value-equatable, and the incremental-caching tests verify the contract that matters (asserting on IncrementalStepRunReason rather than just structure) — the part most generator PRs skip. I confirmed against the codebase that every API the generated code targets exists and is reachable:

• IAmASubscriberRegistry.Register<TRequest,TImpl>() and IAmAnAsyncSubcriberRegistry.RegisterAsync<TRequest,TImpl>() work
• ServiceCollectionSubscriberRegistry implements both registry interfaces, so the open-generic (ServiceCollectionSubscriberRegistry)r cast is valid inside both the Handlers and AsyncHandlers callbacks
• ServiceCollectionMessageMapperRegistryBuilder.Add/AddAsync(Type,Type) and ServiceCollectionTransformerRegistry.Add(Type) exist

A few things worth addressing, roughly in priority order.

1. Correctness — partial class whose base/interface list is on a non-primary file is silently dropped

In SemanticModelReader.ReadClass, discovery is gated two ways:

1. The syntax predicate only fires for ClassDeclarationSyntax where cls.BaseList is not null.
2. IsPrimaryDeclaration then requires the invoking declaration to be the primary one (lowest FilePath, then lowest Span.Start).

These interact badly. Consider a GreetingMapper split across two files where A.cs has the empty partial class declaration (no base list) and B.cs carries : IAmAMessageMapper<E>. Only B.cs's declaration passes the predicate and invokes ReadClass, but the "primary" is A.cs (alphabetically first). primary.SyntaxTree != cls.SyntaxTree, so DiscoveryBatch.Empty is returned and the mapper is never registered, with no diagnostic. That is exactly the silent-missing-registration this feature exists to eliminate.

RegistrationModel.From already calls discovered.Distinct(), so the dedup IsPrimaryDeclaration guards against is already handled by value-equality downstream. Dropping IsPrimaryDeclaration (or selecting the primary only over declarations that carry a base list) would fix the bug. If you keep it, please add a test for the base-list-on-secondary-partial case.

2. Limitation — only class syntax is discovered; record (and struct) mappers/transforms are skipped

The predicate matches ClassDeclarationSyntax only, and IsClassifiable requires TypeKind.Class. Handlers must derive from RequestHandler<T> so they cannot be records, but mappers and transforms implement interfaces only and could legitimately be declared as record. Such a type would be silently missed. Either broaden the predicate to handle RecordDeclarationSyntax, or document the class-only limitation explicitly (the ADR lists the generic-type limitation but not this one).

3. Pre-packaging concerns (flagged because the PR frames this as a NuGet-package story)

The csproj sets IsPackable=false and NoWarn=RS2008 ("not needed for prototype"), yet the description and ADR describe the full package / build/-props consumption flow. So the packaging path (the build/...props Pack/PackagePath, DevelopmentDependency, direct-vs-transitive behaviour) is not actually exercised by a real pack. Before it ships as a package:

• Roslyn version targeting. The generator references Microsoft.CodeAnalysis.CSharp at the repo-pinned 5.3.0. A generator should target the lowest Roslyn it needs, otherwise it fails to load in consumers on older SDKs/compilers. Worth pinning the generator reference to a conservative floor independent of the repo-wide version.
• RS2008 analyzer-release-tracking should be resolved rather than suppressed once this is a shipping analyzer.

4. Footgun — auto-registration defaults on and can coexist with a hand-written method

build/...props defaults BrighterAutoRegistration to true for any direct PackageReference. If a consumer also hand-writes a [BrighterRegistrations] method, both the generated BrighterAssemblyRegistrations.AddFromThisAssembly and their method register the same discovered types — calling both double-registers (and if the user also names their extension AddFromThisAssembly, the two extension methods become an ambiguous call). Consider a diagnostic (or a doc note) when auto-registration and an explicit registration method are both present in one compilation.

5. Minor — per-node MarkerSymbols.Resolve cost

ReadClass/ReadMethod call MarkerSymbols.Resolve(compilation) per node, doing 8 GetTypeByMetadataName lookups every time a class-with-base-list node is (re)transformed. Correct, and it keeps values cacheable, but it is repeated work on every keystroke in a touched file. Not blocking — just the obvious profiling target if IDE responsiveness ever regresses on large projects.

Nits

• Diagnostics, RegistrationWriter, SemanticModelReader, MarkerSymbols and the Model/* types are public. For a dev-dependency generator these could be internal (+ InternalsVisibleTo for tests) to keep the surface tight; public is harmless though.
• New test files omit the MIT licence header. Roughly 40% of existing test files also omit it so this is not inconsistent, but all src/ files carry it.

Test coverage

Strong. The writer is exhaustively unit-tested without a Compilation; end-to-end tests cover sync/async handlers, mappers, transforms, exclusion, partial classes, abstract/private filtering, all five diagnostics, and the auto-registration property gate (true/false/missing). The incrementality tests are the standout. The main gaps map to issues 1 (base-list-on-secondary-partial) and 2 (record mappers/transforms).

Nice work overall — items 1 and 2 are the ones I would want resolved or explicitly documented before merge.

Automated review — feedback may contain mistakes; please verify the partial-class case against your own expectations.

[claude]

Code Review — Source generator for handler/mapper/transform registration

Thorough, well-structured PR. The reader→model→writer separation is exactly the right shape for an incremental generator, the value-equatable record pipeline is done properly, and the test suite (28 tests, including IncrementalStepRunReason assertions and diagnostic coverage) is genuinely above the bar for source-generator PRs. Comments below are mostly refinements; nothing here is blocking on correctness.

Correctness / behavior

• MarkerSymbols.Resolve runs per-syntax-node, not per-compilation. ReadClass and ReadMethod each call MarkerSymbols.Resolve(ctx.SemanticModel.Compilation), and Resolve is a new() factory doing 8x GetTypeByMetadataName. The 1979663 commit message says folding ExcludeAttribute into MarkerSymbols makes it "resolved once per compilation instead of per class" — but the code still resolves all markers fresh for every class with a base list and every attributed method, on every relevant edit. The idiomatic fix is to resolve once via context.CompilationProvider.Select(MarkerSymbols.Resolve) and .Combine(...) into the transforms. It doesn't affect output-cache correctness (everything downstream is still value-equatable), but on a large compilation it's real redundant work each keystroke. (SemanticModelReader.cs:88, SemanticModelReader.cs:53)

• EnsureHandlerIsRegistered cast is a hard dependency on the concrete registry. Open-generic handlers emit (ServiceCollectionSubscriberRegistry)r. That's fine for the shipped DI extension, but if IBrighterBuilder.Handlers ever hands the callback a different IAmASubscriberRegistry, the generated code throws InvalidCastException at runtime with no compile-time signal. Worth a one-line comment in the generated path or a note in the ADR that open-generic registration is coupled to ServiceCollectionSubscriberRegistry. (RegistrationWriter.cs:147)

Packaging gap

• IsPackable=false and there's no *.SourceGenerators.Package project, so the generator can't actually ship as a NuGet package yet. The central design claim — that build/...props applies the opt-in to direct PackageReferences but not transitive ones — is therefore never exercised end-to-end; only the manual ProjectReference + hand-set BrighterAutoRegistration path (the sample) is. That matches the "prototype" comment on RS2008, but it's the load-bearing part of the UX and is currently untested in a real package-consumption scenario. Consider following the Paramore.Brighter.Analyzer.Package pattern before this is relied on. (Paramore.Brighter.SourceGenerators.csproj:7)

Minor / cleanups

• Redundant condition: in ClassifyEntries, if (seenTransform && !type.IsGenericType) — seenTransform is only ever set in the branch where !type.IsGenericType already holds, so the second clause is always true. Harmless, but dead. (SemanticModelReader.cs:163)

• Public surface for testability. SemanticModelReader, RegistrationWriter, MarkerSymbols, GeneratedSource, and the model records are all public. Since the project is IsPackable=false with ProduceReferenceAssembly=false this is harmless to consumers, but internal + InternalsVisibleTo for the test project would better signal intent.

• AddFromThisAssembly with no [BrighterRegistrations] method + auto disabled produces no entry point at all, which is correct, but a consumer who forgets the BrighterAutoRegistration opt-in (e.g. transitive-only) gets a silent "method not found" compile error with no generator diagnostic pointing them at the property. An info-level diagnostic when AddFromThisAssembly is referenced but auto-gen is gated off would shorten that debugging loop. (Optional.)

Nits

• EquatableArray<T> allocates via items.ToArray() on every construction including the hot flatten path — fine at these sizes, just noting.
• The code.Indent++ "historical layout" branch for the namespace-less case (RegistrationWriter.cs:57) is well-commented but subtle; a global-namespace consumer is an odd enough case that a dedicated writer test would lock it down.

Security

No concerns — compile-time only, all emitted type names originate from the compilation symbol table (fully-qualified, global::-prefixed), and BuildHintName's FNV-1a suffix correctly guards against hint-name collisions between types differing only in non-identifier characters.

Test coverage

Strong. The one thing I'd add is an automated end-to-end test that the generated registrations actually resolve and dispatch through a real CommandProcessor (the HelloWorld sample exercises this by hand, but nothing asserts on it in CI). Everything else — incrementality, all five diagnostics, partial-class dedup, abstract/private filtering, the auto-gen gating matrix — is well covered.

---

Reviewed statically; I was unable to run dotnet build/tests in this environment, so I haven't independently confirmed compilation.

🤖 Generated with Claude Code

[claude]

Review: Source generator for handler/mapper/transform registration

I reviewed this against the repo's conventions (CLAUDE.md / .agent_instructions). Overall this is a high-quality, carefully-engineered PR — the reader→model→writer split, value-equatable pipeline records, the WithTrackingName + IncrementalStepRunReason caching tests, analyzer release tracking, and the [GeneratedCode]/banner stamping all reflect current Roslyn best practice. Licence headers, XML docs, and test coverage (RegistrationWriter unit tests + end-to-end generator tests + incrementality tests + auto-registration gating) are all in good shape. The commit history shows the design has already been hardened through several review rounds.

Comments below are mostly forward-looking / nits — nothing I'd consider blocking.

Code quality / correctness — looks solid

• SemanticModelReader is the single Roslyn-touching boundary and everything downstream is a value-equatable record — the discipline holds throughout.
• MarkerSymbols.Resolve memoised via ConditionalWeakTable<Compilation, …> (thread-safe, weak-keyed) is the right call to avoid repeating 8 metadata lookups per node.
• Suppressing the auto class when a manual [BrighterRegistrations] method exists (with the explanatory BRGEN007 info) avoids double-registration cleanly.
• Always emitting MapperRegistry(...) even with zero discovered mappers (to preserve EnsureDefaultMessageMapperIsRegistered parity with AutoFromAssemblies) is a subtle but correct parity fix.
• as … ?? throw for the open-generic registry cast, and the BuildHintName FNV-1a suffix to avoid hint-name collisions, are nice defensive touches.

Things worth flagging before this ships (largely already acknowledged in ADR 0062)

1. Packaging is inert / untested. IsPackable=false, yet the PR adds the build/…props file with Pack="true"/PackagePath. The headline "build/ applies only to direct PackageReferences, not transitive" behaviour — the core of the opt-in design — is therefore not exercised by any test. The tests cover the underlying MSBuild property (BrighterAutoRegistration) via AutoRegistrationTests, which is good, but the NuGet direct-vs-transitive packaging story is unverified. Fine for a "Proposed" ADR, but I'd call it out as a release gate.
2. Roslyn floor not pinned. The generator pulls Microsoft.CodeAnalysis.CSharp from central versioning (currently 5.3.0). A generator is loaded by the consumer's compiler, so building against 5.3 effectively requires a very recent SDK to even load. The csproj comment + ADR already flag pinning to 4.3.1 (lowest with ForAttributeWithMetadataName) and the entangled RS2002 issue — just confirming this needs resolving before publish, since it directly limits who can consume the package.
3. Accessibility asymmetry of the auto class. The generated internal static BrighterAssemblyRegistrations.AddFromThisAssembly is only callable from within the scanned assembly. That's by-design (documented in the ADR), but it's a real ergonomic difference vs AutoFromAssemblies that consumers will hit — worth surfacing prominently in user docs when this graduates from Proposed.

Minor / nits

• Diagnostic-bearing types and cache invalidation: DiscoveryBatch.Diagnostics carry LocationInfo (file + TextSpan). For handlers there are no diagnostics so the cache is pristine (as the tests prove), but a trailing-comment edit above a type that emits BRGEN005/006 will shift its TextSpan and re-run that one batch. Harmless (diagnostics are cheap and the symbol-level Locations.FirstOrDefault() keeps split-partial diagnostics dedupable), just noting the incrementality guarantee is slightly weaker for diagnostic-emitting types than the PR description implies.
• DescriptorFor throws on unknown id (good), but BRGEN007 deliberately never routes through it — that's correct, just non-obvious; a one-line comment there would save a future reader the double-take.
• HelloWorld now demonstrates AddFromThisAssembly() via a ProjectReference with a hand-set BrighterAutoRegistration=true + CompilerVisibleProperty. Because this bypasses the build/props mechanism, the sample doesn't actually demonstrate the package's intended zero-config path — consider a comment or doc note so users copying the sample don't conclude the manual opt-in is always required.

Test coverage

Strong. The IncrementalCachingTests (asserting Cached/Unchanged on tracked stages, with a positive control proving invalidation when a real handler is added) are exactly the right way to test a generator's incrementality, and the commit notes that breaking EquatableArray equality was used to confirm the tests actually go red. Diagnostics BRGEN001–006, partial-class dedup, record mappers, nested-open-generic, and auto/manual collision are all covered. The main untested surface is the NuGet packaging behaviour noted in (1).

Nice work — this is a well-structured feature.

🤖 Generated with Claude Code

[claude]

Code Review — Source generator for handler/mapper/transform registration

Reviewed the full diff (generator, DI changes, sample, tests). This is genuinely high-quality work — the architecture, incrementality discipline, and test depth are all well above the bar for a Roslyn generator. Comments below, grouped by severity. Nothing here is a blocker; the medium item is the only one I'd ask be addressed (even if only with a diagnostic) before merge.

What's strong

• Clean "read → model → write" separation. SemanticModelReader is the only place that touches symbols, everything downstream is value-equatable records, and RegistrationWriter is a pure RegistrationModel → string function that's unit-tested without a Compilation. This is exactly the Dollard pattern and it's followed faithfully.
• Incrementality is verified, not assumed. IncrementalCachingTests drives trackIncrementalGeneratorSteps: true and asserts Cached/Unchanged on trailing-comment and unrelated-class edits, with a positive control proving the cache does invalidate when a real handler is added. This is the test most generators skip and the one that actually matters.
• MarkerSymbols memoised per-Compilation via ConditionalWeakTable (weak keys, no cross-edit leak) — the right call given the per-node transforms.
• Safety-by-construction on the auto path: the generated BrighterAssemblyRegistrations is internal, so two projects each generating it can't collide across an assembly boundary; the build/ (not buildTransitive/) props file correctly scopes opt-in to direct PackageReferences; and a valid manual [BrighterRegistrations] method suppresses the auto class with an explanatory BRGEN007 rather than silently double-registering. The "invalid manual method does not suppress" nuance is even tested.
• Diagnostics are carried as value-equatable DiagnosticInfo/LocationInfo and rebuilt at output time, with analyzer-release tracking (AnalyzerReleases.*.md) kept in sync. Gating discovery diagnostics on generatorActive so a transitive analyzer reference doesn't spam warnings is a thoughtful touch.

Medium — manual [BrighterRegistrations] method in a nested type emits a non-matching partial

SemanticModelReader.ProjectMethod captures only containingType.Name (the simple name) plus ContainingNamespace, and RegistrationWriter.WriteContainingType emits a single containing type. So for:

namespace App;
public static partial class Outer
{
    public static partial class Registrations
    {
        [BrighterRegistrations]
        public static partial IBrighterBuilder AddFromThisAssembly(this IBrighterBuilder builder);
    }
}

the generator emits namespace App { class Registrations { ... } } — losing the Outer nesting — so the partial implementation never binds to the user's declaration and the build fails (the user's partial stays unimplemented). There's no diagnostic for this; the user gets a raw compiler error with no pointer to the cause. Either walk the full containing-type chain when emitting (and in BuildHintName/HintName), or add a BRGEN0xx rejecting nested containing types with a clear message. A diagnostic is the cheaper fix and is consistent with how the other invalid-shape cases are handled.

Low — file-scoped types slip through IsClassifiable

IsReachableFromGeneratedCode accepts Public/Internal, and a file class Foo : IHandleRequests<…> reports Internal accessibility — so it'd be discovered and emit r.Register<…, global::Foo>() in a different generated file, producing CS9051 (file-local type referenced outside its file). Niche, but it would surface as a confusing compiler error rather than a skip. Worth a guard on IsFileLocal (or an exclusion + note).

Low — multi-assembly resolution of the generated marker attributes

The [BrighterRegistrations] / [ExcludeFromBrighterRegistration] attributes are emitted internal per-assembly. If a referenced project also ran the generator, the consumer's compilation sees the metadata copy too, and GetTypeByMetadataName(...) returns null on an ambiguous match. Source-declared types in the current compilation normally win, so this is probably fine in practice — but it's exactly the kind of thing that breaks silently (exclusions quietly stop working). A test that references a second compilation which also embeds the attributes would lock the behaviour down.

Nit

• RegistrationWriter.Write increments code.Indent in the no-namespace branch without a matching decrement. Harmless (it's the end of the file), but asymmetric with the namespace branch's StartBlock/EndBlock.

Test coverage

Coverage is excellent overall (34 tests across writer, end-to-end, auto-registration, and incrementality). Two gaps worth closing:

• Open-generic handler discovery end-to-end. The EnsureHandlerIsRegistered / UnboundGenericName path is well covered at the writer level (RegistrationWriterTests), but there's no end-to-end test that a top-level Handler<T> : IHandleRequests<Cmd> is classified as open-generic by SemanticModelReader and emits typeof(Handler<>). That reader-side logic (IsOpenGeneric, UnboundGenericName arity handling) is currently only exercised indirectly.
• Nested containing type (see Medium) — add whichever behaviour you choose as a regression test.

Nice work — the generator is well-reasoned and the incremental-caching tests in particular set a good example.

🤖 Generated with Claude Code

[codescene-delta-analysis]

Gates Failed
Enforce advisory code health rules (2 files with Excess Number of Function Arguments, Complex Method, Complex Conditional)

Our agent can fix these. Install it.

Gates Passed
3 Quality Gates Passed

Reason for failure

Enforce advisory code health rules	Violations	Code Health Impact
SemanticModelReader.cs	2 advisory rules	9.39	Suppress
RegistrationModel.cs	1 advisory rule	9.69	Suppress

See analysis details in CodeScene

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: Complex Method
AccessibilityModifier has a cyclomatic complexity of 9, threshold = 9

_Suppress

[codescene-delta-analysis]

❌ New issue: Complex Conditional
IsClassifiable has 1 complex conditionals with 2 branches, threshold = 2

_Suppress

[claude]

Code Review — Source generator for handler/mapper/transform registration

Reviewed the full diff (29 files). High-quality, well-architected contribution. Strengths first, then issues by severity.

Strengths

• Clean incremental-generator architecture. The "read → Roslyn-free model → pure writer" split is textbook: SemanticModelReader is the only place that touches symbols, everything downstream is value-equatable records, and RegistrationWriter is a pure RegistrationModel → string function unit-tested with no Compilation. Exactly what keeps the pipeline cacheable.
• Incrementality is verified, not assumed. IncrementalCachingTests drives CSharpGeneratorDriver with trackIncrementalGeneratorSteps: true and asserts on IncrementalStepRunReason — with a positive control (adding a real handler ⇒ exactly one Modified output) and negative controls (trailing comment / unrelated class ⇒ Cached/Unchanged). This is the single most common thing generators get wrong, and it's covered properly.
• Marker resolution is memoized per-Compilation via a ConditionalWeakTable with weak keys — correct way to avoid 8 metadata lookups per node without leaking across edits.
• Diagnostics are first-class: DiagnosticInfo/LocationInfo are value-equatable and rebuilt at output time, release tracking (AnalyzerReleases.*.md) satisfies RS2008, and DescriptorFor throws on an unmapped id so a new diagnostic can't silently slip through.
• Thoughtful edge cases: open-generic handlers route to EnsureHandlerIsRegistered(typeof(Foo<>)) only when needed (interface-only otherwise); generic mappers/transforms ⇒ BRGEN005; nested-in-open-generic ⇒ BRGEN006; partial types split across declarations are de-duped via Distinct(); the always-emitted MapperRegistry(...) preserves EnsureDefaultMessageMapperIsRegistered parity with AutoFromAssemblies. Verified the referenced EnsureHandlerIsRegistered/EnsureDefaultMessageMapperIsRegistered are public and that Handlers(r => …) passes a ServiceCollectionSubscriberRegistry, so the emitted cast is sound.
• Good test breadth (~1,270 lines across 4 files): generator behavior, auto-registration gating, writer output, and caching.

Issues

1. (Major — but acknowledged) The NuGet distribution path described in the PR doesn't actually work yet.
The PR description leads with "Add the package to the top-level project … the opt-in flows via a build/ props file," but Paramore.Brighter.SourceGenerators.csproj sets IsPackable=false, there is no companion *.Package project, and the <None Include="build\…" Pack="true"> is therefore dead — dotnet pack produces nothing, and the direct-vs-transitive build/ (not buildTransitive/) story is never exercised by a real package. To the PR's credit, ADR 0062 explicitly documents this as a follow-up via the existing Paramore.Brighter.Analyzer.Package pattern. Flagging only so reviewers are aware the headline consumer experience isn't reachable from this PR alone — currently only the ProjectReference … OutputItemType="Analyzer" path (the HelloWorld sample) is functional.

2. (Minor bug / unguarded gap) A [BrighterRegistrations] method in a nested containing type emits an uncompilable partial.
SemanticModelReader.ProjectMethod captures only containingType.Name and containingType.ContainingNamespace. For a type nested inside another type, ContainingNamespace skips the outer type, so the generated partial is emitted at namespace scope with just the inner name — a different type from the user's nested one, so their partial method is never satisfied → compile error (CS8795) with no diagnostic. Either emit the enclosing-type wrappers, or add a diagnostic rejecting a registration method whose containing type is nested. All current tests use a top-level Registrations class, so this isn't covered.

3. (Minor) Transforms(...) runtime throw from generated code for non-default builders.
RegistrationWriter.WriteTransforms emits a static call to BrighterBuilderExtensions.Transforms(builder, …), which throws InvalidOperationException if builder isn't a ServiceCollectionBrighterBuilder. Reasonable (mirrors the open-generic-handler cast, and the existing reflection path is equally DI-coupled), but a custom IBrighterBuilder implementation will hit a runtime failure originating from generated code rather than a compile-time signal.

4. (Design question) The auto-generated AddFromThisAssembly is internal.
Correct for the per-assembly model (each project registers its own types, called from within that project), but a multi-project app can't call a referenced library's generated method — each library would need a direct package reference and to re-expose it. Worth a sentence in the docs/ADR on the intended multi-project workflow so consumers don't expect cross-assembly aggregation.

5. (Nits)

• EquatableArray<T>(IEnumerable<T>) calls items.ToArray() with no null guard; not reachable today, but a defensive check is cheap.
• RegisterAutoRegistration depends on CompilationProvider (brighterAvailable), which changes every edit, so the marker resolve re-runs once per keystroke. Bounded and the resulting bool is stable so downstream stays cached — fine in practice, just the one non-ForAttributeWithMetadataName input in an otherwise tightly-incremental pipeline.

Verdict

Architecturally excellent and well-tested. The only true follow-up blocker for the advertised feature is #1 (packaging), which the ADR already calls out. Items #2–#4 are worth addressing or documenting before this is positioned as the recommended replacement for AutoFromAssemblies.

Note: dotnet build/tests could not be run in this review sandbox; the above is static review — please rely on CI for build/test confirmation.

[iancooper]

@slang25 I note that NServiceBus have been moving along with this: https://particular.net/blog/preparing-for-trimming-and-aot

[claude]

Code Review — Source generator for handler/mapper/transform registration

Thanks for this — it's an unusually well-engineered PR. The incremental-generator architecture is textbook (Roslyn-free value-equatable pipeline, a pure RegistrationModel → string writer, marker symbols memoised per-Compilation via ConditionalWeakTable), and the test suite is genuinely strong: IncrementalCachingTests verifies actual incrementality via IncrementalStepRunReason rather than just structure, and the edge cases (partial types with the base list on a secondary declaration, nested-in-open-generic, generic mapper/transform, cross-partial diagnostic de-dup) are all covered. The ADR is also refreshingly honest about what's deferred.

Below is feedback grouped by significance. Most of the larger items are already tracked in ADR-0062's "Deferred follow-ups", so this is mostly confirmation + a couple of things worth watching.

Architecture / design

1. Generated code emits application logic, not inert data — this contradicts the ADR's core Decision. ADR-0062's guiding principle is "the generator emits inert data; all application logic lives in the library," via a RegistrationCatalog + AddRegistrations applier, specifically to "[minimise] the version-skew surface between the generator and the runtime libraries." The shipped generator instead emits imperative bodies bound directly to the DI registration surface — builder.Handlers(r => r.Register<TReq,TImpl>()), builder.MapperRegistry(...), BrighterBuilderExtensions.Transforms(...), and even a hard cast to ServiceCollectionSubscriberRegistry for open generics. That code is compiled into consumer assemblies at their build time, so any change to those method signatures is a break for already-shipped consumers — exactly the failure mode the catalog design was meant to prevent. The ADR acknowledges this ("The branch currently implements the superseded shape (Alternative 5)"), so this isn't a surprise, but it's worth flagging prominently: the coupling surface the generated code binds to should be treated as frozen public API until the catalog migration lands.

2. Packaging is not wired up (documented). IsPackable=false with no companion *.Package project means dotnet pack produces nothing — the <None Include="build\...props" Pack="true"> item is inert, and the direct-vs-transitive build/ props story is never exercised by a real package consumer. Only the in-repo ProjectReference + OutputItemType="Analyzer" path (the sample) works today. So the NuGet consumption described in the PR body isn't functional yet. The ADR notes this and points at the existing Paramore.Brighter.Analyzer.Package pattern to follow — just calling it out since the PR description reads as package-ready.

Behavioural parity with AutoFromAssemblies

3. Handler visibility differs from the reflection scanner. RegisterHandlersFromAssembly requires (ti.IsPublic || ti.IsNestedPublic), but the generator's IsReachableFromGeneratedCode accepts internal too (for handlers, mappers and transforms). Because the generated code lives in the same assembly this compiles fine and is arguably more correct, but it's a real divergence: migrating a project from AutoFromAssemblies to the generator will start registering internal handlers that were previously ignored. Worth a line in the docs, and ideally a test pinning the intended behaviour for an internal top-level handler (the current tests only cover public handlers + a private nested one being excluded).

4. I confirmed the "generator only scans the current compilation, AutoFromAssemblies also scans the core Brighter assembly" difference is not a functional gap — the only core-assembly handler types (RequestHandler<>, RequestHandlerAsync<>) are abstract and filtered out by both paths anyway. 👍

Test coverage

5. The new BrighterBuilderExtensions.Transforms / ServiceCollectionBrighterBuilder.Transforms API is exercised end-to-end through the generator (MapperAndAsyncMapperAndTransform_AreAllDiscovered), but the DI project has no direct unit test for its guard clauses — the ArgumentNullException paths and, in particular, the InvalidOperationException thrown when a non-ServiceCollectionBrighterBuilder is passed. A small targeted test there would lock the contract in.

6. Consider a test for the open-generic handler auto-path emitting the ServiceCollectionSubscriberRegistry cast + EnsureHandlerIsRegistered (the writer branch in WriteOpenGenericHandlers) — it's covered structurally but I don't see an assertion on the emitted cast/text.

Minor / nits

• RegistrationWriter always emits MapperRegistry(r => {}) even with no mappers (intentional, for EnsureDefaultMessageMapperIsRegistered parity — the comment explains it well). Good call.
• The reader/writer/model types are public purely for test access; ADR notes internal + InternalsVisibleTo would be tidier for a dev-dependency generator. Harmless.
• [GeneratedCode(..., ToolVersion)] bakes the generator assembly version into every emitted file, so a generator version bump churns all generated output. Expected, just noting it interacts with deterministic-build expectations.

Verdict

Excellent foundation and, importantly, the code correctly does what it sets out to do. The two things I'd want resolved before this is shipped to consumers (vs. merged as in-progress) are the version-skew coupling in generated output (item 1) and packaging (item 2) — both already tracked in the ADR. Nothing here blocks merging the pipeline itself.

Reviewed against CLAUDE.md conventions. Note: I couldn't run dotnet build/tests in this environment (sandbox), so the review is static — please confirm the suite is green in CI.

[claude]

PR Review — Paramore.Brighter.SourceGenerators

Thanks for this — it is a genuinely high-quality piece of work. The generator follows the canonical read → intermediate model → write structure, keeps Roslyn symbols out of the cached graph, and backs it up with real incremental-caching assertions. Below is feedback organized by area.

Strengths

• Incremental design is textbook. Every value crossing a transform boundary is a value-equatable record (EquatableArray<T>, DiscoveredEntry, MethodTarget, DiagnosticInfo with LocationInfo). Nothing holds a Compilation/ISymbol, and MarkerSymbols is memoized per-Compilation via a ConditionalWeakTable — so the eight metadata lookups collapse to one per pass without leaking across edits. IncrementalCachingTests actually verifies IncrementalStepRunReason (Cached/Unchanged/Modified), which is exactly the right way to prove this.
• Careful edge-case handling: partial types whose base list lives on a secondary declaration; deduping duplicate diagnostics from split partials (Distinct()); nested-in-open-generic and generic mapper/transform surfaced as warnings rather than emitting broken code; deterministic sort in FlattenAndSort for stable output.
• Packaging/analyzer hygiene: EnforceExtendedAnalyzerRules, AnalyzerReleases.*.md release tracking, Roslyn pinned to 4.8.0 for the widest consuming-SDK range, build/ (not buildTransitive/) props so auto-registration only applies to direct references — with a per-project <BrighterAutoRegistration>false</BrighterAutoRegistration> escape hatch and [ExcludeFromBrighterRegistration] per-type opt-out.
• Good diagnostics UX: BRGEN001–007 with actionable messages, and BRGEN007 explains why the auto class vanished when a manual method takes precedence — a nice touch.
• Test coverage is strong (~1,274 lines across 4 files) spanning writer output, discovery, diagnostics, auto-registration on/off/suppressed, and incremental caching.

Questions / possible issues

1. Packaging appears incomplete or deferred — please confirm intent. Paramore.Brighter.SourceGenerators.csproj sets IsPackable=false and IncludeBuildOutput=false, and only the build/*.props is packed. So no NuGet package is produced today, and even if IsPackable were flipped the generator assembly is not placed under analyzers/dotnet/cs. Yet the props comment and the HelloWorld csproj comment both reference "the NuGet package." The samples consume it via ProjectReference … OutputItemType="Analyzer", so this works in-repo — but a consumer cannot yet get it as a package. Is the packaging wiring intended as a follow-up PR? Worth a note in the description if so.

2. Manual [BrighterRegistrations] methods in a nested or generic containing type will emit code that does not compile — and there is no diagnostic. RegistrationWriter.WriteContainingType emits a single class {ContainingTypeName} directly under the namespace, and SemanticModelReader.ProjectMethod sets ContainingTypeName = containingType.Name (innermost name only, no outer-type chain, no type parameters). So a valid-looking method inside Outer.Inner, or inside a generic Foo<T>, generates a top-level partial class Inner / class Foo that will not match the user declaration. ValidateMethod checks partial/static/return/signature but not "top-level, non-generic containing type." Consider either adding a BRGEN diagnostic for unsupported containing types or reconstructing the nesting/type-parameters. The auto path is unaffected (fixed top-level BrighterAssemblyRegistrations), so this only bites hand-written methods — hence lower severity, but currently a silent broken-build trap.

Nits

• GeneratedSource.ToolVersion embeds the generator assembly version in every [GeneratedCode(...)], so any version bump rewrites all generated files. Expected for a generator, just flagging it for anyone who later adds golden/snapshot tests.
• BrighterBuilderExtensions.Transforms (extension) and ServiceCollectionBrighterBuilder.Transforms (instance) now coexist with the same name. Overload resolution prefers the instance method for a concretely-typed variable and the extension for IBrighterBuilder, so there is no ambiguity — but a one-line XML remark cross-referencing the two would avoid future confusion. The symmetry with Handlers/MapperRegistry is a nice API choice.
• For a generic handler over a concrete request (e.g. class H<T> : RequestHandler<SomeCommand>), MakeHandlerEntry routes it to the open-generic EnsureHandlerIsRegistered(typeof(H<>)) path. Probably fine, but a test pinning that behavior would be reassuring.

Verdict

Architecturally sound, well-tested, and clearly written. Nothing here is a blocker for the core (auto-registration) path. I would like clarity on the packaging story (#1) and would suggest guarding the nested/generic containing-type case for manual methods (#2) before this is advertised as a general-purpose hand-written entry point.

[claude]

Claude Code Review — PR #4138

Reviewed against CLAUDE.md / .agent_instructions. This is a genuinely well-engineered incremental source generator: the Dollard "read → model → write" split is clean, the pipeline is verified incremental (asserting on IncrementalStepRunReason, not just structure), and the writer being a pure RegistrationModel → string function makes it exhaustively unit-testable without a Compilation. Diagnostics carried as value-equatable DiagnosticInfo/LocationInfo and rebuilt at output time is exactly right. Nice work.

The review below assumes the two caveats the PR description already flags: (1) the branch still implements the superseded imperative design (ADR Alt 5) rather than the RegistrationCatalog design now settled in ADR 0062, and (2) samples/PROTOTYPE-RegistrationCatalog/ is throwaway and will be reverted. I've therefore skipped the prototype and focused on the generator code that is intended to survive.

🔴 Merge blockers / design status

• The implementation doesn't match the accepted ADR. ADR 0062 now specifies an inert RegistrationCatalog in core + a single AddRegistrations(...) verb, and drops AddFromThisAssembly() (Alternative 7). This branch still emits imperative IBrighterBuilder method bodies and the AddFromThisAssembly sugar. Per the repo's spec workflow (Design → approval → implementation), the writer/pipeline migration is the outstanding work before this is mergeable. Worth splitting the already-solid generator infrastructure (reader/writer/model/caching/tests) from the not-yet-migrated emit shape so the former can land independently.
• <IsPackable>false</IsPackable> in Paramore.Brighter.SourceGenerators.csproj means the whole build/-props opt-in mechanism — the core of the "direct-vs-transitive PackageReference" design — is never actually exercised as a package. The <None ... PackagePath="build/"> and the direct-only gating are effectively untested end-to-end. Acknowledged as deferred packaging work in the ADR, but flagging that the headline gating behavior has no CI coverage until packaging is real.

🟡 Correctness / edge cases

• Multiple [BrighterRegistrations] methods → silent double registration. methodCandidates is a per-method stream Combined with the full discovered set, so each attributed method emits a body registering all discovered handlers/mappers. Two valid holder methods in one compilation both get full bodies; if a consumer wires up both, everything registers twice with no diagnostic. Consider a diagnostic when >1 valid method is present (or document the single-method contract).
• Nested / generic holder types break the generated partial. ProjectMethod captures ContainingTypeName: containingType.Name (simple name) and Namespace from ContainingNamespace, then the writer emits a single top-level class {ContainingTypeName} in that namespace. A [BrighterRegistrations] method on a nested holder (class Outer { partial class Reg { ... } }) or a generic holder would generate a partial that can't merge with the user's declaration and won't compile. Handler/mapper discovery correctly handles nesting via FullyQualifiedFormat; it's only the holder projection that assumes top-level. A BRGEN00x guard (or an explicit test proving it works) would close this.
• Handler inheritance double-registration (base + derived both concrete, both seen via AllInterfaces) is correctly called out in the ADR as known — fine to leave, just confirming it's intentional.

🟢 Minor / nits

• EquatableArray<T>.GetHashCode/Equals are order-sensitive. That's correct here only because FlattenAndSort produces a stable ordering before the arrays are built — worth a one-line comment on the type noting the caller-ordering contract, since the type is public and a future caller could pass unsorted input and silently defeat caching.
• BrighterBuilderExtensions.Transforms throwing InvalidOperationException for a non-ServiceCollectionBrighterBuilder builder is reasonable, but the generated code hard-depends on that concrete type existing (same as the open-generic EnsureHandlerIsRegistered cast). Both are DI-package-coupling points the catalog redesign is meant to remove — another reason the emit-shape migration matters.
• AutoRegistrationSuppressed (BRGEN007) is Info severity; good call — it won't break builds but explains the vanished class.
• Program.cs sample switch from AutoFromAssemblies() to AddFromThisAssembly() will need revisiting once the API becomes AddRegistrations(...).

✅ Tests

34 tests across writer unit tests, end-to-end generator/diagnostic tests (BRGEN001–006), auto-registration gating, partial-class dedup, record mappers, and tracked-stage incrementality — strong coverage for the current shape. Gaps to add when the design settles: multiple-holder-method, nested/generic holder, and (once packaging lands) an actual direct-vs-transitive PackageReference integration test.

---

Overall: high-quality generator engineering, but not mergeable in its current form — the emit shape needs to catch up to the approved ADR 0062, and the prototype sample needs removing. Consider landing the reader/writer/model/caching foundation as a separate PR.

🤖 Generated with Claude Code

[claude]

Review: source generator for handler/mapper/transform registration

Thanks for this — it's a genuinely high-quality piece of work. The reader→model→writer split, the value-equatable pipeline, and (especially) the verified incrementality via IncrementalStepRunReason are exactly how a modern incremental generator should be built. Detailed feedback below.

🚦 Merge-readiness (author-acknowledged)

The PR description already flags these, so I'm just confirming they're the gating items:

1. The code implements the superseded imperative design (ADR 0062 Alternative 5). The target design is the inert RegistrationCatalog in core + AddRegistrations(...) in the DI package. The writer/pipeline still emit imperative IBrighterBuilder method bodies. This is the outstanding work.
2. samples/PROTOTYPE-RegistrationCatalog/ is throwaway and to be reverted before merge (including the hand-faked *.g.cs files).

I'd suggest splitting the ADR (which is valuable and near-final) from the implementation so the design can land while the catalog migration continues.

🐛 / ⚠️ Correctness & design

• Packaging is not wired up. Paramore.Brighter.SourceGenerators.csproj sets IsPackable=false, yet build/…props is marked Pack="true" and nothing references the generator as a packaged analyzer. As it stands the feature is only reachable via the OutputItemType="Analyzer" ProjectReference in the HelloWorld sample — it can't be consumed via NuGet. ADR defers packaging, but worth stating plainly that the feature is not shippable in this state.
• Auto class lands in the framework's namespace. BuildAutoTarget() emits BrighterAssemblyRegistrations into Paramore.Brighter.Extensions.DependencyInjection — i.e. consumer-generated code inside Brighter's namespace. That's surprising and can cause confusing IntelliSense/symbol collisions. The revised ADR design (root namespace + declared holder) resolves this; flagging in case any of it survives the migration.
• brighterAvailable re-resolves markers on every keystroke. context.CompilationProvider.Select(c => MarkerSymbols.Resolve(c).IsValid) runs against a new Compilation on each edit, so the ConditionalWeakTable misses and the 8 GetTypeByMetadataName lookups re-run per edit. Incrementality still holds (the projected bool is stable, so downstream stays Cached), but this is avoidable per-edit work — a cheaper gate (e.g. off MetadataReferencesProvider) would keep it fully cached. Minor.

🧹 Style / API surface

• SemanticModelReader, RegistrationWriter, Diagnostics, GeneratedSource, and MarkerSymbols are public purely for test access. In an analyzer assembly that's harmless at runtime, but it establishes an unintended public contract. Consider internal + InternalsVisibleTo for the test project.
• ServiceCollectionBrighterBuilder.Transforms(...) is a new public instance method in addition to the BrighterBuilderExtensions.Transforms extension. The extension-not-interface choice correctly avoids a binary break on IBrighterBuilder, but note the instance method is itself new public API on the concrete builder. Intentional and fine — just calling it out.
• The InvalidOperationException thrown by the open-generic registry cast in the generated code is good defensive practice, and symmetric with the Transforms extension. 👍

✅ Test coverage

Strong and thoughtfully layered: pure-writer unit tests, end-to-end diagnostics (BRGEN001–007), partial-class dedup, record mappers, auto/manual collision, nested-open-generic, and the incrementality assertions. The negative/positive controls in IncrementalCachingTests (temporarily breaking EquatableArray equality to watch them go red) are exactly the right instinct.

One gap: every test asserts on generated text / diagnostics / cache reasons — none compiles the generated registrations and dispatches a command through a real CommandProcessor. Since AutoFromAssemblies parity is an explicit goal (e.g. the always-emit-MapperRegistry default-mapper behaviour), a runtime integration test that builds the emitted source and verifies a handler/mapper/transform actually resolves and runs would catch emit-shape regressions the snapshot tests can't. HelloWorld exercises this manually but isn't an automated assertion.

🔒 Security

Nothing of concern — compile-time only, and emitted type names are fully-qualified from resolved symbols (not user strings), so there's no injection surface. The FNV-1a suffix on hint names to avoid Foo.Bar/Foo_Bar collisions is a nice touch.

---

Overall: excellent engineering; the main thing standing between this and merge is the author's own noted design migration to RegistrationCatalog and removing the prototype. Happy to re-review once the catalog emission lands.

_{🤖 Automated review by Claude}

[claude]

Review: source generator for handler/mapper/transform registration (1/2 — correctness & packaging)

This is a genuinely well-built generator. The read → model → write separation is real, not just claimed: SemanticModelReader is the only file touching symbols, RegistrationWriter is a pure function testable without a Compilation, diagnostics are deferred through value-equatable DiagnosticInfo/LocationInfo, and IncrementalCachingTests asserts on IncrementalStepRunReason rather than eyeballing structure. The ConditionalWeakTable memoisation in MarkerSymbols, the Distinct() for base lists split across partials, and the "don't dedup on a primary declaration" note in GetDiscoverableType all show the sharp edges were found the hard way. Analyzer release tracking is complete through BRGEN007.

Noting the PR body's own caveat that this implements the superseded imperative design and that samples/PROTOTYPE-RegistrationCatalog/ is throwaway. Comments below target the pipeline/reader, which should largely survive the migration to catalog emission.

🔴 1. Built-in pipeline handlers are never registered — [UsePolicy], [RequestLogging], [Fallback], [Timeout] break at runtime

Biggest functional gap vs AutoFromAssemblies. RegisterHandlersFromAssembly unconditionally appends the framework's own assembly to the scan set:

RegisterHandlersFromAssembly(typeof(IHandleRequests<>), assemblies, typeof(IHandleRequests<>).Assembly, ...)
// ...
assemblies = assemblies.Concat([assembly]);   // ServiceCollectionBrighterBuilder.cs:256

That is what DI-registers RequestLoggingHandler<>, ExceptionPolicyHandler<>, FallbackPolicyHandler<>, TimeoutPolicyHandler<> and the Async variants via EnsureHandlerIsRegistered. Nothing else in the DI or Hosting packages registers them.

The generator only sees the current compilation, so AddFromThisAssembly() emits none of these. Someone moving off AutoFromAssemblies who decorates a handler with [UsePolicy] gets a container resolution failure at pipeline-build time with nothing in the generated source to explain it.

Suggest an EnsureFrameworkHandlersRegistered() in the DI package that the generated method calls once, rather than hard-coding the framework handler list in the generator.

🔴 2. [BrighterRegistrations] on a nested or generic containing type emits the wrong type → CS8795

ProjectMethod flattens the containing type to a single name:

Namespace: hasNamespace ? ns!.ToDisplayString() : null,
ContainingTypeName: containingType.Name,

Roslyn's ContainingNamespace skips enclosing types, so a holder nested in App.Outer emits namespace App { internal static partial class Inner { ... } } — a different type from App.Outer.Inner. The user's partial declaration is never implemented (CS8795) and a stray App.Inner appears. Same for static partial class Registrations<T> (arity dropped). A private/protected nested holder additionally emits an accessibility illegal at namespace scope (CS1527).

Either walk the containing-type chain (MethodTarget needs a containing-type list), or add a BRGEN diagnostic rejecting nested/generic holders. Given the move to a declared-holder model, a diagnostic may be the cheap correct answer.

🔴 3. file-local types pass the reachability filter but can't be referenced from the generated file

IsReachableFromGeneratedCode only checks DeclaredAccessibility, and file class MyHandler : RequestHandler<MyCommand> reports Internal. It's discovered, then referenced from a different file → compile error. INamedTypeSymbol.IsFileLocal exists on the 4.8 floor; a !type.IsFileLocal check in IsClassifiable closes it.

🟠 4. Accessibility divergence from AutoFromAssemblies

The runtime scan filters handlers to ti.IsPublic || ti.IsNestedPublic; the generator accepts internal too. Registering internal handlers is arguably better, but the two paths no longer produce identical registrations — someone switching over picks up handlers they didn't have before. If deliberate (I'd guess so), worth a line in the ADR's parity discussion.

🟠 5. The project can't produce the NuGet package the PR describes

Paramore.Brighter.SourceGenerators.csproj sets IsPackable=false and IncludeBuildOutput=false, with no analyzers/dotnet/cs pack item. Even flipping IsPackable would ship a package containing only the build/ props and no analyzer. The build/ (not buildTransitive/) choice for direct-reference-only opt-in is exactly right — it just isn't reachable yet. Fine if packaging is deferred, but the Description/PackageTags/DevelopmentDependency metadata reads as though it isn't.

🟠 6. The 4.8 Roslyn floor is never exercised

The generator pins VersionOverride="4.8.0"; the test project takes the repo-wide Microsoft.CodeAnalysis.CSharp 5.3.0. Nothing catches an accidental 5.x-only API.

[claude]

Review (2/2 — smaller notes, test coverage, summary)

🟡 Smaller things

• Silent failure when Brighter isn't referenced. ReadMethod returns new MethodCandidate(null, null) when markers.IsValid is false and the source output silently returns. A user with a [BrighterRegistrations] method but a missing Paramore.Brighter.Extensions.DependencyInjection reference gets a bare CS8795 with no explanation. A BRGEN008 ("Brighter is not referenced") would be cheap and much kinder.
• AttributeSource mixes line endings. GeneratedSource.Header + "\n" + AttributeBody — both operands are raw string literals whose newlines follow the source file's line endings at compile time, then joined with a hard \n. RegistrationWriter goes to some trouble to normalise via HeaderLines; the attribute file bypasses it. .gitattributes probably saves you today, but the asymmetry is a trap.
• discoveryDiagnostics has no WithTrackingName. It's the one pipeline stage IncrementalCachingTests can't see, and it sits downstream of Collect() + SelectMany + Distinct() — exactly the shape most likely to lose value equality quietly.
• Analyzer assembly public surface. SemanticModelReader, RegistrationWriter, MarkerSymbols, GeneratedSource, Diagnostics, EquatableArray<T> and the whole Model namespace are public only so tests can reach them. internal + InternalsVisibleTo keeps the surface honest and matches the spirit of EnforceExtendedAnalyzerRules.
• Emitting into Paramore.Brighter.Extensions.DependencyInjection. Placing the consumer's generated BrighterAssemblyRegistrations in Brighter's own namespace makes AddFromThisAssembly() discoverable without an extra using, but the consumer's assembly then declares a type in the vendor's namespace. Moot if the auto path is dropped per Alternative 7 — flagging in case it isn't.
• Transforms extension throws for non-ServiceCollectionBrighterBuilder. Reasonable given the binary-compat constraint, but the generated code calls it unconditionally whenever a transform is discovered, so a custom IBrighterBuilder implementer gets a startup throw from code they didn't write. Worth a line in the ADR.
• ADR 0062 is missing YAML frontmatter. Every other ADR (0060, 0061 x2) opens with id/title/status/author/created/summary; 0062 starts straight at the # heading. /adr:write_adr_metadata will fix it.

🧪 Test coverage

What's covered is covered well — all seven diagnostics, records-as-mappers, partial-with-base-list-on-secondary-file, generic-split-across-partials dedup, abstract/private-nested filtering, and three real incrementality assertions including a positive control. Gaps:

1. [BrighterRegistrations] on a nested type, and on a generic containing type (item 2 in part 1).
2. A file class handler (item 3 in part 1).
3. Open generics end-to-end. OpenGenericHandler_EmitsEnsureHandlerIsRegistered and MixedClosedAndOpenHandlers_EmitsCastOnlyOnce are RegistrationWriter unit tests over a hand-built model. Nothing tests that a real class Foo<T> : RequestHandler<T> flows through SemanticModelReader.IsOpenGeneric/UnboundGenericName and yields typeof(global::App.Foo<>) — that's where the IndexOf('<') + arity arithmetic lives.
4. Nothing verifies the generated code executes. The CSharpSourceGeneratorTest cases assert emitted text. No test builds a real IServiceCollection, runs the generated AddFromThisAssembly(), and compares the resulting SubscriberRegistry/MessageMapperRegistry against what AutoFromAssemblies produces for the same types. That equivalence test is what would have caught item 1, and it's the assertion the whole feature's value rests on.
5. Two mappers for the same request type — does r.Add(typeof(X), ...) twice throw or last-write-win? Worth pinning whichever it is.

Summary

Architecture and incrementality work are solid and should carry over to the catalog design largely intact. The two I'd treat as blocking: item 1 (framework pipeline handlers), which breaks policy/logging attributes silently, and the missing round-trip test, which is the only thing that would systematically catch parity gaps like it. Items 2 and 3 are narrower but produce uncompilable output instead of a diagnostic, which is the failure mode a generator should never have.

Happy to look again once the writer/pipeline moves to RegistrationCatalog emission and the PROTOTYPE directory comes off the branch.

[claude]

Review: source generator for handler/mapper/transform registration (1/2)

Nice piece of work overall — the read → model → write split is textbook, SemanticModelReader is genuinely the only file that touches symbols, RegistrationWriter is unit-testable without a Compilation, and IncrementalCachingTests asserting on IncrementalStepRunReason is the part most generator PRs skip. MarkerSymbols memoised on a ConditionalWeakTable keyed by Compilation is the right fix for per-node symbol lookups, and build/ vs buildTransitive/ for the opt-in property is exactly right.

Below: one correctness bug, a parity gap, a test blind spot, and a convention violation. Smaller notes follow in a second comment.

---

1. EnsureFrameworkHandlersRegistered() double-registers Brighter's own scheduler handlers

ServiceCollectionBrighterBuilder.EnsureFrameworkHandlersRegistered() (ServiceCollectionBrighterBuilder.cs:263) re-scans typeof(IHandleRequests<>).Assembly. But every IBrighterBuilder comes from AddBrighter(), and BrighterHandlerBuilder already does exactly that scan (ServiceCollectionExtensions.cs:186-193) over all loaded Paramore.Brighter* assemblies — including Paramore.Brighter itself.

Open-generic pipeline handlers (ExceptionPolicyHandler<> etc.) go via EnsureHandlerIsRegistered → services.TryAdd, so those are idempotent. But Paramore.Brighter also has two non-generic, public handlers:

• FireSchedulerRequestHandler : RequestHandlerAsync<FireSchedulerRequest>
• FireSchedulerMessageHandler : RequestHandlerAsync<FireSchedulerMessage>

Those go via _serviceCollectionSubscriberRegistry.Add(...) → SubscriberRegistry.Add (SubscriberRegistry.cs:51), which appends to _observers unconditionally — no dedup.

Failure scenario, which is literally the new HelloWorld sample's call shape:

builder.Services.AddBrighter()          // registers FireSchedulerRequest -> FireSchedulerRequestHandler
                .AddFromThisAssembly(); // EnsureFrameworkHandlersRegistered() registers it again

FireSchedulerRequest is a Command, so when the in-memory scheduler fires, CommandProcessor.SendAsync hits AssertValidSendPipeline (CommandProcessor.cs:400 → :1506) with handlerCount == 2 and throws More than one handler was found for the typeof command … a command should only have one handler. Request/message scheduling is broken for anyone on the generated path.

In fairness, AddBrighter().AutoFromAssemblies() has the same latent problem today, since RegisterHandlersFromAssembly unconditionally concats the framework assembly — so it isn't newly introduced. But this is a brand-new API surface, and the new public EnsureFrameworkHandlersRegistered() documents callers … call this once with nothing enforcing it. Suggest making it genuinely idempotent (a bool _frameworkHandlersRegistered guard), or dropping the emitted call altogether since AddBrighter() already did the work.

---

2. The generated path registers internal handlers; the scanning path does not

SemanticModelReader.IsReachableFromGeneratedCode (SemanticModelReader.cs:290) accepts Public or Internal. The runtime scan filters (ti.IsPublic || ti.IsNestedPublic) (ServiceCollectionBrighterBuilder.cs:275) — public only. Mappers and transforms have no such filter on either side, so the divergence is handler-specific.

public   class GreetingHandler       : RequestHandler<GreetingCommand> { }
internal class LegacyGreetingHandler : RequestHandler<GreetingCommand> { }   // dead today

That works under AutoFromAssemblies() and throws More than one handler was found the moment the team switches to AddFromThisAssembly() — a silent behaviour change on exactly the migration path the PR is selling. Either align with the runtime filter, or make it a deliberate documented difference (registering internals is arguably the better behaviour, but then it belongs in the ADR and the release notes).

---

3. RuntimeParityTests structurally cannot detect duplicate handler registrations

HandlersByRequestType reads through IAmASubscriberRegistryInspector.GetHandlerTypes, which returns from _allHandlerTypes — a Dictionary<Type, HashSet<Type>> (SubscriberRegistry.cs:43). Dispatch walks _observers, a Dictionary<Type, List<…>>. The assertion de-duplicates exactly the thing that breaks at runtime — and both sides are equally wrong, because RunScanningPath calls HandlersFromAssemblies, which also concats the framework assembly, so both paths double-register and compare equal.

Asserting on observed dispatch (build the container and Send) would be much stronger. Also the fixture is all-public types, so it can't cover #2 — one internal handler in FixtureSource would have surfaced it immediately.

---

4. InternalsVisibleTo is explicitly prohibited by the repo's own testing guidance

.agent_instructions/testing.md, verbatim:

NEVER use InternalsVisibleTo to expose internal classes for testing.
Only test exports from an assembly … Do not test details, such as methods on internal classes.

Paramore.Brighter.SourceGenerators.csproj:42 adds one, and RegistrationWriterTests / IncrementalCachingTests drive RegistrationWriter, RegistrationModel, MethodTarget, HandlerEntry and TrackingNames directly. It's the only InternalsVisibleTo in any src/ csproj in the repo.

I get the pull — RegistrationWriter really is nicer to test as a pure function. But this assembly's exported behaviour is run the generator over a compilation, get source out, and BrighterRegistrationsGeneratorTests + AutoRegistrationTests already cover that surface. TrackingNames in particular needs no internal access — they're just strings. Worth either routing writer coverage through the driver, or getting an explicit exemption before merge.

---

5. Branch state — code doesn't implement its own ADR

Recording your own note as a merge gate: the ADR is Proposed and specifies RegistrationCatalog + AddRegistrations(...) with AddFromThisAssembly() explicitly dropped (Alternative 7), while the branch implements the superseded Alternative 5. And samples/PROTOTYPE-RegistrationCatalog/ (11 files) is committed — correctly marked throwaway and correctly absent from Brighter.slnx, but it has to come off. Per CLAUDE.md's spec workflow, landing the catalog-shaped implementation is cleaner than merging a divergent one.

[claude]

Review: source generator for handler/mapper/transform registration (2/2) — smaller findings

Registration is not idempotent, and nothing warns. Two [BrighterRegistrations] methods in one assembly, or AddFromThisAssembly() alongside AutoFromAssemblies(), both double-register. Mappers fail loudly — ServiceCollectionMessageMapperRegistryBuilder.Add throws ArgumentException("… has already been registered") — but handlers fail quietly until dispatch. A BRGEN0xx warning when a compilation contains more than one valid registration method would be cheap: hasManualRegistration already collects them, count instead of .Any().

Generated line endings are platform-dependent. CodeWriter : IndentedTextWriter over a StringWriter inherits Environment.NewLine, so the same source emits CRLF on Windows and LF on Linux. The post-init attribute file goes to real trouble to normalise to LF (BrighterRegistrationsGenerator.cs:66-67), and GeneratedSource.HeaderLines exists specifically to keep the banner consistent with the writer — but the writer itself isn't pinned. src/Directory.Build.props sets ContinuousIntegrationBuild for deterministic builds, and this leaks into EmitCompilerGeneratedFiles output and PDB content hashes. One line fixes it: NewLine = "\n"; in the CodeWriter ctor.

[GeneratedCode] stamps the assembly version (GeneratedSource.ToolVersion), so every generator version bump rewrites every generated file for every consumer. Many generators emit the tool name only, or a fixed version, to keep output stable. Minor, but it's churn you get nothing back for.

Auto class lands in Paramore.Brighter.Extensions.DependencyInjection. Emitting an internal type into a namespace the framework owns is convenient (no extra using at the call site) but it's namespace squatting in the consumer's assembly, and two generator-using assemblies joined by InternalsVisibleTo would give AddFromThisAssembly an ambiguous-call error. Worth a sentence in the ADR on why the trade is acceptable.

Transforms / EnsureFrameworkHandlersRegistered as IBrighterBuilder extensions that throw. Both are declared on the interface but throw new InvalidOperationException for any implementation other than ServiceCollectionBrighterBuilder. That's a contract the signature can't honour — an extension advertising IBrighterBuilder should work for IBrighterBuilder. Since RegistrationWriter already emits a cast for the open-generic case (RegistrationWriter.cs:157), the generated code could cast once and call the concrete instance methods, skipping the extensions entirely.

Discovery predicate cost. CreateSyntaxProvider matching every ClassDeclarationSyntax/RecordDeclarationSyntax with a base list means GetDeclaredSymbol + AllInterfaces on every base-listed type in the compilation on every keystroke. Unavoidable for interface-based discovery, but the syntactic predicate could shed a lot of it before touching the semantic model — skip declarations carrying abstract, static or file modifiers, since IsClassifiable rejects all three anyway. On a large solution that's a meaningful fraction of the nodes.

Auto-registration path has no incrementality coverage. IncrementalCachingTests never sets build_property.BrighterAutoRegistration, so all three tests exercise only the manual path. The auto path is the one most consumers will hit, and it hangs off CompilationProvider (BrighterRegistrationsGenerator.cs:194). It should be fine — the Select projects to a bool — but that's precisely the kind of thing this suite exists to prove rather than reason about.

Style / conventions

• <TargetFramework>netstandard2.0</TargetFramework> — repo convention is $(BrighterNetStandardTargetFrameworks).
• IsExternalInit.cs is the only file under src/Paramore.Brighter.SourceGenerators/ without the MIT licence #region.
• Test method names don't follow When_[condition]_should_[expected_behavior] (.agent_instructions/testing.md), and BrighterRegistrationsGeneratorTests.cs is 894 lines / ~18 cases against prefer a test case per file. Class names are correct. No //Arrange //Act //Assert markers.
• .agent_instructions/code_style.md asks for ALL_CAPS consts; the new code uses PascalCase (AttributeName, ToolName, MethodCandidates). The existing codebase is genuinely inconsistent here so I wouldn't block on it, but flagging since it's written down.
• Comment density: several comments are reviewer-facing rationale, which code_style.md routes to the ADR — e.g. Preserve the historical layout (RegistrationWriter.cs:58) refers to history that doesn't exist in a brand-new file, and Kept separate so ClassifyEntries doesn't carry a second block of nested conditionals (SemanticModelReader.cs:301) explains a refactor, not the code. The genuinely valuable why comments — the Distinct() on partial-declaration diagnostics, the ConditionalWeakTable note, the build/-not-buildTransitive/ note — are excellent and should stay.
• HelloWorld now carries a NoOpTransformer whose doc comment says it exists purely to exercise the source generator's transform discovery. Samples are documentation; transform-discovery coverage belongs in the test suite. The sample also loses its demonstration of the AutoFromAssemblies path — worth keeping one sample on each.

Security / AOT

Nothing concerning. Generated code is fully qualified with global::, hint names are sanitised with an FNV-1a suffix to avoid collisions, and the generator does no I/O. The AOT/trimming motivation holds for handlers and transforms, though the emitted r.Add(typeof(X), typeof(Y)) mapper registrations still resolve through the DI container by Type, so the mapper path isn't reflection-free end to end — worth being precise about in the ADR's trimming claims.

---

I wasn't able to build or run the test suite in this environment, so findings 1–3 in the previous comment come from reading the code rather than from a red test; #1 and #3 in particular are worth confirming with a quick Send(new FireSchedulerRequest()) against a container built the HelloWorld way.

Happy to re-review once the catalog migration lands — the pipeline architecture underneath is solid and shouldn't need to change much for it.

[claude]

Review: source generator for handler/mapper/transform registration

I read the generator, the model/writer split, the DI and core changes, ADR 0062, and all five test files. This is unusually high-quality work: the read → model → write split genuinely holds (SemanticModelReader is the only place symbols live), the ConditionalWeakTable memoisation in MarkerSymbols is the right call and correctly reasoned about, and IncrementalCachingTests asserting on IncrementalStepRunReason with a positive control is how incrementality should be proven rather than claimed. All 14 BRGEN0xx ids have test coverage. Findings below, most significant first.

1. The set-semantics fix is only half-applied — mappers still throw on exact duplicates

SubscriberRegistry.Add was changed so a repeated (requestType, handlerType) is a no-op, justified as: "AutoFromAssemblies() alongside a source-generated registration method … the caller means register the union".

But ServiceCollectionMessageMapperRegistryBuilder.Add still throws on a duplicate key even when the mapper type is identical (ServiceCollectionMessageMapperRegistryBuilder.cs:78-88, same for AddAsync). So the exact scenario the registry change exists to enable —

services.AddBrighter().AutoFromAssemblies().AddFromThisAssembly();

— throws ArgumentException at startup for any assembly containing a mapper. Likewise two [BrighterRegistrations] methods in one compilation both called from the composition root: the generator emits the full mapper set into every generated method, so calling two is guaranteed to throw. RuntimeParityTests.DuplicateMapperForSameRequestType_Throws currently pins the throw as intentional, which reads as contradicting the change made two files over. Either make an exact (message, mapper) re-add a no-op (keeping the throw for a genuinely conflicting mapper, which is what the exception text describes), or drop the union claim and document the two mechanisms as mutually exclusive.

2. Handler-visibility divergence breaks the parity claim

RegisterHandlersFromAssembly filters to ti.IsPublic || ti.IsNestedPublic (ServiceCollectionBrighterBuilder.cs:275). The generator's IsReachableFromGeneratedCode accepts public, internal and protected-internal (SemanticModelReader.cs:313-321), and ProtectedInternalNestedHandler_IsRegistered pins that.

So the generator registers handlers the scanner never would. An assembly with a public OrderHandler plus an internal OrderHandlerTestDouble : RequestHandler<PlaceOrder> works today under AutoFromAssemblies(); switching to the generator registers both and Send(new PlaceOrder()) fails with "More than one handler was found" (you'd get BRGEN012 first, but as a new warning on a previously-clean project). RuntimeParityTests can't catch this — the fixture is all-public. Adding internal class InternalHandler : RequestHandler<GreetingCommand> to FixtureSource should fail GeneratedRegistrations_ProduceSameSubscriberRegistry_AsAssemblyScanning today. Mappers/transforms have no visibility filter in the scanner, so only handlers diverge.

3. EnsureFrameworkHandlersRegistered() looks redundant, and re-adds the scan the feature removes

Every generated method emits it unconditionally, and it runs GetLoadableTypes() + GetInterfaces() over all of Paramore.Brighter.dll, twice (sync and async).

• It appears already done: AddBrighter() sweeps AppDomain.CurrentDomain.GetAssemblies() filtered to Paramore.Brighter* and calls HandlersFromAssemblies/AsyncHandlersFromAssemblies (ServiceCollectionExtensions.cs:186-192), which append typeof(IHandleRequests<>).Assembly regardless. You can't get an IBrighterBuilder without AddBrighter(), so the framework pipeline handlers are already there. RuntimeParityTests can't see this because both of its paths start from services.AddBrighter(). If that's right, the call and the new public EnsureFrameworkHandlersRegistered API can both go.
• If it is needed, it undercuts the headline benefit — a full reflection sweep of the core assembly at every startup, multiplied by the number of generated methods called, and not trim/AOT-friendly. The framework handler set is fixed at compile time; emit literal EnsureHandlerIsRegistered(typeof(...)) calls or hold a static Type[] in the DI package.

4. Shipping public API the ADR already says will be replaced

ADR 0062 is status: Proposed, and its own Deferred Follow-ups say this branch implements the superseded shape (Alternative 5) with the RegistrationCatalog/AddRegistrations migration outstanding. Merging as-is ships versioned public surface — the two attributes, BrighterBuilderExtensions.Transforms/.EnsureFrameworkHandlersRegistered, and the BrighterAssemblyRegistrations.AddFromThisAssembly() convention — that the design doc says is going away. Per the repo's spec workflow, ADR → Accepted should precede implementation merge. If the aim is to land the discovery pipeline early (it carries over unchanged), consider holding the public pieces back for a release or gating them behind a preview property so the catalog migration isn't a break.

The ADR has also drifted from the code and needs a refresh: it states IsPackable=false with no companion package project (the csproj now sets IsPackable=true and packs inline), and that the reader/writer/model types are public for testability (they're internal + InternalsVisibleTo).

5. Smaller things

• BRGEN013 is a warning that silently discards every registration. A typo'd BrighterAutoRegistration leaves a green build registering nothing — the exact silent-miss class this generator exists to kill, and the reason RegisterAutoRegistration refuses to emit an empty AddFromThisAssembly. Consider Error.
• Location.None on BRGEN007/010/011/012/013/014. BRGEN012 especially: "Request X has 3 registered handlers" with no file or line, when the pipeline already knows where they are. LocationInfo is value-equatable and already used for BRGEN005/006 — carrying one on DiscoveredEntry would make it actionable in the IDE. Also, with 2+ [BrighterRegistrations] methods, ReportDuplicateHandlers runs per method, so BRGEN012 is emitted N times for the same request.
• Generated line endings are platform-dependent. CodeWriter inherits IndentedTextWriter, so NewLine is Environment.NewLine. Setting NewLine = "\n" makes output deterministic. Corollary: BrighterRegistrationsGeneratorTests.ExpectedRegistration appends GeneratedSource.Header un-normalised while building every other line with '\n'; raw string literals keep the file's line endings, so on a CRLF checkout expected is mixed while actual is uniformly CRLF. Production code guards this (HeaderLines does .Replace("\r\n","\n")), the test helper doesn't — worth confirming green on Windows.
• Packaging deviates from Paramore.Brighter.Analyzer.Package (separate .Package project + TargetsForTfmSpecificContentInPackage + NoPackageAnalysis), and hardcodes netstandard2.0 instead of $(BrighterNetStandardTargetFrameworks).
• RegistrationWriter.Write, no-namespace branch: // Preserve the historical layout — there's no history for a new generator, and the result is a class indented a level with no enclosing block. GlobalNamespace_OmitsNamespaceWrapper doesn't assert on it.
• NoBrighterReference_GeneratesAttributesOnly: name and the // Post-init output is still emitted comment are stale (no attributes, no post-init output anymore), and it asserts nothing. Rename and add Assert.Empty(result.GeneratedSources).
• Three parallel lists of the same 14 ids (s_descriptorsById, Diagnostics, AnalyzerReleases.Unshipped.md). Reflecting over Diagnostics' static fields would collapse two of them and remove the DescriptorFor throw — an exception inside RegisterSourceOutput surfaces as CS8785 and kills the consumer's build.
• EquatableArray<T>.GetHashCode recomputes over the whole array each call, on a compilation-wide value in the hot incremental path; cache it in a field.
• Release notes: the SubscriberRegistry.Add dedup changes behaviour for any handler implementing both IHandleRequests<T> and IHandleRequestsAsync<T> — both funnel into the same Add, so that previously produced two observers.

Security

Nothing of concern. Compile-time only, over the consumer's own source; type names come from SymbolDisplayFormat.FullyQualifiedFormat (which escapes keyword identifiers), so interpolating them into emitted source is safe.

---

Items 1–3 are what I'd want resolved before merge; 4 is a process/API-stability call for the maintainers.

Reviewed statically — I couldn't run a build or the suite in this environment, so §3's redundancy claim and §5's Windows line-ending point are worth verifying rather than taking on trust.