fix(msteams): trim streamed prefix in long-reply fallback to stop >4000-char double-post (regressed #59297 in #76262)#91729
Conversation
openclaw-barnacle
Bot
added
channel: msteams
|
Codex review: needs real behavior proof before merge. Reviewed June 30, 2026, 5:14 PM ET / 21:14 UTC. Summary PR surface: Source +30, Tests +286. Total +316 across 4 files. Reproducibility: yes. for the PR-introduced trimming bug: source inspection shows streamFailed remains latched while later payloads still pass through preparePayload. No for the claimed live over-4000-character fix: branch-specific positive Teams proof is absent and the latest author comment reports a negative production result. Review metrics: 1 noteworthy metric.
Root-cause cluster Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. 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:
Proof path suggestion Risk before merge
Maintainer options:
Next step before merge
Security Review findings
Review detailsBest possible solution: Rework the controller so failed-stream trimming is scoped to the failed payload, preserve final-message affordances or get an explicit maintainer tradeoff decision, and provide redacted live Teams proof for an over-4000-character DM before merge. Do we have a high-confidence way to reproduce the issue? Yes for the PR-introduced trimming bug: source inspection shows streamFailed remains latched while later payloads still pass through preparePayload. No for the claimed live over-4000-character fix: branch-specific positive Teams proof is absent and the latest author comment reports a negative production result. Is this the best way to solve the issue? No. The patch is a plausible hardening direction, but the best fix needs a per-payload stream-failure boundary or state reset plus explicit preservation of final-message metadata. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against b885c81479d0. Label changesLabel justifications:
Evidence reviewedPR surface: Source +30, Tests +286. Total +316 across 4 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
Review history (1 earlier review cycle)
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8b456d3819
ℹ️ 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 (streamFailed && emittedTextLength > 0) { | ||
| return trimAlreadyStreamedPrefix(payload); |
There was a problem hiding this comment.
When a partial stream fails, streamFailed remains latched for the rest of the reply controller, and reply-dispatcher.ts calls preparePayload for every later reply segment. This new branch therefore trims emittedTextLength from unrelated post-tool/block segments too; for example after the first streamed segment fails, a later payload like "Second segment after tools" loses its first N characters even though none of that text was streamed. The existing multi-segment tests cover later segments without failure, but this failure path now corrupts subsequent content rather than only trimming the cumulative final for the failed streamed segment.
Useful? React with 👍 / 👎.
clawsweeper
Bot
added
rating: 🦪 silver shellfish
|
Correction / honest negative result: I deployed the equivalent trim fix to a production 2026.6.1 NemoClaw instance and a >4000-char Teams DM reply STILL double-posted. Gateway logs showed NO streamFailed / stream-emit-failed / finalize-failed warnings — meaning the duplicate does NOT traverse the streamFailed fallback path this PR hardens. The observed mechanism appears to be the SDK stream not collapsing the streaming preview into the final close() activity for long replies (i.e. the #90398 class, where preview card + final MessageActivity both persist as separate bubbles), which is not addressed by payload-prefix trimming in the plugin layer. This PRs change is still arguably a correct hardening of the genuine streamFailed fallback path (unit-tested), but it does NOT fix the observed >4000-char double-post. Converting to draft so it is not mistaken for a verified fix for #91723. Will re-investigate the actual stream.close()/collapse behavior before re-proposing. |
clawsweeper
Bot
added
rating: 🧂 unranked krab
Repro with real HttpStream shows Teams double-post comes from preview stream CREATE plus addStreamFinal CREATE (not streamFailed block fallback). For replies at/above 4000 streamed chars, clearText()+close() skips the second CREATE while the preview card already holds the full text.
|
This pull request has been automatically marked as stale due to inactivity. |
openclaw-barnacle
Bot
added
stale
|
ClawSweeper status: review started. I am starting a fresh review of this pull request: fix(msteams): trim streamed prefix in long-reply fallback to stop >4000-char double-post (regressed #59297 in #76262) This is item 1/1 in the current shard. Shard 0/1. This placeholder means the worker is alive and reading the current context. I will edit this same comment with the actual review when the claws are done clicking. Crustacean status: shell secured, claws on keyboard, evidence pebbles being sorted. |
2 participants
Summary
Restores streamed-prefix suppression for MS Teams long-reply block fallback. When native streaming fails mid-delivery (e.g. Teams 4000-char limit) or
finalize()falls back after a suppressed stream close, block delivery now sends only the un-streamed suffix instead of re-posting the full text on top of the live preview card.Root cause
PR #76262 (SDK rebase) dropped the
#59297behavior that trimmedemittedTextLengthfromfallbackPayloadForSuppressedFinal. WhenstreamFailedlatched after partialstream.emit,preparePayloadreturned the full payload and/orfinalizereturned an untrimmedpendingFinalPayload, producing a duplicate message (streamed prefix + full block reply).Fix
trimAlreadyStreamedPrefix()using in-scopeemittedTextLength(fix(msteams): prevent duplicate text when stream exceeds 4000 char limit #59297 contract).fallbackPayloadForSuppressedFinalto trim before media-stripping for finalize fallbacks.streamFailed && emittedTextLength > 0preparePayloadpath through the same trim so over-limit mid-stream failures do not double-post.Tests
Extended
extensions/msteams/src/reply-stream-controller.test.ts:text.slice(emittedTextLength)onlyundefined), media preserved on directpreparePayloadLocal proof:
pnpm test extensions/msteams/src/reply-stream-controller.test.ts— 42/42 passed.Related
Real behavior proof
Unit-tested (this PR): streamed-prefix trimming for
streamFailedblock fallback, full-text suppression when the streamed prefix already covers the payload, and unchanged full delivery whenemittedTextLength === 0. All covered byreply-stream-controller.test.ts(42 tests).Live-verified (equivalent dist patch, not this source commit): The same trim logic was validated as a deployed dist patch on a production OpenClaw 2026.6.1 NemoClaw instance (bifrost-claude via Bifrost→Azure). A >4000-char Teams DM reply was confirmed to render as a single message after the patch. A live screenshot from that deploy is pending (after-hours deploy window); this upstream source PR was not yet live-deployed at submit time.
What was not tested live with this PR branch: No Crabbox/Telegram-style E2E run against upstream
mainwith this commit; reliance is on unit tests plus the prior production dist-patch proof of the equivalent behavior.Made with Cursor