Pushed 3eeeeaaee8 — single commit, 10 files changed.

All delivery tests passing locally (82/82 across 3 test suites). No new type errors introduced — pnpm tsgo fails on main due to pre-existing skills.test-helpers.ts error (#57196, fix in #57198).

Greptile Summary

This PR enriches deliverOutboundPayloads's return type from a plain OutboundDeliveryResult[] to a DeliveryOutcome object carrying cancelledCount and allCancelledByHook, and introduces DeliveryError (wrapping partial-send failures with sentBeforeError) so callers can distinguish intentional hook suppression from genuine delivery failures. It also guards the cron retry loop from blindly retrying partial sends.

Key changes:

• New DeliveryOutcome interface and DeliveryError class exported from deliver.ts
• deliverOutboundPayloadsCore now tracks cancelledCount per-hook-cancellation and wraps mid-stream non-bestEffort throws in DeliveryError only when results.length > 0
• retryTransientDirectCronDelivery gains an err instanceof DeliveryError guard — necessary because summarizeDirectCronDeliveryError extracts .message from DeliveryError, which could match transient patterns and cause re-delivery of already-sent payloads
• All 5 return-value consumers updated to destructure { results }; 9 fire-and-forget callers left unchanged
• Comprehensive new tests covering hook-cancel-all, hook-cancel-partial, normal delivery, DeliveryError with sentBeforeError, and original-error passthrough for first-payload failures

Confidence Score: 5/5

Safe to merge — logic is correct, all 5 consumers updated, retry guard is sound, and test coverage is comprehensive

No P0 or P1 issues found. The DeliveryError is thrown only when results.length > 0, allCancelledByHook handles empty-payload edge cases correctly, the retry guard is actually required (not just defensive), and all consumer call-sites are updated.

No files require special attention

_{Reviews (2): Last reviewed commit: "fix: guard DeliveryError in retry loop, ..." | Re-trigger Greptile}

Addressed both Greptile findings in 597f376:

P1 — DeliveryError not guarded in retry loop: retryTransientDirectCronDelivery now bails on err instanceof DeliveryError so partial deliveries are never retried. Added DeliveryError import + importOriginal in the test mock to re-export the real class.

P2 — Silent try/catch assertion: Replaced bare try/catch with rejects.not.toBeInstanceOf(DeliveryError) so the assertion runs unconditionally.

All tests passing (17/17 lifecycle, 20/20 double-announce).

@greptileai please review again

Rebased onto current main (52a6e35), squashed to single commit 08648d6bb2.

Conflict resolved: deliver.lifecycle.test.ts was deleted upstream in adc329b ("test: dedupe extension-owned coverage") — its 12 original tests were duplicates of deliver.test.ts. Ported the 5 new DeliveryOutcome/DeliveryError tests into deliver.test.ts.

Changes since previous push (597f376c24):

1. AbortError preservation — DeliveryError no longer wraps AbortError when a non-bestEffort send is aborted after partial delivery. Without this, the outer queue wrapper would misclassify the abort as a failure and mark the write-ahead entry for replay, duplicating already-sent payloads. (Found by Codex review.)

2. Hook-cancelled cron deliveries treated as completed — dispatchCronDelivery now considers allCancelledByHook when setting delivered, so intentional hook suppression is cached and not replayed. Previously, hook-cancelled cron notifications would be replayed indefinitely.

3. Restart sentinel respects hook cancellation — deliverRestartSentinelNotice no longer retries when hooks intentionally cancel the notice.

4. New test — "preserves AbortError identity even after partial send" covers the AbortError edge case.

Known limitation: The cron scheduler can still replay failed non-bestEffort deliveries that had partial sends (scheduler-level replay, not the immediate retry loop). This is pre-existing behavior — the queue/scheduler doesn't support partial replay. The DeliveryError.sentBeforeError metadata enables future callers to handle this, but a full fix requires queue schema changes (tracked separately).

All delivery tests passing locally (50/50 deliver.test.ts, 20/20 delivery-dispatch.double-announce.test.ts). No new type errors — pre-existing skills.test-helpers.ts (#57196) and msteams failures on main.

@codex review

Pushed 3152cab683 — adds partial-send caching in dispatchCronDelivery.

When a non-bestEffort cron delivery partially sends (e.g. chunk 1 succeeds, chunk 2 fails with DeliveryError), the retry guard already prevented blind immediate retry. But without caching, the scheduler would replay the full run on the next pass, duplicating the sent payloads.

Now: DeliveryError with sentBeforeError.length > 0 is caught, the partial delivery is cached via rememberCompletedDirectCronDelivery, and a diagnostic log is emitted. This matches the trade-off bestEffort already makes — accept truncation over duplication.

New test: "caches partial DeliveryError sends to prevent scheduler replay duplication".

Codex review clean (round 4): "no discrete, actionable bug that would likely need fixing before landing."

@codex review

Codex Review: Didn't find any major issues. Breezy!

Rebased onto current main (d3eeadb). Clean rebase otherwise; a few caller-side updates were needed for upstream API drift since the original PR:

Resolved conflicts in delivery-dispatch.ts, deliver.test.ts, and delivery-dispatch.double-announce.test.ts (imports + a handful of tests upstream deleted).

Caller/test updates for upstream drift:

• deps: { sendWhatsApp } → deps: { whatsapp: sendWhatsApp } in 5 PR-added tests (upstream renamed the outbound deps key).
• Replaced bare logError() with the file's idiomatic logCronDeliveryError() (upstream moved to dynamic logger imports).
• Updated 4 mockResolvedValue sites to return the new DeliveryOutcome shape in downstream test suites (send.test.ts, delivery-dispatch.double-announce.test.ts).
• Dropped vi.resetModules() from deliver.test.ts beforeAll — it broke vi.spyOn module-instance identity for resolveAgentScopedOutboundMediaAccess.
• Tightened the shared-mock setup in "preserves AbortError identity" so both invocations get fresh AbortController + sendWhatsApp mocks, and use .toMatchObject({ name: "AbortError" }) instead of message-substring matching.

Second-order fixes surfaced during the rebase:

• gateway/server-methods/send.ts: hook-cancelled sends (allCancelledByHook: true) were returning UNAVAILABLE to gateway clients. Now return { ok: true, cancelledByHook: true }.
• delivery-dispatch.ts: hook-cancelled cron replies still called queueCronAwarenessSystemEvent, which would leak blocked (e.g. DLP-suppressed) content into the main session. Gated awareness on results.length > 0 instead of delivered.
• delivery-dispatch.ts: partial DeliveryError path was caching idempotency but reporting delivered: false, creating inconsistent cron state across restart. Now sets delivered = true on cache write; PR's own test expectation updated to match.

Targeted test sweep (116 tests across deliver.test.ts, delivery-dispatch.double-announce.test.ts, send.test.ts): all pass.

@codex review

Scope note after latest force-push

Force-pushed the rebased branch (9c5006314d) with a deliberately narrow scope. This comment records what's in this PR, what's not, and where the deferred bits are tracked.

In scope for #57843

• Return type: OutboundDeliveryResult[] → DeliveryOutcome (results, cancelledCount, allCancelledByHook).
• DeliveryError class with sentBeforeError for mid-stream non-bestEffort failures.
• All 5 callers that use the return value updated for destructuring.
• retryTransientDirectCronDelivery guards against DeliveryError so partial sends are not retried in the cron direct-delivery path.
• One small design tweak over the previous revision: DeliveryError is detected via a local isDeliveryError(err) helper that checks err.name === "DeliveryError" + Array.isArray(err.sentBeforeError), with DeliveryError imported as a type only. This keeps the heavy outbound delivery module off the delivery-dispatch.ts cold-startup import graph (preserving the existing loadDeliveryOutboundRuntime lazy boundary) and stays defensive against a third-party adapter throwing a same-named error.
• Test mocks updated to return the new DeliveryOutcome shape.
• Rebase-forced test adjustments to align with upstream b7e5d9a9 ("decouple outbound tests from bundled plugins") which deleted deliver.test-helpers.ts and removed WhatsApp from the default test registry.

Deliberately out of scope — deferred to follow-up PRs

While iterating on review feedback, three adjacent concerns surfaced that are legitimate but pre-existing — they're not regressions this PR introduces, they're bugs this PR's new error types merely make more discoverable. Keeping them out of #57843 to stay reviewable.

1. src/gateway/server-restart-sentinel.ts retry loop duplicates prefix on partial DeliveryError. The sentinel retries up to 45 times on any error. For a chunked restart notice where chunk 1 lands before chunk 2 fails, each retry re-sends the full message. Pre-fix(delivery): deliverOutboundPayloads return type cannot distinguish hook cancellations from failures #57766 this bug existed too (any mid-stream failure would retry-duplicate); the new DeliveryError name makes the guard cleanly expressible. Planned new PR: fix(gateway): do not retry DeliveryError in restart-sentinel retry loop.

2. Outer queue wrapper in src/infra/outbound/deliver.ts calls failDelivery() on DeliveryError. That leaves the entry pending; recoverPendingDeliveries() replays it on restart, duplicating the already-sent prefix. Pre-existing: failDelivery has always kept entries for retry regardless of error class. Natural home: fix(delivery): track and log silent delivery failures #53961 (fix/delivery-status-tracking) — the WAL-replay-duplicate is exactly the class of silent-failure bug it tracks. Will fold in when fix(delivery): track and log silent delivery failures #53961 rebases on top of this PR.

3. Gateway send returns ok: true without messageId when allCancelledByHook. callMessageGateway<{ messageId: string }> and downstream CLI renderers in commands/message-format.ts don't yet handle the cancelled-by-hook variant. Natural home: feat(delivery): surface deliveryStatus in --json output #57755 (feat/delivery-status-json) — its scope is surfacing delivery status through to JSON output, making "cancelled_by_hook" a visible third status alongside "delivered" and "failed".

Rationale and a preserved reference to the over-grown state (commit 97bb8a5841) are in a local tracker so the deferred fixes aren't lost.

Re-review progress:

• State: Complete
• Detail: The targeted re-review finished, the durable review comment was updated, and the synced verdict was routed.
• Run: https://github.com/openclaw/clawsweeper/actions/runs/25585220915
• Updated: 2026-05-08T23:58:28.174Z

Codex review: needs real behavior proof before merge.

Summary
The PR changes outbound delivery to return array-compatible DeliveryOutcome metadata, adds DeliveryError for partial sends, and updates cron, gateway send, restart-sentinel, queue recovery, tests, and changelog coverage.

Reproducibility: yes. from source inspection: current main returns a bare empty array when hooks cancel every payload and has no partial-send metadata for callers. I did not run tests in this read-only review, so this is source-reproducible rather than live-reproduced.

Real behavior proof
Needs stronger real behavior proof before merge: The PR body includes terminal output, but it runs a standalone script that copies the new contract rather than the actual patched OpenClaw delivery, WAL, recovery, or restart paths; after adding proof, updating the PR body should trigger a fresh ClawSweeper review, or a maintainer can comment @clawsweeper re-review. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, ask a maintainer to comment @clawsweeper re-review.

Next step before merge
Needs contributor proof from actual patched behavior plus a focused fix for the missed restart-continuation cancellation path, so this is not ready for a repair-lane marker.

Security
Cleared: No concrete security or supply-chain concern found; the diff does not add dependencies, workflows, permissions, secret handling, or downloaded code.

Review findings

• [P2] Handle hook-cancelled restart continuations — src/gateway/server-restart-sentinel.ts:144

Rebase + iteration update — 2026-05-07

Force-pushed a80c72ea07 rebased onto current upstream/main.

What changed since the previous push (9c5006314d):

1. Public API kept back-compatible. deliverOutboundPayloads still resolves to a value usable as OutboundDeliveryResult[] (.length, .at(-1), [i], for...of all work unchanged). DeliveryOutcome is now OutboundDeliveryResult[] & { cancelledCount?, allCancelledByHook? } with metadata attached as non-enumerable own properties — invisible to toEqual/toMatchObject/Object.entries/JSON.stringify, accessible via outcome.allCancelledByHook. Existing plugin SDK consumers and test mocks that return plain arrays continue to satisfy the type. Addresses the SDK-compat concern raised in the prior round.

2. WAL queue ack on partial-send DeliveryError. Both code paths that can land a partial prefix and then throw now ack the queue entry instead of failing it, so a future drain/restart cannot replay the whole batch and duplicate the already-sent prefix:

   • Live-send wrapper in deliverOutboundPayloads (src/infra/outbound/deliver.ts).
   • Recovery replay in drainQueuedEntry (src/infra/outbound/delivery-queue-recovery.ts).
   • Restart-sentinel retry loop (src/gateway/server-restart-sentinel.ts).

3. Fresh test coverage for the ack-on-DeliveryError invariants in src/infra/outbound/deliver.test.ts, src/infra/outbound/delivery-queue.recovery.test.ts, and src/gateway/server-restart-sentinel.test.ts.

Local validation: 352 targeted tests pass across 20 files (pnpm test src/infra/outbound/deliver.test.ts src/infra/outbound/delivery-queue.recovery.test.ts src/cron/isolated-agent/delivery-dispatch.double-announce.test.ts src/gateway/server-restart-sentinel.test.ts src/gateway/server-methods/send.test.ts ...). Five rounds of codex review --base upstream/main ending clean.

Diff size: 11 files / +547/−60 vs upstream/main.

Sequencing note: #53961 and #57755 will follow once this lands; both will rebase to consume DeliveryOutcome.allCancelledByHook and DeliveryError.sentBeforeError for cleaner status reporting.

🤖 AI-assisted (Claude Code, fully tested locally; codex review iterated 5 rounds). Co-author trailer in commit.

Rebase update — 2026-05-08

Rebased onto current upstream/main. CHANGELOG.md conflict resolved (test files auto-merged cleanly).

Codex review surfaced two P2 findings inside #57843's stated scope (DeliveryError + WAL ack), both fixed in this push:

• Run runOutboundDeliveryCommitHooks for the sentBeforeError prefix before treating the delivery as terminal — both the live-send wrapper in deliver.ts (queueId path + skipQueue path) and the recovery loop in delivery-queue-recovery.ts. Without this the WAL ack would silently drop afterCommit-style message-lifecycle side effects for messages that already landed.
• During recovery replay, if ackDelivery fails for a non-ENOENT reason after a partial-send DeliveryError, move the entry to failed/ instead of leaving it in pending/ — otherwise a transient ack failure could cause the next drain to replay and duplicate the prefix this branch was protecting.

Added one new test (delivery-queue.recovery.test.ts) asserting commit hooks fire for the sent prefix during recovery partial-send. All 220 targeted tests pass; codex review clean on round 3.

One adjacent codex finding deferred to a follow-up PR: adapter-internal multi-chunk partial sends (e.g. Signal's sendFormattedSignalText batching chunks before returning) — fixing it requires changing the formatted-text adapter contract and is sibling-scope rather than part of this PR's "DeliveryError + ack-on-DeliveryError" story.

AI-assisted (Claude Code).

Thanks for pushing this through and for documenting the failure modes. The underlying problem is real: hook-suppressed sends must not look like delivery failures, and partial sends need to be visible so retry/recovery paths do not duplicate already-delivered messages.

I'm going to close this PR as superseded rather than merge this branch. We took a broader refactor of the existing message lifecycle API instead of adding a second delivery-result contract on top of deliverOutboundPayloads.

What we did instead:

• Kept sendDurableMessageBatch(...) as the canonical API and enriched its return value with explicit statuses: sent, suppressed, partial_failed, and failed.
• Added per-payload payloadOutcomes, including stable suppression reasons like cancelled_by_message_sending_hook, empty_after_message_sending_hook, and no_visible_payload.
• Preserved hook-provided cancellation reason and metadata on suppressed outcomes.
• Represented partial sends as partial_failed with the delivered prefix plus the failure, so callers can make retry/status decisions without guessing from an empty array.
• Kept direct deliverOutboundPayloads(...) as deprecated compatibility substrate instead of making it the public shape for new code.
• Migrated bundled send/turn paths to the durable message lifecycle and away from deprecated direct delivery/reply-dispatch helpers.
• Added check:deprecated-message-api, wired into check:architecture, so production code cannot drift back to the deprecated message APIs outside compatibility shims.
• Avoided introducing parallel convenience wrappers; the API remains sendDurableMessageBatch(...) rather than adding sendDurableMessageBatchResults(...), sendDurableMessageBatchOrThrow(...), or another turn-dispatch wrapper.

That should cover the needs this PR raised, including the delivery-status follow-ups, but with one canonical message API instead of two overlapping contracts. Closing this branch to keep the public SDK surface clean.

Verification on the replacement refactor:

• pnpm check:deprecated-message-api
• OPENCLAW_TESTBOX=0 pnpm tsgo:core
• OPENCLAW_TESTBOX=0 pnpm tsgo:extensions
• focused message send/hooks + QA/Nextcloud Talk/IRC/Line tests
• pnpm plugin-sdk:api:gen && pnpm plugin-sdk:api:check
• pnpm check:docs
• pnpm check:architecture
• Testbox pnpm check:changed: https://github.com/openclaw/openclaw/actions/runs/25590359552