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.

[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]

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

Thanks for this — it's a genuinely high-quality piece of work. The architecture follows the recommended incremental-generator pattern faithfully: SemanticModelReader is the only thing that touches Roslyn symbols, everything downstream is value-equatable records, RegistrationWriter is a pure function, and the incrementality is actually verified via IncrementalCachingTests rather than just asserted structurally. The diagnostics-as-data (DiagnosticInfo/LocationInfo) approach and the build/-not-buildTransitive/ opt-in design are both correct and well-reasoned. ADR 0062 is excellent.

Feedback grouped by severity — most is minor; a couple are worth a look before merge.

Potential bug — partial class whose base list lives in a non-"primary" file

ReadClass is fed only by the discovery predicate node is ClassDeclarationSyntax cls && cls.BaseList is not null, so partial declarations without a base list never reach the transform. But IsPrimaryDeclaration (SemanticModelReader.cs:232) picks the primary from all DeclaringSyntaxReferences, including those without a base list:

```csharp
var primary = refs
.OrderBy(r => r.SyntaxTree.FilePath, StringComparer.Ordinal)
.ThenBy(r => r.Span.Start)
.First();
return primary.SyntaxTree == cls.SyntaxTree && primary.Span == cls.Span;
```

Consider a handler split as:

• A.cs: public partial class GreetingHandler { /* methods */ } (no base list)
• B.cs: public partial class GreetingHandler : RequestHandler<GreetingCommand> { }

A.cs is filtered out by the predicate. B.cs reaches ReadClass, but the primary (ordered by file path) is A.cs, so primary.SyntaxTree == cls.SyntaxTree is false → DiscoveryBatch.Empty → the handler is silently never registered. The dedup only works when the alphabetically-first declaration happens to carry the base list. Consider restricting the "primary" selection to declarations that have a base list, or dedup on the symbol after .Collect() instead of at the syntax level.

Packaging is described but not wired up

The ADR and PR description center on shipping an analyzer-only NuGet package (build/ props, DevelopmentDependency, the <None ... Pack="true"> item), but the csproj sets IsPackable=false and there's no companion .Package project. The repo's existing convention (Paramore.Brighter.Analyzer.Package) packs via a separate project with _AddAnalyzersToOutput placing the dll under analyzers/dotnet/cs. As it stands, dotnet pack produces nothing, so the entire build/-props distribution story is inert — only the in-repo ProjectReference / OutputItemType="Analyzer" path (used by the sample) actually works.

Fine if packaging is intentionally deferred (the csproj comments say "prototype"), but worth stating explicitly and tracking the follow-up to add the .Package project — otherwise the props mechanism this PR builds can't be exercised by real consumers yet.

Roslyn version pin / consumer compatibility

Microsoft.CodeAnalysis.CSharp is pinned at 5.3.0. A generator's referenced Roslyn version sets the minimum compiler/SDK a consumer must use. Generators are normally built against the lowest version providing the APIs they need (you use ForAttributeWithMetadataName, 4.3.1+) to maximize the range of consuming SDKs. Pinning to the newest Roslyn forces consumers onto a very recent toolchain. Worth confirming this is intended, or pinning the generator project to a lower floor.

Default method name collides with the documented hand-written convention

