Greptile Summary

This PR surfaces missing message delivery results as explicit tool errors rather than silent successes. When a core send action completes but the response carries no result.messageId, the tool now returns a status: "error" payload — preserving the original outbound details — instead of forwarding the raw payload as if the send had succeeded.

Key changes:

• readMessageDeliveryMessageId: safely traverses sendResult.result (with a payload.result fallback) to extract the delivery messageId.
• buildMissingDeliveryResultToolResult: produces an error jsonResult by spreading the original outbound payload and overwriting status/error, but only for non-dry-run send results that lack a messageId. Plugin-handled sends are unaffected because they always produce a toolResult (via tryHandleWithPluginAction) which is consumed earlier in the pipeline.
• A regression test is added for the core-send/no-delivery-result path. One gap worth noting: there is no explicit test for the dryRun: true bypass (confirming that dry-run sends still return the raw payload), which would make the test suite more complete.

Confidence Score: 4/5

• This PR is safe to merge — the change is narrowly scoped to core send results and does not affect plugin-handled sends or any other action type.
• The logic is sound: buildMissingDeliveryResultToolResult correctly guards on kind !== "send" and dryRun, and readMessageDeliveryMessageId uses proper runtime type checks before accessing nested properties. Plugin sends always bypass the new check via toolResult. The regression test covers the primary scenario. A minor gap is the absence of a test confirming dryRun: true sends are unaffected, but this is low risk given the explicit guard in the implementation.
• No files require special attention.

_{Last reviewed commit: 9f00b90}

Observed in production on a Feishu delivery path where the outbound action returned payload metadata but no confirmed delivery messageId.

Before this patch, message-tool could still serialize that payload as a successful tool result. The visible symptom was that the assistant looked like it had already sent something and then stopped talking, because downstream logic was more likely to suppress the fallback reply path.

This patch intentionally treats "no concrete delivery result" as a tool error so the failure is surfaced instead of being mistaken for a successful send.

This pull request has been automatically marked as stale due to inactivity.
Please add updates or it will be closed.

Thanks for the context here. I swept through the related work, and this is now duplicate or superseded.

Keep open. The underlying missing-delivery-result bug is credible, but this PR is not merge-ready because its receipt predicate is too strict for current accepted send-success shapes, the branch is currently conflicting, and no after-fix real behavior proof has been supplied.

Canonical path: Close this stale PR. The latest review rated it F, the branch still lacks merge-ready proof, and there has been no human follow-up after the durable review.

So I’m closing this here because the remaining work is already tracked in the canonical issue.

Status update on this PR:

Added regression coverage for the two paths the Codex review flagged as missing (commit 4b01122c):

• does not flag missing delivery result on dry-run sends — verifies dryRun: true does not get caught by the new missing-delivery guard.
• returns the payload as-is when delivery yields a concrete messageId — verifies the success path still returns the raw payload (with result.messageId) untouched.

Local verification on the rebased branch:

• pnpm vitest run src/agents/tools/message-tool.test.ts → 19 passed.
• pnpm tsgo → clean.
• pnpm check → clean.

On rebase: I attempted git rebase upstream/main and hit ~40 add/add conflicts across ui/src/ui/views/**, ui/vite.config.ts, ui/vitest.config.ts, vendor/a2ui/renderers/lit/tsconfig.json etc. — these look like upstream UI restructuring rather than anything message-tool related, and resolving them speculatively would add a lot of churn unrelated to this fix. The branch's existing merge commit (2faf9e6c) is from 2026-03-13 against an old main.

Would maintainers prefer:

1. I cherry-pick 9f00b90 + the new test commit onto current main and force-push (clean two-commit branch, drops the old merge), or
2. I do an in-place merge of current upstream/main into this branch (preserves history but adds a new merge commit), or
3. Maintainers handle the rebase as part of merge?

Happy to take whichever route; just want to avoid blindly resolving the UI-tree conflicts without context.

cc the production Feishu signal noted in the PR body — the silent-success fallback is still present in src/agents/tools/message-tool.ts on current main per the Codex review comment above.

ClawSweeper PR egg

🎁 Pass real behavior proof to wake the egg and unlock a hatchable treat.

ClawSweeper applied the proposed close for this PR.

• Action: closed this PR.
• Close reason: duplicate or superseded.
• Evidence: durable ClawSweeper review.