[iancooper]

When an Inbox receives a duplicate message, we may choose to replay any outgoing messages that it triggered when it was originally called. This makes us idempotent, in that our behavior matches that of the original call. It also allows us to recreate messages for downstream consumers who might have failed, by triggering from upstream, thus allowing us to replay a whole workflow until we reach the point where it failed.

Closes #2541

[claude]

Code Review — PR #4067: Replay Outgoing Messages When Inbox Receives Duplicate

This PR contains design and specification documents only — no implementation code yet. The changes are: PROMPT.md (root), docs/adr/0057-replay-outbox-on-inbox-duplicate.md, and 12 spec files under specs/0027-replay-matching-outbox-events-when-inbox-has-already-seen/.

---

🔴 Critical: PROMPT.md should not be committed

PROMPT.md at the repo root is an AI session state artifact — it tracks which round of ADR review was completed, which spec phases are approved, and what "next step" the AI assistant should take. This is an internal working document for the development workflow tool, not project documentation.

Recommendation: Remove PROMPT.md from this PR and add it to .gitignore. Its content is already captured in the spec files and ADR that are correctly committed.

---

✅ ADR Quality

docs/adr/0057-replay-outbox-on-inbox-duplicate.md is thorough and follows the project's ADR conventions well:

• Problem statement is clear: no mechanism to replay a failed workflow from an upstream trigger
• Decision rationale is solid — using CausationId as a scoped, single-invocation concept (vs. CorrelationId for request-reply, vs. future WorkflowId) avoids conceptual collision
• The SupportsCausationTracking() runtime schema check is a correct defensive choice — fail fast at startup rather than at runtime
• The tidy-first prerequisite list (tasks 1–4) correctly separates structural refactors from behavioral changes per project conventions

---

Design Observations

Interface duplication on SupportsCausationTracking()

Both IAmACausationTrackingInbox and IAmACausationTrackingOutbox expose both sync and async variants:

bool SupportsCausationTracking();
Task<bool> SupportsCausationTrackingAsync(CancellationToken cancellationToken = default);

This is a schema check that typically only needs to run once at startup (or can be cached). Consider whether the async variant adds real value, or if a single sync check (cached after first call) is sufficient. Most schema-existence checks against relational databases complete in microseconds on a live connection. The async overload adds interface surface that every implementing store (all 18 of them) must provide.

ReplayCausation args parameter

void ReplayCausation(string causationId, RequestContext? requestContext,
    Dictionary<string, object>? args = null);

The args dictionary follows the existing Brighter convention for extensibility, which is fine. However, the ADR does not document what keys/values are valid. Adding a brief note about expected args (or confirming none are currently used) would help implementors.

UseInboxHandler prerequisite: switching from InitRequestContext() to this.Context

The ADR correctly flags this as a tidy-first change (Task 1), but notes it "affects the existing Throw and Warn paths as well." This is a subtle behavioral change: existing tests may have been written against the old private context. The task spec calls this out with "needs focused regression tests" — make sure the existing When_Handling_A_Command_Once_Only_With_Throw_Enabled and Warn tests still pass after the refactor, not just after the full implementation.

Describe() global inbox injection

Task 3 adds global inbox attributes to Describe() output to match Build(). The ADR notes this is a shared infrastructure change that "should be treated as a tidy-first structural change with its own focused test coverage." This is the right call — the existing pipeline validation test suite should be run as a regression gate before any behavioral tasks proceed.

Relay vs. Replay enum name

