FEAT: Generated transport conformance tests should cover new reject/dlq/requeue paths

[iancooper]

Generated transport conformance tests - should cover new reject/dlq/requeue paths

Closes #4240.

While generating the transport conformance tests, I added new functionality around reject/dlq/requeue. This functionality can now be universal; where the message-oriented middleware doesn't support it, we can fall back to a Brighter substitute.

This replaces the four legacy per-transport gate keys, which controlled generation of functionality for these features, dependent on native support, with a generated canonical conformance suite that runs
against every gateway.

As some gateways have not yet implemented Brighter support for missing features, we also added a checked-in conformance ledger (specs/0036-universal-transport-conformance-tests/conformance-status.md) recording per-configuration conformance for 11 canonical behaviors (FR-2, 4, 5, 6, 7, 8, 9, 15, 16, 17, 22).

Every canonical test's Skip is driven by this ledger, so a behavior is either proven against a real broker or carries a signed-off Deferred -> #4240 marker. No silent skips.

Status — 62 of 62 tasks; phases 0–6 complete

All 24 wired configuration rows are resolved — zero Unknown cells, across all twelve targeted
transports:

Configuration	Result
AWS V3 x4 + V4 x4	11 Pass, except SqsFifo FR-9 Deferred (SQS FIFO rejects per-message delay) and Sns* FR-9 Fixed
Kafka Classic / Consumer / PartitionKey	11/11 Pass
PostgresSQL	11/11 Pass (native visibility-timeout redelivery)
MSSQL, Redis	10 Pass + FR-16 Deferred (destructive read)
RMQ.Async Classic / Quorum	10 Pass + FR-5 Deferred (invalid-channel routing)
RMQ.Sync	10 Fixed + FR-5 Deferred, mirroring RMQ.Async
MQTT	10 Fixed + FR-16 Deferred
RocketMQ	9 Fixed + FR-2 / FR-15 Deferred (upstream no-op Requeue)
GCP Pull / PullOrdering	4 Pass + FR-9 Fixed + 6 Deferred (emulator-only verification)
GCP Stream / StreamOrdering	all 11 Deferred — streaming pull hangs on the emulator, excluded in CI, no real creds
AzureServiceBus	all 11 Deferred — no compose file, no credentials, no emulator; deferral of verification, not a finding of non-conformance

55 of the 264 cells are Deferred, each carrying a signed-off #4240 marker.

Transport src changes (deliberately localized)

• Paramore.Brighter.MessagingGateway.RocketMQ — guard the Baggage property; empty baggage crashed every send.
• Paramore.Brighter.MessagingGateway.GcpPubSub — GcpPullMessageConsumer.Receive/ReceiveAsync ignored timeOut and long-polled, blocking the pump; now bound via CallSettings expiration.
• Paramore.Brighter.MessagingGateway.AWSSQS (V3 + V4) — sync SendWithDelay passed TimeSpan.Zero.

Everything else is test-harness, generator template, or ledger work.

Terminal cleanup — done

Gated on the zero-Unknown ledger, and performed in the order the acceptance criteria require (the
test-configuration.json keys last, so each earlier step was provably a no-op):

• the four legacy templates and their generated copies deleted — 8 templates + 72 copies, 80 files;
• the SkipTest gate branches and the LegacyGatedTemplates closed list stripped;
• the three retired properties removed from MessagingGatewayConfiguration;
• the three gate keys removed from every test-configuration.json (9 files, 63 occurrences).

Regeneration after each step was a content no-op — not one generated file differs — which is the
proof that nothing was still reading those keys. The three retained capability gates
(HasSupportToPublishConfirmation, HasSupportToValidateBrokerExistence,
HasSupportToValidateInfrastructure) keep their meaning and are still tested.

One retained gate was found to be mis-declared in the same way the retired ones were, and is
corrected here rather than left. Kafka / Consumer was the only Kafka configuration setting
HasSupportToValidateInfrastructure: false, so Classic and PartitionKey ran assume_channel and
validate_channel while Consumer ran neither. Measured against a live broker, that configuration
splits cleanly: validate_channel passes both variants, assume_channel fails both,
deterministically — the flag was suppressing two working tests in order to suppress two that do not
work. A narrower gate, HasSupportToDetectMissingInfrastructureOnAssume, now skips assume_channel
alone; HasSupportToValidateInfrastructure is unchanged in meaning, so MQTT and Redis are unaffected.
The Kafka suite goes 188 -> 190 pass / 0 fail, by exactly the two tests recovered.

The underlying cause is a real behavioural difference, declared rather than papered over: EnsureTopic()
returns immediately on OnMissingChannel.Assume without an admin call, and the test compose disables
topic auto-creation, so the topic genuinely does not exist — the classic consumer surfaces that, while
the KIP-848 consumer-group protocol completes silently. Fixing that error surfacing in
KafkaMessageConsumer is deliberately not attempted here; it sits outside this PR's localized src
boundary and belongs with the KIP-848 author.

CI audit — no silent skips

A read-only, network-free audit (it never queries the issue tracker) enforces the deferral trail in
three ways:

• every messaging-gateway Skip must match Deferred: #<n> — a bare or reasonless Skip fails the
  build;
• every Deferred ledger cell must carry both an issue link and a recorded sign-off;
• every generated test's Skip must agree with its own (configuration × behaviour) ledger cell, in
  both directions — a cell flipped without regenerating fails, whether that leaves a test wrongly
  skipped or wrongly running. The expected value comes from the generator's own emitter, so the audit
  cannot drift from what generation produces.

The audits assert their own non-vacuity (files scanned, markers found, and that both branches were
exercised), and each failure mode is covered by a synthetic-tree test, so they cannot pass by checking
nothing.

Out of scope

Fixing all brokers that don't currently implement Brighter support when there is no native support is out of scope for this work.

Every Deferred cell resolves to #4240, this PR's own tracking issue, by maintainer ruling — no
per-deferral follow-up issues are raised. The #NNNN pre-audit placeholders are fully reconciled: none
remains in the ledger or in any generated Skip marker.

🤖 Generated with Claude Code

[claude]

Review: Universal transport conformance tests (1 of 2)

Reviewed the hand-written surface — the six src/ files, the generator (ConformanceLedger, MessagingGatewayGenerator, BaseGenerator), the 24 new Liquid templates, the ledger, and a sample of the providers/harnesses. I skimmed rather than read the ~74k lines of generated output, since it is mechanically derived from the templates.

The shape of this is good: replacing three coarse capability booleans with a per-(configuration x behaviour) ledger that drives Skip is a real improvement, and forcing every deferral to carry a signed-off marker is the right governance move. Comments are ordered by how much I think they matter. I could not build or run anything in this environment, so everything below is from reading.

---

1. GCP: Expiration.None does not preserve prior behaviour

src/Paramore.Brighter.MessagingGateway.GcpPubSub/GcpPullMessageConsumer.cs

The comment on BuildPullCallSettings says the null-timeout branch preserves prior default behaviour, but it does not. Previously that path called PullAsync(request, cancellationToken), which merges with the configured PullSettings on SubscriberServiceApiClient — including its default expiration. CallSettings.FromExpiration(Expiration.None) explicitly overrides that default and makes the call unbounded, so the null path is now less bounded than before.

Returning null CallSettings on that branch would actually preserve it:

private static CallSettings? BuildPullCallSettings(TimeSpan? timeOut) =>
    timeOut is { } window && window > TimeSpan.Zero
        ? CallSettings.FromExpiration(Expiration.FromTimeout(window))
        : null;

Two smaller points on the same change:

• Swallowing DeadlineExceeded as an empty receive also swallows it when the broker is genuinely slow or unhealthy, not just when the subscription is empty. Unavailable gets Log.ReceiveConnectionError; consider at least a debug-level log on the deadline path so that failure mode is not invisible.
• With a very short timeOut and a message actually available, the deadline can fire after the server has dispatched the message. It is then delivered-but-unacked and invisible until the ack deadline expires. Worth a note on the floor below which callers should not go.

2. MQTT: ReceiveAsync now blocks the calling thread

src/Paramore.Brighter.MessagingGateway.MQTT/MQTTMessageConsumer.cs:210-244

Receive gained a Thread.Sleep(10) poll loop, and ReceiveAsync is Task.FromResult(Receive(timeOut)). Receive runs before the Task is constructed, so the proactor path now blocks its caller synchronously for the full timeout (300 ms by default, longer if the pump passes its own), busy-sleeping in 10 ms slices. The cancellationToken is ignored entirely, so shutdown cannot interrupt the wait. Before this change Receive returned immediately on an empty queue, so this is a new blocking cost on every empty poll.

Suggest signalling rather than polling — a SemaphoreSlim released by the ApplicationMessageReceivedAsync handler, with Wait(timeOut) for the sync path and await WaitAsync(timeOut, cancellationToken) for the async one. That removes the sleep loop, honours cancellation, and reacts on arrival instead of on the next 10 ms tick.

Minor, same method: DateTime.UtcNow for the deadline is subject to wall-clock adjustment; Stopwatch is the monotonic choice, and the templates already use it for exactly this.

3. FR-5 templates skip the FR-8 relaxation guard

..._delivery_error_should_send_to_dlq, ..._with_unknown_reason_..., ..._should_include_metadata, and ..._unacceptable_and_no_invalid_channel_... all wrap their metadata assertions in if (keys.StampsRejectionMetadata). When_rejecting_message_with_unacceptable_reason_should_send_to_invalid_channel does not — both Reactor and Proactor assert Bag.ContainsKey(keys.OriginalTopic) unconditionally.

For a native-dead-letter provider (RMQ returns string.Empty for all five keys) that resolves to Bag.ContainsKey(""), which fails. It is masked today only because RMQ.Async FR-5 is Deferred. As soon as a native-dead-letter transport is proven conformant on FR-5, that template fails for a reason unrelated to FR-5. Please add the same guard for consistency.

4. FR-7 assertion can be satisfied by the failure it is meant to catch

In When_rejecting_message_with_no_channels_configured_should_acknowledge_and_log, received2 is only checked for NotEqual(MT_NONE, ...). If the reject returned true but did not remove M1, M1 is redelivered on visibility-timeout expiry and satisfies the assertion — precisely the requeue loop AC-7 exists to forbid. Assert.NotEqual(received1.Id, received2.Id) is ordering-agnostic, so safe across transports, and closes the hole.

