feat(sdk): run tail recovery for every chat.agent and let a transcript storage own the model's context - #4895
Conversation
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Team Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThe SDK adds an optional Merge Risk: 🟡 Moderate · up to Storage-owned context can duplicate head-start conversation history in model prompts, producing incorrect responses and unnecessary token usage. This should be fixed before merge; the deprecation warning and tests also need alignment with SDK conventions. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Description checkExplanation The description clearly explains the implementation and behavior changes, but it does not follow the required template. It omits the issue reference, checklist, testing section, changelog, and screenshots section. Full details: Docstring CoverageExplanation Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 4 functions across 2 files. (1 skipped: 1 too large.) ✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
c5e1f5d to
bac2a16
Compare
@trigger.dev/build
trigger.dev
@trigger.dev/core
@trigger.dev/python
@trigger.dev/react-hooks
@trigger.dev/redis-worker
@trigger.dev/rsc
@trigger.dev/schema-to-json
@trigger.dev/sdk
commit: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Team
Run ID: a0fcdd61-0077-4264-95e3-15387d6c6adf
📒 Files selected for processing (3)
packages/trigger-sdk/src/v3/ai.tspackages/trigger-sdk/src/v3/transcriptStorage.tspackages/trigger-sdk/test/transcript-gate-split.test.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 5 remain after this review.
📜 Review details
🧰 Additional context used
📓 Path-based instructions (9)
Always import from `@trigger.dev/sdk`.
📄 CodeRabbit inference engine (AGENTS.md)
Files:
packages/trigger-sdk/test/transcript-gate-split.test.tspackages/trigger-sdk/src/v3/transcriptStorage.tspackages/trigger-sdk/src/v3/ai.ts
We use vitest exclusively.
📄 CodeRabbit inference engine (AGENTS.md)
Files:
packages/trigger-sdk/test/transcript-gate-split.test.ts
**Prefer static imports over dynamic imports.**
📄 CodeRabbit inference engine (AGENTS.md)
Files:
packages/trigger-sdk/test/transcript-gate-split.test.tspackages/trigger-sdk/src/v3/transcriptStorage.tspackages/trigger-sdk/src/v3/ai.ts
Add crumbs as you write code — not just when debugging.
📄 CodeRabbit inference engine (AGENTS.md)
Files:
packages/trigger-sdk/test/transcript-gate-split.test.tspackages/trigger-sdk/src/v3/transcriptStorage.tspackages/trigger-sdk/src/v3/ai.ts
In the Trigger.dev SDK (packages/trigger-sdk), prefer isomorphic code like fetch and ReadableStream instead of Node.js-specific code
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
packages/trigger-sdk/test/transcript-gate-split.test.tspackages/trigger-sdk/src/v3/transcriptStorage.tspackages/trigger-sdk/src/v3/ai.ts
Use vitest for all tests in the Trigger.dev repository
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
packages/trigger-sdk/test/transcript-gate-split.test.ts
Use function declarations instead of default exports
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
packages/trigger-sdk/test/transcript-gate-split.test.tspackages/trigger-sdk/src/v3/transcriptStorage.tspackages/trigger-sdk/src/v3/ai.ts
Use types over interfaces for TypeScript Avoid using enums; prefer string unions or const objects instead
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
packages/trigger-sdk/test/transcript-gate-split.test.tspackages/trigger-sdk/src/v3/transcriptStorage.tspackages/trigger-sdk/src/v3/ai.ts
When creating or editing OTEL metrics (counters, histograms, gauges), ensure metric attributes have low cardinality by using only enums, booleans, bounded error codes, or bounded shard IDs Do not use high-cardinality attributes in OTEL metr...
📄 CodeRabbit inference engine (.cursor/rules/otel-metrics.mdc)
Files:
packages/trigger-sdk/test/transcript-gate-split.test.tspackages/trigger-sdk/src/v3/transcriptStorage.tspackages/trigger-sdk/src/v3/ai.ts
🧠 Learnings (2)
📚 Learning: 2026-05-19T22:37:47.286Z
Learnt from: ericallam
Repo: triggerdotdev/trigger.dev PR: 3671
File: packages/trigger-sdk/test/recovery-boot.test.ts:456-457
Timestamp: 2026-05-19T22:37:47.286Z
Learning: In `packages/trigger-sdk` (Trigger.dev SDK), `logger.warn` (and other SDK logger methods) should route to the Trigger.dev structured logger sink, not to `console.warn`. In SDK tests, `vi.spyOn(console, "warn")` (or similar console spies) should only be used to suppress stray console output; reviewers should not suggest asserting on `console.warn` spies to verify SDK-internal warning/fallback log behavior. Use the SDK’s structured-logger outputs/capture approach instead of console spies.
Applied to files:
packages/trigger-sdk/test/transcript-gate-split.test.tspackages/trigger-sdk/src/v3/ai.ts
📚 Learning: 2026-08-16T18:36:58.179Z
Learnt from: ericallam
Repo: triggerdotdev/trigger.dev PR: 4537
File: packages/trigger-sdk/test/normalizeKeyString.test.ts:1-2
Timestamp: 2026-08-16T18:36:58.179Z
Learning: For related SDK `chat.agent` tests in the Trigger.dev repository—including chat channels, handover, snapshot, and transport-event coverage—keep new test files under `packages/trigger-sdk/test/` rather than colocating them with the `packages/trigger-sdk/src/v3/` source files.
Applied to files:
packages/trigger-sdk/test/transcript-gate-split.test.ts
| console.warn( | ||
| `[chat.agent] \`hydrateMessages\` on "${agentId}" is deprecated. Give the agent a transcript ` + | ||
| "storage instead: `save` receives every change to the conversation and `loadContext` " + | ||
| "lets the application own the model's context, with crash recovery and durable " + | ||
| "compaction that `hydrateMessages` never had." | ||
| ); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Route the deprecation warning through the SDK logger.
The new warning uses console.warn, and the test locks that behavior. SDK warnings must use the Trigger.dev structured logger sink.
packages/trigger-sdk/src/v3/ai.ts#L5135-L5140: replaceconsole.warnwithlogger.warnand retain the migration guidance in the structured message and fields.packages/trigger-sdk/test/transcript-gate-split.test.ts#L171-L181: verify the warning through the SDK structured-logger capture path instead of asserting calls toconsole.warn.
Based on learnings: “logger.warn (and other SDK logger methods) should route to the Trigger.dev structured logger sink, not to console.warn.”
📍 Affects 2 files
packages/trigger-sdk/src/v3/ai.ts#L5135-L5140(this comment)packages/trigger-sdk/test/transcript-gate-split.test.ts#L171-L181
Source: Learnings
| // and it's safe because the route handler isn't subject to the | ||
| // `/in/append` 512 KiB cap. | ||
| if ( | ||
| !hydrateMessages && |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not seed head-start messages when loadContext owns context.
When storage defines loadContext, this condition seeds payload.headStartMessages into previousMessages. The turn-zero path then passes the same array as incomingMessages. A callback that combines both inputs duplicates the head-start history in the model prompt.
Gate this path on !loadContextHook, not only !hydrateMessages. Add a storage-loadContext head-start regression test.
Proposed fix
- !hydrateMessages &&
+ !loadContextHook &&📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| !hydrateMessages && | |
| !loadContextHook && |
| import { mockChatAgent } from "../src/v3/test/index.js"; | ||
|
|
||
| import type { LanguageModelV3StreamPart } from "@ai-sdk/provider"; | ||
| import type { UIMessage } from "ai"; | ||
| import { simulateReadableStream, streamText } from "ai"; | ||
| import { MockLanguageModelV3 } from "ai/test"; | ||
| import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Use a testcontainers-backed integration path.
This test uses mockChatAgent and MockLanguageModelV3. Repository tests must use Vitest without mocks. Replace these mocked runtime dependencies with testcontainers-backed dependencies.
As per coding guidelines: “We use vitest exclusively. Never mock anything - use testcontainers instead.”
Source: Coding guidelines
…t storage own the model's context One condition used to decide three things at boot: whether to read the persisted transcript, whether to replay the session's output tail, and whether to replay unacknowledged input. Registering hydrateMessages switched all three off, so an app that owned its own context also lost crash recovery, and no application can rebuild the tail its dead run had already emitted. The replays and onRecoveryBoot now run for every agent; only the transcript read is skipped for hydrateMessages. The storage can now declare loadContext, which the runtime calls on every turn and action in place of the accumulated transcript, the role hydrateMessages played, while save keeps receiving every change. hydrateMessages is deprecated with a one-time warning, and configuring it together with a storage that has loadContext is an error.
bac2a16 to
bb1acb8
Compare
1 participant
Summary
Crash recovery now runs for every
chat.agent, and a transcript storage can own the model's context throughloadContext, which replaces the deprecatedhydrateMessageshook.One condition used to decide three things at boot: whether to read the persisted transcript, whether to replay the session's output tail, and whether to replay unacknowledged input. Registering
hydrateMessagesswitched all three off, so an app that owned its own context also lost recovery of a half-written answer and of the messages the dead run never acknowledged, which no application can rebuild from its own rows because the tail is exactly what never got persisted. The replays andonRecoveryBootnow run for every agent; only the transcript read is skipped forhydrateMessages, whose store is the source of truth.Design
A storage may declare
loadContext(scope, event). When present, the runtime calls it on every turn and action, with the same eventhydrateMessagesreceived, and uses what it returns as the model's context, whilesavekeeps receiving every change.hydrateMessageskeeps working with a one-time warning at definition. Setting it together with a storage that hasloadContextis an error.For
hydrateMessagesagents this is the behaviour-changing PR: they regain tail recovery. Runtime writes stay off for them, as documented on the actions page.