refactor(samples): modernise CommandProcessor samples

[holytshirt]

• Use Host.CreateApplicationBuilder() instead of CreateDefaultBuilder()
• Replace GetService with GetRequiredService for safer DI resolution
• Remove Serilog dependencies in favor of built-in logging
• Use string interpolation in handlers
• Make command classes internal sealed
• HelloWorldInternalBus: switch to async handler, remove custom mapper

[claude]

Pull Request Review

Summary

This PR modernizes the CommandProcessor sample applications by adopting current .NET hosting patterns and simplifying dependencies. The changes align well with modern .NET best practices.

---

Positive Changes

1. Modern Hosting Patterns

• Switching from Host.CreateDefaultBuilder() to Host.CreateApplicationBuilder() follows the recommended .NET 6+ pattern
• Using host.Run() instead of host.WaitForShutdown() is the standard approach for console host apps
• Cleaner service registration with the builder pattern

2. Safer Dependency Injection

• GetRequiredService<IAmACommandProcessor>() instead of GetService<>() is the correct choice - it will throw if the service is not registered, catching configuration errors early rather than failing with NullReferenceException later

3. Dependency Simplification

• Removing Serilog dependencies reduces external dependencies in sample code
• Built-in logging is simpler for samples and demonstrates the framework without extra setup

4. Code Quality Improvements

• String interpolation is more readable than format strings
• internal sealed classes follow good encapsulation practices for sample code
• Consistent use of CancellationToken cancellationToken = default parameter

5. HelloWorldInternalBus Improvements

• Switching to async handler (GreetingCommandHandlerAsync) shows modern async patterns
• Removing custom mapper simplifies the sample - good for demonstrating core concepts
• Added proper MIT license header to Program.cs (was missing)
• More streamlined service registration

---

Minor Observations

1. Namespace Inconsistency (samples/CommandProcessor/HelloWorldInternalBus/GreetingCommandHandlerAsync.cs:29)

The handler is in namespace HelloWorld while the command is in HelloWorldInternalBus. This works but creates inconsistency - consider aligning the namespace to HelloWorldInternalBus for consistency.

2. tests.http Formatting (samples/WebAPI/WebAPI_Dapper/tests.http)

The JSON spacing changes are correct and follow standard JSON formatting, though this file seems unrelated to the CommandProcessor samples modernization.

---

Code Quality Assessment

Adherence to Repository Conventions:

• Follows Responsibility-Driven Design principles
• Proper use of internal sealed for sample classes
• Good separation of concerns
• No primitive obsession issues
• Simple, focused methods

Best Practices:

• No security vulnerabilities introduced
• No breaking changes to public APIs
• Async/await patterns used correctly
• CancellationToken handling is appropriate
• No resource leaks

Performance:

• No performance regressions
• Async patterns properly implemented

---

Test Coverage

Note: These are sample applications demonstrating usage patterns. They do not require unit tests themselves, but they should successfully compile, run without errors, and demonstrate best practices.

Recommendation: Verify that each sample builds and runs successfully in CI.

---

Overall Assessment

This is a high-quality refactoring PR that:

• Modernizes the samples to use current .NET patterns
• Simplifies dependencies
• Improves code safety with GetRequiredService
• Removes unnecessary complexity
• Makes the samples better teaching tools

The changes follow the repository design principles by focusing on structural improvements without altering behavior.

Recommendation: Approve with minor namespace consistency suggestion.

---

Review generated following repository CLAUDE.md guidelines and design principles.

[iancooper]

Looks good. Thanks for updating these @holytshirt

[claude]

PR Review: Modernise CommandProcessor Samples

Overall Assessment

This is a well-executed modernization of the sample applications. The changes align with .NET 8 best practices and improve code quality. ✅

Positive Changes

1. Modern Host Builder Pattern ✅

• Switching from CreateDefaultBuilder() to Host.CreateApplicationBuilder() is the correct approach for .NET 8+
• More concise and idiomatic for modern .NET applications

2. Safer Dependency Injection ✅

• Changing GetService() to GetRequiredService() is excellent
• Fails fast if dependencies aren't registered, making bugs more obvious

3. Dependency Cleanup ✅

• Removing Serilog dependencies in favor of built-in logging reduces complexity
• Appropriate for sample code that should demonstrate core functionality

4. Code Quality Improvements ✅

• String interpolation ($"Hello {name}") is more readable than String.Format()
• Making command classes internal sealed follows encapsulation best practices
• Removing ConfigureAwait() in application code is correct per Stephen Cleary's guidance

5. Handler Consistency ✅

• Renaming GreetingCommandRequestHandlerAsync → GreetingCommandHandlerAsync improves consistency
• Namespace fix in HelloWorldInternalBus (HelloWorld → HelloWorldInternalBus) resolves confusion

6. Architecture Simplification ✅

• Removing the custom GreetingCommandMessageMapper from HelloWorldInternalBus simplifies the sample
• Using async handler directly is cleaner for demonstration purposes

Issues & Concerns

1. ⚠️ Breaking Change in HelloWorldInternalBus/Program.cs:69

var commandProcessor = host.Services.GetService<IAmACommandProcessor>();
commandProcessor?.Post(new GreetingCommand("Ian"));

Issue: Using GetService() instead of GetRequiredService() is inconsistent with the other samples.

Impact: If the service isn't registered, this will silently do nothing (null reference). The null-conditional operator ?. masks the problem.

Recommendation: Change to:

var commandProcessor = host.Services.GetRequiredService<IAmACommandProcessor>();
commandProcessor.Post(new GreetingCommand("Ian"));

This matches the pattern used in HelloWorld and HelloWorldAsync samples (lines 36-38 in HelloWorld/Program.cs).

2. ⚠️ Async Consistency in HelloWorldInternalBus

The sample uses Post() (fire-and-forget) but the handler is async. While this works, consider documenting why Post() is used instead of SendAsync().

Current:

commandProcessor.Post(new GreetingCommand("Ian"));
await host.RunAsync();

The async handler will execute, but users might wonder about the mixing of sync/async patterns.

3. Minor: WebAPI_Dapper/tests.http Changes

The formatting changes to tests.http are stylistic improvements but unrelated to the PR's stated purpose. Consider whether these belong in a separate commit for clarity.

Code Style Compliance

Checking against .agent_instructions/code_style.md:

• ✅ PascalCase for public members
• ✅ Internal sealed classes
• ✅ String interpolation usage
• ✅ Nullable reference types (implicitly handled)
• ✅ Expression-bodied members where appropriate

Security Considerations

No security concerns identified. The changes are refactoring existing sample code without introducing new attack surfaces.

Performance Considerations

• ✅ Host.CreateApplicationBuilder() has better performance characteristics than CreateDefaultBuilder()
• ✅ Removing unnecessary dependencies reduces startup time
• No negative performance impacts identified

Test Coverage

⚠️ Note: These are sample applications without automated tests. Manual testing should verify:

1. All three samples still execute successfully
2. HelloWorldInternalBus properly routes messages through the internal bus
3. Handler attributes (e.g., [RequestLoggingAsync]) still function correctly

Recommendations

Required Changes

1. Fix HelloWorldInternalBus/Program.cs:69 - Use GetRequiredService() instead of GetService() for consistency and fail-fast behavior

Optional Improvements

1. Consider adding a comment in HelloWorldInternalBus explaining why Post() is used with an async handler
2. Separate the tests.http formatting changes into a distinct commit if you value atomic commits

Summary

This PR demonstrates good modernization practices and improves code quality. With the one consistency fix for GetRequiredService(), this will be ready to merge. The changes make the samples more idiomatic for .NET 8 developers and easier to understand.

Recommendation: Approve after addressing the GetRequiredService() consistency issue.

---

Review generated by Claude Code following repository conventions from CLAUDE.md

[claude]

Pull Request Review: Modernize Sample Code to .NET 8 Patterns

Overall Assessment

This is a high-quality PR that successfully modernizes 30+ sample applications to use .NET 8 hosting patterns and best practices. The changes are well-executed and remarkably consistent across the codebase.

Overall Grade: A-

What This PR Does Well

1. Consistent Migration: All samples follow the same modernization pattern using Host.CreateApplicationBuilder()
2. Improved Null Safety: Changed GetService to GetRequiredService provides fail-fast behavior
3. Modern C# Syntax: String interpolation throughout
4. Better Encapsulation: Classes properly marked as internal sealed where appropriate
5. Naming Consistency: Handler naming standardized
6. Simplified Code: Removed unnecessary boilerplate
7. Dependency Reduction: Removing Serilog simplifies samples
8. License Compliance: Added missing MIT license headers

