Skip to content

fix(reply): also drop tool-error progress payloads under messages.suppressToolErrors#88898

Closed
amittell wants to merge 1 commit into
openclaw:mainfrom
amittell:fix/suppress-tool-error-progress
Closed

fix(reply): also drop tool-error progress payloads under messages.suppressToolErrors#88898
amittell wants to merge 1 commit into
openclaw:mainfrom
amittell:fix/suppress-tool-error-progress

Conversation

@amittell

@amittell amittell commented Jun 1, 2026

Copy link
Copy Markdown
Contributor

Summary

messages.suppressToolErrors is an existing user-facing config knob that hides tool-error noise from chat (landed in #16620 / #81561). Today it only drops the warning TEXT inside src/auto-reply/reply/payloads.ts. The error tool-result payload is still delivered as channel progress through the onToolResult path in src/auto-reply/reply/dispatch-from-config.ts unless sourceReplyDeliveryMode === "message_tool_only". Operators who set messages.suppressToolErrors: true therefore still see tool-error noise as progress payloads, contradicting the documented behavior of the knob.

Change

Add a config-gated early-return inside onToolResult in src/auto-reply/reply/dispatch-from-config.ts that drops the visible progress delivery when payload.isError === true and replyConfig.messages?.suppressToolErrors === true, regardless of sourceReplyDeliveryMode. 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 in src/auto-reply/reply/dispatch-from-config.test.ts exercises the suppressed-error path through dispatchReplyFromConfig and asserts neither onToolResult nor sendToolResult is called while the final reply still ships.

Real behavior proof

Behavior addressed: when messages.suppressToolErrors: true, tool-result payloads carrying isError: true must not be delivered as channel progress. Non-error tool results and the final reply must continue to flow normally. When suppressToolErrors is 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_.js contains the new gate at line 1458).

Exact steps or command run after this patch: copied /tmp/proof-toolerr-amittell.mjs to rh-bot.lan and ran node /tmp/proof-toolerr-amittell.mjs against the deployed bundle. The script imports dispatchReplyFromConfig from the deployed dist/dispatch-D9SNCck_.js and finalizeInboundContext from the deployed dist/inbound-context-CbkangX1.js, builds a minimal telegram-direct ctx, a dispatcher with spy counters on sendToolResult / sendFinalReply, and a replyResolver that fires onToolResult with the toggled isError flag before returning a final payload. Three scenarios were exercised:

A. messages.suppressToolErrors: true + isError: true
B. messages.suppressToolErrors: true + isError: false
C. messages.suppressToolErrors: false + isError: true

agents.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:1458 as if (payload.isError === true && replyConfig.messages?.suppressToolErrors === true) return;. Script output:

--- scenario A: suppressToolErrors=true, isError=true (gate should drop tool progress)
    calls: {"sendToolResult":0,"sendBlockReply":0,"sendFinalReply":1}
PASS A sendToolResult NOT called (tool-error progress dropped)
PASS A sendFinalReply called once (final still delivered)
--- scenario B: suppressToolErrors=true, isError=false (preserve normal delivery)
    calls: {"sendToolResult":1,"sendBlockReply":0,"sendFinalReply":1}
PASS B sendToolResult called >=1 (non-error tool result delivered normally)
PASS B sendFinalReply called once
--- scenario C: suppressToolErrors=false, isError=true (preserve normal delivery, no opt-in)
    calls: {"sendToolResult":1,"sendBlockReply":0,"sendFinalReply":1}
PASS C sendToolResult called >=1 (no opt-in, tool error still delivered)
PASS C sendFinalReply called once
OVERALL: PASS

