[iancooper]

What this fixes

Two receivers in samples/TaskQueue/MsSqlMessagingGateway/ could never have started. Both built their subscriptions as Subscription<T>, and MessagingGateway.MsSql's ChannelFactory downcasts to MsSqlSubscription and throws in all three of its channel-creation methods (ChannelFactory.cs:46, :66, :88). Measured, with a control:

Subscription<T>      : ConfigurationException: MS SQL ChannelFactory We expect an
                       MsSqlSubscription or MsSqlSubscription<T> as a parameter
MsSqlSubscription<T> : accepted

It compiles, which is why nothing caught it. Two further applications in the same folder were also unstartable: CompetingSender passed a bare new Publication() and threw ConfigurationException: Topic is missing from the publication, and CompetingReceiverConsole defaulted to a Proactor pump against a sync handler.

GreetingsSender had no Outbox and no host. At the base commit it built a bare ServiceCollection, took .Create() on a producer registry and called Post — no Outbox, no Box Provisioning, no Host. This PR adds all three, and the ordering matters: UseBoxProvisioning registers an IHostedService, and BoxProvisioningHostedService is the only caller of ProvisionAsync in src/, so the sender uses StartAsync to let provisioning complete before the send. (An earlier revision of this description said the call existed and nothing ran it; that described a snapshot from partway through the work, not the base.)

The global InboxConfiguration was silently doing nothing. It reaches the pipeline only through AddEventBus's ExternalBus arms; with no producers the NoExternalBus branch is taken, and that overload accepts no inbox. Measured: 0 Inbox rows with no producer registered, 1 with one. De-duplication now comes from [UseInbox] on the handler, which does not depend on a bus; InboxConfiguration remains as what supplies the store.

What this adds

One database carrying the queue, the Outbox and the Inbox, and the sample now runs from a clean machine.

• The queue table is provisioned by the sample, in SampleInfrastructure/QueueTableProvisioner.cs, guarded and idempotent. Its catalog lookups take the names as parameters — the values are constants, so nothing is injectable, but a sample is a copy-paste source. The MSSQL gateway has no provisioning path — OnMissingChannel.Create is inert there — so MsSqlQueueBuilder.GetDDL/GetIndexDDL are public precisely for callers to run. This is now their only production call site in the repository.
• BrighterSqlQueue.sql is 13 lines of portable UTF-8 creating only the database. It was a 2018 SQLEXPRESS dump: UTF-16, with data files pinned to C:\Program Files\...\MSSQL12.SQLEXPRESS\, so it could not run off Windows.
• Each process provisions the box it owns, so either can be started first — including concurrently, which the provisioner handles.
• A README.md, documenting the changed setup contract: script → database, applications → tables.
• All four applications read ConnectionStrings:Brighter, defaulting to local SQL Express with TrustServerCertificate=True — required, since Microsoft.Data.SqlClient (pinned 7.0.2) defaults Encrypt to true.
• Both receivers call .ValidatePipelines(), so a pump/handler mismatch is a named startup error rather than a per-message failure.
• A SampleInfrastructure project holding the provisioner and SampleDatabase, so Events carries contracts and no database adapter — it is the assembly every application hands to AutoFromAssemblies.
• A test pinning the downcast, in tests/Paramore.Brighter.MSSQL.Tests/MessagingGateway/, needing no database: three channel-creation methods throw ConfigurationException for a plain Subscription<T>, and a control gets a channel for MsSqlSubscription<T>. Red-proofed by neutering the three guards — 3 failed, the control passed.

Testing

Proven by running, on a fresh SQL Server 2022 container with the .sql run once, no manual table creation, committed source and one environment variable:

two receivers started CONCURRENTLY on a database with zero tables:
  instance A: 0 race errors, instance B: 0 race errors
  tables: __BrighterMigrationHistory, InboxMessages, QueueData
  IX_QueueData_Topic present exactly once

convergence: index dropped by hand, recreated on next start, 0 errors

round trip:
  sender: Provisioned Outbox 'Outbox' / Decoupled invocation
  delivery 1, message id X : handler ran 1 time, 0 validation errors
  delivery 2, message id X : handler ran 0 times
    warn: UseInboxHandler  Command 01a08bdb-... has already been seen
  InboxMessages rows: 1

The Competing pair now round-trips, and #4338 was this sample's own defect

It sent inside a TransactionScope it deliberately never completed, on the premise that a message is queued whether the transaction commits or aborts. Post opens a SqlConnection, Enlist defaults to true, so the insert joined that transaction and rolled back with it. True of a broker outside your database; false when the broker is your database.

default connection string : 3 Posts, no errors, QueueData 0 rows
...;Enlist=False          : 3 Posts, no errors, QueueData 3 rows
with scope.Complete()     : 6 sent, handled 2 and 4 by two receivers, queue drained

#4338 closes with this PR.

Filed from this PR

#4334 validation spec for subscription vs ChannelFactoryType · #4335 NoExternalBus discarding InboxConfiguration · #4336 InboxScope inert · #4337 CommandProcessor.Call's "fpr" typo naming the wrong type · #4338 the Competing pair, closed by this PR · #4339 a test pinning the ChannelFactory downcast, now closed by the test in this PR · #4340 superseded by the README here.

Why

This accompanies a Brighter documentation guide on composing MSSQL as transport, Outbox and Inbox, and the sample is what the guide points readers at. Raised separately because the subscription fix stands on its own.

🤖 Generated with Claude Code

https://claude.ai/code/session_0146UueHL6H3zGBGTYwz7GtL

[claude]

Review

I checked the claims in the description against the code rather than taking them on trust, and the headline fix holds up: ChannelFactory.CreateSyncChannel / CreateAsyncChannel / CreateAsyncChannelAsync each do subscription as MsSqlSubscription and throw ConfigurationException on null (src/Paramore.Brighter.MessagingGateway.MsSql/ChannelFactory.cs:46, :65, :88), so both receivers really were dead on Dispatcher start. Good catch, and both MsSqlMessagingGateway receivers are covered — no other sample in samples/ references MessagingGateway.MsSql with a subscription, so the fix is complete.

One blocking issue in the new Outbox/Inbox work, then some smaller notes.

🔴 Blocking — box provisioning never runs in GreetingsSender, so the Outbox/Inbox tables are never created

UseBoxProvisioning does not provision anything itself; it registers a hosted service:

// src/Paramore.Brighter.BoxProvisioning/BrighterBuilderBoxProvisioningExtensions.cs:93
builder.Services.TryAddEnumerable(ServiceDescriptor.Singleton<IHostedService, BoxProvisioningHostedService>());

BoxProvisioningHostedService.StartAsync is the only thing in the codebase that resolves IAmABoxProvisioner and calls ProvisionAsync — I grepped, there is no other consumer. GreetingsSender builds a bare container:

var serviceProvider = serviceCollection.BuildServiceProvider();   // no IHost
var commandProcessor = serviceProvider.GetRequiredService<IAmACommandProcessor>();

With no IHost, nothing ever calls StartAsync, so the two provisioners registered by AddMsSqlOutbox / AddMsSqlInbox are never invoked. BrighterSqlQueue.sql creates only QueueData (I read it — one CREATE TABLE [dbo].[QueueData] plus its index), MsSqlOutbox has no create-if-missing path (SqlOutboxBuilder is a DDL helper with no runtime call site), and MsSqlInbox likewise. So the run order is:

1. DepositPost → Invalid object name 'Outbox'
2. and had it got that far, the receiver's MsSqlInbox → Invalid object name 'InboxMessages'

which means the code comment above the call is not true as written:

// Creates and migrates the Outbox and Inbox tables at startup.

The sibling PostgreSQL sample gets this right and is worth copying verbatim — it uses a host and explains exactly why (samples/TaskQueue/PostgresTaskQueue/GreetingsSenderWithOutbox/Program.cs:112-117):

var host = builder.Build();

// StartAsync rather than RunAsync, because we have work to do between starting the host and
// waiting on it. Starting is what provisions the Outbox table and starts the Sweeper, so it
// has to happen before the send rather than after.
await host.StartAsync();

Converting GreetingsSender to Host.CreateApplicationBuilder + await host.StartAsync() also brings it in line with the two receivers in the same folder, both of which are already hosts. If you'd rather keep the bare-container shape, the explicit alternative is to drive the provisioners yourself before the first DepositPost (outboxes before inboxes — the ordering BoxProvisioningHostedService does for you), but the host route is less to explain in a sample whose job is to be copied.

Given the sample can't reach SQL Server in CI, this is exactly the class of defect the description's "the rest is a build" caveat leaves open — the same shape as the Subscription<T> bug being fixed here: compiles clean, dies on first contact with the database.

🟡 Inbox provisioning is registered in the process that doesn't own the Inbox

Even once the above is fixed, the sender provisions the table the receiver reads, so the sample only works if the sender is started before the receiver handles its first message. GreetingsReceiverConsole is already a host, so .UseBoxProvisioning(o => o.AddMsSqlInbox(configuration)) there would actually execute and would make each process responsible for its own box. Reasonable to keep it in the sender if the guide's narrative is "one database, provisioned once" — but then the comment should say the receiver depends on the sender having run first, because a reader starting the receiver alone will hit a missing table.

🟡 The shared configuration is duplicated, and the comment promises it isn't

GreetingsReceiverConsole/Program.cs:41 says "The same three tables the sender names" — true today, enforced by nothing. Connection string plus three table names are copy-pasted across two files; the first reader who changes InboxMessages in one place gets a silent mismatch (sender provisions InboxMessages, receiver reads Inbox, no error until the query runs). Either add a short comment on both sides pointing at the other file, or note in the guide that the two must agree.

Minor, in the same area:

• The receiver's RelationalDatabaseConfiguration sets outBoxTableName: "Outbox" although the receiver has no Outbox. Harmless, but it's one more line a reader has to decide is significant.
• inboxTableName: "InboxMessages" overrides the default (INBOX_TABLE_NAME = "Inbox", src/Paramore.Brighter/RelationalDatabaseConfiguration.cs:7). Taking the default in both processes would remove a parameter and remove the drift risk above. If InboxMessages is deliberate, a word on why would help.

🟢 Things I checked that are fine

• OnceOnlyAction.Warn does short-circuit rather than warn-and-continue (UseInboxHandler.cs:116-119 returns the request without calling base.Handle), so "a redelivered message is recognised rather than reprocessed" is accurate. Worth noting for readers that Warn means the duplicate is silently dropped after a log line — fine for a sample, but Throw is what you want if a caller needs to know.
• MsSqlInbox derives from RelationalDatabaseInbox, which implements both IAmAnInboxSync and IAmAnInboxAsync, so it works with the MessagePumpType.Reactor subscription this sample uses (AddConsumers registers whichever interfaces the inbox implements).
• The IAmARelationalDatabaseConfiguration registration is genuinely required — configure.TransactionProvider = typeof(MsSqlTransactionProvider) is container-activated and MsSqlConnectionProvider asks for that interface. The comment earns its place.
• The claim about the queue table is right: nothing in MessagingGateway.MsSql acts on OnMissingChannel, and MsSqlQueueBuilder has no call site anywhere in src/, tests/ or samples/ other than your new comment. If you want the sample to be runnable without the manual step, MsSqlQueueBuilder.GetDDL("QueueData") + GetExistsSQL would let the sender create the queue table too — optional, and arguably better as its own PR.
• GetService → GetRequiredService and dropping the stray BOM are both improvements.

