Repository navigation
Conversation
… chunks Signed-off-by: xh123637 <3324733895@qq.com>
|
@xh123637 is attempting to deploy a commit to the Vercel Team on Vercel. A member of the Team first needs to authorize it. |
…elta In chunk mode every text delta went out as its own chunks append, and the ChatStreamer flushes on any append carrying chunks, so each token-sized delta cost an awaited chat.appendStream call — where the markdown_text path is buffered by the streamer up to its 256-char buffer size. Buffer chunk-mode text in the adapter up to the same size and send it as a single markdown_text chunk, flushing at the ordering boundaries: before a structured chunk, at rotation (riding the stop call), and before the final stop. Signed-off-by: xh123637 <3324733895@qq.com>
|
Good catch — confirmed against the pinned Fixed by buffering chunk-mode text in the adapter, at the same cadence the streamer's own buffer gives the
Tests: three new cases in the |
bensabic
left a comment
There was a problem hiding this comment.
Thanks for the detailed write-up and the follow-up on buffering. Requesting changes because I can't reproduce the bug this PR describes with any @slack/web-api version the adapter allows. Against the real ChatStreamer, this diff only changes how many API calls a stream makes.
What I checked
- Checked out
0621f63f(merges cleanly into currentmain).vitest runinpackages/adapter-slackgives 1089/1089 passing;tsc --noEmitand Biome are clean. CI is green. - Read
ChatStreamerin@slack/web-api7.18.0 (the lockfile), 7.19.0 (the newest version^7.18.0allows), and 8.3.0. All three behave the same way:append()andstop()addmarkdown_textto an internal buffer and send it only aschunks: [{ type: "markdown_text" }]onstartStream/appendStream/stopStream. Themarkdown_textrequest parameter is never forwarded. The last version I found that forwarded it was 7.11.0, which is below the adapter's declared range. - Wire-level smoke test. I used the real
WebClient+ChatStreamerand replaced onlychat.startStream,chat.appendStreamandchat.stopStreamwith recorders that also act like Slack: they throwstreaming_mode_mismatchif amarkdown_textparameter arrives after chunks. I ran the same scenarios onmainand on this PR. Neither branch ever sends the parameter, and neither triggers the mismatch. Here are the HTTP calls:
| Scenario | main |
this PR |
|---|---|---|
| A. plan + task, then text (the #1074 shape) | start[plan] → append[task] → stop[md] |
start[plan] → append[task] → append[md] → stop[] |
| D. text between task updates | start[plan] → append[task] → append[md, task] ×2 → stop[md] (5 calls) |
start[plan] → append[task] → (append[md] → append[task]) ×2 → append[md] → stop[] (8 calls) |
| B. 600 chars in 5-char deltas after a task | the 602-char text goes out in one append |
identical |
| E. rotation in chunk mode | stop[md, task] → start[plan, task] → stop[md] |
stop[md, task] → start[plan, task] → append[md] → stop[] |
The text, its order and its chunk form are the same on both branches. The only wire difference is extra round-trips.
- Mutation testing on the new code (against
index.test.ts): 5 of 11 mutants are caught. The ones that survive are all bookkeeping that only matters for expiry recovery, plus one dead branch. Details are inline.
Requested changes
- Show the root cause at the HTTP layer, or change the description and changeset. The new tests mock
chatStream, so they check what the adapter passes to the streamer, and onmainthat never reached Slack in parameter form. All three new tests can fail onmainwhile the actual Slack traffic is already correct. Please add a regression test that replacesclient.chat.*Stream(notchatStream) and fails onmain. If you can't write one, the bug in #1074 has some other cause. Two possibilities worth ruling out first:- The reporter's runtime resolved a different
@slack/web-apithan 7.18+ (checknpm ls @slack/web-apiin their app, and any overrides). - Slack drops
markdown_textchunks that arrive inchat.stopStreamonce a stream is in chunk mode. Onmainthat's where the trailing text goes, and this PR happens to move it into a separateappendStream. If this is the real cause, the fix should target it directly, and it has to cover the rotationstoptoo (scenario E), which still sends text instopStreamchunks on this branch.
- The reporter's runtime resolved a different
- Don't add API calls without a demonstrated benefit. Today, buffered text rides along in the same request as the next structured chunk and as the final
stop. This PR splits each of those into two requests (+1 call per structured chunk that follows text, +1 per stream).appendStreamis Tier 4, and tool-heavy agents (the Mastragroupedcase) send a lot of task updates. - Remove the duplicated buffer, or justify it.
pendingChunkTextplus the hard-codedCHUNK_MODE_TEXT_FLUSH_SIZE = 256rebuild buffering thatChatStreameralready does, and the copy can silently drift from the SDK default. If (1) shows no wire bug, the simplest correct change may be no adapter change at all. - The changeset will publish a release note for a failure mode that supported SDK versions can't produce. Please update it once (1) is settled.
Happy to re-review once there's a wire-level repro.
| const prefixed = withSegmentPrefix(text); | ||
| let response: Awaited<ReturnType<typeof segment.streamer.append>> = | ||
| null; | ||
| if (segment.appendedChunks) { |
There was a problem hiding this comment.
This branch has no wire effect with the SDK range the adapter allows. ChatStreamer.append({ markdown_text }) in 7.18/7.19/8.x buffers the text and sends it as chunks: [{ type: "markdown_text", text }]. It never sends the markdown_text request parameter, so main already sends text in chunk form after a plan/task chunk. I confirmed this by recording the real chat.appendStream/chat.stopStream arguments on both branches; neither sends the parameter.
Please add a regression test at that layer (replace client.chat.startStream/appendStream/stopStream, keep the real chatStream) that fails on main. Without one, this branch, the appendedChunks flag and pendingChunkText aren't fixing anything we can observe.
| if (segment.appendedChunks) { | ||
| segment.pendingChunkText += prefixed; | ||
| if ( | ||
| segment.flushNextAppend || |
There was a problem hiding this comment.
As far as I can tell, segment.flushNextAppend is never true here. appendedChunks only becomes true after a successful chunk append (sendStructuredChunk or the rotation replay), and both call markSegmentStarted(), which clears flushNextAppend. Deleting this condition leaves the whole suite green (mutation survived). Please remove it, or add a test that reaches it.
| // failure lands in the same catch as a failed chunk append, | ||
| // mirroring how this append flushes the streamer's own text | ||
| // buffer in markdown mode. | ||
| if (await flushPendingChunkText()) { |
There was a problem hiding this comment.
This turns one HTTP call into two. On main, buffered text and the structured chunk go out together as appendStream { chunks: [md, task_update] } (the streamer prepends its buffer). Here, pending text goes out as its own appendStream first. In my smoke test with text between task updates I measured 5 calls on main and 8 on this branch.
The comment ("mirroring how this append flushes the streamer's own text buffer") also isn't accurate: the streamer flushes its buffer inside the same request. If ordering is the only concern, you could send chunks: [{ type: "markdown_text", text: pending }, chunk] in one append.
Also: removing lastFlushed = lastAppended here leaves the suite green. That bookkeeping decides what gets resent after a message_not_in_streaming_state, so it needs a test (or should go away along with the buffer).
| // streamer never saw; deliver it before stop finalizes the message. | ||
| // Inside the try so an expired segment lands in the recovery below, | ||
| // whose unconfirmed tail still covers the buffered text. | ||
| if (await flushPendingChunkText()) { |
There was a problem hiding this comment.
This adds an appendStream before every final stopStream that has pending text. On main that text rides in the stopStream request itself (chunks: [md]); here stopStream goes out with chunks: []. So "the chat.stopStream calls are unchanged" holds at the mock level but not on the wire.
If moving the trailing text out of stopStream is what actually fixes #1074 (Slack ignoring text chunks on stop in chunk mode), please say so, prove it, and apply it to the rotation stop too, which still sends text in stopStream chunks. Mutation note: dropping lastFlushed = lastAppended here also survives the suite, so the expiry-after-flush path (which should return the expired message without resending) isn't tested.
| ] as ChatAppendStreamArguments["chunks"], | ||
| token, | ||
| }); | ||
| segment.pendingChunkText = ""; |
There was a problem hiding this comment.
The "on failure the text stays buffered" contract isn't tested: clearing pendingChunkText before the await survives the suite, and so does removing the reset in startNextSegment. Both only matter in the expiry-recovery paths, where getting them wrong means duplicated or lost text. If the buffer stays, please cover: (a) the final flush throws message_not_in_streaming_state and the continuation segment carries the text exactly once, and (b) the flush succeeds but stop expires, and nothing is resent.
| const chatStream = vi | ||
| .fn() | ||
| .mockImplementation(() => segments[chatStream.mock.calls.length - 1]); | ||
| mockClientMethod(adapter, "chatStream", chatStream); |
There was a problem hiding this comment.
Mocking chatStream means these tests check what the adapter passes to ChatStreamer, not what reaches Slack. Since the real streamer already turns markdown_text into a markdown_text chunk, all three "fails on main" tests fail at a layer Slack never sees.
Suggest a variant that keeps the real streamer and stubs the HTTP methods:
const chat = (adapter as any)._client.chat;
chat.startStream = vi.fn().mockResolvedValue({ ok: true, ts: "1.1" });
chat.appendStream = vi.fn().mockResolvedValue({ ok: true });
chat.stopStream = vi.fn().mockResolvedValue({ ok: true, ts: "1.1" });
// assert: no appendStream/stopStream call has a `markdown_text` keyIf that assertion passes on main for the #1074 stream shape, the bug lives somewhere else.
| "@chat-adapter/slack": patch | ||
| --- | ||
|
|
||
| Fix Slack native streaming losing reply text after structured chunks. Once a stream segment had appended `plan_update` or `task_update` chunks, later text deltas were still sent through the `markdown_text` parameter, which Slack rejects with `streaming_mode_mismatch` in chunk mode, so the text never appeared in the final message. Text deltas on a segment that has appended structured chunks — including a segment opened by rotation, which replays the plan and open tasks — are now sent as `markdown_text` chunks. Text sent before any structured chunk is unchanged. |
There was a problem hiding this comment.
With @slack/web-api ^7.18.0, later text deltas were never "sent through the markdown_text parameter". The SDK sends buffered text as markdown_text chunks. As written, this release note describes a failure mode that supported versions can't hit. Please reword it once the actual cause is confirmed.
Fixes #1074
What happens
With native streaming and structured chunks (for example Mastra's
toolDisplay: 'grouped', which streamsplan_update/task_update), the reply's text never shows up in the final Slack message. The message completes with only the plan/task blocks, even though notifications briefly showed the text.Root cause
Once a stream has received
chunksthroughchat.appendStream, Slack rejects later appends that use themarkdown_textparameter withstreaming_mode_mismatch.SlackAdapter.stream()always sent text deltas that way insendDelta, including aftersendStructuredChunkhad appended chunks to the segment. The rejection was not surfaced, and the text was lost.Change
stream()now tracks chunk mode per segment inpackages/adapter-slack/src/index.ts:sendDeltasends the delta aschunks: [{ type: "markdown_text", text }]while the segment is in chunk mode, and keeps using themarkdown_textparameter otherwise, so text streamed before any structured chunk is unchanged.structuredChunksSupportedswitched off) never enters chunk mode, and the post-and-edit fallback is untouched.The
chat.stopStreamcalls are unchanged: the reporter's reproduction showsstopStreamcompleting the message, and rotation's stop already pairs its text with the closing task chunks in a single call.Tests
New
native streaming after structured chunksdescribe inpackages/adapter-slack/src/index.test.ts, drivingstream()with a mockedchatStream:markdown_textchunk and no append uses themarkdown_textparameter.markdown_textparameter; text after it switches to chunk form.All three fail on
mainand pass with this change (verified by stashing the fix).pnpm --filter @chat-adapter/slack test: 1,075 passed. The 11 failures are inparseMessagelayout-block tests and one slash-command token test; they fail identically on unmodifiedmainin my environment (5 s timeouts) and are unrelated to streaming.pnpm --filter @chat-adapter/slack typecheck,build, and Biome check on the changed files pass.A changeset for
@chat-adapter/slack(patch) is included.Note on the pinned Slack SDK
The
@slack/web-apiversion currently pinned in this repo (7.18.0) buffersmarkdown_textappends inside itsChatStreamerand flushes them asmarkdown_textchunks, so it does not pass themarkdown_textparameter through to the API itself; the mismatch in #1074 was observed against the real API under the dependency combination in use at the time (adapter 4.42/4.43). This change fixes the contract at the adapter layer — after structured chunks, text is handed to the streamer as chunks — which is also the level this package's streaming tests assert at (they mockchatStreamand assert theappendarguments). Whether the mismatch can surface on the wire still partly depends on the web-api version in use; flagging it here so reviewers can judge.