← Back to PRs

#20382: fix: move suppressToolErrors check before mutating tool check

by klawdius-noodle open 2026-02-18 21:01 View on GitHub →
agents size: XS
## Summary The `shouldShowToolErrorWarning()` function was checking for mutating tools *before* checking `suppressToolErrors`, which caused exec tool errors (and other mutating tool errors) to always bypass the suppression setting. ## Root Cause In the current logic: ```js if (isMutatingToolError) return true; // Always runs first for exec if (params.suppressToolErrors) return false; // Never reached! ``` Since `exec` is in `MUTATING_TOOL_NAMES`, the function always returns `true` for exec errors, never reaching the `suppressToolErrors` check. ## Fix Move the `suppressToolErrors` check to run *before* the mutating tool check: ```js if (params.suppressToolErrors) return false; // Takes precedence if (isMutatingToolError) return true; ``` ## Testing Tested with `messages.suppressToolErrors: true` — exec tool errors are now properly suppressed in Telegram. Fixes #20140 <!-- greptile_comment --> <h3>Greptile Summary</h3> This PR reorders the condition checks in `shouldShowToolErrorWarning()` so that `suppressToolErrors` takes precedence over the mutating tool error check. While the intent is to allow suppression of `exec` tool errors via `messages.suppressToolErrors`, the change breaks a **safety-critical invariant**: mutating tool errors (from `exec`, `write`, `bash`, etc.) were intentionally shown even when `suppressToolErrors` is set, to prevent users from thinking a destructive action succeeded when it actually failed. - The existing test `"still shows mutating tool errors when messages.suppressToolErrors is enabled"` (`payloads.e2e.test.ts:244`) directly contradicts this change and will fail. - The codebase already has `suppressToolErrorWarnings` as the nuclear override that suppresses all errors including mutating ones — this is the correct flag for the use case described in the PR. - A more targeted fix (e.g., a separate config option or adjusting which tools are classified as mutating) would be safer than changing the global precedence order. <h3>Confidence Score: 1/5</h3> - This PR breaks a safety invariant that prevents silent suppression of mutating tool errors, and will cause an existing test to fail. - Score of 1 reflects that the change directly contradicts an existing test assertion ("still shows mutating tool errors when messages.suppressToolErrors is enabled") and breaks a documented safety invariant (line 258 comment: "Always surface mutating tool failures so we do not silently confirm actions that did not happen"). The existing `suppressToolErrorWarnings` flag already provides the nuclear override capability the PR is trying to achieve. - `src/agents/pi-embedded-runner/run/payloads.ts` — the reordered condition checks break the intended suppression hierarchy <sub>Last reviewed commit: c3b65ef</sub> <!-- greptile_other_comments_section --> <sub>(2/5) Greptile learns from your feedback when you react with thumbs up/down!</sub> <!-- /greptile_comment -->

Most Similar PRs