fix(reply): also drop tool-error progress payloads under messages.suppressToolErrors#88898
fix(reply): also drop tool-error progress payloads under messages.suppressToolErrors#88898amittell wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds a hard suppression for tool-error progress payloads when messages.suppressToolErrors is enabled, ensuring such payloads are never surfaced as channel progress regardless of source delivery mode.
Changes:
- Drop tool-result payloads with
isError === trueearly inonToolResultwhenreplyConfig.messages?.suppressToolErrorsis true. - Adds a unit test verifying that no tool result is dispatched while final reply still flows through.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/auto-reply/reply/dispatch-from-config.ts | Early-return in onToolResult for error payloads when suppressToolErrors is enabled. |
| src/auto-reply/reply/dispatch-from-config.test.ts | Adds test asserting tool-error payloads are suppressed and final reply is delivered. |
| if (!suppressAutomaticSourceDelivery && shouldSendToolSummaries()) { | ||
| await onToolResultFromReplyOptions?.(payload); | ||
| } | ||
| // When the operator opts into messages.suppressToolErrors, never | ||
| // surface tool-error tool-result payloads as channel progress, | ||
| // regardless of source delivery mode. payloads.ts already drops | ||
| // the warning text; this drops the visible progress delivery too. | ||
| if (payload.isError === true && replyConfig.messages?.suppressToolErrors === true) { | ||
| return; | ||
| } |
|
Codex review: needs maintainer review before merge. Reviewed June 21, 2026, 8:16 AM ET / 12:16 UTC. Summary PR surface: Source +7, Tests +33. Total +40 across 2 files. Reproducibility: yes. at source level. Current main forwards Review metrics: none identified. Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Mantis proof suggestion Risk before merge
Maintainer options:
Next step before merge
Security Review detailsBest possible solution: Land the narrow shared dispatch guard after maintainer review, while leaving broader Do we have a high-confidence way to reproduce the issue? Yes, at source level. Current main forwards Is this the best way to solve the issue? Yes. The shared dispatch handler is the right fix layer because it gates both the source callback and dispatcher delivery before channel adapters see the payload, while the sibling final-warning payload path already honors the same boolean. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 12c34fc3a951. Label changesLabel 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
|
7a39e69 to
329aab2
Compare
e5fe083 to
8010c6d
Compare
8010c6d to
e8688ae
Compare
…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)
e8688ae to
ba0405b
Compare
|
Note on the failing Also fixed a stale sentence in the PR body: the suppression gate sits before the optional source-delivery callback (per Copilot's review, verified by the regression test asserting |
Summary
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.