Skip to content

fix(discord): preserve streamed replies after tool warnings#84359

Closed
joshavant wants to merge 2 commits into
mainfrom
fix/discord-preview-tool-warning
Closed

fix(discord): preserve streamed replies after tool warnings#84359
joshavant wants to merge 2 commits into
mainfrom
fix/discord-preview-tool-warning

Conversation

@joshavant

@joshavant joshavant commented May 19, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Problem: Discord draft-preview finalization could clear a streamed final answer when a later failed-tool warning arrived as an error final.
  • Solution: keep later error finals out of the draft-preview finalizer after a non-error final has started delivery, while still delivering the warning normally.
  • What changed: added a bounded Discord delivery state guard and a regression test for final answer plus later tool-warning delivery.
  • What did NOT change (scope boundary): standalone error finals still use the existing draft cleanup path; no Discord config or streaming defaults changed.

Motivation

  • Fixes a user-visible Discord streaming failure where the final answer can be replaced by or cleared after a later tool-call failure warning.

Change Type (select all)

  • Bug fix
  • Feature
  • Refactor required for the fix
  • Docs
  • Security hardening
  • Chore/infra

Scope (select all touched areas)

  • Gateway / orchestration
  • Skills / tool execution
  • Auth / tokens
  • Memory / storage
  • Integrations
  • API / contracts
  • UI / DX
  • CI/CD / infra

Linked Issue/PR

Real behavior proof (required for external PRs)

Behavior addressed: Discord streamed final replies should remain visible when a later failed-tool warning is delivered.
Real environment tested: Source checkout on macOS with focused Discord monitor tests; pre-patch AWS Crabbox attempts from current main did not reproduce the exact destructive deletion state.
Exact steps or command run after this patch: node scripts/run-vitest.mjs extensions/discord/src/monitor/message-handler.process.test.ts; pnpm check:changed
Evidence after fix: The new regression test keeps the preview message id after delivering a normal final followed by an isError final, and the full Discord process test file passes.
Observed result after fix: The finalized preview remains intact and the failed-tool warning is delivered through normal Discord reply delivery.
What was not tested: A successful after-patch live AWS Discord deletion reproduction/fix comparison, because current main did not reliably reproduce the destructive live state during investigation.
Before evidence (optional but encouraged): The new regression test failed before the fix because the later error final cleared the already-finalized draft preview.

Root Cause (if applicable)

  • Root cause: Discord final delivery let later error finals re-enter the finalizable draft-preview adapter after a non-error final had already promoted the draft preview into the visible answer.
  • Missing detection / guardrail: There was no regression test for multiple final payloads where a normal answer is followed by a failed-tool warning.
  • Contributing context (if known): Shared finalizer fallback cleanup is correct for standalone error finals, but not after a successful preview finalization has already made the draft durable.

Regression Test Plan (if applicable)

  • Coverage level that should have caught this:
    • Unit test
    • Seam / integration test
    • End-to-end test
    • Existing coverage already sufficient
  • Target test or file: extensions/discord/src/monitor/message-handler.process.test.ts
  • Scenario the test should lock in: Discord partial streaming finalizes a preview, then a later isError final is delivered without clearing the preview.
  • Why this is the smallest reliable guardrail: The failure is in Discord monitor delivery/finalizer coordination and can be reproduced with the existing process test harness.
  • Existing test that already covers this (if any): N/A
  • If no new test is added, why not: N/A

User-visible / Behavior Changes

Discord streamed replies should no longer disappear when a later failed-tool warning is delivered.

Diagram (if applicable)

Before:
final answer -> preview finalized -> later tool warning -> preview cleanup clears answer

After:
final answer -> preview finalized -> later tool warning delivered normally -> answer remains visible

Security Impact (required)

  • New permissions/capabilities? No
  • Secrets/tokens handling changed? No
  • New/changed network calls? No
  • Command/tool execution surface changed? No
  • Data access scope changed? No
  • If any Yes, explain risk + mitigation: N/A

Repro + Verification

Environment

  • OS: macOS local checkout; AWS Crabbox was used during pre-patch investigation
  • Runtime/container: Node/Vitest local test harness
  • Model/provider: N/A for regression test
  • Integration/channel (if any): Discord
  • Relevant config (redacted): Discord streamMode partial in the regression test

Steps

  1. Run the focused Discord process test file.
  2. Verify the new regression test finalizes a preview, sends a later isError final, and does not clear the preview.
  3. Run changed typecheck/lint/guards.

Expected

  • The final answer preview remains visible and the warning is delivered separately.

