Publish archive/restore lifecycle notifications only after the proposal commits (#2934)

[Chris0Jeky]

What

CardService.SetArchivedAsync published its card.archived / card.restored realtime event
immediately after its own SaveChangesAsync. On the proposal apply lane that SaveChangesAsync is
still inside AutomationExecutorService's outer transaction, so a later operation could fail, the
executor rolled the archive back, and SignalR subscribers had already been told about a transition
that never happened.

This adds a bounded transaction bridge and routes the lifecycle lane through it.

• DeferredBoardRealtimeNotifier (new, Taskdeck.Application/Services/) — an
  IBoardRealtimeNotifier that buffers instead of publishing. FlushAsync publishes the batch in
  staging order and empties the buffer before the first publish, so a downstream failure can never
  republish it; Discard drops it.
• OperationHandlerRegistry receives that buffer as a per-call parameter on
  ExecuteOperationAsync and hands it to SetArchivedAsync as notificationSink — the proposal lane
  only. It sits next to the recordLifecycleAudit: false argument from Align card lifecycle proposal side effects and audit receipts #3001, which is untouched.
• CardService.SetArchivedAsync gains one optional trailing-ish parameter,
  IBoardRealtimeNotifier? notificationSink = null. Null (every direct API call) means notify
  immediately, exactly as today. The card.updated events for children detached by an archive ride
  the same sink.
• AutomationExecutorService creates one buffer per execution (a local, never a field — this
  service is Scoped), flushes it as the first statement after CommitTransactionAsync, and discards
  it in a finally — one drain point covering every non-commit exit (the rollback returns, the two
  guard refusals, the unexpected-error catch, cancellation). The flush takes no cancellation token
  and is best-effort and logged: the board write is already durable, so neither an abandoned request
  nor a dead notification channel may turn an applied proposal into a reported failure.

Why this shape

The issue asked for a bounded bridge, not a notification redesign. The buffer holds one list, knows
nothing about transactions, and belongs to exactly one execution: it is created as a local inside
ExecuteProposalWithReceiptCoreAsync and passed down per call, so nothing depends on how any caller
sequences its executions. It mirrors the NotifyAssignmentsCommittedAsync pattern already in the
executor: stage inside the transaction, publish after the commit.

Not fixed here, by design: create / update / move on the proposal lane share the same premature
publish. Routing them through this bridge is not the one-line reuse the issue scoped in — each needs
its own sink parameter and call site on CreateCardAsync / UpdateCardAsync / MoveCardAsync, plus
their own tests. Left as-is and noted, per the issue's explicit instruction.

Checks run

Head is 449037027 after two review rounds. Round 1 (fresh-context adversarial + Codex):
two MEDIUM fixed, two LOW closed with tests, one declined. Round 2 (coordinator): the
notification buffer is now created per execution instead of living on the Scoped service,
the load-bearing flush catch got a test proving a dead notification channel cannot flip a
committed proposal to Failed, and the post-commit webhook durability trade is documented and
tracked in #3024. Full triage in the two review comments on this PR. The rows below are
from earlier heads unless marked otherwise; the round-2 comment carries the 449037027 counts.

The branch also merges origin/main (b93d2768f) — clean, no conflicts.

Check	Result
Application: ProposalLifecycleNotificationTests + DeferredBoardRealtimeNotifierTests	13 passed, 0 failed
Application: the above + AutomationExecutorService + OperationHandlerRegistry + ExecutionAuditRecorder classes	78 passed, 0 failed
Api: AutomationProposalsApiTests + CardAssignmentApiTests + McpToolsTests + ArchivedBoardProposalDecisionConcurrencyTests	144 passed, 0 failed
dotnet build backend/Taskdeck.sln -c Release	succeeded, 0 errors
dotnet test backend/Taskdeck.sln -c Release -m:1 (required backend check)	green on 449037027, exit 0, complete run. Domain 1676 · Application 4376 · Api 3174 (4 skipped) · Cli 243 · Architecture 28 (1 skipped) · Integration 7 (29 Docker-gated skips) — 0 failed anywhere. Detail in the gate comment on this PR.
node scripts/check-docs-governance.mjs	passed
node scripts/check-doc-links.mjs	passed (695 files, 0 broken links)

Non-vacuity checks (both probes reverted and re-run green afterwards):

• Sink wiring reverted to null → the three proposal-lane ordering cases fail (two on
  CommittedAtPublishTime, one on the rolled-back event being published); the two direct-API cases
  still pass.
• The flush catch body replaced with throw; → the channel-failure case fails on exactly its own
  assertion, "Expected result.IsSuccess to be True because the board change committed; a dead
  notification channel is not a failure, but found False."

Tests added

ProposalLifecycleNotificationTests drives the real CardService and the real
AutomationExecutorService over a mocked unit of work whose commit/rollback callbacks flip a flag,
and records each published event together with that flag at publish time — so the assertion is the
ordering, not merely that an event eventually appeared.

• archive-lifecycle followed by an operation the registry refuses → rollback, nothing published
  (and the archive provably applied first, so the empty list cannot pass vacuously);
• successful archive-lifecycle and successful restore-lifecycle → exactly one event each, with
  correct board/card ids and operation name, and CommittedAtPublishTime == true;
• an archive with a child → two events, both post-commit, in staging order (card.archived then the
  child's card.updated);
• an executor built with no notifier → the lifecycle event is still published, through the card
  service's own notifier;
• an executor wired to a notifier that always throws → still succeeds, still commits, never rolls
  back, proposal stays Applied;
• direct SetArchivedAsync both directions → published immediately, no transaction opened.

DeferredBoardRealtimeNotifierTests covers the bridge alone: staging, flush ordering, discard, no
republish after a flush, and no republish after a downstream channel throws mid-batch.

NOT verified

• No E2E or live-stack run: the SignalR hub and the frontend useBoardRealtime consumer were read
  but not exercised. The frontend behaviour is unchanged either way — it debounces a refetch and
  ignores operation semantics, which is why the issue rated this MEDIUM with no persisted corruption.
• Outbound webhook delivery rows for a proposal-lane lifecycle event now get written after the
  outer transaction commits rather than inside it. OutboundWebhookService.EnqueueBoardMutationAsync
  does its own SaveChangesAsync, so they persist — but that new ordering was verified by reading
  the code, not by a webhook integration test, and a host crash between the commit and the flush
  loses them with no retry. Documented on the autodoc sentence and tracked in Outbox for post-commit webhook deliveries (durability gap opened by #2934) #3024.
• The IBoardRealtimeNotifier now resolved into AutomationExecutorService by DI was verified by
  build and tests, not by starting the API host, and not by asserting which of the two public
  constructors the container selects. That selection is non-load-bearing by construction: when no
  notifier is supplied no buffer is created and the card service notifies directly, as before.
• The per-execution buffer removes the possibility of two executions sharing a list; no test
  actually drives two concurrent executions on one scope (the shared DbContext forbids it anyway).
• Docker-gated Taskdeck.Integration.Tests cases (29) stayed skipped locally, as they always do on this box.
• Create / update / move on the proposal lane remain unfixed (see above) — deliberately out of scope,
  not verified as safe.

Docs

One sentence added to autodoc/interfaces/proposal-operation-vocabulary.md on the archive-lifecycle
bullet. No docs/STATUS.md or masterplan change: this is an ordering bug fix, not a change in
shipped capability or sequencing.

Closes #2934

[chatgpt-codex-connector]

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-09-11T20:22:37.123161Z	8ea5111	New commits

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 49c9a582f2

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[Chris0Jeky]

Fresh-context adversarial review — findings and triage

One independent read-only review pass over this branch's diff and its seams (DI registration, the
composite/webhook notifiers, UnitOfWork, the batch lane, both new test files). No CRITICAL or
HIGH findings. Two MEDIUM items were real and both sat in lines this PR introduced, so both are
fixed in b652faed7; the LOW items are triaged below.

Fixed (MEDIUM)

1 — a committed archive could publish nothing at all. The flush ran after
NotifyAssignmentsCommittedAsync and took the caller's cancellationToken. An aborted request
(browser navigation, proxy timeout) in the post-commit window, or a transient throw in that
assignment call, reached the finally and discarded events describing an already-durable write —
strictly worse than the bug being fixed, because before this PR the event had already gone out.
The flush is now the first statement after CommitTransactionAsync and uses
CancellationToken.None: the write happened, so telling subscribers is not the caller's to cancel.

2 — the 6-argument constructor silently disabled the lane instead of falling back. It delegates
with realtimeNotifier: null, which wrapped NoOpBoardRealtimeNotifier and still handed that buffer
to the handler as the sink, so lifecycle events were staged into a black hole for every short-ctor
caller (McpToolsTests builds the executor that way). The buffer is now null when no notifier is
supplied, and a null buffer means the handler passes no sink — CardService notifies through its own
notifier exactly as it did before this bridge existed. New test pins it.

Production DI was not affected either way: AddScoped<IAutomationExecutorService, AutomationExecutorService>()
selects the 9-argument constructor (every parameter resolvable, the 6-argument parameter set a strict
subset, so no ambiguity). That is constructor-selection reasoning plus the fact that the Api proposal
tests resolve and exercise the endpoint — not a direct assertion on which constructor ran. The new
fallback removes the trap regardless of which one wins.

Fixed (LOW)

3 — vacuous-pass risk in the rollback test. Published.Should().BeEmpty() also holds if the
archive silently stopped applying. It now also asserts the card really archived and that a save
happened inside the transaction before the failure.

4 — the detached-children deferral was changed but untested, while the autodoc sentence claims
it. Added a case with one child: two events, both after the commit, in staging order
(card.archived then the child's card.updated).

Declined as non-blocking (tracked here, not fixed)

5 — FlushAsync has no per-event isolation. A throw on event n abandons n+1..N, and the
warning log names neither the events nor the count. Unreachable with the production wiring —
CompositeBoardRealtimeNotifier.NotifySafeAsync already swallows per channel — so this is a latent
contract gap, not a live defect. Adding a try/catch inside the loop would also change the
contract that DeferredBoardRealtimeNotifierTests.Flush_ShouldNotRepublish_WhenTheDownstreamChannelThrows
deliberately pins. Worth revisiting only if a different IBoardRealtimeNotifier is ever registered.

Out of scope, restated: create / update / move on the proposal lane share the same premature
publish. Routing them through this bridge needs a sink parameter and call site on each of
CreateCardAsync / UpdateCardAsync / MoveCardAsync plus their own tests — not the one-line reuse
#2934 scoped in, and deliberately left alone.

Verification on the fix head

• ProposalLifecycleNotificationTests + DeferredBoardRealtimeNotifierTests: 12 passed, 0 failed
  (was 10 before the two added cases).
• Full Taskdeck.Application.Tests and the Api proposal/archive classes were green on the previous
  head; ci-required re-runs the whole solution on this one.

[Chris0Jeky]

Coordinator's fresh-context independent review (round 1 for the merge gate) at exact head b652faed71c09f07c39174a98c73f92ffe3133c5 against base 4c479a7ff311912ffb96da027f6e525521bb70df: no CRITICAL/HIGH, three MEDIUMs, two of which go into the fix round because they guard the CRITICAL scenario. The author's own earlier review pass and the Codex thread are noted; this pass is the independent lens.

Refuted from source (read-only):

• DI constructor selection: the 9-parameter constructor's type set is a strict superset of the 6-parameter one, IBoardRealtimeNotifier is registered (composite), so the buffer is live in the API host; the MCP hosts register no executor.
• Committed proposal reported as failed: FlushDeferredNotificationsAsync catches, and the composite and webhook notifiers each swallow per channel; unchanged from base behaviour on notifier failure.
• Direct API timing unchanged (the sink parameter defaults to null and precedes CancellationToken, so a positional token would not compile).
• Discard coverage is complete (the finally covers every exit after BeginTransactionAsync; every earlier return precedes staging), and the batch lane executes sequentially, one transaction, flush and discard per item.
• Tests pin the ordering property with a recording notifier that captures commit state at publish time, guard the rollback case against vacuity, and cover flush-empties-before-publish.

Fix round (law 2a judgment: cheap, and each protects the committed-then-marked-failed path):

• MEDIUM: the buffer is cached on the Scoped service for its lifetime; the "one sequential execution" invariant holds only because the batch loop is sequential. Create the buffer per execution and pass it down.
• MEDIUM: no test exercises the guard that keeps a throwing notifier from reaching the outer catch (which would flip an Applied proposal to Failed). Add it; bind and log the exception with the proposal id.

Tracked, not fixed (law 2c):

• MEDIUM: webhook delivery rows now persist after the outer commit outside any transaction, so a crash between commit and flush loses the delivery row with no retry (before, the row was durable with the mutation; the same property already existed for post-commit assignment notifications). Tracked as a follow-up issue for an outbox, plus one clause in the autodoc sentence.
• LOW: flush is at-most-once on a bare throwing channel (by design, documented, tested); create/update/move/delete on the proposal lane still notify pre-commit (scoped out by the issue; follow-up).

[Chris0Jeky]

Round 2 (final fix round) — coordinator review

Fix commit 449037027, new head 449037027432d3fc27e9109a9d9839cee548b6c1. The branch also
carries a merge of origin/main (b93d2768f) taken before the fixes — clean, no conflicts; the diff
against the merge base is still only this PR's own files.

Fixed — buffer lifetime (MEDIUM)

DeferredBoardRealtimeNotifier was built once in the constructor of a Scoped service and shared
for its whole lifetime. "One sequential execution" held only because BatchProposalExecutionService
happens to iterate with a foreach — an invariant AutomationExecutorService cannot see and does not
own.

The executor now keeps the raw IBoardRealtimeNotifier and creates the buffer as a local inside
ExecuteProposalWithReceiptCoreAsync, passing it down through
OperationHandlerRegistry.ExecuteOperationAsync(operation, ct, actorUserId, deferredNotifications) —
per call, not per instance. Two concurrent executions on one scope can no longer append to the same
list. The registry loses its buffer field and constructor parameter (only the executor and two tests
constructed it, all with fewer arguments). DeferredBoardRealtimeNotifier itself is unchanged, as
asked.

Fixed — the load-bearing guard now has a test (MEDIUM)

The catch inside FlushDeferredNotificationsAsync was the only thing stopping a throwing notifier
from unwinding into the outer catch, rolling back an already-committed transaction and flipping an
Applied proposal to Failed — a committed board change reported as a failure. It had no test.

ExecuteProposal_ShouldStayApplied_WhenTheNotificationChannelThrowsAfterCommit: an executor wired to
an IBoardRealtimeNotifier that always throws still returns success, the flush really reached the
channel (Attempts == 1), commit observed, RollbackTransactionAsync never called, proposal entity
still Applied.

Verified non-vacuous: replacing the catch body with throw; fails that test on exactly the
intended assertion — "Expected result.IsSuccess to be True because the board change committed; a
dead notification channel is not a failure, but found False." Restored and re-run green.

Fixed — log detail (LOW)

The flush log now binds the exception and names the proposal. This file otherwise logs without
exception objects on purpose (AssertLogsContainNoSensitiveFailure enforces it for the
operation-failure paths), so to be explicit about why this one differs: it comes from a realtime or
webhook channel rather than persistence, it never reaches the caller's result,
CompositeBoardRealtimeNotifier already logs its channel exceptions the same way, and without it a
dropped notification is undiagnosable. The comment in the code says so.

Documented + tracked, not fixed — webhook durability (MEDIUM)

A lifecycle event's outbound webhook delivery rows are now written by the post-commit flush, outside
any transaction (EnqueueBoardMutationAsync does its own SaveChangesAsync). A host crash between
the commit and the flush loses them with no retry, where before they were durable with the mutation.
The realtime half self-heals — useBoardRealtime refetches — but a lost delivery row is a
permanently missed webhook.

One clause added to the autodoc/interfaces/proposal-operation-vocabulary.md sentence, and
#3024 — "Outbox for post-commit webhook deliveries" now tracks it, referencing #2934 and this PR.
That issue explicitly says reverting the ordering is not the fix.

Remaining LOWs — recorded, not fixed

• At-most-once flush on a bare throwing channel, by design. FlushAsync empties the buffer
  before the first publish, so a throw on event n abandons n+1..N. That is deliberate: the
  alternative is republishing a partially delivered batch. Unreachable with the production wiring
  (CompositeBoardRealtimeNotifier swallows per channel) and pinned by
  DeferredBoardRealtimeNotifierTests.Flush_ShouldNotRepublish_WhenTheDownstreamChannelThrows.
• Create / update / move / delete on the proposal lane still notify pre-commit — the identical
  defect with a wider blast radius. Out of scope per [Archive] Publish lifecycle notifications after proposal commit #2934's own wording ("fix them through the same
  bridge only if it is a one-line reuse; otherwise leave them and note it"): each needs its own sink
  parameter, call site and tests. The bridge is now in place for whoever picks it up. No issue exists
  for it yet; flagging rather than filing, since the coordinator asked only for the outbox issue.

Checks on 449037027

Check	Result
dotnet build backend/Taskdeck.sln -c Release	succeeded, 0 errors
Application: ProposalLifecycleNotificationTests + DeferredBoardRealtimeNotifierTests	13 passed, 0 failed
Application: the above + AutomationExecutorService + OperationHandlerRegistry + ExecutionAuditRecorder classes	78 passed, 0 failed
Api: AutomationProposalsApiTests + CardAssignmentApiTests + McpToolsTests + ArchivedBoardProposalDecisionConcurrencyTests	144 passed, 0 failed
node scripts/check-docs-governance.mjs	passed
node scripts/check-doc-links.mjs	passed (695 files, 0 broken links)
dotnet test backend/Taskdeck.sln -c Release -m:1	running — result posted below when it lands

CardAssignmentApiTests and McpToolsTests are in that Api filter on purpose: they are the two
call sites that construct OperationHandlerRegistry / the short executor constructor and would catch
the signature change or a silently-disabled bridge.

[Chris0Jeky]

Required backend gate — green on 449037027

dotnet test backend/Taskdeck.sln -c Release -m:1, exit 0, complete run (the box finally freed
up, so this is the whole solution rather than the partial run reported on the earlier head):

Project	Result
Taskdeck.Domain.Tests	1676 passed, 0 failed, 0 skipped
Taskdeck.Application.Tests	4376 passed, 0 failed, 0 skipped
Taskdeck.Api.Tests	3174 passed, 0 failed, 4 skipped
Taskdeck.Cli.Tests	243 passed, 0 failed, 0 skipped
Taskdeck.Architecture.Tests	28 passed, 0 failed, 1 skipped
Taskdeck.Integration.Tests	7 passed, 0 failed, 29 skipped (Docker-gated)

That closes the one "NOT verified" item that mattered in the PR body: every backend project now has
a local green on this exact head, layer purity included. The skips are the pre-existing
Docker-gated integration cases and one roadmap-invariant test, not anything this PR touches.

CI on the same head: 0 failed, remaining checks still running at the time of writing.

[Chris0Jeky]

Red check on this head: Smart CI / Required Gate — investigated, not caused by this PR

Flagging rather than dismissing, since law 1 says every failure gets investigated.

What failed:

### Smart CI / Required Gate — shadow — ❌ fail (would FAIL in enforce mode)
- ❌ `planner-error` — Error: pull-request planning requires merge SHA and tree SHA
     from the same fetched merge ref
- ℹ️ error-plan trust is pinned to T3; the event would classify T1
- ℹ️ escalated: planner-error

Why it is not this PR's code. That check ran three times on this same commit
449037027432d3fc27e9109a9d9839cee548b6c1:

Started	Result
19:44:34Z	success
19:46:09Z	success
20:08:30Z	failure (planner-error)

Identical tree, identical SHA, two greens then a red. The 20:08 run was triggered by a
pull_request edited event — I was updating this PR's body with the solution-gate results at
that moment — and the planner fetched the PR merge ref while GitHub was recomputing it, so the merge
SHA and tree SHA it read came from different states of refs/pull/3016/merge. The failure is in the
planner's own input, before it evaluates anything about the diff. The same check was green on both
earlier heads of this branch (49c9a582f2, b652faed7).

Standing of the check. Per CLAUDE.md: "ci-required.yml is the required CI gate; CI Extended
and the Smart CI shadow lane are advisory." The job's own output says shadow — would FAIL in enforce mode, i.e. it is reporting what enforce mode would do, not blocking.

Everything else on this head: 17 success, 11 skipped, 1 failure (the above), 1 still running
(API Integration (windows-latest)). No required backend or frontend check has failed.

Not fixed here. The CI region is R4-class under .claude/rules/ci-control.md and outside this
PR's scope, so I have not touched it. But the underlying fragility is real and reproducible in
shape: editing a PR's body can red a shadow gate that is green on the same commit, because the
planner re-runs on edited and races the merge-ref recomputation. Worth an issue against the Smart
CI planner — retry the merge-ref fetch, or read merge and tree SHA from one atomic ref read — but I
am leaving that call to the coordinator rather than filing into a region I do not own. A re-run of
the job on this head should also simply pass, as it did twice already.

[Chris0Jeky]

Round 2 (fix round) review at head 449037027432d3fc27e9109a9d9839cee548b6c1, fix commit 449037027: no merge blocker. Round ceiling reached (law 2d).

Scoped fresh-context verification of the fix:

• OperationHandlerRegistry is constructed in one production place and ExecuteOperationAsync called in one; both updated, and the three hops to SetCardArchivedAsync forward the buffer, so no proposal-lane path reverts to pre-commit notification; direct and test callers pass null and notify immediately as intended.
• The per-execution buffer is a local created before BeginTransactionAsync, in scope of the finally, and the same instance is flushed after commit; this removes the sharing hazard that the batch loop's reuse of one Scoped executor made real.
• The new guard test is non-vacuous: removing the flush catch unwinds into the outer catch, rolls back and returns UnexpectedError, flipping its assertions; Attempts == 1 proves the sink was reached.
• Flush-failure log binds the exception and a structured ProposalId; nothing under CI paths changed.

Coordinator's merge resolution (commit 8ea511153, no logic change): the branch had merged main before PRs #3019 and #3022 landed, so the head's vocabulary line lacked main's #2926 sentences and the executor lacked #3022's actorUserId: callerUserId. Merged current main keeping both sides of the vocabulary line and both executor changes, and dropped a stray UTF-8 BOM the fix commit had added to AutomationExecutorService.cs. Re-proved locally at the merged head: Application filter over the lifecycle-notification, deferred-notifier, audit-recorder and contract-validator classes, 90/90 passed; docs governance and link checks pass. The hosted rollup at the merged head is the gate.

Triage (law 2c): webhook durability trade tracked as #3024 with the autodoc clause; create/update/move/delete still notifying pre-commit on the proposal lane is follow-up material (no issue yet); the earlier Smart CI / Required Gate red was the advisory shadow lane racing a PR-body edit and touches nothing in this diff.