feat(ai): assistant message row at stream start + status column (streaming|complete|interrupted)#2076
Merged
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR adds persisted ChangesStreaming message durability
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant ChatRoute
participant Lifecycle
participant MessageDB
Client->>ChatRoute: Start AI generation
ChatRoute->>MessageDB: Insert streaming placeholder
ChatRoute->>Lifecycle: Finish with abort state
Lifecycle-->>ChatRoute: Return buffered parts
ChatRoute->>MessageDB: Persist complete or interrupted status
MessageDB-->>Client: Load terminalized message
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bce267e62a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Terminal write: flips a 'streaming' placeholder row to 'complete'. Every caller of | ||
| // saveMessageToDatabase persists a finished message (execute-end, onFinish, ask_agent, | ||
| // consult, etc.) — never a mid-flight one — so this is unconditionally correct here. | ||
| status: 'complete', |
There was a problem hiding this comment.
When a user stops after any chunk has been buffered, the execute/onFinish persistence still goes through this helper, and this new conflict update always flips the placeholder to complete even though the route later calls lifecycle.finish(true) for aborted runs. The new status contract says interrupted rows are terminal partial output, but stopped partial replies will be stored as completed and consumers cannot tell they were cut off; the global helper has the same unconditional complete update. Please pass the aborted outcome into the message write or update the row to interrupted on stop.
Useful? React with 👍 / 👎.
Owner
Author
There was a problem hiding this comment.
Fixed in 2dac307. saveMessageToDatabase/saveGlobalAssistantMessageToDatabase now accept an explicit status param instead of always writing 'complete'. Both routes' execute-end and onFinish blocks compute aborted = agentRun?.terminalReason === 'aborted' || abortSignal.aborted and pass status: aborted ? 'interrupted' : 'complete'. New tests cover both the aborted-with-content and non-aborted cases; verified the assertions fail against the pre-fix code before committing.
| isActive: true, | ||
| userId: null, | ||
| sourceAgentId: null, | ||
| status: 'streaming', |
There was a problem hiding this comment.
This inserts a streaming message row for every non-preaborted stream, but the stop paths only terminalize ai_stream_sessions; if the user stops before any buffered part or responseMessage exists, no saveMessageToDatabase call ever touches this row. Because the new readers hide status='streaming' by default and edit/delete now return 409 for it, an empty stopped reply can remain an invisible/locked placeholder indefinitely. Please clear the placeholder or mark it interrupted from the abort/finish path; the global route has the same start-row issue.
Useful? React with 👍 / 👎.
Owner
Author
There was a problem hiding this comment.
Fixed in 2dac307, both routes. chat/route.ts's execute-end guard widened from bufferedParts.length > 0 to bufferedParts.length > 0 || aborted, so an aborted run with zero buffered parts still writes a (possibly empty-content) 'interrupted' row. global/[id]/messages/route.ts has no execute-end persist block at all (a pre-existing architectural difference from chat/route.ts, not introduced by this PR) — added a fallback branch in its onFinish for !responseMessage && aborted that persists using lifecycle.getBufferedParts(), mirroring chat route's execute-end pattern.
Also closed the rarer symmetric gap where createUIMessageStream itself throws before execute()/onFinish ever run: both routes' outer catch now does a best-effort terminal write (guarded by a new assistantMessagePersisted flag so it can never downgrade a row already settled by a successful execute-end/onFinish write).
New tests in both routes cover aborted-with-zero-content and the outer-catch case; verified each fails against the pre-fix code before committing.
2witstudios
added a commit
that referenced
this pull request
Jul 14, 2026
…lete Addresses two Codex review findings on #2076: 1. saveMessageToDatabase / saveGlobalAssistantMessageToDatabase's onConflictDoUpdate always set status:'complete', even for a run the user (or credit gate) stopped — contradicting the status contract ('interrupted' = terminal, real partial output). Both now accept an explicit status param; execute-end and onFinish in both chat routes pass 'interrupted' whenever agentRun.terminalReason === 'aborted' or the route's abortSignal is aborted. 2. If a stream was stopped before any buffered part or responseMessage existed, no saveMessageToDatabase call ever touched the placeholder row — since streaming rows are hidden from every reader by default and edit/delete now 409 on them, that left an invisible, permanently-locked ghost row. Fixed by writing a (possibly empty-content) 'interrupted' row whenever the run was aborted, regardless of buffered content: - chat/route.ts: execute-end's guard widened from `bufferedParts.length > 0` to `bufferedParts.length > 0 || aborted`. - global/[id]/messages/route.ts: onFinish gained a fallback branch for `!responseMessage && aborted`, using lifecycle.getBufferedParts() the same way chat/route.ts's execute-end block does (this route had no execute-end persist block at all — a pre-existing architectural difference from chat/route.ts, not something this fix introduces). Also closes the same failure class for the rarer case where createUIMessageStream itself throws before execute()/onFinish ever run: both routes' outer catch now does a best-effort terminal write (status:'interrupted'), guarded by a new assistantMessagePersisted flag so it can never downgrade a row a successful execute-end/onFinish write already settled. New tests in both routes' stream-socket-events.test.ts covering: aborted-with-content → interrupted, aborted-with-zero-content → still persisted as interrupted, non-aborted → complete (regression guard), and outer-catch cleanup. Verified each new assertion fails against the pre-fix code (temporarily reverted, confirmed RED, restored) before committing. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013QQTjjDoYto6GHcBBfWEnB
2witstudios
added a commit
that referenced
this pull request
Jul 14, 2026
…lete Addresses two Codex review findings on #2076: 1. saveMessageToDatabase / saveGlobalAssistantMessageToDatabase's onConflictDoUpdate always set status:'complete', even for a run the user (or credit gate) stopped — contradicting the status contract ('interrupted' = terminal, real partial output). Both now accept an explicit status param; execute-end and onFinish in both chat routes pass 'interrupted' whenever agentRun.terminalReason === 'aborted' or the route's abortSignal is aborted. 2. If a stream was stopped before any buffered part or responseMessage existed, no saveMessageToDatabase call ever touched the placeholder row — since streaming rows are hidden from every reader by default and edit/delete now 409 on them, that left an invisible, permanently-locked ghost row. Fixed by writing a (possibly empty-content) 'interrupted' row whenever the run was aborted, regardless of buffered content: - chat/route.ts: execute-end's guard widened from `bufferedParts.length > 0` to `bufferedParts.length > 0 || aborted`. - global/[id]/messages/route.ts: onFinish gained a fallback branch for `!responseMessage && aborted`, using lifecycle.getBufferedParts() the same way chat/route.ts's execute-end block does (this route had no execute-end persist block at all — a pre-existing architectural difference from chat/route.ts, not something this fix introduces). Also closes the same failure class for the rarer case where createUIMessageStream itself throws before execute()/onFinish ever run: both routes' outer catch now does a best-effort terminal write (status:'interrupted'), guarded by a new assistantMessagePersisted flag so it can never downgrade a row a successful execute-end/onFinish write already settled. New tests in both routes' stream-socket-events.test.ts covering: aborted-with-content → interrupted, aborted-with-zero-content → still persisted as interrupted, non-aborted → complete (regression guard), and outer-catch cleanup. Verified each new assertion fails against the pre-fix code (temporarily reverted, confirmed RED, restored) before committing. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013QQTjjDoYto6GHcBBfWEnB
2witstudios
force-pushed
the
pu/e2-message-status
branch
from
July 14, 2026 23:49
109e02c to
0f70674
Compare
…aming|complete|interrupted) Adds status text NOT NULL DEFAULT 'complete' (streaming|complete|interrupted) to chat_messages and messages, and inserts an empty assistant placeholder row the moment generation starts (inside startGenerationExclusive's run closure, same critical section as takeover+lifecycle-create per the PR 4 seam note). Reader inventory (~30 sites) updated to exclude 'streaming' placeholders: - Model-context/compaction loads: page + global history loads, chatMessageRepository (v1/completions, page-payload-service), ask_agent, consult route (+ fixed a pre-existing missing isActive filter on its fallback branch), page-read-tools, ask-user-resume's fetchById/fetchLastAssistant (both page + global adapters). - Previews: conversation-repository's raw-SQL CTEs (last-message preview + count). - Mutations: edit/delete of a streaming row now 409s (chat + global [messageId] routes). Undo: the undo-vs-in-flight-stream UX call (409 whole undo / allow+hidden write / allow+abort) is a product decision, left open — ai-undo-service now excludes streaming rows from its sweep entirely (preview count + both soft-delete updates) as the safe interim default, so undo can never soft-delete a live stream's row. - Converters: convertDbMessageToUIMessage / convertGlobalAssistantMessageToUIMessage (all branches, including a reconstruction-success branch that previously dropped extra fields entirely) now propagate status. - Read APIs: chat/messages, global/[id]/messages, page-agents conversation messages, and v1/conversations/[id] GET all gate streaming rows behind includeStreaming=1. Client dedup: mergeServerAndPending (shared by AiChatView and SidebarChatTab) now replaces a server-loaded streaming placeholder with the richer live stream version instead of losing to the empty DB row. - Exports: tenant-export's column lists and gdpr-export's collectUserMessages both carry status now. Terminal writes: saveMessageToDatabase / saveGlobalAssistantMessageToDatabase's onConflictDoUpdate sets status: 'complete' (execute-end + onFinish, matching the existing durable-persist / best-effort-refine split). 'interrupted' via abort/ materialization is PR 3's wiring, per the epic's own "contract fixed here" framing. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013QQTjjDoYto6GHcBBfWEnB
…lete Addresses two Codex review findings on #2076: 1. saveMessageToDatabase / saveGlobalAssistantMessageToDatabase's onConflictDoUpdate always set status:'complete', even for a run the user (or credit gate) stopped — contradicting the status contract ('interrupted' = terminal, real partial output). Both now accept an explicit status param; execute-end and onFinish in both chat routes pass 'interrupted' whenever agentRun.terminalReason === 'aborted' or the route's abortSignal is aborted. 2. If a stream was stopped before any buffered part or responseMessage existed, no saveMessageToDatabase call ever touched the placeholder row — since streaming rows are hidden from every reader by default and edit/delete now 409 on them, that left an invisible, permanently-locked ghost row. Fixed by writing a (possibly empty-content) 'interrupted' row whenever the run was aborted, regardless of buffered content: - chat/route.ts: execute-end's guard widened from `bufferedParts.length > 0` to `bufferedParts.length > 0 || aborted`. - global/[id]/messages/route.ts: onFinish gained a fallback branch for `!responseMessage && aborted`, using lifecycle.getBufferedParts() the same way chat/route.ts's execute-end block does (this route had no execute-end persist block at all — a pre-existing architectural difference from chat/route.ts, not something this fix introduces). Also closes the same failure class for the rarer case where createUIMessageStream itself throws before execute()/onFinish ever run: both routes' outer catch now does a best-effort terminal write (status:'interrupted'), guarded by a new assistantMessagePersisted flag so it can never downgrade a row a successful execute-end/onFinish write already settled. New tests in both routes' stream-socket-events.test.ts covering: aborted-with-content → interrupted, aborted-with-zero-content → still persisted as interrupted, non-aborted → complete (regression guard), and outer-catch cleanup. Verified each new assertion fails against the pre-fix code (temporarily reverted, confirmed RED, restored) before committing. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013QQTjjDoYto6GHcBBfWEnB
…At; dedup aborted check Self-review (8-angle) on the previous fix commit surfaced two real bugs and one worthwhile simplification, independently confirmed by 3 separate finder angles: 1. CRITICAL: the outer-catch cleanup wrote in the wrong order — it called lifecycle.finish(true) BEFORE reading lifecycle.getBufferedParts(). finish() deletes the multicast registry entry getBufferedParts() reads from, so the cleanup write always persisted EMPTY content, silently discarding any real partial content the fix's own comment claimed to preserve. Fixed in both routes by capturing getBufferedParts() before finish(). chat/route.ts has a SEPARATE inner catch (createUIMessageStream construction failure) that also calls finish() before rethrowing to the outer catch — that inner catch now captures the buffer too (bufferedPartsAtStreamError), and the outer catch prefers that capture when set, since a fresh call by the time it runs would already see the inner catch's cleared buffer. 2. The global route's new onFinish fallback branch (no responseMessage, but aborted) didn't bump conversations.lastMessageAt the way the sibling responseMessage branch does — a conversation whose only new activity was an interrupted, no-responseMessage assistant row wouldn't surface in a lastMessageAt-sorted conversation list. Fixed to match the sibling branch. 3. `agentRun?.terminalReason === 'aborted' || abortSignal.aborted` was duplicated 3x across the two routes (with a comment on one copy claiming it was "computed once and reused" — true within its own closure, false across the file). Extracted to `isRunAborted()` in run-agent-with-retry.ts, which already owns the terminalReason vocabulary. New tests: both routes now have a regression test that reproduces the real finish()-then-getBufferedParts() ordering (mocked to return [] only after finish fires) and asserts the persisted uiMessage.parts still contain the pre-crash content; a lastMessageAt bump assertion for the fallback branch (call-count delta across the onFinish boundary, to avoid a false-pass from other db.update calls elsewhere in the same request); and a unit test suite for isRunAborted. All three new/changed assertions verified RED against the prior code before committing. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013QQTjjDoYto6GHcBBfWEnB
master gained two new migrations (0208_cloudy_ozymandias, 0209_sprite_reclaim_triggers) since this branch was last rebased, colliding with this PR's own 0208 migration number. Rebased onto origin/master, resolved the migration collision by keeping master's 0208/0209 journal entries and dropping this PR's stale 0208 migration + snapshot, then regenerated via `bun run db:generate` — the status column migration is now 0210_next_mariko_yashida.sql, journal-clean on top of master's latest. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013QQTjjDoYto6GHcBBfWEnB
2witstudios
force-pushed
the
pu/e2-message-status
branch
from
July 15, 2026 00:06
0f70674 to
19ad496
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web/src/app/api/ai/chat/route.ts (1)
1481-1500: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winTerminalize non-aborted exhausted runs.
Both routes can leave an assistant placeholder permanently
streamingwhen retries exhaust without producing buffered parts or aresponseMessage.
apps/web/src/app/api/ai/chat/route.ts#L1481-L1500: persist when the run is exhausted, even with an empty buffer, usinginterrupted.apps/web/src/app/api/ai/global/[id]/messages/route.ts#L1369-L1397: extend the no-response branch to terminalize exhausted runs asinterrupted.🤖 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 `@apps/web/src/app/api/ai/chat/route.ts` around lines 1481 - 1500, Update the assistant persistence logic in apps/web/src/app/api/ai/chat/route.ts at lines 1481-1500 to persist exhausted runs even when bufferedParts is empty, marking them interrupted. In apps/web/src/app/api/ai/global/[id]/messages/route.ts at lines 1369-1397, extend the no-response branch to terminalize exhausted runs as interrupted, preserving existing behavior for runs that produce a response.
🤖 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
`@apps/web/src/app/api/ai/global/`[id]/messages/__tests__/stream-socket-events.test.ts:
- Around line 777-783: Update the onFinish handling exercised by
stream-socket-events.test.ts so a normal run with responseMessage undefined
still persists the assistant placeholder with buffered or empty content and
terminal status complete. Revise the test around
captured.createUIMessageStreamOptions.onFinish to expect an assistant save with
role assistant and complete status, while preserving the existing aborted
fallback behavior.
In `@apps/web/src/app/api/ai/global/`[id]/messages/route.ts:
- Around line 1152-1173: Add an execute-end terminal write for the global
assistant placeholder in the stream lifecycle flow around streamLifecycle,
ensuring it runs even when the client disconnects after generation. Mirror the
page route’s idempotent execute-end upsert, using lifecycle-buffered parts to
finalize serverAssistantMessageId and avoid leaving the row in streaming;
preserve the preAborted behavior.
---
Outside diff comments:
In `@apps/web/src/app/api/ai/chat/route.ts`:
- Around line 1481-1500: Update the assistant persistence logic in
apps/web/src/app/api/ai/chat/route.ts at lines 1481-1500 to persist exhausted
runs even when bufferedParts is empty, marking them interrupted. In
apps/web/src/app/api/ai/global/[id]/messages/route.ts at lines 1369-1397, extend
the no-response branch to terminalize exhausted runs as interrupted, preserving
existing behavior for runs that produce a response.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: f29e8bdf-bc20-43b4-8c7a-1e9cc79d740a
📒 Files selected for processing (47)
apps/web/src/app/api/ai/chat/__tests__/stream-socket-events.test.tsapps/web/src/app/api/ai/chat/messages/[messageId]/__tests__/route.test.tsapps/web/src/app/api/ai/chat/messages/[messageId]/route.tsapps/web/src/app/api/ai/chat/messages/__tests__/route.test.tsapps/web/src/app/api/ai/chat/messages/route.tsapps/web/src/app/api/ai/chat/route.tsapps/web/src/app/api/ai/global/[id]/messages/[messageId]/__tests__/route.test.tsapps/web/src/app/api/ai/global/[id]/messages/[messageId]/route.tsapps/web/src/app/api/ai/global/[id]/messages/__tests__/credit-gate.test.tsapps/web/src/app/api/ai/global/[id]/messages/__tests__/stream-socket-events.test.tsapps/web/src/app/api/ai/global/[id]/messages/route.tsapps/web/src/app/api/ai/page-agents/[agentId]/conversations/[conversationId]/messages/__tests__/route.test.tsapps/web/src/app/api/ai/page-agents/[agentId]/conversations/[conversationId]/messages/route.tsapps/web/src/app/api/ai/page-agents/consult/__tests__/conversation-continuity.test.tsapps/web/src/app/api/ai/page-agents/consult/__tests__/conversation-listing-index.test.tsapps/web/src/app/api/ai/page-agents/consult/__tests__/credit-gate.test.tsapps/web/src/app/api/ai/page-agents/consult/__tests__/mcp-scope-tool-filtering.test.tsapps/web/src/app/api/ai/page-agents/consult/__tests__/recent-history-ordering.test.tsapps/web/src/app/api/ai/page-agents/consult/__tests__/step-cap-parity.test.tsapps/web/src/app/api/ai/page-agents/consult/route.tsapps/web/src/app/api/v1/chat/completions/__tests__/route-backfill.test.tsapps/web/src/app/api/v1/chat/completions/__tests__/route.test.tsapps/web/src/app/api/v1/conversations/[id]/route.tsapps/web/src/app/api/v1/conversations/__tests__/route.test.tsapps/web/src/lib/ai/core/__tests__/ask-user-resume.test.tsapps/web/src/lib/ai/core/__tests__/run-agent-with-retry.test.tsapps/web/src/lib/ai/core/ask-user-resume.tsapps/web/src/lib/ai/core/message-utils.tsapps/web/src/lib/ai/core/run-agent-with-retry.tsapps/web/src/lib/ai/streams/__tests__/mergeServerAndPending.test.tsapps/web/src/lib/ai/streams/mergeServerAndPending.tsapps/web/src/lib/ai/tools/__tests__/page-read-tools.test.tsapps/web/src/lib/ai/tools/agent-communication-tools.tsapps/web/src/lib/ai/tools/page-read-tools.tsapps/web/src/lib/repositories/chat-message-repository.tsapps/web/src/lib/repositories/conversation-repository.tsapps/web/src/lib/repositories/global-conversation-repository.tsapps/web/src/services/api/__tests__/ai-undo-service.test.tsapps/web/src/services/api/ai-undo-service.tspackages/db/drizzle/0210_next_mariko_yashida.sqlpackages/db/drizzle/meta/0210_snapshot.jsonpackages/db/drizzle/meta/_journal.jsonpackages/db/src/schema/conversations.tspackages/db/src/schema/core.tspackages/lib/src/compliance/export/gdpr-export.tspackages/lib/src/services/page-payload-service.tsscripts/tenant-export.ts
…uck-streaming gaps Addresses two CodeRabbit findings: 1. Global route had no execute-end durable-persist block at all (only chat/route.ts did). If the client disconnected after generation (mobile backgrounding — the exact scenario #2065 just fixed client-side for this same route), onFinish might never fire and the placeholder stayed 'streaming' forever. Added an execute-end block to the global route mirroring chat/route.ts's: unconditional, using whatever lifecycle.getBufferedParts() has, status aborted ? 'interrupted' : 'complete', plus the same conversations.lastMessageAt bump. onFinish's own no-responseMessage branch is now a no-op (execute-end has already terminalized the row by the time it would run) — replaced the duplicate persist logic there with a debug log. 2. Both routes' execute-end persist was still gated on `bufferedParts.length > 0 || aborted`. A run that exhausted its retries without ever aborting or producing a responseMessage (sustained provider outage) fell through that guard AND onFinish's `if (responseMessage)` guard, leaving the placeholder stuck at 'streaming' forever — same failure class as the abort gap already fixed, just for a third path (clean exit, no content) neither prior fix covered. Both routes' execute-end write is now unconditional. Updated/added tests in both routes' stream-socket-events.test.ts for: non-aborted empty-buffer persists as 'complete' (chat), aborted-with-content persists via execute-end (global, migrated off the now-inert onFinish path), lastMessageAt bump via execute-end (global), and non-aborted-with-content persists as 'complete' via execute-end (global). Fixed global credit-gate.test.ts's lifecycle mock (missing getBufferedParts, now called unconditionally). Verified each new/changed assertion against the pre-fix code before committing. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013QQTjjDoYto6GHcBBfWEnB
- ask-user-resume.ts's persist closures never passed status, silently resetting an 'interrupted' row back to 'complete' on every ask_user merge/dismiss write. Both page and global adapters now preserve the fetched row's status. - onFinish in both routes wrote a phantom empty 'interrupted' row for pre-aborted streams: the AI SDK always calls onFinish with a non-null responseMessage (an empty shell) even when execute() wrote nothing, but the placeholder INSERT is deliberately skipped when preAborted. Guarded on lifecycle.preAborted. - Outer-catch cleanup in both routes could fabricate a phantom row for an exception thrown before `lifecycle` was ever assigned (inside startGenerationExclusive's callback, before the placeholder INSERT ran) — now requires `lifecycle` itself, not just !preAborted. - Chat route's credit-gate abort was invisible to runAgentWithRetry's classification: the combined AbortSignal.any([...]) was only passed to the nested streamText call, not to runAgentWithRetry's own abortSignal param, so a mid-stream credit exhaustion was misclassified and persisted as 'complete' instead of 'interrupted'. - search-tools.ts and discovery-service.ts read chat_messages/messages without excluding status='streaming', a gap in the PR's own reader inventory. All fixes verified RED->GREEN. Full typecheck+build clean.
Owner
Author
Round 4: self-review (8-angle, high-effort) — 5 more findings, all fixedRan a fresh 8-angle diff review (line-by-line, removed-behavior, cross-file tracer, reuse, simplification, efficiency, altitude, conventions) against the full PR diff after CI went green. Found 5 genuine correctness/coverage gaps beyond what earlier rounds caught, all fixed in
All verified RED→GREEN (new/updated tests for #1, #2, #3; #4 verified by source-level trace through the AI SDK's |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/web/src/app/api/ai/global/[id]/messages/__tests__/stream-socket-events.test.ts (1)
763-779: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStrengthen the
lastMessageAtassertion to check the actual update payload.
expect(vi.mocked(db.update).mock.calls.length).toBeGreaterThan(...)only proves somedb.updatecall fired, not that it targetedconversations.lastMessageAt. Any unrelated update in the same code path (e.g. the message status write) would make this pass without exercising the behavior the test name claims to guard.♻️ Suggested stronger assertion
- const updateCallsBeforeExecute = vi.mocked(db.update).mock.calls.length; await captured.createUIMessageStreamOptions.execute?.({ write: vi.fn() }); - expect(vi.mocked(db.update).mock.calls.length).toBeGreaterThan(updateCallsBeforeExecute); + const updateCalls = vi.mocked(db.update).mock.calls; + const conversationsUpdateCall = updateCalls.find((c) => c[0] === conversations); + expect(conversationsUpdateCall).toBeDefined();Adjust to however
set()payload is captured/mocked in this test harness so thelastMessageAtfield itself can be asserted.🤖 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 `@apps/web/src/app/api/ai/global/`[id]/messages/__tests__/stream-socket-events.test.ts around lines 763 - 779, Strengthen the assertion in the aborted buffered-content test around captured execute handling to inspect the db.update set payload, not merely the update-call count. Verify that the update targeting conversations includes a lastMessageAt field, while preserving the existing setup and execution flow.
🤖 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
`@apps/web/src/app/api/ai/global/`[id]/messages/__tests__/stream-socket-events.test.ts:
- Around line 763-779: Strengthen the assertion in the aborted buffered-content
test around captured execute handling to inspect the db.update set payload, not
merely the update-call count. Verify that the update targeting conversations
includes a lastMessageAt field, while preserving the existing setup and
execution flow.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 64e43db6-b9c1-48ad-8397-981ed1ac174c
📒 Files selected for processing (11)
apps/web/src/app/api/ai/chat/__tests__/stream-socket-events.test.tsapps/web/src/app/api/ai/chat/route.tsapps/web/src/app/api/ai/global/[id]/messages/__tests__/credit-gate.test.tsapps/web/src/app/api/ai/global/[id]/messages/__tests__/stream-socket-events.test.tsapps/web/src/app/api/ai/global/[id]/messages/route.tsapps/web/src/lib/ai/core/__tests__/ask-user-resume.test.tsapps/web/src/lib/ai/core/ask-user-resume.tsapps/web/src/lib/ai/tools/__tests__/search-tools.test.tsapps/web/src/lib/ai/tools/search-tools.tsapps/web/src/lib/memory/__tests__/discovery-service.test.tsapps/web/src/lib/memory/discovery-service.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- apps/web/src/app/api/ai/global/[id]/messages/tests/credit-gate.test.ts
- apps/web/src/app/api/ai/chat/route.ts
- apps/web/src/lib/ai/core/ask-user-resume.ts
- apps/web/src/app/api/ai/global/[id]/messages/route.ts
…ot just call count CodeRabbit round-4 review nitpick: the assertion only checked that db.update's call count increased, not that any of the new calls actually targeted conversations. Any unrelated update in the same code path would have made the test pass without exercising the lastMessageAt bump. Now asserts one of the new update calls targets the conversations table specifically. RED-verified by temporarily removing the lastMessageAt bump from apps/web/src/app/api/ai/global/[id]/messages/route.ts and confirming the assertion fails; restored and confirmed GREEN. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013QQTjjDoYto6GHcBBfWEnB
Owner
Author
|
Fixed in 127e49d. The assertion only checked that |
This was referenced Jul 15, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
PR 2 of the "Server Stream Durability & Rejoin" epic. Adds
status text NOT NULL DEFAULT 'complete'(streaming | complete | interrupted) tochat_messagesandmessages, and inserts an empty assistant placeholder row the moment generation starts, so history can show in-flight entries and pre-checkpoint process death doesn't silently lose the reply.The entire risk of this PR is the readers: every consumer of these two tables (~30 sites) was inventoried and updated to correctly handle a
streamingplaceholder (empty content, mid-flight, excluded from model context / previews / mutations by default) and aninterruptedrow (real partial content, terminal, included everywhere).Schema / insert
0208_motionless_thunderbolts.sql(additive, default-covered, generated viabun run db:generateonly).startGenerationExclusive'srunclosure in bothPOST /api/ai/chatandPOST /api/ai/global/[id]/messages— same critical section as takeover+lifecycle-create (PR 4's seam note). Skipped when pre-aborted.saveMessageToDatabase/saveGlobalAssistantMessageToDatabaseaccept an explicitstatusparam. execute-end and onFinish in both routes pass'interrupted'whenever the run was aborted (user Stop or credit gate),'complete'otherwise — including the zero-buffered-content abort case, and a best-effort outer-catch write ifcreateUIMessageStreamitself throws before either callback runs.'interrupted'via cross-instance abort/materialization (a stream this process didn't own) is PR 3's wiring — this PR fixes the contract and owns same-process abort, doesn't implement the cross-instance reaper.Readers updated (exclude
streamingby default)chatMessageRepository,ask_agent,consultroute — plus fixed a pre-existing missingisActivefilter on consult's fallback branch,page-read-tools,ask-user-resume's fetchers on both page and global adapters).conversation-repository's raw-SQL CTEs).convertDbMessageToUIMessage/convertGlobalAssistantMessageToUIMessage, all branches) now propagatestatus.chat/messages,global/[id]/messages, page-agent conversation messages,v1/conversations/[id]) gate streaming rows behindincludeStreaming=1(stale-tab rollout protection).mergeServerAndPending(shared byAiChatViewandSidebarChatTab) now replaces a server-loaded streaming placeholder with the live stream's richer content instead of losing to the empty DB row.tenant-export's column lists andgdpr-export'scollectUserMessagesboth carrystatus.Open product decision (explicitly not resolved here)
Undo vs. an in-flight stream: whether undo should 409 the whole undo, allow it with a hidden terminal write, or allow it + abort the stream. Left as a
blockedsub-task with evidence. The safe interim default is implemented and tested:ai-undo-serviceexcludesstatus='streaming'rows from its sweep entirely (preview count + both soft-delete updates), so undo can never soft-delete a live stream's row — everything else in range still gets undone normally.Review fixes (Codex)
'complete'.saveMessageToDatabase/saveGlobalAssistantMessageToDatabasealways wrotestatus: 'complete'on their terminal upsert, even for a run the user (or credit gate) stopped — contradicting the contract that'interrupted'rows are terminal-with-real-partial-content. Fixed: both now take an explicitstatus, and both routes computeaborted = agentRun?.terminalReason === 'aborted' || abortSignal.abortedat execute-end/onFinish to pick it.createUIMessageStreameven finished constructing) never calledsaveMessageToDatabaseat all — sincestreamingrows are hidden from every reader by default and edit/delete 409 on them, that left an invisible, permanently-locked ghost row (no reaper exists yet; PR 3 isn't merged). Fixed in both routes' execute-end/onFinish (write an empty-content'interrupted'row when aborted, regardless of buffered content) and in a new best-effort outer-catch cleanup (guarded so it can never downgrade a row already settled).lifecycle.getBufferedParts()after callinglifecycle.finish(), which deletes the buffergetBufferedParts()reads from — so the cleanup always persisted empty content, silently discarding any real partial content it claimed to preserve. Fixed by capturing the buffer beforefinish()in both routes (chat/route.ts has a separate inner catch that also callsfinish()first, so it captures the buffer too and hands it to the outer catch). Also deduplicated theagentRun?.terminalReason === 'aborted' || abortSignal.abortedcheck (previously copy-pasted 3x) intoisRunAborted()inrun-agent-with-retry.ts, and fixed a missingconversations.lastMessageAtbump on the aborted/no-responseMessage fallback branch.responseMessage(sustained provider outage) fell through both the execute-end buffered-content-or-aborted guard and onFinish'sif (responseMessage)guard — same stuck-placeholder failure class, a third path neither prior fix covered. (b) the global route had no execute-end durable-persist block at all (only chat/route.ts did) — if the client disconnected after generation (mobile backgrounding, the exact scenario PR fix(ai): deterministic stream recovery for global assistant on mobile #2065 just fixed client-side for this same route), onFinish might never fire and the row stayed'streaming'forever. Fixed by making both routes' execute-end write unconditional (chat/route.ts) and by adding an execute-end block to the global route mirroring chat/route.ts's exactly, including thelastMessageAtbump. onFinish's no-responseMessage branch is now a no-op in both routes — execute-end has already terminalized the row by the time onFinish would run.ask-user-resume.ts'spersistForclosures (both page and global adapters) never passedstatustosaveMessageToDatabase/saveGlobalAssistantMessageToDatabase, which default it to'complete'. A row correctly terminalized as'interrupted'(stopped mid-flight with a pendingask_usercall) silently flipped back to'complete'the next time the user answered or a new message dismissed it — erasing the "this reply was cut short" signal for exactly the rows most likely to carry a pending question. Both adapters now thread the fetched row's own status through.onFinishcould write a phantom row for a pre-aborted stream: the AI SDK always invokesonFinishwith a non-nullresponseMessage(an empty{parts: []}shell) even whenexecute()returned immediately without writing anything, but the placeholder INSERT is deliberately skipped whenpreAborted— so the upsert would INSERT a brand-new empty'interrupted'row for a request that never reached the model. Guarded on!lifecycle?.preAborted.lifecyclewas ever assigned (insidestartGenerationExclusive's callback — e.g.takeOverConversationStreamsor the placeholder INSERT itself failing). The guard checked!lifecycle?.preAborted, which is vacuously true whenlifecycleisundefined. Now requireslifecycleitself.creditAbortController) was combined into the nestedstreamTextcall'sabortSignalbut not intorunAgentWithRetry's ownabortSignalparam — soclassifyAttempt/isRunAbortednever saw it, and a run cut short by the credit gate mid-stream was misclassified and persisted as'complete'instead of'interrupted'(and could even retry against an already-exhausted balance). Fixed by combining the same signal at therunAgentWithRetrycall site.search-tools.ts's line-numbering subquery anddiscovery-service.ts's Global Assistant preference-extraction query both readchat_messages/messageswithout excludingstatus='streaming'— two sites the PR's own ~30-site reader inventory missed.execute-end should bump conversations.lastMessageAttest only asserted thatdb.update's call count increased, not that any of the new calls actually targetedconversations— an unrelateddb.updatecall in the same code path would have made the test pass without exercising the bump it claims to guard. Fixed by asserting one of the new calls targets theconversationstable specifically (same mocked module reference). RED-verified by temporarily removing thelastMessageAtbump and confirming the assertion fails.Test plan
bun run typecheckgreen across all 16 monorepo packages (includingweb:build)bun vitest rungreen on every touched file (154 tests across 9 files) plus the full apps/web + packages/lib sweep otherwise unaffected — the sweep's remaining failures are two pre-existing, documented.pu-worktree environment limitations unrelated to this diff (dual-React module resolution in component tests; integration tests requiring a real Postgres connection unavailable locally), neither of which touches any file this PR changes. CI'sci / Unit Tests(real Postgres) is green.includeStreaming=1gating,mergeServerAndPending's live-vs-placeholder dedup, the abort-status fix, the buffered-content-preserved-through-cleanup regression guard, the global route's execute-end coverage,isRunAborted's unit tests,ask-user-resume's status-preservation on merge/dismiss, and both routes' pre-aborted-phantom-row guards (onFinish + outer-catch) — every new/changed assertion verified RED→GREEN by temporarily reverting the corresponding fix and confirming failure before restoring it.🤖 Generated with Claude Code
https://claude.ai/code/session_013QQTjjDoYto6GHcBBfWEnB
Summary by CodeRabbit
includeStreaming=1opt-in to display streaming placeholders in message listings.