Actual

  • The final answer preview remains visible and the warning is delivered separately.

Evidence

  • Failing test/log before + passing after
  • Trace/log snippets
  • Screenshot/recording
  • Perf numbers (if relevant)

Human Verification (required)

What you personally verified (not just CI), and how:

  • Verified scenarios: new regression failed before the fix and passed after; full Discord process test file passed; changed checks passed.
  • Edge cases checked: standalone error final behavior remains covered by the adjacent existing test.
  • What you did not verify: post-patch live AWS Discord reproduction, because the exact destructive live state was not reliably reproducible on current main.

Review Conversations

  • I replied to or resolved every bot review conversation I addressed in this PR.
  • I left unresolved only the conversations that still need reviewer or maintainer judgment.

Compatibility / Migration

  • Backward compatible? Yes
  • Config/env changes? No
  • Migration needed? No
  • If yes, exact upgrade steps: N/A

Risks and Mitigations

  • Risk: accidentally changing standalone error-final draft cleanup.
    • Mitigation: the guard only skips draft-preview finalization for an error final that follows a non-error final delivery start; existing standalone error-final coverage remains intact.

@openclaw-barnacle openclaw-barnacle Bot added the channel: discord Channel integration: discord label May 19, 2026
@openclaw-barnacle openclaw-barnacle Bot added size: XS maintainer Maintainer-authored PR labels May 19, 2026
@clawsweeper

clawsweeper Bot commented May 19, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge.

Workflow note: Future ClawSweeper reviews update this same comment in place.

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.

Summary
The PR adds Discord delivery state so a later isError final skips draft-preview finalization after a non-error final starts, adds a regression test, and adds a changelog entry.

Reproducibility: yes. Source inspection shows the current-main Discord finalizer can send later error finals through fallback cleanup that clears the attached draft, and the linked issue discussion reports the matching real Discord symptom.

PR rating
Overall: 🦐 gold shrimp
Proof: 🦪 silver shellfish
Patch quality: 🐚 platinum hermit
Summary: The patch is focused and source-supported, but proof on this branch is mock-only and the branch overlaps a broader active replacement PR.

Rank-up moves:

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.

Real behavior proof
Needs real behavior proof before merge: The PR body reports focused mocked Discord monitor tests and changed checks, but no successful after-patch live Discord proof; add redacted runtime output, logs, screenshots, or a recording, then update the PR body to trigger re-review or ask a maintainer to comment @clawsweeper re-review.

Mantis proof suggestion
A short real Discord visual or transcript proof would materially reduce the remaining message-delivery risk. A maintainer can ask Mantis to capture proof by posting a new PR comment that starts with the OpenClaw Mantis account mention, followed by:

visual task: verify in Discord that a failed tool-call warning does not remove the final `delivery survived` reply.

Risk before merge

  • This PR branch has only mocked/focused test proof; the body explicitly says a successful after-patch live AWS Discord deletion comparison was not tested.
  • The active replacement PR fix(discord): preserve streamed replies after tool warnings #84169 covers the same issue plus a warning-before-final variant, so maintainers should choose one canonical branch to avoid divergent Discord delivery semantics.
  • The diff changes Discord final-payload routing around draft cleanup, so exact-head Discord process checks and, ideally, redacted live Discord proof matter before merge.

Maintainer options:

  1. Require live Discord proof before merge (recommended)
    Ask for redacted Discord transcript, API output, terminal output, or a short recording showing a failed-tool warning no longer removes the final reply on this exact branch.
  2. Land this narrower branch intentionally
    Maintainers can accept this branch if they only want to guard later error finals while preserving the current standalone error-final cleanup path.
  3. Continue with the replacement PR
    If the warning-before-final variant should be included, pause or close this PR and finish fix(discord): preserve streamed replies after tool warnings #84169 instead.

Next step before merge
Needs maintainer handling because the PR has a protected label, mock-only proof, and overlaps active replacement PR #84169.

Security
Cleared: The diff only changes Discord delivery state handling, a colocated regression test, and a changelog entry; it does not touch credentials, permissions, dependencies, CI, scripts, or package resolution.

Review details

Best possible solution:

Choose one canonical Discord fix, keep standalone terminal-error cleanup intentional, and merge only after exact-head checks plus real Discord proof that the final answer and warning remain visible.

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

Yes. Source inspection shows the current-main Discord finalizer can send later error finals through fallback cleanup that clears the attached draft, and the linked issue discussion reports the matching real Discord symptom.

Is this the best way to solve the issue?