Issues and Recommendations

High Priority - Requires Verification:

1. Message Mapper Removal in HelloWorldInternalBus - The mapper was completely removed from samples/CommandProcessor/HelloWorldInternalBus/GreetingCommandMessageMapper.cs. Need to verify the sample still functions correctly. Manual testing of HelloWorldInternalBus is required.

Medium Priority - Consider Addressing:

2. Logging Visibility - Removing Serilog without replacement configuration may reduce logging output visibility. Consider adding a comment in key samples about logging configuration.

3. ConfigureAwait Removal - This is actually correct for .NET 8 console applications. Well done!

Code Quality Analysis

Pattern Consistency: 5/5 - Excellent, same pattern across all samples
Null Safety: 5/5 - GetRequiredService is safer
Modern Syntax: 5/5 - String interpolation, top-level statements
Encapsulation: 5/5 - Proper use of internal sealed
Async Patterns: 5/5 - Correct removal of ConfigureAwait
Documentation: 3/5 - Could benefit from comments

Security and Performance

Security: No concerns. The switch to GetRequiredService improves null safety.
Performance: The sealed keyword additions provide minor performance benefits.
Breaking Changes: Changes are limited to samples, no impact on library code.

Testing Recommendations

1. Run each modified sample to verify basic functionality
2. Verify logging output is visible and useful
3. Specifically test HelloWorldInternalBus due to mapper removal
4. Test with various .NET 8 SDK versions

Final Recommendation

APPROVE WITH MINOR VERIFICATION

This PR is well-executed and ready to merge after verifying HelloWorldInternalBus functions correctly without the message mapper. The modernization brings the samples up to .NET 8 standards and makes them more accessible to developers learning Brighter.

Great work on this comprehensive modernization effort!

Review performed by Claude Code - Analyzed 1,158 additions and 1,734 deletions across 39 files

[iancooper]

@holytshirt Do you want to update, and then we can merge?

[holytshirt]

Yeah! I try later this evening or over the weekend

[claude]

PR Review: refactor(samples) - modernise CommandProcessor samples

Overall this is a solid modernisation pass. The changes are consistent, reduce boilerplate, and align the samples with current .NET idioms.

---

Positive changes

• Host.CreateApplicationBuilder() over CreateDefaultBuilder() - correct modern approach; the builder is exposed directly rather than via callback lambdas.
• GetRequiredService over GetService - eliminates silent null-reference failures. Particularly good in CompetingSender/Program.cs where the factory delegate was also updated.
• Removing Serilog from samples - reduces external dependency noise and lets the built-in Microsoft.Extensions.Logging defaults do the job.
• internal sealed on command classes - appropriate scoping for sample assemblies.
• Namespace fix in HelloWorldInternalBus - the handler was previously in the wrong HelloWorld namespace; now correctly HelloWorldInternalBus.
• Handler rename (GreetingCommandRequestHandlerAsync to GreetingCommandHandlerAsync) - cleaner naming, drops the redundant "Request" infix.
• Removal of GreetingCommandMessageMapper - correct, an InternalBus does not serialise messages to wire format so the mapper was unnecessary clutter.
• Top-level statements - dramatically reduces nesting across many files.

---

Concerns

1. Removal of .ConfigureAwait(ContinueOnCapturedContext) in HelloWorldAsync handler

// samples/CommandProcessor/HelloWorldAsync/GreetingCommandHandlerAsync.cs
-return await base.HandleAsync(command, cancellationToken).ConfigureAwait(ContinueOnCapturedContext);
+return await base.HandleAsync(command, cancellationToken);

ContinueOnCapturedContext is a protected property on RequestHandlerAsync<T> that controls whether continuation runs on the original synchronisation context. Handler code is expected to honour this flag. Dropping .ConfigureAwait(ContinueOnCapturedContext) silently changes async behaviour - in environments with a synchronisation context (e.g. classic ASP.NET, WinForms) this pattern helps avoid deadlocks and unexpected thread-hopping.

Recommendation: Restore .ConfigureAwait(ContinueOnCapturedContext) to match the base-class convention, or open a follow-up issue to decide whether it is still meaningful in samples.

---

2. HelloWorld/Program.cs - WaitForShutdown() replaced with host.Run()

-host.WaitForShutdown();
+host.Run();

