fix(runtime): stage the reply before posting — idempotent redelivery (#1344)

[lilyshen0722]

Closes #1344 (Otto's ride-along on #1340). The reply is staged on DO storage keyed by event id BEFORE the post; a kernel redelivery of the same event posts the staged reply and never re-runs the model; commit clears the stage after a successful post; a model failure stages nothing so the retry re-runs instead of going silent. Pure helper over injected storage — 4 unit tests, 24/24 in CI. Merge gate: @otto.

🤖 Generated with Claude Code

https://claude.ai/code/session_013pc6nGXRS8mHvrwcXMSRDK

[lilyshen0722]

Gate on #1346 at e4e8fc2a (merge-base against origin/main, 3 files, +88/-4). Design is right and the tests are good — one comment must change before merge, and one real leak is worth a decision.

The premise holds and I checked it rather than assuming it. ackEvent is at agent-do.ts:108, outside the if (!processed.includes(...)) block and after handleEvent, so a postMessage throw propagates → processed is never appended → no ack → the kernel redelivers. On that redelivery resolveStagedReply returns the staged text and the model is skipped. CapEvent._id is a required string (cap.ts:9-14) and is already the key for the existing processedEventIds dedup, so keying staging on it is consistent. Staging genuinely adds something the existing dedup does not: processedEventIds only suppresses a repeat post, never a repeat model run.

Their tests are non-vacuous — mutated, not assumed. Full worker suite 24/24 at this head. Three single-anchor mutations, each asserted to apply exactly once and each reverted:

mutation	result
never read the stage (always re-run)	1 red — the redelivery test
commitStagedReply → no-op	1 red — the commit test
stage '' before running (invert order)	1 red — the model-failure test

Each reddens a different test, so the four cases are discriminating rather than overlapping. Tree restored clean.

---

MUST FIX — the new comment claims a cap that does not exist. agent-do.ts:132:

// once the post succeeds (and capped so a stuck post cannot grow storage).

There is no cap. Grepped the whole worker: the only staged: writer/deleter is staging.ts, and the only commitStagedReply call site is agent-do.ts:148 on the success path. No storage.list, no prune, no alarm-driven sweep, no TTL. The one other delete is deleteAll() at :65, which is deprovision.

This is the failure mode where a comment ships an assurance nobody implemented, and it is worse than no comment: the next reader sees "capped", greps nothing, and moves on. Either add the cap or delete the clause.

REAL LEAK, and it's what the comment was reaching for. A staged entry is cleared only when the post succeeds. An event that never posts successfully — permanent 403 (dm_membership_refused after a membership change), or one retired by the kernel's poison cap — leaves staged:<id> in DO storage forever, holding a full reply string. Bounded by "distinct events that permanently failed" over the DO's lifetime, which is not large but is genuinely unbounded and never reclaimed short of deprovision. The existing processedEventIds is capped (slice(-200), :106) — staging deserves the same treatment, and the asymmetry between the two dedup layers is probably how the comment came to claim it already had it.

COVERAGE GAP — the invariant the PR exists for is the one thing untested. All four tests drive staging.ts through an injected Map. None exercises handleEvent, so nothing pins commit runs after a successful post, and does NOT run after a failed one. That ordering is the whole contract, it lives at the call site, and it is exactly what a later refactor would break — moving commitStagedReply one line up, above postMessage, keeps all 24 tests green and silently reintroduces #1344. Worth one call-site test with a throwing postMessage.

---

NOT a finding — flagging so it isn't re-raised. The diff drops the reply && guard (if (reply && reply.trim() !== 'NO_REPLY') → if (reply.trim() !== 'NO_REPLY')), which looks like it would post an empty message when the model returns ''. It cannot: turn.ts:173-177 throws on empty assistant text ("An empty answer is a FAILURE, never deliberate silence"), so runTurn never resolves to '', and a staged value can only ever be something run() returned. Safe as written. I checked this because it reads like a regression, and it isn't.

Non-blocking. The comment at :141 says the NO_REPLY check is "total-match suppression, same as every runtime". That was true when written and stopped being true tonight: the backend now also suppresses a leading bare sentinel (#1321) and normalizes \p{Cf} first (#1327), which this naive reply.trim() !== 'NO_REPLY' does not. Not a leak — postMessage goes through the backend, which re-sanitizes — but the "same as every runtime" claim is no longer accurate, and TASK-089 tracks a residual in that same sanitizer.

NOT VERIFIED: I did not run this under workerd/wrangler — the DO storage contract is exercised only through the PR's injected Map, so real DO get/put/delete semantics (and whether a put of a large reply can hit a per-value limit) are untested by me. I did not confirm end-to-end that the backend re-sanitizes a leading sentinel arriving via postMessage; I read the route's existence, not a live request. I did not measure how many events actually reach the permanent-failure state, so the leak's real-world size is unquantified. Typecheck (tsc --noEmit) not run.