Mostly yes for the later-warning order this branch targets. The remaining maintainer choice is whether this narrower guard is preferred over the active replacement PR that also covers warning-before-final ordering.

Label changes:

  • add P1: The PR targets a user-visible Discord workflow where a generated final reply can disappear after a failed-tool warning.
  • add merge-risk: 🚨 message-delivery: The patch changes Discord final-payload routing and draft cleanup, which directly affects whether replies or warnings are preserved, cleared, or duplicated.
  • add rating: 🦐 gold shrimp: Current PR rating is 🦐 gold shrimp because proof is 🦪 silver shellfish, patch quality is 🐚 platinum hermit, and The patch is focused and source-supported, but proof on this branch is mock-only and the branch overlaps a broader active replacement PR.
  • add status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The PR body reports focused mocked Discord monitor tests and changed checks, but no successful after-patch live Discord proof; add redacted runtime output, logs, screenshots, or a recording, then update the PR body to trigger re-review or ask a maintainer to comment @clawsweeper re-review.

Label justifications:

  • P1: The PR targets a user-visible Discord workflow where a generated final reply can disappear after a failed-tool warning.
  • merge-risk: 🚨 message-delivery: The patch changes Discord final-payload routing and draft cleanup, which directly affects whether replies or warnings are preserved, cleared, or duplicated.
  • rating: 🦐 gold shrimp: Current PR rating is 🦐 gold shrimp because proof is 🦪 silver shellfish, patch quality is 🐚 platinum hermit, and The patch is focused and source-supported, but proof on this branch is mock-only and the branch overlaps a broader active replacement PR.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The PR body reports focused mocked Discord monitor tests and changed checks, but no successful after-patch live Discord proof; add redacted runtime output, logs, screenshots, or a recording, then update the PR body to trigger re-review or ask a maintainer to comment @clawsweeper re-review.

What I checked:

Likely related people:

  • Patrick Erichsen: Blame and git log -S show the Discord draft-preview adapter and shared live-preview finalizer entering this file in the progress-preview commit. (role: introduced behavior; confidence: high; commits: d60ab485114a; files: extensions/discord/src/monitor/message-handler.process.ts, extensions/discord/src/monitor/message-handler.process.test.ts, src/channels/message/live.ts)
  • Peter Steinberger: Recent history shows multiple Discord message-handler changes and the 2026.5.18 release commit carrying the current file state. (role: recent area contributor and release integrator; confidence: high; commits: 50a2481652b6, a32a3e23314d, eb297824167b; files: extensions/discord/src/monitor/message-handler.process.ts, extensions/discord/src/monitor/message-handler.process.test.ts)
  • Vincent Koc: Recent history includes Discord/channel streaming config normalization and several adjacent Discord message-handler touches. (role: adjacent streaming contributor; confidence: medium; commits: 0fdf9e874b7e, 505b980f63df; files: extensions/discord/src/monitor/message-handler.process.ts)
  • joshavant: The linked issue is assigned to this person and this branch is their signed follow-up, but I did not find prior merged history in the central Discord delivery files. (role: current follow-up owner; confidence: medium; commits: ce6db0b93c33, 1ff545bdc2f8; files: extensions/discord/src/monitor/message-handler.process.ts, extensions/discord/src/monitor/message-handler.process.test.ts, CHANGELOG.md)

Codex review notes: model gpt-5.5, reasoning high; reviewed against a00e7d3898cf.

@clawsweeper clawsweeper Bot added rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P1 High-priority user-facing bug, regression, or broken workflow. merge-risk: 🚨 message-delivery 🚨 May drop, duplicate, misroute, suppress, or wrongly target messages. labels May 19, 2026
@clawsweeper

clawsweeper Bot commented May 19, 2026

Copy link
Copy Markdown
Contributor

ClawSweeper PR egg

🎁 Pass real behavior proof to wake the egg and unlock a hatchable treat.

Where did the egg go?
  • The egg game starts only after the PR passes the real-behavior proof check.
  • Before that, no creature, rarity, or ASCII portrait is rolled. The treat waits for real proof.
  • This is still just collectible flavor: proof affects review readiness, not creature quality.

@clawsweeper clawsweeper Bot mentioned this pull request May 20, 2026
Merged
@joshavant joshavant closed this May 20, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

channel: discord Channel integration: discord maintainer Maintainer-authored PR merge-risk: 🚨 message-delivery 🚨 May drop, duplicate, misroute, suppress, or wrongly target messages. P1 High-priority user-facing bug, regression, or broken workflow. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. size: XS status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Bug: Discord streamed reply disappears after tool-call failure warning

1 participant

@joshavant