Test coverage

Samples aren't unit tested, and the solution build is the only guard — which by construction can't catch either of these two bugs. Not asking for sample tests here, but the underlying contract is untested too: tests/Paramore.Brighter.MSSQL.Tests/MessagingGateway/ covers scheduler forwarding and requeue, and nothing asserts that the ChannelFactory rejects a non-MsSqlSubscription. A small When_creating_a_channel_with_a_non_mssql_subscription test would document the constraint this PR just paid for discovering, and per CLAUDE.md that one belongs in /test-first on its own branch.

Happy to re-review once the provisioning path actually executes.

[iancooper]

Both points were right, and the blocking one was right for the reason you gave — I verified it before changing anything: UseBoxProvisioning registers an IHostedService (BrighterBuilderBoxProvisioningExtensions.cs:92), and BoxProvisioningHostedService.cs:89 is the only caller of ProvisionAsync anywhere in src/. A bare BuildServiceProvider() never starts hosted services, so my comment was simply false.

Fixed in 807b7e7e4.

1. GreetingsSender is a host now

Converted to Host.CreateApplicationBuilder + await host.StartAsync(), copying the PostgreSQL sibling you pointed at — including its reason for StartAsync over RunAsync.

2. Each process provisions the box it owns

Sender provisions the Outbox, receiver provisions the Inbox. The receiver can now be started on its own, which was your second point.

3. A third one, which only running it surfaced

The global InboxConfiguration was doing nothing at all. It reaches the pipeline only through the ExternalBus overloads (ServiceCollectionExtensions.cs:657-676); with no producers registered the NoExternalBus branch is taken, and that overload accepts no inbox. So a consumer-only process — the natural shape for a receiver — gets no Inbox and no warning.

Measured, with a control:

InboxMessages rows, receiver with NO producer registered : 0   (table existed, was provisioned)
InboxMessages rows, receiver WITH a producer registered  : 1

GreetingEventHandler now carries [UseInbox], which does not depend on there being a bus. The InboxConfiguration registration stays, because it is what supplies the store.

Related, and worth knowing: InboxScope is referenced nowhere in src/ outside its own declaration — 0 hits, against 43 for OnceOnlyAction as a control. The scope: argument is inert. I have not used it here.

Proven by running, not building

You were right that the description's "the rest is a build" caveat left this open, so I stopped leaving it open. SQL Server 2022 in Docker, the queue table created from MsSqlQueueBuilder's own DDL:

Provisioning Outbox 'Outbox'...          Provisioned Outbox 'Outbox' successfully
Provisioning Inbox 'InboxMessages'...    Provisioned Inbox 'InboxMessages' successfully

delivery 1, message id X : handler ran 1 time
delivery 2, message id X : handler ran 0 times
  warn: Paramore.Brighter.Inbox.Handlers.UseInboxHandler
        Command 01a08aef-c8a4-738e-ad13-317ce5686212 has already been seen
InboxMessages rows after both : 1

Connection strings in the diff are unchanged from the original SQLEXPRESS ones; the Docker run used a temporary edit that is not committed.

On your two remaining notes

• Duplicated configuration — both files now carry a comment pointing at the other and saying the values must agree. The receiver no longer sets outBoxTableName, since it has no Outbox.
• InboxMessages vs the Inbox default — kept deliberately, because the Brighter documentation's MSSQL Inbox page uses that name and this sample is what the new guide points readers at. Happy to take the default instead if you would rather the sample be minimal.

On the two things you suggested as separate work

Both sound right and neither is in this PR: the ChannelFactory-rejects-a-non-MsSqlSubscription test, and having the sender create the queue table via MsSqlQueueBuilder. The InboxConfiguration-silently-ignored behaviour in (3) also looks like a product defect rather than a documentation one — a ConfigurationException, or honouring the inbox on the NoExternalBus path, would both beat silence. Say the word and I will raise any of the three; I would rather not open issues on your repo uninvited.

[claude]

Review

I checked the claims in this PR against the source rather than taking them on trust. The central fix is real and the reasoning holds up. Notes below are ordered by what I think matters.

Verified

• ChannelFactory does downcast in all three methods — ChannelFactory.cs:46, :66, :88 (the body says :65 for the second; trivial). A plain Subscription<T> throws ConfigurationException at dispatcher-build time. The fix is correct.
• The switch to MsSqlSubscription<T> is behaviour-preserving where args were omitted: Subscription<T> and MsSqlSubscription<T> both default messagePumpType to Proactor (Subscription.cs:291), so CompetingReceiverConsole — which doesn't pass one — doesn't silently change pump. Positional order matches too (3rd positional is routingKey in both).
• The "global InboxConfiguration is inert without a bus" finding in commit 2 is correct: AddEventBus passes serviceActivatorOptions?.InboxConfiguration only on the two ExternalBus arms (ServiceCollectionExtensions.cs:659, :670); !hasEventBus takes NoExternalBus() at :655, which accepts no inbox. And the [UseInbox] route works because AddConsumers registers IAmAnInboxSync from options.InboxConfiguration.Inbox (ServiceActivator ServiceCollectionExtensions.cs:45-58), which is exactly what UseInboxHandler<T>'s ctor takes. MsSqlInbox → RelationalDatabaseInbox : IAmAnInboxSync, IAmAnInboxAsync, so the Reactor pump gets a sync inbox. Consistent.
• await host.StartAsync() before DepositPost is the right shape: BoxProvisioningHostedService.StartAsync awaits each ProvisionAsync to completion. Hosted-service ordering in the receiver is also right — UseBoxProvisioning runs inside the AddConsumers(…) chain, so its TryAddEnumerable lands before AddHostedService<ServiceActivatorHostedService>() and the tables exist before the pump starts. Worth noting that this ordering is load-bearing and invisible; a one-line comment would protect it from a future reorder.
• The IAmARelationalDatabaseConfiguration registration is genuinely required — MsSqlTransactionProvider/MsSqlConnectionProvider ctors take exactly that interface.
• InboxScope really is dead in src/ (3 occurrences, all inside InboxConfiguration.cs). Good catch — that deserves its own tracking issue, not just a commit-message footnote.

1. The framework should be catching this, not the sample

The bug is fixed in two files, but the class of bug is untouched and will recur in the next sample or the next user's app. Two things that already exist could close it:

• Subscription.ChannelFactoryType (Subscription.cs:172, overridden by MsSqlSubscription to typeof(ChannelFactory)) — currently read only by CombinedChannelFactory.
• RegisterConsumerValidationSpecs already registers four ISpecification<Subscription> rules (PumpHandlerMatch, HandlerRegistered, RequestTypeSubtype, UnwrapTransformResolvable) — and none checks that a subscription is compatible with the channel factory it will be handed.

A fifth rule comparing (subscription.ChannelFactory ?? options.DefaultChannelFactory).GetType() against subscription.ChannelFactoryType would turn "compiles, then dies as the Dispatcher builds channels" into a named startup error for every transport. That's a /test-first follow-up per CLAUDE.md, not something to bolt onto this PR — but it is the change that makes the explanatory comments in these two files unnecessary.

Separately: the PR says the ChannelFactory behaviour "was run rather than read", but nothing in the repo pins it now. A short test in tests/Paramore.Brighter.MSSQL.Tests asserting ConfigurationException for a plain Subscription<T> would keep that observation from decaying into a comment.

2. As committed, the sample cannot be run by the reader who most needs it

Three files hardcode Server=.\sqlexpress;Integrated Security=SSPI — Windows-only. The commit message is candid that the Docker verification "used a temporary edit that is not in this diff", i.e. the committed form is not the form that ran. For a sample that exists to back a documentation guide, that is the gap I would close first.

Both csprojs already reference Microsoft.Extensions.Configuration.Binder and Microsoft.Extensions.Configuration.EnvironmentVariables and use neither. One line each —

var connectionString = builder.Configuration.GetConnectionString("Brighter")
    ?? @"Database=BrighterSqlQueue;Server=.\sqlexpress;Integrated Security=SSPI;";

— keeps the existing default for Windows readers and makes the sample runnable on Linux/macOS/Docker without editing source.

Related, and a blocker for that path: BrighterSqlQueue.sql is UTF-16 with hardcoded FILENAME = N'C:\Program Files\Microsoft SQL Server\MSSQL12.SQLEXPRESS\MSSQL\DATA\…', so it cannot create the queue table on a containerised SQL Server. This PR now depends on that script as the only route to the queue table, which raises its importance. Dropping the explicit ON PRIMARY (NAME=…, FILENAME=…) clauses would let it run anywhere.

While there: this sample has no README.md, unlike its PostgresTaskQueue sibling. Setup is now genuinely two-step ("run the .sql for the queue, the app provisions the rest"), and that asymmetry is the single most surprising thing about it.

3. Table names now agree only by comment

Commit 2 split one config into two that deliberately differ (sender: outbox + queue; receiver: inbox + queue), each carrying a comment saying "the table names have to agree across the two files". CompetingReceiverConsole holds a third copy of the connection string and queueStoreTable.

The sample's whole lesson is one database, three roles. A shared static class Db in the Events project with ConnectionString / QueueTable / OutboxTable / InboxTable would make that agreement structural instead of aspirational — and it is the shape a reader should copy. As it stands, the sample teaches the coupling and then models the fragile way of managing it.

4. [UseInbox] on a handler in the shared Events project

Events is referenced by all four apps and every one of them calls AutoFromAssemblies() — including GreetingsSender (Program.cs:63) and CompetingSender (Program.cs:46). Only GreetingsReceiverConsole registers an inbox. So three processes now carry a registered handler whose pipeline cannot be constructed.

Latent today, because none of them Send/Publish a GreetingEvent in-process and pipelines are built lazily. But it will surface as a DI resolution failure for the first reader who adds one, and it would fail ValidatePipelines() if anyone enables it in the sender. Given the inbox policy belongs to the receiver, moving GreetingEventHandler into GreetingsReceiverConsole (or at minimum noting the constraint at the sender's AutoFromAssemblies()) would keep the coupling honest.

5. Minor

• Line-number citations rot, and have already rotted. GreetingEventHandler.cs:41 cites ServiceCollectionExtensions.cs:657-666; the actual branch is :655 and the inbox arguments are :665/:676. The filename is also ambiguous — ServiceCollectionExtensions.cs exists in both Paramore.Brighter.Extensions.DependencyInjection and Paramore.Brighter.ServiceActivator.Extensions.DependencyInjection, and the comment does not say which. Cite AddEventBus by name instead; it is stable and greppable.
• The PR body is stale after commit 2. It still says GreetingsSender "names all three tables on one RelationalDatabaseConfiguration" and provisions "the Outbox and Inbox through UseBoxProvisioning" — commit 2 split those and turned the sender into a Host. Since a docs guide will point at this, worth rewriting before merge.
• GreetingsSender/Program.cs:1 — using System.Threading.Tasks; is unused under top-level statements.
• GreetingsSender gets Microsoft.Extensions.Hosting only transitively, via Paramore.Brighter.BoxProvisioning. It compiles, but a sample that calls Host.CreateApplicationBuilder should say so with an explicit CPM-versioned <PackageReference Include="Microsoft.Extensions.Hosting" />; otherwise narrowing BoxProvisioning to Hosting.Abstractions breaks it for reasons nothing in the csproj explains.
• AddSingleton<ILoggerFactory>(new SerilogLoggerFactory()) now competes with the host's own logging registration. It works (last registration wins), but on a Host the idiomatic form is builder.Logging.ClearProviders().AddSerilog(), and the factory instance is never disposed. No Log.CloseAndFlush() either — harmless with the console sink, lossy with any other.
• GreetingsReceiverConsole's explicit Inbox.MsSql project reference is now redundant (BoxProvisioning.MsSql brings it). Arguably clearer as explicit; just flagging that it is no longer load-bearing.

Summary

The subscription-type fix is correct, well-evidenced, and stands on its own — I would take it as-is. The Outbox/Inbox demonstration is a real improvement to what the sample teaches, and commit 2's self-correction (provisioning that never ran; a global inbox config that was silently ignored) is exactly the right instinct.

What I would want before merge is (2): a sample that only runs on Windows SQLEXPRESS, and whose own verification required an uncommitted edit, is a sample most readers of the guide cannot follow. (1) and (3) are the changes that stop this recurring; both are fine as follow-ups.

🤖 Generated with Claude Code

[iancooper]

No blocker this round, but two of these were defects I introduced and both are fixed in 411b97e11.

4 — [UseInbox] in the shared Events project. You are right, and it was mine.

All four apps reference Events and all four call AutoFromAssemblies(); only the receiver registers an inbox. Three processes were registering a handler whose pipeline cannot be built. Latent for the reason you give, and real. GreetingEventHandler now lives in GreetingsReceiverConsole, which owns the policy — and the comment says why, so the next person does not move it back.

5 — the line-number citation had already rotted, in a comment about rotting citations

ServiceCollectionExtensions.cs:657-666 against an actual :655 / :665 / :676, and ambiguous between two files of that name. It now names AddEventBus. Taking the general point: citing a method is stable, citing a line is a hostage.

2 — the committed form was not the form that ran

Fair, and the sharpest note here. Both apps now read ConnectionStrings:Brighter with the SQLEXPRESS string as the fallback, so Windows readers are unaffected and everyone else can point it at a container. The packages were already referenced and unused, as you said.

Re-proven end to end on the committed source, with only an environment variable set:

Provisioned Outbox 'Outbox' successfully
Decoupled invocation of message: Topic:greeting.event Id:01a08b63-...
receiver: provisioned Inbox 'InboxMessages'
delivery 1, message id X : handler ran 1 time
delivery 2, message id X : handler ran 0 times
  warn: UseInboxHandler  Command 01a08b63-5db8-711a-905d-9582f4557f77 has already been seen
InboxMessages rows after both : 1

Also done: GreetingsSender now references Microsoft.Extensions.Hosting explicitly rather than leaning on BoxProvisioning's transitive one, and the stale using System.Threading.Tasks; is gone. The PR body is rewritten — you were right that it still described the pre-Host shape.

What I have not done, and why

• The validation spec comparing (subscription.ChannelFactory ?? options.DefaultChannelFactory).GetType() against Subscription.ChannelFactoryType, and the test pinning the downcast. Both are clearly the right fix — they would make the explanatory comments in these files unnecessary — and both are src/tests changes that belong on their own branch rather than bolted onto a samples PR.
• Shared Db constants. I take the point that the sample teaches a coupling and then models the fragile way of managing it. It is a bigger edit than the rest of this PR and touches CompetingReceiverConsole/CompetingSender too, so I would rather do it deliberately than late.
• BrighterSqlQueue.sql (UTF-16, hardcoded FILENAME) and a README.md. Both worth doing, and both now more visible because this PR leans on that script.
• InboxScope being dead. Agreed it deserves an issue rather than a commit-message footnote.

That is five follow-ups. I would rather not open issues on your repo uninvited — say which of them you want raised and I will, or tell me to raise the lot.

[claude]

Review

I traced each claim in the description back through src/, and the three diagnoses hold up:

• ChannelFactory does downcast to MsSqlSubscription and throw in all three creation methods (src/Paramore.Brighter.MessagingGateway.MsSql/ChannelFactory.cs:44-93), so both receivers really were unstartable. OnMissingChannel is genuinely ignored there too.
• The global inbox reaches the pipeline only via the two ExternalBus arms of AddEventBus (ServiceCollectionExtensions.cs:655-676); NoExternalBus() takes no inbox.
• The [UseInbox] route works because AddConsumers registers IAmAnInboxSync/IAmAnInboxAsync from options.InboxConfiguration.Inbox (ServiceActivator...DependencyInjection/ServiceCollectionExtensions.cs:45-58), and MsSqlInbox -> RelationalDatabaseInbox implements both. Keeping InboxConfiguration as just the store, with the attribute carrying policy, is right.

One property worth folding into the GreetingEventHandler comment, because it makes the belt-and-braces setup safe rather than merely redundant: PipelineBuilder skips the global inbox attribute for any handler already carrying [UseInbox] (HasExistingUseInboxAttributesInPipeline, PipelineBuilder.cs:389-394). So adding a producer here later will not double up the inbox handler.

One thing I would fix before merge, then smaller points.

1. CompetingReceiverConsole still cannot process a message

CompetingReceiverConsole/Program.cs:21 omits messagePumpType, so MsSqlSubscription<T> defaults to Proactor — but CompetingConsumerCommandHandler is a sync RequestHandler<T> and CompetingConsumerCommand : Command. The Proactor pump calls SendAsync, the async chain is empty, and AssertValidSendPipeline throws on every delivery. Brighter treats this as an Error in its own rules (ConsumerValidationRules.PumpHandlerMatch, src/Paramore.Brighter.ServiceActivator/Validation/ConsumerValidationRules.cs:79-83):

Subscription uses Proactor (async) pump but handler '...' is sync — use Reactor for sync handlers

"Behaviour-preserving where arguments were omitted" is true as stated, but the net effect is that this app now starts and then fails per message; the old Subscription<T> bug was masking it, and the Testing section only exercises the Greetings pair. Suggest matching GreetingsReceiverConsole/Program.cs:70 and adding messagePumpType: MessagePumpType.Reactor. Given the PR framing ("two receivers could never have started"), leaving one broken at dispatch is the same bug one layer down.

2. .ValidatePipelines() would have caught both bugs

It already ships (BrighterPipelineValidationExtensions.cs:59) and AddConsumers already registers the consumer specs, so one line on each receiver would surface #1 at startup — and it is the natural home for the subscription-vs-ChannelFactoryType spec you list as a follow-up (MsSqlSubscription.ChannelFactoryType is already typeof(ChannelFactory); only the rule is missing). Optional, and I know you are keeping scope tight.

3. The connection-string story is only half-portable

• GreetingsSender/Program.cs:31 says "appsettings or the environment", but neither project has an appsettings.json and a Microsoft.NET.Sdk console app does not copy one to the output directory by default — so only ConnectionStrings__Brighter (or the command line) actually works. Either narrow the comment or add the file plus a Content ... CopyToOutputDirectory item.
• CompetingSender/Program.cs:28 and CompetingReceiverConsole/Program.cs:29 still hardcode .\sqlexpress, so "runs against a container without editing source" holds for the Greetings pair only.
• The queue table still needs BrighterSqlQueue.sql with its Windows FILENAME clauses, so a container user still edits something. Not a blocker given the README follow-up — just don't want the comment to over-promise.

4. ?? does not catch an empty connection string

GreetingsSender/Program.cs:32, GreetingsReceiverConsole/Program.cs:49: GetConnectionString returns "" (not null) when the key exists but is blank — easy to produce from an env var. The fallback is skipped and MsSqlConnectionProvider then throws ArgumentNullException on ConnectionString, well away from the cause. string.IsNullOrWhiteSpace(...) reads the same and fails better.

5. Hidden ordering dependency in the receiver

Provisioning beats the dispatcher only because .UseBoxProvisioning(...) (line 86) registers its hosted service before AddHostedService<ServiceActivatorHostedService>() (line 89). Reverse them and the pump consumes against a missing InboxMessages. The comment explains that it runs; given how carefully everything else here is explained, it is worth saying it must run first. (Concurrent sender/receiver provisioning of one database is fine — Global history scope plus the migration lock covers it.)

6. Smaller points

• contextKey: nameof(GreetingEventHandler) (GreetingEventHandler.cs:48) — every other sample uses the Type overload (contextKey: typeof(GreetingMadeHandlerAsync) in WebAPI_Dynamo/_EFCore/_Dapper), which stores FullName and cannot collide across namespaces. Same one-token change, and it matches what the global config would have written.
• GreetingsSender/Program.cs was rewritten in full but has no #region Licence block; .agent_instructions/documentation.md:88-94 asks for one, and the sibling it is modelled on has it (PostgresTaskQueue/GreetingsSenderWithOutbox/Program.cs:1), as do the two other files you touched.
• databaseName: "BrighterSqlQueue" is threaded through both configs, but IAmARelationalDatabaseConfiguration.DatabaseName has no consumer on any relational path (only Mongo/Dynamo read it). Pre-existing and I would leave the value alone, but now that the connection string is overridable a reader may think the two must be kept in sync — they need not be.
• GreetingsSender: no Log.CloseAndFlush() and the host is not disposed; await using var host = builder.Build(); would make it a better thing to copy. Nothing is lost today with a synchronous console sink.
• GreetingsReceiverConsole.csproj references Serilog packages the receiver never configures. Pre-existing, and it matters slightly more now: with OnceOnlyAction.Warn the only signal of a duplicate is a log line. Default host console logging does emit it, so your captured output is right — but the unused references invite a reader to wire Serilog up and wonder why nothing changes.
• "they would each register a handler whose pipeline cannot be built" is a shade stronger than the mechanism warrants: pipelines build lazily per request type, so the other apps would only fail if they dispatched a GreetingEvent (or opted into ValidatePipelines()). The move is still the right call — the reason is latent, not certain, breakage.

On Post -> DepositPost + ClearOutbox

Checked the shapes: DepositPost returns Id, and ClearOutbox(Id[]) is genuinely synchronous through _mediator.ClearOutbox, so StopAsync cannot race the dispatch. MsSqlTransactionProvider's constructor does take IAmARelationalDatabaseConfiguration, so the singleton at line 44 is required exactly as the comment says — that comment will save someone an hour. Consistent with the PostgresTaskQueue/GreetingsSenderWithOutbox pattern.

Test coverage

None, which is defensible for a samples-only change, and you have named the two useful follow-ups. I would rank the subscription-vs-ChannelFactoryType spec highest: it turns this whole class of bug from "compiles, dies at startup" into a startup validation error, and the data it needs already exists on every gateway's subscription type. While in that file: the three rmqSubscription locals are assigned, null-checked, then never used (and misnamed rmq in the MSSQL gateway).

Net: #1 is the one I would want addressed before merge — the startup failure is fixed for CompetingReceiverConsole, but it is still broken at dispatch. Everything else is polish. The commentary in these files is unusually good, the fixes are correctly diagnosed, and the Outbox/Inbox composition is worth having in the samples.

Reviewed by reading source, not by building — dotnet build was unavailable in this sandbox, so treat compile-level claims as source analysis. Nothing above depends on a build.

[iancooper]

Point 1 was right and it was mine — and running it turned up a third one. Fixed in 1b32c94ee.

1. CompetingReceiverConsole — Proactor pump, sync handler

Verified before touching it: CompetingConsumerCommandHandler is a sync RequestHandler<T>, the subscription omits messagePumpType so MsSqlSubscription<T> defaults to Proactor, and ConsumerValidationRules.PumpHandlerMatch classes that as an Error in Brighter's own words. Your framing is the right one: fixing the subscription type moved the failure one layer down. Now Reactor, with the reason in a comment.

2. …and CompetingSender has never started either

You said the Testing section only exercised the Greetings pair. It did, so I ran the other one:

Unhandled exception. Paramore.Brighter.ConfigurationException:
  MS SQL Message Producer Factory: Topic is missing from the publication
   at MsSqlMessageProducerFactory.Create() … MsSqlMessageProducerFactory.cs:line 62
   at Program.<Main>$ … CompetingSender/Program.cs:line 33

It passes [new Publication()] — no Topic. It now publishes Publication<CompetingConsumerCommand> on the routing key the receiver subscribes to.

3, 4, 5, 6 — all taken

• IsNullOrWhiteSpace rather than ??, for exactly the reason you give.
• The comment said "appsettings or the environment"; there is no appsettings.json and a console app would not copy one, so it now says ConnectionStrings__Brighter. I have not claimed the Competing pair is portable — it still hardcodes SQLEXPRESS, and I would rather leave that honest than half-fix it.
• contextKey: typeof(GreetingEventHandler), matching the WebAPI samples.
• #region Licence restored on GreetingsSender/Program.cs — dropped when I rewrote the file, and .agent_instructions/documentation.md asks for it.
• The provisioning ordering is now stated as load-bearing, not merely explained.
• Your last point is fair: "pipeline cannot be built" was stronger than the mechanism warrants. The comment now says latent rather than certain. I have also folded in HasExistingUseInboxAttributesInPipeline — thank you, that is the property that makes keeping InboxConfiguration alongside the attribute safe rather than just redundant, and I did not know it.

What I am not claiming

The Competing pair builds and starts, and neither of its two proven defects remains — but it does not round-trip. The sender logs three Decoupled invocation lines with no error and nothing lands in QueueData; the receiver then reports "There were 0 commands handled by this consumer". That is pre-existing, I have not diagnosed it, and I do not think a samples PR should grow a third investigation. Flagging it rather than papering over it.

The Greetings pair is re-proven end to end on the committed source with only an environment variable: Outbox provisioned, Inbox provisioned, delivery 1 handled once, delivery 2 of the same id handled zero times with "has already been seen", Inbox holding 1 row.

On .ValidatePipelines() and the ChannelFactoryType spec

Both agreed, and your ranking is right — the spec turns this whole class from "compiles, dies at startup" into a named startup error, and MsSqlSubscription.ChannelFactoryType already carries the data. Still holding off on opening issues uninvited; that is now seven follow-ups I have offered on this PR and its sibling. Say the word and I will file them, or tell me to stop offering and just file.

[claude]

Review

I checked every mechanism claim in the description against src/ on this branch. They all hold:

Claim	Verified at
MSSQL ChannelFactory downcasts to MsSqlSubscription and throws	src/Paramore.Brighter.MessagingGateway.MsSql/ChannelFactory.cs:46,66,88; MsSqlSubscription<T> exists with matching timeOut/messagePumpType params (MsSqlSubscription.cs:117-134)
Global InboxConfiguration never reaches the pipeline without producers	Paramore.Brighter.Extensions.DependencyInjection/ServiceCollectionExtensions.cs:655 — if (!hasEventBus) … NoExternalBus(); the inbox is only passed on the two ExternalBus arms (:665, :676)
options.InboxConfiguration still supplies the store for [UseInbox]	ServiceActivator.Extensions.DependencyInjection/ServiceCollectionExtensions.cs:45-58 registers IAmAnInboxSync; MsSqlInbox → RelationalDatabaseInbox : IAmAnInboxSync, IAmAnInboxAsync
OnceOnlyAction.Warn logs and drops without calling Handle	Inbox/Handlers/UseInboxHandler.cs:115-118 (return request)
[UseInbox] suppresses the global inbox attribute	PipelineBuilder.cs:366 HasExistingUseInboxAttributesInPipeline()
Hosted-service ordering	UseBoxProvisioning appends the IHostedService descriptor before AddHostedService<ServiceActivatorHostedService>() at GreetingsReceiverConsole/Program.cs:95, and hosted services start in registration order — the comment is correct
ConsumerValidationRules.PumpHandlerMatch naming	ServiceActivator/Validation/ConsumerValidationRules.cs:46
A Publication needs a Topic; Publication<T> sets RequestType	Publication.cs:175-185

No other references to the deleted Events.Ports.CommandHandlers.GreetingEventHandler remain, and all five projects are in Brighter.slnx, so CI's dotnet build (.github/workflows/ci.yml:60) will compile-check the new project references. The measurement-with-a-control style in the description is genuinely useful evidence — thank you for that.

Below is what I would change or follow up on. Nothing here blocks the two fatal fixes, which are clearly right.

1. The sample still cannot be run from a clean machine — and a comment now claims otherwise

GreetingsReceiverConsole/Program.cs:88 says "Each process provisions the box it owns, so the receiver can be started on its own", and the description says the connection-string fallback means "the sample runs against a container without editing source". Both are true only after two undocumented manual steps: the BrighterSqlQueue database and the QueueData table must already exist. The only thing that creates them is BrighterSqlQueue.sql, which is UTF-16 with hardcoded Windows paths:

FILENAME = N'C:\Program Files\Microsoft SQL Server\MSSQL12.SQLEXPRESS\MSSQL\DATA\BrighterSqlQueue.mdf'

So against a Linux container you must edit source (or hand-write the DDL) before either process gets past its first query — the first symptom being Invalid object name 'QueueData' from inside the pump, which is exactly the class of failure this PR is fixing elsewhere.

MsSqlQueueBuilder.GetDDL / GetIndexDDL / GetExistsQuery are public statics with no production call site (MessagingGateway.MsSql/MsSqlQueueBuilder.cs:63-84). Having the receiver create the queue table at startup, guarded by GetExistsQuery, would make "either can be started alone" literally true and would give that otherwise-dead builder a call site. Failing that, please either ship the README or shorten the claim in the comment and state the prerequisite where a reader will hit it.

2. databaseName: is hardcoded next to a configurable connection string

GreetingsSender/Program.cs:65 and GreetingsReceiverConsole/Program.cs:58 pin databaseName: "BrighterSqlQueue" while the connection string is env-overridable. On the relational paths DatabaseName is read only for telemetry — BoxSpanInfo(dbSystem, DatabaseConfiguration.DatabaseName, …) in RelationalDatabaseInbox.cs:86 and RelationDatabaseOutbox.cs:86 — so overriding ConnectionStrings__Brighter to a differently-named database silently mislabels db.name on every outbox/inbox span. Either derive it (new SqlConnectionStringBuilder(connectionString).InitialCatalog) or drop the argument; a sample that shows a value which is inert-but-wrong is worse than one that omits it.

3. Prefer a shared constant over a comment asserting agreement

Both files now carry "the connection string and the table names have to agree across the two files" — the default connection string, "BrighterSqlQueue", "QueueData", "InboxMessages" and greeting.event are all duplicated. Events is already referenced by all four apps, so a public static class SampleDatabase { public const string QueueTable = "QueueData"; … } would turn a comment-time contract into a compile-time one, for less code than the comment explaining it. The description defers this; I would argue it is the cheaper option, not the more expensive one.

4. Comment density is inverted for a sample

GreetingEventHandler.cs carries ~20 lines of comment above a 6-line handler, and most of it documents framework internals (AddEventBus's arms, HasExistingUseInboxAttributesInPipeline, the ChannelFactory downcast, PumpHandlerMatch). I verified all of it is accurate today, which is the problem: samples are compiled but never asserted against those internals, so when (say) NoExternalBus starts forwarding the inbox, the comment becomes confidently wrong with nothing to catch it. CLAUDE.md asks for comment density matching the surrounding code, and the rest of samples/TaskQueue/* is much lighter. Suggest keeping the one-line "why" at each site (MsSqlSubscription, not Subscription — the MSSQL ChannelFactory downcasts) and moving the mechanism into the guide this PR accompanies, which can be revised when the mechanism changes.

5. Two framework defects are routed around, with no test pinning either

The description lists these as follow-ups; I would like them filed rather than left in prose, because both bite silently:

• Subscription/channel-factory mismatch. Subscription.ChannelFactoryType already exists (Subscription.cs:172) and CombinedChannelFactory already matches on it (CombinedChannelFactory.cs:34), but nothing validates it — I confirmed ConsumerValidationRules has no such spec. A spec there would convert a ConfigurationException thrown deep in dispatcher start into a startup validation error, and would have caught both of the receivers fixed here. That is the fix that stops the next sample (or user) making the same mistake.
• NoExternalBus() dropping InboxConfiguration. A consumer-only process is the normal shape for a receiver, and today it gets no de-duplication and no diagnostic. Even a warning when InboxConfiguration.Inbox is a non-default instance on the NoExternalBus path would have saved this investigation.

Per CLAUDE.md's TDD rule these belong in src/ behind /test-first. Worth noting that CI only compiles samples, which is precisely why "it compiles" was enough to hide the fatal subscription type for this long — startup validation is the only realistic net.

6. Smaller things

• Half the sample is still un-configurable. CompetingSender/Program.cs:29 and CompetingReceiverConsole/Program.cs:32 keep the hardcoded .\sqlexpress string, so only the Greetings pair can point at a container. Both files are already touched here; the same six-line fallback would keep the directory coherent.
• GreetingsSender/Program.cs:3 claims Copyright © 2026 on a pre-existing file; sibling files in this sample keep 2014.
• No Log.CloseAndFlush() in GreetingsSender, and if ClearOutbox throws (the missing-QueueData case above) host.StopAsync() is skipped as well. A try/finally would make the sample's own failure path exemplary rather than incidental.
• DepositPost comment. "In a real handler this call shares a transaction with your own write" sits above the overload that does not take a transaction provider (IAmACommandProcessor.cs:303 vs :320). Since the sample already registers MsSqlTransactionProvider, using the DepositPost<TRequest, TTransaction> overload would let the code demonstrate the sentence instead of asserting it.

Test coverage

Samples are not testable in this repo, so no tests are expected in this PR — but that is exactly the gap that let this ship broken. The items in §5 are where the tests should land.

[iancooper]

Your first point was the important one and it was right. That is the third time in this PR a comment of mine has promised more than the code delivered — first the Docker proof that used an uncommitted edit, then "runs against a container without editing source", now "the receiver can be started on its own". Fixed in 1610d5862.

The sample now runs from a clean machine, and I proved it that way

I took your suggestion rather than shortening the claim: the queue table is provisioned by the sample, guarded by GetExistsQuery, in Events/QueueTableProvisioner.cs. It makes "either can be started alone" literally true, and it gives MsSqlQueueBuilder.GetDDL / GetIndexDDL / GetExistsQuery their only production call site in the repository — they are public precisely because callers must run them, and until now nobody did.

BrighterSqlQueue.sql is now 23 lines of portable UTF-8 creating only the database, because a database cannot create itself. The 2018 SQLEXPRESS dump with FILENAME = N'C:\Program Files\...\MSSQL12.SQLEXPRESS\...' is gone.

Fresh SQL Server 2022 container, the .sql run once, no manual table creation, committed source, one environment variable:

receiver started FIRST, on a database with zero tables:
  Created queue table 'QueueData'
  Provisioned Inbox 'InboxMessages' successfully
sender:
  Provisioned Outbox 'Outbox' successfully / Decoupled invocation
delivery 1, message id X : handler ran 1 time
delivery 2, message id X : handler ran 0 times
  warn: UseInboxHandler  Command 01a08bdb-... has already been seen
InboxMessages rows: 1

Starting the receiver first is the case that could not work before.

2, 3, 4, 6 — all taken

• databaseName: dropped. Your telemetry argument settles it: pinning a value that only reaches BoxSpanInfo beside an overridable connection string silently mislabels db.name on every span. A value that is inert-but-wrong is worse than an absent one.
• Shared constants. You were right that this is the cheaper option, not the more expensive one — Events/SampleDatabase.cs is shorter than the comments it replaced, and a mismatch was silent rather than loud.
• Comment density. Cut from ~20 lines to 6. Your reason is the one that convinced me: the mechanism is accurate today and asserted nowhere, so it belongs in the guide, which can be revised when the mechanism changes.
• The Competing pair is configurable too, so the directory is coherent; licence year back to 2014; try/finally with Log.CloseAndFlushAsync; and DepositPost now takes the transaction provider, so the code demonstrates the sentence above it instead of asserting it.

What I am still not claiming

The Competing pair does not round-trip. Both apps start, are configurable and provision what they need, but CompetingSender logs three Decoupled invocation lines with no error and nothing reaches QueueData — under any topic — while the Greetings pair round-trips against the same table in the same session. Filed as #4338 with what is ruled out, rather than guessed at here.

The five follow-ups are filed

The maintainer asked for them rather than more offers:

• Validate a subscription against its channel factory's ChannelFactoryType at startup #4334 — validation spec comparing a subscription against its channel factory's ChannelFactoryType. Your ranking was right; this is the one that stops the next occurrence.
• NoExternalBus() silently discards InboxConfiguration, so a consumer-only process gets no Inbox #4335 — NoExternalBus() discarding InboxConfiguration.
• InboxScope is referenced nowhere outside its own declaration — the scope: argument does nothing #4336 — InboxScope inert.
• CommandProcessor.Call: "No Subscription registered fpr replies of type" — typo, and it names the wrong type #4337 — CommandProcessor.Call's "fpr" typo, which also prints typeof(T) where the match is on TResponse.
• samples: MsSqlMessagingGateway CompetingSender/Receiver do not round-trip #4338 — the Competing pair above.
• tests: nothing asserts that a gateway ChannelFactory rejects a non-matching Subscription #4339 — a test pinning the ChannelFactory downcast.
• samples: MsSqlMessagingGateway has no README, unlike its PostgresTaskQueue sibling #4340 — a README for this sample.

Thank you for pushing on the runnability point rather than accepting a narrowed comment. It turned out to be the difference between a sample that documents a two-step setup and one that just works.

[claude]

Review — part 1 of 3: what I verified

I checked the three claims in the description against the source rather than taking them on trust, and they hold up:

• ChannelFactory (src/Paramore.Brighter.MessagingGateway.MsSql/ChannelFactory.cs:46,66,88) does subscription as MsSqlSubscription in all three creation methods and throws on null. Subscription<T> could never have produced a channel — a genuine "compiles, cannot run" bug in both receivers.
• AddEventBus (.../Extensions.DependencyInjection/ServiceCollectionExtensions.cs:655) takes the NoExternalBus() arm when no IAmAnOutboxProducerMediator is registered, and only the two ExternalBus(...) arms receive serviceActivatorOptions?.InboxConfiguration. A consumer-only process really does get no global inbox.
• UseBoxProvisioning registers BoxProvisioningHostedService via TryAddEnumerable (BrighterBuilderBoxProvisioningExtensions.cs:92), so it genuinely needs a Host, and it genuinely lands before the later AddHostedService<ServiceActivatorHostedService>(). The load-bearing-ordering comment is accurate.

Worth stating in the sample, because it looks like a hazard and isn't: setting options.InboxConfiguration and [UseInbox] on the handler cannot double-wrap — PipelineBuilder.AddGlobalInboxAttributes bails on HasExistingUseInboxAttributesInPipeline() (PipelineBuilder.cs:394), so the attribute wins. And InboxConfiguration is still required, because that is what puts IAmAnInboxSync in the container (ServiceActivator...ServiceCollectionExtensions.cs:47-52) for UseInboxHandler<T> to resolve.

Also spot-checked and fine: MsSqlBoxDetectionHelper/MsSqlPayloadModeValidator have no ctor deps, so the receiver not registering IAmARelationalDatabaseConfiguration is OK; AddProducers defaults to ServiceLifetime.Transient, so resolving IAmATransactionConnectionProvider from the root provider will not trip scope validation; ClearOutbox dispatches synchronously, so the send has completed before StopAsync.

[claude]

Part 2 of 3: findings

1. The default connection string will fail on the path the sample documents

SampleDatabase.DefaultConnectionString is Database=BrighterSqlQueue;Server=.\sqlexpress;Integrated Security=SSPI;. Microsoft.Data.SqlClient is pinned at 7.0.2 (Directory.Packages.props:84), and since 4.0 Encrypt defaults to true — against a default SQL Express instance with a self-signed certificate that fails validation at login.

Every other MSSQL connection string in the repo carries the opt-out:

tests/Paramore.Brighter.MSSQL.Tests/Configuration.cs:9     ...;Encrypt=false
samples/WebAPI/WebAPI_Dapper/*/appsettings.*.json          ...;TrustServerCertificate=true;Encrypt=false

You tested via ConnectionStrings__Brighter against a container — which is exactly the path that bypasses this constant. The fallback, the one a reader hits first with no env var set, is the one that will not connect. I take the CLAUDE.md point about not changing defaults unasked, but this default was previously unreachable (the receiver threw first), so this PR is the first time it is on the happy path. Suggest ...;Integrated Security=SSPI;TrustServerCertificate=True; plus a line in the XML doc saying why.

2. EnsureQueueTable is check-then-act, and races in exactly the scenario it was added for

if (Convert.ToInt32(exists.ExecuteScalar()) == 1) return;
...
create.ExecuteNonQuery();

All four apps now run this at startup against one shared database. "Each process provisions the box it owns, so either can be started alone" is the goal — but concurrent start on a fresh database is the case this breaks: both read 0, both CREATE TABLE, and the loser dies with SqlException 2714 (There is already an object named 'QueueData'). Box Provisioning is safe here because it takes MsSqlAdvisoryLock; the queue provisioner has no equivalent.

Cheapest fix is one statement instead of two round-trips (IF NOT EXISTS (...) BEGIN <GetDDL>; <GetIndexDDL>; END), or catch 2714 and return. Either beats a flake that reads to a newcomer as "the sample is broken".

3. EnsureQueueTable does not converge, and its check and its DDL disagree about schema

• The index is only created on the branch where the table was just created. If CREATE INDEX failed, or the table pre-exists from the old BrighterSqlQueue.sql / DDL Scripts/QueueStore.sql (whose index is IX_Topic, not IX_QueueData_Topic), the index is never created and never repaired. Ensure* implies convergence; this is create-once.
• MsSqlQueueBuilder.GetExistsQuery(name) defaults schemaName to dbo, but GetDDL emits CREATE TABLE [QueueData] unqualified, which lands in the caller's default schema. For a login whose default schema is not dbo the check never matches, so every start re-attempts the create. Worth a comment at minimum, or thread SchemaName through for symmetry with the box tables.

4. The description is stale about BrighterSqlQueue.sql

"Not in this PR" lists "the UTF-16 BrighterSqlQueue.sql with hardcoded Windows FILENAME clauses" as a follow-up — but the diff rewrites that file (UTF-8, CREATE DATABASE only, no FILENAME). Please fix the description; it is the record for this change.

More substantively, the script no longer creates QueueData, so the setup contract changed: script -> database, apps -> tables. Better split, but the header comment in the .sql is currently the only place it is written down. Of the deferred items, the README.md is the one I would pull into this PR.

5. SampleDatabase.Name is unused, and the name it holds is still duplicated three times

The stated rationale is "constants rather than a comment asking two files to agree, because a mismatch here is silent" — but Name has no callers, while BrighterSqlQueue still appears literally in DefaultConnectionString and twice in BrighterSqlQueue.sql. That is the one value the class fails to centralise. Either build the default connection string from Name, or drop it.

[claude]

Part 3 of 3: findings, nits, verdict

6. Events.csproj picks up Microsoft.Data.SqlClient transitively

QueueTableProvisioner uses it directly, but Events.csproj declares no PackageReference — it arrives via the new MessagingGateway.MsSql -> Paramore.Brighter.MsSql chain. Compiles, but the repo pins it centrally and every project that uses it declares it. Add <PackageReference Include="Microsoft.Data.SqlClient" />.

7. Events is the contracts project; this puts infrastructure in it

Events was ports — commands, mappers, a counter interface. It now carries a database provisioner and a dependency on the MSSQL gateway, which both Competing* apps now inherit through the shared contracts assembly. Per .agent_instructions/design_principles.md that is a responsibility mismatch: SampleDatabase (names/config, genuinely shared) and QueueTableProvisioner (infrastructure) are two different jobs in one place. Not blocking for a sample.

8. The PumpHandlerMatch comment describes a check that is not running

... an Error under Brighter's own ConsumerValidationRules.PumpHandlerMatch

ConsumerValidationRules only runs behind ValidatePipelines(), which is opt-in and is called by none of these four apps. The Reactor choice is right — both handlers are sync RequestHandler<T> — but with Proactor a reader gets a runtime pump failure, not that validation Error.

Better resolution: add .ValidatePipelines() to both receivers. One line, it exercises the rule the comment cites, and it is the shape of the deferred "validation spec comparing a subscription against its channel factory's ChannelFactoryType" — MsSqlSubscription.ChannelFactoryType already returns typeof(ChannelFactory), so the data exists today.

9. No tests — and the cheapest one is the one being deferred

CLAUDE.md makes /test-first mandatory for behaviour changes. Samples are not production code and CI's root dotnet build does cover compilation (these projects are in Brighter.slnx:88-92) — but a build is exactly what did not catch this bug. The deferred "test pinning the ChannelFactory downcast" needs no container and no infrastructure:

When_a_subscription_is_not_an_mssql_subscription -> CreateSyncChannel throws ConfigurationException

That belongs in this PR. Seconds to run, pins the behaviour the fix depends on, and the same test shape applies to every other gateway that downcasts its subscription.

10. Framework issue worth raising alongside

The NoExternalBus gap is the real defect the sample works around: a consumer-only process silently loses de-duplication from a configured InboxConfiguration, with no warning. NoExternalBus() taking no inbox looks like an oversight rather than a decision — a consumer with no producers is the normal receiver shape. Worth its own issue next to the InboxScope one you already flagged.

Nits

• GreetingsSender/Program.cs:95 — await using var host = builder.Build(); would dispose the transient MsSqlTransactionProvider and its open SqlConnection. Harmless in a process that exits, but this is teaching material.
• Moving GreetingEventHandler into GreetingsReceiverConsole changes typeof(...).FullName, so the inbox ContextKey changes from Events.Ports.CommandHandlers.GreetingEventHandler to GreetingsReceiverConsole.GreetingEventHandler. Irrelevant on a fresh table; rows from an earlier run stop matching. Just noting it is a data-visible move.
• CompetingSender/Program.cs:2-3 — using System.Threading; looks unused (pre-existing).

---

Overall: the subscription fix is correct, well-evidenced, and stands on its own; the Outbox/Inbox composition is wired correctly, and the commentary is unusually good — it explains why at each decision rather than restating the code. My main concerns are #1 (the documented default cannot connect) and #2 (the provisioning race defeats the "start either first" property this PR is built around); #9 is the one I would push on given the TDD guardrail in CLAUDE.md. The rest is polish.

[iancooper]

Both headline findings were real. Fixed in 22394a6e3.

1. The documented default could not connect

Right, and the diagnosis of why I missed it is the sharper half: I tested through ConnectionStrings__Brighter against a container, which is exactly the path that bypasses the constant. The fallback — what a reader hits first, with no environment variable — was the one path never exercised. And your point that it was previously unreachable because the receiver threw first is what makes this PR the first time it matters.

TrustServerCertificate=True added, with the reason in the XML doc: Microsoft.Data.SqlClient defaults Encrypt to true from 4.0, this repo pins 7.0.2, and SQL Express presents a self-signed certificate.

2. The race, in exactly the scenario the provisioner was added for

Also right, and it undercut the property this PR is built on — "either can be started alone" invites starting both. Now one batch with IF NOT EXISTS guards plus a catch for 2714/1913, so a genuine race resolves as success rather than as a flake that reads like a broken sample.

Measured, two receivers started concurrently against a database with zero tables:

instance A: 0 race errors
instance B: 0 race errors
tables    : __BrighterMigrationHistory, InboxMessages, QueueData
IX_QueueData_Topic present exactly once

3. Convergence, and the schema disagreement

Taken in full, since I was rewriting the method anyway. The index is guarded independently, so a table that exists without its index gets one — including a table left by the old script, whose index had a different name. Both guards test SCHEMA_NAME() rather than a literal dbo, because GetDDL emits an unqualified CREATE TABLE: a login whose default schema is not dbo would never match its own table and would re-attempt the create on every start.

index dropped by hand, then restart:
  before: 0    after: 1    errors: 0

4, 5, 6, 8 and the nits

• The description is rewritten — you were right that it was the record and that it still listed a file this diff rewrites.
• The README is in this PR, not deferred. It documents the changed setup contract in the place a reader will look: script → database, applications → tables.
• SampleDatabase.Name now has a caller — the default connection string is built from it, so the one value the class failed to centralise no longer is.
• Events declares Microsoft.Data.SqlClient rather than inheriting it.
• Both receivers call .ValidatePipelines(). This was the best catch of the three smaller ones: my comment cited ConsumerValidationRules.PumpHandlerMatch, and that rule only runs behind an opt-in call none of these apps made — a comment describing a check that was not running. Now it runs, and the round trip reports 0 validation errors.
• Host disposed; unused using gone.

9 — deferred, by the maintainer's call

Adding tests to samples is a policy change wider than a PR whose job is to support a documentation page, so it stays filed as #4339. I take the CLAUDE.md point and I do not think you are wrong about the cost; it is just not this PR's decision to make.

7 — not fixed

Events now carries infrastructure as well as contracts. You marked it non-blocking and I have left it, because all four applications need the provisioner and splitting it means a fifth project for a sample. Recorded here rather than silently ignored.

10 — already filed

NoExternalBus dropping InboxConfiguration is #4335, alongside InboxScope as #4336. Seven issues in total came out of this PR and its sibling.

Thank you for splitting the review into parts — the two that mattered were unambiguous, and finding #1 by asking which path did the testing not take is the kind of thing I would not have got to on my own.

[claude]

Review

I read the diff against the source rather than taking the description on trust, and the three headline claims all hold up:

• The downcast is real. ChannelFactory.CreateSyncChannel/CreateAsyncChannel/CreateAsyncChannelAsync (src/Paramore.Brighter.MessagingGateway.MsSql/ChannelFactory.cs:46, :66, :88) each do subscription as MsSqlSubscription and throw. A plain Subscription<T> could never have produced a channel. Good catch, and the fix is right.
• The InboxConfiguration claim is real. AddEventBus only forwards serviceActivatorOptions?.InboxConfiguration on the two ExternalBus arms (ServiceCollectionExtensions.cs:665, :676); NoExternalBus() at :656 takes no inbox. A consumer-only process genuinely gets nothing.
• And the [UseInbox] route still resolves its store, because AddConsumers registers IAmAnInboxSync from options.InboxConfiguration.Inbox independently of the bus (ServiceActivator.Extensions.DependencyInjection/ServiceCollectionExtensions.cs:45-52). So keeping the InboxConfiguration registration while moving the policy onto the attribute is the correct shape, not a workaround.

Two other things I checked because the comments assert them:

• UseBoxProvisioning does register an IHostedService (BrighterBuilderBoxProvisioningExtensions.cs:92), so the "registration order is load-bearing" comment in GreetingsReceiverConsole/Program.cs:79 is accurate.
• AutoFromAssemblies scans AppDomain.CurrentDomain.GetAssemblies() (ServiceCollectionBrighterBuilder.cs:118), so moving GreetingEventHandler into the receiver assembly still registers it — and it stops CompetingReceiverConsole from picking up a handler that needs an inbox it doesn't configure. That relocation is doing more work than the diff makes obvious; worth keeping.

The comments throughout explain why rather than what, which is exactly what a sample needs. Below is what I'd change.

---

1. Test coverage — the one item I'd push back on deferring

CLAUDE.md makes TDD mandatory, and this PR fixes a defect whose entire character is "compiles fine, fatal at runtime, invisible to review." Nothing in the PR stops it recurring — in this sample or in anyone's application.

The pieces to catch it already exist:

• Subscription.ChannelFactoryType is already there (Subscription.cs:172), and MsSqlSubscription already overrides it to typeof(ChannelFactory) (MsSqlSubscription.cs:32). Today the only consumer is CombinedChannelFactory. A consumer validation spec comparing subscription.ChannelFactoryType against the resolved channel factory would turn this whole class of bug into a named startup error for every transport at once.
• tests/Paramore.Brighter.MSSQL.Tests/MessagingGateway/ already holds channel-factory tests that need no database (When_mssql_channel_factory_forwards_scheduler_to_consumers.cs). A test asserting CreateSyncChannel(new Subscription<T>(...)) throws ConfigurationException is a handful of lines and needs no container.

Both are on the "Not in this PR" list. The spec is fair to defer — it is a design change with its own review surface. The test pinning the downcast is not, and it is the one that makes the fix durable.

Separately, SampleDatabase.ConnectionString (Events/SampleDatabase.cs:67) encodes a deliberate decision — whitespace rather than null, because a blank environment variable defeats ??. That reasoning lives in the XML doc and nowhere else. It is a pure function; a two-case test would pin it.

2. QueueTableProvisioner: parameterize the lookups

Events/QueueTableProvisioner.cs interpolates queueTableName straight into T-SQL, including into string literals in the two EXISTS guards. Here the value is a compile-time constant, so there is no live injection — but samples are copy-paste sources, and a reader who wires the table name to configuration inherits an injection point in code that runs with DDL rights.

The two catalog lookups can be parameterized outright; only the DDL identifier has to stay interpolated:

WHERE t.name = @queueTable AND s.name = SCHEMA_NAME()
...
command.Parameters.AddWithValue("@queueTable", queueTableName);

This is what MsSqlBoxMigrationRunner already does for the same kind of check (AddWithValue("@SchemaName", schema) at :244), so it also makes the sample match the framework's own habits.

3. QueueTableProvisioner: the index name is duplicated, and the duplication fails silently

The guard hardcodes 'IX_{queueTableName}_Topic' while the statement it guards gets the real name from MsSqlQueueBuilder.GetIndexDDL. If the builder's naming convention ever changes, the guard stops matching, every start attempts a CREATE INDEX that fails with 1913, and the catch swallows it — so a permanently-failing statement looks exactly like success. MsSqlQueueBuilder does not expose the index name today, so the least you can do is a comment at the guard pointing at QUEUE_TABLE_INDEX_DDL as the source of truth; better would be a GetIndexName(queueTableName) on the builder.

The race handling itself is sound — I walked the interleavings, and the swallowed 2714 leaves the winner having created both objects.

4. Layering: Events now carries infrastructure

Events is the shared ports assembly (Commands, Mappers, the competing-consumer handler) and it now depends on Paramore.Brighter.MessagingGateway.MsSql and Microsoft.Data.SqlClient so it can host QueueTableProvisioner. It is expedient, and I understand why — all four apps need the provisioner — but it puts a database adapter inside the project the repo's Responsibility-Driven Design guidance would keep free of one, and it is the project every app hands to AutoFromAssemblies. A small SampleInfrastructure project would keep the layering the sample is otherwise teaching.

5. TrustServerCertificate=True deserves a line in the README

The remarks in SampleDatabase.cs explain why it is needed against SQL Express, which is the right call for a sample that has to run. But the README's container example uses Encrypt=false, and neither place tells the reader not to carry either setting into production. One sentence under "Pointing it at something other than SQL Express" would close it.

6. The PR description is out of step with the diff

Two places, and both matter because this description is the record of why:

• "Not in this PR ... the UTF-16 BrighterSqlQueue.sql with hardcoded Windows FILENAME clauses." The PR does rewrite it — to ASCII, with the FILENAME clauses gone and the table DDL removed in favour of the provisioners. That is a good change; it just is not a follow-up.
• "behaviour-preserving where arguments were omitted — both types default messagePumpType to Proactor." True of the types, but CompetingReceiverConsole/Program.cs:30 now explicitly sets Reactor. That is the correct fix (CompetingConsumerCommandHandler is a sync RequestHandler<T>) and it is academic since the receiver never started — but "behaviour-preserving" reads past it.

Also worth a sentence given CLAUDE.md's change-scope rule: the default connection string gains TrustServerCertificate=True, and databaseName: is dropped from every RelationalDatabaseConfiguration. I checked the latter — DatabaseName is unreferenced across Paramore.Brighter.MessagingGateway.MsSql and Paramore.Brighter.MsSql, so it is inert. Fine to drop; just not implied by the stated scope.

7. Nits

• using System; is unused in both new files (QueueTableProvisioner.cs, SampleDatabase.cs).
• SampleDatabase.ConnectionString(string? configured) uses a nullable annotation, but samples/Directory.Build.props does not import the root one, so samples do not get <Nullable>enable</Nullable> — that is a CS8632 warning. Adding <Nullable>enable</Nullable> to Events.csproj fixes it.
• The four-line "resolve connection string, then EnsureQueueTable" prologue is repeated in all four Program.cs files. Explicitness is defensible in a sample; just flagging that SampleDatabase could own it if you would rather.

On shipping CompetingSender knowingly broken

Documenting it in the README table with a link to #4338 is the honest thing to do, and the subscription fix stands on its own. No objection — just noting that a reader who runs the competing pair first will conclude the sample is broken before they reach the table. A one-line note at the top of "The applications" pointing at the greetings pair as the working path would help.

---

Net: the diagnosis work behind this is strong, the fixes are correct, and the comments will save the next reader real time. The test pinning the ChannelFactory downcast is the piece I would want before merge; the rest is polish.

🤖 Generated with Claude Code

[iancooper]

All of it taken bar one piece, in 6b449d1be. The item I pushed back on deferring last round was the one you were right about, and for a reason worse than disagreement: my refusal rested on a misreading of my own PR.

1. The test — and the deferral was wrong on its facts

I wrote "adding tests to samples is a policy change". The test is not in samples. You said where it goes — tests/Paramore.Brighter.MSSQL.Tests/MessagingGateway/, beside When_mssql_channel_factory_forwards_scheduler_to_consumers, which needs no database — and I answered as though it were going in the sample folder. There was no policy question to defer; there was a test to write.

When_a_subscription_is_not_an_mssql_subscription.cs, four facts, no container:

Should_throw_when_creating_a_sync_channel                      Passed
Should_throw_when_creating_an_async_channel                    Passed
Should_throw_when_creating_an_async_channel_asynchronously     Passed
Should_create_a_channel_for_an_mssql_subscription              Passed   <- the control

The control is there because without it the three would also pass against a factory that threw for everything. And since a test written after the fix proves nothing until it has been seen to fail, I neutered all three if (rmqSubscription == null) guards in ChannelFactory and re-ran:

Failed!  - Failed: 3, Passed: 1, Total: 4

Three red, control green, src/ reverted. #4339 is closed by this PR. #4334 — the validation spec comparing subscription.ChannelFactoryType against the resolved factory — stays deferred, because that one really is a design change with its own review surface, and it is the fix that would retire this test for every transport at once.

What I did not do: the SampleDatabase.ConnectionString two-case test. You are right that the whitespace-not-null decision lives only in an XML doc, and it is a pure function begging to be pinned. But no test project in the repository references samples/, and making the framework suite the first one inverts the dependency to pin two lines. If you would rather have it than have that boundary, say so and it is five minutes.

2. Parameterized

Both catalog lookups now bind @queueTable and @indexName; only the DDL interpolates, because an object name cannot be a parameter and that DDL is Brighter's own. The comment says why a constant still gets parameters — the reader copying this file is the one at risk, not this file. MsSqlBoxMigrationRunner:244 cited as the precedent.

3. The index name

Now a named IndexName(queueTableName) method whose remarks state the duplication outright: it repeats QUEUE_TABLE_INDEX_DDL's IX_{0}_Topic, MsSqlQueueBuilder exposes no GetIndexName to ask instead, and if that convention moves the guard stops matching, 1913 is caught forever and a permanently failing statement looks exactly like success. One place to change, and the dependency named rather than buried in a string.

4. Layering — split, and you asked twice

SampleInfrastructure now holds QueueTableProvisioner and SampleDatabase. Events.csproj is byte-identical to master again — no MessagingGateway.MsSql reference, no Microsoft.Data.SqlClient — so the assembly every application hands to AutoFromAssemblies is back to commands, mappers and the handler. You raised this at review 5 §7 and again here; I had recorded a refusal rather than acting, and a fifth project turned out to be four ProjectReference lines and one entry in Brighter.slnx. That solution-file edit is outside samples/, and so is the test; flagging both rather than letting them pass as sample changes.

5, 6, 7

• README: a paragraph saying neither Encrypt=false nor TrustServerCertificate=True belongs in production, and what each one actually switches off — the first drops TLS, the second keeps it and accepts any certificate. Plus a line at the top of The applications sending readers to the greetings pair first, which is your point about someone running the competing pair and concluding the sample is broken.
• Description: re-checked as you asked. The "behaviour-preserving where arguments were omitted" phrasing and the stale "Not in this PR" entry both went in the 16:14 rewrite; what was left was the provisioner's old path, and the new project and the test are now in it.
• Nits: both unused using System; gone. <Nullable>enable</Nullable> on the new project, and your CS8632 diagnosis was exactly right — proven both ways, since a claim about a warning is worth as little as any other unrun claim:

Events (no <Nullable>), string? probe : warning CS8632
SampleInfrastructure with <Nullable>  : 0 warnings

• The repeated four-line prologue I have left explicit, and that is a choice rather than an oversight: the two steps are the setup contract the README describes, and a reader who cannot see the provisioner run in Program.cs has to be told it happens. Happy to fold it into SampleInfrastructure if you would rather have it once.

Re-run after the move

The move touched every application's composition root and the provisioner's SQL, so the measurements were taken again rather than carried over. Fresh SQL Server 2022 container, BrighterSqlQueue.sql once, committed source, one environment variable:

two receivers started CONCURRENTLY on a database with zero tables:
  0 errors either side
  tables: __BrighterMigrationHistory, InboxMessages, QueueData
  IX_QueueData_Topic present exactly once
convergence: index dropped by hand, recreated on next start, 0 errors
round trip: same message id queued twice
  "Received Greeting" 1 time, "has already been seen" 1 time, queue drained

---

Six rounds, six real findings, and this one caught me refusing a test on grounds that did not survive reading my own diff. Thank you.

🤖 Generated with Claude Code

[claude]

Review

Thanks — this is a genuinely useful clean-up. The core fix is right and well evidenced: ChannelFactory does downcast to MsSqlSubscription in all three creation methods, so Subscription<T> could never have worked, and the new test pins it with a positive control, which is the right shape. I also checked the riskiest silent change — dropping databaseName: from RelationalDatabaseConfiguration — and it is safe: nothing in the MSSQL gateway, outbox, inbox or box-provisioning reads RelationalDatabaseConfiguration.DatabaseName (only MySqlBoxMigrationRunner does). The hosted-service ordering claim in GreetingsReceiverConsole checks out, and .ValidatePipelines() is correctly placed last in both chains, as its own XML doc requires.

Findings below, most useful first.

1. The "known limitation" (#4338) looks explainable from the diff — Post is enlisting in a TransactionScope that is never completed

CompetingSender/Program.cs posts inside a TransactionScope(TransactionScopeOption.RequiresNew, ...) that is deliberately never completed, on the premise that "a message is always queued, whether the transaction commits or aborts".

The send path is Post -> producer -> MsSqlMessageQueue.Send -> _connectionProvider.GetConnection() -> MsSqlConnectionProvider.cs:34, which is new SqlConnection(connectionString) followed by Open(). Enlist defaults to true and the connection string does not override it, so that INSERT enlists in the ambient transaction. The scope is then disposed without Complete(), so every insert is rolled back. Nothing in MessagingGateway.MsSql or Paramore.Brighter.MsSql suppresses the ambient transaction — I grepped.

That predicts exactly the reported symptom: three Decoupled invocation lines, no error, and nothing in QueueData. It also explains why the Greetings pair round-trips against the same table in the same session — it has no ambient transaction. Single-connection enlistment is LTM, so no MSDTC and no PlatformNotSupportedException on Linux; the sequential connections reuse the same pooled physical connection inside the transaction, so there is no promotion either.

The comment's premise is wrong for this transport specifically: with a database as the broker, the send is not decoupled from your transaction. That is the strongest possible motivation for the Outbox, and the sample could say so. Options, roughly in order of teaching value:

• Keep the scope and make the point explicit: show the sends vanishing, then show DepositPost/ClearOutbox surviving the same rollback.
• Add Enlist=False to the sender's connection string, or wrap the loop in TransactionScopeOption.Suppress, if you want the original "always queued" claim to actually hold.
• Call scope.Complete().

Worth confirming before merge, because the README and PR body currently teach this as unexplained, and #4338 may close on a one-line change. Good candidate for /bugfix:triage.

2. Moving GreetingEventHandler out of Events appears unnecessary, and contradicts the README

UseInboxAttribute (src/Paramore.Brighter/Inbox/Attributes/UseInboxAttribute.cs) and OnceOnlyAction (src/Paramore.Brighter/Inbox/OnceOnlyAction.cs) both live in the core Paramore.Brighter assembly, which Events.csproj already references. So [UseInbox] could have been added in place with no new dependency — the stated rationale for the split ("Events ... with no database dependency") does not apply to the attribute.

Three consequences:

• The README says "Events stays what its name says: the commands, their mappers and the handler" — now false for GreetingEventHandler.
• It is asymmetric: CompetingConsumerCommandHandler stayed in Events/Ports/CommandHandlers/, so the two handlers in one sample now live in different places for no visible reason.
• Every other sample keeps handlers in the shared project under Ports/CommandHandlers/ (RMQTaskQueue, KafkaTaskQueue, MultiBus, RMQDeferOnError), so a reader moving between samples hits a one-off layout.

It still works — AutoFromAssemblies scans AppDomain.CurrentDomain.GetAssemblies(), so the entry assembly is included — but absent a dependency reason, moving it back keeps the sample conventional and the README accurate. If there was a reason (a ValidatePipelines finding, say), it belongs in the README, because it is surprising.

3. QueueTableProvisioner — two notes on the index guard

• The batch aborts on a swallowed error. Table DDL and index DDL are one DbCommand. If the table create loses the race and throws 2714, the batch is abandoned and the index statement in the same batch never runs. Benign in the usual interleaving (the winner creates both), but "safe to run concurrently" then holds only because the winner is assumed to finish. Two separate DbCommands, each with its own guard and catch, makes that unconditional — and lets you drop the IndexName duplication entirely: attempt CREATE INDEX unconditionally and treat 1913 as success. That removes the silent-failure mode your own IndexName remark is rightly worried about.
• The table name reaches DDL unvalidated. The remark says "there is nothing to inject ... but a sample is a copy-paste source" — and then the copy-paste hazard is left in place: MsSqlQueueBuilder.GetDDL string-formats the name into CREATE TABLE [{0}], where a ] closes the bracket. Since you have reasoned this far, a two-line guard (reject anything not matching ^[A-Za-z_][A-Za-z0-9_]*$ before use) would make the sample safe to copy, not merely safe as written.

4. Comment volume and style vs .agent_instructions/code_style.md

The style guide is explicit: keep comments concise ("one tight line over a paragraph"); explain why, not what; and "inline comments are not notes to a reviewer ... context about the fix, the alternatives considered, the previous behaviour, or the history of a change ... belongs in the ADR, not in an inline comment."

Samples legitimately carry more explanation than library code, and much of this commentary is good — but some is reviewer-facing history, or belongs in the README you have already written:

• BrighterSqlQueue.sql: "The previous version of this script was a 2018 SQLEXPRESS dump: UTF-16, and with the data files pinned to ...". That is a changelog entry.
• GreetingsReceiverConsole/Program.cs: the four-line note on hosted-service registration order, and "The guide accompanying this sample explains why".
• QueueTableProvisioner: the 12-line IndexName remark and the 10-line injection remark — two lines each, with the reasoning in the README, would land better.
• SampleDatabase.DefaultConnectionString: the TrustServerCertificate remarks duplicate a README section that says it better.

Related: MsSqlBoxMigrationRunner:244 and the ChannelFactory.cs:46-style references in comments and the PR body will rot — the cited migration-runner code is currently at 238-244. Prefer naming the method. The README itself is excellent and is the right home for nearly all of this.

5. Test coverage

• QueueTableProvisioner is the one piece of real logic the PR adds — idempotence on a second run, convergence when the index is dropped, concurrent execution — and it is verified only by the manual run recorded in the PR body. Paramore.Brighter.MSSQL.Tests already has a live server in CI, so the idempotence and convergence cases look cheap to pin there. Per CLAUDE.md this should have gone through /test-first.
• When_a_subscription_is_not_an_mssql_subscription.cs omits the // Arrange / // Act / // Assert structure that .agent_instructions/testing.md mandates and that its neighbours use (e.g. When_mssql_channel_factory_forwards_scheduler_to_consumers.cs).
• Class naming: testing.md says class names should be [Behavior]Tests, and When_ is "for method names and file names only, never class names". Your file matches the folder's existing style, so this may be a stale rule rather than a real violation — flagging so it can be settled either way.

6. Nits

• using Events; was added to CompetingSender, GreetingsSender and GreetingsReceiverConsole, but the only type in the root Events namespace is IAmACommandCounter, which none of them use — three unused usings (SampleDatabase is in SampleInfrastructure).
• BrighterSqlQueue.sql ends with USE [BrighterSqlQueue]; GO, which does nothing once the batch ends. Harmless, but it reads as if something follows.
• The PR body says the provisioner is in Events/QueueTableProvisioner.cs; it is in SampleInfrastructure/. The README is correct.
• Heads-up rather than a defect: MsSqlMessageQueue's constructor logs the full connection string at Debug, and GreetingsSender configures Serilog at MinimumLevel.Debug(). It happens not to print today, because the producer registry is built before ApplicationLogging.LoggerFactory is set from DI — but the README now tells readers to put an sa password in that connection string, so a reader who raises the receivers' log level would print it. A line of caution in the README, or MinimumLevel.Information in the sender, would cover it.

Overall: the subscription fix, the Host-based sender, the provisioning story and the README are all clear improvements, and the evidence-by-running discipline is appreciated. Item 1 is the one I would resolve before merge.

🤖 Generated with Claude Code

[iancooper]

Four of the five conventions are done in 2bb052a6c (rebased onto master, which had moved). (1) is yours to take and I have not taken it; (5) is recorded and not filed.

2. TODO(#4335) — the style guide settles it, and against me

I added that line last round answering review 17's finding 6, so this is two reviewers disagreeing and a document deciding. code_style.md:40 is unambiguous and the inline reference is gone. The README keeps its markdown link, which is where the traceability belongs.

4. Licence headers

Both new test files now carry Copyright © 2026 Ian Cooper <ian_hammond_cooper@yahoo.co.uk>, matching the sibling in the same folder. QueueTableProvisioner.cs and SampleDatabase.cs had 2014 copied from the template; both now say 2026. For what it is worth, Copyright © 2026 is already the plurality in src and tests at 672 files, so the template is the outlier rather than the convention.

6. s_identifier, and the stray Events file

^[A-Za-z_][A-Za-z0-9_]{0,127}$, so the guard's message is the one you get rather than SQL Server's. Measured with a control on both sides, because a bound that rejects everything would look identical to a bound that works:

QueueTable (shipped)   len=  10  old=True  new=True
128 chars              len= 128  old=True  new=True
129 chars              len= 129  old=True  new=False
bad char ]             len=  11  old=False new=False

The message now reads "is not a plain SQL identifier of 128 characters or fewer".

Events/acceptance_test_message_body.txt is gone. It was a dump of one GreetingEvent body with a MessageType=2 header, referenced by nothing in the repository, and it survived a move it should have gone with.

3. The comments that pin another assembly's internals

All three trimmed, and you are right about which half was load-bearing:

• The two ValidatePipelines comments no longer enumerate what producer-side validation does and does not cover; they say why the call is there and, in GreetingsSender, why it is last.
• The Close/IsSharedConnection sentence is gone. What is left — "Commit clears the provider's transaction, which is what keeps the Rollback below a no-op" — is the part a reader of this sample needs, and it is about this file rather than about RelationalDbTransactionProvider.
• The QueueTableProvisioner remarks keep the conclusion and drop the comparison: the check asks SCHEMA_NAME() because the CREATE TABLE it guards is unqualified and so is everything the gateway then issues, so the guard has to agree. The GetExistsQuery alternatives-considered paragraph is gone.

I have not asked for a samples exemption from code_style.md:43. If one is ever wanted it should be written down rather than inferred from this PR.

6, first bullet — the repeated preamble, which I would keep

You framed this as conditional on my agreeing, and I do not. The four Program.cs files are read one at a time by someone who has opened exactly one of them, and the connection string → EnsureQueueTable → RelationalDatabaseConfiguration sequence is the thing they came to see. Folding it into SampleInfrastructure buys tidiness across files nobody reads together and costs visibility in the file they do. Happy to be overruled.

1. Where QueueTableProvisioner lives — yours, and three reviews now agree

I am not moving it on my own authority: it leaves samples/, it adds public surface to Paramore.Brighter.BoxProvisioning.MsSql, and AddMsSqlQueue(configuration) is an API name. But the argument is right and it is the third time it has been made, so recording where it stands: the code is needed by every MSSQL transport user rather than by this sample, it is the most intricate thing here and the only untested thing here, and moving it puts the concurrent-start behaviour I verified by hand into a CI suite that already runs SQL Server. Say the word and it is a small change.

5. Both worth issues, neither filed

MsSqlMessageQueue.cs:34 logging the connection string at Debug, and :169 interpolating the topic while :215 and :236 parameterize. Both recorded with evidence; filing is your call, as #4334–#4340 were.

---

Verification

The two new test classes run in 51 ms with no database — 9 facts, 0 failures, on both net9.0 and net10.0:

Passed!  - Failed: 0, Passed: 9, Skipped: 0, Total: 9, Duration: 51 ms

All six sample projects and Paramore.Brighter.MSSQL.Tests build with 0 errors after the rebase.

🤖 Generated with Claude Code

[iancooper]

Green at 2bb052a6c: 24 pass, 0 failing. Recording how sqlserver-ci was settled, because this one was not covered by the samples-only re-run rule and I nearly mis-read it twice.

sqlserver-ci and rabbitmq-sync-ci both went red on the first run. sqlserver-ci is reachable from this diff — the PR edits tests/Paramore.Brighter.MSSQL.Tests/ — so it did not get waved through.

What the log actually said, 1 of 247:

Paramore.Brighter.MSSQL.Tests.Outbox.Binary.Sync
  .WhenStoringAMessageWithARelativeDataSchemaItShouldBeReadBack
    Microsoft.Data.SqlClient.SqlException : A severe error occurred on the current command.
      at Paramore.Brighter.MSSQL.Tests.Configuration.EnsureDatabase (Configuration.cs:45)

It failed in database setup at 00:00:01.51, not in an assertion, and the Azure SQL Edge container log shows the race that produced it — [DBMgr::FindFreeDatabaseID] Next available DbId EX locked: 5 on spid51 and : 6 on spid52 at the same instant, the two TFM test hosts creating a database concurrently against one container.

The failing test is not one of mine; it arrived from master in the rebase. The 247 is master's 238 plus the 9 added here.

A re-run of the failed jobs on the same commit passed, 24 of 24. By the rule this repository has been using — two reds on one commit is a defect, a red that clears on a re-run of the same commit is a distribution — that settles it as the latter, in the same test-isolation-racy family as the postgres-ci and dynamo-ci cases.

One connection I am not waving away: this PR adds 9 tests to that project, and adding tests changes the timing of a race. That is weaker than a code change and it is not nothing, so it is written down rather than argued away.

And a correction to my own method, which is the part worth keeping. My first reading of the log was "no tests ran at all" — I had grepped for the execution markers, found none, and was about to conclude the step died before running. Running the same grep against a passing sqlserver-ci job returned the same count, so the absence was an artefact of my filter and proved nothing. The two-way control is what caught it; a control that only confirms absence confirms nothing about the grep.

🤖 Generated with Claude Code