Observed result after fix: with messages.suppressToolErrors: true and isError: true, the deployed bundle dropped the tool-result progress delivery (sendToolResult count was 0) while the final reply still shipped (sendFinalReply count was 1). With either suppressToolErrors: true + non-error payload or suppressToolErrors: false + error payload, tool-result delivery was preserved (sendToolResult count 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_only source-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.

Copilot AI review requested due to automatic review settings June 1, 2026 03:42

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 === true early in onToolResult when replyConfig.messages?.suppressToolErrors is 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.

Comment on lines +2534 to +2543
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;
}
@openclaw-barnacle openclaw-barnacle Bot added the proof: supplied External PR includes structured after-fix real behavior proof. label Jun 1, 2026
@clawsweeper

clawsweeper Bot commented Jun 1, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge. Reviewed June 21, 2026, 8:16 AM ET / 12:16 UTC.

Summary
The PR adds a messages.suppressToolErrors guard in shared reply dispatch and a focused regression test so configured tool-error progress payloads do not reach source callbacks or dispatcher delivery while final replies still send.

PR surface: Source +7, Tests +33. Total +40 across 2 files.

Reproducibility: yes. at source level. Current main forwards isError tool-result progress through the reply-options callback and dispatcher path before applying messages.suppressToolErrors; the PR proof then exercises suppressed-error, non-error, and disabled-config scenarios.

Review metrics: none identified.

Merge readiness
Overall: 🦞 diamond lobster
Proof: 🦞 diamond lobster
Patch quality: 🦞 diamond lobster
Result: ready for maintainer review.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Mantis proof suggestion
Native Telegram proof would materially show the user-visible chat delivery behavior that the shared dispatch guard changes. A maintainer can ask Mantis to capture proof by posting this exact PR comment:

@openclaw-mantis telegram desktop proof: verify messages.suppressToolErrors drops tool-error progress while preserving the final reply.

Risk before merge

  • [P1] The diff intentionally drops configured isError tool-result progress messages, so maintainers may want visible Telegram proof before accepting the chat-delivery change.
  • [P1] The latest native Telegram/Mantis proof lane did not complete due credential-broker infrastructure, though the PR body supplies live dispatch-level deployed-bundle proof for the changed path.

Maintainer options:

  1. Rerun native Telegram proof
    A maintainer can rerun Mantis to capture that the configured Telegram chat no longer receives the error progress bubble and still receives the final reply.
  2. Land with dispatch proof
    Maintainers may accept the existing live deployed-bundle proof because the guard is shared dispatch logic, the default behavior is unchanged, and the focused regression test covers callback plus dispatcher suppression.

Next step before merge

  • [P2] No repair lane is needed; the remaining action is maintainer merge review, with an optional Mantis rerun if maintainers want visible Telegram proof before accepting the delivery change.

Security
Cleared: The diff only changes shared reply dispatch suppression logic and a regression test; it does not alter dependencies, workflows, secrets, permissions, downloads, or code-execution surfaces.

Review details

Best possible solution:

Land the narrow shared dispatch guard after maintainer review, while leaving broader all | final | off warning policy and dedicated warning-routing decisions to #39406 and #45565.

Do we have a high-confidence way to reproduce the issue?

Yes, at source level. Current main forwards isError tool-result progress through the reply-options callback and dispatcher path before applying messages.suppressToolErrors; the PR proof then exercises suppressed-error, non-error, and disabled-config scenarios.

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 changes

Label justifications:

  • P2: This fixes a bounded user-facing chat-noise bug in an existing opt-in message config path without changing default behavior.
  • merge-risk: 🚨 message-delivery: The PR intentionally suppresses a class of configured tool-error progress messages, which is a visible delivery behavior change even though it is opt-in.
  • rating: 🦞 diamond lobster: Overall readiness is 🦞 diamond lobster; proof is 🦞 diamond lobster and patch quality is 🦞 diamond lobster.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (live_output): The PR body includes after-fix live deployed-bundle output for suppressed error, non-error, and disabled-config scenarios, with final reply preservation shown.
  • proof: sufficient: Contributor real behavior proof is sufficient. The PR body includes after-fix live deployed-bundle output for suppressed error, non-error, and disabled-config scenarios, with final reply preservation shown.
  • mantis: telegram-visible-proof: Mantis should capture Telegram visible proof. The change removes a visible Telegram/direct-chat tool-error progress bubble while preserving the final reply, which a short Telegram Desktop recording can demonstrate.
