Skip to content

fix(think): messenger delivery, prompt truncation and media eviction - #2387

Merged
threepointone merged 6 commits into
mainfrom
fix/think-messenger-prompt-review
Sep 27, 2026
Merged

threepointone merged 6 commits into
mainfrom
fix/think-messenger-prompt-review

Conversation

@threepointone

@threepointone threepointone commented Sep 27, 2026 •

Copy link
Copy Markdown
Contributor

Follow-ups from a review of recently landed Think messenger/prompt PRs (#2356, #2339, #2355, #2354, #2325, #2324, #2323, #2321, #1839). Each fix has a regression test.

Messengers

  • Follow-ups during long turns are no longer dropped. The Chat SDK thread lock (30s) was never extended during a Think turn and queue entries expired after 90s. The in-repo state adapter gains a lockHeartbeat option (re-extends held locks every TTL/3), messengers enable it, and queued entries now last 30 minutes.
  • Queued messages drain after a restart. After an interrupted reply is recovered, messages still queued for the thread are answered. This calls two private Chat SDK methods (getLockKey, drainQueue), feature-checked at runtime.
  • A failing startTyping no longer stops the turn; typing is refreshed every 4s until first text; no blank message before the apology on an interrupted empty turn.
  • Burst messages that mention the bot are detected even when the adapter doesn't set isMention; unsubscribed threads get answered and subscribed.
  • Stored message metadata comes from the serializable event (no raw provider payloads/bytes); inline attachment data (Buffer, ArrayBuffer, typed array, Blob) is used when there's no fetchData; a full-buffer view is returned without copying.

Prompt construction

  • Media eviction moves in the same steps as truncation, so it no longer rewrites messages the model still sees at full fidelity or breaks the prompt cache once per media message.
  • Older tool results: inline images/file bytes are replaced with a marker and inline-text files count toward the budget (fix(think): truncate older tool results after toModelOutput (#2014) #2339). Results are matched to calls by position, not id alone.
  • The assistant metadata writer receives continuation, and a recovery continuation no longer overwrites existing metadata such as createdAt (think: write assistant-message metadata via forwarded messageMetadata (#1873) #2321).
  • The default model is resolved only if beforeTurn doesn't override it; getGateway() returning a Promise fails with a clear error.
  • truncationStep = Infinity means truncation is off (previously became 1). Documented alongside the hydration byte-budget caveat.

Verified on a combined branch with the other review follow-ups: pnpm run check, agents chat/workers/react, ai-chat, and Think suites all pass.

Conflicts expected with the sandbox/model PR (resolveModel, hooks.test.ts) and the recovery PR (messenger test agent); both sides should be kept.


Devin Review

…review fixes

- keep Chat SDK thread locks alive (agents/chat-sdk lockHeartbeat) and give
  messenger queue entries a 30 minute TTL
- drain queued messages after an interrupted reply is recovered
- refresh the typing indicator until first text, tolerate its failure, and
  never post a blank reply before the apology
- detect mentions on skipped burst messages, including unsubscribed threads
- strip raw payloads/bytes from persisted metadata; map inline attachment data
- keep start metadata when a recovery continuation extends a message
- truncate inline media/text file items in older tool results and match
  results by message position
- align media eviction with the stepped truncation cutoff; truncationStep
  Infinity disables truncation
- resolve the default model lazily; reject async getGateway clearly

Co-authored-by: Cursor <cursoragent@cursor.com>
@changeset-bot

changeset-bot Bot commented Sep 27, 2026 •

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: ef6c108

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages
Name Type
@cloudflare/think Patch
agents Patch
@cloudflare/agent-think Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@devin-ai-integration devin-ai-integration Bot left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note

Newer findings are available below. Devin Review posted a newer report on this PR, in addition to the findings presented here.

Devin Review found 4 potential issues.

Devin Review

Comment thread packages/think/src/think.ts
Comment thread packages/agents/src/chat-sdk/adapter.ts
Comment on lines +478 to +483
if (
typeof internals.getLockKey !== "function" ||
typeof internals.drainQueue !== "function"
) {
return;
}

@devin-ai-integration devin-ai-integration Bot Sep 27, 2026 •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 Recovery draining relies on private SDK methods

The recovery drain calls private Chat SDK methods. If either method disappears, the feature check silently skips draining and queued messages wait for another webhook. Consider testing this path against the supported SDK range.

Devin Review


Was this helpful? React with 👍 or 👎 to provide feedback.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a guard test in messengers.test.ts (165344b) asserting Chat.prototype.getLockKey and drainQueue exist with the expected arity, so a Chat SDK upgrade that drops them fails CI rather than silently skipping the drain. Runtime feature check kept.

Comment thread docs/think/messengers.md
Comment on lines +189 to +193
Think keeps the thread lock alive for as long as a reply runs, and keeps queued
messages for 30 minutes (the Chat SDK default is 90 seconds), so a message that
arrives during a slow reply is still answered after it. Set `queueEntryTtlMs`
in a `ConcurrencyConfig` to change that. Messages still queued when the Durable
Object restarts are answered once the interrupted reply is recovered.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 Check upstream documentation sync

The docs rules require matching updates in cloudflare-docs. These Think documentation changes need corresponding upstream MDX updates, potentially in a separate PR.

Devin Review


Was this helpful? React with 👍 or 👎 to provide feedback.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Acknowledged — the matching cloudflare-docs update will go in a separate PR there.

@agent-think

agent-think Bot commented Sep 27, 2026 •

Copy link
Copy Markdown
Contributor

🟡 agents import sizes: 1 entry point grew

Entry point Exports Largest gzip change Size now
🟡 agents/chat 1 resized +271 B (+7.94%) 3.6 KiB
Changed exports (1)
Import Gzip change Size now
🟡 agents/chat#truncateOlderToolResults +271 B (+7.94%) 3.6 KiB
How this works

Each runtime export is bundled on its own, minified, and gzipped. Changes smaller than 100 B, or smaller than 1% and 1 KiB, are ignored. Growth over 10% or 5 KiB is marked 🔴. This report is informational and does not fail CI. The workflow artifact contains every measurement.

Compared f9049991 → ef6c108b · workflow run · reported by agent-think[bot]

@pkg-pr-new

pkg-pr-new Bot commented Sep 27, 2026 •

Copy link
Copy Markdown

Open in StackBlitz

agents

npm i https://pkg.pr.new/agents@2387

@cloudflare/ai-chat

npm i https://pkg.pr.new/@cloudflare/ai-chat@2387

@cloudflare/codemode

npm i https://pkg.pr.new/@cloudflare/codemode@2387

hono-agents

npm i https://pkg.pr.new/hono-agents@2387

@cloudflare/shell

npm i https://pkg.pr.new/@cloudflare/shell@2387

@cloudflare/think

npm i https://pkg.pr.new/@cloudflare/think@2387

@cloudflare/voice

npm i https://pkg.pr.new/@cloudflare/voice@2387

@cloudflare/worker-bundler

npm i https://pkg.pr.new/@cloudflare/worker-bundler@2387

commit: ef6c108

…p lock heartbeats on disconnect

- Extension beforeTurn snapshots use the subclass model override instead of
  reading ctx.model, which lazily resolved the default model (and its AI
  binding/gateway) even when unused.
- ChatSdkStateAdapter.disconnect() clears lock heartbeat timers, and
  heartbeats never start or tick while disconnected.
- Test that the installed chat package still has the private
  getLockKey/drainQueue methods the recovery drain relies on.

Co-authored-by: Cursor <cursoragent@cursor.com>
devin-ai-integration[bot]

This comment was marked as resolved.

A failed messenger reply recovery that the Tasks run will replay no longer
starts the queue drain, so follow-ups queued behind it are not answered ahead
of the original reply. Success and terminal failures still drain.

Co-authored-by: Cursor <cursoragent@cursor.com>

@devin-ai-integration devin-ai-integration Bot left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note

Newer findings are available below. Devin Review posted a newer report on this PR, in addition to the findings presented here.

Devin Review found 2 new potential issues.

Devin Review

export function messengerContextFromEvent(
event: MessengerEvent
): MessengerContext {
const serializable = serializableMessengerEvent(event);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Live messenger attachments lose their bytes

During live turns, messengerContextFromEvent strips data and fetch from attachments. Hooks and tools reading getMessengerContext() can no longer open them.

Learn more

A messenger event contains live attachment data and download functions. Previously the persisted user-message metadata used the live message objects, so getMessengerContext exposed those attachment capabilities during a turn. Now the metadata is serialized before storage, and deliverMessengerReply also passes a serialized event into the live turn. Both paths leave hooks and tools with attachment descriptions but no way to read the bytes.

Example: A custom adapter sends a file with data: Buffer.from("report") and no URL or fetch metadata. A tool reads getMessengerContext()?.message?.attachments[0]; its data and fetch are both undefined, although the webhook supplied the bytes.

Recommended fix: Keep the persisted metadata sanitized, but pass the original live event to chatWithMessengerContext and let the turn-scoped context expose it while the turn is active. Ensure recovered turns still use the serializable snapshot.

Devin Review


Was this helpful? React with 👍 or 👎 to provide feedback.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a regression from this PR. On main the runtime already built snapshotEvent = serializableMessengerEvent(event) in both enqueueReply and answer (chat-sdk.ts ~521/557) and passed that snapshot to chatWithMessengerContext; serializableMessengerMessage already dropped data/fetch/raw. getMessengerContext() returns the active turn context first, so tools/hooks in a live turn never saw the bytes. This PR only sanitizes the metadata fallback, which previously exposed live objects only via the in-memory cache until rehydration (they became {} once persisted). Exposing bytes during a turn would be a new capability, would diverge live vs recovered turns, and can't cross the sub-agent hop — code that needs bytes can use the live event in toEvent/conversation, or fetchMetadata. Leaving as-is.

Comment thread packages/agents/src/chat/truncate-older-messages.ts
threepointone and others added 2 commits September 27, 2026 18:14
…sults

convertToModelMessages emits provider-executed results in the assistant message, so they never occupy a position among tool-message results. Clarify producesToolResult and cover the case with the real converter.

Co-authored-by: Cursor <cursoragent@cursor.com>
…rompt-review

Co-authored-by: Cursor <cursoragent@cursor.com>

# Conflicts:
#	packages/think/src/tests/agents/messengers.ts

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Devin Review found 2 new potential issues.

Devin Review

return chunk;
}
const messageMetadata = Object.fromEntries(
Object.entries(chunk.messageMetadata).filter(([key]) => !(key in existing))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Recovery drops new metadata keys

When a recovery continuation adds toString metadata, withoutOverwrittenStartMetadata drops it despite no existing value. The in check matches inherited object properties, so the new key never reaches the assistant message.

Suggested change
Object.entries(chunk.messageMetadata).filter(([key]) => !(key in existing))
Object.entries(chunk.messageMetadata).filter(
([key]) => !Object.prototype.hasOwnProperty.call(existing, key)
)

Devin Review


Was this helpful? React with 👍 or 👎 to provide feedback.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Valid — fixed in follow-up #2389: the filter now uses Object.prototype.hasOwnProperty.call(existing, key), with a regression test for toString/constructor metadata keys.

Comment on lines +491 to +493
if (options.surface.startTyping && typingRefreshMs > 0) {
typingTimer = setInterval(() => void sendTyping(), typingRefreshMs);
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Typing indicator reappears after reply

If startTyping takes longer than a refresh interval, sendTyping leaves requests in flight when the first text posts. Those requests can show typing after the answer has arrived.

Learn more

A refresh tick starts a new typing request without waiting for the previous request. Stopping the interval prevents new ticks but cannot stop one already in flight. If that request finishes after the first reply text is posted, the provider displays a stale typing indicator.

Example: A refresh starts at 4 seconds and finishes at 8 seconds, while the first reply text posts at 5 seconds. The indicator appears again at 8 seconds, after the answer.

Recommended fix: Serialize refresh requests and coordinate any pending startTyping call with the first post. Where the provider supports cancellation, abort pending indicator calls when visible delivery begins.

Devin Review


Was this helpful? React with 👍 or 👎 to provide feedback.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Valid — fixed in follow-up #2389: typing requests are serialized, no new ones start once posting begins, and the first post waits (bounded) for any in-flight request, with an ordering regression test.

@threepointone
threepointone merged commit 9f70bc8 into main Sep 27, 2026
16 checks passed
@threepointone
threepointone deleted the fix/think-messenger-prompt-review branch September 27, 2026 18:23
@github-actions github-actions Bot mentioned this pull request Oct 2, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant