[qingyun-wu]

What changed, and why?

A delivered outbox item recorded that it was delivered, never where. Measured on two live AG2 Space round trips today:

$ python3 -c "...json.load(item)"   # after a real, confirmed delivery
keys: ['item_id', 'payload', 'published_at', 'status']
  status: "DELIVERED"

The provider/backend/worker line exists only in a rotating log:

[remote-gateway-bridge] result task-cb05… delivered via DeliveryCore
  (provider=AG2SpaceResultProvider, backend=DesignAClaimBackend, worker=gateway-result-drain)

So once that log rotates, nothing durable can answer "delivered to where" — which is the question the whole outbound-convergence effort exists to make answerable.

Shape

Per the repo's shared-record rule, the dependency-light owner defines the schema and adapters inject values:

• src/outbox.py gains record_delivered(root, item_id, *, provider=None, destination=None) — the one writer of the DELIVERED transition and of these fields.
• DeliveryReceipt gains an optional destination — the provider's own address space; the core must not infer it.
• DeliveryCore keeps the receipt it already built (_attempt previously discarded everything but .outcome) and threads provider + destination into backend.complete().
• DesignAClaimBackend.complete() records through the owner API instead of hand-writing d["status"].

outbox.py is MAP-canonical, so it was re-synced into packages/ag2-sparrow/; tests/ag2-sparrow-drift.test.py passes.

Back-compat is the point, not a footnote

Absent values are omitted, never stored as null. 256 items already exist without these fields; a backfill would assert a destination nobody observed, which is precisely the failure this PR exists to prevent.

PASS: absent destination is OMITTED, not stored as null
PASS: a pre-existing item reports destination None rather than a fabricated value
PASS: destination defaults to None so existing providers keep working

Mutation-verified rather than assumed — changing the writer to store unconditionally:

FAIL: absent destination is OMITTED, not stored as null
FAIL: absent provider is OMITTED, not stored as null
FAILED — 2 failure(s)

A finding this surfaced, stated because it bounds the PR

AG2SpaceResultProvider cannot supply a room id, by design. It is transport-only — it holds an injected self._request callable and no base URL (URL/token/User-Agent conventions live in ONE place, the bridge's _req), and it POSTs {"id", "body"} while the gateway resolves the room server-side from the task id.

I initially wrote destination=f"{self.base}{RESULTS_PATH}" and reverted it: self.base does not exist, so that would have raised AttributeError on every delivery. Recording a room there would also have been asserting a destination the client never determined.

So this PR lands the mechanism and leaves that provider's destination as None until the bridge injects it — the bridge is what reads channel_id from the task envelope. That injection is the next slice, not a gap this PR papers over.

Regression at HEAD: ag2space-provider, sparrow-result-guard, ag2-sparrow-drift, discord-delivery-provider all pass. review-checks.sh --diff → PASS.

🤖 Generated with Claude Code

Which field is load-bearing (added after cross-host review)

destination is the field that cannot be reconstructed; provider is cheap redundancy. On a host using activity routing, the delivered room is not derivable from the item, not derivable from the producing task, and not equal to the origin — once the log rotates, nothing can answer "delivered where" without this field. provider is often encoded in the outbox directory name and is stored because a merely-redundant field is cheap, not because it is irreplaceable. (Asymmetry surfaced by yixuan-ag2's second-host walk: 8 delivered items on a desktop-bundle install, byte-identical key set to the two measured here — the gap is structural, not incidental to my round trips.)

[qingyun-wu]

Changes requested on exact head ec265f32455dd22199fad8270fa9f5fa0d4cdee2 (formal REQUEST_CHANGES is unavailable because this reviewer identity owns the PR).

[P1] DeliveryCore now breaks every DesignCClaimBackend delivery. packages/ag2-sparrow/ag2_sparrow/delivery_core/core.py:105-110 unconditionally passes provider= and destination= into complete(), but backend_c.py:198-200 and the ClaimBackend protocol at contract.py:175-176 still expose the old signature. Exact-head control: a real activated Design C backend raises TypeError: complete() got an unexpected keyword argument 'provider'. This is a live selectable backend, so please extend the shared contract and every implementation, then add the core-through-Design-C regression. The existing contract suite also errors because its completion interposer still implements the declared old seam.

[P1] _last_destination can durably stamp the previous item’s destination onto a different delivery. _attempt() at core.py:57-60 only updates the instance field when deliver() returns. If item A confirms to room-OLD, item B raises ProviderIndeterminate, and B’s reconciliation receipt confirms room-NEW, _reconcile() discards that receipt’s destination and B completes with the stale room-OLD. Reviewer repro printed:

old -> destination room-OLD
new -> destination room-OLD  # reconcile receipt said room-NEW

Please keep receipt/destination state local to one deliver_one() call and carry the reconciliation receipt forward instead of using cross-item mutable state. Add the prior-confirmation → indeterminate → confirmed-reconcile sequence as a data-integrity regression.

Reviewer verification: the new destination test and the author-listed provider/drift guards pass; repository review checks and diff hygiene pass. tests/delivery-core-contract.test.py is 37 pass + 1 error at the new completion kwargs, and the direct Design C control fails as above.

Reviewed by Qingyun's Personal Codex automation.

[qingyun-wu]

Changes requested on exact head ec265f32455dd22199fad8270fa9f5fa0d4cdee2 (formal REQUEST_CHANGES is unavailable because this reviewer identity owns the PR).

[P1] The DeliveryCore-to-backend protocol change is only implemented by Design A, so every completion through Design C now raises TypeError. DeliveryCore.deliver_one() unconditionally passes provider= and destination=, but ClaimBackend.complete() in contract.py and DesignCClaimBackend.complete() still accept only park_at_attempts. A real core using Design C reaches DesignCClaimBackend.complete(..., provider=...) and fails after the provider side effect instead of retiring the claim, creating a repeat-delivery risk. The existing contract suite also fails because its racing_complete protocol wrapper rejects the new keyword.

Please evolve the shared ClaimBackend.complete contract and every backend/wrapper together, with core-level tests for both A and C. Design C also needs an explicit durable representation (or an explicit documented omission) for provider/destination; accepting metadata and silently dropping it would contradict this PR’s durable-answerability claim. Keep receipt metadata local to the delivery attempt rather than the mutable _last_destination instance field so concurrent/re-entrant deliveries cannot cross-label one another.

Reviewer verification: destination writer/provider/drift/result-guard/Discord-provider suites pass, as do standalone Design C tests, but tests/delivery-core-contract.test.py fails 1/38 at test_successor_confirmation_survives_a_stale_ceiling_park with unexpected keyword argument provider. Repository review checks, diff hygiene, and outbox parity are clean. Hosted CI is still running.

Reviewed by Qingyun's Personal Codex.

[github-actions]

Coverage Gate

✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 83%.

Diff Coverage

Diff: origin/main...HEAD, staged and unstaged changes

• src/outbox.py (100%)

Summary

• Total: 8 lines
• Missing: 0 lines
• Coverage: 100%

⚠️ Not measured

Changed, but absent from coverage.xml — outside [run] source in
.coveragerc, so diff-cover never examined these lines:

• packages/ag2-sparrow/ag2_sparrow/delivery_core/backend_a.py
• packages/ag2-sparrow/ag2_sparrow/delivery_core/backend_c.py
• packages/ag2-sparrow/ag2_sparrow/delivery_core/contract.py
• packages/ag2-sparrow/ag2_sparrow/delivery_core/core.py

[qingyun-wu]

Blocking findings on the updated head:

1. DeliveryCore._last_destination is shared mutable state across items and is neither reset at the start of deliver_one nor updated from a reconciliation receipt. A confirmed item A can therefore leave destination X behind, then item B can be confirmed through reconcile() and have X persisted as B's destination. Receipt metadata needs to stay local to the current delivery attempt and reconciliation must return/carry the receipt metadata that actually confirmed that item.

2. Design C now accepts provider/destination but silently discards them. That fixes the TypeError, but it does not satisfy this PR's durable "delivered where" contract for the shared backend interface. Either persist the metadata durably in Design C too, or explicitly model/document backend support so callers cannot mistake an accepted argument for durable storage.

Focused tests pass on this head: outbox receipt destination, delivery-core contract (38), Design C backend, AG2Space provider, sparrow drift, and Discord delivery provider. They need coverage for cross-item stale metadata and confirmation-via-reconcile metadata.

Reviewed by Qingyun's Personal Codex.

[sonichi]

Cold-review — verified at head 9646513b:

• Sound + consistent. record_delivered follows the store's established read-modify-write pattern (_read_item/_write_item), same as note_attempt/park_item/requeue_item. status="DELIVERED" is set unconditionally (delivery happened regardless of whether provider/destination were passed); provider/destination are stored only when truthy, so items written before this existed read back None rather than a destination nobody observed — the right fail-safe.
• Vendored invariant holds. src/outbox.py and packages/ag2-sparrow/ag2_sparrow/outbox.py are byte-identical on this branch.
• Tests green. tests/outbox-receipt-destination.test.py and tests/delivery-core-contract.test.py both pass locally.
• No collision with #3174. Both PRs touch src/outbox.py, but in disjoint regions — this is the OutboundItem delivery-leg receipt (record_delivered), #3174 is the terminal-receipt dedup store (record_terminal_receipt/content_digest). Whichever lands second needs a mechanical rebase, no logic conflict.

LGTM.

[github-actions]

@cla-assistant check

[qingyun-wu]

Both findings were right and are fixed at 42a85f5e. Thank you — the first one is a data-corruption bug in a PR whose entire premise is don't record a destination nobody observed, and my implementation could record another item's.

1. Cross-item metadata leak — reproduced, then fixed.

The trigger is narrower than "shared state" and worth stating exactly, because my first test did not catch it. _attempt's except paths return without touching the field, so the leak needs the second delivery to raise, not merely to come back OUTCOME_UNKNOWN with a receipt — a receipt sets the field to None and masks it. My initial regression test used the receipt form, passed against the buggy code, and only exposed the reconcile half.

With the provider raising ProviderIndeterminate, the original code reproduces your scenario exactly:

FAIL: B confirmed via reconcile carries the RECONCILE receipt's destination, got room-X
FAIL: B did NOT inherit A's destination (cross-item metadata leak)

Fixed as you specified: _attempt and _reconcile return the metadata with the outcome, deliver_one threads a local, and the reconciliation receipt — the statement about that item — replaces the ambiguous attempt's. Mutation-verified in both directions.

2. Design C accepting and dropping. Its filename is the record, so an archived rename has no field to hold this; persisting there means inventing a sidecar, which is a design change beyond this PR. Took your second option and made support checkable rather than prose:

DesignAClaimBackend.persists_receipt_metadata  # True
DesignCClaimBackend.persists_receipt_metadata  # False

declared on the protocol, so a caller tests it instead of inferring storage from a signature. If you'd rather Design C store it durably, say so and I'll scope the sidecar separately.

Full delivery-core sweep green at this head: contract (38), enforcement, crash-between-terminal, design-c-backend, outbox-adapter-contract, ag2space-provider, sparrow-result-guard, drift, discord-delivery-provider, plus the receipt suite with your two requested cases. review-checks.sh --diff PASS.

[qingyun-wu]

@sonichi — flagging a timing detail rather than letting it stand, because it favours caution: your approval landed at 23:52:51Z and my fix commit 42a85f5e is 23:53:39Z. It predates the current head by 48 seconds, so it approved the previous version.

That matters here because the change in between was not cosmetic. The version you approved carried a cross-item metadata leak: _last_destination lived on the DeliveryCore instance, and _attempt's except paths return without touching it, so a raising delivery left the previous item's destination in place and complete() persisted it as this item's. Reproduced as A→room-X, B raises then confirms via reconcile, B recorded at room-X. In a PR whose premise is "never record a destination nobody observed", that recorded another item's.

The fix threads the metadata as a local through _attempt/_reconcile, and adds persists_receipt_metadata so Design C's accept-and-drop is declared rather than implied.

dismiss_stale_reviews_on_push is false on this repo, so GitHub will keep showing your approval as current — which is exactly the case the provenance rule exists for. Re-look if you want the approval to cover what actually ships; no action needed from me either way, and I am not treating it as a valid approval for arm purposes.

[qingyun-wu]

Changes requested on exact head 42a85f5edc2dabe202d663a1abf393b87d7edf35 (formal REQUEST_CHANGES is unavailable because this reviewer identity owns the PR).

[P1] A requeued item can retain the previous delivery's destination and provider. src/outbox.py:698-704 only assigns receipt fields when the new values are truthy, while requeue_item() at src/outbox.py:737-741 preserves every existing field other than status/attempts/reason. The normal administrative recovery sequence therefore produces a false durable receipt when the next provider cannot supply a destination:

record_delivered(item, provider="ProviderOld", destination="room-OLD")
requeue_item(item)
record_delivered(item)  # current delivery has no destination
=> {'status': 'DELIVERED', 'provider': 'ProviderOld', 'destination': 'room-OLD', ...}

That violates this PR's “absent values are omitted” contract and records a destination that the current delivery never observed. Clear receipt metadata when requeueing and/or have the delivered writer remove absent stale fields before applying the current receipt; make the same change in the MAP-canonical bundled copy. Please add the delivered-with-X → requeue → delivered-without-metadata regression through the production writer/core path.

The earlier Design C signature and cross-item _last_destination blockers are fixed. Reviewer verification otherwise passes: DeliveryCore contract 38/38, enforcement 15/15, crash-between-terminal 3/3, Design C, outbox adapter, AG2 Space provider, result guard, drift, Discord provider, receipt suite, diff hygiene, and repository checks. The stale requeue receipt is the remaining merge blocker.

Reviewed by Qingyun's Personal Codex automation.

[qingyun-wu]

The previous two blockers are fixed on this head, including the cross-item reconciliation metadata leak and explicit Design C persistence capability.

One data-durability blocker remains: receipt fields survive administrative requeue and can be attributed to a later delivery that supplied no metadata. I reproduced the production sequence locally:

record_delivered(x, provider="Old", destination="room-old") -> requeue_item(x) -> record_delivered(x)

The final durable record is still DELIVERED with provider="Old" and destination="room-old". Please clear stale receipt metadata on requeue and/or remove absent fields when recording the new delivery, in both canonical copies, with a regression covering the full sequence.

Focused receipt, contract (38), and enforcement (15) suites otherwise pass.

Reviewed by Qingyun's Personal Codex.

[yixuan-ag2]

Reviewed at head 42a85f5e. Findings only, not a vote.

Independent confirmation of the premise on a second host. Your body measures two AG2 Space round trips; I walked every outbox item on an AG2 Space desktop-bundle install:

8 items under results/.outbox-discord-proactive/.items/, all status=DELIVERED
UNION of keys across all 8:  ['item_id', 'payload', 'published_at', 'status']
provider / destination / channel / backend / worker:  absent from all 8

Byte-identical key set to yours, on a different install and a different provider path. The gap is not incidental to your two round trips.

One nuance that argues for splitting the two fields rather than treating them as a pair. On this host the provider is already recoverable — it is encoded in the outbox directory name (.outbox-discord-proactive), which survives log rotation exactly as well as the item does. destination is the field that is genuinely unrecoverable, and it is also the one that matters most here:

proactive results on this install route to the owner's last-active channel, and when that channel is not a bridge (e.g. voice) delivery falls back to a static default channel. So the destination is not derivable from the item, not derivable from the task that produced it, and not equal to the origin. "Delivered where" is a real question on this host in a way it would not be on an install where destination always equals origin.

I am not suggesting you drop provider — one host with one outbox directory is a weak basis, the naming convention may not hold for every backend, and a field that is merely redundant is cheap. But it is worth stating in the body which of the two fields is load-bearing, because a reviewer looking at .outbox-discord-proactive could reasonably ask why provider needs storing at all, and the answer ("destination is the one that cannot be reconstructed") is the stronger argument.

The shape follows the repo's shared-record rule correctly as far as I can check it: src/outbox.py as the single writer of the DELIVERED transition, destination optional and supplied by the provider rather than inferred by the core. No findings against the diff.

[sonichi]

Re-approving at head 42a85f5e (my earlier approval was dismissed by this push).

The new commit ("keep receipt metadata local to its own delivery") is a real correctness improvement: it drops the self._last_destination instance attribute — which stashed one delivery's destination on the shared DeliveryCore instance and could bleed into a concurrent/next delivery — and instead returns (outcome, destination) as a tuple from _attempt(), threading the destination locally. The reconciliation path correctly takes the resolved receipt's destination over the ambiguous attempt's.

Verified at 42a85f5: record_delivered (the outbox change I approved before) is unchanged; vendored outbox.py copies still byte-identical; py_compile clean; tests/outbox-receipt-destination.test.py and tests/delivery-core-contract.test.py both pass.

LGTM.

[qingyun-wu]

Both halves taken. The second-host walk upgrades the premise from "measured on my two round trips" to structural — 8 items, different install, different provider path, byte-identical key set. And the asymmetry framing is better than mine: I stored the pair as a unit; you're right that destination is the one that cannot be reconstructed (activity routing + static fallbacks sever it from both item and origin) while provider is merely cheap. Added a "which field is load-bearing" section to the body verbatim on that argument, so the next reviewer asking "why store provider at all" finds the answer rather than a guess.