Evidence reviewed

PR surface:

Source +7, Tests +33. Total +40 across 2 files.

View PR surface stats
Area Files Added Removed Net
Source 1 7 0 +7
Tests 1 33 0 +33
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 2 40 0 +40

What I checked:

  • Repository policy read: Root AGENTS.md was read fully and applied; its PR review policy requires whole-path message-delivery, config, proof, and best-fix review rather than diff-only review. (AGENTS.md:1, 12c34fc3a951)
  • Telegram maintainer note read: The Telegram note says Telegram behavior PRs touching visible delivery should have real Telegram proof when practical, which informs the optional Mantis proof recommendation. (.agents/maintainer-notes/telegram.md:1, 12c34fc3a951)
  • Current main forwards error progress before suppression: On current main, the onToolResult handler forwards to onToolResultFromReplyOptions and later dispatcher delivery without checking replyConfig.messages?.suppressToolErrors, so the reported progress leak is source-reproducible. (src/auto-reply/reply/dispatch-from-config.ts:2929, 12c34fc3a951)
  • Existing user-facing config contract: The current config type documents messages.suppressToolErrors as suppressing tool-error warnings from being shown to the user, and schema help gives the same contract. (src/config/types.messages.ts:156, 12c34fc3a951)
  • Sibling warning policy honors the same boolean: The embedded-runner warning policy already returns showWarning: false when params.suppressToolErrors is true, so the PR aligns the progress-delivery path with the existing payload-warning path. (src/agents/embedded-agent-runner/run/payloads.ts:199, 12c34fc3a951)
  • Adjacent tests cover payload-warning suppression: Current tests already cover messages.suppressToolErrors suppressing non-mutating and mutating warning payloads, while the PR adds the missing dispatch-progress case. (src/agents/embedded-agent-runner/run/payloads.errors.test.ts:521, 12c34fc3a951)

Likely related people:

  • vai-oro: Authored the merged PR that introduced the messages.suppressToolErrors config surface this PR completes for progress payloads. (role: feature introducer; confidence: high; commits: 2c8b9210548d, fdb4d235fcc3, 9ae4394b81bf; files: src/config/types.messages.ts, src/config/zod-schema.session.ts, src/config/schema.help.ts)
  • sebslight: Merged the original config PR and authored adjacent regression commits for the suppression config surface. (role: reviewer/merger and adjacent test author; confidence: high; commits: 2c8b9210548d, bb54fb011434, 9ae4394b81bf; files: src/agents/pi-embedded-runner/run/payloads.e2e.test.ts, src/config/types.messages.ts)
  • moeedahmed: Authored the merged PR that moved messages.suppressToolErrors before mutating-tool warning generation in the sibling payload-warning path. (role: recent adjacent contributor; confidence: high; commits: 9c00268914b9, 77db9bab48f0; files: src/agents/embedded-agent-runner/run/payloads.ts, src/agents/embedded-agent-runner/run/payloads.errors.test.ts)
  • jalehman: Merged the mutating-tool suppression PR and authored the final branch commit on that adjacent behavior. (role: merger and adjacent patch author; confidence: high; commits: 9c00268914b9, 7462a862be9e; files: src/agents/embedded-agent-runner/run/payloads.ts, src/agents/embedded-agent-runner/run/payloads.errors.test.ts)
  • vincentkoc: Current shallow blame attributes the surrounding shared reply dispatch callback path to a recent mainline commit, so this is a useful routing signal even though older feature history is better captured by the linked PRs. (role: recent area contributor; confidence: medium; commits: 7dd01d15c56d; files: src/auto-reply/reply/dispatch-from-config.ts)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

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 keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

