fix(discord): guard message-tool-only final delivery#84485
Conversation
|
Codex review: needs maintainer review before merge. Reviewed July 8, 2026, 5:46 PM ET / 21:46 UTC. Summary PR surface: Source +245, Tests +959. Total +1204 across 6 files. Reproducibility: yes. The PR branch contains targeted Review metrics: 1 noteworthy metric.
Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Risk before merge
Maintainer options:
Next step before merge
Maintainer decision needed
Security Review detailsBest possible solution: Land this only if maintainers want the bounded Discord guard policy; otherwise narrow it behind an approved delivery rule or leave the broader stranded-final recovery work to the separate canonical recovery discussion. Do we have a high-confidence way to reproduce the issue? Yes. The PR branch contains targeted Is this the best way to solve the issue? Yes if maintainers accept the policy: the guard lives at the delivery decision boundary, uses existing suppression-bypass metadata, and avoids publishing private final text. The remaining uncertainty is product/compatibility acceptance, not a concrete patch defect found in this review. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 7d82e5b65895. Label changesLabel justifications:
Evidence reviewedPR surface: Source +245, Tests +959. Total +1204 across 6 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 (3 earlier review cycles) |
|
ClawSweeper PR egg 🎁 Pass real behavior proof to wake the egg and unlock a hatchable treat. Where did the egg go?
|
b93852b to
788ec23
Compare
martingarramon
left a comment
There was a problem hiding this comment.
This catches a real Discord message_tool_only gap: if the model finishes without message.send, the turn should not silently collapse into NO_REPLY.
From the candidacy check, the guard is skipped for heartbeats, visible reply evidence, non-message_tool_only delivery, room_event, and silentExpected. It also respects allowEmptyAssistantReplyAsSilent, except for directly addressed turns. The tests cover the positive and suppression paths well, especially committed sends and approval prompts.
One question: does currentInboundEventKind === "room_event" cover all non-conversational inbound event kinds that should bypass this, or are there other synthetic/internal kinds worth excluding?
LGTM.
77fd52d to
ecac27b
Compare
|
Thanks, good question. For the current inbound-turn taxonomy, yes: If we add another synthetic/internal inbound kind later, it should either be normalized into |
|
|
3e18cd7 to
48173e5
Compare
|
Heads up: this PR needs to be updated against current |
48173e5 to
40fc3e8
Compare
40fc3e8 to
9ec4ab5
Compare
Resolve current-main drift for the message-tool-only guard branch while preserving the source-visible delivery guard behavior. Validation: - node scripts/run-vitest.mjs run src/auto-reply/reply/agent-runner.runreplyagent.e2e.test.ts src/auto-reply/tokens.test.ts --reporter=dot - node scripts/run-tsgo.mjs -p tsconfig.core.json --incremental --tsBuildInfoFile .artifacts/tsgo-cache/core.tsbuildinfo - git diff --check
|
@clawsweeper re-review Behavior addressed: Current-head refresh after merging current Findings addressed:
Exact steps or command run after this patch:
Evidence after fix:
Observed result after fix: The branch is clean against current What was not tested: I did not repeat the older Discord sandbox visible-message proof on this refreshed head; the existing PR already has Requested outcome if accepted: restore/update to |
|
🦞🧹 I asked ClawSweeper to review this item again. |
|
@clawsweeper re-review Current head status reconciliation request:
Author-side status:
Please re-evaluate the current head and either transition it toward maintainer review if the existing proof plus green current head is sufficient, or return a concrete current-head proof gap that is still author-actionable. |
Treat successfully delivered substantive direct-block payloads as visible Discord reply evidence so the message-tool-only guard does not synthesize a duplicate warning. Status/progress-only direct blocks remain activity and still require a final visible reply. Validation: - focused E2E cases: 2 passed, 86 skipped - type-aware oxlint: 0 warnings/errors - git diff --check Note: full tsgo reached unrelated existing config/secrets errors in this branch.
|
@clawsweeper re-review Current head addresses the unresolved substantive direct-block delivery finding. Behavior fixed:
Current head: Validation:
Please re-evaluate the current head for transition toward maintainer review. |
|
🦞🧹 I asked ClawSweeper to review this item again. |
|
@clawsweeper re-review Current head: Behavior addressed:
Real setup tested:
Exact steps or command run after this patch:
Evidence after fix:
Observed result after fix:
What was not tested:
|
|
🦞🧹 I asked ClawSweeper to review this item again. |
|
@clawsweeper re-review Current head update: pushed Quick reviewer map:
Behavior addressed: Real setup tested: Exact steps or command run after this patch: Evidence after fix:
Observed result after fix: What was not tested: |
|
🦞🧹 I asked ClawSweeper to review this item again. |
|
@clawsweeper re-review Current head update: pushed Why this head changed:
Behavior addressed: Real setup tested: Exact steps or command run after this patch: Evidence after fix:
Observed result after fix: What was not tested: |
|
🦞🧹 I asked ClawSweeper to review this item again. |
|
Current-head CI follow-up for maintainers:
Validation:
Observed result:
Request:
|
|
@clawsweeper re-review Behavior addressed:
Evidence after fix:
Exact steps or command run after this patch:
Observed result after fix:
What was not tested:
|
|
🦞🧹 I asked ClawSweeper to review this item again. |
|
@clawsweeper re-review Current-head update for this PR. What changed:
Validation run on the merged current head:
Please re-evaluate the current head for ready status once CI lands. |
|
🦞🧹 I asked ClawSweeper to review this item again. |
Resolve current main drift for PR openclaw#84485 and keep the PR-specific behavior intact.
|
@clawsweeper re-review Current head update: merged latest main and resolved the current auto-reply Discord delivery guard conflicts. Head: 1c14c66 Validation:
|
|
🦞🧹 I asked ClawSweeper to review this item again. |
|
Closing this voluntarily. Since this PR opened, #99536 merged the canonical recovery for the core failure reported in #85714: a substantive private final that would otherwise be stranded in The remaining unique behavior in this branch is narrower: whether an empty or I am therefore retiring this implementation rather than rebasing another overlapping delivery path. I am not opening a replacement PR now. If maintainers explicitly want the residual visible-guard policy, I would revisit it as a minimal current-main change with the silence and duplication semantics decided up front. |
Summary
Discord
message_tool_onlyturns can currently end without any visible source-channel delivery when the model does not call themessagetool:NO_REPLYdespite an owed visible status/result;Prompt/tool-description guidance helps, but it is advisory. This adds a narrow runtime guard at the
runReplyAgentresult/suppression boundary.What changed
sourceReplyDeliveryMode: "message_tool_only"turns with no committed successful message-tool/media/target delivery evidence.deliverDespiteSourceReplySuppressionmetadata so dispatch can surface them instead of silently suppressing them.NO_REPLY/ empty-final paths, synthesize a visible guard/error payload, also marked for source delivery.NO_REPLYquiet but treat tool activity, block progress, cron side effects, or substantive final text as response-owed evidence.NO_REPLYfinals whensourceReplyDeliveryModeis notmessage_tool_only; real thread traces showed that path can occur while visible progress/tool chatter is emitted but no final/status answer is delivered.room_eventstrict/background turns;Related context
This complements, rather than replaces, nearby work:
reconcileUnknownSendpath (open at PR preparation time).message_toolpolicy baseline.Real behavior proof
message_tool_onlyturns should no longer end with no visible source-channel reply when a directly addressed turn producesNO_REPLY, an empty final, or normal final text without a successfulmessage.send.OpenClaw 2026.5.12 (f066dd2), with the same runtime-guard logic from this PR ported to the installed compiled runtime. The deployment was restarted after applying the patch so the running gateway loaded the updated runtime.NO_REPLY/ empty final, then ported the response-owed guard to source and to the installed runtime bundle on disk.node --checkagainst the installed runtime bundle.beforeAgentRunBlockedskip, re-entry preservation for already-marked payloads, guard trace logging, pending-tool-drain ordering comment, and the narrower visible-reply evidence split. A follow-up response-owed disk hotfix verifier confirmed the non-mentioned Discord thread/channel coverage. A later real trace showed an additional gap wheresourceReplyDeliveryModewas notmessage_tool_onlyandtoolSummarywas absent even though visible tool/progress chatter existed; the latest source update adds a regression test for that path and treats successful progress/side-effect activity as response-owed activity while still keeping pure ambient/no-tool silence quiet.Latest v3 refinement
After applying the response-owed variant in a real deployment, follow-up Discord traces exposed an additional path: the source reply mode was not
message_tool_only, but the run still emitted tool/progress chatter and then ended with an empty final. The latest source head handles that path while avoiding a false positive where block-streamed content already delivered a substantive automatic reply:NO_REPLYfinal ⇒ guard payload.message_tool_onlysubstantive final payloads stay on the normal automatic-delivery path.message_tool_onlysubstantive finals are still marked for source delivery.message.sendevidence, and approval prompts still skip the guard.Validation
Validated in an isolated disposable checkout; no live Gateway/runtime/config/state was used.
Result: