fix(reply): suppress tool-error progress delivery when messages.suppressToolErrors is set#98063
Conversation
…essToolErrors is set 5.22 already drops the tool-error WARNING text via payloads.ts, but the error tool-result payload was still delivered as channel progress unless sourceReplyDeliveryMode was message_tool_only. Operators who opt into messages.suppressToolErrors expect no tool-error noise in chat at all. Add a config-gated early-return in the onToolResult dispatch path so the visible progress delivery is dropped too, matching the warning-text policy. No-op unless messages.suppressToolErrors is true. Folds the mac-mini deploy hotfix into the tracked branch. (cherry picked from commit a973410)
|
Codex review: needs maintainer review before merge. Reviewed June 30, 2026, 3:19 AM ET / 07:19 UTC. Summary PR surface: Source +7, Tests +33. Total +40 across 2 files. Reproducibility: yes. from source inspection: current main forwards payload.isError tool results through onToolResult/sendToolResult without checking messages.suppressToolErrors, while the warning-builder path already honors the knob. I did not run a local test because this review is read-only. Review metrics: 1 noteworthy metric.
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:
Risk before merge
Maintainer options:
Next step before merge
Security Review detailsBest possible solution: Land this focused dispatch-boundary fix after maintainers accept the intentional config-gated message-delivery change, then treat #88898 as superseded by this clean replacement. Do we have a high-confidence way to reproduce the issue? Yes from source inspection: current main forwards payload.isError tool results through onToolResult/sendToolResult without checking messages.suppressToolErrors, while the warning-builder path already honors the knob. I did not run a local test because this review is read-only. Is this the best way to solve the issue? Yes, this is the best narrow fix: the leak is in dispatch progress delivery, and changing the payload builder or channel adapters would either miss the callback path or duplicate core policy. The default behavior remains unchanged unless the existing config knob is explicitly enabled. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 56c2d637d940. Label changesLabel changes:
Label justifications:
Evidence reviewedPR surface: Source +7, Tests +33. Total +40 across 2 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
|
clawsweeper
Bot
added
proof: sufficient
3 participants
Replacement for #88898
This carries the same narrow fix as #88898, rebased onto current
main, from a Moeed-owned fork branch that agents can update if follow-up is needed. The original PR remains blocked because its contributor-fork branch is conflicting and this account has no write access to that fork or upstream.Local verification on this replacement branch:
git diff --checkpassednode_modulesis not installed; upstream CI is expected to provide full test proofSummary
messages.suppressToolErrorsis an existing user-facing config knob that hides tool-error noise from chat (landed in #16620 / #81561). Today it only drops the warning TEXT insidesrc/auto-reply/reply/payloads.ts. The error tool-result payload is still delivered as channel progress through theonToolResultpath insrc/auto-reply/reply/dispatch-from-config.tsunlesssourceReplyDeliveryMode === "message_tool_only". Operators who setmessages.suppressToolErrors: truetherefore still see tool-error noise as progress payloads, contradicting the documented behavior of the knob.Change
Add a config-gated early-return inside
onToolResultinsrc/auto-reply/reply/dispatch-from-config.tsthat drops the visible progress delivery whenpayload.isError === trueandreplyConfig.messages?.suppressToolErrors === true, regardless ofsourceReplyDeliveryMode. Per review feedback, the gate is positioned before the optional source-delivery callback, so a suppressed error payload never reaches progress delivery or the source callback. A focused regression test insrc/auto-reply/reply/dispatch-from-config.test.tsexercises the suppressed-error path throughdispatchReplyFromConfigand asserts neitheronToolResultnorsendToolResultis called while the final reply still ships.Real behavior proof
Behavior addressed: when
messages.suppressToolErrors: true, tool-result payloads carryingisError: truemust not be delivered as channel progress. Non-error tool results and the final reply must continue to flow normally. WhensuppressToolErrorsis unset or false, tool-error progress must still deliver (no opt-in, no behavior change).Real environment tested: live deployed openclaw gateway on rh-bot.lan, pid 71842, build SHA 23804e6 (upgrade-v2026.5.28 includes this commit; deployed bundle
dist/dispatch-D9SNCck_.jscontains the new gate at line 1458).Exact steps or command run after this patch: copied
/tmp/proof-toolerr-amittell.mjsto rh-bot.lan and rannode /tmp/proof-toolerr-amittell.mjsagainst the deployed bundle. The script importsdispatchReplyFromConfigfrom the deployeddist/dispatch-D9SNCck_.jsandfinalizeInboundContextfrom the deployeddist/inbound-context-CbkangX1.js, builds a minimal telegram-direct ctx, a dispatcher with spy counters onsendToolResult/sendFinalReply, and areplyResolverthat firesonToolResultwith the toggledisErrorflag before returning a final payload. Three scenarios were exercised:A.
messages.suppressToolErrors: true+isError: trueB.
messages.suppressToolErrors: true+isError: falseC.
messages.suppressToolErrors: false+isError: trueagents.defaults.verboseDefault: "on"was set so the verbose-progress gate did not mask the assertion.Evidence after fix: deployed bundle gate verified at
/Users/alexm/.openclaw/openclaw/dist/dispatch-D9SNCck_.js:1458asif (payload.isError === true && replyConfig.messages?.suppressToolErrors === true) return;. Script output:Observed result after fix: with
messages.suppressToolErrors: trueandisError: true, the deployed bundle dropped the tool-result progress delivery (sendToolResultcount was 0) while the final reply still shipped (sendFinalReplycount was 1). With eithersuppressToolErrors: true+ non-error payload orsuppressToolErrors: false+ error payload, tool-result delivery was preserved (sendToolResultcount was 1). Existing behavior is preserved outside the explicit opt-in.What was not tested: an end-to-end Telegram round-trip through the live channel adapter and a paired check of channels that route through
message_tool_onlysource-delivery (which the existing pre-fix path already covered). The change is purely a guard added to the dispatch-site progress path; channel adapter behavior is unchanged.