@clawsweeper clawsweeper Bot added proof: sufficient ClawSweeper judged the real behavior proof convincing. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. P2 Normal backlog priority with limited blast radius. labels Jun 1, 2026
@amittell
amittell force-pushed the fix/suppress-tool-error-progress branch from 7a39e69 to 329aab2 Compare June 1, 2026 22:31
@openclaw-barnacle openclaw-barnacle Bot removed the proof: sufficient ClawSweeper judged the real behavior proof convincing. label Jun 1, 2026
@amittell
amittell force-pushed the fix/suppress-tool-error-progress branch 2 times, most recently from e5fe083 to 8010c6d Compare June 1, 2026 22:35
@clawsweeper clawsweeper Bot added the proof: sufficient ClawSweeper judged the real behavior proof convincing. label Jun 1, 2026
@amittell
amittell force-pushed the fix/suppress-tool-error-progress branch from 8010c6d to e8688ae Compare June 11, 2026 12:24
@openclaw-barnacle openclaw-barnacle Bot removed the proof: sufficient ClawSweeper judged the real behavior proof convincing. label Jun 11, 2026
@clawsweeper clawsweeper Bot added the proof: sufficient ClawSweeper judged the real behavior proof convincing. label Jun 11, 2026
…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)
@amittell
amittell force-pushed the fix/suppress-tool-error-progress branch from e8688ae to ba0405b Compare June 11, 2026 13:19
@openclaw-barnacle openclaw-barnacle Bot removed the proof: sufficient ClawSweeper judged the real behavior proof convincing. label Jun 11, 2026
@clawsweeper clawsweeper Bot added proof: sufficient ClawSweeper judged the real behavior proof convincing. mantis: telegram-visible-proof Mantis should capture Telegram visible proof. labels Jun 11, 2026
@amittell

Copy link
Copy Markdown
Contributor Author

Note on the failing Run agentic native Telegram proof check: the failure is capture-infrastructure, not this PR — the Mantis baseline session died restoring the leased Telegram credential payload (payload-chunk returned an invalid response in telegram-user-credential.ts, then a bare fetch failed on retry) before the PR code was ever exercised, and the agent wrote the mandated comparison.pass: false failure manifest. All other checks on this head are green, including every checks-node-core-* lane and the parsed Real behavior proof. I don't have rerun rights on this workflow; a maintainer re-trigger of the Mantis job (or the mantis: telegram-visible-proof flow) should produce the visible proof once the credential broker behaves.

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 onToolResult is never called); the body previously still described the pre-review position.

@openclaw-barnacle openclaw-barnacle Bot removed the proof: sufficient ClawSweeper judged the real behavior proof convincing. label Jun 12, 2026
@clawsweeper clawsweeper Bot added rating: 🌊 off-meta tidepool PR readiness rating does not apply to this item. and removed rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. mantis: telegram-visible-proof Mantis should capture Telegram visible proof. labels Jun 12, 2026
@clawsweeper clawsweeper Bot added proof: sufficient ClawSweeper judged the real behavior proof convincing. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. mantis: telegram-visible-proof Mantis should capture Telegram visible proof. and removed rating: 🌊 off-meta tidepool PR readiness rating does not apply to this item. labels Jun 19, 2026
@clawsweeper clawsweeper Bot added rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. merge-risk: 🚨 message-delivery 🚨 May drop, duplicate, misroute, suppress, or wrongly target messages. and removed rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. labels Jun 20, 2026
@vincentkoc

Copy link
Copy Markdown
Member

Thanks for the fix here. This behavior is now covered by #98063, which landed as c99add6. Closing this PR to keep the queue clean because it is superseded by the merged canonical fix.

@vincentkoc vincentkoc closed this Jul 1, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

mantis: telegram-visible-proof Mantis should capture Telegram visible proof. merge-risk: 🚨 message-delivery 🚨 May drop, duplicate, misroute, suppress, or wrongly target messages. P2 Normal backlog priority with limited blast radius. proof: sufficient ClawSweeper judged the real behavior proof convincing. proof: supplied External PR includes structured after-fix real behavior proof. rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. size: XS status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants