Conversation
andrexibiza
left a comment
There was a problem hiding this comment.
Reviewed exact head be05a4f16e088e93900fbe251b06018ed5ce744d against live main@fa75692211001f64b2a65557fd498725eb0fe32e, plus the durable-ingress/output regressions, BasePlatformAdapter ledger boundary, startup recovery, run_turn streaming handoff, and the actual GatewayStreamConsumer transport paths.
The core transfer invariant is good work: once _record_delivery_obligation() owns every durable input member, retiring the ingress spool is ordered correctly; text/attachment component receipts stay independently recoverable; route-scoped receipts survive replay; and startup does not rerun model work for already-transferred inputs. The mixed-batch, compaction, identical-text, media-only, and attachment-recovery coverage is materially stronger than the current foundation.
I do have one P1 lifecycle blocker in the streamed path (inline). Streaming can create a real external platform effect before this PR publishes any durable output owner for the inbound batch. A process death in that gap leaves the input spool classified as untransferred, so restart can replay a turn whose stream is already partly/fully visible. This is not the same ambiguity the PR body already acknowledges (“send accepted, receipt not yet persisted”): in that documented case an obligation exists; here there is no output obligation at all, so the recovery authority still says the input is safe to execute again.
Required closure: publish an output/execution generation before the first externally visible stream effect, or durably settle that generation as indeterminate so recovery cannot blindly rerun it. Please add a deterministic abrupt-restart witness that pauses after a real stream send/edit/frame is accepted, terminates before the obligation write below, starts a fresh runner, and proves there is no second turn/effect execution and no duplicate terminal output. The witness needs to exercise the live streaming transport seam rather than beginning from an already-assumed text_already_delivered=True state.
Interlocks / provenance:
- #106316 is the merged queued-final delivery-ledger foundation, explicitly salvaged from #103754 with
AlexxRussellauthorship preserved. #107317 correctly extends that bracket from one raw inbound id to durable batch ownership. - #106709 (
fangliquanflq) is a direct overlapping carrier for attachment-manifest/media-only crash recovery. #107317 is broader, but if it becomes the canonical carrier, that work should be treated as overlapping/superseded-by-consolidation rather than erased as an unrelated duplicate. - #105007 (
moatinside) is complementary: it gates external streaming on canonical session persistence and explicitly leaves durable-spool/idempotency/unknown-delivery recovery as follow-up. It does not close this pre-output-owner crash window. - The existing streamed-delivery reconciliation work remains adjacent: reconciling a visible stream with later ledger redelivery is downstream of the missing ownership publication here, not a substitute for it.
The branch is currently 1 ahead / 4 behind live main. Hosted settlement is also absent on this exact head: CI 34471940944, Docker 34471939856, and Nix 34471939852 are all action_required with zero jobs, and the commit has zero check-runs. So the train is 0/1 commits proven hosted-green regardless of the strong local receipts in the PR body.
This is substantial architecture work. The durable input-member identity plus component-level output ledger is the right shape; close the pre-stream authority gap and get exact-head CI green, and this becomes a much stronger canonical carrier. 🚀
| if session_key and isinstance(adapter, BasePlatformAdapter): | ||
| media_files, _ = adapter.extract_media(response) | ||
| manifest = adapter._delivery_manifest(media_files, [], [], "[[as_document]]" in response) if deliver_media else [] | ||
| obligation_id, already_owned = await adapter._record_delivery_obligation( |
There was a problem hiding this comment.
P1 — this is too late for the text_already_delivered=True path. GatewayStreamConsumer has already driven real send_stream_frame / send / edit_message transport before _hmwa_deliver_turn_response() reaches this post-agent handoff. If the process dies after one of those external effects is accepted but before this call commits, the durable ingress record still has no output owner, so startup treats it as replayable and can execute a turn whose output is already real. Please publish an output/execution generation before the first stream effect (or durably mark it indeterminate) and add a crash/restart regression that kills the runner in exactly that gap.
Summary
Transfer every durable batch member to one authoritative recoverable output before retiring inbound spool records. Preserve original identities and route-scoped reply receipts through queueing, recovery and compaction. Recover confirmed text/attachment components through the existing sender without rerunning the model for already-transferred input.
This is a standalone PR against main. The independent Kanban lifecycle/summary change in #107315 touches disjoint files; neither PR requires the other.
Verification
On exact head be05a4f, the canonical per-file runner passed 387 tests across 36 files (0 failures). Coverage includes disk-backed crash/restart, mixed batches, fresh identical text, attachment failure/recovery, scoped reply receipts, compaction, queued/streamed finals, media and multiplex routing. Tests use synthetic identities and fake transports; no real messages or provider calls.
The combined integration acceptance run passed 526 tests with one platform skip before an additional media-diagnostic compatibility repair. Broad gateway testing exposed two media diagnostics regressions, now repaired and covered by the passing exact-head suite. It also exposed browser-control failures outside this change; a fresh upstream-baseline run reproduced one browser-control failure. A completely green full gateway suite is not claimed.
Retained pre-fix disk-backed replay regressions fail before the implementation and pass after it. Scope includes source, synthetic tests and design documentation only; no generated artifacts, runtime configuration, personal fixtures or archives.
Boundaries and risks
No runtime rollout, historical reconciliation, or merge is included.