Separately, the _and_log half of the behaviour is never asserted. Either rename the test or assert the log — the PR already pulls OpenTelemetry.Exporter.InMemory into two test projects, so the infrastructure is close at hand. As written, the ledger records Pass for a behaviour that is half-verified.

5. Retry bounds are split between template and provider, and the comment is wrong

Every DLQ / invalid-channel template says // bounded retry loop: 500 ms poll, 30 s ceiling (NFR-2) and then loops on GetMessageFromDeadLetterQueueAsync(_subscription) with no delay in the loop body. All pacing comes from whatever the provider does internally, and the interface only says "polls with a bounded retry" without naming the bound. In practice providers differ by two orders of magnitude:

Provider	Internal behaviour per call
KafkaClassicMessageGatewayProvider.ReceiveOneAsync	one 500 ms receive
PostgresMessageGatewayProvider / MsSqlMessageGatewayProvider	for (i < 10) of a 5 s receive plus Task.Delay(1000) — up to 60 s

Consequences:

• The 30 s ceiling is not a ceiling. A single Postgres/MSSQL call can exceed it, so the outer loop is decorative there.
• The 500 ms poll is not a poll interval; it is a Kafka implementation detail that happens to pace the loop. A provider implemented as a non-blocking peek would spin hot against the broker for 30 s.
• Negative assertions pay the full internal retry. The FR-5 template ends with a single GetMessageFromDeadLetterQueue call asserting MT_NONE — on Postgres/MSSQL that is ~60 s of polling to prove a queue is empty, per test, per variant.

Suggest making the contract explicit: GetMessageFromDeadLetterQueueAsync(subscription, TimeSpan timeOut, CancellationToken ct) performing exactly one bounded read, with all retry living in the template. That makes the comment true, makes the ceiling real, and makes negative assertions cheap.

[claude]

Review: Universal transport conformance tests (2 of 2)

Should fix

MQTT producer hardcodes InstrumentationOptions.All — MQTTMessageProducer.cs:133,166. Every other gateway threads a configured value (_instrumentation, _options, instrumentation); MQTT is the only one that ignores configuration and always emits full producer events. MqttMessageProducer has no instrumentation constructor parameter today, so this needs one added (following MsSqlMessageProducer) rather than a constant.

The ledger fails open. ConformanceLedger.GetSkip returns string.Empty for an unknown row key, an unknown column, and any cell value outside the four known prefixes (ComputeSkip falls through to string.Empty). MessagingGatewayGenerator.LoadLedgerFromFileSystem returns null when the file is not found, and GenerateAsync then silently passes prepareModel: null, disabling skips for the whole run with no log line. Each of these turns a governance mistake — a renamed row, a typo, a unicode arrow in Deferred -> #123, running the generator from a published location — into "this behaviour is now claimed conformant", which is the exact silent-skip failure the ledger exists to prevent.

All 24 LedgerKey values in the test-configuration.json files do currently resolve against the table, so nothing is broken today. But I would make it fail loudly: throw on an unrecognised cell value, throw or log an error on a LedgerKey absent from the ledger, and log a warning when the ledger file cannot be located. Also worth noting the header detection is line.StartsWith(pipe) && line.Contains("FR-2") && line.Contains("FR-4"), which would bind to the legend table at lines 11-16 if anyone ever mentioned those columns there.

Docker resource and image changes. docker-compose-rocketmq.yaml raises the broker heap from 128 MB to 2 GB and the proxy from 64 MB to 1 GB — with the nameserver that is well over 3 GB of JVM heap on what may be a 7 GB CI runner. Was this validated against the CI runner size, or is it a local-only accommodation? docker-compose-rmq.yaml also switches from the official rabbitmq:management to brightercommand/rabbitmq:4.2-management-delay; a floating tag on an org-owned image is a supply-chain and reproducibility consideration worth calling out in the PR description, and ideally digest-pinning.

RocketMQ baggage guard is symptom-scoped. The fix guards Baggage because empty baggage crashed every send, but Source, SpecVersion, Topic, and both ContentType properties (RocketMqMessageProducer.cs:80-85,113-114) are equally unguarded against the same AddProperty rejection. If the underlying cause is "AddProperty throws on an empty value", a small AddPropertyIfNotEmpty helper would close the class of bug rather than one instance. Unrelated pre-existing oddity while you are in there: HeaderNames.Type is added twice, at lines 93-96 and 108-111, under different conditions.

No unit-level regression tests for the three src fixes. CLAUDE.md makes /test-first mandatory for bug fixes. The AWS SendWithDelay and GCP timeout fixes are covered by the canonical suite, but only against a live broker — and the MQTT Receive rewrite has no test at all. The TimeSpan.Zero bug in particular is trivially unit-testable with a fake producer and would have been caught years earlier by one.

Nits and maintainability

• When_generating_everywhere_should_find_exactly_twenty_wired_configurations asserts EXPECTED_WIRED_CONFIGURATION_COUNT = 24.
• TEMPLATE_FR_COLUMNS in that same test duplicates the generator CANONICAL_TEMPLATE_FR_COLUMNS, with a comment acknowledging it is a mirror. If they drift, the gate silently checks the wrong column. Make the generator map internal and add InternalsVisibleTo — there is already an AssemblyInfo.cs in the test project.
• The structural gate checks presence and Skip agreement but not content. With ~74k lines of generated code checked in, a hand-edit to a generated file is undetectable. A regenerate-to-temp-and-diff test would close that.
• Eight *HarnessMessageScheduler.cs files, ~1450 lines, differing mainly in producer/publication type. Given the generator gained a Shared/ template folder in this PR, this looks like a natural Shared/HarnessMessageScheduler.cs.liquid.
• MessagingGatewayGenerator mixes SCREAMING_SNAKE (CANONICAL_TEMPLATE_FR_COLUMNS, FR_COLUMN_BEHAVIOURS) and PascalCase (LegacyGatedTemplates) for statics in the same class.
• SnsHarnessMessageScheduler.Republish takes _lock to add a producer; if a timer fires during Dispose, the add lands after _producers.Clear() and that producer is never disposed. Test-only leak, but a _disposed flag under the lock is two lines.
• DateTimeOffset.TryParse(timestampValue, out var parsed) in the FR-8 template — worth pinning to CultureInfo.InvariantCulture / DateTimeStyles.RoundtripKind for an ISO-8601 value.
• FR-8 cells read Pass for RMQ, with the "routing only, no metadata stamped" caveat living solely in prose. The table is the machine-readable artifact; a distinct value such as Pass (routing only) would stop it overstating conformance.
• manual-test-plan.md at the repo root — specs/0036-.../ looks like the natural home.
• ADR 0070 is deleted rather than marked Superseded with a pointer to 0066/0067. It was only Proposed, so this is defensible, but the index already carries a Deprecated status and the decision trail has some value.

PR description is stale relative to the ledger

The description says MQTT / RMQ.Sync / AzureServiceBus are the only Unknown rows left, but the committed matrix has all three resolved (MQTT: 10 Fixed plus FR-16 Deferred; RMQ.Sync: 10 Fixed plus FR-5 Deferred; ASB: 11 Deferred) and there are no Unknown cells anywhere — confirmed by no NNNN placeholder appearing in any generated test. It also lists two Kafka rows where the configs and ledger have three (Classic, Consumer, PartitionKey). Worth refreshing before this leaves draft, since the description is what a reviewer trusts for the conformance summary.

---

Nice work overall. Item 5 in part 1 is the one I would most want resolved before the terminal cleanup lands, since a lot of test wall-clock and a lot of the credibility of the suite rest on those bounds.

[claude]

Review — #4297 Generated transport conformance tests (1/2: blocking items)

Reviewed by category: the 6 src/ gateway changes, the generator/ledger machinery in tools/, the new generator tests, the harness schedulers, the ledger, and CI/compose wiring. Generated output was spot-checked rather than read line by line.

Genuinely good work — replacing four boolean capability gates with a per-(configuration × behaviour) ledger is a real improvement in honesty, and the ledger's prose annotations (why SQS FIFO can't do per-message delay, why Redis/MSSQL can't Nack) are the kind of thing that usually gets lost. The IAmAConformanceLedger seam with an InMemoryConformanceLedger for tests is clean and idiomatic here.

I was unable to build or run anything in this environment, so everything below is from reading the diff.

---

1. Paramore.Brighter.Test.Generator.Tests is never run in CI 🔴

The project is in Brighter.slnx:259, but there is no dotnet test line for it anywhere in .github/workflows/ci.yml — the unit-test job (lines 63–65) runs only Core, Extensions and Transforms. This PR adds ~19 test classes there, including When_ledger_marks_a_cell_should_emit_skip_only_when_not_proven and the whole CanonicalTemplates/ set.

Those are the only automated guard on the mechanism this PR is built around. As it stands they pass locally and never run again. Please wire the project into the unit-test job.

2. A missing ledger silently disables the entire Skip mechanism 🔴

MessagingGatewayGenerator.cs:97,102,141:

_ledger = ledger ?? LoadLedgerFromFileSystem();
var prepareModel = _ledger != null ? (Action<string, object>)SetCanonicalSkip : null;

LoadLedgerFromFileSystem returns null when FindLedgerPath can't walk up to the ledger (ConformanceLedger.cs:71-81). Then prepareModel is never invoked, MessagingGatewayConfiguration.Skip stays at its deserialized default of null, and — by the reasoning in your own comment at MessagingGatewayGenerator.cs:277-280 (nil != empty is true in Liquid) — every canonical template renders [Fact(Skip = "")].

A regeneration run from a layout where the walk-up fails therefore rewrites 400+ generated files with a meaningless empty Skip and drops every deferral, with no error and nothing in the log. That is precisely the failure mode the "no silent skips" goal exists to prevent, just relocated from test-run time to generation time.

Suggest: throw from GenerateAsync when the ledger can't be located (it is a required input now, not an enhancement), and initialise Skip to string.Empty on the property rather than depending on prepareModel running.

3. #4240 is both the closing issue and every deferral target 🔴

All 40-odd Deferred -> #4240 (sign-off: @maintainer) cells, plus every Fixed (#4240), point at the issue this PR closes. On merge every deferral resolves to a closed issue and the governance the ledger provides quietly evaporates. The body commits to reconciling these — that needs to happen before merge, not after. Also:

• @maintainer is a literal placeholder in all 24 rows, so no cell is actually signed off by a named person, despite the vocabulary at conformance-status.md:16 requiring one.
• Fixed (#4240) uses the issue number where the vocabulary (line 15) specifies Fixed (#PR/commit); #4297 would be correct.

4. MQTTMessageConsumer.ReceiveAsync now blocks a thread-pool thread 🔴

MQTTMessageConsumer.cs:217-221 turns Receive from a non-blocking drain into a Thread.Sleep(10) poll loop up to timeOut. But ReceiveAsync (lines 239-243) is Task.FromResult(Receive(timeOut)) — so the proactor path now synchronously blocks its calling thread for the full timeout before the Task is even created. With the suite's 2000 ms negative windows and 500 ms polls, an MQTT proactor pump holds a thread-pool thread for seconds at a time. That is a starvation hazard under concurrency and a regression from Receive returning immediately.

Same method, three related points:

• Make the wait async. A SemaphoreSlim signalled from the ApplicationMessageReceivedAsync handler gives await sem.WaitAsync(timeOut, ct) for the async path and sem.Wait(timeOut) for the sync path — no polling, no 10 ms latency floor, and cancellationToken finally honoured (it is currently ignored entirely).
• DateTime.UtcNow for the deadline (lines 217-218) is wall-clock and moves under NTP/DST adjustment; Stopwatch or the TimeProvider used elsewhere in this codebase would be correct.
• The drain is unbounded (lines 231-234). Documenting the Channel.BufferSize obligation is honest, but now that the method waits before draining, overflow is materially more likely than before. Capping at the consumer's batch size turns a caller-facing InvalidOperationException into backpressure.

(Non-blocking findings — instrumentation, GCP expiration, ledger parsing, CI cost, and assorted polish — follow in a second comment.)

[claude]

Review — #4297 (2/2: non-blocking findings)

5. MQTT producer hardcodes InstrumentationOptions.All 🟠

MQTTMessageProducer.cs:133,166. All includes RequestBody, and BrighterTracer.WriteProducerEvent:1054-1055 adds the full message body as a span tag under that flag. So every MQTT payload is exported into traces regardless of configuration — a PII exposure and a span-size cost that an operator cannot turn off.

Every other transport threads the configured value through (KafkaMessageProducer.cs:293 → _instrumentation, RmqMessageProducer.cs:197 → _instrumentationOptions, SnsMessageProducer.cs:181 → _options). The MQTT producer has no such field, so this needs a constructor parameter to match. Slightly larger change, but tracing that can't be dialled down is worse than no tracing.

6. GCP: Expiration.None is not the prior default 🟠

GcpPullMessageConsumer.cs:194-198. The comment says the null case preserves prior default behaviour, but it doesn't: previously the call used the client's default PullSettings expiration (a bounded deadline), whereas Expiration.None explicitly overrides that with no deadline at all. A Receive(null) against a wedged connection can now hang forever where it used to time out. Returning null (or CallSettings.FromCancellationToken(ct)) for that case would actually preserve the default.

The bounded-Pull fix itself and the DeadlineExceeded → empty receive handling both look right.

7. Unrecognised ledger vocabulary fails open 🟠

ConformanceLedger.cs:86-87 and :115 all return string.Empty, which means "run the test":

• A LedgerKey typo ("Kafka/Classic" vs "Kafka / Classic") → row not found → all 11 canonical tests run unskipped.
• A cell value outside the vocabulary (Fail, Defered -> #123) → falls through line 115 → test runs.
• A new transport with no ledger row → all canonical tests run.

The last is arguably the right default. The first two are config errors that should be caught at generation: have the parser throw on any cell not matching the four documented forms, and GenerateAsync throw when a non-null LedgerKey has no matching row. All 24 keys currently match all 24 rows, so this is about keeping it that way.

Minor parser notes: the header heuristic Contains("FR-2") && Contains("FR-4") (line 141) would also match a row containing FR-22/FR-4x, and ParseLedger stops at the first non-| line, so the table must stay one contiguous block.

8. The ledger lives in a spec folder but is a permanent generator input 🟠

RELATIVE_LEDGER_PATH (ConformanceLedger.cs:60-61) hardcodes specs/0036-universal-transport-conformance-tests/conformance-status.md. Spec folders are per-feature artefacts by the workflow in CLAUDE.md; this file is now a forever runtime dependency of a shipped tool. Archive or rename that directory and the generator silently drops into failure mode #2. Consider moving it to tools/Paramore.Brighter.Test.Generator/ (with CopyToOutputDirectory) and leaving a pointer in the spec.

9. RocketMQ tests are commented out in CI 🟠

ci.yml:749. The ledger claims 9 Fixed (#4240) cells for RocketMQ and this PR ships a src fix to RocketMqMessageProducer.cs:85-90, so that fix has zero CI coverage and those nine claims can't regress-detect. The docker-compose-rocketmq.yaml bump (broker 128m → 2g, proxy 64m → 1g) suggests the job was disabled for resource reasons — worth recording that in the ledger, or re-enabling now that sizing is understood. A standard runner has 7 GB, so 3 GB of JVM heap will be tight alongside everything else.

The baggage guard itself is correct and well-scoped.

10. PR description is materially out of date 🟡

"Remaining before this leaves draft" lists work already done in this diff:

• FR-20 onboardings — MQTT, RMQ.Sync and AzureServiceBus all have test-configuration.json, providers, LedgerKeys and full generated suites (16/19/20 files per variant).
• Terminal cleanup — the three HasSupportTo* properties are already removed from MessagingGatewayConfiguration.cs, their SkipTest branches are gone (MessagingGatewayGenerator.cs:188-199), and the keys are stripped from every test-configuration.json.
• Zero Unknown cells across all 24 rows, so the cleanup gate at conformance-status.md:6-7 is satisfied.

Worth refreshing the body and considering taking it out of draft; as written a reviewer would think three transports and the whole cleanup are still outstanding.

11. Smaller things

• SnsHarnessMessageScheduler.Republish swallows everything (lines 145-149). The stated rationale is that failures surface as the after-delay arm timing out, but that turns a one-line credential/topic error into a 30-second timeout with MT_NONE and no clue why. Please surface the exception (ITestOutputHelper, or at minimum Console.Error). Same pattern in the sibling schedulers.
• Dispose/Republish lock ordering, same file: a timer callback blocked on _lock (line 137) can add to _producers after Dispose has run _producers.Clear() (line 172), leaking an undisposed producer. Harness-only, but a _disposed flag checked under the lock closes it.
• Stale test comment, When_ledger_marks_a_cell_should_emit_skip_only_when_not_proven.cs:222: "The chosen row is one that is currently Unknown, so today it also exercises the #NNNN emission path." Redis / RedisMessagingGateway FR-22 is now Pass (conformance-status.md:345), so the test takes the much weaker DoesNotContain branch and no longer exercises #NNNN. Either pick a currently-deferred cell (Redis FR-16) or drop the claim.
• The fixture clobbers a real template in the build output (same file, ctor lines 56-64): it overwrites the real When_requeuing_a_failed_message_should_be_redelivered.cs.liquid under AppContext.BaseDirectory and restores it in Dispose. DisableTestParallelization = true handles concurrency, but an aborted run leaves the output directory corrupted until rebuild, and it serialises the whole assembly for one class's needs. Writing the stub to a temp Templates dir would be more robust — though that needs BaseGenerator to accept a template root rather than hardcoding AppContext.BaseDirectory.
• Model mutation across renders: MessagingGatewayConfiguration.Skip is transient per-render state living on a JSON-bound config object, mutated in place by prepareModel (BaseGenerator.cs:78). Works because rendering is sequential and awaited, but breaks the moment the template loop is parallelised, and the Shared pass is invoked without prepareModel so it inherits whatever the last Proactor template left. A small per-render wrapper model would be cleaner and let Skip be non-nullable.
• Dictionary drift: CANONICAL_TEMPLATE_FR_COLUMNS and FR_COLUMN_BEHAVIOURS are maintained separately and GetValueOrDefault(frColumn, frColumn) (line 270) silently masks a missing label; one record array would make them impossible to desync. Nothing asserts that each name in the canonical map corresponds to a template that exists, either — a rename would silently un-defer 20 configurations' worth of tests.
• Naming: those two are static readonly, not const. .agent_instructions/code_style.md:10 reserves ALL_CAPS for const; PascalCase (or s_-prefixed, matching s_logger elsewhere) would fit.
• Skip strings aren't escaped into the C# literal (Skip = "{{ Skip }}"); a " or \ in a cell yields uncompilable output. Ledger content is trusted, so defence-in-depth — the vocabulary validation in Should Brighter move into its own repository #7 would cover it.

On the src fixes

The two SnsMessageProducer.SendWithDelay one-liners (V3 and V4) are clearly correct — the sync path was discarding its delay argument. Good catch, and good that the V4 copy was found too.

On CI cost

Each *_with_delay_* canonical test has a hard floor of ~7 s (5 s delay + 2 s negative window) and up to 37 s on failure, times two variants, times 20 wired configurations. Worth measuring the added wall-clock per transport job before this lands, and possibly parameterising the 5 s delay per configuration so slow brokers get headroom and fast ones don't pay for it. Relatedly, the arrange-phase receive in the delay templates is a single bounded ReceiveAsync(ReceiveMessageTimeoutInMilliseconds) with a hard assert while every later receive uses the bounded retry loop — that asymmetry is a plausible flake source on a cold broker, and reusing the retry pattern for the first receive costs nothing.

[claude]

Review — Generated transport conformance tests (#4297) — part 1/2

Reviewed the hand-written surface: src/ gateway changes, the generator tool + Liquid templates, the ledger machinery and its audits, and a sample of the harness providers. The 70k lines of generated .cs were spot-checked for ledger↔skip agreement rather than read line by line.

Overall: big, coherent work and the direction is right — replacing four boolean capability gates with a per-cell ledger is a real improvement, and the audit suite (non-vacuity assertions, synthetic-repo canaries, EnumerateGatewayArtifacts shared between both audits so they provably scan the same set) is better engineered than most conformance harnesses. Findings below are mostly silent-fallback paths in the ledger plumbing plus a few template/gateway bugs. Marked WIP, so several may already be on your list.

I verified the wiring end to end: 24 LedgerKeys, 24 ledger rows, all matching; 24 configs × the full canonical set in both variants; MSSQL/FR-16 and RMQ.Async / Classic/FR-5 render skips correctly and Kafka renders none.

Correctness

1. MqttMessageConsumer.ReceiveAsync now synchronously blocks the caller — src/Paramore.Brighter.MessagingGateway.MQTT/MQTTMessageConsumer.cs:241-244

ReceiveAsync is Task.FromResult(Receive(timeOut)). Receive now spins on Thread.Sleep(10) until the deadline (lines 218-221). Before this change it returned essentially immediately, so the sync-over-async delegation was harmless; now the Proactor pump blocks a thread-pool thread for the full timeout before the Task is even constructed, and cancellationToken is ignored entirely — a cancelled token cannot shorten the wait. Suggest a real async loop (await Task.Delay(10, cancellationToken)), or better, signal arrival with a SemaphoreSlim / Channel<Message> from the ApplicationMessageReceivedAsync handler and drop polling on both paths.

2. FR-5 template asserts metadata without the StampsRejectionMetadata guard — tools/.../Templates/MessagingGateway/Reactor/When_rejecting_message_with_unacceptable_reason_should_send_to_invalid_channel.cs.liquid:77-81 (and the Proactor twin)

Every other reject template gates its metadata assertions on if (keys.StampsRejectionMetadata). This one does not — it indexes Bag[keys.OriginalTopic] unconditionally. The three RMQ providers declare all-empty keys (RmqClassicMessageGatewayProvider.cs:363-370, plus the Quorum/Sync equivalents), so Bag.ContainsKey("") is false and Assert.True fails. Masked today because all three RMQ configs carry FR-5 Deferred — but that means the ledger attributes the deferral to invalid-channel routing not being conformant when the test as written cannot pass for a native-DLX transport regardless of gateway behaviour. Worth fixing the guard before reconciling those #4240 rows, or the recorded deferral reasons are wrong.

3. GCP: Expiration.None is not the prior default — src/Paramore.Brighter.MessagingGateway.GcpPubSub/GcpPullMessageConsumer.cs:194-197

The comment says a null timeout "leaves the call unbounded, preserving the prior default behaviour", but the two differ. PullAsync(request, cancellationToken) resolves to CallSettings.FromCancellationToken, which leaves the client's configured default expiration and retry settings in place. CallSettings.FromExpiration(Expiration.None) explicitly overrides that default to no deadline at all — so a null-timeout Receive/ReceiveAsync can now hang indefinitely where it previously had the client default. Returning null CallSettings (plus .WithCancellationToken on the async path) in that branch preserves the old behaviour exactly.

Secondary: Expiration.FromTimeout(window) makes the caller's timeout the total gRPC deadline, covering connection setup and retries. With a short pump timeout on a cold channel that can DeadlineExceeded before a message that genuinely exists is returned — worth a little headroom.

4. Missing ledger row / typo'd LedgerKey silently un-skips, and the audit cannot see it

ConformanceLedger.GetSkip returns string.Empty for an unknown row or column (ConformanceLedger.cs:86-87), and ComputeSkip returns empty for any unrecognised cell value (line 115). SetCanonicalSkip likewise sets Skip = "" when LedgerKey is null. So a typo, a renamed configuration, or a dropped ledger row all produce no skip — the behaviour is silently treated as proven.

The obvious guard does not catch it either: AssertCanonicalSkipMatchesLedger (When_generating_everywhere_...:178) computes its expectation through the same GetSkip, so both sides agree on "no skip" and the test passes. Nothing asserts LedgerKey ∈ ledger rows, nor that every ledger row is claimed by exactly one config.

Suggest throwing on an unknown row/column at generation time, plus a test asserting the LedgerKey set equals the ledger row set. The ADR is rightly strict about "no silent skips" — "no silent un-skips" deserves the same treatment, and is the more dangerous direction since it surfaces as a red CI run against a real broker rather than a missing test.

5. Missing ledger file disables the whole suite — MessagingGatewayGenerator.cs:252-256

LoadLedgerFromFileSystem returns null when the ledger cannot be found → prepareModel is null → Skip is never assigned on the model. Per your own comment at lines 281-283 (nil != empty is TRUE in Liquid), that renders [Fact(Skip = "")] on every canonical test in every transport, which xUnit treats as skipped. GatewaySkipConventionAudit does flag empty Skip values, so it fails eventually — but as several hundred violations rather than "conformance ledger not found". Given the spec directory is intended to be temporary, throwing here would be friendlier.

6. LedgerSkipCrossCheckAudit direction-1 is effectively vacuous — LedgerSkipCrossCheckAudit.cs:116-124

The Skip→Ledger check compares sets of issue numbers. Every Deferred cell uses #4240, so it reduces to {4240} ⊆ {4240} and passes regardless of which cells are deferred or which tests are skipped. Direction 2 only validates cell formatting — it never asserts a Deferred cell has a corresponding generated Skip. The docstring's "two-direction cross-check" does less than it reads; keying on (LedgerKey, FR-column) → expected file would make both directions real. Related: SIGN_OFF_PATTERN accepts the literal @maintainer, so the sign-off gate is satisfied by the placeholder — worth tightening in the same pass as the #4240 reconciliation.

7. Coverage regression: requeue-count-exceeded → DLQ deleted with no replacement

When_requeuing_a_message_too_many_times_should_move_to_dead_letter_queue is gone from both templates and all four generated copies. The 11 canonical behaviours cover reject→DLQ but not requeue-count-exceeded→DLQ, a distinct Brighter behaviour (Subscription.RequeueCount / handled-count). Deliberate? If so it deserves a line in the ADR or ledger — it is the only capability the old gate set covered that the canonical set does not.

8. Nack two-message test does not assert what its comment claims — When_nacking_a_message_it_should_be_redelivered.cs.liquid:135

received2 is asserted non-MT_NONE but never asserted to be message2 — a second redelivery of M1 passes. _messageAssertion.Assert(message2, received2) closes it. Separately, line 119 asserts strict head-of-line ordering (M1 before M2), stronger than FR-16 and inherently order-dependent on non-FIFO brokers; likely a flake source on SQS Standard and Kafka even though the ledger records both Pass.

Performance

9. DLQ/invalid-channel poll loops spin with no sleep, and nest inside a longer inner retry

When_rejecting_message_should_include_metadata.cs.liquid:67-74 and siblings loop for 30 s calling GetMessageFromDeadLetterQueue(_subscription) with nothing between iterations. Unlike the channel loops (which block inside Receive(500ms)), nothing guarantees this call blocks — and the SQS implementation builds a new ChannelFactory and channel on every call (SqsStandardMessageGatewayProvider.cs:255-256), so each iteration re-provisions the DLQ channel.

For AWS the numbers invert: the provider internally retries 10 × (5 s receive + 1 s delay) ≈ 60 s (:260-269), exceeding the template's documented "30 s ceiling" — the outer loop runs exactly once and is dead code, while the effective budget is a minute, not 30 s. That lands hardest on the negative assertions: ..._should_send_to_invalid_channel.cs.liquid:84 does a single "DLQ must be empty" read costing ~60 s on AWS. Times 4 AWS configs × 2 SDK versions × 2 variants × several templates, that is a lot of CI time spent proving a queue is empty.

Suggest giving GetMessageFromDeadLetterQueue/GetMessageFromInvalidChannel an explicit timeout parameter so the budget lives in one place, dropping the outer loop (or adding a sleep), and using a much shorter budget for negative arms.

10. RocketMQ compose memory bumped 16× — docker-compose-rocketmq.yaml:35,57

128m→2g broker and 64m→1g proxy. Plausibly necessary, but 3g+ of JVM heap alongside the nameserver is a real constraint on 7 GB GitHub-hosted runners — worth confirming the smallest value that actually works rather than landing on round numbers.

[claude]

Review — #4297 — part 2/2

Consistency / design

11. MQTT tracing hardcodes InstrumentationOptions.All — MQTTMessageProducer.cs:133,166

Every other gateway threads a configured value (_instrumentation / _options / instrumentation), typically a ctor parameter defaulting to All — see RedisMessageProducer.cs:59. MQTT is the only one that hardcodes it, so InstrumentationOptions.RequestBody is permanently on and the full message body is written into the span (BrighterTracer.cs:1054) with no way for an operator to turn it off. An optional ctor parameter defaulting to All matches the surrounding pattern and costs nothing.

12. PR description omits the MQTT src changes

"Transport src changes (deliberately localized)" lists RocketMQ, GcpPubSub and AWSSQS, but MQTTMessageConsumer.Receive was rewritten and MQTTMessageProducer gained tracing calls. Per CLAUDE.md's Change Scope rule these should at minimum be listed; the producer tracing in particular looks beyond the reject/dlq/requeue scope.

13. Receive drains unboundedly and documents the failure onto callers — MQTTMessageConsumer.cs:203-208, 231-235

The XML doc acknowledges the Channel throws InvalidOperationException if more messages come back than its buffer holds. ChannelFactory.cs:67 already has subscription.BufferSize in hand — plumbing it into MqttMessagingGatewayConsumerConfiguration and bounding the drain turns a documented crash into a non-event.

14. Duplicated TEMPLATE_FR_COLUMNS map

When_generating_everywhere_...:47-62 duplicates MessagingGatewayGenerator.CANONICAL_TEMPLATE_FR_COLUMNS verbatim. If the generator's map changes, the test's copy silently diverges and starts checking the wrong set — the exact failure the test exists to prevent. Make the generator's map public static and reference it.

15. Model mutation leaks across renders — MessagingGatewayGenerator.cs:265-285

SetCanonicalSkip mutates the shared MessagingGatewayConfiguration. The Shared folder generation call (:122-127, :161-166) passes no prepareModel, so Skip carries the last Proactor template's value into that render. Harmless today (RejectionMetadataKeys.cs.liquid does not reference Skip) but latent. Same shape: _ledger is a mutable instance field assigned inside GenerateAsync, not safe if the generator instance is ever reused concurrently — the primary-constructor ledger parameter plus a Lazy<> would avoid both.

16. FR-8 Pass for native-DLX transports overstates coverage

When StampsRejectionMetadata is false the metadata test degenerates to exactly the FR-4 assertion (message reached the DLQ). RMQ therefore records FR-8 Pass on a test that verified nothing FR-8-specific. A distinct cell value — N/A (native DLX) or similar — would keep the matrix honest.

17. Ledger parsing is brittle — ConformanceLedger.cs:140

Header detection is line.StartsWith('|') && line.Contains("FR-2") && line.Contains("FR-4"). "FR-2" is a prefix of FR-20/FR-22, and if the header ever changes shape the parse returns an empty dictionary → no skips anywhere (see finding 5). The audit's own parser anchors on "| Configuration |" (LedgerSkipCrossCheckAudit.cs:155), which is both stricter and already in the codebase — worth using the same anchor in both. Also ParseLedger breaks at the first non-| line, so only the first matching table is ever read.

Minor

• Name/constant mismatch — When_generating_everywhere_should_find_exactly_twenty_wired_configurations asserts EXPECTED_WIRED_CONFIGURATION_COUNT = 24. The PR body also says "20 wired configuration rows"; there are 24.
• SnsHarnessMessageScheduler.Dispose can leak a producer — :155-187. Dispose holds _lock while clearing _producers; a timer callback that already fired blocks on _lock, then adds its producer to the now-cleared list, so that SNS client is never disposed (and publishes after teardown). Timer.Dispose() does not wait for in-flight callbacks — a _disposed flag checked under the lock in Republish fixes it. Also Schedule returns an id it never records and Cancel silently succeeds; NotSupportedException would fail loudly if a future test relies on it.
• RejectionTimestamp assertion is one-sided — ..._should_include_metadata.cs.liquid:106. UtcNow - parsed < 1 min passes for any future timestamp. Assert.InRange catches clock-skew and wrong-field bugs.
• ExtractSkipValues uses Match, not Matches — GatewaySkipConventionAudit.cs:134. Two Skip attributes on one line and the second is invisible to the audit.
• Negative delay arm is weakly bounded — When_sending_a_delayed_message_...:61-62. A single Receive(2000ms) against a 5 s delay only proves "nothing in the first 2 s" if the gateway blocks for the full window; a gateway whose Receive returns immediately makes the arm vacuous. Looping the negative arm and asserting MT_NONE each iteration until 2 s elapses would be robust across transports — precisely the class of bug the GCP fix in this PR addresses.
• Retry-loop duplication — the same 8-line Stopwatch + Receive(500) + break block appears ~4× per template across 24 templates and hundreds of generated files. A ReceiveUntil(channel, ceiling) helper in the non-generated harness would shrink the templates and let the polling policy be tuned in one place.
• GetAwaiter().GetResult() in the sync provider paths (e.g. SqsStandardMessageGatewayProvider.cs:282) — the codebase has BrighterAsyncContext.Run for exactly this, and it is used in the src changes in this same PR.

Worth calling out

• The SnsMessageProducer.SendWithDelay fix (TimeSpan.Zero → delay, both V3 and V4) is a genuine user-facing bug — sync SendWithDelay silently dropped the delay on every SNS send. Independently shippable, and arguably should not have to wait on the conformance work to land.
• The RocketMQ Baggage guard and the GCP timeOut honouring are both real fixes surfaced by the conformance suite — a good sign it is doing its job.
• docker-compose-rmq.yaml now matches the rabbitmq-async-ci service image, good local/CI alignment. (rabbitmq-sync-ci still pins 3.13, so one compose file cannot serve both; probably fine, just noting.)
• The audit design deserves credit: non-vacuity assertions (FilesScanned > 0, ConformingSkipsFound > 0), synthetic-repo canaries, and routing both audits through one EnumerateGatewayArtifacts so they cannot desync. Findings 4/5/6 are about closing the remaining silent-fallback paths in that same spirit, not about the approach.

On the description

The status section reads as stale relative to the checked-in ledger: it says MQTT, RMQ.Sync and AzureServiceBus are "the only Unknown rows left", but conformance-status.md now has zero Unknown cells — those three are Fixed/Deferred. Since the terminal cleanup is explicitly gated on a zero-Unknown ledger, a reviewer cannot tell from the description whether that gate has now opened. Worth a refresh before this leaves draft.

[claude]

Review — PR #4297 (generated transport conformance suite) — part 1/2

Reviewed at c965a6b. I read all the hand-written code — tools/Paramore.Brighter.Test.Generator/**, the 24 canonical .liquid templates, the audit/cross-check tests, the six src gateway changes, the harness providers and the ledger — and sampled the generated output rather than all ~1,180 generated files.

Overall this is strong work. Replacing four coarse HasSupportTo* flags with a per-(configuration × behaviour) ledger, and having the audit re-derive the expected Skip from ConformanceLedger.GetSkip itself (LedgerSkipCrossCheckAudit.CheckCellAgreement), is a genuinely good design — the audit can't drift from the emitter because it is the emitter. The ledger prose explaining why each cell is Deferred (Redis destructive read, SQS FIFO rejecting DelaySeconds, SNS having no native delayed publish) is the kind of documentation that survives contact with the next maintainer. All three src fixes are real bugs the suite found, which is the suite justifying itself.

Findings roughly in priority order.

1. MQTT ReceiveAsync now blocks a thread pool thread for the full timeout

MQTTMessageConsumer.cs:218-221 adds a Thread.Sleep(10) spin to Receive, and ReceiveAsync (line 241) is still Task.FromResult(Receive(timeOut)). Before this PR Receive returned essentially immediately so the sync-over-async shim was cheap; now every proactor ReceiveAsync blocks synchronously for up to timeOut before the Task even exists — and MQTT.Tests/test-configuration.json sets ReceiveMessageTimeoutInMilliseconds: 5000. That's a real regression for anyone running the MQTT proactor pump, not just for tests.

Give ReceiveAsync a genuinely async wait (await Task.Delay(10, cancellationToken), or better a SemaphoreSlim/Channel<T> signalled by ApplicationMessageReceivedAsync so there's no poll at all) and keep Thread.Sleep on the sync path only. cancellationToken is also ignored entirely, so a cancelled pump waits out the whole window. Minor: prefer Stopwatch over DateTime.UtcNow + timeOut for the deadline.

2. A missing ledger file emits Skip = "" on every canonical test

MessagingGatewayGenerator only wires prepareModel when a ledger resolved (_ledger != null ? (Action<string, object>)SetCanonicalSkip : null), and LoadLedgerFromFileSystem() returns null silently when FindLedgerPath reaches the filesystem root. Then SetCanonicalSkip never runs, Skip stays null, and — per your own comment at MessagingGatewayGenerator.cs:250-252 — Fluid evaluates nil != empty as true, so {%- if Skip != empty -%} renders and every canonical test comes out as [Fact(Skip = "")]. xUnit skips on any non-null Skip, so a moved or renamed ledger turns the whole suite green-by-skipping: exactly the failure mode this PR sets out to eliminate.

GatewaySkipConventionAudit would catch the empty marker, which is good defence in depth, but the generator shouldn't lean on it. Two fixes: always pass prepareModel (its else-branch already sets Skip = string.Empty), and log an error or throw when FindLedgerPath returns null — a run that can't find the ledger driving it shouldn't succeed quietly.

3. A typo'd LedgerKey silently disables every deferral for a configuration

ConformanceLedger.GetSkip returns string.Empty when the row is missing (if (!_cells.TryGetValue(ledgerKey, out var row)) return string.Empty;). So if a test-configuration.json LedgerKey doesn't match a ledger row — typo, trailing space, or a row renamed without updating the config — expected is empty, the regenerated file has no Skip, and CheckCellAgreement passes. CrossCheck's Direction 1 doesn't help: it only checks an issue number appears somewhere in the ledger.

Worth asserting (a) every non-empty LedgerKey resolves to a parsed ledger row, and (b) every non-placeholder ledger row is claimed by some LedgerKey. Relatedly, ComputeSkip falls through to string.Empty for any value outside the four-word vocabulary — Fail, N/A, Blocked, or a typo like Passs all quietly mean "run the test". Since the vocabulary is fixed and documented, I'd throw on an unrecognised cell.

4. The documented "500 ms poll" doesn't exist in the DLQ/invalid-channel loops

Every reject template says // Assert — bounded retry loop: 500 ms poll, 30 s ceiling, but the loop has no pacing at all:

while (stopwatch.Elapsed < TimeSpan.FromSeconds(30))
{
    dlqMessage = _messageGatewayProvider.GetMessageFromDeadLetterQueue(_subscription);
    if (dlqMessage.Header.MessageType != MessageType.MT_NONE) break;
}

The 500 ms actually comes from each provider's helper (KafkaClassicMessageGatewayProvider.cs:329 — consumer.Receive(TimeSpan.FromMilliseconds(500))), so the pacing is a provider convention the template silently depends on. Where a provider returns immediately this is a 30-second hot spin — not hypothetical, since TryGetRejectionTopic returns Message.Empty synchronously when the DLQ/invalid routing key is unset, so the misconfiguration case burns a core for 30 s and then fails. The templates polling _channel.Receive(500ms) are fine (the blocking receive is the poll); it's the ~10 GetMessageFromDeadLetterQueue/GetMessageFromInvalidChannel loops that need an explicit Thread.Sleep(500) / await Task.Delay(500), which also makes the comment true.

5. GCP Expiration.None removes the client's default deadline

BuildPullCallSettings returns CallSettings.FromExpiration(Expiration.None) for a null timeout, commented as "preserving the prior default behaviour" — it doesn't. Previously the sync path passed no CallSettings at all, so the generated client's configured expiration applied; an explicit Expiration.None overrides that and leaves the Pull unbounded. Prefer null (sync) / CallSettings.FromCancellationToken(ct) (async) so client defaults survive.

Also: mapping any DeadlineExceeded to an empty receive swallows a genuine network stall — consider only doing so when timeOut was actually set; and a very small timeOut now yields a tight loop of RPCs that each deadline out, so a floor on the derived expiration is cheap insurance.

(continued in part 2)

[claude]

Review — PR #4297 — part 2/2

6. CI wall-clock

679 generated files carry a TimeSpan.FromSeconds(30) ceiling. Green runs exit on first hit so that's fine, but one transport regression turns a job into a long timeout parade — and the FR-2/FR-9 templates have an unavoidable floor (2 s negative arm + 5 s delay ≈ 7 s each, ×2 variants ×20 configurations ≈ 9–10 min of pure waiting on a fully green run). I'd make both numbers configuration-driven rather than hard-coded in templates; you already have the pattern with ReceiveMessageTimeoutInMilliseconds. [Fact(Timeout = …)] would also turn a hang into a clean failure rather than a stall.

7. Smaller items

• MQTTMessageProducer.cs:133,166 hard-codes InstrumentationOptions.All. Every other transport threads configured options through (_instrumentation, _options, instrumentation), so MQTT becomes the one gateway a user can't turn instrumentation down on.
• RocketMqMessageProducer.cs:87-91 — the Baggage guard is the right fix, but it's the third instance of the same bug class in one method: Source (line 84) and ContentType/DataContentType (113-114) are still unguarded .ToString() calls that could hand RocketMQ an empty property value. An AddPropertyIfNotEmpty helper closes the category rather than the instance. Separately HeaderNames.Type is added twice (lines 95 and 110) — pre-existing, but worth tidying while you're here.
• SnsHarnessMessageScheduler.Republish swallows everything with a bare catch { }. The comment is right that a broker error surfaces as the after-delay arm timing out — but that's a 30-second timeout with no diagnostic, the hardest failure to debug in CI. Storing the last exception and surfacing it in the failure message costs almost nothing. There's also a benign race: Dispose disposes producers under _lock while a timer callback can be mid-Send outside it.
• SnsHarnessMessageScheduler.Schedule returns a GUID never associated with the timer, so Cancel/ReScheduler cannot work even in principle. Fine for a harness and you say so, but returning silently rather than NotSupportedException means a future template that starts using it fails confusingly.
• The comment above private IAmAConformanceLedger? _ledger; describes a template→FR-column dictionary, not a ledger — it looks left behind when that map moved to CanonicalBehaviours.
• Mutable shared model: SetCanonicalSkip mutates the same MessagingGatewayConfiguration that serves as the Fluid model, and _ledger is instance state assigned inside GenerateAsync. Correct today only because rendering is strictly sequential. A per-template wrapper ({ Config, Skip }) removes the hazard. Note the Shared folder renders without prepareModel, so Skip leaks in from the last Proactor template — harmless only because RejectionMetadataKeys.cs.liquid doesn't reference it.
• {{ Skip }} is interpolated into a C# string literal unescaped. A " or \ in a ledger row key produces code that won't compile — an escape filter, or a charset assertion in the audit, would harden this.
• Test coverage: the two SnsMessageProducer.SendWithDelay fixes, the GCP timeout fix and the MQTT Receive change are covered only by broker-backed generated tests, and RocketMQ's CI job is commented out (ci.yml:687) so its nine Fixed cells rest on local runs alone. Given CLAUDE.md's TDD workflow, the one-line SendWithDelay delay-drop especially deserves a fast unit test asserting the sync overload forwards its argument — that's a regression that reappears the next time someone touches the overload set.

8. Docs / housekeeping

• .agent_instructions/generated_tests.md drops the three retired flags but never documents what replaced them: LedgerKey, the ledger, the cell vocabulary, or how to defer a behaviour. CLAUDE.md treats .agent_instructions/ as authoritative, so that's the first file a contributor reads.
• manual-test-plan.md (249 lines) sits at the repo root; it reads as spec-scoped material and would be better under specs/0036-universal-transport-conformance-tests/ or docs/.
• docs/adr/0070-…md is deleted rather than marked Superseded by 0066/0067. ADRs are normally immutable once accepted; deleting one loses the record of a decision actually taken, and 0066/0067 lose their supersedes backlink.
• The PR body is stale and will mislead reviewers: it lists MQTT / RMQ.Sync / AzureServiceBus as remaining Unknown rows, but the ledger has zero Unknown cells and all three are filled (MQTT and RMQ.Sync are wired with a test-configuration.json + providers; ASB is a fully-Deferred placeholder row). It also lists removal of the three MessagingGatewayConfiguration properties and the gate keys as still pending, but both are already done in this diff, and the Kafka summary omits the Kafka / Consumer row.
• Every Deferred cell points at #4240. You flag this as a placeholder — just noting the consequence: until real per-behaviour issues land, CrossCheck's Direction 1 ("every Skip issue number appears in a Deferred cell") is tautological, and the emitted (maintainer sign-off) suffix carries no actual handle even though the ledger cell has a sign-off: @… field to draw from.
• Test.Generator.Tests/AssemblyInfo.cs disables parallelisation assembly-wide; only the tree-scanning audits need it, so a [Collection] would keep the rest parallel.

Nothing here challenges the design — the ledger-plus-audit approach is the right call. Items 1, 2 and 3 from part 1 are the ones I'd want fixed before merge. Happy to look again once this is out of draft.

[claude]

Review — PR #4297 (WIP): Generated transport conformance tests (1/2)

Reviewed the hand-written surface: the three src/ gateway fixes, the generator + ledger + audit machinery under tools/, the canonical Liquid templates, and the new harness types. I skimmed rather than read all ~65k lines of Generated/ copies, since those are derived from the templates. I could not build or run anything in this environment, so everything below is from reading.

Overall this is strong work. The ledger + cross-check audit design is genuinely good — in particular the non-vacuity assertions in When_a_skip_has_no_matching_deferred_ledger_row_should_fail_audit.cs:175-187 (assert the parser actually found rows, Deferred cells and Skip markers before asserting zero violations) are exactly the guard most "audit the tree" tests forget, and CanonicalBehaviours as a single shared source of truth for the template→FR-column map is the right call for the reason its doc comment gives. The three src/ fixes all look correct and appropriately narrow.

---

1. The two-message canonical templates cannot distinguish M1 from M2 — the assertion that matters is vacuous

DefaultMessageBuilder assigns _messageId = Id.Random() once per builder instance (DefaultMessageBuilder.cs:58) and reuses the same _body (:254). The two-message templates call Build() twice on the same builder:

var message1 = _messageBuilder.SetTopic(_publication.Topic!).Build();
var message2 = _messageBuilder.SetTopic(_publication.Topic!).Build();

so M1 and M2 have an identical MessageId and an identical body. FifoMetadataProducer.StampFifoMetadata already documents this ("the canonical suite reuses one message builder, so its two 'distinct' messages share an id and body", FifoMetadataProducer.cs:78-81) but works around only the FIFO-dedup symptom, not the assertion gap.

• FR-7 (When_rejecting_message_with_no_channels_configured_should_acknowledge_and_log): the point is M1 was acknowledged and is not redelivered. The test only asserts received2.Header.MessageType != MT_NONE (:80). A gateway that leaves M1 on the queue and redelivers it passes.
• FR-16 (When_nacking_a_message_it_should_be_redelivered): the comment at :106 claims "M1 redelivered before M2", but _messageAssertion.Assert(message1, redelivered) compares MessageId — which M2 also satisfies. The ordering claim is untested, and :135 does not check identity at all.

Both are marked Pass for most configurations in the ledger, so this signs off on behaviour the tests cannot observe. Suggested fix: give each message a distinct id in the templates (fresh builder, or an explicit SetMessageId(Id.Random())), then assert Assert.Equal(message2.Id, received2.Id) for FR-7 and Assert.NotEqual(received1.Id, received2.Id) for FR-16. With distinct ids, FifoMetadataProducer's dedup stamp may become unnecessary too.

2. MQTTMessageConsumer.ReceiveAsync now blocks a thread pool thread for the full timeout

ReceiveAsync is Task.FromResult(Receive(timeOut)) (MQTTMessageConsumer.cs:241-244), and Receive now spins Thread.Sleep(10) until the deadline (:218-221). Before this PR Receive returned immediately, so ReceiveAsync was effectively non-blocking; now every proactor poll parks a thread pool thread for up to timeOut, and the cancellationToken is never observed — shutdown waits out the full window per in-flight receive.

Consider a real async implementation (await Task.Delay(10, cancellationToken), or better: signal arrival from the ApplicationMessageReceivedAsync handler via a SemaphoreSlim / Channel<Message> so neither path polls). Also Stopwatch rather than DateTime.UtcNow for the deadline — the latter is not monotonic.

Separately the drain loop at :232-235 is now unbounded, and the XML remark you added at :203-209 documents the resulting hazard (Channel throws InvalidOperationException if more messages come back than its buffer holds). Capping the drain at the consumer's own batch size would be more robust than documenting it.

3. MQTTMessageProducer hardcodes InstrumentationOptions.All — message bodies always go to telemetry

MQTTMessageProducer.cs:133 and :166:

BrighterTracer.WriteProducerEvent(Span, "mqtt", message, InstrumentationOptions.All);

All includes RequestBody (InstrumentationOptions.cs:43), and WriteProducerEvent writes the full body as a tag when that flag is set (BrighterTracer.cs:1054-1055). Every other transport threads a configured value through (SqsMessageProducer.cs:160, RmqMessageProducer.cs:197, KafkaMessageProducer.cs:293, GcpMessageProducer.cs:58). MQTT would be the only one opting every user into full payload capture regardless of configuration — a privacy exposure as well as an inconsistency. The producer has no instrumentation field today; a defaulted constructor parameter threaded from the factory would fix it.

Note this also newly propagates trace context into the message headers via PropogateContext (BrighterTracer.cs:1067) — a behaviour change for MQTT users beyond what the PR description covers.

4. GCP: Expiration.None on the null-timeout path is not the prior default

BuildPullCallSettings (GcpPullMessageConsumer.cs:194-197) returns CallSettings.FromExpiration(Expiration.None) when timeOut is null, commented as preserving prior behaviour. Previously no CallSettings was supplied at all, so the client's own default expiration for Pull applied; now the deadline is explicitly removed. The sync Receive overload has no cancellation token either, so a wedged Pull can block the calling thread indefinitely where it previously would have surfaced a deadline. Returning null (let client defaults stand) matches the comment's intent.

The <param name="timeOut"> doc on the sync overload also still reads "not strictly used by the underlying Google Pub/Sub client" (:203) — stale now.

(continued in next comment)

[claude]

Review — PR #4297 (WIP) (2/2)

5. The ledger fails open, silently, in several places

ConformanceLedger returns string.Empty (i.e. test runs, no Skip) for an unknown ledgerKey (:86), an unknown frColumn (:87), and any cell value that is not Pass/Fixed/Unknown/Deferred -> (:115) — a typo like Deferred #4240 (missing ->) falls through there. And MessagingGatewayGenerator.LoadLedgerFromFileSystem returns null when the file is not found, disabling the Skip mechanism for the whole run with no log line (MessagingGatewayGenerator.cs:222-226); since AppContext.BaseDirectory is the search root, that triggers whenever the generator runs from outside the repo tree.

Given ADR 0067's "no silent skips" stance, the symmetric guarantee ("no silently ungoverned cell") seems worth having: throw on an unrecognised cell value, and log at Warning/Error when the ledger cannot be located, so ledger drift is a build failure rather than a quiet no-op.

Related, ParseLedger finds the matrix header with line.StartsWith('|') && line.Contains("FR-2") && line.Contains("FR-4") (:140). "FR-2" is a substring of FR-20/FR-22, so any earlier pipe-prefixed line mentioning FR-22 and an FR-4x wins instead. It works against today's conformance-status.md (header at line 326, nothing earlier matches), but a summary table added above it would silently break generation. Anchoring on | Configuration | — which the audit's own error message at :177 already assumes — is sturdier. SplitTableRow also does not handle escaped \|, and duplicate row keys silently overwrite at :169.

6. DLQ polling loops hot-spin

Every DLQ / invalid-channel template does:

while (stopwatch.Elapsed < TimeSpan.FromSeconds(30))
{
    dlqMessage = _messageGatewayProvider.GetMessageFromDeadLetterQueue(_subscription);
    if (dlqMessage.Header.MessageType != MessageType.MT_NONE) break;
}

with no delay between iterations. Whether that is a tight loop depends on the provider: KafkaClassicMessageGatewayProvider.GetMessageFromDeadLetterQueue returns Message.Empty immediately when there is no DLQ routing key (:259-260), so it hammers for a full 30 s and then fails with an opaque MT_NONE assertion; RedisMessageGatewayProvider's blocks internally for up to ~60 s (:309-316), so the "30 s ceiling" is not the real bound there either. A Thread.Sleep(500) in the loop — matching the Receive(500ms) cadence the other loops get for free — would make the ceiling mean the same thing everywhere.

7. Harness schedulers: silent swallow, and a disposal race

KafkaHarnessMessageScheduler (and the Rmq/Redis/MsSql/Sns/Gcp/Mqtt siblings, which look structurally identical):

• Republish's catch { } (:140-144) discards everything with no logging. The comment notes a broker error surfaces as the after-delay arm timing out — true, but that turns "the harness producer failed to connect" into "FR-2 is non-conformant", exactly the misattribution the ledger exists to prevent. Even a Console.WriteLine would help when adjudicating a Deferred row.
• Dispose holds _lock while disposing and clearing _registries (:147-179), but a timer callback already inside Republish blocks on the same lock at :132, then adds its registry to the freshly-cleared list — leaking a live producer. Timer.Dispose() also does not wait for in-flight callbacks; Dispose(WaitHandle) or DisposeAsync() would.
• Schedule returns a GUID that is never recorded and Cancel(string) is a no-op reporting success. Documented as unexercised, but a future test calling Cancel gets a silent lie — NotSupportedException would be safer.

8. Smaller things

• docker-compose-rocketmq.yaml: broker heap 128m → 2g and proxy 64m → 1g, so >3 GB committed heap across two containers plus the nameserver. That is a lot for a 7 GB GitHub-hosted runner running other services alongside, and it is imposed on local runs too. Was the jump measured, or is there a smaller value that clears the failures?
• Hardcoded timings in templates: FromSeconds(5) / FromMilliseconds(2000) / FromSeconds(30) / FromMilliseconds(500) are literals while ReceiveMessageTimeoutInMilliseconds is templated. The 2 s negative window against a 5 s delay (When_requeuing_a_failed_message_with_delay...:68) is the likeliest flake source on a loaded runner and cannot be tuned per transport. Consider promoting these to MessagingGatewayConfiguration.
• DefaultMessageAssertion on redelivered messages: asserts expected.Header.HandledCount == actual.Header.HandledCount (:59) and actual.Header.Delayed == TimeSpan.Zero (:60), both against the original message in the FR-2/FR-15/FR-16 templates. A transport that increments HandledCount or stamps Delayed on redelivery fails for a reason unrelated to the behaviour under test. Latent rather than live given the ledger, but it will bite on the next onboarding.
• MessagingGatewayGenerator.cs:53-56: the comment above private IAmAConformanceLedger? _ledger still describes the template→FR-column dictionary that moved to CanonicalBehaviours. Also SetCanonicalSkip mutates the shared MessagingGatewayConfiguration between renders — safe today because rendering is sequential, but Skip is a per-render value on a JSON-deserialised config object, which is a trap if GenerateAsync is ever parallelised.
• RetryableChannelSync/Async: the timeout-bounding rewrite is a clear improvement, but if inner.Receive returns MT_NONE without consuming its timeout the loop spins with no delay until the budget is exhausted. Kafka's consumer blocks, so theoretical today.
• GatewaySkipConventionAudit.ExtractSkipValues takes only the first SKIP_EXTRACTOR.Match per line (:134); a second Skip = "..." on the same line is invisible. Cheap to make Matches.
• PR description is stale on the terminal cleanup: it lists removing the three MessagingGatewayConfiguration properties and the three gate keys from every test-configuration.json as remaining, but the diff already removes them — no residual references anywhere in tests/ or tools/ outside the two audit tests asserting their absence. Worth updating so reviewers know what is actually left.
• ADR 0070 deleted rather than superseded: only Proposed, so low stakes, but docs/adr/index.md opens by saying identity is the filename stem — deleting the file breaks any link to it. Rejected / Superseded by 0066, 0067 preserves the trail.

---

On the TDD workflow

CLAUDE.md makes /test-first mandatory for TEST tasks with a per-test approval gate. With 62 tasks and ~6,200 lines of new generator tests it is hard to tell from the diff whether each canonical behaviour's test preceded its template — the ralph-tasks flow may well cover this, but it would help for the PR body to say which route was taken.

Answering the specific questions

• Correctness: finding 1 is the one I would want fixed before this leaves draft — it undermines two of the eleven canonical behaviours across all 20 configurations. Findings 2–4 are behaviour changes in shipped transports.
• Performance: findings 6 and 7 plus the RocketMQ heap bump are test-infrastructure cost. Finding 2 is a runtime thread-pool cost for MQTT proactor users.
• Security: finding 3 (unconditional message-body capture in MQTT traces) is the only one.
• Test coverage: the audit layer is well built — failing-case tests plus a live-tree fact with non-vacuity guards is a good pattern. The gaps are finding 1 and the fail-open paths in finding 5.

Nice work overall — the ledger-as-source-of-truth approach is a much better answer than per-transport capability flags.

[claude]

Review — #4297 (generated transport conformance tests) — part 1/3: src changes

Reviewed the hand-written surface: the six src/ files, the generator/ledger/audit code under tools/ and tests/Paramore.Brighter.Test.Generator.Tests/, the canonical templates, the harness providers, and the ledger. I spot-checked the generated output rather than reading all 72 copies.

The shape of this is good — a single canonical suite with per-cell deferral beats four opaque boolean gates, and the two-direction cross-check plus a synthetic-tree test for each failure mode is more than most conformance harnesses get. The RetryableChannel* rework (bounding the re-poll to the caller's timeout) is a real correctness improvement, and the four src bug fixes are all legitimate finds.

---

1. MQTTMessageConsumer.ReceiveAsync now blocks a thread pool thread — MQTTMessageConsumer.cs:210-244

Receive gained a Thread.Sleep(10) poll loop, and ReceiveAsync is Task.FromResult(Receive(timeOut)). On an idle subscription every Proactor pump iteration now occupies a thread pool thread for the whole window rather than returning immediately. cancellationToken is not observed at all, so shutdown is delayed by up to timeOut per consumer.

The queue is fed from an event handler, so this wants a signal rather than a poll: a SemaphoreSlim/Channel<Message> released in ApplicationMessageReceivedAsync, with ReceiveAsync doing await sem.WaitAsync(timeOut, cancellationToken) and Receive the sync twin. Failing that, at minimum make ReceiveAsync a real async method that await Task.Delay(10, cancellationToken)s.

2. Unbounded drain in the same method — MQTTMessageConsumer.cs:231-235

The <remarks> block documents that the Channel wrapper throws InvalidOperationException when more messages come back than BufferSize holds, and then the code drains the whole queue anyway. Concretely: the MQTT provider sets BufferSize = 5; a publisher bursts 20 messages during the 300 ms window; the next Receive returns 20 and the pump dies on enqueue. Cap the drain rather than documenting the hazard.

Minor: DateTime.UtcNow for the deadline is sensitive to a clock step — Stopwatch (as used in the templates) is the safer idiom.

3. Hardcoded InstrumentationOptions.All in the MQTT producer — MQTTMessageProducer.cs:133,166

Every other gateway threads a configured value (see RmqMessageProducer.cs:197, KafkaMessageProducer.cs:293, PostgresMessageProducer.cs:58). MQTT is now the only one that hardcodes.

That matters because All includes RequestBody, and BrighterTracer.WriteProducerEvent (BrighterTracer.cs:1054) writes message.Body.Value — the full payload — into the span. Every MQTT publish will export its message body to the trace backend with no operator opt-out. Take InstrumentationOptions on the constructor like the peers do.

4. Expiration.None does not preserve the prior default — GcpPullMessageConsumer.cs:194-197

The comment says a null/non-positive timeout "leaves the call unbounded, preserving the prior default behaviour", but CallSettings.FromExpiration(Expiration.None) explicitly overrides the client's configured PullSettings expiration rather than inheriting it. Returning null CallSettings for that branch would actually preserve the previous behaviour. Small, but the comment currently asserts something the code does not do.

Also: with Expiration.None, a DeadlineExceeded can only originate server- or channel-side, and the new catch now reports it as a normal empty receive.

5. RocketMqMessageProducer baggage guard is right but narrow — RocketMqMessageProducer.cs:87-91

Good catch on the crash. If AddProperty rejects empty values, though, SpecVersion (line 85) and ContentType/DataContentType (lines 113-114) are the same bug waiting for the right message. A small AddIfNotEmpty(builder, key, value) applied across the block would close the class rather than the instance. (Unrelated, pre-existing: HeaderNames.Type is added twice, lines 93-96 and 108-111.)

6. RetryableChannelSync/Async can hot-spin — RetryableChannelSync.cs:50-67

Bounding to the caller's timeout is the right fix. But if inner.Receive returns MT_NONE immediately — broker down, or remaining down to a millisecond — this spins for the full timeout, where the old code made at most six calls. A floor (if (remaining < TimeSpan.FromMilliseconds(50)) break;) would keep the new semantics without the spin.

[claude]

Review — #4297 — part 2/3: ledger, audits, ledger accuracy

The load-bearing part

The stated invariant is "no silent skips". The audits enforce that direction well. The inverse — silently losing a skip, or silently gating nothing — has three open paths:

7. ComputeSkip falls through to "no Skip" on an unrecognised cell — ConformanceLedger.cs:115

Deferred #4240 (no arrow), a unicode arrow, or Defered all hit the final return string.Empty and generate an unskipped test. And because LedgerSkipCrossCheckAudit.CheckCellAgreement derives its expectation from this same method, the audit agrees with the typo — the "cannot drift from what generation produces" property is exactly what prevents it from catching this. Throwing on a value outside the documented vocabulary would close it.

8. Two silent "no ledger at all" paths — ConformanceLedger.cs:147 and MessagingGatewayGenerator.cs:233

ParseLedger returns an empty dictionary when the header row is not matched; LoadLedgerFromFileSystem returns null when the file is not found (and GenerateAsync then passes no prepareModel). Either one makes a full regeneration emit zero Skips tree-wide. For a mechanism whose entire purpose is auditable deferral, both should be hard failures.

9. An unknown LedgerKey silently ungates a whole configuration — ConformanceLedger.cs:86

GetSkip returns empty when the row key is not in the ledger. A typo in a test-configuration.json ("AWS/SqsFifo" vs "AWS / SqsFifo") therefore makes every behaviour for that configuration expect no Skip, in both the generator and the audit. LedgerSkipCrossCheckAudit.cs:352 already enumerates the declared keys — asserting each one resolves to a ledger row would be a cheap fix.

10. Two ledger parsers with different header heuristics

ConformanceLedger.ParseLedger:140 matches Contains("FR-2") && Contains("FR-4"); LedgerSkipCrossCheckAudit.ParseLedger:188 matches "| Configuration |". CanonicalBehaviours's own doc-comment makes the argument against exactly this ("two copies could drift and the audit would then be checking the wrong cell") — it applies here too. Also Contains("FR-2") matches FR-22.

11. Line-based Skip extraction — GatewaySkipConventionAudit.cs:129-144

SKIP_EXTRACTOR matches once per line. This repo runs csharpier, and the Skip strings are long enough that a future formatter setting could wrap or split them — and the audit would see nothing and pass. A cheap guard: assert that the count of [Fact(Skip occurrences in a file equals the count of extracted values.

---

Ledger accuracy

12. Fixed (#4240) cells that no change in this PR fixed — conformance-status.md:348-351

The ledger's own vocabulary says Fixed (#PR/commit) means "Conformed via an in-spec gateway fix linked to the PR or commit". But the entire src diff is six files / 75 lines: SNS delay x2, GCP pull timeout, MQTT Receive + tracer, RocketMQ baggage. Against that:

• MQTT carries 10 Fixed (#4240) cells including FR-4 through FR-8 and FR-22. MqttMessageConsumer.Reject, ResolveRejectionProducer and Requeue are all already on master (git show master:src/.../MQTTMessageConsumer.cs puts them at lines 229, 280, 316); this PR only touched Receive and the doc comments.
• RMQ.Sync carries 10 Fixed (#4240) cells and has no src change in this PR at all — it is a new onboarding, not a fix.
• RocketMQ carries Fixed (#4240) for FR-4 through FR-9, FR-16, FR-17, FR-22; only the baggage guard is plausibly load-bearing there, and only for the send path.

These read as Pass by the ledger's own definitions. Separately, #4240 is the tracking issue, not a PR or commit, so Fixed (#4240) does not satisfy the Fixed (#PR/commit) form either. This matters because Fixed vs Pass is precisely the distinction the ledger exists to record, and neither audit can tell them apart (both render no Skip) — so nothing but review catches it.

[claude]

Review — #4297 — part 3/3: test coverage, housekeeping, nits

Test coverage

13. No direct regression test for any of the four src fixes.

CLAUDE.md makes /test-first mandatory for behavioural change. All four fixes are covered only by the broker-requiring generated suite, and for RocketMQ / MQTT / GCP those cells are Deferred or emulator-only, i.e. not run in CI.

The SNS one is the clearest gap: SendWithDelay dropping its delay argument is a one-line bug that existed independently in two copies of the file, which is a strong argument that it needs a guard. A fake IAmAMessageSchedulerSync asserting the scheduler receives the requested delay rather than TimeSpan.Zero is a few lines and needs no broker. The MQTT Receive rewrite is the largest behavioural change in the PR and has no direct coverage at all.

14. Doubly-nested retry inflates failure cost — MsSqlMessageGatewayProvider.cs:228-248

The provider already retries internally for up to ~60 s (10 x (5 s receive + 1 s sleep)), and the canonical template wraps that in a further 30 s stopwatch loop that constructs a fresh MsSqlMessageConsumer each outer iteration. A single failing DLQ assertion therefore costs at least 60 s, and the outer loop is effectively dead (the inner one always outlives it). Pick one layer. Also messages.First() throws if a gateway ever returns an empty array.

15. Culture-sensitive ISO-8601 assertion — When_rejecting_message_should_include_metadata.cs.liquid:104-107

DateTimeOffset.TryParse(timestampValue, out ...) uses the ambient culture with no DateTimeStyles. For an assertion whose message is literally "must be parseable ISO-8601", pass CultureInfo.InvariantCulture and DateTimeStyles.RoundtripKind. The freshness check also has no lower bound, so a timestamp arbitrarily far in the future passes.

---

Housekeeping

16. manual-test-plan.md (repo root, 249 lines) is stale. It documents the Kafka DLQ hooks as return Message.Empty; stubs and CreateProducer as wiring no scheduler — but KafkaClassicMessageGatewayProvider.cs:257 now implements the DLQ read for real and the ledger records Kafka 11/11 Pass. As a root-level file it will be the first thing a contributor finds and the wrongest. Move it under specs/0036-.../ as a dated historical record, or drop it.

17. ADR 0070 is deleted rather than superseded — docs/adr/index.md. The index preamble treats the filename stem as stable identity; deleting the file breaks any external reference and loses the record of why the earlier design was abandoned. Status: Superseded by 0066, 0067 is the usual ADR move, and it is more useful to the next person than absence.

18. specs/README.md drops the old 0036 row without adding one for 0036-universal-transport-conformance-tests, so the spec index no longer lists this spec at all.

19. docker-compose-rmq.yaml:5 — rabbitmq:management becomes brightercommand/rabbitmq:4.2-management-delay. Two changes in one: the delayed-message plugin (clearly needed) and a RabbitMQ major-version move to 4.2 across every RMQ suite (less obviously). Where does that image's Dockerfile live, and is the tag immutable? A one-line comment in the compose file pointing at the source would help — a custom image on the test path is otherwise unauditable from the repo.

20. docker-compose-rocketmq.yaml:35,57 — broker heap 128m to 2g and proxy 64m to 1g, so around 3 GB of JVM heap before the nameserver and everything else. On a standard 7 GB GitHub runner that is close to the edge. Was the bump measured against an actual OOM, or is something smaller sufficient?

21. .agent_instructions/generated_tests.md documents the retired gates and the new HasSupportToDetectMissingInfrastructureOnAssume, but not LedgerKey — which is now the single key a contributor must set for a new configuration to be ledger-gated at all (omit it and it is silently ungated, per #9). The ledger workflow for onboarding a transport is not documented anywhere outside the ADRs.

22. Stale comment and duplicated ledger state — MessagingGatewayGenerator.cs:52-56. The "Canonical-template base name to conformance-ledger FR column ... This list is authoritative" block now sits above private IAmAConformanceLedger? _ledger;; it belonged to the dictionary that moved to CanonicalBehaviours. The class also holds both a primary-constructor ledger parameter and a mutable _ledger field assigned inside GenerateAsync — resolving once in the constructor would remove the two-state and the null-check at each use site.

---

Nits

• SnsHarnessMessageScheduler.Republish:148 swallows every exception, so an FR-9 failure surfaces only as "message never arrived" with the actual broker error discarded. Capturing it and surfacing it in the timeout would make these tests much cheaper to diagnose.
• SnsHarnessMessageScheduler.Schedule:76 mints an id it never records, so Cancel(id) cannot work. Documented as out of scope — worth a NotSupportedException instead of a silent no-op so a future caller finds out immediately.
• specs/.current-spec has no trailing newline.

---

Nothing here blocks the direction. #7/#8/#9 and #12 are the ones I would want addressed before merge, since they undercut the guarantee the whole ledger mechanism is built to provide, and #1/#2/#3 are shipping behaviour rather than test scaffolding.

Verified while reviewing: no #NNNN placeholders remain in any generated Skip or in the ledger; the matrix is 24 rows x 11 columns = 264 cells with no Unknown; and no Generated/ file carries a non-conforming Skip value. Those claims in the PR description hold.