fix(workflows): one workflow step per LLM call so runs stop blowing the 800s ceiling - #808
Conversation
…he 800s ceiling
runAgentStep wrapped the entire agent loop via stopWhen: stepCountIs(111),
so a single "use step" ran 11-25 minutes. WDK deploys step handlers with
maxDuration: max, which resolves to 800s on Pro, so Vercel killed the
invocation and the step queue redelivered it. Confirmed on prod:
Step "step//./app/lib/workflows/runAgentStep//runAgentStep"
exceeded max retries (4 retries) [USER_ERROR]
with the step recorded at attempt: 5 and an empty {"message":"Unknown error"}
(the signature of a platform kill, not a thrown error). Each attempt was a
complete agent run that mailed the customer again: 45 of the last 100 runs
failed this way, every one at ~72.5 min = 5 attempts x ~870s.
Moves the loop into the workflow body, one journaled step per LLM call:
- runAgentStep drops stopWhen; the AI SDK default isStepCount(1) bounds it
to a single model call plus that call's tool executions. It now takes
modelMessages/originalMessages and returns responseMessages for threading.
- runAgentWorkflow owns the loop, appending each iteration's
responseMessages so iteration N+1 sees iteration N's tool results, and
bounding it at CHAT_AGENT_MAX_ITERATIONS.
- The turn's stream envelope moves up: sendStreamStart/sendStreamFinish are
workflow-level steps and each iteration passes sendStart/sendFinish: false,
so the client renders one assistant message instead of one per iteration.
- buildMessageMetadataCallback takes a seed so usage/cost totals span the
whole turn rather than resetting each iteration.
- convertMessagesStep runs the conversion once, journaled, before the loop.
CHAT_AGENT_STOP_WHEN stays: getGeneralAgent (the non-durable /api/chat
route) still runs its tool loop inside streamText.
Mirrors the reference in vercel-labs/open-agents
apps/web/app/workflows/chat.ts.
Refs recoupable/chat#1918
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThe durable agent workflow converts messages once, executes bounded journaled model-call iterations, threads response messages and metadata across steps, persists assistant state, and emits one stream start and finish for the complete turn. ChangesDurable agent workflow
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing Touches📝 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
lib/chat/const.ts (1)
14-14: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDerive one iteration constant from the other.
CHAT_AGENT_STOP_WHENandCHAT_AGENT_MAX_ITERATIONSboth hard-code111. The comment says they must stay equal "for behavioural parity," but nothing enforces that. A future change to one value will silently break parity with the other.Define a single source of truth and derive both from it.
♻️ Proposed fix to remove the duplicated magic number
-export const CHAT_AGENT_STOP_WHEN = stepCountIs(111); +const CHAT_AGENT_STEP_LIMIT = 111; + +export const CHAT_AGENT_STOP_WHEN = stepCountIs(CHAT_AGENT_STEP_LIMIT); ... -export const CHAT_AGENT_MAX_ITERATIONS = 111; +export const CHAT_AGENT_MAX_ITERATIONS = CHAT_AGENT_STEP_LIMIT;Also applies to: 16-29
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/chat/const.ts` at line 14, Update the iteration constants in lib/chat/const.ts so they share one numeric source of truth instead of independently hard-coding 111. Define the shared value once, derive CHAT_AGENT_MAX_ITERATIONS and CHAT_AGENT_STOP_WHEN from it, and preserve their equal-value behavioral parity.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@lib/chat/const.ts`:
- Line 14: Update the iteration constants in lib/chat/const.ts so they share one
numeric source of truth instead of independently hard-coding 111. Define the
shared value once, derive CHAT_AGENT_MAX_ITERATIONS and CHAT_AGENT_STOP_WHEN
from it, and preserve their equal-value behavioral parity.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 4a460823-f0b6-447a-bb9c-aade74c115b2
⛔ Files ignored due to path filters (3)
app/lib/workflows/__tests__/runAgentStep.test.tsis excluded by!**/*.test.*,!**/__tests__/**and included byapp/**app/lib/workflows/__tests__/runAgentWorkflow.test.tsis excluded by!**/*.test.*,!**/__tests__/**and included byapp/**app/lib/workflows/__tests__/runAgentWorkflowLoop.test.tsis excluded by!**/*.test.*,!**/__tests__/**and included byapp/**
📒 Files selected for processing (7)
app/lib/workflows/convertMessagesStep.tsapp/lib/workflows/runAgentStep.tsapp/lib/workflows/runAgentWorkflow.tsapp/lib/workflows/sendStreamFinish.tsapp/lib/workflows/sendStreamStart.tslib/agent/messageMetadata/buildMessageMetadataCallback.tslib/chat/const.ts
…ai@6) The first pass used `result.responseMessages`, which does not exist on StreamTextResult in ai@6.0.190 — it is an ai@7 accessor. `next build` caught it; local checks did not, because the dev node_modules had ai@7.0.2 installed against a package.json that pins 6.0.190. In 6.0.190 the equivalent is `(await result.response).messages`: the assistant message for this call plus any tool-result message, which is exactly what the next iteration needs appended. Verified against a clean `pnpm install --frozen-lockfile` (ai@6.0.190): the build's TypeScript step passes and it proceeds to page-data collection. Refs recoupable/chat#1918 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Local dev environments have
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
5 issues found and verified against the latest diff
Confidence score: 2/5
- In
app/lib/workflows/convertMessagesStep.ts, resumed turns with persisted or client-sent incomplete tool calls can throwAI_MessageConversionErrorbefore the first model step, so the workflow can fail to continue for affected conversations — passignoreIncompleteToolCallsduring conversion for resumed inputs. - In
app/lib/workflows/runAgentWorkflow.ts, conversion and initial stream-write now happen outside the cleanuptry/finally, so early failures can leave chats stuck in an active state and block future turns — move both pre-loop steps back inside the protected cleanup scope. - In
app/lib/workflows/runAgentWorkflow.ts, charging credits only whenresult?.responseMessageexists ties billing to the last loop iteration instead of accumulated multi-iteration usage, which can under/over-charge turns — base charging on the aggregated usage/cost state for the full run. - In
lib/agent/messageMetadata/buildMessageMetadataCallback.ts, the newseedbehavior can carry cumulative usage/cost/finish-reason totals into later contexts, andlib/chat/const.tsnow hardcodes111in two places, increasing drift risk for iteration-stop parity — reset/validate seeded totals per turn and derive one constant from the other.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="lib/agent/messageMetadata/buildMessageMetadataCallback.ts">
<violation number="1" location="lib/agent/messageMetadata/buildMessageMetadataCallback.ts:32">
P2: Custom agent: **Flag AI Slop and Fabricated Changes**
The new `seed` parameter in `buildMessageMetadataCallback` is the sole behavior change in this file, enabling cumulative usage/cost/finish-reason totals to carry across per-iteration closures. However, the existing unit-test file for this module was not updated in this PR, so the seeded accumulation path remains untested. Adding a regression test (e.g., passing a `seed` with known totals, processing a `finish-step`, and asserting the result equals seed + step values) would pin the under-reporting fix described in the PR.</violation>
</file>
<file name="app/lib/workflows/convertMessagesStep.ts">
<violation number="1" location="app/lib/workflows/convertMessagesStep.ts:17">
P1: A resumed workflow containing a persisted or client-sent incomplete tool call can fail before the first model step with `AI_MessageConversionError`, preventing the turn from continuing. Passing `ignoreIncompleteToolCalls: true` here matches the existing chat conversion path and drops unfinished tool input before rebuilding the model history.</violation>
</file>
<file name="lib/chat/const.ts">
<violation number="1" location="lib/chat/const.ts:28">
P3: The bound value 111 is now duplicated in this constants file across CHAT_AGENT_STOP_WHEN and the new CHAT_AGENT_MAX_ITERATIONS. Since the two are meant to stay in parity, hardcoding 111 twice lets them silently diverge when one is adjusted. Consolidate to a single constant, e.g. export const CHAT_AGENT_MAX_ITERATIONS = 111 and define CHAT_AGENT_STOP_WHEN = stepCountIs(CHAT_AGENT_MAX_ITERATIONS).</violation>
</file>
<file name="app/lib/workflows/runAgentWorkflow.ts">
<violation number="1" location="app/lib/workflows/runAgentWorkflow.ts:112">
P1: A conversion or initial stream-write failure can leave the chat permanently marked active because both new pre-loop steps run outside the cleanup `try/finally`. Keeping conversion and stream-start inside the protected region would preserve cleanup on these failures.</violation>
<violation number="2" location="app/lib/workflows/runAgentWorkflow.ts:180">
P2: The credit charge for this turn is gated on `result?.responseMessage`, which is now just the LAST loop iteration's message — but the usage/cost that matters is the accumulated content across all iterations (`pendingAssistantResponse` / `modelMessages`). If the terminal iteration finishes without producing a message (e.g. a `stop`/`length` finish with no streamed content, or an abort before `onStepFinish` fires), `handleChatCredits` is skipped entirely and the account is not billed for tokens consumed by earlier tool-call iterations that the provider did charge for. Consider tracking whether ANY iteration produced a responseMessage (or gating on the accumulated metadata) rather than checking only the terminal iteration, so the whole turn's consumption is always billed.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| export async function convertMessagesStep(messages: UIMessage[]): Promise<ModelMessage[]> { | ||
| "use step"; | ||
|
|
||
| return convertToModelMessages(messages); |
There was a problem hiding this comment.
P1: A resumed workflow containing a persisted or client-sent incomplete tool call can fail before the first model step with AI_MessageConversionError, preventing the turn from continuing. Passing ignoreIncompleteToolCalls: true here matches the existing chat conversion path and drops unfinished tool input before rebuilding the model history.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At app/lib/workflows/convertMessagesStep.ts, line 17:
<comment>A resumed workflow containing a persisted or client-sent incomplete tool call can fail before the first model step with `AI_MessageConversionError`, preventing the turn from continuing. Passing `ignoreIncompleteToolCalls: true` here matches the existing chat conversion path and drops unfinished tool input before rebuilding the model history.</comment>
<file context>
@@ -0,0 +1,18 @@
+export async function convertMessagesStep(messages: UIMessage[]): Promise<ModelMessage[]> {
+ "use step";
+
+ return convertToModelMessages(messages);
+}
</file context>
| // Convert once, before the loop. The workflow body owns this array and | ||
| // appends every iteration's `responseMessages` to it, which is how | ||
| // iteration N+1 sees iteration N's tool results. | ||
| const modelMessages = await convertMessagesStep(input.messages); |
There was a problem hiding this comment.
P1: A conversion or initial stream-write failure can leave the chat permanently marked active because both new pre-loop steps run outside the cleanup try/finally. Keeping conversion and stream-start inside the protected region would preserve cleanup on these failures.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At app/lib/workflows/runAgentWorkflow.ts, line 112:
<comment>A conversion or initial stream-write failure can leave the chat permanently marked active because both new pre-loop steps run outside the cleanup `try/finally`. Keeping conversion and stream-start inside the protected region would preserve cleanup on these failures.</comment>
<file context>
@@ -97,27 +106,79 @@ export async function runAgentWorkflow(input: RunAgentWorkflowInput): Promise<vo
+ // Convert once, before the loop. The workflow body owns this array and
+ // appends every iteration's `responseMessages` to it, which is how
+ // iteration N+1 sees iteration N's tool results.
+ const modelMessages = await convertMessagesStep(input.messages);
+
+ // The assistant message under construction. Threaded into each iteration
</file context>
| * this iteration's numbers and under-report the turn. Pass the in-progress | ||
| * assistant message's metadata to keep the totals cumulative. | ||
| */ | ||
| seed?: Pick<AgentMessageMetadata, "totalMessageUsage" | "totalMessageCost" | "stepFinishReasons">; |
There was a problem hiding this comment.
P2: Custom agent: Flag AI Slop and Fabricated Changes
The new seed parameter in buildMessageMetadataCallback is the sole behavior change in this file, enabling cumulative usage/cost/finish-reason totals to carry across per-iteration closures. However, the existing unit-test file for this module was not updated in this PR, so the seeded accumulation path remains untested. Adding a regression test (e.g., passing a seed with known totals, processing a finish-step, and asserting the result equals seed + step values) would pin the under-reporting fix described in the PR.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/agent/messageMetadata/buildMessageMetadataCallback.ts, line 32:
<comment>The new `seed` parameter in `buildMessageMetadataCallback` is the sole behavior change in this file, enabling cumulative usage/cost/finish-reason totals to carry across per-iteration closures. However, the existing unit-test file for this module was not updated in this PR, so the seeded accumulation path remains untested. Adding a regression test (e.g., passing a `seed` with known totals, processing a `finish-step`, and asserting the result equals seed + step values) would pin the under-reporting fix described in the PR.</comment>
<file context>
@@ -19,12 +19,23 @@ import type { AgentStepFinishMetadata } from "@/lib/agent/messageMetadata/AgentS
+ * this iteration's numbers and under-report the turn. Pass the in-progress
+ * assistant message's metadata to keep the totals cumulative.
+ */
+ seed?: Pick<AgentMessageMetadata, "totalMessageUsage" | "totalMessageCost" | "stepFinishReasons">;
+}) {
let lastStepUsage: LanguageModelUsage | undefined;
</file context>
| if (result.responseMessage) { | ||
| const metadata = result.responseMessage.metadata as AgentMessageMetadata | undefined; | ||
| // the turn ended. | ||
| if (result?.responseMessage) { |
There was a problem hiding this comment.
P2: The credit charge for this turn is gated on result?.responseMessage, which is now just the LAST loop iteration's message — but the usage/cost that matters is the accumulated content across all iterations (pendingAssistantResponse / modelMessages). If the terminal iteration finishes without producing a message (e.g. a stop/length finish with no streamed content, or an abort before onStepFinish fires), handleChatCredits is skipped entirely and the account is not billed for tokens consumed by earlier tool-call iterations that the provider did charge for. Consider tracking whether ANY iteration produced a responseMessage (or gating on the accumulated metadata) rather than checking only the terminal iteration, so the whole turn's consumption is always billed.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At app/lib/workflows/runAgentWorkflow.ts, line 180:
<comment>The credit charge for this turn is gated on `result?.responseMessage`, which is now just the LAST loop iteration's message — but the usage/cost that matters is the accumulated content across all iterations (`pendingAssistantResponse` / `modelMessages`). If the terminal iteration finishes without producing a message (e.g. a `stop`/`length` finish with no streamed content, or an abort before `onStepFinish` fires), `handleChatCredits` is skipped entirely and the account is not billed for tokens consumed by earlier tool-call iterations that the provider did charge for. Consider tracking whether ANY iteration produced a responseMessage (or gating on the accumulated metadata) rather than checking only the terminal iteration, so the whole turn's consumption is always billed.</comment>
<file context>
@@ -97,27 +106,79 @@ export async function runAgentWorkflow(input: RunAgentWorkflowInput): Promise<vo
- if (result.responseMessage) {
- const metadata = result.responseMessage.metadata as AgentMessageMetadata | undefined;
+ // the turn ended.
+ if (result?.responseMessage) {
+ const metadata = pendingAssistantResponse.metadata as AgentMessageMetadata | undefined;
await handleChatCredits({
</file context>
| * `CHAT_AGENT_STOP_WHEN` stays for the non-durable `/api/chat` route | ||
| * (`getGeneralAgent`), which still runs its tool loop inside `streamText`. | ||
| */ | ||
| export const CHAT_AGENT_MAX_ITERATIONS = 111; |
There was a problem hiding this comment.
P3: The bound value 111 is now duplicated in this constants file across CHAT_AGENT_STOP_WHEN and the new CHAT_AGENT_MAX_ITERATIONS. Since the two are meant to stay in parity, hardcoding 111 twice lets them silently diverge when one is adjusted. Consolidate to a single constant, e.g. export const CHAT_AGENT_MAX_ITERATIONS = 111 and define CHAT_AGENT_STOP_WHEN = stepCountIs(CHAT_AGENT_MAX_ITERATIONS).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/chat/const.ts, line 28:
<comment>The bound value 111 is now duplicated in this constants file across CHAT_AGENT_STOP_WHEN and the new CHAT_AGENT_MAX_ITERATIONS. Since the two are meant to stay in parity, hardcoding 111 twice lets them silently diverge when one is adjusted. Consolidate to a single constant, e.g. export const CHAT_AGENT_MAX_ITERATIONS = 111 and define CHAT_AGENT_STOP_WHEN = stepCountIs(CHAT_AGENT_MAX_ITERATIONS).</comment>
<file context>
@@ -13,6 +13,20 @@ export const MAX_MESSAGES = 55;
+ * `CHAT_AGENT_STOP_WHEN` stays for the non-durable `/api/chat` route
+ * (`getGeneralAgent`), which still runs its tool loop inside `streamText`.
+ */
+export const CHAT_AGENT_MAX_ITERATIONS = 111;
+
export const SYSTEM_PROMPT = `You are Recoup, a friendly, sharp, and strategic AI assistant for the music industry. You help music executives, artist teams, and self-starting artists analyze fan data, optimize marketing, and grow artist careers.
</file context>
There was a problem hiding this comment.
9 issues found across 10 files
Confidence score: 2/5
- In
app/lib/workflows/convertMessagesStep.ts,app/lib/workflows/sendStreamStart.ts, andapp/lib/workflows/runAgentWorkflow.ts, failures before entering the cleanuptry/finallycan leaveactive_stream_idclaimed and ephemeral keys/writables unreleased, which risks stuck chats until TTL fallback—move conversion/stream-start into guaranteed cleanup scope and preserve stream context through conversion errors. - In
app/lib/workflows/sendStreamFinish.ts, user-cancel cleanup can throw when the writable is already closed, so cancellation paths may fail noisily instead of finishing cleanup—treat already-closed/errored stream writes as idempotent no-ops. - In
lib/agent/messageMetadata/buildMessageMetadataCallback.ts, accepting client-seeded assistant metadata can makehandleChatCreditstrust a positive seededtotalMessageCost, underreporting usage and creating billing integrity risk—only seed trusted server-derived metadata and ignore client-supplied cost totals. app/lib/workflows/runAgentStep.tssize growth pluslib/chat/const.tsstale/duplicated constant documentation and missing seed regression coverage inlib/agent/messageMetadata/buildMessageMetadataCallback.tsincrease change fragility and future drift risk—splitrunAgentStep, update the JSDoc/constant coupling, and add the seed accumulation regression test.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="app/lib/workflows/sendStreamFinish.ts">
<violation number="1" location="app/lib/workflows/sendStreamFinish.ts:16">
P2: User-cancelled runs can fail during cleanup when workflow cancellation has already closed the writable, because this rejected finish write is propagated. Treat an already-closed/errored stream as a no-op here, consistent with `closeChatStream`'s defensive cleanup behavior.</violation>
</file>
<file name="lib/chat/const.ts">
<violation number="1" location="lib/chat/const.ts:28">
P3: The bound 111 is duplicated here and inside `CHAT_AGENT_STOP_WHEN = stepCountIs(111)` on the line above. The new comment says the two must stay in "behavioural parity", but nothing enforces it — bump one and the other silently diverges, letting the durable workflow's loop and the non-durable streamText stop condition drift apart. Derive `stepCountIs(...)` from the shared constant so they can't go out of sync: `export const CHAT_AGENT_MAX_ITERATIONS = 111; export const CHAT_AGENT_STOP_WHEN = stepCountIs(CHAT_AGENT_MAX_ITERATIONS);`.</violation>
<violation number="2" location="lib/chat/const.ts:28">
P2: Custom agent: **Flag AI Slop and Fabricated Changes**
The unchanged JSDoc above `CHAT_AGENT_STOP_WHEN` is now stale: it claims the constant is "Used by /api/chat/workflow (via runAgentStep)", but this PR removed `stopWhen` from `runAgentStep.ts` and replaced that path's limit with `CHAT_AGENT_MAX_ITERATIONS`. The old comment directly contradicts the new JSDoc added just below it and will mislead anyone reading this file about which route uses which constant. Please update the `CHAT_AGENT_STOP_WHEN` JSDoc to remove the workflow/runAgentStep reference so it only documents the non-durable `/api/chat` route.</violation>
</file>
<file name="app/lib/workflows/sendStreamStart.ts">
<violation number="1" location="app/lib/workflows/sendStreamStart.ts:21">
P2: A closed or cancelled client stream before the first chunk can leave the chat's `active_stream_id` set and skip writable cleanup because this rejection occurs outside the workflow's cleanup `try`. Keeping the initial stream write inside the same cleanup scope (or otherwise ensuring cleanup runs before propagating the error) prevents chats from remaining stuck as streaming.</violation>
</file>
<file name="app/lib/workflows/convertMessagesStep.ts">
<violation number="1" location="app/lib/workflows/convertMessagesStep.ts:17">
P1: Resuming a turn with an unfinished tool call can fail during conversion before cleanup runs, leaving the chat's `active_stream_id` claimed and headless ephemeral keys unreleased until their fallback TTL. Preserve the existing conversion behavior by enabling `ignoreIncompleteToolCalls` here.</violation>
</file>
<file name="lib/agent/messageMetadata/buildMessageMetadataCallback.ts">
<violation number="1" location="lib/agent/messageMetadata/buildMessageMetadataCallback.ts:32">
P2: Custom agent: **Flag AI Slop and Fabricated Changes**
The new `seed` option and its cumulative behavior across workflow iterations are not covered by any test. Adding a regression-style test that seeds a second callback with the metadata from a first callback and asserts cumulative `totalMessageUsage`, `totalMessageCost`, and `stepFinishReasons` would prevent silent regressions in the per-turn badge totals introduced by this PR.</violation>
<violation number="2" location="lib/agent/messageMetadata/buildMessageMetadataCallback.ts:37">
P1: Credit billing can be underreported because this new seed accepts metadata from client-supplied assistant messages and `handleChatCredits` treats a positive seeded `totalMessageCost` as authoritative. Seed only metadata loaded from trusted server persistence (or exclude client-provided assistant metadata from the billing seed) before carrying totals across iterations.</violation>
</file>
<file name="app/lib/workflows/runAgentWorkflow.ts">
<violation number="1" location="app/lib/workflows/runAgentWorkflow.ts:112">
P2: The message conversion and stream-start now run before the cleanup `try`/`finally` in `runAgentWorkflow`. `convertMessagesStep` performs real I/O (downloading file parts) and can throw; if it or `sendStreamStart` throws, the `finally` block that closes the client writable and clears `active_stream_id` never runs, so the client's stream hangs open (~2m until the runtime GCs it) and the chat's `active_stream_id` stays stale. Previously the whole turn sat inside the try, so any failure still reached cleanup. Consider moving the conversion and stream-start inside the try (right before the loop) so a conversion failure still tears down the stream cleanly.</violation>
</file>
<file name="app/lib/workflows/runAgentStep.ts">
<violation number="1" location="app/lib/workflows/runAgentStep.ts:25">
P1: Custom agent: **Enforce Clear Code Style and Maintainability Practices**
This file (`runAgentStep.ts`) is 272 lines long, nearly 3× the repository's custom 100-line file limit (Rule 3). The current diff adds roughly 30 new lines of JSDoc and logic to an already oversized file, further worsening the maintainability and single-responsibility violation. Since the PR already extracts small helpers like `sendStreamStart.ts` into standalone files, the same pattern should be applied here: split large concerns (type definitions, stream construction, message metadata building, cancellation/finalization logic) into separate modules so each file stays under the 100-line cap.</violation>
</file>
Architecture diagram
sequenceDiagram
participant Client as Client
participant WF as Agent Workflow (runAgentWorkflow)
participant Convert as convertMessagesStep
participant Start as sendStreamStart
participant Step as runAgentStep
participant Finish as sendStreamFinish
participant AI as AI SDK (streamText)
participant Store as Stream Store (Redis)
participant DB as Database
Note over Client,DB: Agent Loop - One Step Per LLM Call
Client->>WF: POST /api/chat/runs (messages, modelId, agentContext)
WF->>Convert: convertMessagesStep(messages)
Convert->>Convert: "use step" - convert UI→model messages
Convert-->>WF: modelMessages[]
WF->>Start: sendStreamStart(writable, assistantMessageId)
Start->>Store: write {type: "start", messageId}
Start-->>WF: done
loop per LLM call (up to CHAT_AGENT_MAX_ITERATIONS=111)
WF->>Step: runAgentStep(modelMessages, originalMessages, writable)
Step->>Store: acquire stream lock (per-step scope)
Step->>AI: streamText({model, system, messages, tools})
Note over AI: NO stopWhen - default isStepCount(1)
AI->>AI: ONE model call + tool executions
alt tool-calls finish reason
AI-->>Step: finishReason: "tool-calls"
Step->>AI: toUIMessageStream({sendStart:false, sendFinish:false})
Note over Step,AI: Suppress per-iteration start/finish chunks
AI->>Store: write chunks to shared writable (same Redis stream)
Store-->>AI: acknowledged
AI-->>Step: responseMessage (cumulative)
Step-->>WF: {finishReason: "tool-calls", responseMessages, responseMessage}
else stop/other finish reason
AI-->>Step: finishReason: "stop" or "length" etc.
Step->>AI: toUIMessageStream({sendStart:false, sendFinish:false})
AI-->>Step: responseMessage
Step-->>WF: {finishReason: "stop", responseMessages, responseMessage}
end
Step->>Store: release stream lock
alt user aborted
Step->>Step: abort streamText via AbortController
Step-->>WF: {aborted: true}
end
WF->>WF: append responseMessages to modelMessages
WF->>WF: update pendingAssistantResponse
alt aborted OR finishReason != "tool-calls"
WF->>WF: break loop
end
end
WF->>Finish: sendStreamFinish(writable)
Finish->>Store: write {type: "finish"}
Finish-->>WF: done
alt has responseMessage
WF->>DB: handleChatCredits (deduct credits)
alt not aborted
WF->>DB: autoCommitChatTurn (persist sandbox state)
end
end
WF-->>Client: workflow completes
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| export async function convertMessagesStep(messages: UIMessage[]): Promise<ModelMessage[]> { | ||
| "use step"; | ||
|
|
||
| return convertToModelMessages(messages); |
There was a problem hiding this comment.
P1: Resuming a turn with an unfinished tool call can fail during conversion before cleanup runs, leaving the chat's active_stream_id claimed and headless ephemeral keys unreleased until their fallback TTL. Preserve the existing conversion behavior by enabling ignoreIncompleteToolCalls here.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At app/lib/workflows/convertMessagesStep.ts, line 17:
<comment>Resuming a turn with an unfinished tool call can fail during conversion before cleanup runs, leaving the chat's `active_stream_id` claimed and headless ephemeral keys unreleased until their fallback TTL. Preserve the existing conversion behavior by enabling `ignoreIncompleteToolCalls` here.</comment>
<file context>
@@ -0,0 +1,18 @@
+export async function convertMessagesStep(messages: UIMessage[]): Promise<ModelMessage[]> {
+ "use step";
+
+ return convertToModelMessages(messages);
+}
</file context>
| let lastStepCost: number | undefined; | ||
| let totalMessageCost: number | undefined; | ||
| let stepFinishReasons: AgentStepFinishMetadata[] = []; | ||
| let totalMessageCost: number | undefined = opts.seed?.totalMessageCost; |
There was a problem hiding this comment.
P1: Credit billing can be underreported because this new seed accepts metadata from client-supplied assistant messages and handleChatCredits treats a positive seeded totalMessageCost as authoritative. Seed only metadata loaded from trusted server persistence (or exclude client-provided assistant metadata from the billing seed) before carrying totals across iterations.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/agent/messageMetadata/buildMessageMetadataCallback.ts, line 37:
<comment>Credit billing can be underreported because this new seed accepts metadata from client-supplied assistant messages and `handleChatCredits` treats a positive seeded `totalMessageCost` as authoritative. Seed only metadata loaded from trusted server persistence (or exclude client-provided assistant metadata from the billing seed) before carrying totals across iterations.</comment>
<file context>
@@ -19,12 +19,23 @@ import type { AgentStepFinishMetadata } from "@/lib/agent/messageMetadata/AgentS
let lastStepCost: number | undefined;
- let totalMessageCost: number | undefined;
- let stepFinishReasons: AgentStepFinishMetadata[] = [];
+ let totalMessageCost: number | undefined = opts.seed?.totalMessageCost;
+ let stepFinishReasons: AgentStepFinishMetadata[] = [...(opts.seed?.stepFinishReasons ?? [])];
</file context>
|
|
||
| export type RunAgentStepInput = { | ||
| messages: UIMessage[]; | ||
| /** |
There was a problem hiding this comment.
P1: Custom agent: Enforce Clear Code Style and Maintainability Practices
This file (runAgentStep.ts) is 272 lines long, nearly 3× the repository's custom 100-line file limit (Rule 3). The current diff adds roughly 30 new lines of JSDoc and logic to an already oversized file, further worsening the maintainability and single-responsibility violation. Since the PR already extracts small helpers like sendStreamStart.ts into standalone files, the same pattern should be applied here: split large concerns (type definitions, stream construction, message metadata building, cancellation/finalization logic) into separate modules so each file stays under the 100-line cap.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At app/lib/workflows/runAgentStep.ts, line 25:
<comment>This file (`runAgentStep.ts`) is 272 lines long, nearly 3× the repository's custom 100-line file limit (Rule 3). The current diff adds roughly 30 new lines of JSDoc and logic to an already oversized file, further worsening the maintainability and single-responsibility violation. Since the PR already extracts small helpers like `sendStreamStart.ts` into standalone files, the same pattern should be applied here: split large concerns (type definitions, stream construction, message metadata building, cancellation/finalization logic) into separate modules so each file stays under the 100-line cap.</comment>
<file context>
@@ -22,7 +22,19 @@ import { getWorkflowMetadata } from "workflow";
export type RunAgentStepInput = {
- messages: UIMessage[];
+ /**
+ * Conversation so far, in model form. Owned by `runAgentWorkflow`, which
+ * appends each iteration's `responseMessages` before the next call — that
</file context>
|
|
||
| const writer = writable.getWriter(); | ||
| try { | ||
| await writer.write({ type: "finish" }); |
There was a problem hiding this comment.
P2: User-cancelled runs can fail during cleanup when workflow cancellation has already closed the writable, because this rejected finish write is propagated. Treat an already-closed/errored stream as a no-op here, consistent with closeChatStream's defensive cleanup behavior.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At app/lib/workflows/sendStreamFinish.ts, line 16:
<comment>User-cancelled runs can fail during cleanup when workflow cancellation has already closed the writable, because this rejected finish write is propagated. Treat an already-closed/errored stream as a no-op here, consistent with `closeChatStream`'s defensive cleanup behavior.</comment>
<file context>
@@ -0,0 +1,20 @@
+
+ const writer = writable.getWriter();
+ try {
+ await writer.write({ type: "finish" });
+ } finally {
+ writer.releaseLock();
</file context>
|
|
||
| const writer = writable.getWriter(); | ||
| try { | ||
| await writer.write({ type: "start", messageId }); |
There was a problem hiding this comment.
P2: A closed or cancelled client stream before the first chunk can leave the chat's active_stream_id set and skip writable cleanup because this rejection occurs outside the workflow's cleanup try. Keeping the initial stream write inside the same cleanup scope (or otherwise ensuring cleanup runs before propagating the error) prevents chats from remaining stuck as streaming.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At app/lib/workflows/sendStreamStart.ts, line 21:
<comment>A closed or cancelled client stream before the first chunk can leave the chat's `active_stream_id` set and skip writable cleanup because this rejection occurs outside the workflow's cleanup `try`. Keeping the initial stream write inside the same cleanup scope (or otherwise ensuring cleanup runs before propagating the error) prevents chats from remaining stuck as streaming.</comment>
<file context>
@@ -0,0 +1,25 @@
+
+ const writer = writable.getWriter();
+ try {
+ await writer.write({ type: "start", messageId });
+ } finally {
+ writer.releaseLock();
</file context>
| // Convert once, before the loop. The workflow body owns this array and | ||
| // appends every iteration's `responseMessages` to it, which is how | ||
| // iteration N+1 sees iteration N's tool results. | ||
| const modelMessages = await convertMessagesStep(input.messages); |
There was a problem hiding this comment.
P2: The message conversion and stream-start now run before the cleanup try/finally in runAgentWorkflow. convertMessagesStep performs real I/O (downloading file parts) and can throw; if it or sendStreamStart throws, the finally block that closes the client writable and clears active_stream_id never runs, so the client's stream hangs open (~2m until the runtime GCs it) and the chat's active_stream_id stays stale. Previously the whole turn sat inside the try, so any failure still reached cleanup. Consider moving the conversion and stream-start inside the try (right before the loop) so a conversion failure still tears down the stream cleanly.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At app/lib/workflows/runAgentWorkflow.ts, line 112:
<comment>The message conversion and stream-start now run before the cleanup `try`/`finally` in `runAgentWorkflow`. `convertMessagesStep` performs real I/O (downloading file parts) and can throw; if it or `sendStreamStart` throws, the `finally` block that closes the client writable and clears `active_stream_id` never runs, so the client's stream hangs open (~2m until the runtime GCs it) and the chat's `active_stream_id` stays stale. Previously the whole turn sat inside the try, so any failure still reached cleanup. Consider moving the conversion and stream-start inside the try (right before the loop) so a conversion failure still tears down the stream cleanly.</comment>
<file context>
@@ -97,27 +106,79 @@ export async function runAgentWorkflow(input: RunAgentWorkflowInput): Promise<vo
+ // Convert once, before the loop. The workflow body owns this array and
+ // appends every iteration's `responseMessages` to it, which is how
+ // iteration N+1 sees iteration N's tool results.
+ const modelMessages = await convertMessagesStep(input.messages);
+
+ // The assistant message under construction. Threaded into each iteration
</file context>
| * `CHAT_AGENT_STOP_WHEN` stays for the non-durable `/api/chat` route | ||
| * (`getGeneralAgent`), which still runs its tool loop inside `streamText`. | ||
| */ | ||
| export const CHAT_AGENT_MAX_ITERATIONS = 111; |
There was a problem hiding this comment.
P2: Custom agent: Flag AI Slop and Fabricated Changes
The unchanged JSDoc above CHAT_AGENT_STOP_WHEN is now stale: it claims the constant is "Used by /api/chat/workflow (via runAgentStep)", but this PR removed stopWhen from runAgentStep.ts and replaced that path's limit with CHAT_AGENT_MAX_ITERATIONS. The old comment directly contradicts the new JSDoc added just below it and will mislead anyone reading this file about which route uses which constant. Please update the CHAT_AGENT_STOP_WHEN JSDoc to remove the workflow/runAgentStep reference so it only documents the non-durable /api/chat route.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/chat/const.ts, line 28:
<comment>The unchanged JSDoc above `CHAT_AGENT_STOP_WHEN` is now stale: it claims the constant is "Used by /api/chat/workflow (via runAgentStep)", but this PR removed `stopWhen` from `runAgentStep.ts` and replaced that path's limit with `CHAT_AGENT_MAX_ITERATIONS`. The old comment directly contradicts the new JSDoc added just below it and will mislead anyone reading this file about which route uses which constant. Please update the `CHAT_AGENT_STOP_WHEN` JSDoc to remove the workflow/runAgentStep reference so it only documents the non-durable `/api/chat` route.</comment>
<file context>
@@ -13,6 +13,20 @@ export const MAX_MESSAGES = 55;
+ * `CHAT_AGENT_STOP_WHEN` stays for the non-durable `/api/chat` route
+ * (`getGeneralAgent`), which still runs its tool loop inside `streamText`.
+ */
+export const CHAT_AGENT_MAX_ITERATIONS = 111;
+
export const SYSTEM_PROMPT = `You are Recoup, a friendly, sharp, and strategic AI assistant for the music industry. You help music executives, artist teams, and self-starting artists analyze fan data, optimize marketing, and grow artist careers.
</file context>
| * this iteration's numbers and under-report the turn. Pass the in-progress | ||
| * assistant message's metadata to keep the totals cumulative. | ||
| */ | ||
| seed?: Pick<AgentMessageMetadata, "totalMessageUsage" | "totalMessageCost" | "stepFinishReasons">; |
There was a problem hiding this comment.
P2: Custom agent: Flag AI Slop and Fabricated Changes
The new seed option and its cumulative behavior across workflow iterations are not covered by any test. Adding a regression-style test that seeds a second callback with the metadata from a first callback and asserts cumulative totalMessageUsage, totalMessageCost, and stepFinishReasons would prevent silent regressions in the per-turn badge totals introduced by this PR.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/agent/messageMetadata/buildMessageMetadataCallback.ts, line 32:
<comment>The new `seed` option and its cumulative behavior across workflow iterations are not covered by any test. Adding a regression-style test that seeds a second callback with the metadata from a first callback and asserts cumulative `totalMessageUsage`, `totalMessageCost`, and `stepFinishReasons` would prevent silent regressions in the per-turn badge totals introduced by this PR.</comment>
<file context>
@@ -19,12 +19,23 @@ import type { AgentStepFinishMetadata } from "@/lib/agent/messageMetadata/AgentS
+ * this iteration's numbers and under-report the turn. Pass the in-progress
+ * assistant message's metadata to keep the totals cumulative.
+ */
+ seed?: Pick<AgentMessageMetadata, "totalMessageUsage" | "totalMessageCost" | "stepFinishReasons">;
+}) {
let lastStepUsage: LanguageModelUsage | undefined;
</file context>
| * `CHAT_AGENT_STOP_WHEN` stays for the non-durable `/api/chat` route | ||
| * (`getGeneralAgent`), which still runs its tool loop inside `streamText`. | ||
| */ | ||
| export const CHAT_AGENT_MAX_ITERATIONS = 111; |
There was a problem hiding this comment.
P3: The bound 111 is duplicated here and inside CHAT_AGENT_STOP_WHEN = stepCountIs(111) on the line above. The new comment says the two must stay in "behavioural parity", but nothing enforces it — bump one and the other silently diverges, letting the durable workflow's loop and the non-durable streamText stop condition drift apart. Derive stepCountIs(...) from the shared constant so they can't go out of sync: export const CHAT_AGENT_MAX_ITERATIONS = 111; export const CHAT_AGENT_STOP_WHEN = stepCountIs(CHAT_AGENT_MAX_ITERATIONS);.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/chat/const.ts, line 28:
<comment>The bound 111 is duplicated here and inside `CHAT_AGENT_STOP_WHEN = stepCountIs(111)` on the line above. The new comment says the two must stay in "behavioural parity", but nothing enforces it — bump one and the other silently diverges, letting the durable workflow's loop and the non-durable streamText stop condition drift apart. Derive `stepCountIs(...)` from the shared constant so they can't go out of sync: `export const CHAT_AGENT_MAX_ITERATIONS = 111; export const CHAT_AGENT_STOP_WHEN = stepCountIs(CHAT_AGENT_MAX_ITERATIONS);`.</comment>
<file context>
@@ -13,6 +13,20 @@ export const MAX_MESSAGES = 55;
+ * `CHAT_AGENT_STOP_WHEN` stays for the non-durable `/api/chat` route
+ * (`getGeneralAgent`), which still runs its tool loop inside `streamText`.
+ */
+export const CHAT_AGENT_MAX_ITERATIONS = 111;
+
export const SYSTEM_PROMPT = `You are Recoup, a friendly, sharp, and strategic AI assistant for the music industry. You help music executives, artist teams, and self-starting artists analyze fan data, optimize marketing, and grow artist careers.
</file context>
Caught on the preview, not by the unit tests. A 13-iteration run persisted an assistant message with only 2 parts (step-start + text) — every tool call was gone from chat_messages. The outer createUIMessageStream is what assembles the message handed to onStepFinish/onFinish, and it was not given originalMessages. Per the ai@6 docs that field is what puts the stream in "persistence mode", so without it each iteration rebuilt the message from its own chunks alone and the final text-only persist overwrote every tool call earlier in the turn. Passing originalMessages to the inner toUIMessageStream was not enough. Adds a regression test asserting createUIMessageStream is in persistence mode, since this failure is invisible to a green unit suite. Refs recoupable/chat#1918 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Preview verification — passed, after catching one regression the unit suite could not seePreview Run 2 —
|
| Assertion | Result |
|---|---|
runAgentStep records |
13 (one per LLM call) |
Every record attempt: 1 |
✅ — no retries fired |
| Longest single iteration | 3.4 s against the 800 s ceiling (~235× headroom) |
sendStreamStart / sendStreamFinish |
1 / 1 across all 13 iterations |
| Any non-completed step | none — 22/22 completed |
chat_messages rows |
1 assistant row, not 13 |
| Parts on that row | 38 — 13 step-start, 13 text, 12 tool-bash |
| Threading | agent answered alpha, bravo, charlie, delta, echo, stating it read each from the file |
Before this PR the same work was one step of 11-25 minutes that blew the ceiling and was retried 4 times. It is now 13 journaled steps of 1.4-3.4 s each. A retry is now structurally unable to re-send an email, because no step lives long enough to be killed.
The regression the preview caught — and the unit tests did not
Run 1 (wrun_01KYZ2Y9FX3V7RE6DHTKSKA9BE, commit b81fa0f1) had 13 iterations all at attempt: 1 — the decomposition itself was correct — but persisted an assistant message with only 2 parts (step-start, text). Every one of the 12 tool calls was missing from the transcript.
Cause: the OUTER createUIMessageStream is what assembles the message handed to onStepFinish/onFinish, and it was not given originalMessages. Per the ai@6 docs that field is what puts the stream in "persistence mode". Passing it to the inner toUIMessageStream was not sufficient — so each iteration rebuilt the message from its own chunks alone, and the final text-only persist overwrote every earlier tool call.
Fixed in c14fd92b with a regression test asserting persistence mode, since a green unit suite is not evidence for this class of failure. Re-verified above: 2 parts → 38 parts.
Not covered
No email was sent by this run, so "exactly one email_send_log row" is not directly demonstrated. What is demonstrated is the mechanism that caused the duplicates: steps are now seconds long, so the kill-and-retry loop that produced 5 sends per run cannot trigger. A live scheduled task run is the remaining end-to-end confirmation.
There was a problem hiding this comment.
3 issues found across 10 files
Confidence score: 3/5
- In
app/lib/workflows/runAgentWorkflow.ts, failures before the new cleanup path can leaveactive_stream_idset and the client stream hanging, which risks orphaned state and stuck user sessions — move stream setup/teardown back under the existingtry/finallyso cleanup always runs. - In
app/lib/workflows/sendStreamFinish.ts,writer.write({ type: "finish" })can reject when cancellation has already closed the writable, so cancelled runs may surface as errors instead of ending cleanly — treat already-closed/errored writers as a non-fatal finish path. - In
app/lib/workflows/convertMessagesStep.ts, pre-loop conversion can fail on incomplete tool calls during interrupted/resumed turns, preventing execution from reachingrunAgentStepand causing avoidable run failures — align this path withignoreIncompleteToolCalls: truebehavior.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="app/lib/workflows/runAgentWorkflow.ts">
<violation number="1" location="app/lib/workflows/runAgentWorkflow.ts:112">
P2: A conversion or stream-start failure can leave `active_stream_id` stuck and the client stream open because cleanup starts only after these new awaits; placing setup inside the existing `try/finally` would preserve cleanup on every workflow failure.</violation>
</file>
<file name="app/lib/workflows/sendStreamFinish.ts">
<violation number="1" location="app/lib/workflows/sendStreamFinish.ts:16">
P2: User-cancelled runs can fail while emitting the terminal chunk because the writable may already be closed by workflow cancellation, causing `writer.write({ type: "finish" })` to reject. Treat an already-closed/errored stream as a best-effort finish (or skip the finish write for aborted runs) so cancellation does not turn into a failed workflow and potential retry.</violation>
</file>
<file name="app/lib/workflows/convertMessagesStep.ts">
<violation number="1" location="app/lib/workflows/convertMessagesStep.ts:17">
P2: Interrupted or resumed turns containing an incomplete tool call can fail during this pre-loop conversion instead of reaching `runAgentStep`, because this path omits the existing `ignoreIncompleteToolCalls: true` behavior. Carry that option into the workflow conversion (and keep it aligned with `setupChatRequest`).</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| // Convert once, before the loop. The workflow body owns this array and | ||
| // appends every iteration's `responseMessages` to it, which is how | ||
| // iteration N+1 sees iteration N's tool results. | ||
| const modelMessages = await convertMessagesStep(input.messages); |
There was a problem hiding this comment.
P2: A conversion or stream-start failure can leave active_stream_id stuck and the client stream open because cleanup starts only after these new awaits; placing setup inside the existing try/finally would preserve cleanup on every workflow failure.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At app/lib/workflows/runAgentWorkflow.ts, line 112:
<comment>A conversion or stream-start failure can leave `active_stream_id` stuck and the client stream open because cleanup starts only after these new awaits; placing setup inside the existing `try/finally` would preserve cleanup on every workflow failure.</comment>
<file context>
@@ -97,27 +106,79 @@ export async function runAgentWorkflow(input: RunAgentWorkflowInput): Promise<vo
+ // Convert once, before the loop. The workflow body owns this array and
+ // appends every iteration's `responseMessages` to it, which is how
+ // iteration N+1 sees iteration N's tool results.
+ const modelMessages = await convertMessagesStep(input.messages);
+
+ // The assistant message under construction. Threaded into each iteration
</file context>
|
|
||
| const writer = writable.getWriter(); | ||
| try { | ||
| await writer.write({ type: "finish" }); |
There was a problem hiding this comment.
P2: User-cancelled runs can fail while emitting the terminal chunk because the writable may already be closed by workflow cancellation, causing writer.write({ type: "finish" }) to reject. Treat an already-closed/errored stream as a best-effort finish (or skip the finish write for aborted runs) so cancellation does not turn into a failed workflow and potential retry.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At app/lib/workflows/sendStreamFinish.ts, line 16:
<comment>User-cancelled runs can fail while emitting the terminal chunk because the writable may already be closed by workflow cancellation, causing `writer.write({ type: "finish" })` to reject. Treat an already-closed/errored stream as a best-effort finish (or skip the finish write for aborted runs) so cancellation does not turn into a failed workflow and potential retry.</comment>
<file context>
@@ -0,0 +1,20 @@
+
+ const writer = writable.getWriter();
+ try {
+ await writer.write({ type: "finish" });
+ } finally {
+ writer.releaseLock();
</file context>
| export async function convertMessagesStep(messages: UIMessage[]): Promise<ModelMessage[]> { | ||
| "use step"; | ||
|
|
||
| return convertToModelMessages(messages); |
There was a problem hiding this comment.
P2: Interrupted or resumed turns containing an incomplete tool call can fail during this pre-loop conversion instead of reaching runAgentStep, because this path omits the existing ignoreIncompleteToolCalls: true behavior. Carry that option into the workflow conversion (and keep it aligned with setupChatRequest).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At app/lib/workflows/convertMessagesStep.ts, line 17:
<comment>Interrupted or resumed turns containing an incomplete tool call can fail during this pre-loop conversion instead of reaching `runAgentStep`, because this path omits the existing `ignoreIncompleteToolCalls: true` behavior. Carry that option into the workflow conversion (and keep it aligned with `setupChatRequest`).</comment>
<file context>
@@ -0,0 +1,18 @@
+export async function convertMessagesStep(messages: UIMessage[]): Promise<ModelMessage[]> {
+ "use step";
+
+ return convertToModelMessages(messages);
+}
</file context>
… the wrapper Aligns runAgentStep with vercel-labs/open-agents apps/web/app/workflows/chat.ts rather than keeping our own variant of it. The variant is what produced the transcript loss caught on the preview in c14fd92. - Drops the outer createUIMessageStream. Upstream iterates `result.toUIMessageStream({...})` directly and writes each part to the shared writable with getWriter/write/releaseLock. Our wrapper existed only to get onStepFinish for in-step persistence, which fires once per step now that a step is one model call — and it had to be put in "persistence mode" separately from the inner stream, which is exactly what was missed. - Moves persistence to the workflow body via persistAssistantMessageStep, mirroring upstream's persistAssistantMessage(chatId, pendingAssistantResponse). runAgentStep no longer takes chatId at all. - Replaces pipeWorkflowStreamWithStopDetection with upstream's isAbortError check around the for-await, plus isRunCancelled to preserve the one case upstream does not have: run.cancel() closes our writable, so a write can fail with an unrelated error before the poller notices. - finalizeAbortedAssistantMessage folded into the step as closeOpenToolCalls; the body does the persisting. Deleted as dead: pipeWorkflowStreamWithStopDetection, finalizeAbortedAssistantMessage. Full suite 4307 pass; build's TypeScript step passes. Refs recoupable/chat#1918 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/lib/workflows/persistAssistantMessageStep.ts`:
- Around line 17-25: Update persistAssistantMessage and
persistAssistantMessageStep so Supabase persistence failures are no longer
swallowed: propagate the write error or return an explicit failure status, and
ensure persistAssistantMessageStep causes the journaled workflow to fail before
stream completion or charging continues.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 23079a8f-febe-4385-b649-381802b18817
⛔ Files ignored due to path filters (6)
app/lib/workflows/__tests__/runAgentStep.test.tsis excluded by!**/*.test.*,!**/__tests__/**and included byapp/**app/lib/workflows/__tests__/runAgentStepStreaming.test.tsis excluded by!**/*.test.*,!**/__tests__/**and included byapp/**app/lib/workflows/__tests__/runAgentWorkflow.test.tsis excluded by!**/*.test.*,!**/__tests__/**and included byapp/**app/lib/workflows/__tests__/runAgentWorkflowLoop.test.tsis excluded by!**/*.test.*,!**/__tests__/**and included byapp/**lib/chat/__tests__/finalizeAbortedAssistantMessage.test.tsis excluded by!**/*.test.*,!**/__tests__/**and included bylib/**lib/chat/__tests__/pipeWorkflowStreamWithStopDetection.test.tsis excluded by!**/*.test.*,!**/__tests__/**and included bylib/**
📒 Files selected for processing (7)
app/lib/workflows/persistAssistantMessageStep.tsapp/lib/workflows/runAgentStep.tsapp/lib/workflows/runAgentWorkflow.tslib/chat/finalizeAbortedAssistantMessage.tslib/chat/isAbortError.tslib/chat/isRunCancelled.tslib/chat/pipeWorkflowStreamWithStopDetection.ts
💤 Files with no reviewable changes (2)
- lib/chat/pipeWorkflowStreamWithStopDetection.ts
- lib/chat/finalizeAbortedAssistantMessage.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- app/lib/workflows/runAgentWorkflow.ts
- app/lib/workflows/runAgentStep.ts
| * `persistAssistantMessage` swallows its own errors, so this never throws. | ||
| */ | ||
| export async function persistAssistantMessageStep( | ||
| chatId: string, | ||
| message: UIMessage, | ||
| ): Promise<void> { | ||
| "use step"; | ||
|
|
||
| await persistAssistantMessage(chatId, message); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
rg -n -C 12 'persistAssistantMessage' lib/chat/persistAssistantMessage.ts
rg -n -C 10 'persistAssistantMessageStep|persistAssistantMessage' app/lib/workflows --glob '*test*.ts'Repository: recoupable/api
Length of output: 17425
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
fd -a 'persistAssistantMessageStep\.(ts|tsx)$|runAgentWorkflow\.(ts|tsx)$|upsertChatMessage\.ts$|updateChat\.ts$' . | sed 's#^\./##'
printf '\n--- persistAssistantMessageStep ---\n'
cat -n app/lib/workflows/persistAssistantMessageStep.ts
printf '\n--- runAgentWorkflow relevant sections ---\n'
ast-grep outline app/lib/workflows/runAgentWorkflow.ts --view expanded || true
rg -n -C 6 'persistAssistantMessageStep|autoCommitChatTurn|closeChatStream|runAgentStep|sendStreamFinish' app/lib/workflows/runAgentWorkflow.ts
printf '\n--- helpers relevant sections ---\n'
rg -n -C 8 '(^async function (upsertChatMessage|updateChat)|function (upsertChatMessage|updateChat)|export async function (upsertChatMessage|updateChat)|export function (upsertChatMessage|updateChat))' app lib --glob '*.{ts,tsx}'Repository: recoupable/api
Length of output: 17654
Make persistence failures fail the journaled workflow step.
persistAssistantMessageStep awaits persistAssistantMessage, but persistAssistantMessage logs and swallows Supabase failures. When persistence fails, the workflow continues, sends stream finish, and still charges/auto-commits without making assistant message persistence observable. Make persistent write failures propagate or return an explicit failure status that the workflow handles.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/lib/workflows/persistAssistantMessageStep.ts` around lines 17 - 25,
Update persistAssistantMessage and persistAssistantMessageStep so Supabase
persistence failures are no longer swallowed: propagate the write error or
return an explicit failure status, and ensure persistAssistantMessageStep causes
the journaled workflow to fail before stream completion or charging continues.
Re-verified after aligning to upstream open-agents (
|
| Assertion | Result |
|---|---|
runAgentStep records |
13 |
Every record attempt: 1 |
✅ |
| Longest single iteration | 6.6 s vs the 800 s ceiling |
persistAssistantMessageStep |
13 — one per iteration, from the workflow body |
sendStreamStart / sendStreamFinish |
1 / 1 |
| Non-completed steps | none — 35/35 completed |
chat_messages rows |
1 assistant row |
| Parts on that row | 38 — 13 step-start, 13 text, 12 tool-bash |
| Threading | alpha, bravo, charlie, delta, echo, read from the files |
Identical outcome to the pre-refactor run, now with the wrapper gone and persistence in the workflow body.
What changed and why
The wrapper we had around the stream was our invention, not upstream's, and it is what dropped every tool call from the transcript two commits ago. Rather than keep patching it, this now follows vercel-labs/open-agents apps/web/app/workflows/chat.ts:
- Dropped
createUIMessageStream. Upstream iteratesresult.toUIMessageStream({...})directly and writes each part withgetWriter()/write/releaseLock(). Our wrapper existed only to exposeonStepFinishfor in-step persistence — which fires once per step now that a step is one model call — and it had to be put in "persistence mode" separately from the inner stream. Missing that was the bug. - Persistence moved to the workflow body via
persistAssistantMessageStep, mirroring upstream'spersistAssistantMessage(options.chatId, pendingAssistantResponse).runAgentStepno longer takeschatIdat all, and a test guards against re-coupling them. pipeWorkflowStreamWithStopDetectionreplaced by upstream'sisAbortErrorcheck around thefor await.
One deliberate addition upstream does not have
isRunCancelled. Our stop path (POST /api/chat/[chatId]/stop → run.cancel()) closes the run's writable, so a write can fail with an unrelated stream error before the cancellation poller notices — a case upstream's isAbortError alone does not cover, because their stop flow differs. The old pipeTo code handled it by checking run status, and dropping that check silently would have turned user-stops into failed workflows. It is scoped to the rethrow decision and treats a failed status read as "not cancelled", so genuine errors still surface.
Deleted as dead: pipeWorkflowStreamWithStopDetection, finalizeAbortedAssistantMessage (folded into the step as closeOpenToolCalls, with the body doing the persist).
Full suite 4,307 pass; build's TypeScript step passes; eslint clean.
There was a problem hiding this comment.
7 issues found across 18 files
Confidence score: 2/5
- In
lib/agent/messageMetadata/buildMessageMetadataCallback.ts, client-provided assistant metadata can be used as the billing seed, which could let forged usage/cost values influence account debits; this is the highest-risk path because it affects billing integrity — derive seed metadata only from trusted persisted/workflow state (or strip/validate client fields before seeding). - In
app/lib/workflows/runAgentWorkflow.ts, setup calls that mark streaming happen before the cleanuptry/finally, so early conversion/stream-write failures can leave chats stuck in a permanent streaming state; and inapp/lib/workflows/persistAssistantMessageStep.ts, swallowed Supabase write errors allow the workflow to continue as if persistence succeeded — move setup into the guarded region and propagate persistence failures so state and delivery stay consistent. - In
app/lib/workflows/convertMessagesStep.ts, resumed turns with tool results can fail or lose content because this path omits the tool set andignoreIncompleteToolCalls: trueused elsewhere, creating a concrete regression risk for resumed/tool-heavy conversations — pass the same per-run tools and conversion options as the existing chat flow. - In
app/lib/workflows/runAgentStep.ts, failures after partial model output currently drop the in-progress assistant content, and reduced test coverage inapp/lib/workflows/__tests__/runAgentStep.test.tsplus mock state leakage risk inapp/lib/workflows/__tests__/runAgentWorkflowLoop.test.tsmake this easier to miss — persist incremental streamed state and restore abort/cancellation coverage while isolating mocks with reset behavior.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="app/lib/workflows/persistAssistantMessageStep.ts">
<violation number="1" location="app/lib/workflows/persistAssistantMessageStep.ts:25">
P2: persistAssistantMessageStep awaits persistAssistantMessage, but that helper logs and swallows Supabase failures internally. If the write fails, this step still resolves successfully, so the workflow proceeds to send the stream finish event and to charge/auto-commit the turn even though the assistant message was never actually persisted. Consider propagating the failure (or returning an explicit success/failure status) so the workflow can react instead of silently treating a failed persist as a completed step.</violation>
</file>
<file name="app/lib/workflows/runAgentWorkflow.ts">
<violation number="1" location="app/lib/workflows/runAgentWorkflow.ts:113">
P1: A conversion or initial stream-write failure can leave the chat permanently marked as streaming because both new setup calls run before the cleanup `try/finally`; placing setup inside the guarded region would ensure `clearChatActiveStream` and `closeChatStream` still run.</violation>
</file>
<file name="app/lib/workflows/runAgentStep.ts">
<violation number="1" location="app/lib/workflows/runAgentStep.ts:209">
P2: A model call that fails after producing partial output drops that output instead of preserving the in-progress assistant message. Capturing the streamed message state incrementally (or persisting it before propagating the error) would retain partial replies and tool calls on crash.</violation>
</file>
<file name="lib/agent/messageMetadata/buildMessageMetadataCallback.ts">
<violation number="1" location="lib/agent/messageMetadata/buildMessageMetadataCallback.ts:35">
P1: Client-supplied assistant metadata can now become the billing seed, allowing forged usage/cost values to affect the account debit. Seed only metadata reconstructed from trusted persisted/workflow state, or strip/validate incoming assistant metadata before passing it here.</violation>
</file>
<file name="app/lib/workflows/convertMessagesStep.ts">
<violation number="1" location="app/lib/workflows/convertMessagesStep.ts:17">
P1: Resumed turns containing tool results can lose or fail conversion because this workflow omits the tool set and `ignoreIncompleteToolCalls: true` used by the existing chat path. Passing the same per-run tools and conversion options into this step would keep multi-turn tool conversations model-valid.</violation>
</file>
<file name="app/lib/workflows/__tests__/runAgentWorkflowLoop.test.ts">
<violation number="1" location="app/lib/workflows/__tests__/runAgentWorkflowLoop.test.ts:70">
P3: vi.clearAllMocks() leaves mock implementations in place, so the `mockResolvedValue` default that queueSteps() bakes in at the end of one test carries over into later tests. It works now only because every test re-establishes its own implementation; a future test that counts runAgentStep calls without setting one would silently inherit the stale `finishReason:"stop"` default and loop once, masking an over-iteration bug. Use vi.resetAllMocks() (which also clears implementations and once-queues) in beforeEach — generateAssistantMessageId/convertMessagesStep defaults are re-set right after, so nothing else changes.</violation>
</file>
<file name="app/lib/workflows/__tests__/runAgentStep.test.ts">
<violation number="1" location="app/lib/workflows/__tests__/runAgentStep.test.ts:379">
P2: Removing the `user-abort path` describe block drops coverage for runAgentStep's remaining abort internals — the poller firing via pollWorkflowCancellation, isRunCancelled fallback detection, and closeOpenToolCalls closing mid-tool-call parts — which are still live code in runAgentStep.ts. runAgentWorkflowLoop.test.ts mocks runAgentStep and runAgentStepStreaming.test.ts only exercises the stream-throws-AbortError case, so the closeOpenToolCalls-on-abort and cancelled-run-detection paths are no longer guarded. Consider retaining a focused test for these paths, since this abort handling is central to the duplicate-email/retry bug the PR addresses.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| // Convert once, before the loop. The workflow body owns this array and | ||
| // appends every iteration's `responseMessages` to it, which is how | ||
| // iteration N+1 sees iteration N's tool results. | ||
| const modelMessages = await convertMessagesStep(input.messages); |
There was a problem hiding this comment.
P1: A conversion or initial stream-write failure can leave the chat permanently marked as streaming because both new setup calls run before the cleanup try/finally; placing setup inside the guarded region would ensure clearChatActiveStream and closeChatStream still run.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At app/lib/workflows/runAgentWorkflow.ts, line 113:
<comment>A conversion or initial stream-write failure can leave the chat permanently marked as streaming because both new setup calls run before the cleanup `try/finally`; placing setup inside the guarded region would ensure `clearChatActiveStream` and `closeChatStream` still run.</comment>
<file context>
@@ -97,27 +107,84 @@ export async function runAgentWorkflow(input: RunAgentWorkflowInput): Promise<vo
+ // Convert once, before the loop. The workflow body owns this array and
+ // appends every iteration's `responseMessages` to it, which is how
+ // iteration N+1 sees iteration N's tool results.
+ const modelMessages = await convertMessagesStep(input.messages);
+
+ // The assistant message under construction. Threaded into each iteration
</file context>
| }) { | ||
| let lastStepUsage: LanguageModelUsage | undefined; | ||
| let totalMessageUsage: LanguageModelUsage | undefined; | ||
| let totalMessageUsage: LanguageModelUsage | undefined = opts.seed?.totalMessageUsage; |
There was a problem hiding this comment.
P1: Client-supplied assistant metadata can now become the billing seed, allowing forged usage/cost values to affect the account debit. Seed only metadata reconstructed from trusted persisted/workflow state, or strip/validate incoming assistant metadata before passing it here.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/agent/messageMetadata/buildMessageMetadataCallback.ts, line 35:
<comment>Client-supplied assistant metadata can now become the billing seed, allowing forged usage/cost values to affect the account debit. Seed only metadata reconstructed from trusted persisted/workflow state, or strip/validate incoming assistant metadata before passing it here.</comment>
<file context>
@@ -19,12 +19,23 @@ import type { AgentStepFinishMetadata } from "@/lib/agent/messageMetadata/AgentS
+}) {
let lastStepUsage: LanguageModelUsage | undefined;
- let totalMessageUsage: LanguageModelUsage | undefined;
+ let totalMessageUsage: LanguageModelUsage | undefined = opts.seed?.totalMessageUsage;
let lastStepCost: number | undefined;
- let totalMessageCost: number | undefined;
</file context>
| export async function convertMessagesStep(messages: UIMessage[]): Promise<ModelMessage[]> { | ||
| "use step"; | ||
|
|
||
| return convertToModelMessages(messages); |
There was a problem hiding this comment.
P1: Resumed turns containing tool results can lose or fail conversion because this workflow omits the tool set and ignoreIncompleteToolCalls: true used by the existing chat path. Passing the same per-run tools and conversion options into this step would keep multi-turn tool conversations model-valid.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At app/lib/workflows/convertMessagesStep.ts, line 17:
<comment>Resumed turns containing tool results can lose or fail conversion because this workflow omits the tool set and `ignoreIncompleteToolCalls: true` used by the existing chat path. Passing the same per-run tools and conversion options into this step would keep multi-turn tool conversations model-valid.</comment>
<file context>
@@ -0,0 +1,18 @@
+export async function convertMessagesStep(messages: UIMessage[]): Promise<ModelMessage[]> {
+ "use step";
+
+ return convertToModelMessages(messages);
+}
</file context>
| ): Promise<void> { | ||
| "use step"; | ||
|
|
||
| await persistAssistantMessage(chatId, message); |
There was a problem hiding this comment.
P2: persistAssistantMessageStep awaits persistAssistantMessage, but that helper logs and swallows Supabase failures internally. If the write fails, this step still resolves successfully, so the workflow proceeds to send the stream finish event and to charge/auto-commit the turn even though the assistant message was never actually persisted. Consider propagating the failure (or returning an explicit success/failure status) so the workflow can react instead of silently treating a failed persist as a completed step.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At app/lib/workflows/persistAssistantMessageStep.ts, line 25:
<comment>persistAssistantMessageStep awaits persistAssistantMessage, but that helper logs and swallows Supabase failures internally. If the write fails, this step still resolves successfully, so the workflow proceeds to send the stream finish event and to charge/auto-commit the turn even though the assistant message was never actually persisted. Consider propagating the failure (or returning an explicit success/failure status) so the workflow can react instead of silently treating a failed persist as a completed step.</comment>
<file context>
@@ -0,0 +1,26 @@
+): Promise<void> {
+ "use step";
+
+ await persistAssistantMessage(chatId, message);
+}
</file context>
| // would see one per iteration and render N assistant messages. | ||
| sendStart: false, | ||
| sendFinish: false, | ||
| onFinish: ({ responseMessage: finalMessage }) => { |
There was a problem hiding this comment.
P2: A model call that fails after producing partial output drops that output instead of preserving the in-progress assistant message. Capturing the streamed message state incrementally (or persisting it before propagating the error) would retain partial replies and tool calls on crash.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At app/lib/workflows/runAgentStep.ts, line 209:
<comment>A model call that fails after producing partial output drops that output instead of preserving the in-progress assistant message. Capturing the streamed message state incrementally (or persisting it before propagating the error) would retain partial replies and tool calls on crash.</comment>
<file context>
@@ -142,87 +154,113 @@ export async function runAgentStep(input: RunAgentStepInput): Promise<RunAgentSt
+ // would see one per iteration and render N assistant messages.
+ sendStart: false,
+ sendFinish: false,
+ onFinish: ({ responseMessage: finalMessage }) => {
+ responseMessage = finalMessage;
+ },
</file context>
| }); | ||
| }); | ||
|
|
||
| describe("user-abort path", () => { |
There was a problem hiding this comment.
P2: Removing the user-abort path describe block drops coverage for runAgentStep's remaining abort internals — the poller firing via pollWorkflowCancellation, isRunCancelled fallback detection, and closeOpenToolCalls closing mid-tool-call parts — which are still live code in runAgentStep.ts. runAgentWorkflowLoop.test.ts mocks runAgentStep and runAgentStepStreaming.test.ts only exercises the stream-throws-AbortError case, so the closeOpenToolCalls-on-abort and cancelled-run-detection paths are no longer guarded. Consider retaining a focused test for these paths, since this abort handling is central to the duplicate-email/retry bug the PR addresses.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At app/lib/workflows/__tests__/runAgentStep.test.ts, line 379:
<comment>Removing the `user-abort path` describe block drops coverage for runAgentStep's remaining abort internals — the poller firing via pollWorkflowCancellation, isRunCancelled fallback detection, and closeOpenToolCalls closing mid-tool-call parts — which are still live code in runAgentStep.ts. runAgentWorkflowLoop.test.ts mocks runAgentStep and runAgentStepStreaming.test.ts only exercises the stream-throws-AbortError case, so the closeOpenToolCalls-on-abort and cancelled-run-detection paths are no longer guarded. Consider retaining a focused test for these paths, since this abort handling is central to the duplicate-email/retry bug the PR addresses.</comment>
<file context>
@@ -1,24 +1,17 @@
import { describe, it, expect, vi, beforeEach } from "vitest";
-import { streamText, createUIMessageStream } from "ai";
+import { streamText } from "ai";
import { runAgentStep } from "@/app/lib/workflows/runAgentStep";
-import { persistAssistantMessage } from "@/lib/chat/persistAssistantMessage";
-import { pollWorkflowCancellation } from "@/lib/chat/pollWorkflowCancellation";
-import { getRun } from "workflow/api";
vi.mock("ai", async () => {
</file context>
| } | ||
|
|
||
| beforeEach(() => { | ||
| vi.clearAllMocks(); |
There was a problem hiding this comment.
P3: vi.clearAllMocks() leaves mock implementations in place, so the mockResolvedValue default that queueSteps() bakes in at the end of one test carries over into later tests. It works now only because every test re-establishes its own implementation; a future test that counts runAgentStep calls without setting one would silently inherit the stale finishReason:"stop" default and loop once, masking an over-iteration bug. Use vi.resetAllMocks() (which also clears implementations and once-queues) in beforeEach — generateAssistantMessageId/convertMessagesStep defaults are re-set right after, so nothing else changes.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At app/lib/workflows/__tests__/runAgentWorkflowLoop.test.ts, line 70:
<comment>vi.clearAllMocks() leaves mock implementations in place, so the `mockResolvedValue` default that queueSteps() bakes in at the end of one test carries over into later tests. It works now only because every test re-establishes its own implementation; a future test that counts runAgentStep calls without setting one would silently inherit the stale `finishReason:"stop"` default and loop once, masking an over-iteration bug. Use vi.resetAllMocks() (which also clears implementations and once-queues) in beforeEach — generateAssistantMessageId/convertMessagesStep defaults are re-set right after, so nothing else changes.</comment>
<file context>
@@ -0,0 +1,221 @@
+}
+
+beforeEach(() => {
+ vi.clearAllMocks();
+ vi.mocked(generateAssistantMessageId).mockResolvedValue("asst-loop-id");
+ vi.mocked(convertMessagesStep).mockResolvedValue([{ role: "user", content: "hi" }] as never);
</file context>
| vi.clearAllMocks(); | |
| vi.resetAllMocks(); |
Replayed Nena's actual task — the case that produced the duplicatesThe synthetic 12-tool-call test proved the mechanism but not the workload. This replays Getting the call right mattered. Result —
|
| Run status | completed, no error |
| Wall clock | 56.2 min |
runAgentStep iterations |
45 |
Every iteration attempt: 1 |
✅ — zero retries |
| Longest single iteration | 197.5 s against the 800 s ceiling |
| Top iterations | 197.5, 192.8, 165.3, 159.7, 125.5, 123.1, 120.2, 113.1 s |
| Time inside iterations | 50.5 min of the 56.2 |
persistAssistantMessageStep |
45 |
| Non-completed steps | none |
email_send_log rows |
1 |
A 56-minute run completed. On main that is structurally impossible: the single step would have hit 800 s at ~13 min, been killed, retried 4 times, and failed at ~72.5 min — having sent an email on each attempt. That is exactly the 45-of-100 failure signature, and exactly the 3-5 copies Method Music and Nena received.
The margin is real but not infinite: 197.5 s is ~4× under the ceiling, not the ~200× my synthetic test implied. A single unusually slow tool call could still breach 800 s and retry. That is why api#807 is required rather than defence-in-depth, as chat#1918 now states.
What this does NOT show, and why
The one email_send_log row is rejected, so this demonstrates one send attempt, not one delivery. Cause is my test setup, not the code: I redirected the recipient to sweetman+stamp@recoupable.com, which is not an account email for the key's account, so assertRecipientsAllowed correctly refused it (no card on file → own addresses only). Incidentally confirms that guard works.
For the duplicate-email defect the meaningful number is the attempt count: 1, not 5. End-to-end delivery still wants one live scheduled run after merge.
Four deliberate deviations from the production task, all to avoid touching a customer:
- Recipient →
sweetman+stamp@recoupable.com, so nothing could reachnenx.mgmt@gmail.com. - Dropped the
room_id: 6eb63090…tag, so the test could not write into Nena's chat. - API base → the preview host. The sandbox's ephemeral
recoup_sk_key is minted on the preview and production rejects it; an earlier replay (wrun_01KYZGY2QW8JRK6F0MVGQD573Y) got 401 on four endpoints and the agent honestly reported it could not proceed rather than fabricating a report. Worth knowing independently: the sandbox email path is untestable on preview unless the prompt targets the same deployment. - Ran under my own account, no
artistId— Nena's artist belongs to her account.
That earlier 401 run is itself useful data: 17 iterations, all attempt: 1, longest 68.6 s, completed in 5.3 min.
Bug confirmed in passing
The rejected row has account_id: null and chat_id: null — the under-reporting api#790 fixes. Attribution required matching on raw_body and timestamp.
End-to-end: Nena's task replayed with a real email deliveredCloses the gap left by the previous replay, where the send was Run
The delivered email matches the task specChecked against the prompt's requirements:
Not verified: that every application URL resolves. That is task-output quality, outside this PR. What this proves, and what it does notProves: the exact production workload that produced 3-5 duplicate emails now runs 55.7 minutes to completion, across 44 journaled steps, none retried, and delivers one correct email. On Does not prove that the prompt's wording fixed anything. Nena's prompt already says "enviar EXACTAMENTE UN (1) correo", and it said so on every day she received the wrong batch. The count is 1 here because no step lived long enough to be killed and redelivered, not because the instruction started working. This is the distinction chat#1918 records under "Fix the platform, not the prompts." Margin, stated plainly203.5 s against an 800 s ceiling is ~3.9× of headroom, not the ~200× my earlier synthetic test suggested. A single unusually slow tool call can still breach the ceiling and be retried, which re-sends. That is why api#807 is required rather than optional. Deviations from the production task
|
1 participant
Fixes the duplicate task emails and the failing scheduled runs in chat#1918 by giving the agent loop real step boundaries.
The bug, from the runtime
runAgentStepwrapped the entire agent loop in a single"use step"viastopWhen: CHAT_AGENT_STOP_WHEN(stepCountIs(111)), so one step ran 11-25 minutes. WDK deploys step handlers withmaxDuration: max, and on Pro that resolves to 800 s (Vercel duration limits) — not unlimited. Vercel terminated the invocation, the step queue redelivered it (retryAfterSeconds: 5), and each retry was a complete agent run that mailed the customer again.Step record on
wrun_01KYY8X6WKMHQ8DN5P8JQP2H4T:status: failed,attempt: 5, 09:01:31 → 10:13:55 = 72.4 min, error{"message": "Unknown error"}— no exception and no stack, which is what a platform kill looks like. Every other step in that run completed on attempt 1 in under 0.2 s.45 of the last 100 runs failed this way, 42 of them at 72.4-73.5 min (5 attempts × ~870 s). Completed runs sit at a 4.0 min median — interactive chat turns finish inside 800 s and were never affected. Full evidence in root-cause note v3.
The fix
One journaled step per LLM call, with the loop in the workflow body.
runAgentStep.tsstopWhen. The AI SDK default isisStepCount(1)(@default isStepCount(1)inai@6.0.190), so a step is now one model call plus that call's tool executions. TakesmodelMessages/originalMessages, returnsresponseMessages.runAgentWorkflow.tsresponseMessagesso iteration N+1 sees iteration N's tool results; bounded byCHAT_AGENT_MAX_ITERATIONS; breaks on abort or any finish reason other thantool-calls.sendStreamStart.ts/sendStreamFinish.tsconvertMessagesStep.tsbuildMessageMetadataCallback.tsseedso usage/cost totals span the whole turn instead of resetting per iteration.lib/chat/const.tsCHAT_AGENT_MAX_ITERATIONS = 111.The streaming objection, and why it does not block this
The concern was that per-iteration steps break streaming and per-turn persistence. They do not, and the reference implementation (
vercel-labs/open-agentsapps/web/app/workflows/chat.ts) shows the shape:sendStart: false, sendFinish: falsetotoUIMessageStream, so only the workflow body emits the envelope. Without this the client would render N assistant messages instead of one.writable. WDK streams are Redis-backed, and perfoundations/streaming.mdx: "Stream locks acquired in a step only apply within that step. This enables multiple writers to write to the same stream concurrently."originalMessagesthreads the in-progress assistant message into each iteration, soresponseMessagestays cumulative and each persist overwrites one row.pipeWorkflowStreamWithStopDetectionalready passedpreventClose: true, so the shared writable already survived a step returning — no change needed there.Two deliberate calls
CHAT_AGENT_STOP_WHENstays.getGeneralAgent(the non-durable/api/chatroute) still runs its tool loop insidestreamText. Only the workflow path stops using it.modelMessagesis passed as a snapshot ([...modelMessages]), not the live array. A durable step input must describe the conversation as it was at that call, unaffected by later appends. A test caught this.Tests
app/lib/workflows/__tests__/runAgentWorkflowLoop.test.ts(new, 5 tests) — written first, confirmed RED (4 failing), then GREEN:tool-calls, stops onstopresponseMessagesinto the next iteration'smodelMessagestool-callsCHAT_AGENT_MAX_ITERATIONSPlus 3 new cases in
runAgentStep.test.ts:stopWhenis absent (regression guard — if it creeps back the step regrows past 800 s and this bug returns),sendStart/sendFinisharefalse, andresponseMessagesis returned. The obsoleteprepareStepcacheControl test is replaced by one asserting cacheControl on the messages handed tostreamText.pnpm install --frozen-lockfile).pnpm build: TypeScript step passes; the local run then stops at page-data collection on env vars the worktree lacks (STRIPE_SK), which CI has. Vercel check is green.tsc --noEmit: 203 errors, of which exactly one touches a file in this PR —runAgentWorkflow.test.tsProperty 'sandbox' does not exist on type 'never', which is pre-existing onmain(line 315 there, 333 here after this diff). No new type errors.eslintclean on all touched files.Verification still owed before merge
Preview verification against a real run is not done yet and is the gate for this PR: start a long
POST /api/chat/runs, then confirm vianpx workflow inspect steps --runId=<id> --env previewthat everyrunAgentSteprecord isattempt: 1and under 800 s, that the chat renders one assistant message, and that exactly oneemail_send_logrow lands. Results will be posted as a comment here.Refs recoupable/chat#1918
🤖 Generated with Claude Code
Summary by cubic
Split the agent loop into one workflow step per LLM call to keep each step under Vercel’s 800 s limit and stop duplicate customer emails. Streaming now matches upstream open-agents and preserves tool-call parts across iterations. Fixes recoupable/chat#1918.
Bug Fixes
result.response.messages.Refactors
runAgentWorkflowwithCHAT_AGENT_MAX_ITERATIONS;runAgentStepruns one model call (nostopWhen).createUIMessageStream; writeresult.toUIMessageStream(...)parts directly to the shared writable withsendStart: falseandsendFinish: false.persistAssistantMessageStep;runAgentStepno longer takeschatId. SeededbuildMessageMetadataCallbackfromoriginalMessagesso usage/cost totals carry across iterations.convertMessagesStep,sendStreamStart,sendStreamFinish; removedpipeWorkflowStreamWithStopDetectionandfinalizeAbortedAssistantMessage; addedisAbortErrorandisRunCancelled.{ modelMessages, originalMessages }; step returnsresponseMessagesfor threading into the next iteration.Written for commit 8b6be5a. Summary will update on new commits.
Summary by CodeRabbit