WaitForShutdown() blocks waiting for CTRL+C but does not start hosted services. host.Run() both starts hosted services and blocks. For this simple sample that posts one command synchronously, this is fine today - just worth noting the behavioural difference in case anyone adds an IHostedService later.

---

3. AWS samples: services registered outside the credential guard

In GreetingsPumper/Program.cs and GreetingsReceiverConsole/Program.cs (AWS), AddHostedService<RunCommandProcessor>() / AddHostedService<ServiceActivatorHostedService>() are registered unconditionally, while AddBrighter() / AddConsumers() are only called inside the if (credentials found) block. If credentials are absent, the hosted services will fail to resolve IAmACommandProcessor at startup with a cryptic DI error.

This is a pre-existing bug carried over from the original code, but the refactor rearranged registration enough to make it more visible. Worth a follow-up fix (move AddHostedService inside the credential block, or handle the missing-credentials case gracefully).

---

4. Minor: inconsistent indentation in HelloWorldInternalBus/Program.cs

builder.Services.AddConsumers(options =>
    {   // extra indent level compared to surrounding code
        options.Subscriptions = subscriptions;

Cosmetic only.

---

Summary

The PR is in good shape and achieves its goals cleanly. The main actionable item is restoring .ConfigureAwait(ContinueOnCapturedContext) in the async handler to preserve the base-class contract. Everything else is minor or pre-existing. Happy to see this land once that is addressed.

[claude]

PR Review: refactor(samples): modernise CommandProcessor samples

Overall, this is a solid modernisation effort. The changes are consistent, purposeful, and move the codebase to modern .NET idioms.

---

Host.CreateApplicationBuilder() migration - Consistent across all samples. This is the correct modern pattern; CreateDefaultBuilder() is the legacy builder. The resulting code is more readable since service registration is no longer nested inside a lambda.

GetRequiredService<T>() everywhere - Replacing GetService<T>() (returns null) with GetRequiredService<T>() (throws) is the right default. Null-conditional operators on resolved services (commandProcessor?.Post(...)) are correctly removed now that resolution is guaranteed to succeed or throw.

Serilog removal - Reduces the dependency footprint and eliminates logging bootstrapping boilerplate. The built-in Microsoft.Extensions.Logging with the default console provider added by CreateApplicationBuilder is appropriate for samples.

Namespace bug fix in HelloWorldInternalBus - The old GreetingCommandHandler.cs was in namespace HelloWorld (wrong) while importing using HelloWorldInternalBus. The new GreetingCommandHandlerAsync.cs correctly uses namespace HelloWorldInternalBus. This was a real bug.

Deletion of GreetingCommandMessageMapper - The InternalBus passes commands directly without serialisation, so the mapper was never required. Removing it simplifies the sample and avoids misleading readers into thinking a mapper is needed for the internal bus.

CancellationToken cancellationToken = default - Correctly added to the HandleAsync override in HelloWorldInternalBus, matching the RequestHandlerAsync<T> interface signature.

internal sealed on command/handler classes - Appropriate for single-assembly samples. No external consumers, no inheritance needed.

---

Issues and Suggestions:

1. HelloWorldAsync - host is built but never started

The host is built and commandProcessor is resolved, but neither host.Run() nor host.RunAsync() is called. This pre-existed the PR (no regression), but since the file is touched it would be worth adding a comment explaining why the host is not started, to prevent readers who copy this pattern from accidentally applying it to scenarios that do require hosted services to be running.

2. Removal of .ConfigureAwait(ContinueOnCapturedContext) in GreetingCommandHandlerAsync

ContinueOnCapturedContext is a protected field on the base RequestHandlerAsync<T> class. Removing it defaults to ConfigureAwait(true) behaviour. In console apps with no SynchronizationContext this makes no practical difference, but samples serve as reference implementations. Preserving the call (or adding a comment explaining the omission) helps readers carry the correct habit into production handler code.

3. AWSTaskQueue/GreetingsPumper - conditional service registration

The pattern of conditionally registering services then calling host.Build() is technically correct (the else branch returns early before Build()). However, a short comment noting that the return is load-bearing would help future readers who might remove it thinking it is optional.

4. Whitespace noise in licence headers

Several files have trivial trailing-whitespace changes in the MIT licence block. These appear to be editor artefacts. They inflate the diff and make it harder to spot substantive changes. Worth stripping before merge.

5. CompetingReceiverConsole/RunStuff - async method with no async work

StartAsync is marked async but only does await Task.CompletedTask, which is a no-op. It should be return Task.CompletedTask (non-async). This pre-existed the PR but was surfaced by moving the class to top-level scope.

---

Summary:

Category	Assessment
Code quality	Good - consistent, purposeful modernisation
Potential bugs	Low - HelloWorldAsync host lifecycle and ConfigureAwait pattern are minor concerns
Performance	No impact - samples only
Security	No concerns
Test coverage	N/A - samples

The PR achieves its stated goals cleanly. The two things most worth addressing before merge are (1) the HelloWorldAsync host lifecycle - at minimum a clarifying comment - and (2) the whitespace noise in the licence headers. Everything else is fine as-is or pre-existing.

Reviewed with Claude Code

[codescene-delta-analysis]

No application code in the PR — skipped Code Health checks.

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.

[claude]

PR Review: refactor(samples) modernise CommandProcessor samples

[claude]

Review posted - see below for full details

[claude]

PR Review: refactor(samples) modernise CommandProcessor samples

Overall this is a well-executed modernisation. The changes are consistent and align with current .NET idioms.

Positives: Host.CreateApplicationBuilder() is the right move for .NET 7+. GetRequiredService over GetService fails fast rather than returning null. Serilog removal reduces friction for new developers. String interpolation is a minor but consistent improvement. internal sealed on command/handler types is appropriate for samples. Namespace fix in HelloWorldInternalBus (was incorrectly in namespace HelloWorld). Explicit error messages for missing AWS credentials are much friendlier than a null-reference failure downstream.

[claude]

Issue 1: ConfigureAwait(ContinueOnCapturedContext) removed without explanation

File: samples/CommandProcessor/HelloWorldAsync/GreetingCommandHandlerAsync.cs

Before: return await base.HandleAsync(command, cancellationToken).ConfigureAwait(ContinueOnCapturedContext);
After: return await base.HandleAsync(command, cancellationToken);

ContinueOnCapturedContext is a protected virtual property on RequestHandlerAsync that the framework uses to control synchronisation-context propagation. Removing ConfigureAwait silently opts into ConfigureAwait(true) behaviour. This pattern is idiomatic Brighter and its removal could mislead developers copying from this sample into omitting ConfigureAwait throughout their real handlers. Consider restoring the original pattern or adding a comment explaining why it can safely be omitted here.

[claude]

Issue 2: HelloWorldInternalBus - sync Post changed to async PostAsync (behavioural change)

File: samples/CommandProcessor/HelloWorldInternalBus/Program.cs

Before: commandProcessor?.Post(new GreetingCommand("Ian"));
After: await commandProcessor.PostAsync(new GreetingCommand("Ian"));

The async path is appropriate, but check whether any documentation or README still describes the synchronous behaviour.

Issue 3: GreetingsPumper StartAsync still calls synchronous Post inside an async loop

File: samples/TaskQueue/AWSTaskQueue/GreetingsPumper/Program.cs

This is pre-existing, but since the class was touched it is a natural opportunity to switch to PostAsync. If kept synchronous intentionally (e.g. to demonstrate outbox behaviour), a comment explaining why would help readers.

Issue 4: Noisy whitespace diff in licence headers

Several files have trailing-space removals in the licence block. These inflate the diff and distract from the meaningful changes. Consider handling whitespace-only changes in a separate commit.

Issue 5: HelloWorldInternalBus mapper removal

Deleting GreetingCommandMessageMapper is correct since the internal bus does not require serialisation. Worth confirming AutoFromAssemblies does not inadvertently attempt to resolve a mapper for GreetingCommand at runtime.

[claude]

Minor suggestions: KafkaDynamicEventStream/TaskReceiverConsole adds using System explicitly - with ImplicitUsings enabled this should be unnecessary. The explanatory comment moved to the top of GreetingsScopedReceiverConsole/Program.cs is a nice touch and belongs in a README too.

[claude]

Summary: Solid modernisation across a large surface area. Main items before merging: (1) Clarify or restore the ConfigureAwait(ContinueOnCapturedContext) pattern in GreetingCommandHandlerAsync - it is idiomatic Brighter and its removal could mislead developers copying from the sample. (2) Consider switching RunCommandProcessor.StartAsync to PostAsync for async consistency. Everything else is non-blocking or pre-existing. Nice work!