Defer turn-skip counter until finalization succeeds

onAssistantMessageStart increments streamedTurnCount before attempting updateMattermostPost. If that update fails (for example API/network errors or the post being removed), the later final-delivery loop still skips one turn, leaving that turn stuck as partial streamed content with no authoritative final message. Increment this counter only after a successful finalization (or roll it back on failure) so failed finalization still falls through to normal final delivery.

Useful? React with 👍 / 👎.

Good catch. Fixed in 913adeb — streamedTurnCount is now decremented in the catch block so deliver() falls back to normal re-delivery when finalization fails.

Preserve replyToId when creating streamed posts

The streaming path initializes streamReplyToId from threadRootId only, so first streamed posts ignore payload.replyToId. For top-level inbound posts where reply threading is driven by reply-to directives or reply mode, this publishes streaming output as a top-level post, and later final edits cannot relocate it to the intended thread. The stream-post target should be resolved with the same reply-root logic used by final non-streaming sends.

Useful? React with 👍 / 👎.

This is inherited from the base branch (#33506) where streamReplyToId was deliberately set to threadRootId only — see the comment block at line 1648-1651. The payload.replyToId is not available in onPartialReply callbacks (they only carry {text, mediaUrls}), so the streaming path cannot use it. For replyToMode: "all" (our setup), threadRootId is already the correct thread root. No change needed here.

streamedTurnCount incremented before finalization can succeed

streamedTurnCount++ happens at line 1947, before the updateMattermostPost call at line 1958. If that network call throws (caught at line 1962), the count has already been incremented but the streaming post was never finalized with its complete text — it remains with whatever partial content was last emitted by the interval ticker.

When the core later delivers the full final reply array, deliver will see streamedTurnCount > 0, decrement it, and silently skip this turn. The user is left with a post showing truncated content, and the complete text for that turn is never sent.

The fix is to decrement streamedTurnCount in the catch block so deliver can fall back to normal re-delivery:

This is a comment left during a code review.
Path: extensions/mattermost/src/mattermost/monitor.ts
Line: 1947-1966

Comment:
**`streamedTurnCount` incremented before finalization can succeed**

`streamedTurnCount++` happens at line 1947, before the `updateMattermostPost` call at line 1958. If that network call throws (caught at line 1962), the count has already been incremented but the streaming post was never finalized with its complete text — it remains with whatever partial content was last emitted by the interval ticker.

When the core later delivers the full final reply array, `deliver` will see `streamedTurnCount > 0`, decrement it, and silently skip this turn. The user is left with a post showing truncated content, and the complete text for that turn is never sent.

The fix is to decrement `streamedTurnCount` in the catch block so `deliver` can fall back to normal re-delivery:

```suggestion
                  // Wait for any in-flight patch to complete before finalizing.
                  const deadline = Date.now() + 2000;
                  while (patchSending && Date.now() < deadline) {
                    await new Promise<void>((r) => setTimeout(r, 20));
                  }

                  // Finalize the old streaming message with its complete text.
                  if (!finalizeText) return;
                  try {
                    await updateMattermostPost(blockStreamingClient, finalizeId, {
                      message: finalizeText,
                    });
                    runtime.log?.(`stream-patch finalized turn ${finalizeId}`);
                  } catch (err) {
                    // Undo the increment so deliver() can re-deliver this turn's content
                    // rather than silently skipping it after a failed finalization.
                    streamedTurnCount--;
                    logVerboseMessage(
                      `mattermost stream-patch turn finalize failed: ${String(err)}`,
                    );
                  }
```

How can I resolve this? If you propose a fix, please make it concise.

Fixed in 913adeb — streamedTurnCount-- in the catch block.

Unused imports added

resolveAllowlistProviderRuntimeGroupPolicy and resolveDefaultGroupPolicy are imported here but never referenced anywhere in channel.ts (only appear in the import statement). These should be removed to avoid dead imports.

This is a comment left during a code review.
Path: extensions/mattermost/src/channel.ts
Line: 17-18

Comment:
**Unused imports added**

`resolveAllowlistProviderRuntimeGroupPolicy` and `resolveDefaultGroupPolicy` are imported here but never referenced anywhere in `channel.ts` (only appear in the import statement). These should be removed to avoid dead imports.

```suggestion
  normalizeAccountId,
  setAccountEnabledInConfigSection,
```

How can I resolve this? If you propose a fix, please make it concise.

Removed in 913adeb.

streamedTurnCount incremented before confirming successful finalization

streamedTurnCount++ is called at line 1947 unconditionally, before the updateMattermostPost call that actually finalizes the turn. If that network call throws (caught at line 1962), the count is left incremented but the streaming post still contains only the last partial/in-flight text — not the complete turn text.

When deliver later runs for this turn (as part of the core's final reply array), it sees streamedTurnCount > 0 and skips re-delivery, so the complete turn text is silently dropped. The user ends up with a post showing intermediate partial content.

The count should only be incremented when finalization actually succeeds, or the catch block should decrement it to let deliver handle recovery:

// Finalize the old streaming message with its complete text.
if (!finalizeText) return;
try {
  await updateMattermostPost(blockStreamingClient, finalizeId, {
    message: finalizeText,
  });
  runtime.log?.(`stream-patch finalized turn ${finalizeId}`);
} catch (err) {
  logVerboseMessage(
    `mattermost stream-patch turn finalize failed: ${String(err)}`,
  );
  // Undo the count so deliver() can re-deliver this turn as a new post.
  streamedTurnCount--;
}

This is a comment left during a code review.
Path: extensions/mattermost/src/mattermost/monitor.ts
Line: 1947-1966

Comment:
**`streamedTurnCount` incremented before confirming successful finalization**

`streamedTurnCount++` is called at line 1947 unconditionally, before the `updateMattermostPost` call that actually finalizes the turn. If that network call throws (caught at line 1962), the count is left incremented but the streaming post still contains only the last partial/in-flight text — not the complete turn text.

When `deliver` later runs for this turn (as part of the core's final reply array), it sees `streamedTurnCount > 0` and skips re-delivery, so the complete turn text is silently dropped. The user ends up with a post showing intermediate partial content.

The count should only be incremented when finalization actually succeeds, or the `catch` block should decrement it to let `deliver` handle recovery:

```typescript
// Finalize the old streaming message with its complete text.
if (!finalizeText) return;
try {
  await updateMattermostPost(blockStreamingClient, finalizeId, {
    message: finalizeText,
  });
  runtime.log?.(`stream-patch finalized turn ${finalizeId}`);
} catch (err) {
  logVerboseMessage(
    `mattermost stream-patch turn finalize failed: ${String(err)}`,
  );
  // Undo the count so deliver() can re-deliver this turn as a new post.
  streamedTurnCount--;
}
```

How can I resolve this? If you propose a fix, please make it concise.

Fixed in 913adeb — same as above.

Unused imports added to channel.ts

resolveAllowlistProviderRuntimeGroupPolicy and resolveDefaultGroupPolicy are imported here but never referenced anywhere in channel.ts. Both functions are already imported and used in monitor.ts (lines 27–28 and 702–704). These can be removed to avoid dead imports.

This is a comment left during a code review.
Path: extensions/mattermost/src/channel.ts
Line: 17-18

Comment:
**Unused imports added to `channel.ts`**

`resolveAllowlistProviderRuntimeGroupPolicy` and `resolveDefaultGroupPolicy` are imported here but never referenced anywhere in `channel.ts`. Both functions are already imported and used in `monitor.ts` (lines 27–28 and 702–704). These can be removed to avoid dead imports.

```suggestion
```

How can I resolve this? If you propose a fix, please make it concise.

Removed in 913adeb.

Duplicate MattermostAttachment type / unused exports

MattermostAttachment is already exported from interactions.ts (line 245), and MattermostAttachmentAction roughly mirrors MattermostButton from the same file. Neither of the two new types added here is imported or referenced anywhere in the files changed by this PR — channel.ts and monitor.ts only import createMattermostClient, deleteMattermostPost, normalizeMattermostBaseUrl, and updateMattermostPost from this module.

If these types are preparatory ground-work for an upcoming change, a comment to that effect would help reviewers understand why they're here. Otherwise, they risk confusing future contributors who stumble on two separate (and structurally different) MattermostAttachment shapes in the same package.

This is a comment left during a code review.
Path: extensions/mattermost/src/mattermost/client.ts
Line: 178-198

Comment:
**Duplicate `MattermostAttachment` type / unused exports**

`MattermostAttachment` is already exported from `interactions.ts` (line 245), and `MattermostAttachmentAction` roughly mirrors `MattermostButton` from the same file. Neither of the two new types added here is imported or referenced anywhere in the files changed by this PR — `channel.ts` and `monitor.ts` only import `createMattermostClient`, `deleteMattermostPost`, `normalizeMattermostBaseUrl`, and `updateMattermostPost` from this module.

If these types are preparatory ground-work for an upcoming change, a comment to that effect would help reviewers understand why they're here. Otherwise, they risk confusing future contributors who stumble on two separate (and structurally different) `MattermostAttachment` shapes in the same package.

How can I resolve this? If you propose a fix, please make it concise.

The MattermostAttachment and MattermostAttachmentAction types in client.ts are part of the base branch (#33506), not added by this PR. They appear as a diff here only because this branch is based on #33506. Once #33506 merges, this PR will be rebased onto main and the types will not appear in our diff. No change needed from our side.

Misleading early-return guard after streamedTurnCount increment

if (!finalizeText) return is only reachable when streamMessageId was non-null (the earlier guard at line 1934 ensures that). Because streamMessageId is set exclusively inside schedulePatch's interval tick — which requires a non-empty pendingPatchText to proceed — and pendingPatchText is never cleared between interval ticks and onAssistantMessageStart, finalizeText will always be non-empty when this line is reached. The guard is dead code in practice.

The concern is that the pattern reads like:

streamedTurnCount++;       // count is incremented …
if (!finalizeText) return; // … but we return without finalizing the post

which mirrors exactly the pre-fix footgun that was caught in previous review threads. A brief comment clarifying why the condition can never be true (or a console.assert/runtime assertion during development) would make the invariant explicit and prevent future developers from incorrectly reasoning that streamedTurnCount could be left inflated here.

This is a comment left during a code review.
Path: extensions/mattermost/src/mattermost/monitor.ts
Line: 1955-1956

Comment:
**Misleading early-return guard after `streamedTurnCount` increment**

`if (!finalizeText) return` is only reachable when `streamMessageId` was non-null (the earlier guard at line 1934 ensures that). Because `streamMessageId` is set exclusively inside `schedulePatch`'s interval tick — which requires a non-empty `pendingPatchText` to proceed — and `pendingPatchText` is never cleared between interval ticks and `onAssistantMessageStart`, `finalizeText` will always be non-empty when this line is reached. The guard is dead code in practice.

The concern is that the pattern reads like:
```
streamedTurnCount++;       // count is incremented …
if (!finalizeText) return; // … but we return without finalizing the post
```
which mirrors exactly the pre-fix footgun that was caught in previous review threads. A brief comment clarifying why the condition can never be true (or a `console.assert`/runtime assertion during development) would make the invariant explicit and prevent future developers from incorrectly reasoning that `streamedTurnCount` could be left inflated here.

How can I resolve this? If you propose a fix, please make it concise.

Good catch — the ordering was misleading. Fixed in 43b14e2: streamedTurnCount++ is now moved after the if (!finalizeText) return guard, with a comment explaining the invariant. The guard itself is dead code in practice (the interval only fires when pendingPatchText is non-empty, and pendingPatchText is finalizeText), but having the increment appear before it created exactly the footgun pattern that was flagged in the earlier review round.

Avoid patching current stream with older re-delivered turn

If turn-finalization fails in onAssistantMessageStart, the catch block decrements streamedTurnCount so that older turn is re-delivered later; however this branch patches whatever streamMessageId is currently active. In a multi-turn run, that is typically the newest turn’s streaming post, so the re-delivered older turn can overwrite the latest in-progress message before its own final payload arrives, producing out-of-order/corrupted output on transient Mattermost API failures.

Useful? React with 👍 / 👎.

Good catch. Fixed in 0ce9120 — streamMessageId is now cleared before returning in the skip path. This ensures that if a turn finalization failed (catch block decremented the counter) and the turn reaches re-delivery via deliver(), it creates a new post rather than patching whatever streaming post is currently active for a newer turn.

Deliver attachments before skipping streamed final turns

This skip returns before any media handling, so previously streamed turns lose attachments whenever their final payload includes mediaUrls. The streaming path only patches text (onPartialReply ignores media), so in multi-turn responses where an earlier turn has media and a later tool call starts another turn, those attachments are dropped permanently instead of being sent as follow-up posts.

Useful? React with 👍 / 👎.

Valid edge case. onPartialReply only carries text (no mediaUrls), so streaming turns never include media. In practice, media in multi-turn responses is sent in the final turn only. However the skip path does silently drop media if it were present in an intermediate turn payload. Deferring this to a follow-up: the fix would add a media-only delivery before the return in the skip block, but the added complexity is out of scope for this PR. Opening a follow-up issue.

Preserve active stream ID while skipping finalized turns

The skip branch for previously streamed turns unconditionally sets streamMessageId = null, but this branch also runs while the newest turn is still streaming; in a normal multi-turn run, that clears the ID of the active latest post before its final payload is processed. As a result, the last final reply can no longer take the patchMattermostPost path and falls back to sending a new post, leaving the original streamed post orphaned/stale and producing duplicate output for the final turn.

Useful? React with 👍 / 👎.

Reset turn state even when no stream post exists yet

Returning early on if (!streamMessageId) misses assistant-turn boundaries whenever the prior turn ended before the 200ms ticker created a post (or while the first send is still in flight), which is common for short tool-call turns. In that case pendingPatchText/interval state carries into the next turn, so subsequent final delivery can patch the newer streamed post with an older turn’s text (out-of-order/corrupted turn mapping) instead of keeping turns isolated.

Useful? React with 👍 / 👎.

Fixed in 3dc639d: onAssistantMessageStart now calls stopPatchInterval() and clears pendingPatchText/lastSentText unconditionally at the top, before the streamMessageId guard. Short tool-call turns where the 200ms ticker has not fired yet (no stream post) now get a proper boundary reset, preventing stale text/interval state from the previous turn carrying into the next.

streamMessageId = null in skip block orphans the last turn's streaming post

In the happy path (all turn finalizations succeed), deliver is called for turns 1 through N in sequence. For a 3-turn response:

• streamedTurnCount = 2 and streamMessageId = turn3Id when deliver is first called.
• deliver(turn 1) enters this skip block and sets streamMessageId = null, clearing turn3Id.
• deliver(turn 2) skips similarly.
• deliver(turn 3) falls through with streamMessageId = null, misses the isFinal && streamMessageId && payload.text patch branch, and lands in normal delivery — creating a duplicate post for turn 3 while the actual streaming post is left with only the last-tick content.

The comment describes a failure-recovery scenario (finalization failed → count decremented → this turn falls through), but in that scenario streamedTurnCount would have already been decremented back to 0 before deliver runs, meaning this skip block would never even be entered for that turn. The clearing therefore does not protect against the described race and instead introduces a duplicate-post regression in the common case.

The fix is to remove the streamMessageId = null line from this block. The current active streaming post (for the latest turn) should remain valid until deliver for that turn explicitly patches or clears it.

This is a comment left during a code review.
Path: extensions/mattermost/src/mattermost/monitor.ts
Line: 1815-1820

Comment:
**`streamMessageId = null` in skip block orphans the last turn's streaming post**

In the happy path (all turn finalizations succeed), `deliver` is called for turns 1 through N in sequence. For a 3-turn response:

- `streamedTurnCount = 2` and `streamMessageId = turn3Id` when `deliver` is first called.
- `deliver(turn 1)` enters this skip block and sets `streamMessageId = null`, clearing `turn3Id`.
- `deliver(turn 2)` skips similarly.
- `deliver(turn 3)` falls through with `streamMessageId = null`, misses the `isFinal && streamMessageId && payload.text` patch branch, and lands in normal delivery — creating a **duplicate post** for turn 3 while the actual streaming post is left with only the last-tick content.

The comment describes a failure-recovery scenario (finalization failed → count decremented → this turn falls through), but in that scenario `streamedTurnCount` would have already been decremented back to 0 before `deliver` runs, meaning this skip block would never even be entered for that turn. The clearing therefore does not protect against the described race and instead introduces a duplicate-post regression in the common case.

The fix is to remove the `streamMessageId = null` line from this block. The current active streaming post (for the latest turn) should remain valid until `deliver` for that turn explicitly patches or clears it.

```suggestion
          if (isFinal && streamedTurnCount > 0) {
            streamedTurnCount--;
            runtime.log?.(
              `stream-patch skipping already-delivered turn (${streamedTurnCount} remaining)`,
            );
            return;
          }
```

How can I resolve this? If you propose a fix, please make it concise.

Fixed in 3151cef: removed the streamMessageId = null line from the skip block.

Your analysis is correct — in the happy path, deliver(turn 1) and deliver(turn 2) both cleared the active stream ID, so deliver(turn 3) fell through with streamMessageId = null and landed in normal delivery, creating a duplicate post while the actual streaming post stayed with tick-level content.

The failure-recovery rationale in the comment was also wrong: when finalization fails, the catch block decrements streamedTurnCount back before deliver runs, so the skip block is never entered for that turn anyway. The clearing served no purpose and broke the common case.

updateMattermostPost vs patchMattermostPost inconsistency

onAssistantMessageStart uses updateMattermostPost (which sends a PUT /posts/{id} with {id, message}) to finalize a turn, while the rest of the streaming path (schedulePatch, flushPendingPatch, and the final-delivery branch in deliver) consistently use the newly-added patchMattermostPost (PUT /posts/{id}/patch with just {message}).

The Mattermost "update post" endpoint is a full-replace: if the post has attachments, file_ids, or custom props, sending only {id, message} may silently strip them. The /patch endpoint is specifically designed for partial updates and only touches the fields you supply. Using patchMattermostPost here would be consistent with the rest of the streaming code and safer:

This is a comment left during a code review.
Path: extensions/mattermost/src/mattermost/monitor.ts
Line: 1955-1957

Comment:
**`updateMattermostPost` vs `patchMattermostPost` inconsistency**

`onAssistantMessageStart` uses `updateMattermostPost` (which sends a `PUT /posts/{id}` with `{id, message}`) to finalize a turn, while the rest of the streaming path (`schedulePatch`, `flushPendingPatch`, and the final-delivery branch in `deliver`) consistently use the newly-added `patchMattermostPost` (`PUT /posts/{id}/patch` with just `{message}`).

The Mattermost "update post" endpoint is a full-replace: if the post has attachments, `file_ids`, or custom `props`, sending only `{id, message}` may silently strip them. The `/patch` endpoint is specifically designed for partial updates and only touches the fields you supply. Using `patchMattermostPost` here would be consistent with the rest of the streaming code and safer:

```suggestion
                    await patchMattermostPost(blockStreamingClient, {
                      postId: finalizeId,
                      message: finalizeText,
                    });
```

How can I resolve this? If you propose a fix, please make it concise.

Fixed in 3151cef: switched to patchMattermostPost in onAssistantMessageStart.

Good catch on the API semantics — the PUT update endpoint does a full replace and would silently strip file_ids or custom props. The PATCH endpoint is the correct partial-update path and is now used consistently across all streaming code paths (schedulePatch, flushPendingPatch, deliver final edit, and onAssistantMessageStart turn finalization).

Deliver media payloads when finalizing streamed replies

The final-streaming fast path edits the existing post and returns whenever payload.text is present, but it never forwards payload.mediaUrl(s). For any final reply that contains both text and media (a supported ReplyPayload shape used by deliverMattermostReplyPayload), the caption gets patched while all attachments are silently dropped. This regression only appears when edit-in-place streaming is enabled, and it causes user-visible data loss.

Useful? React with 👍 / 👎.

Clear streamed previews when the final payload is skipped

This cleanup only runs inside deliver(), but createReplyDispatcher() never calls deliver() when the final payload normalizes to silent, heartbeat, or empty (src/auto-reply/reply/normalize-reply.ts). If onPartialReply has already created or updated a Mattermost preview, those skip paths leave patchInterval alive and the partial post orphaned in-channel after withReplyDispatcher() completes. Adding an onSkip cleanup here would keep suppressed finals from leaking streamed drafts.

Useful? React with 👍 / 👎.

Fixed in 1ef86dc: added an onSettled hook that runs regardless of whether deliver() was called. If a streamed preview post exists when the dispatcher exits (silent/heartbeat/empty response), stopPatchInterval() is called and the orphaned post is deleted best-effort.