fix(discord): backport recovered tool warning suppression#87452
Conversation
(cherry picked from commit da27904)
|
Codex review: needs real behavior proof before merge. Reviewed May 27, 2026, 8:23 PM ET / 00:23 UTC. Summary PR surface: Source +60, Tests +137, Config +2, Other +2. Total +201 across 10 files. Reproducibility: no. high-confidence live reproduction/proof is present. The source path and focused tests cover the recovered-warning fallback behavior, but there is no redacted live Discord transcript or runtime log showing after-fix behavior. Review metrics: 2 noteworthy metrics.
Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Proof guidance: Mantis proof suggestion Risk before merge
Maintainer options:
Next step before merge Security Review detailsBest possible solution: Merge the backport only after maintainer approval and real Discord proof showing duplicate recovered warnings are suppressed while warning-only and failed-final fallback warnings still deliver. Do we have a high-confidence way to reproduce the issue? No high-confidence live reproduction/proof is present. The source path and focused tests cover the recovered-warning fallback behavior, but there is no redacted live Discord transcript or runtime log showing after-fix behavior. Is this the best way to solve the issue? Yes on code shape: the branch moves fallback replay into the dispatcher settled path and adds a freshness recheck. The remaining issue is merge readiness, not a clear code defect. AGENTS.md: found and applied where relevant. Codex review notes: model gpt-5.5, reasoning high; reviewed against 748510b7a3e1. Label changesLabel changes:
Label justifications:
Evidence reviewedPR surface: Source +60, Tests +137, Config +2, Other +2. Total +201 across 10 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
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9d7f57c02e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| dispatchAborted = true; | ||
| return; | ||
| } | ||
| await deliverPendingToolWarningFinalIfNeeded(); |
There was a problem hiding this comment.
Recheck freshness before replaying pending warnings
When an older Discord turn only has a metadata tool-warning final, deliverDiscordPayload stores it as pendingToolWarningFinal with visibleReplySent: false, and this replay happens only after dispatchChannelInboundReply has returned. The foreground reply fence in src/auto-reply/dispatch.ts performs its second freshness check inside beforeDeliver and is ended before that return, so a newer same-session/channel message that starts and delivers in the gap is no longer able to cancel this fallback; the stale warning can then be posted after the newer answer. Keep this fallback inside the dispatcher/fence path or repeat the same freshness check before sending it.
Useful? React with 👍 / 👎.
|
ClawSweeper PR egg 🎁 Pass real behavior proof to wake the egg and unlock a hatchable treat. Where did the egg go?
|
Dependency Changes DetectedThis PR changes dependency-related files. Maintainers should confirm these changes are intentional. Changed files:
Maintainer follow-up:
|
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 764dbf08d9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if ( | ||
| !userFacingFinalDelivered && | ||
| (!finalReplyStartNotified || userFacingFinalDeliveryFailed) |
There was a problem hiding this comment.
Queue warning fallback after typing start
When a Discord turn has already fired onReplyStart (normal model runs do this for typing/draft start) but the only final that reaches delivery is a metadata non-terminal tool warning, this condition leaves pendingToolWarningFinal unset because finalReplyStartNotified is already true. The new fallback is then dropped instead of being sent on onSettled; the added warning-only test does not catch this because its mock never invokes replyOptions.onReplyStart. Gate this on an actually delivered/recovered final (or failed delivery state) rather than the reply-start notification.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 71af832aa5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const settledResult = await params.dispatcherOptions.onSettled?.(); | ||
| if (isExplicitlyVisibleDelivery(settledResult)) { |
There was a problem hiding this comment.
Preserve visible partial errors from settled delivery
When onSettled performs the only visible send and then throws an error annotated as sentBeforeError/visibleReplySent (for example the WhatsApp media coalescer marks errors this way after flushing at least one deferred media reply), this inlined call no longer runs the isVisiblePartialDeliveryError catch path that the removed helper had. In a same-session/channel race, an older dispatch waiting in the foreground fence can miss that the newer turn already produced visible output and then continue with its stale fallback; wrap this onSettled call with the same visible-partial-error handling used for normal and fresh-settled deliveries.
Useful? React with 👍 / 👎.
Summary
release/2026.5.27.Verification
pnpm test extensions/discord/src/monitor/message-handler.process.test.ts -- --reporter=dot(78 passed on release branch)pnpm check:changedvia Blacksmith Testboxtbx_01ksnwp61m86gqtb00r6pffzea, Actions run26545083807(passed; release/main comparison widened to all lanes)Real behavior proof
Behavior addressed: backports the Discord recovered-tool-warning suppression from #87451 to the current release branch.
Real environment tested: local focused Vitest on the release branch plus Blacksmith Testbox changed gate.
Exact steps or command run after this patch:
pnpm test extensions/discord/src/monitor/message-handler.process.test.ts -- --reporter=dot;pnpm check:changed.Evidence after fix: focused Discord shard passed 78 tests on the release branch; Testbox
tbx_01ksnwp61m86gqtb00r6pffzeaexited 0.Observed result after fix: metadata-marked recovered warnings are held as fallback only; warning-only and mutating-failure warning behavior remains visible.
What was not tested: live Discord production message send against Hetzner.