The ADR (round 4 finding #3) notes that the requirements use "e.g." so Replay is not mandated. The name Replay is clear and consistent with the feature concept — this is fine as-is.

---

Spec Completeness

The tasks.md (23 tasks) is very well structured with clear TDD commands, file locations, and dependency ordering. A few observations:

• Task 4 (expose Outbox from IAmAnOutboxProducerMediator) is a public API surface change. The ADR should explicitly document this interface change as a breaking/additive decision, not just as an implementation prerequisite.
• Tasks 19–22 (all 18 persistent stores) are large in scope. The task list correctly gates these behind base test classes (Task 18), but there should be an explicit note about whether the persistent store migration SQL is expected in this PR or arrives separately. The ADR says "a separate PR provides migration tooling" — the tasks.md should reference that dependency.
• Task 16 (add telemetry to existing Throw/Warn/Add paths) is labeled a "tidy improvement" but is included in the same tasks.md. Consider whether this should be a separate spec/PR to keep scope clear.

---

Summary

Area	Status
PROMPT.md in repo root	❌ Should be removed
ADR quality and completeness	✅ Well-written
Interface design	⚠️ SupportsCausationTracking async variant worth revisiting
Tidy-first discipline	✅ Correctly scoped
Task list TDD discipline	✅ Follows project /test-first conventions
Scope creep (Task 16)	⚠️ Consider separating telemetry tidy from feature work
Persistent store migration dependency	⚠️ Needs explicit cross-PR reference in tasks.md

The core design is sound. The main ask is removing PROMPT.md before merge; the interface and scope observations are lower priority suggestions.

[claude]

Code Review — PR #4067: Replay Outgoing Messages When Inbox Receives Duplicate

Overview

This PR is a specification and design phase PR only — it contains requirements, ADR 0057, and task definitions for the Replay-on-Inbox-Duplicate feature. No production code has been written yet (the PROMPT.md explicitly states "Implementation — not started"). The 1606 additions are almost entirely documentation and spec artefacts.

---

Issues

🔴 PROMPT.md Should Not Be Merged

PROMPT.md is a Claude Code session-resume artefact containing internal workflow state (task review history, implementation notes). It is simultaneously committed and gitignored in this PR, which means it will persist in the repository's git history even after being ignored for future commits.

Recommendation: Remove PROMPT.md from the branch before merging. The .gitignore entry is the right call, but the file itself should be deleted so it does not land in master's history.

🟡 README.md for Spec 0027 Shows Tasks as Incomplete

specs/0027-.../README.md shows:

- [ ] Tasks (`tasks.md`)

...but .tasks-approved is committed in the same PR, meaning tasks are approved.

Recommendation: Update the README checkbox to [x] Tasks.

🟡 Inconsistent specs/.current-spec → specs/.current_spec Rename

The PR deletes specs/.current-spec (hyphen) and updates specs/.current_spec (underscore). This is a housekeeping fix but worth a brief mention in the PR description so reviewers understand the intent.

---

ADR 0057 Quality

The ADR is thorough and well-reasoned. Specific strengths:

• Non-breaking opt-in design via role interfaces (IAmACausationTrackingInbox, IAmACausationTrackingOutbox) is the right approach. Third-party implementations continue to work unchanged.
• Pipeline validation at startup using Specification<HandlerPipelineDescription> is consistent with ADR 0053 and catches misconfiguration before any message is processed.
• Tidy-first prerequisites are clearly separated from behavioral changes — switching UseInboxHandler to use the pipeline's this.Context before adding the Replay path is correct.
• Race condition acknowledged (sweeper dispatches while ReplayCausation clears DispatchedAt): the decision to accept this in favour of downstream inbox deduplication is sound and correctly documented.

One design consideration worth flagging:

SupportsCausationTracking() I/O cost at startup: The ADR describes this as a "runtime schema check" — for relational stores, this almost certainly requires a round-trip to the database (e.g. querying INFORMATION_SCHEMA). Pipeline validation runs at startup, and if the check is synchronous and hits the DB, it adds latency to application startup. For async stores this is worse as there will be a sync-over-async call path. The ADR should document whether this check is cached after the first call, and whether the async variant (SupportsCausationTrackingAsync) is used during async pipeline validation.

---

Spec / Requirements Quality

• Requirements are clear and well-scoped.
• Out-of-scope items (saga orchestration, immediate-send replay, migration tooling) are explicitly listed — good.
• AC-8 covers all persistent stores, which aligns with the ADR's 18-store list.
• Minor typo in requirements.md line 507: "Brigter consumer" and "te re-execution" / "re never actioned" — these don't affect correctness but should be fixed before merge.

---

Summary

Area	Status
PROMPT.md in commit history	❌ Remove before merge
specs/0027/README.md tasks checkbox	⚠️ Update to [x]
ADR 0057 design quality	✅ Solid
SupportsCausationTracking() startup I/O cost	⚠️ Document caching/async behaviour
Requirements completeness	✅ Good — minor typos only
Non-breaking / opt-in design	✅ Correct approach

The design is sound and the specification artefacts are high quality. The main blocker before merging is removing PROMPT.md from the branch history.

---

Review generated by Claude Code

[claude]

Code Review — PR #4067: Replay Outgoing Messages When Inbox Receives Duplicate

Note: This PR is a specification/planning PR — it contains no implementation code. All changes are documentation, ADR, and spec artifacts. The review below is therefore focused on the design quality and planning artifacts.

---

What's in this PR

• .gitignore updated to exclude PROMPT.md
• PROMPT.md — a Claude Code session resume file (accidentally committed)
• docs/adr/0057-replay-outbox-on-inbox-duplicate.md — comprehensive ADR (624 lines)
• specs/0027-replay-matching-outbox-events-when-inbox-has-already-seen/ — full spec: requirements, tasks (23 tasks), design review, approval markers
• Review/approval artifacts for specs 0022 and 0026
• .current_spec pointer updated; duplicate .current-spec (hyphen) file deleted

---

Issues

PROMPT.md should not be committed

The .gitignore update adds PROMPT.md, which is correct going forward, but the file itself is now tracked and is part of this PR's diff. It contains Claude Code internal session state (task history, round-by-round review notes, implementation roadmap). This should be removed from git tracking:

git rm --cached PROMPT.md

Without this, the session file becomes permanent history and will appear in everyone's working trees on checkout.

---

ADR Quality — docs/adr/0057-replay-outbox-on-inbox-duplicate.md

The ADR is well-structured and thorough. The following points are worth addressing before or during implementation:

1. DescribePipelines() also needs InboxConfiguration

The ADR correctly identifies that ValidatePipelines() creates PipelineBuilder without InboxConfiguration. However, DescribePipelines() (line 89 in BrighterPipelineValidationExtensions.cs) has the same gap. The review-design.md notes this at score 35. Since DescribePipelines() is a diagnostic tool used by operators to inspect pipeline state, giving it an incomplete view of the pipeline (missing global inbox attributes) makes its output misleading. Worth fixing as part of Task 3.

2. SupportsCausationTracking() synchronous schema check at startup

SupportsCausationTracking() is described as a "permanent runtime schema check." For persistent stores this likely means a DB query (e.g. checking if the CausationId column exists). When called during ValidatePipelines() at startup, this blocks application startup for every registered pipeline. For stores with slow connections (cloud-hosted DBs, cold containers), this could meaningfully extend startup time. The ADR should document the expected performance characteristics of this check, or note that implementations should cache the result.

3. Race condition framing could be clearer

The ADR describes the race condition — sweeper dispatches while ReplayCausation is clearing — and correctly notes it's acceptable because downstream inboxes handle duplicates. However, the framing implies only a single delivery. In practice, a message could be delivered twice: once by the in-flight sweep, and once after the dispatch state is cleared. The mitigation should explicitly say "downstream consumers with OnceOnly inboxes handle the duplicate." Without inboxes downstream, this becomes a real correctness concern. Worth adding a note about the requirement for downstream idempotency.

4. CausationId defaults to request.Id

Using the command's Id as the default CausationId is convenient, but the implications should be documented: if request.Id is not truly unique across handler invocations (e.g. deterministic IDs in tests or retried commands with the same ID), two different handler invocations could share the same CausationId, causing one invocation's replay to clear another's outbox messages. The ADR should note this assumption.

---

Requirements (requirements.md)

Minor: two typos in the Problem Statement — "Brigter consumer" → "Brighter consumer" and "those that we re never actioned" → "were never actioned". Small but this document is the canonical source of truth for the feature.

---

Tasks (tasks.md)

The task structure is clean with clear TDD enforcement. A few observations:

Task 3 — DescribePipelines() not in scope

As noted above, DescribePipelines() has the same InboxConfiguration gap as ValidatePipelines(). Consider adding it to Task 3 to fix both paths together.

Task 14 — 7 validation scenarios in one test file

The review-tasks.md (score 50) flags that Task 14 packs 7 scenarios into one test file and recommends [Theory]/[InlineData]. This is the right call — enforce it explicitly in the task description so the implementing agent follows it, rather than leaving it as a "recommendation."

Task 23 — "run all core tests" is underspecified

Task 23 says "Build + run all core tests" without listing which projects constitute "core." Given the breadth of this change (UseInboxHandler, PipelineBuilder, validation, InMemoryInbox/Outbox), consider enumerating the minimum test suite: Paramore.Brighter.Core.Tests, the OnceOnly suite, and the Validation suite at minimum.

Dependency on schema migration PR

The ADR notes a separate schema migration PR must land on master before this feature. This external dependency is not tracked in the task list. Suggest adding a prerequisite note or a blocking PR link so the implementation isn't merged ahead of its schema dependency.

---

Summary

Area	Status
PROMPT.md in repo	Must fix — remove from git tracking
ADR structure & completeness	Good — minor gaps noted
DescribePipelines() gap	Low priority — fix in Task 3
SupportsCausationTracking() startup cost	Needs documentation
Race condition clarity	Needs a note on downstream idempotency requirement
Requirements typos	Trivial — worth fixing
Task 14 scenario structure	Clarify [Theory] expectation in task
Task 23 test scope	Enumerate specific suites
Schema migration PR dependency	Track explicitly

The design itself is sound — the role interface approach (IAmACausationTrackingInbox/Outbox) is non-breaking and consistent with Brighter's existing patterns. The decision to use the outbox sweeper for re-dispatch rather than building a new dispatch path is the right call. The CausationId concept is well-scoped and distinct from CorrelationId/JobId.

[claude]

Review — PR #4067: Replay Outgoing Messages When Inbox Receives Duplicate

Thanks for the very thorough design work — ADR 0057 is well structured (clear Forces, Alternatives Considered, Consequences, Risks table), the role-interface approach (IAmACausationTrackingInbox/IAmACausationTrackingOutbox) is consistent with Brighter's IAmA* conventions, and the opt-in/non-breaking framing is sound. The task breakdown follows the mandated TDD-first workflow and maps cleanly back to the FRs.

Most of my feedback is about scope/hygiene of this PR, since it ships only design+spec docs — no production code or tests yet.

🔴 Blocking / should-resolve-before-merge

1. Closes #2541 with no implementation. This PR contains the ADR, requirements, and task list but none of the 23 implementation tasks. Merging as-is will auto-close [Feature] Force Replay of Outbox on seeing duplicate Processed Inbox Message #2541 even though the feature isn't built. Suggest either dropping Closes (use "Part of [Feature] Force Replay of Outbox on seeing duplicate Processed Inbox Message #2541" / "Design for [Feature] Force Replay of Outbox on seeing duplicate Processed Inbox Message #2541") or keeping the issue open until the implementation PR lands.

2. PROMPT.md is committed and added to .gitignore. Patch 1 adds PROMPT.md (tracked), patch 3 adds it to .gitignore. Adding to .gitignore does not untrack an already-tracked file, so it stays in the repo. Per CLAUDE.md, PROMPT.md is intended as temporary cross-conversation state — it carries session-resume notes, review-round history, and a stale status block. Recommend git rm --cached PROMPT.md (and commit) if the intent is to ignore it, or drop it from the PR entirely. As-is the two changes contradict each other.

3. Unrelated spec state bleeding into this PR. The diff touches files belonging to other specs:

   • specs/0022-Defer-Message-Action-Backstop-Handler/review-tasks.md (new)
   • specs/0026-timed-outbox-archiver-sync-fallback/review-tasks.md and .tasks-approved (new)
   • flips to both specs/.current-spec and specs/.current_spec

   CLAUDE.md's "Change Scope" rule says not to make changes beyond what was requested. These look like workflow scratch state for unrelated specs and should be split out so this PR is just spec 0027.

4. Two divergent "current spec" files: .current-spec (hyphen) and .current_spec (underscore). The PR edits the hyphen one, then deletes it, and edits the underscore one. This looks like a typo'd duplicate. Worth settling on a single canonical filename so the spec tooling doesn't read stale state.

🟡 Design observations (for the implementation PR)

These don't block a docs PR but are worth tracking against the tasks:

• ADR InboxItem example uses primary-constructor syntax (ADR ~line 339) while the real src/Paramore.Brighter/InMemoryInbox.cs uses a traditional constructor. review-design.md already flagged this (score 45) but the ADR text wasn't updated — an implementer following the snippet could needlessly refactor the class. Add an "illustrative only" note or align it.
• Replay no-op when tracking unsupported at runtime. The Consequences section acknowledges Replay silently does nothing if inbox/outbox don't support causation tracking and pipeline validation is the safety net — but several validation branches emit Warnings, not Errors (no outbox, schema not yet migrated). Confirm a Warning is loud enough that a misconfigured Replay is actually noticed, given the runtime behavior is a silent skip.
• Double-dispatch race (sweeper dispatching while ReplayCausation clears state) is accepted as "downstream inbox dedup handles it." Reasonable, but worth an explicit test asserting idempotency expectations so it's a documented contract, not an assumption.
• review-tasks.md finding Support management and Monitoring of ServiceActivator instances #6 notes UseInboxHandlerAsync calls base.InitializeFromAttributeParams() twice. Good to fold that fix into Task 1's tidy-first pass as noted.

🟢 Nits

• requirements.md Problem Statement has several typos: "Brigter consumer", "te re-execution", "those that we re never actioned", "enabled instead.." (double period), "This a terminal step". Minor, but these are the authoritative requirement statements.
• Terminology mixes TimeFlushed (in-memory) vs DispatchedAt/Dispatched (persistent) — partially reconciled in patch 2; consider a one-line glossary so implementers don't conflate them.

Test coverage

N/A for this PR (docs only). The task list correctly front-loads in-memory /test-first tasks before persistent stores and routes outbox store tests through the Liquid generator — good adherence to the mandatory TDD workflow.

---

Summary: The design is strong and review-ready. Before merge I'd (1) fix Closes #2541 so the issue isn't prematurely closed, (2) resolve the PROMPT.md tracked-vs-ignored contradiction, and (3) split out the unrelated 0022/0026 spec files and the duplicate .current_spec/.current-spec so this PR is scoped to spec 0027 only.

🤖 Generated with Claude Code

[claude]

PR Review — #4067 Replay Outgoing Messages When Inbox Receives Duplicate

Reviewed against the repo's CLAUDE.md and spec workflow conventions. Overall: the design work here is excellent — but this PR is design/spec only and ships no implementation, and it carries a few hygiene/scope problems that should be fixed before merge.

Scope of the change

This PR touches 0 files under src/ or tests/. It adds:

• docs/adr/0057-replay-outbox-on-inbox-duplicate.md (the design)
• specs/0027-.../ requirements, tasks, review docs, and approval markers
• PROMPT.md, a .gitignore line, and updates to spec marker files
• review-task files for unrelated specs 0022 and 0026

That's consistent with the spec workflow (Requirements → ADR → Tasks, all approved), but the PR is framed as the feature itself. See the first blocking point.

---

🔴 Blocking / should-fix

1. Closes #2541 will auto-close the issue, but the feature isn't built. This PR delivers the spec/design only (Implementation is [ ] not started in PROMPT.md and tasks.md). Merging it will close [Feature] Force Replay of Outbox on seeing duplicate Processed Inbox Message #2541 even though no behavior exists yet. Either drop Closes (use Refs #2541), or hold the merge until the 23 implementation tasks land.

2. PROMPT.md is committed and listed in .gitignore (line 346). These contradict each other — the commit "chore: add PROMPT.md to .gitignore" signals intent not to track it, yet it's in the PR. Per CLAUDE.md, PROMPT.md is local, conversation-spanning working state, not a repo artifact. Run git rm --cached PROMPT.md and drop it from the PR (the .gitignore entry can stay).

3. Unrelated spec files included — scope creep. specs/0022-Defer-Message-Action-Backstop-Handler/review-tasks.md, specs/0026-timed-outbox-archiver-sync-fallback/review-tasks.md, and specs/0026-.../.tasks-approved belong to other features. CLAUDE.md → Change Scope: "Do NOT change … beyond what was explicitly requested." Please split these out.

4. Two competing marker files: specs/.current-spec (hyphen) and specs/.current_spec (underscore). This PR empties one and updates the other (the merge history shows a conflict between them). Having both is confusing and will keep colliding. Consolidate on a single convention and delete the other.

---

🟡 Minor

5. Typos in requirements.md (Problem Statement, line 9; Proposed Solution, line 23): "Brigter", "te re-execution", "we re never actioned", "instead..". Worth a cleanup since this is the authoritative requirements doc.

6. Two low-scored items from your own review-design.md remain open and are worth carrying into implementation: the InboxItem example uses primary-constructor syntax while the real class doesn't (don't let an implementer refactor it), and DescribePipelines() should also receive InboxConfiguration for consistency with ValidatePipelines().

---

Design assessment (ADR 0057)

The ADR is genuinely strong — 7 review rounds show. I spot-checked its claims against the current code and they hold up:

• OnceOnlyAction is a 2-value enum (Throw, Warn) — adding Replay is additive ✅
• UseInboxHandler does use a private InitRequestContext() today (lines 81/111) — the tidy-first switch to this.Context is a real and correct prerequisite ✅
• IAmAnOutboxProducerMediator exists and the Outbox property addition is sound ✅

The opt-in / non-breaking strategy, role interfaces (IAmACausationTracking*), reuse of the existing sweeper, and the startup pipeline validation are all well-judged. Alternatives and risks (double-dispatch under sweeper race, inbox-Add-after-outbox-write failure) are addressed honestly.

Two things to watch during implementation:

• Instance identity between validation and runtime. Validation reads the outbox via IAmAnOutboxProducerMediator.Outbox, while UseInboxHandler receives IAmACausationTrackingOutbox? via constructor DI. These are two resolution paths — make sure they resolve the same instance, or SupportsCausationTracking() could pass at startup against one object while replay runs against another. A test asserting instance identity would be cheap insurance.

• Cross-PR sequencing. The ADR depends on a separate schema-migration-tooling PR being "merged into master before this feature." That ordering constraint is invisible from this PR; calling it out in the PR description (with the dependent PR number) would prevent a premature merge.

---

Summary: The design is approved-quality and the task breakdown is thorough. Before merge, please address the four blocking items — most importantly, decide whether this is the spec-only PR (then don't Closes #2541) or wait for implementation, and remove PROMPT.md + the unrelated 0022/0026 files.

_{🤖 Generated with Claude Code}

[claude]

Code Review: Replay Outbox Messages on Inbox Duplicate (#4067)

Reviewed the core mechanism (UseInboxHandler/Async, InMemoryInbox/Outbox, RelationDatabaseOutbox/RelationalDatabaseInbox), the new role interfaces, DI wiring, pipeline validation, and the test surface. This is a large, carefully-executed change — the ADR is thorough, the schema probe is defensive and well-documented, breaking changes are called out in release_notes.md, and test coverage is broad (base tests + generated outbox tests + per-store inbox tests + legacy/un-migrated + migration/provisioning tests). Nicely done.

A few things worth a look before merge.

1. Async telemetry events may be silently dropped on the duplicate paths (correctness, low severity — telemetry only)

RequestContext.Span is thread-affine — the getter keys on Thread.CurrentThread.ManagedThreadId (RequestContext.cs:114-126). In UseInboxHandlerAsync.HandleAsync, the Throw/Warn/Replay branches read Context?.Span after the await _inbox.ExistsAsync(...) (UseInboxHandlerAsync.cs:108, :115, :123). With ContinueOnCapturedContext == false the continuation typically resumes on a different thread-pool thread, so Span returns null and the AddEvent(...) never fires — meaning the "Duplicate Throw/Warn/Replay" events the ADR promises are lost in the common async case.

Note the Add path deliberately captures addSpan = Context?.Span (:145) to survive the base.HandleAsync/AddAsync awaits — but even that capture is after the ExistsAsync await, so it is subject to the same loss. Suggest capturing the span once, before the first await, and reusing that captured value on every path (Throw/Warn/Replay/Add) for consistent behaviour. Functional replay is unaffected (it uses requestContext, not the span) — this is observability only.

2. Default causation id is not unique per handler invocation (design)

On first handling the causation id defaults to request.Id.Value (UseInboxHandler.cs:92-93). Both the inbox entry and every downstream outbox message are then tagged with command.Id. If the same message id is processed by more than one [UseInbox] handler (e.g. an event with multiple subscribers, each with its own inbox/context key), they all share causation id = command.Id. GetCausationId is context-scoped, but the value it returns is identical across contexts, and ReplayCausationCommand matches on that value alone (UPDATE ... WHERE CausationId = @CausationId). So a replay triggered by one handler resends the outbox messages of all handlers that share the command id.

This is at odds with the ADR's framing that causation is "scoped to a single handler invocation." It is fine as long as callers who need isolation set a unique causation id in the bag themselves — but the default does not guarantee that scoping. Worth either documenting the caveat prominently or scoping the default (e.g. incorporating the context key) so the out-of-the-box behaviour matches the stated design.

3. Custom IRequestContext silently degrades replay (documented, minor)

var requestContext = Context as RequestContext ?? new RequestContext { ... } (UseInboxHandler.cs:90): if a custom IAmARequestContextFactory returns a non-RequestContext IRequestContext, the causation id is stamped into a throwaway context that downstream handlers never see, so Replay becomes a no-op. The code comment acknowledges this, but since it fails silently (no warning/log), a user on a custom context would get no replay and no signal. Consider a one-time debug/warn log when this fallback is hit and Replay is configured.

Minor

• Deferred DI factory (ServiceCollectionExtensions.cs:464-466) returns a null-forgiving cast, so GetRequiredService<IAmACausationTrackingOutbox>() returns null rather than throwing for a non-tracking outbox. Documented in the comment; just flagging that it violates the usual GetRequiredService contract for anyone who resolves it directly.
• The memoized _causationColumnExists probe is never invalidated, so a mid-process schema migration requires a restart. Documented in the field comment — reasonable, just confirming it is intentional.

Positives

• Exception-safe schema probe wrapper (SupportsCausationTrackingSafely) so a broken store probe cannot take down validation.
• Graceful degradation when schema is un-migrated (AC10) is consistently applied across read/write/replay on both inbox and outbox, with matching legacy tests.
• Breaking changes (IRequestContext.InstrumentationOptions, IAmABoxMigration, etc.) are clearly documented in release notes with migration guidance.

Overall this is solid, well-tested work. Item #1 is the only concrete bug (and it is telemetry-only); #2 is the one I would most want a maintainer decision on before merge.

Automated review — please verify findings against intent.

[claude]

Code Review — PR #4067: Replay outbox messages on inbox duplicate detection

Reviewed the core feature (OnceOnlyAction.Replay, causation tracking through the inbox → outbox), the UseInboxHandler/UseInboxHandlerAsync changes, the relational + NoSQL store implementations, the DI wiring, pipeline validation, and the migration catalogs. This is a large, carefully-executed change (ADR 0057, 7 rounds of review) and the overall quality is high — role interfaces are cleanly separated, the mixed-migration ("upgraded binaries, un-migrated schema") case is handled thoughtfully in the relational path, telemetry is consistent across sync/async, and test coverage is broad (legacy-schema tests, validation tests, end-to-end replay tests, per-backend tests). The comments explaining the tricky bits (span thread-affinity, _causationColumnExists memoization, ??= caching) are excellent.

A few points below, one worth addressing before merge.

1. (Medium) DynamoDB ReplayCausationAsync doesn't guard against a missing causation GSI — asymmetric with the relational path

RelationDatabaseOutbox.ReplayCausation[Async] deliberately gates on the live-schema probe and no-ops when the CausationId column is absent (the AC10 "inbox migrated, outbox not" case):

if (CausationQueries is null || !CausationColumnExists())
    return;

The DynamoDB outboxes (both Outbox.DynamoDB and Outbox.DynamoDB.V4) do not apply the equivalent guard — ReplayCausationAsync issues a QueryAsync against _configuration.CausationIndexName unconditionally:

• src/Paramore.Brighter.Outbox.DynamoDB/DynamoDbOutbox.cs:599
• src/Paramore.Brighter.Outbox.DynamoDB.V4/DynamoDbOutbox.cs:612

In a mixed state (inbox has the CausationId attribute — the Dynamo inbox is schemaless and SupportsCausationTracking() always returns true, so GetCausationId returns a non-null id — but the outbox table was provisioned before the Causation GSI was added), the handler reaches ReplayCausationAsync, which then queries a non-existent index and DynamoDB throws a ValidationException. That exception unwinds the handler pipeline on every duplicate, whereas the relational backends degrade gracefully to a no-op in exactly this scenario.

Both outboxes already have the _causationIndexExists probe (SupportsCausationTrackingAsync) available; gating the replay on it (return early if the index is absent, mirroring the relational no-op and the existing ConditionalCheckFailedException tolerance) would restore parity. Startup validation only warns for the mixed state, so it doesn't prevent this at runtime.

There's a When_a_dynamodb_outbox_table_lacks_the_causation_index_should_not_claim_causation_support test asserting SupportsCausationTracking() is false, but no test drives ReplayCausationAsync against a table whose GSI is missing — worth adding alongside the guard.

2. (Minor) .gitignore no longer ignores PROMPT.md

The diff removes the # Claude Code session state / PROMPT.md ignore entry. CLAUDE.md still references PROMPT.md as a place for temporary cross-conversation state, so dropping the ignore makes it easy to accidentally commit session scratch state. If this removal was intentional (e.g., PROMPT.md is now meant to be tracked), ignore this; otherwise consider keeping the entry.

3. (Nit) Sync-over-async in the NoSQL replay entry points

DynamoDbOutbox.ReplayCausation (.GetAwaiter().GetResult()) and FirestoreOutbox.ReplayCausation (BrighterAsyncContext.Run) block on the async path. This matches the existing convention in those classes, so it's consistent rather than a regression — just flagging it as the usual sync-over-async deadlock caveat for the sync UseInboxHandler path.

Things I checked that looked good

• Relational replay adds an index on CausationId in the V8 migration (idx_{table}_CausationId), so UPDATE ... WHERE CausationId = @id won't full-scan.
• _causationColumnExists memoization is correct for both sync (??=) and async (explicit cache check), and the "requires restart after mid-process migration" limitation is documented.
• The handler correctly falls back to a fresh RequestContext (with a one-time warning) when a custom IRequestContext can't carry the causation id, and captures the span before the first await.
• Pipeline validation (ReplayRequiresCausationTracking) correctly distinguishes Error (role not implemented) vs Warning (role present, schema not migrated / no outbox), and wraps the probe so an unreachable store doesn't crash host startup.
• Replay path returns the duplicate command without re-adding it to the inbox — correct.

Nice work overall — the mixed-migration handling in particular is very thorough. Addressing #1 would bring the DynamoDB backends up to the same runtime robustness as the relational ones.

🤖 Generated with Claude Code

[claude]

Code Review — Replay Outgoing Messages When Inbox Receives Duplicate (#4067)

Thanks for a substantial, well-structured feature. I reviewed the core mechanics (the sync/async UseInboxHandler, RelationDatabaseOutbox/RelationalDatabaseInbox, the in-memory boxes, DI wiring, pipeline validation) and spot-checked the per-backend query/migration catalogs and the DynamoDB path. Overall this is high quality: spec-driven (ADR 0057 / spec 0027), heavily documented, and with broad test coverage.

What's strong

• Graceful degradation is thorough. Live-schema probes (CausationColumnExists, SupportsCausationTracking) gate every write/replay path, so a mixed state (inbox migrated, outbox not) safely no-ops rather than erroring against an absent column. The legacy "lack causation column" tests across every backend confirm this.
• Idempotent migrations — ADD COLUMN IF NOT EXISTS (Postgres), IdempotencyCheckSql gating (SQLite/MSSQL), plus replay indexes. Safe on chain replay.
• Telemetry distinguishes Replay from Replay Skipped (no causation id found), which will save operators from misreading a no-op as a successful resend.
• The async span-capture comment/fix (capturing Context?.Span before the first await because RequestContext.Span is thread-affine) is a genuinely subtle correctness point handled correctly.
• DynamoDB replay handles GSI pagination and eventual-consistency vanished items (ConditionalCheckFailedException) instead of unwinding the loop.
• Removing the duplicated base.InitializeFromAttributeParams(...) call in the async handler is a nice incidental cleanup.

Things worth confirming / addressing

1. Binary-breaking interface changes — release/versioning.
IRequestContext.InstrumentationOptions and IAmAnOutboxProducerMediator.Outbox are added as members with no default interface implementation, so any external type implementing these interfaces directly breaks at both source and binary level. The XML doc on InstrumentationOptions says "minor-version bump," but a binary-breaking interface change is conventionally a major version per SemVer. Please confirm this lands in a release that allows the break (or consider a default interface method where the target frameworks permit).

2. Log-level change is a behavior change beyond feature scope.
UseInboxHandler (sync) changes CommandHasAlreadyBeenSeenAsWarning from LogLevel.Debug to LogLevel.Warning. It aligns sync with the async handler (already Warning) and matches the intent of OnceOnlyAction.Warn, so it looks deliberate — but it silently changes logging output for existing Warn users. Per CLAUDE.md's "Change Scope" guidance, worth calling out explicitly in the release notes as an intentional fix.

3. Causation-inheritance / replay-scope semantics.
The handler stamps bag[CausationId] = command.Id only when the key is absent. Today nothing else populates that bag, so causation == the command's own id and replay is nicely scoped to exactly the messages that handling produced. But the moment causation propagation is added (e.g. from message headers / a workflow), a single duplicate would reset the dispatched state of every outbox row sharing that causation id — including sibling/upstream messages. The PR description ("replay a whole workflow") suggests this may be intended; if so, please document the replay-scope semantics near OnceOnlyAction.Replay / ReplayCausation so it isn't a surprise later.

4. Replay is unconditional and untransacted.
ReplayCausation resets dispatched state for the whole causation on every duplicate hit, regardless of whether those messages were already re-dispatched. That's consistent with at-least-once delivery, but a duplicate storm will repeatedly reset-and-resend. Fine as designed — just flagging it as a property to be aware of.

5. Validation runs a live DB probe at startup.
ReplayRequiresCausationTracking calls SupportsCausationTracking() during pipeline validation, which issues a schema round-trip at host startup and requires the store to be reachable. It's correctly wrapped in SupportsCausationTrackingSafely (probe failure → Warning, not a crash), so this is acceptable — noting it for anyone surprised by a DB call during validation.

Nits

• s_warnedAboutCustomContext is a static field on the generic UseInboxHandler<T>, so the "Set once, process-wide" comment is slightly inaccurate — it's once per closed generic T. Harmless, but the comment overstates it.
• Column-type inconsistency: Postgres inbox uses character varying(256) for causationid while the outbox uses character varying(255). Ids are GUID-length in practice, so it doesn't bite, but consider aligning.

None of these are blockers on correctness of the happy path, which looks sound end-to-end (bag stamped before base.Handle → outbox rows carry the causation → inbox stores it → duplicate replays exactly those rows). The main ask before merge is a decision on item 1 (SemVer) and a doc note on items 2 and 3.

Automated review — I built understanding from the diff and source; I was not able to run a local build in this environment, so I'm relying on CI for compile/test verification.

[claude]

Code Review: Replay Outgoing Messages When Inbox Receives Duplicate (#4067)

I reviewed the core design and implementation (inbox/outbox handlers, the causation-tracking role interfaces, the relational + DynamoDB + in-memory stores, DI registration, and pipeline validation). This is a large but very well-executed change — the ADR-0057 write-up, the backward-compatibility gating, the telemetry, and the test coverage (base tests + generated + a genuine end-to-end pump test) are all excellent. Feedback below is mostly nuance and a couple of footguns worth surfacing.

Strengths

• Backward compatibility is handled carefully. The memoized live-schema probe (_causationColumnExists / _causationIndexExists) gates the write path on actual column/GSI existence rather than the static driver capability, so an upgraded-but-un-migrated store keeps depositing unchanged. The reasoning is documented inline where it matters.
• Concurrency edge cases are respected. The async handlers capture Context?.Span before the first await (span is thread-affine), and DynamoDB's ReplayCausationAsync paginates the GSI and swallows ConditionalCheckFailedException for items that vanished from an eventually-consistent index. Both are subtle and correct.
• Validation fails fast. ReplayRequiresCausationTracking distinguishes Error (role not implemented) from Warning (role present but schema not migrated / no outbox), and wraps the probe in SupportsCausationTrackingSafely so an unreachable store can't crash host startup.

Concerns worth a look

1. The biggest footgun — silent no-op if Context isn't threaded through Post/DepositPost. You've documented this prominently in the release notes (👏), but it deserves emphasis: replay correctness depends on every message-producing handler forwarding its Context to Post/DepositPost. Forgetting it stores a null CausationId and a later Replay finds nothing — no error, no warning. Given how idiomatic _commandProcessor.Post(evt) (no context) is across existing Brighter code and docs, this will bite people. Consider whether a diagnostic is feasible — e.g. when OnceOnlyAction.Replay is configured and an outbox Add happens with a null CausationId in a context that had one available, log once at Warning. Not blocking, but the silent failure mode is the thing users will actually hit.

2. DynamoDbConfiguration.CausationIndexName is a settable property that must never be set. The GSI hash-key annotation on MessageItem.CausationId uses a compile-time constant "Causation", so overriding this property silently re-points the probe/query at a GSI the model never declares → replay silently finds nothing. The XML remarks warn against it, but exposing a public setter that can only break things is a trap (contrast the Outstanding/Delivered index names, which are meaningfully overridable). Consider making it get-only / a const, or at least not offering a setter.

3. Extra probe round-trip on first deposit for all relational users. Add/AddAsync now call CausationColumnExists[Async]() unconditionally, so every relational inbox/outbox instance issues one extra schema-probe query on its first deposit — even for users who never touch Replay. It's memoized per instance so aggregate cost is negligible, but the ADR's "byte-for-byte identical deposit behaviour" claim is only true of the INSERT itself; there's a new (small) transient-failure surface on the very first deposit (if the probe throws, that first Add fails and retries next call). Worth being precise about in the notes.

4. GetRequiredService<IAmACausationTrackingOutbox>() can return null in the factory-based AddProducers overload. Because the descriptor is always registered (factory returns (outbox as IAmACausationTrackingOutbox)!), a non-tracking outbox yields null from GetRequiredService rather than the usual throw. You've documented this and the sole consumer uses GetService + null-check, so it's safe today — but a future caller reasonably trusting GetRequiredService to be non-null would be surprised. A tiny risk given the note.

5. Multi-subscriber causation collision. As the ADR notes, because the default causation id is command.Id and ReplayCausation matches on value alone, a duplicate delivered to one [UseInbox] handler replays the outbox messages of every handler that shared that command id. This is intended (drive the whole workflow forward) and [UseInbox] is command-oriented in practice, but it's an operational sharp edge worth keeping visible in user-facing docs, not just the ADR.

Minor

• InMemoryOutbox.ReplayCausationAsync runs the sync body inline and only routes cancellation through the TCS; a synchronous throw from ReplayCausation would propagate directly rather than via the returned task. Harmless for in-memory, just slightly inconsistent with the TCS pattern.
• IRequestContext.InstrumentationOptions is correctly flagged as a source/binary-breaking addition for external IRequestContext implementers (no default interface member on netstandard2.0). Good that it's called out for versioning.

Tests

Coverage looks strong: base tests exercised across every persistent backend, generated causation outbox tests, validation tests (including the "replay configured without causation tracking" error case), telemetry assertions, and a real end-to-end pump test that posts a duplicate and asserts the outbox replays. The DynamoDB "vanished item" and legacy/un-migrated-schema paths are explicitly covered too. Nice.

Overall: high-quality, well-documented work. Nothing here is a blocker; items 1 and 2 are the ones I'd most want addressed (or explicitly accepted) before merge.

Reviewed with Claude Code.

[claude]

Review: Replay outbox messages on inbox duplicate detection (#4067)

Reviewed against the repo's CLAUDE.md/.agent_instructions conventions. This is a large, well-structured feature (ADR 0057, spec 0027) that adds OnceOnlyAction.Replay plus CausationId tracking across every inbox/outbox backend. Overall the code quality is high and the design is carefully thought through — the role-interface split, the backward-compatibility handling, the telemetry, and the pipeline validation are all solid. Comments below are mostly minor observations and a couple of things worth confirming.

Strengths

• Clean role-interface design. IAmACausationTrackingInbox / IAmACausationTrackingOutbox are optional role interfaces layered onto the existing stores, so non-tracking implementations keep working. Good adherence to Responsibility-Driven Design.
• Backward compatibility is handled thoughtfully. New CausationId columns are nullable; the "inbox migrated, outbox not" mixed state is explicitly handled by gating ReplayCausation on a live-schema probe (CausationColumnExists) rather than the static query interface (AC10). This is exactly the right call and is well-commented.
• Async telemetry correctness. Capturing span before the first await in UseInboxHandlerAsync (because RequestContext.Span is thread-affine) is a subtle but correct detail, and the comment explains why.
• Test coverage is excellent. Base tests, per-backend generated causation tests, validation tests, telemetry tests, mixed-migration/legacy seeders, DynamoDB "vanished item" edge case, and an end-to-end replay test. This is comprehensive.
• Pipeline validation (ReplayRequiresCausationTracking) gives users an actionable startup error/warning when Replay is configured without the necessary support, and defensively swallows probe failures so validation can't crash host startup.

Observations / things to confirm

1. Telemetry can report "Replay" when nothing was replayed (mixed-migration state). In UseInboxHandler(.Async), WriteReplayEvent distinguishes "...Duplicate Replay" vs "...Duplicate Replay Skipped" solely on causationId is null. But RelationDatabaseOutbox.ReplayCausation / DynamoDbOutbox.ReplayCausationAsync silently no-op when the outbox schema lacks the column/index while the inbox still returns a non-null causation id. In that state the span records "Duplicate Replay" even though the outbox resent nothing — an operator would misread it as a successful replay. Consider having ReplayCausation signal whether it actually did work (e.g. return rows-affected / a bool) so the event can reflect the no-op, or at least document this gap. (UseInboxHandler.cs:186, UseInboxHandlerAsync.cs:198)

2. s_warnedAboutCustomContext comment says "process-wide", but it's per-closed-generic-type. Static fields on UseInboxHandler<T> are per T, so the "custom context disables Replay" warning fires once per message type, not once per process. Not a bug (the intent — keep the warning off the hot path — still holds), but the comment overstates the dedup scope. (UseInboxHandler.cs:48-50)

3. Deferred DI registration returns null via a null-forgiving !. In the factory-based AddProducers overload, IAmACausationTrackingOutbox is registered as sp => (sp.GetRequiredService<IAmAnOutbox>() as IAmACausationTrackingOutbox)!, so GetRequiredService<IAmACausationTrackingOutbox>() returns null rather than throwing for a non-tracking outbox. The comment documents this and the only consumer uses optional resolution, so it's safe — but it's a footgun for any future consumer that reasonably expects GetRequiredService to throw. (ServiceCollectionExtensions.cs:464-466)

4. Memoized schema probe is never invalidated. _causationColumnExists (relational) and _causationIndexExists (DynamoDB) are cached for the store's lifetime, so a store instance constructed before provisioning caches "absent" and a mid-process migration requires a restart. This is documented in the code comments; just make sure it's surfaced in user-facing docs/release notes so operators migrating a live system know a restart is required for Replay to activate.

5. ReplayCausationCommand is an unbounded single UPDATE. UPDATE {0} SET Dispatched = NULL WHERE CausationId = @CausationId resets every message for a causation in one statement (no batching/limit). Fine for typical causation fan-out, but a pathological causation with a very large number of messages would produce one large write. There is an index on causationid (good), so the predicate is covered; worth being aware of for high-fan-out workflows.

6. Sync span vs. requestContext divergence (very minor). In the sync UseInboxHandler, telemetry uses span = Context?.Span while inbox/outbox calls use the local requestContext, which in the custom-context fallback is a fresh RequestContext { Span = Activity.Current }. The two spans could differ in that fallback path. Thread-affinity isn't a concern on the sync path, so this is cosmetic, but the sync/async paths aren't quite symmetric here.

Correctness spot-checks that look right

• Causation id is consistent end-to-end: the bag is stamped with request.Id (when absent), the inbox stores it, produced outbox messages carry it, and ReplayCausation matches on it. Inbox/outbox causation values line up for the same handling.
• InMemoryOutbox.ReplayCausation resets TimeFlushed = DateTimeOffset.MinValue, which is exactly the sentinel OutstandingMessages tests for — correct.
• DynamoDB replay tolerates the eventually-consistent GSI listing a vanished base item (ConditionalCheckFailedException swallowed per-item) and paginates correctly.

Nice work — this is a substantial, careful feature. The main thing I'd want addressed before merge is item #1 (the telemetry can misreport a no-op replay as a successful one in the mixed-migration state); the rest are minor or confirm-and-document.

Reviewed with Claude Code.

[claude]

Code Review — PR #4067: Replay outbox messages on inbox duplicate detection

Reviewed the core library changes (I focused on src/Paramore.Brighter/** rather than the ~150 generated/migration test files, which look mechanically consistent). Overall this is a high-quality, well-engineered PR: the spec-driven workflow shows, the optional role interfaces (IAmACausationTrackingInbox / IAmACausationTrackingOutbox) preserve backward compatibility, the mixed-migration-state handling is carefully thought through, and the inline comments explaining non-obvious decisions (span capture before await, the memoization race, the live-schema gate vs. the static query interface) are excellent. Nice catch fixing the duplicate base.InitializeFromAttributeParams(...) call in the async handler too.

Below are observations, mostly minor — nothing I'd consider a blocker.

Design & correctness

1. IRequestContext.InstrumentationOptions is a binary/source-breaking change. Adding an interface member with no default implementation breaks any external type implementing IRequestContext directly. The XML remarks already call this out and recommend a minor-version bump — good. Please make sure this is surfaced prominently in release_notes.md / migration notes, since it's easy for a downstream implementer to miss.

2. Sync handler log-level change (Debug → Warning) for the Warn action. CommandHasAlreadyBeenSeenAsWarning was previously logged at Debug; it now logs at Warning. This aligns the sync handler with the async handler (which was already Warning), so it looks like an intentional bugfix — but it's a behavior change not mentioned in the PR description, and per the repo's "change scope" guidance it's worth an explicit confirmation that it's intended.

3. Handler now passes the shared pipeline Context to _inbox.Add. Previously InitRequestContext() always handed the inbox a fresh RequestContext { Span = Activity.Current }; now it passes Context as RequestContext (falling back to a fresh one only for custom contexts). This is necessary for the causation id to flow, but it is a semantic change — the inbox Add now sees the live pipeline context (full Bag, policies, etc.) rather than an isolated one. Intentional, just flagging it for awareness.

Minor / nits

4. s_warnedAboutCustomContext is not actually "process-wide". The comment says "Set once, process-wide", but a static field in a generic type UseInboxHandler<T> has one instance per closed generic type, so the warning will fire once per request type T, not once per process. Not harmful, but either the comment should be corrected or the flag moved to a non-generic holder if truly once-per-process is desired.

5. Replay telemetry reports success on a zero-row replay. WriteReplayEvent emits "UseInboxHandler Duplicate Replay" whenever replayed == true, but replayed only means the outbox ran the reset — a causation whose messages matched zero rows still reports a successful replay rather than a skip. Consider whether operators would find "replayed 0 messages" misleading vs. the existing "...Replay Skipped" event.

6. Memoized _causationColumnExists is never invalidated. A store constructed before schema provisioning caches "absent" and requires a process restart to pick up a later migration. This is explicitly documented in the code comment (good) — just ensure the operational caveat also lands in the user-facing docs, since "I migrated but replay is still a no-op until restart" is a plausible support question.

7. First Add pays a schema-probe round trip on a separate connection. The initial CausationColumnExists() probe runs via ReadFromStore/ReadFromStoreAsync, i.e. on its own connection outside any ambient user transaction passed via transactionProvider. Fine for an information_schema-style read, but worth being aware of if anyone later expects the probe to participate in the caller's transaction.

8. Startup validation only calls the sync SupportsCausationTracking() probe (SupportsCausationTrackingSafely takes Func<bool>), even for async pipelines — so a relational store opens a synchronous connection during host startup. It's wrapped in try/catch and degraded to a Warning, so it won't crash startup, but async-only environments will still take the sync path here.

9. Housekeeping: the diff touches both specs/.current-spec (hyphen) and specs/.current_spec (underscore, now emptied). Looks like a stray/duplicated tracking file worth cleaning up.

Tests

Test coverage is a real strength — the base-test pattern (CausationTrackingInboxBaseTests / CausationTrackingOutboxBaseTests) plus generated per-backend suites gives broad coverage, and When_a_seen_message_is_replayed_end_to_end is a genuine end-to-end exercise through a real pump/bus rather than a seam. Good coverage of the "un-migrated schema" degradation path (When_..._lack_causation_column_should_still_add_and_retrieve), the vanished-item race for DynamoDB, and the validation error path. 👍

Nice work overall — the concerns above are polish, not correctness blockers.

[claude]

Code Review — PR #4067: Replay outbox messages on inbox duplicate detection

Reviewed against the design in docs/adr/0057-* and the guardrails in CLAUDE.md. This is a large, well-structured change: the role-interface split (IAmACausationTrackingInbox / IAmACausationTrackingOutbox), the opt-in schema evolution via BoxProvisioning, the runtime SupportsCausationTracking() probe, and the startup pipeline validation are all thoughtful, non-breaking, and consistent with the existing IAmA* conventions. Test coverage is broad (base tests reused across every persistent store, plus telemetry, validation, and end-to-end tests). Nice work overall.

One blocking concern and a few smaller notes.

🔴 Blocking — likely compile error in both inbox handlers

src/Paramore.Brighter/Inbox/Handlers/UseInboxHandler.cs:100 and src/Paramore.Brighter/Inbox/Handlers/UseInboxHandlerAsync.cs:102:

private static bool s_warnedAboutCustomContext;   // line 51
...
if (_onceOnlyAction is OnceOnlyAction.Replay
    && Interlocked.CompareExchange(ref s_warnedAboutCustomContext, 1, 0) == 0)

Interlocked.CompareExchange has no overload that takes ref bool — the overloads are int, uint, long, ulong, float, double, IntPtr, UIntPtr, object, and T where T : class. Because ref/out arguments require an exact type match (no implicit conversion of the referent), ref bool cannot bind to the ref int overload, and the 1/0 operands plus the == 0 comparison rule out the generic reference-type overload. This should fail to compile (CS1503 / CS0411).

The usual idiom is to make the flag an int:

private static int s_warnedAboutCustomContext;

(Note: I could not run dotnet build in this review sandbox to confirm, so please double-check against CI — but I am fairly confident this does not compile as written.)

🟡 Minor / worth a look

1. DI role registration is inconsistent between the two outbox registration overloads. In the eager overload the IAmACausationTrackingOutbox descriptor is registered only when the outbox implements the role (ServiceCollectionExtensions.cs:332), so GetRequiredService<IAmACausationTrackingOutbox>() throws for a non-tracking outbox. In the deferred overload (:464) the descriptor is always registered and the factory returns null for a non-tracking outbox, so GetRequiredService returns null instead of throwing. It is documented and the only consumer uses the optional ctor arg, so it is not a bug — but the divergent resolution semantics for the same service type are a latent trap for a future consumer. Consider aligning both paths.

2. Null causation id added as a telemetry tag. In WriteReplayEvent, { BrighterSemanticConventions.CausationId, causationId } is added even when causationId is null. Harmless, but you may prefer to omit the tag when there is nothing to record so the skipped-replay event does not carry a null attribute.

3. Multiple-subscriber causation collision is documented as intended (default causation id = command.Id, so a replay from one [UseInbox] handler resends the outbox messages of every handler sharing that id). The ADR justifies this well for the Command case; worth surfacing in the user-facing docs/release notes too, since an event delivered to multiple subscribers with the same id is an easy footgun.

4. WriteToStore(null, ...) in ReplayCausation deliberately runs the UPDATE ... SET Dispatched = NULL outside any ambient transaction provider. Reasonable for a standalone replay, but worth a one-line comment noting the intent, since every other write path threads a provider through.

✅ Things I checked and liked

• The live-schema probe (CausationColumnExists) is memoized and correctly gates both the Add path and ReplayCausation, giving a clean no-op (returning false) in the "inbox migrated, outbox not" mixed state rather than issuing SQL against an absent column.
• Migration adds CausationId as a nullable column plus an idempotent replay index — non-breaking for un-migrated stores.
• The replayed bool threaded back into telemetry correctly distinguishes "replayed" from "skipped/no-op".
• Span captured once before the first await in the async handler, with a clear comment explaining the thread-affinity rationale.

Once the Interlocked field type is fixed (assuming it is confirmed), this looks solid.

Automated review by Claude Code.

[claude]

Reviewed the core plumbing (UseInboxHandler/Async, RelationDatabaseOutbox, RelationalDatabaseInbox, the role interfaces, DI wiring, pipeline validation) plus the per-backend implementations and migrations. (review 1 of 2)

This is a well-executed feature. The things I would normally have to ask for are already here: an ADR, migrations for all eight backends with idempotent guards and Identifiers.AssertSafe, generated base tests fanned out across every store, legacy/un-migrated seeder tests for the mixed-schema state, a startup validation rule, telemetry, and release notes that lead with the footgun rather than burying it. The DynamoDB replay in particular is careful: it is the exact inverse of MarkDispatchedAsync, it paginates, and it swallows ConditionalCheckFailedException for GSI eventual consistency.

Findings roughly by severity.

1. Firestore replay commits an unbounded write batch

src/Paramore.Brighter.Outbox.Firestore/FirestoreOutbox.cs:907-942

ReplayCausationAsync streams every matching document into a single List<Write> and then issues one CommitRequest. Firestore caps a commit at 500 writes, and the query itself has no Limit/paging. A causation that produced more than 500 outbox messages fails the whole replay with INVALID_ARGUMENT, and a very large causation buffers unboundedly first.

Compare the DynamoDB implementation (DynamoDbOutbox.cs:621-675), which pages the GSI query and updates item-by-item. Firestore should chunk into commits of at most 500, and ideally commit as it streams rather than after.

2. Mongo sync replay reports the wrong DB operation in telemetry

src/Paramore.Brighter.Outbox.MongoDb/MongoDbOutbox.cs:866

The sync ReplayCausation builds its span with BoxDbOperation.MarkDispatched; the async twin correctly uses BoxDbOperation.Replay. Copy-paste slip, but a replay from the sync pipeline then shows up in traces as the precise opposite of what happened.

3. The schema probe now runs a read on a second connection inside Add

RelationDatabaseOutbox.cs:94-100 -> CausationColumnExists() -> ReadFromStore (:1194-1203)

Add(message, ctx, timeout, transactionProvider) calls CausationColumnExists(), which goes through ReadFromStore -> GetOpenConnection(ConnectionProvider, null) — deliberately not the transaction provider — and ends with finally { connection.Close(); }. Two consequences worth checking before merge:

• IAmATransactionConnectionProvider is both interfaces (IAmATransactionConnectionProvider.cs:5) and RelationalDbTransactionProvider.GetConnection() returns a cached connection. Any app registering its unit-of-work under IAmARelationalDbConnectionProvider — the hierarchy actively invites this, even though the in-tree samples register a separate provider — will have the probe close the very connection carrying the callers open transaction. Since the samples do not do this, treat it as "please confirm" rather than a confirmed break, but the failure mode is a silently aborted outbox transaction, which is expensive to diagnose.
• Even with distinct providers, the first deposit inside a user transaction now opens a second connection. Under SQLite default rollback journal, a reader against a DB with an open write transaction gets SQLITE_BUSY.

Every other ReadFromStore caller (Get, OutstandingMessages, DispatchedMessages) is sweeper-side and never runs inside a user transaction — Add is the first that does, so this coupling is new. Suggest hoisting the probe out of the write path (warm once at construction / first use outside a transaction), or routing it through GetOpenConnection(ConnectionProvider, transactionProvider) and skipping the Close() when a transaction is open.

Related and worth a release-note line: the probe fires on every relational outbox Add, so all users pay one extra round-trip on first deposit and an extra INSERT column thereafter, whether or not they ever enable Replay.

4. ReplayCausation reports success without knowing anything changed

RelationDatabaseOutbox.cs:1069 — WriteToStore discards the ExecuteNonQuery() row count, so the relational replay returns true whenever the column exists, even when the UPDATE matched zero rows (messages archived, swept, or never written with a causation id). The handler then emits "UseInboxHandler Duplicate Replay", indistinguishable from a genuine resend. Firestore does the same explicitly (:934-935).

Given the release notes correctly identify "silent no-op" as the failure mode of this feature, surfacing the affected row count would make it diagnosable. Right now the telemetry answers "were we allowed to try?" rather than "did we replay?".

Relatedly, in UseInboxHandler.ReplayCausation (:178-188) the "no causation id found" path returns quietly, with a span event as the only signal. A Warning-level log there would catch exactly the Post(evt)-without-Context mistake the release notes warn about.

5. Memoized probe caches "absent" forever, and validation may be what populates it

RelationDatabaseOutbox.cs:36-46 documents that a store constructed before provisioning caches false for the process lifetime. Combined with ValidatePipelines() calling the synchronous SupportsCausationTracking() from BrighterValidationHostedService, there is an ordering hazard: if validation runs before box provisioning, the probe caches false, Replay is silently dead for the process, and the user gets a spurious "migrate your schema" warning about a schema that is about to be migrated.

The ValidatePipelines doc comment says "call this last in the Brighter builder chain", which happens to give the right IHostedService order — but that is load-bearing and not documented as such. Either call it out explicitly, or only memoize true so a false is re-probed.

Minor, same area: HandlerPipelineValidationRules calls the blocking SupportsCausationTracking() from what is ultimately an async StartAsync. No deadlock risk in a modern host, but the async variant exists and would be the better call.

(continued in next comment)

[claude]

(review 2 of 2)

6. DI factory that can return null

src/Paramore.Brighter.Extensions.DependencyInjection/ServiceCollectionExtensions.cs:464-466

sp => (sp.GetRequiredService<IAmAnOutbox>() as IAmACausationTrackingOutbox)!

The comment is admirably honest about this, and it does work with Microsoft.Extensions.DependencyInjection. But a factory returning null breaks the IServiceProvider contract, and third-party container adapters over IServiceCollection (Autofac, Lamar, Grace) do not all tolerate it. Since the only consumer is UseInboxHandlers optional ctor arg — and MS.DI already fills unregistered optional parameters with their default value — you could simply not register the descriptor on this path, or resolve through IAmAnOutboxProducerMediator.Outbox, which you have just exposed for validation anyway.

Also on the eager path (:332-336): Services.Add rather than TryAdd means a second AddProducers call duplicates the descriptor.

7. Schema-qualified table names silently disable the feature on MySQL and Spanner

MySqlQueries.cs:29 uses table_name = ... and SpannerQueries.cs:68 uses TABLE_NAME = ..., both comparing a bare identifier against information_schema. If OutBoxTableName is configured schema-qualified (mydb.Outbox), the probe returns false and Replay no-ops forever with no error. MSSQL OBJECT_ID and Postgres to_regclass both resolve qualified names correctly, so the behaviour is inconsistent across backends.

Also worth noting: these probes embed the configured table name inside a SQL string literal, so a name containing a single quote breaks out. Not user input, and the migration catalogs already guard with Identifiers.AssertSafe — but the query layer does not, and this is the first place a config value lands inside quotes rather than as an identifier.

8. Inbox and outbox thread the probe result differently

RelationalDatabaseInbox.cs:503-508 picks its SQL from the field (_causationColumnExists == true) while the parameter list two frames up was built from the method (CausationColumnExists()), relying on the caller having populated the memo. RelationDatabaseOutbox passes includeCausation through explicitly (:1330-1336), which is the better shape.

It is safe today — the memo only ever goes null to true or null to false, never flips — but "parameters and SQL derived from two different reads of a mutable field" is the kind of coupling that breaks silently under a future edit, and the failure is a parameter-count mismatch at runtime. Prefer the outboxs explicit thread-through in both.

9. Duplicated global-inbox logic in PipelineBuilder

TryCreateGlobalInboxAttribute re-implements the guards and attribute construction of AddGlobalInboxAttributes; its own XML comment names the drift risk it creates. Worth extracting the "which global inbox attribute applies to this handler?" decision into one method both paths call, so Describe cannot diverge from Build.

Tests

Coverage is strong — generated causation base tests across every backend, legacy un-migrated seeders, telemetry assertions, the validation rule, and the DynamoDB vanished-item case. Two gaps that map onto findings above:

• No test for "causation id found, zero outbox rows matched" (finding 4) — that is the case the current code reports as a successful replay.
• No test for two duplicates of the same request arriving concurrently on different consumers. The replay UPDATE has no concurrency guard, so both can reset dispatched state while the sweeper is mid-flight. Almost certainly benign under at-least-once, but it is the sort of thing the ADR should state explicitly rather than leave to the reader.

Nits

• RelationDatabaseOutbox.cs:1557 uses #if NETSTANDARD while :1506/:1535 use #if NETSTANDARD2_0. Both are correct for the current TFMs; the inconsistency is pre-existing in this file, just noting it since the new code had to pick one.
• Copyright headers on the new files read 2026, consistent with the other new files here — flagging only in case the convention is year of authorship.

Nothing here blocks the design, which I think is right: an optional role interface, a runtime schema probe rather than a static claim, and graceful degradation in the mixed-migration state. Findings 1-3 are the ones I would want addressed before merge; the rest are follow-ups.