The auto-generated extension is BrighterAssemblyRegistrations.AddFromThisAssembly on IBrighterBuilder. The docs/tests also show users hand-writing a [BrighterRegistrations] method named AddFromThisAssembly. With auto-registration on by default, a consumer who does both gets two AddFromThisAssembly(this IBrighterBuilder) extensions in scope → ambiguous-call compile error. Not a blocker (user's choice, clear error), but a different auto name or a doc note would avoid the footgun.

Minor

• MarkerSymbols.Resolve per class (SemanticModelReader.cs:88): the discovery transform runs 8 GetTypeByMetadataName lookups for every class-with-a-base-list on every relevant edit. Cheap individually, but it's the hot path of the discovery stream — could be hoisted into a compilation-level provider and combined in, if profiling ever shows it mattering.
• PR description wording: it says "A new IBrighterBuilder.Transforms(...) callback" — but Transforms is deliberately not on the interface (extension method + concrete method on ServiceCollectionBrighterBuilder). The implementation choice is the right one (avoids a binary-breaking interface change, as the XML doc explains); just the description is slightly misleading.
• DescriptorFor throws on unknown id (BrighterRegistrationsGenerator.cs:221): a throw inside RegisterSourceOutput surfaces as a generator crash. Only reachable via internal programming error, so acceptable as a fail-fast guard — just noting it.
• build_property.BrighterAutoRegistration parsing: any value that fails bool.TryParse is silently treated as false. A typo like <BrighterAutoRegistration>tru</...> quietly disables generation with no diagnostic. Low priority, but a one-line warning would save a confusing afternoon.

Test coverage

Strong. RegistrationWriterTests exercises the pure writer without a Compilation; BrighterRegistrationsGeneratorTests and AutoRegistrationTests cover end-to-end paths and diagnostics; IncrementalCachingTests genuinely verifies IncrementalStepRunReason. Gaps worth adding:

• The partial-class-across-files scenario above (would currently fail).
• Open-generic handler emission via EnsureHandlerIsRegistered (the cast path in WriteHandlers).
• A class implementing both sync and async handler/mapper interfaces.
• [ExcludeFromBrighterRegistration] on a discovered type.

Overall: solid design, clean code, well-tested. The partial-class discovery gap and the packaging-vs-intent mismatch are the two things I'd want resolved (or explicitly deferred) before this is considered done.

🤖 Generated with Claude Code

[claude]

Review: source generator for handler/mapper/transform registration

I read through the generator (SemanticModelReader, RegistrationWriter, BrighterRegistrationsGenerator), the model/pipeline records, the DI extension changes, the build props gating, and the test suite. This is high-quality work — easily one of the cleaner incremental source generators I've reviewed in this codebase. Detailed feedback below.

What's strong

• Architecture follows the Dollard reader→model→writer pattern faithfully. SemanticModelReader is the only place that touches ISymbol/Compilation; everything downstream is value-equatable records. RegistrationWriter is a pure RegistrationModel → string and is unit-testable without a Compilation. This is exactly right.
• Incrementality is genuinely verified, not just structurally plausible. IncrementalCachingTests drives CSharpGeneratorDriver with trackIncrementalGeneratorSteps and asserts on IncrementalStepRunReason per named stage (TrackingNames), including a negative control (trailing-comment edit → Cached/Unchanged) and a positive control (new handler → Modified). The note about "breaking EquatableArray equality and watching it go red" is the correct way to validate this.
• MarkerSymbols.Resolve memoised by ConditionalWeakTable<Compilation,…> correctly collapses the 8 metadata lookups to once-per-pass with weak keys — no leak across edits.
• Mapping availability/options to bool early (brighterAvailable, autoEnabled) so the volatile CompilationProvider/AnalyzerConfigOptionsProvider don't poison downstream caching is the right call.
• build/ vs buildTransitive/ gating for BrighterAutoRegistration is a clever way to make auto-registration apply only to direct PackageReferences, and it's clearly documented in the props file and ADR.
• API surface choice (Transforms as an extension on IBrighterBuilder rather than an interface member) avoids a binary-breaking change — good instinct, and the rationale is captured in XML docs.
• Analyzer release tracking (AnalyzerReleases.*.md) instead of suppressing RS2008; [GeneratedCode] stamping; DescriptorFor throwing on unknown id to catch missed updates — all nice touches.
• I verified the emitted call shapes against the real APIs: IAmASubscriberRegistry.Register<TRequest,TImpl>(), RegisterAsync<,>, ServiceCollectionSubscriberRegistry.EnsureHandlerIsRegistered, ServiceCollectionMessageMapperRegistryBuilder.Add/AddAsync(Type,Type), and ServiceCollectionTransformerRegistry.Add(Type) all match what the writer generates.

Potential bugs / correctness

1. UnboundGenericName is wrong for nested generic types (SemanticModelReader.cs:273). It inserts arity-1 commas after the first <, so a handler like Outer<T>.Inner<U> would emit global::Outer<,> instead of global::Outer<>.Inner<>, and type.TypeParameters.Length is the type's own arity, not the total. Relatedly, a non-generic handler nested inside an open-generic outer (Outer<T>.Handler : IHandleRequests<Cmd>) is classified as closed (since Handler.IsGenericType == false), so the writer emits r.Register<Cmd, global::Outer<T>.Handler>() referencing an unbound T — which won't compile. These are admittedly rare shapes, but today they fail at the generated-code level rather than with a clean BRGEN diagnostic. Consider handling nested generics, or detecting "type is reachable only through an open-generic container" and emitting BRGEN005-style guidance. A test for the nested case would be worth adding.

2. Discovery diagnostics aren't de-duplicated (BrighterRegistrationsGenerator.cs:120-121). Entries get collapsed by RegistrationModel.From's .Distinct(), but discoveryDiagnostics just SelectManys the per-batch DiagnosticInfos. A generic mapper/transform whose base list is split across two partial declarations reaches ReadClass twice and produces two identical BRGEN005 records (same type.Locations.FirstOrDefault() location). Worth confirming whether the compiler de-dups exact-equal generator diagnostics; if not, a .Distinct() on the flattened diagnostics would make this robust.

Design / UX observations (not blocking)

3. Auto class is silently suppressed when any valid manual [BrighterRegistrations] method exists (BrighterRegistrationsGenerator.cs:152-154, 184). The suppression itself is correct (avoids double registration / ambiguous AddFromThisAssembly), and it's documented in ADR 0062 — but a user who opted into auto-registration and hand-wrote a method gets no signal that the auto path vanished. A low-severity info diagnostic ("auto-registration suppressed because a manual registration method is present") would save a confusing debugging session.

4. BrighterBuilderExtensions.Transforms throws InvalidOperationException for any non-ServiceCollectionBrighterBuilder (BrighterBuilderExtensions.cs). Reasonable given the registry lives on the concrete type, but it means an alternative IBrighterBuilder implementer hits a runtime failure for an API that looks interface-level. The XML doc calls this out, so it's a conscious trade-off — just flagging.

Minor / nits

• Mixed line endings in generated output. RegistrationWriter uses IndentedTextWriter (newline = Environment.NewLine), but GeneratedSource.Header is a raw-string literal carrying \n. On Windows the emitted file will mix LF (inside the banner) and CRLF (everywhere else). Cosmetic — doesn't affect compilation — but if the snapshot tests ever run on Windows CI they'll need newline normalization.
• AccessibilityModifier maps ProtectedAndInternal → "private protected" and ProtectedOrInternal → "protected internal" correctly; the _ => "internal" fallback is a safe default. Fine as-is.

Test coverage

Coverage is strong (~1100 lines across writer unit tests, end-to-end generator tests for BRGEN001–005, partial-class dedup, auto/manual collision, record mappers, IsClassifiable filtering, and the incrementality assertions). Gaps that line up with the findings above: (a) nested-generic / open-generic-container handler shapes (#1), and (b) duplicate-diagnostic de-dup across split partials (#2).

Overall: solid, well-tested, well-documented feature. The two correctness items (#1, #2) are edge cases and the rest are polish — none look like blockers for the core scenario.

Reviewed by Claude (Opus 4.8).

[claude]

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

Reviewed against CLAUDE.md and the repo conventions. Overall this is a high-quality, well-architected PR. The incremental-generator design is textbook: a clean read → model → write separation, a Roslyn-free value-equatable pipeline, ForAttributeWithMetadataName for the cheap attribute path, marker-symbol memoisation keyed on the Compilation via a weak table, and — notably — incrementality that is verified (IncrementalStepRunReason) rather than merely asserted structurally. The RegistrationWriter/CodeWriter split makes the emitter unit-testable without a Compilation, and the diagnostics (BRGEN001–007) are tracked in AnalyzerReleases.*.md. Nicely done.

A few items worth addressing before merge, ordered by importance.

1. Roslyn version — pin to the lowest supported, not the latest (important)

Directory.Packages.props adds Microsoft.CodeAnalysis.CSharp 5.3.0, and the generator references it. A source generator is loaded by the consumer's compiler, so it can only run in an SDK/IDE whose Roslyn is ≥ the version it was built against. Pinning to 5.3.0 means the generator silently won't run (or emits CS9057) for anyone on an older SDK — a large fraction of Brighter's install base. The convention for shipped generators is to reference the minimum Roslyn you actually need (you use ForAttributeWithMetadataName, so 4.3.x is the floor) with PrivateAssets="all". The unit-test project can use whatever current version it likes; only the generator assembly's reference gates compatibility.

2. Packaging isn't wired up (please confirm intent)

The PR description is framed entirely around "add the package to the top-level project", but Paramore.Brighter.SourceGenerators.csproj sets <IsPackable>false</IsPackable> and there is no <None Include="$(OutputPath)\$(AssemblyName).dll" Pack="true" PackagePath="analyzers/dotnet/cs" Visible="false" /> item (with IncludeBuildOutput=false, nothing pins the generator dll into the package). A repo-wide search found no shared analyzer-packaging target either. As-is, no consumable NuGet is produced and the build/…props opt-in mechanism never ships. If shipping is intentionally deferred to a later PR that's fine — but worth stating, since the description implies otherwise. (The sample works only because it uses a ProjectReference with OutputItemType="Analyzer".)

3. Hard cast for open-generic handlers (minor robustness)

RegistrationWriter.WriteHandlers emits var registry = (ServiceCollectionSubscriberRegistry)r; for open generics. The generated method's parameter is IBrighterBuilder and the callback's r is IAmASubscriberRegistry. In the DI path this is always the concrete registry so it's safe, but a custom IBrighterBuilder/registry would get an InvalidCastException with no context — whereas the new Transforms extension you added throws a clear InvalidOperationException. Consider an as-cast + descriptive throw for symmetry, or at least a comment that the open-generic path is DI-only.

Things I checked that are correct

• Partial-declaration de-dup: emitting type.Locations.FirstOrDefault() (stable first declaration) makes the Distinct() on DiagnosticInfo actually collapse the duplicate BRGEN005/006 from split partials — GenericMapperSplitAcrossPartials_ReportsSingleBRGEN005 covers it. Subtle and right.
• Incrementality under CompilationProvider churn: projecting to bool (brighterAvailable) and to value-equatable records before any Combine means per-keystroke compilation identity changes don't invalidate downstream — good.
• DiscoveredEntry carries no Location, so unrelated edits above a handler don't perturb the cached source output (only the diagnostics stream, which is acceptable).
• EquatableArray equality/hash, ConditionalWeakTable thread-safety, deterministic FlattenAndSort ordering, IsReachableFromGeneratedCode accessibility walk, and internal additive API (Transforms kept off IBrighterBuilder to avoid a binary break) all look correct.

Test coverage

Strong — 33 tests spanning the pure writer, generator behaviour, auto-registration on/off/suppressed, and verified incrementality. Two suggestions:

• An end-to-end test (or assertion in the sample) that the generated registrations actually compile and resolve a handler/mapper/transform from the container at runtime — the current tests assert on emitted text, not runtime behaviour.
• A negative test for BrighterBuilderExtensions.Transforms(...) against a non-ServiceCollectionBrighterBuilder (the InvalidOperationException path).

Nothing here is blocking on correctness; #1 and #2 are the two I'd want resolved (or explicitly deferred) before this is consumed as a package.

🤖 Generated with Claude Code

[claude]

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

Thanks for this — it is a well-architected PR. The Kathleen Dollard read → model → write structure is followed faithfully: SemanticModelReader is the only place that touches Roslyn symbols, everything downstream is value-equatable records, and RegistrationWriter is a pure function that is unit-testable without a Compilation. The IncrementalCachingTests that assert on IncrementalStepRunReason (rather than just structural shape) are exactly the right way to prove incrementality, and the positive/negative controls there are excellent. Diagnostics with release tracking (AnalyzerReleases.*.md), the FNV-hashed hint names to avoid collisions, and the ConditionalWeakTable memoisation of MarkerSymbols are all solid touches.

A few things worth a closer look.

Potential bug — default message mapper is not registered on the handlers-only path

AutoFromAssemblies always calls MapperRegistryFromAssemblies, which calls EnsureDefaultMessageMapperIsRegistered() unconditionally — so the default CloudEventJsonMessageMapper/...Async are always added to the container.

The generated path only emits a MapperRegistry(...) block when at least one mapper is discovered (RegistrationWriter.WriteMappers returns early if both lists are empty). For an assembly that has handlers/transforms but no message mappers, the generated AddFromThisAssembly never calls MapperRegistry(...), so EnsureDefaultMessageMapperIsRegistered() never runs and the default mappers are absent from DI — a behavioural divergence from the AutoFromAssemblies path it is meant to replace.

Notably the HelloWorld sample itself exercises this path (handler + NoOpTransformer, no mapper). It works there only because it does internal Send with no mapping. Could you confirm this is intended? If parity is the goal, the generated method probably needs to call MapperRegistry(...) (or otherwise ensure the default mapper is registered) even when no mappers are discovered.

Diagnostics fire even when nothing is generated

discoveryDiagnostics is registered as its own RegisterSourceOutput, independent of whether any [BrighterRegistrations] method or the auto-class is being emitted. So a consumer that references the analyzer with BrighterAutoRegistration=false and no manual method will still get BRGEN005/BRGEN006 warnings for any generic mapper / nested-in-open-generic type in their compilation, despite the generator producing no registration for them. That may be surprising/noisy. Consider gating discovery diagnostics on whether something is actually being generated, or documenting that they are emitted whenever the analyzer is present.

Minor

• GeneratedSource.ToolVersion embeds the generator assembly version into every emitted file via [GeneratedCode(...)]. A version bump changes the generated source text (and invalidates the output cache) on every consumer even when nothing else changed. A common and acceptable tradeoff, just flagging it.
• Generated output uses the writer Environment.NewLine, so line endings are platform-dependent. Current Assert.Contains tests are fine; just be aware before adding any snapshot/golden-file tests.
• hasManualRegistration only counts candidates where Method is not null, so an invalid manual [BrighterRegistrations] method (which produces a diagnostic) does not suppress the auto-class. That seems reasonable, but worth a confirming test if not already covered.

Things I checked that look correct

• Open-generic handler cast (ServiceCollectionSubscriberRegistry)r is valid in both the Handlers and AsyncHandlers callbacks (both are handed the concrete subscriber registry).
• UnboundGenericName arity handling is correct for arity >= 1.
• Records-with-base-list are picked up but filtered to TypeKind.Class, excluding record structs.
• Keeping Transforms off IBrighterBuilder (extension method) to avoid a binary-breaking interface change is a nice call, and symmetric with Handlers/MapperRegistry.
• EquatableArray<T> value semantics and the partial-declaration Distinct() dedup are sound.

Overall this is high quality. The default-mapper question is the one I would want resolved before merge; the rest are minor.

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.