fix(heartbeat): suppress fallback after route-matched message tool#84975
fix(heartbeat): suppress fallback after route-matched message tool#84975luoyanglang wants to merge 1 commit into
Conversation
|
Codex review: needs real behavior proof before merge. Reviewed July 17, 2026, 11:00 AM ET / 15:00 UTC. Summary PR surface: Source +110, Tests +536. Total +646 across 11 files. Reproducibility: yes. at source level. The linked issue provides production logs for a generic message-tool send followed by heartbeat fallback, and current code still lacks this candidate route-scoped suppression path; this review did not rerun the live channel scenario. Review metrics: none identified. Root-cause cluster Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Proof guidance:
Mantis proof suggestion Risk before merge
Maintainer options:
Next step before merge
Security Review detailsBest possible solution: Land the route- and terminal-aware fix after exact-head Telegram proof shows one same-route terminal send, while progress, terminal failure, wrong-route or wrong-thread, rich payload, and relayable exec cases remain visible. Do we have a high-confidence way to reproduce the issue? Yes, at source level. The linked issue provides production logs for a generic message-tool send followed by heartbeat fallback, and current code still lacks this candidate route-scoped suppression path; this review did not rerun the live channel scenario. Is this the best way to solve the issue? Yes, subject to exact-head proof. Keeping the decision at the heartbeat owner boundary and reusing shared route matching is narrower and safer than suppressing all free text after any message-tool activity. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 75697ad3bf6c. Label changesLabel changes:
Label justifications:
Evidence reviewedPR surface: Source +110, Tests +536. Total +646 across 11 files. View PR surface stats
What I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
Review history (5 earlier review cycles)
|
|
ClawSweeper PR egg ✨ Hatched: ✨ glimmer Pearl Crabkin Hatch commandComment Hatchability rules:
Rarity: ✨ glimmer. What is this egg doing here?
|
9f3e455 to
78ecbc4
Compare
78ecbc4 to
87d31f6
Compare
3efd23c to
0bb15b5
Compare
88f67b0 to
48b7aaf
Compare
|
This assigned pull request has been automatically marked as stale after being open for 27 days. |
8 similar comments
|
This assigned pull request has been automatically marked as stale after being open for 27 days. |
|
This assigned pull request has been automatically marked as stale after being open for 27 days. |
|
This assigned pull request has been automatically marked as stale after being open for 27 days. |
|
This assigned pull request has been automatically marked as stale after being open for 27 days. |
|
This assigned pull request has been automatically marked as stale after being open for 27 days. |
|
This assigned pull request has been automatically marked as stale after being open for 27 days. |
|
This assigned pull request has been automatically marked as stale after being open for 27 days. |
|
This assigned pull request has been automatically marked as stale after being open for 27 days. |
|
This assigned pull request has been automatically marked as stale after being open for 27 days. |
3 similar comments
|
This assigned pull request has been automatically marked as stale after being open for 27 days. |
|
This assigned pull request has been automatically marked as stale after being open for 27 days. |
|
This assigned pull request has been automatically marked as stale after being open for 27 days. |
|
Rescued and refreshed the PR on exact head The final implementation now suppresses heartbeat fallback narration only when the matching provider/target/account/thread has a successful terminal visible Exact-tree local validation:
GitHub now reports this head mergeable; exact-head CI is running. It currently reports @clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
Security-sensitive guard clearedThis PR no longer has blocked security-sensitive file changes. A future security-sensitive change requires a fresh
|
Dependency graph guard clearedThis PR no longer has blocked dependency graph changes. A future dependency graph change requires a fresh
|
|
Authorized force-with-lease correction completed on exact head The previous merge topology expanded GitHub's comparison to unrelated base-history changes. The branch is now a single fix commit on main Exact-candidate validation:
Exact-head CI is running now. The old dependency/security guard failures belonged to the expanded comparison and must not be attributed to this head. @clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
|
This assigned pull request has been automatically marked as stale after being open for 27 days. |
7 similar comments
|
This assigned pull request has been automatically marked as stale after being open for 27 days. |
|
This assigned pull request has been automatically marked as stale after being open for 27 days. |
|
This assigned pull request has been automatically marked as stale after being open for 27 days. |
|
This assigned pull request has been automatically marked as stale after being open for 27 days. |
|
This assigned pull request has been automatically marked as stale after being open for 27 days. |
|
This assigned pull request has been automatically marked as stale after being open for 27 days. |
|
This assigned pull request has been automatically marked as stale after being open for 27 days. |
When heartbeat runs in message-tool-only mode, a successful generic message tool delivery can still be followed by duplicate free-text fallback narration in the same Telegram chat.
Fixes #84217
Affected surface
src/auto-reply/reply/agent-runner.tssrc/auto-reply/reply-payload.tssrc/auto-reply/heartbeat-reply-payload.tssrc/infra/heartbeat-runner.tssrc/auto-reply/reply/agent-runner-payloads.tssrc/infra/heartbeat-runner.tool-response.test.tssrc/auto-reply/reply/agent-runner.runreplyagent.e2e.test.tsScope
This is a parameter/evidence threading fix. It does not add a config flag, does not change heartbeat scheduling, and does not touch Slack identity, commitment, or channel-specific private paths.
The important semantic boundary is that "generic message tool delivery evidence" is not the same as "has outbound reply content". This PR only marks heartbeat replies when existing message-tool target evidence matches the same delivery route via the existing
resolveMessagingToolPayloadDeduperoute matcher. GlobalmessagingToolSentTextswithout route targets is intentionally not enough.After Vincent's thread-route follow-up, I found one extra current-head issue while refreshing proof: generic reply-payload dedupe could drop the matched heartbeat payload before
heartbeat-runnersaw the route evidence. The current head keeps heartbeat payloads out of that generic dedupe layer so the heartbeat-specific suppression path receives the evidence and can still preserve no-route / wrong-topic fallback behavior.Prior art
Prior attempt #84273 was closed by contributor due to inability to provide live channel proof. This PR provides expanded source + runtime helper proof, with the helper semantic explicitly distinguishing "generic message tool delivery evidence" from "has outbound reply content" to address the message-delivery risk flagged in #84273.
The key difference from #84273 is route scope: #84273 marked heartbeat payloads after any message-tool send, while this PR only marks a heartbeat payload when committed
messagingToolSentTargetsmatch the reply route. If the message tool delivered to another route, or only global text evidence exists, heartbeat fallback still sends.Current-head P1 follow-up proof
This update addresses the ClawSweeper P1 finding "Suppress same-route fallback even when the text differs" on head
25958135ed2135dede29e716668ac34cfaea8007.The same-route heartbeat evidence rule now treats committed route-matched generic
messagetool target evidence as sufficient for a single plain heartbeat fallback payload, even when the fallback text differs from the tool body. Multi-payload heartbeat replies keep the existing text-match guard so unrelated fallback payloads are not suppressed. Rich payloads, media payloads, reasoning, no-route evidence, wrong-route evidence, and wrong-thread/topic evidence remain outside this suppression path.Commands run locally:
Updated live Telegram proof for head
25958135ed2135dede29e716668ac34cfaea8007The earlier Mantis Telegram workflow could not run because this actor lacks write/maintain/admin permission, so I reran the current-head proof locally with the existing Telegram proof credential and a temporary harness outside the patch. The harness used the real current-head route-evidence path:
buildReplyPayloads(...)built heartbeat payloads from route-scopedmessagingToolSentTargets.runHeartbeatOnce(...)made the heartbeat delivery/suppression decision.sendMessagepath.Visible Telegram screenshot: #84975 (comment)
{ "proof": "openclaw-pr-84975-heartbeat-live-proof", "head": "25958135ed2135dede29e716668ac34cfaea8007", "mode": "live-telegram", "route": { "provider": "telegram", "to": "telegram:group:<redacted-chat>", "thread": "topic:<redacted-thread>" }, "scenarios": [ { "scenario": "same-route-different-text", "expected": "suppress", "result": "PASS", "decision": "suppress", "evidence": { "mode": "same-route", "targetCount": 1, "metadataDelivered": true, "targetRoute": "telegram:group:<redacted-chat>" }, "transport": { "messageToolSendCount": 1, "heartbeatFallbackSendCount": 0, "sends": [ { "phase": "message-tool", "method": "sendMessage", "status": "ok", "message_id": "15" } ] } }, { "scenario": "same-route-same-text", "expected": "suppress", "result": "PASS", "decision": "suppress", "evidence": { "mode": "same-route", "targetCount": 1, "metadataDelivered": true, "targetRoute": "telegram:group:<redacted-chat>" }, "transport": { "messageToolSendCount": 1, "heartbeatFallbackSendCount": 0, "sends": [ { "phase": "message-tool", "method": "sendMessage", "status": "ok", "message_id": "16" } ] } }, { "scenario": "no-route", "expected": "send", "result": "PASS", "decision": "send", "evidence": { "mode": "none", "targetCount": 0, "metadataDelivered": false, "targetRoute": "none" }, "transport": { "messageToolSendCount": 0, "heartbeatFallbackSendCount": 1, "sends": [ { "phase": "heartbeat-fallback", "method": "sendMessage", "status": "ok", "message_id": "17" } ] } }, { "scenario": "wrong-thread", "expected": "send", "result": "PASS", "decision": "send", "evidence": { "mode": "wrong-thread", "targetCount": 1, "metadataDelivered": false, "targetRoute": "telegram:group:<redacted-chat> topic:<wrong-thread-redacted>" }, "transport": { "messageToolSendCount": 0, "heartbeatFallbackSendCount": 1, "sends": [ { "phase": "heartbeat-fallback", "method": "sendMessage", "status": "ok", "message_id": "18" } ] } } ], "summary": { "pass": 4, "fail": 0 } }Observed visible Telegram messages match the decisions above: same-route different-text only shows the message-tool visible body, same-route same-text only shows one visible body, and the no-route / wrong-thread cases show heartbeat fallback messages.
Current-head maintenance proof
After Vincent's route/thread follow-up moved this PR to
f21343bfd4, CI exposed one current-head regression:checks-node-auto-reply-reply-agent-runnertimed out insrc/auto-reply/reply/agent-runner-payloads.test.tsatdedupes duplicate same-target reply text without suppressing unrelated finals.Root cause: route dedupe called
getChannelPlugin("telegram")from the generic reply-payload path, which lazily loads the bundled Telegram plugin in this test/runtime path and can block the shard. Current headdf6f33925bkeeps the route/thread semantics from Vincent's follow-up, uses only the already-loaded channel registry for plugin-specific matching, and preserves internal route-string fallback for forms liketelegram:group:<chat>:topic:<thread>without loading bundled channel implementations.Current-head commands run locally:
Note: I did not rerun the earlier live Telegram helper on this current head. The current-head proof above is local source/runtime test coverage plus the prior PR body runtime helper proof retained below for the original route-suppression behavior.
Real behavior proof
messagetool has already delivered to the same route, but it must still send fallback when no same-route delivery evidence exists.wolf/heartbeat-suppress-after-message-tool, head14b8854108.runHeartbeatOnce, seeds a realopenclaw.json-shaped config and session store, registers a local Telegram outbound adapter, then executes three heartbeat ticks: same-route evidence, no-route evidence, and wrong-thread/topic-unmarked evidence.runHeartbeatOncecompletes and does not invoke the Telegram send path (telegramSendCount: 0), so the fallback narration is suppressed. Without route evidence, and for the wrong-topic/wrong-thread-unmarked fallback case, the same runtime path invokes the Telegram send path once (telegramSendCount: 1), proving fallback remains available when the message tool did not prove same-route delivery.Supplemental validation
RED before fix
The new regression was first run before the implementation.
runHeartbeatOncestill sent fallback text even when the reply carried route-scoped message-tool delivery evidence:Current-head GREEN after fix
Previous changed gate
Coverage note
Push-before searches were rerun against open PRs:
No open PR was found that fixes #84217 or covers the route-scoped message-tool fallback behavior.
Updated live Telegram proof for head 6c40273
This proof refreshes the current-head route-suppression behavior after the text-evidence guard fix. It uses the PR source from
/code/workspaces/openclaw/openclaw-84975, calls current-headbuildReplyPayloads(...), then runsrunHeartbeatOnce(...)with real Telegram Bot API sends. Secrets, chat id, topic id, usernames, local temp paths, and process details are omitted or redacted.Redacted route:
{"provider":"telegram","to":"telegram:group:<redacted-chat>","thread":"topic:<redacted-thread>"}Covered cases:
Redacted live log:
Verification after the code fix:
Updated live Telegram proof for head dc6fa90
This refreshes the live proof after rebasing the branch onto latest
origin/mainto clear the PR conflict insrc/auto-reply/reply/agent-runner.runreplyagent.e2e.test.ts.The temporary harness stayed outside the patch and used current-head source from this branch:
buildReplyPayloads(...)built heartbeat payload metadata from route-scopedmessagingToolSentTargets.runHeartbeatOnce(...)made the heartbeat suppression/fallback decision.runHeartbeatOnce(...); it was not mocked.Covered cases:
Redacted live log:
Verification after rebase: