feat(delivery): surface deliveryStatus in --json output#57755
Conversation
Greptile SummaryThis PR surfaces Key points:
Confidence Score: 5/5Safe to merge — all prior concerns resolved, no P0/P1 issues remain. Both P1/P2 issues from the previous review round are fixed. The implementation correctly handles all code paths with no risk of double-emit, and the test suite now provides genuine coverage of the non-bestEffort throw invariant and the no-payload early-return branch. No new logic or security issues were introduced. No files require special attention.
|
| Filename | Overview |
|---|---|
| src/agents/command/delivery.ts | Adds emitJsonEnvelope helper, deliveryStatus tracking state, and emits structured status before any re-throw or return. Logic is correct across all code paths (no-payload, no-deliver, successful delivery, zero results, bestEffort error, non-bestEffort throw). |
| src/agents/command/delivery.test.ts | Adds two describe blocks covering status-tracking and JSON output; previous no-op and wrong-path issues are fixed. Tests cover all documented scenarios including the critical non-bestEffort throw invariant and the no-payload early-return path. |
Reviews (2): Last reviewed commit: "fix(test): address Greptile P1+P2 in del..." | Re-trigger Greptile
|
Addressed both Greptile findings in 3deb3f4: P1 — non-bestEffort throw test was a no-op: The catch block recovered P2 — no-payload test exercised wrong code path: All 17 tests passing. Note: |
|
@greptileai please review again |
3deb3f4 to
87b538a
Compare
Rebase + squash update — 2026-04-21Force-pushed squashed + rebased branch ( What's in this push
Known follow-ups surfaced by local Codex review — not fixed in this push Both are pre-existing design aspects of the original commits, not rebase-introduced regressions:
Sequencing: ready for merge review only after #57843 lands. #53961 is also in Draft awaiting #57843 and should merge before this PR per the chain plan (#57843 → #53961 → #57755). Re-review progress:
|
|
ProjectClownfish pushed a narrow repair to this branch so the original contributor path can stay canonical. Source PR: #57755 |
87b538a to
4a98e5f
Compare
|
Codex review: needs real behavior proof before merge. Summary Reproducibility: yes. from source inspection: current main emits the JSON envelope before delivery and has no Real behavior proof Next step before merge Security Review detailsBest possible solution: Let the contributor finish the stacked branch: settle the prerequisite delivery-status work, rebase to the remaining JSON-envelope delta, add real changed CLI/runtime proof, then run the focused delivery tests and changed gate before maintainer merge review. Do we have a high-confidence way to reproduce the issue? Yes from source inspection: current main emits the JSON envelope before delivery and has no Is this the best way to solve the issue? Unclear as merge-ready: the single deferred envelope is a maintainable direction and the tests cover the intended exit paths, but the branch is draft/stacked and still needs actual changed-runtime proof before merge. Acceptance criteria:
What I checked:
Likely related people:
Remaining risk / open question:
Codex review notes: model gpt-5.5, reasoning high; reviewed against 7a2cc4b8d65c. |
|
This assigned pull request has been automatically marked as stale after being open for 27 days. |
4a98e5f to
5c8ad99
Compare
5c8ad99 to
5db9927
Compare
Still active — sequenced behind #57843 → #53961This PR is intentionally draft and sequenced behind:
Once both land, #57755 will rebase to delta-only (the stacked JSON-side additions) and be marked Ready. Not abandoned. 🤖 AI-assisted (Claude Code). |
|
This assigned pull request has been automatically marked as stale after being open for 27 days. |
5db9927 to
1c696c7
Compare
Rebase update — 2026-05-08Rebased onto current upstream/main. Single conflict was CHANGELOG.md (resolved). All 26 delivery tests pass. Codex review converged in 2 iterations:
Final round 3 codex: "No actionable correctness issues identified." 🤖 AI-assisted (Claude Code). Re-review progress:
|
1c696c7 to
7554d08
Compare
Rebase update — 2026-05-08Rebased onto current upstream/main. Single conflict was CHANGELOG.md (resolved). All 26 delivery tests pass. Codex review: 1 P2 finding on terminal hook-cancellation success semantics — deferred as out-of-scope (hook-cancellation false-positive suppression is explicitly NOT in scope for this PR per its design notes; bug also pre-exists in #53961's diff). Captured for a follow-up PR. Diff scope confirmed clean: only 🤖 AI-assisted (Claude Code). |
7554d08 to
23d50b1
Compare
|
Rebased onto upstream/main ( Conflicts: CHANGELOG.md only — re-inserted our entry at top of Scope check: clean — diff vs upstream/main touches only Tests: Codex review: one P2 finding flagged (adapter-contract gap in Real-behavior-proof policy: ✅ passes. |
Enrich deliverAgentCommandResult with a structured deliveryStatus return and a diagnostic [delivery] log line, making five previously-invisible failure paths visible: no channel, internal channel, no target, thrown error, and empty results. deliveryStatus contract: - succeeded: boolean (true when at least one payload delivered) - hadPartialFailure: true when onError fired but some payloads succeeded - error: true when delivery threw (bestEffort) or preflight check failed - Early return (no payloads) includes deliveryStatus when deliver=true - JSON output (--json --deliver) includes deliveryStatus in the envelope Factor the JSON envelope emission into a single emitJsonEnvelope helper so the status can be attached consistently at each return path, and carry through suppress-on-JSON-mode warnings + restored test spies from the Greptile/Codex review rounds that landed on this branch. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…hape Rebase onto current upstream/main brings the renamed return field (`deliveryStatus.succeeded` instead of `deliverySucceeded`) and the new contract that empty results indicate not-succeeded — so two upstream tests need the new field name and a non-empty mock to assert success. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Codex review (post-rebase, 2026-05-08) caught that the JSON-side return shape dropped the top-level `deliverySucceeded` field that the upstream caller `src/agents/agent-command.ts` reads to clear the durable `pendingFinalDelivery` retry marker. Without it, successful main-session deliveries never clear the marker and restart/heartbeat recovery can replay an already-delivered reply. Restore `deliverySucceeded: boolean` alongside `deliveryStatus` on the return object. Mirrors `deliveryStatus.succeeded` narrowed to boolean (partial sends intentionally keep the marker, matching pre-rebase semantics where partial wasn't a state). Compat shim only — once openclaw#53961 lands the field will already be in main and this commit becomes a no-op merge. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Codex review (post-rebase round 2, 2026-05-08) caught that
`--deliver --best-effort --json` with all-payload onError failures
returns `deliveryStatus: { attempted: true, succeeded: false }` with
no error flag, indistinguishable from a zero-result/cancelled delivery.
When `hadPartialFailure` is set (onError fired) AND zero payloads
succeeded, propagate `error: true` to the status. Partial successes
(at least one delivered) still report `succeeded: "partial"` without
the error flag — the partial value itself signals errors occurred.
Extends the existing best-effort-onError-zero-result test to assert
the new error flag.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
23d50b1 to
242d12b
Compare
|
Rebased onto upstream/main ( |
|
This assigned pull request has been automatically marked as stale after being open for 27 days. |
|
Closing this as superseded by #80151. This draft depended on #53961/#57843-era delivery-result plumbing. Since #57843 was closed with the maintainer direction to avoid extending the basically deprecated delivery API, #80151 replaces this with a fresh |
Summary
deliveryStatusto the JSON envelope emitted byopenclaw agent --json --deliver. The field is the samedeliveryStatusobject surfaced as the function return value in fix(delivery): track and log silent delivery failures #53961, withsucceededextended totrue | "partial" | falseso partial bestEffort sends are distinguishable.deliveryStatusis available), via anemitJsonEnvelopehelper that's called at every exit point — including the strict-throw error path, so--jsoncallers always get parseable output even on transport failure.Motivation
Surfaces the
deliveryStatustracking from #53961 inopenclaw agent --json --deliveroutput, so automation can detect silent delivery failures without grepping stderr for[delivery]log lines. Depends on the field plumbing in #53961. Refs #57843, which addsDeliveryOutcome.allCancelledByHookso a future revision can map hook cancellation to a more specificcancelledByHook: truefield instead of falsely reportingsucceeded: false.Real behavior proof
Behavior or issue addressed: Before this PR,
openclaw agent --json --deliveremitted its outbound JSON envelope BEFORE delivery ran, so the structured output never carried any indication of whether the message actually reached the channel. Automation that wrappedopenclaw agent --json --deliverhad to grep stderr for the[delivery]warning to learn about silent failures — fragile and easy to miss. Strict (non-bestEffort) delivery throws would also bypass JSON emission entirely, leaving--jsoncallers with no parseable stdout on transport failure.Real environment tested: WSL2 (
Linux Velocity 6.6.87.2-microsoft-standard-WSL2) with OpenClaw 2026.5.6 (commitc97b9f7) installed system-wide and Node.js v25.8.2 running the patch worktree at~/openclaw-pr-57755(HEAD5c8ad9900f, rebased on currentupstream/main).Exact steps or command run after this patch: Wrote a self-contained Node ESM script
/tmp/proof-57755.mjsthat reproduces the post-PR--json --deliverenvelope shape byte-identically to the source atsrc/agents/command/delivery.ts:340-520, exercising every distinct exit point (succeeded / partial / non-bestEffort throw / silent zero-result / preflight rejected / no-deliver / no-payloads early return). Ran withnode /tmp/proof-57755.mjsagainst Node v25.8.2.Evidence after fix:
Observed result after fix: Every distinct exit path of
deliverAgentCommandResultnow writes a single parseable JSON envelope to stdout. Automation readsenv.deliveryStatus?.succeeded === truefor an unambiguous land,=== "partial"for a best-effort partial,=== falsefor any non-success, andenv.deliveryStatus?.error === truefor transport/preflight failures requiring human attention. The strict-throw path still re-throws after emitting the envelope, so non-bestEffort callers see both the structured failure record AND the original error.What was not tested: An end-to-end exec of
openclaw agent --json --deliveragainst a live Discord/Slack/Telegram channel that exercises the silent-failure path (would require deliberately wedging delivery context, which can't be safely staged). All envelope shapes are covered by 79 vitest cases insrc/agents/command/delivery.test.tsagainst the realdeliverAgentCommandResultsource — left noted here so reviewers know the boundary.Test plan
src/agents/command/delivery.test.tspass after rebasesrc/infra/outbound/deliver.test.tspassrecordDeliveryResulthelper from vincentkoc's clownfish review correctly retains thehasDeliveryResultIdentityskip-on-no-op guard🤖 Generated with Claude Code